Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

api/metadata input validation: length and hashes #1451

Merged
merged 3 commits into from
Jun 22, 2021
Merged

api/metadata input validation: length and hashes #1451

merged 3 commits into from
Jun 22, 2021

Conversation

sechkova
Copy link
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

@sechkova sechkova changed the title Hash validation api/metadata input validation: length and hashes Jun 16, 2021
@sechkova sechkova mentioned this pull request Jun 16, 2021
Closed
MVrachev
MVrachev reviewed Jun 16, 2021
View reviewed changes
tuf/api/metadata.py
@@ -708,6 +720,14 @@ def __init__(
hashes: Optional[Dict[str, str]] = None,
unrecognized_fields: Optional[Mapping[str, Any]] = None,
) -> None:

if version <= 0:
Copy link
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?

Copy link
Contributor Author

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
Copy link
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.

@jku jku mentioned this pull request Jun 16, 2021
Merged
3 tasks
@jku
Copy link
Member

jku commented Jun 17, 2021 •
edited
Loading

... 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
Copy link
Contributor Author

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
Copy link
Member

@jku jku left a comment

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.

tuf/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)}")
Copy link
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...

Copy link
Contributor Author

@sechkova sechkova Jun 17, 2021 •
edited
Loading

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.

@sechkova sechkova 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 dismissed their stale review June 21, 2021 07:35

oops, this was too hasty

MVrachev
MVrachev approved these changes Jun 21, 2021
View reviewed changes
Copy link
Collaborator

@MVrachev MVrachev left a comment

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:develop Jun 22, 2021
@MVrachev MVrachev mentioned this pull request Jun 22, 2021
Closed
@MVrachev MVrachev mentioned this pull request Nov 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jku jku jku approved these changes

@MVrachev MVrachev MVrachev approved these changes

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