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.

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

Metadata Attribute research: Threshold #1439

Closed
MVrachev opened this issue Jun 8, 2021 · 4 comments · Fixed by #1450
Closed

Metadata Attribute research: Threshold #1439

MVrachev opened this issue Jun 8, 2021 · 4 comments · Fixed by #1450
Assignees
Labels
backlog Issues to address with priority for current development goals discussion Discussions related to the design, implementation and operation of the project
Milestone

Comments

@MVrachev
Copy link
Collaborator

MVrachev commented Jun 8, 2021

This issue aims to document thoughts about the Threshold attribute from the root and targets roles and more precisely in our implementation Role and DelegatedRole classes.
The goal is to understand how we use that attribute, what might go wrong with it and how we can validate it.

We want to answer/address the following 6 questions/points based on my comment here:

  1. How do we use it?
  2. What might go wrong?
  3. Rethink how we store it?
  4. If, after the changes in step 3 there are concerns, then add a validation function
  5. Make the necessary changes for points 1 - 4, think and propose a way to integrate it into the class/classes (decorators, descriptors, etc.)
  6. create a pr for all additional changes on top of the validation functions

PS: The 7-th point is covered by documenting this issue.

@MVrachev MVrachev added backlog Issues to address with priority for current development goals discussion Discussions related to the design, implementation and operation of the project labels Jun 8, 2021
@sechkova sechkova added this to the weeks24-25 milestone Jun 9, 2021
@MVrachev
Copy link
Collaborator Author

MVrachev commented Jun 9, 2021

  1. How do we use it?
  • Initialization in Role.__init__ and DelegationRole.__init__ (still in DelegationRole we call Role.__init__ for threshold).
  • Then after initialization, in MetadataBundle.verify_with_threshold and in pr Metadata API: Implement threshold verification #1436 we are using threshold as a read-only constant.
  1. What might go wrong?
  • Passing a threshold below 1.
  1. Rethink how we store it?
  • We are initializing it from one place so probably there is no sense in making it a property.
  1. If, after the changes in step 3 there are concerns, then add a validation function
  • No need.
  1. Make the necessary changes for points 1 - 4, think and propose a way to integrate it into the class/classes (decorators, descriptors, etc.)
  • No need.
  1. create a pr for all additional changes on top of the validation functions
  • Should be added. We can add validation only in Role.__init__ because it's called when creating both Role and DelegationRole.

Would love to hear @jku opion.

@jku
Copy link
Member

jku commented Jun 14, 2021

Then after initialization, in MetadataBundle.verify_with_threshold and in pr #1436 we are using threshold as a read-only constant.

once we get to repository-side code, Role and DelegatedRole will be modified though: threshold and keyids do potentially change. Not much thought has been put into this yet (possibly we want more API than add_key()/remove_key())... I think it's fine to implement validation based on current use -- if we don't know what the modifying API looks like we can't really validate that.

@jku
Copy link
Member

jku commented Jun 15, 2021

once we get to repository-side code, Role and DelegatedRole will be modified though: threshold and keyids do potentially change. Not much thought has been put into this yet (possibly we want more API than add_key()/remove_key())... I think it's fine to implement validation based on current use -- if we don't know what the modifying API looks like we can't really validate that.

One alternative to this is to consider Role and DelegatedRole immutable objects: when repository code wants to change keys or threshold of a role, it should just add/remove/replace the complete role instead of modifying the keys/threshold in the role. This way it would be easier to ensure that e.g. the delegation keys includes the keys (and only the keys) that are actually used by current delegation roles.

Regardless, i don't think this should affect what you do to threshold within the Role object: what you're planning seems fine.

@MVrachev
Copy link
Collaborator Author

I think mentally I always considered Key as an immutable object.
As you pointed out: because we are not sure what modification API do we want to add we can only verify what we know.
Will submit a pr verifying only the simplest case - that threshold is more than 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals discussion Discussions related to the design, implementation and operation of the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants