Skip to content

api/metadata input validation: length and hashes - #1451

Merged
jku merged 3 commits into
theupdateframework:developfrom
sechkova:hash-validation
Jun 22, 2021
Merged

api/metadata input validation: length and hashes#1451
jku merged 3 commits into
theupdateframework:developfrom
sechkova:hash-validation

Conversation

@sechkova

@sechkovasechkova commented Jun 16, 2021

Copy link
Copy Markdown
Contributor

Fixes#1441

Description of the changes being introduced by the pull request:

Draft since it is dependent on #1437

Adds input validation checks to MetaFile and TargetFile common fields:

  • valid length: greater than zero
  • valid hashes: a non-empty dictionary

Checking the validity of hash algorithms is not part of the metadata input validation and is done
by securesystemslib during hash verification.

Please verify and check that the pull request fulfills the following
requirements:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@sechkovasechkova changed the title Hash validationapi/metadata input validation: length and hashesJun 16, 2021
@sechkovasechkova mentioned this pull request Jun 16, 2021
Closed
MVrachev
MVrachev reviewed Jun 16, 2021
View reviewed changes
Comment threadtuf/api/metadata.py
unrecognized_fields: Optional[Mapping[str, Any]] = None,
) -> None:

if version <= 0:

@MVrachevMVrachevJun 16, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these days I had a thought that it's probably easier to read if we use if version < 1 when doing this check.
What do you think?

@sechkovasechkovaJun 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find <= 0 very readable

@MVrachev

MVrachev commented Jun 16, 2021

Copy link
Copy Markdown
Collaborator

Even though we don't want to focus on validating the semantic behind hashes probably it's good to validate that
the keys and values in this dictionaries are strings.
If they aren't how are we going to compare the hashes against what we expect?
We did a similar choice for #1449 where we don't validate the semantics, but we make sure the types are as expected
and we can do the required comparisons.

@jkujku mentioned this pull request Jun 16, 2021
Merged
3 tasks
@jku

jku commented Jun 17, 2021•
edited
Loading

Copy link
Copy Markdown
Member

... probably it's good to validate that the keys and values in this dictionaries are strings.

I've started to agree with type checks in this situation:

  • we explicitly do not want to check the semantics of the keys (algorithms) and checking value content (hashes) is a bit pointless
  • this means the keys/values could be any json object after deserialization...
  • which should not be an issue for our code (sslib will raise in _verify_hashes() and we will handle that in MetaFile/TargetFile: ensure securesystemslib errors are all handled #1452), but ...
  • we will happily serialize the invalid metadata, if asked to

The last point feels quite wrong so maybe we should check that they are str at object intialization

@sechkova
sechkova marked this pull request as ready for review June 17, 2021 10:23
@sechkova

sechkova commented Jun 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Rebased on develop and added type checks for the dictionary key-values based on your suggestions.

jku
jku previously approved these changes Jun 17, 2021
View reviewed changes

@jkujku left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks fine to me. Left a comment about a check that isn't strictly necessary anymore but if you want to keep it, that's fine too.

Comment threadtuf/api/metadata.py Outdated
Comment on lines +707 to +708
if not isinstance(hashes, dict):
raise TypeError(f"Hashes must be a dictionary, got {type(hashes)}")

@jkujkuJun 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check could now be dropped -- the error we get from calling hashes.items() on something that isn't a dict is not as good but it does raise an exception without extra code...

@sechkovasechkovaJun 17, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped it and force pushed, we raise build-in errors anyway.

@sechkovasechkova mentioned this pull request Jun 17, 2021
Merged
3 tasks
@sechkova
Add hash and length validation
03f39b0
- valid length: greater than zero
- valid hashes: a non-empty dictionary of type Dict[str, str]
Checking the validity of hash algorithms is not part
of the metadata input validation and is done by
securesystemslib during hash verification.
Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Remove empty hash dict check from MetaFile
e30faa8
The check for an empty hash dictionary is now part
of the hash validation function.
Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
@sechkova
Remove trailing comma from test data
328f637
A trailing comma makes any element a one-item tuple.
Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
jku
jku reviewed Jun 21, 2021
View reviewed changes
Comment threadtuf/api/metadata.py
@jku
jku dismissed their stale reviewJune 21, 2021 07:35

oops, this was too hasty

jku
jku approved these changes Jun 21, 2021
View reviewed changes
MVrachev
MVrachev approved these changes Jun 21, 2021
View reviewed changes

@MVrachevMVrachev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jku
jku merged commit cfc7638 into theupdateframework:developJun 22, 2021
@MVrachevMVrachev mentioned this pull request Jun 22, 2021
Closed
@MVrachevMVrachev mentioned this pull request Nov 17, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkujkujku approved these changes
+1 more reviewer
@MVrachevMVrachevMVrachev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

api/metadata input validation: hashes

3 participants

@sechkova@MVrachev@jku