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

New API: Add Key and Role classes #1360

Merged
merged 7 commits into from
Apr 29, 2021
Merged

New API: Add Key and Role classes #1360

merged 7 commits into from
Apr 29, 2021

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Apr 21, 2021 •
edited
Loading

Addresses, but doesn't fix: #1139

Description of the changes being introduced by the pull request:

In the top level metadata classes, there are complex attributes such as
meta in Targets and Snapshot, key and roles in Root etc.
We want to represent those complex attributes with a class to allow
easier verification and support for metadata with unrecognized fields.
For more context read ADR 0004 and ADR 0008 in the docs/adr folder.

The changes in this pr include:

  • addition of a new Key class integrated into Root
  • addition of a new Roles class integrated into
  • added support for unrecognized fields
  • added tests for unrecognized fields in Key and Roles

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

@MVrachev MVrachev changed the title Key role classes New API: Add Key and Role classes Apr 21, 2021
@MVrachev MVrachev mentioned this pull request Apr 21, 2021
Merged
3 tasks
@MVrachev
Copy link
Collaborator Author

There is an interesting reason for the CI failure:
/home/runner/work/tuf/tuf/tuf/api/metadata.py:532:4: R0914: Too many local variables (18/15) (too-many-locals)

I used many local variables to make it clear what their purpose is.
Any suggestions? Should I disable this warning with an inline comment or?

@mnm678
Copy link
Contributor

mnm678 commented Apr 21, 2021

There is an interesting reason for the CI failure:
/home/runner/work/tuf/tuf/tuf/api/metadata.py:532:4: R0914: Too many local variables (18/15) (too-many-locals)

I used many local variables to make it clear what their purpose is.
Any suggestions? Should I disable this warning with an inline comment or?

This is interesting. I think local variables generally make code more readable, so I'm in favor of disabling this warning.

@jku
Copy link
Member

jku commented Apr 22, 2021

I used many local variables to make it clear what their purpose is.
Any suggestions? Should I disable this warning with an inline comment or?

I don't think the function looks horrible but ... you could make Role.from_dict() and Key.from_dict() -- this would fit the existing style and would probably remove half the variables.

jku
jku reviewed Apr 22, 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.

Thanks Martin. I have one potential bug (the keyids type issue which I'm not yet sure how to handle), the rest are just nits/suggestions/questions

tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Show resolved Hide resolved
@jku
Copy link
Member

jku commented Apr 22, 2021

There are currently no Key/Role specific tests: We probably should have at least very basic ones if we now have objects?

One thing that might be nice to check for is invalid data -- what does the failure look like if the metadata is not valid (say a missing required field). I don't think this needs to be exhaustively tested but would be nice to see at least an example: I assume with this code we mostly get KeyErrors?

@MVrachev MVrachev mentioned this pull request Apr 22, 2021
Merged
3 tasks
jku
jku requested changes Apr 26, 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.

Just one real issue: the keyids uniqueness checks and creation of the set still have a smell around them: I think the error isn't currently raised (when using from_dict()).

tests/test_api.py
Comment on lines +427 to +438
# Add unrecognized fields to all metadata sub (helper) classes.
if metadata == "root":
for keyid in dict1["signed"]["keys"].keys():
dict1["signed"]["keys"][keyid]["d"] = "c"
for role_str in dict1["signed"]["roles"].keys():
dict1["signed"]["roles"][role_str]["e"] = "g"
Copy link
Member

Choose a reason for hiding this comment

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

this looks like it will become hard to maintain when more tests are added but I'm fine with it for now: can you make an issue about doing this in a more structured way (like automate the dictionary poisoning so that ever dictionary is injected, not just ones we list here)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, you are right. We should automate this.
In a discussion, you showed a prototype automating this process.
Will appreciate it if you propose these changes and we can discuss them, but I don't think this is in the scope of this pr.

tuf/api/metadata.py Outdated Show resolved Hide resolved
@MVrachev
Copy link
Collaborator Author

MVrachev commented Apr 27, 2021 •
edited
Loading

Updated the pr by:

  • changing the type of keyds to a list inside the Role.__init__
  • don't make create a set in Role.from_dict and left the Role.__init__ to make that check
  • added a unit test case for creating a Role instance with duplicates in keyid.

@MVrachev
Add Key class and integrate it into Root
11e0a87 
In the top level metadata classes, there are complex attributes such as
"meta" in Targets and Snapshot, "key" and "roles" in Root etc.
We want to represent those complex attributes with a class to allow
easier verification and support for metadata with unrecognized fields.
For more context read ADR 0004 and ADR 0008 in the docs/adr folder.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Add a Role class and integrate it into Root
eab8c31 
In the top level metadata classes, there are complex attributes such as
"meta" in Targets and Snapshot, "key" and "roles" in Root etc.
We want to represent those complex attributes with a class to allow
easier verification and support for metadata with unrecognized fields.
For more context read ADR 0004 and ADR 0008 in the docs/adr folder.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Add from_dict methods in Key and Role classes
ef71c2d 
The from_dict() method simplifies the object creation.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Copy link
Collaborator Author

Rebased after #1363 was merged.

@MVrachev
Make keyids in Role a set
0c3131b 
From the specification:
"Clients MUST ensure that for any KEYID represented in this key list
and in other files, only one unique key has that KEYID."

The “only one unique key has that KEYID” is a requirement which can’t
be achieved if two keyids are the same.
So, in order to mandate that requirement it makes sense to use a set
which will guarantee us the keyid’s uniqueness.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
Check the format of keyval in Root
0c9dc0e 
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
New API: Add Key/Role specific tests
54a535e 
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
jku
jku approved these changes Apr 27, 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.

I think this looks good. I did notice one unneeded "if" that I had missed earlier (adding duplicates to a set is safe).

tuf/api/metadata.py Outdated
Comment on lines 580 to 581
if keyid not in self.roles[role].keyids:
self.roles[role].keyids.add(keyid)
Copy link
Member

Choose a reason for hiding this comment

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

only realized this now but the check here is not needed with a set

@MVrachev
keyid: verify adding an existing key is ignored
1ce94b9 
Verify that adding an already existing key to keyid for a particular
role in Root won't create duplicate key.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@jku jku merged commit 765caf5 into theupdateframework:develop Apr 29, 2021
@MVrachev MVrachev deleted the key-role-classes branch April 29, 2021 13:02
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add classes for complex metadata fields
3 participants
@MVrachev @mnm678 @jku