Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Check timestamp/snapshot contains snapshot/targets description #226
base: master
Are you sure you want to change the base?
Check timestamp/snapshot contains snapshot/targets description #226
Changes from all commits
e97d6ad
440e099
cc41726
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we discard the new timestamp metadata? Could an attacker later roll back the timestamp file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's necessarily resisting an attack, but more about enabling the optimization where we can skip downloading metadata if we already have it locally without putting our local trusted metadata in an inconsistent state (such as in #114 and #227).
It might be helpful if we work with an example. Say we have locally trusted metadata, with Timestamp T1 and Snapshot S1, which is at version N. The remote repository is at Timestamp T1 and Snapshot S2, which also is at version N.
When we perform a client update, we will:
At this point we could decide that we we already have the trusted Snapshot S1, so we could abort the update workflow. However this would be incorrect, because it's possible that Snapshot S2 could have different content than Snapshot S1. If Timestamp T2 contains the optional length and/or hashes of Snapshot S2. This means that if we create a brand new client based off the local trusted metadata, we cannot use the length/hashes from Timestamp T2 to verify Snapshot S1. This would then force us to throw away the Snapshot S1 and download the new S2 version.
This patch tries to avoid this issue by enforcing that we cannot get into this state by treating Timestamp T2 as invalid, since it doesn't match what we already trust.
This would be especially handy if we implement transactional downloading of metadata in #69 to make sure the local metadata doesn't get into an incomplete state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean T2? You do say the client downloads T2 below. I assume this is a typo here?
Just to make sure, do I understand correctly that in your example:
This implies that the repo has incorrectly updated snapshot and timestamp, without bumping snapshot's version number, right?
Following you proposal, the client that already trusts T1 and S1 can only update, if the repo issues a new T2 (or higher) describing a new S2, with a version number higher than S1. Is that correct? If so, I think it's fair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was covered in the above comment, so lets track it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to discard the snapshot file in this case? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was covered in the above comment, so lets track it there.