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

ngclient: review snapshot hashes/length check #1523

Closed
jku opened this issue Aug 18, 2021 · 2 comments · Fixed by #1605
Closed

ngclient: review snapshot hashes/length check #1523

jku opened this issue Aug 18, 2021 · 2 comments · Fixed by #1605
Assignees
Labels
backlog Issues to address with priority for current development goals ngclient
Milestone

Comments

@jku
Copy link
Member

jku commented Aug 18, 2021

Update process if timestamp meta contains hashes for snapshot:

  • local root is loaded, root is updated until final
  • local timestamp is loaded
  • timestamp is updated (meta (version, hashes, length) for snapshot changes here)
  • local snapshot is loaded (hashes do not match meta in timestamp, preventing loading)
  • remote snapshot is loaded -- rollback checks are not done because local snapshot was not loaded

The same issues do not exist for targets as they have no rollback checks.

Similar issues for version numbers and expiry are being fixed by delaying the checks until we know we have the "final" snapshot. We don't want to do the exact same thing here as the hashes are meant to prevent even parsing data we don't trust... This seems to be a case where "trusted local metadata" and "untrusted data from network" is the deciding factor. TrustedMetadataSet does not have this information currently so possibly we need to add an argument to update_snapshot():

def update_snapshot(self, data: bytes, trusted: Optional[bool] = False)

where trusted data would not be hash/length checked, but untrusted data would. Looks a bit ugly since none of the other update functions need this but it would seem to work in all cases:

  • when loading local snapshot, we know the data has at some point been trusted (signatures have been checked): it does not need to match hashes now
  • if there is no local snapshot and we're updating from remote, the remote data must match meta hashes in timestamp
  • if there is a local snapshot and we're updating from remote, the remote data must match meta hashes in timestamp
@joshuagl joshuagl added the backlog Issues to address with priority for current development goals label Aug 18, 2021
@MVrachev
Copy link
Collaborator

I would like to take this issue and will assign myself.
If somebody else is working on it, please let me know.

@MVrachev
Copy link
Collaborator

MVrachev commented Sep 2, 2021

I will wait with the implementation of a pr resolving this issue until there is a prototype for #1444 which will make creating a test
for this use case easier.
@jku is already working on it as he shared in his comment under this issue: #1444 (comment)

@sechkova sechkova modified the milestone: Sprint 8 Sep 15, 2021
@sechkova sechkova added this to the Sprint 9 milestone Sep 29, 2021
@jku jku modified the milestones: Sprint 9, Sprint 10 Oct 13, 2021
@jku jku closed this as completed in #1605 Oct 13, 2021
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 ngclient
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants