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

Avoid unnecessary delegated role downloads #811

Merged
merged 2 commits into from
Dec 31, 2018

Conversation

awwad
Copy link
Contributor

@awwad awwad commented Dec 7, 2018

Using deprecated function Updater.targets_of_role() to fetch the targets listed by the top-level Targets role previously led to the unneeded download of all delegated targets roles. This PR fixes that specifically for calls to targets_of_role('targets') (which is the only well-defined and secure use of the deprecated function targets_of_role).

All delegated targets continue to be downloaded if targets_of_role is called on a delegated targets role. This provides something like what we think users who might do this expect (It's not clear there are any people doing this.), refreshing all delegated targets roles to avoid use of outdated delegation information, and essentially hoping that the most-recently-validated delegation to the role of interest is what the user would expect that role to have been validated by.....

targets_of_role will be removed or replaced, so this is a fix for the sole reasonable use case until then.

The PR also adds some explanatory comments and warnings to the code.

  • 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

in comments that explain a bit about why, in updater.py.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
If the role you're fetching the targets of is the 'targets' role,
do not download all delegated targets roles....

Continue to do that only if you're fetching the targets of a
delegated targets role, for historical reasons until this
deprecated function is removed / replaced.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>
@JustinCappos JustinCappos merged commit 12de53e into develop Dec 31, 2018
@trishankatdatadog
Copy link
Member

We should remove this function altogether, then.

Sometimes, you cannot use get_one_valid_targetinfo(), because you don't know the names of the targets in advance (e.g., public keys for the root in-toto layout). However, you do know that they are signed by the top-level targets role.

How should we handle this going forward?

@awwad
Copy link
Contributor Author

awwad commented Jan 2, 2019

I'm planning on rewriting the function to provide target info from a given role only from what is verified and in memory (rather than trying to refresh and verify the metadata). If there is no verified metadata for the given role, the function would raise an error indicating that. Basically, it becomes a convenience function that extracts the info from what is already known and verified about the role. Use for role 'targets' would then just be:

refresh()
targets_of_role('targets')

As for delegated roles, if the user wants to skip the delegation tree and jump straight to a role (instead of using the proper method to walk down the delegation tree starting at the 'targets' role), it would be the user's job to get the metadata verified through the appropriate path.

(The broader problem of the reference implementation sometimes treating roles and delegations as interchangeable needs to be addressed.)

@trishankatdatadog
Copy link
Member

Yes, the key, role, and delegation management in the reference implementation is a bit of a mess, I'm afraid. I had advocated using a more object-oriented approach a long time ago, which I think would have led to a cleaner, more intuitive implementation.

@awwad
Copy link
Contributor Author

awwad commented Jan 2, 2019

We can do it with or without OO, and I think the role-vs-delegation trap was there either way. Based on lab principles, we'll try to keep OO to a minimum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants