-
Notifications
You must be signed in to change notification settings - Fork 109
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
Rotate roots per spec #143
Rotate roots per spec #143
Conversation
Pull Request Test Coverage Report for Build 1242433780
💛 - Coveralls |
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.
Some drive by comments. I'm not very familiar with go-tuf, so take them with a grain of salt.
This PR is very hard to review because of the number of added files. Do we need them all? Perhaps separate them into a separate commit so that the core logic change is easier to review?
Personally, I'm not a fan of the very large comments which seem likely to become out-of-date. But that appears to be the style for the go-tuf implementation.
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.
First round of comments. Thanks for the great work, @hosseinsia! 👏🏽 💯
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.
updated according to comments. PTAL.
Major changes:
- The code is being changes such won't persist a bad root metadata (except for when it is expired).
- Added a few more tests to check for the cases of non-root key change.
- Refactored/simplified the client_test
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.
Nice PR! This is my first pass, I've left a few minor comments. I'll go through the test cases next
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.
So close! Thank you very much.
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.
lgtm!
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.
Almost there!
@trishankatdatadog addressed all comments. PTAL. |
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.
Thanks so much for continuing to review!
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.
Fantastic work, thank you very much, Hossein! 🎉
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.
lgtm!
@hosseinsia do we have tests for whether expirations are ignored and not ignored correctly? |
Just added a test. ptal. |
… have concerete timestamps (either expired or long exiring one)
…cal root but no other top-level metadata
45675bf
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.
Took another pass, LGTM!
* update roots * removing some debugging comments * removing duplicate code for getLocalRootMeta by calling it from getLocalMeta * fix based on the reviews. * enable an arbitrary root verify another root (use case: n verify n+1) without the need for store them permanently. * check non root metadata, refactor test, address comments * updated according to the comments * remove persistent metadata is the keys have changed. * removing the unused ErrWrongRootVersion * add DeleteMeta to the LocalStore interface and implemenet in MemoryLocalStore and FileLocalStore subtypes. * delete (instead of setting to an empty raw message) the top-level metadata when their key has changed. * add test fixtures for fast forward attack recovery. * test for fast forward attack recovery * addressed several comments. * addressed more comments. Set the rootVersion in loadAndVerifyLocalRootMeta. Fixed a buggy test. * Fixed a buggy test. * fix comment typos * fix race condition related to the expired check. * fix race condition related to the expired check. * kill unmarshalIgnoreExpired. * add test for root update for client version above 1. * add test for root update for client version greater than 1. * update the VerifyIgnoreExpiredCheck method signature and add test for it. * Avoid mocking IsExpired in the tests. Instead update test fixtured to have concerete timestamps (either expired or long exiring one) * remove commented code * update fixtures and clarify test comments. * updating the comments based on the feedbacks. * update roots * removing some debugging comments * removing duplicate code for getLocalRootMeta by calling it from getLocalMeta * fix based on the reviews. * enable an arbitrary root verify another root (use case: n verify n+1) without the need for store them permanently. * check non root metadata, refactor test, address comments * updated according to the comments * remove persistent metadata is the keys have changed. * removing the unused ErrWrongRootVersion * delete (instead of setting to an empty raw message) the top-level metadata when their key has changed. * add test fixtures for fast forward attack recovery. * test for fast forward attack recovery * addressed several comments. * addressed more comments. Set the rootVersion in loadAndVerifyLocalRootMeta. Fixed a buggy test. * Fixed a buggy test. * fix comment typos * Update client/client_test.go Co-authored-by: Trishank Karthik Kuppusamy <[email protected]> * Update client/client_test.go Co-authored-by: Trishank Karthik Kuppusamy <[email protected]> * fix race condition related to the expired check. * fix race condition related to the expired check. * kill unmarshalIgnoreExpired. * add test for root update for client version above 1. * add test for root update for client version greater than 1. * update the VerifyIgnoreExpiredCheck method signature and add test for it. * Avoid mocking IsExpired in the tests. Instead update test fixtured to have concerete timestamps (either expired or long exiring one) * remove commented code * update fixtures and clarify test comments. * updating the comments based on the feedbacks. * rebase and update test cases to long expiration (10 years from now), by default. * add test cases for (1) when there is no local root, (2) there is a local root but no other top-level metadata * remove the 'previous' of test folders Co-authored-by: Trishank Karthik Kuppusamy <[email protected]>
Update root following the TUF specification v1.0.19
https://theupdateframework.github.io/specification/v1.0.19/index.html#load-trusted-root
Logic: