Rotate roots per spec by hosseinsia · Pull Request #143 · theupdateframework/go-tuf

hosseinsia · 2021-08-10T14:25:16Z

Update root following the TUF specification v1.0.19
https://theupdateframework.github.io/specification/v1.0.19/index.html#load-trusted-root

Logic:

Call the update root first in any client updates.
Start from the current root version (n), loop through next versions by trying to fetch (n+1).root.json file.
For any new root, verify the signatures.

coveralls · 2021-08-10T14:29:04Z

Pull Request Test Coverage Report for Build 1242433780

192 of 243 (79.01%) changed or added relevant lines in 5 files are covered.
55 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-1.5%) to 67.999%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
verify/errors.go	0	3	0.0%
verify/verify.go	14	27	51.85%
client/client.go	171	206	83.01%

Files with Coverage Reduction	New Missed Lines	%
client/errors.go	12	7.84%
client/client.go	43	75.04%

Totals
Change from base Build 1221339773:	-1.5%
Covered Lines:	1872
Relevant Lines:	2753

💛 - Coveralls

joshuagl

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.

trishankatdatadog

First round of comments. Thanks for the great work, @hosseinsia! 👏🏽 💯

hosseinsia

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

raphaelgavache

Nice PR! This is my first pass, I've left a few minor comments. I'll go through the test cases next

trishankatdatadog

So close! Thank you very much.

raphaelgavache

lgtm!

trishankatdatadog

Almost there!

hosseinsia · 2021-09-03T01:06:41Z

@trishankatdatadog addressed all comments. PTAL.

asraa

Thanks so much for continuing to review!

trishankatdatadog

Fantastic work, thank you very much, Hossein! 🎉

raphaelgavache

lgtm!

trishankatdatadog · 2021-09-07T15:47:34Z

@hosseinsia do we have tests for whether expirations are ignored and not ignored correctly?

hosseinsia · 2021-09-07T16:41:30Z

@hosseinsia do we have tests for whether expirations are ignored and not ignored correctly?

Just added a test. ptal.

… it.

… have concerete timestamps (either expired or long exiring one)

…by default.

hosseinsia · 2021-09-14T01:28:02Z

@asraa and @joshuagl ptal!

…cal root but no other top-level metadata

asraa

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 <trishank.kuppusamy@datadoghq.com> * Update client/client_test.go Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> * 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 <trishank.kuppusamy@datadoghq.com>

joshuagl reviewed Aug 10, 2021

View reviewed changes

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

trishankatdatadog suggested changes Aug 10, 2021

View reviewed changes

asraa reviewed Aug 11, 2021

View reviewed changes

Comment thread client/client.go Outdated

asraa reviewed Aug 11, 2021

View reviewed changes

Comment thread client/errors.go Outdated

asraa reviewed Aug 11, 2021

View reviewed changes

Comment thread client/client.go Outdated

hosseinsia commented Aug 23, 2021

View reviewed changes

hosseinsia mentioned this pull request Aug 24, 2021

Support root rotation DataDog/go-tuf#8

Closed

raphaelgavache reviewed Aug 24, 2021

View reviewed changes

Comment thread client/errors.go Outdated

Comment thread verify/errors.go Outdated

Comment thread verify/verify.go Outdated

Comment thread client/client_test.go Outdated

trishankatdatadog suggested changes Aug 24, 2021

View reviewed changes

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go

Comment thread client/client.go Outdated

Comment thread client/client.go

raphaelgavache reviewed Aug 25, 2021

View reviewed changes

Comment thread client/client.go

Comment thread client/client.go

Comment thread client/client.go

hosseinsia mentioned this pull request Aug 26, 2021

client side rootupdate + test fixtures DataDog/go-tuf#10

Closed

raphaelgavache previously approved these changes Aug 31, 2021

View reviewed changes

hosseinsia dismissed raphaelgavache’s stale review via 78b1631 September 1, 2021 23:22

trishankatdatadog suggested changes Sep 2, 2021

View reviewed changes

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client_test.go Outdated

hosseinsia mentioned this pull request Sep 3, 2021

Update the fast forward recovery implementation #149

Closed

raphaelgavache reviewed Sep 3, 2021

View reviewed changes

Comment thread client/client.go

asraa reviewed Sep 3, 2021

View reviewed changes

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go Outdated

Comment thread client/client.go

trishankatdatadog reviewed Sep 3, 2021

View reviewed changes

Comment thread client/client_test.go Outdated

trishankatdatadog reviewed Sep 3, 2021

View reviewed changes

Comment thread client/client_test.go Outdated

trishankatdatadog reviewed Sep 3, 2021

View reviewed changes

Comment thread client/client_test.go Outdated

trishankatdatadog previously approved these changes Sep 3, 2021

View reviewed changes

hosseinsia dismissed trishankatdatadog’s stale review via d9a4507 September 4, 2021 03:25

raphaelgavache reviewed Sep 4, 2021

View reviewed changes

Comment thread client/client.go Outdated

raphaelgavache previously approved these changes Sep 7, 2021

View reviewed changes

hosseinsia dismissed raphaelgavache’s stale review via cafa7b6 September 7, 2021 16:06

joshuagl reviewed Sep 8, 2021

View reviewed changes

Comment thread client/client_test.go Outdated

Comment thread client/client_test.go Outdated

hosseinsia added 11 commits September 11, 2021 22:41

fix race condition related to the expired check.

c3e4817

fix race condition related to the expired check.

0f72725

kill unmarshalIgnoreExpired.

3505474

add test for root update for client version above 1.

1e7d10f

add test for root update for client version greater than 1.

1a51430

update the VerifyIgnoreExpiredCheck method signature and add test for…

32ebe0a

… it.

Avoid mocking IsExpired in the tests. Instead update test fixtured to…

9681093

… have concerete timestamps (either expired or long exiring one)

remove commented code

0f5fe0e

update fixtures and clarify test comments.

79e0bc6

updating the comments based on the feedbacks.

ba6ffde

rebase and update test cases to long expiration (10 years from now), …

bdb5cee

…by default.

raphaelgavache previously approved these changes Sep 13, 2021

View reviewed changes

trishankatdatadog previously approved these changes Sep 13, 2021

View reviewed changes

add test cases for (1) when there is no local root, (2) there is a lo…

45675bf

…cal root but no other top-level metadata

hosseinsia dismissed stale reviews from trishankatdatadog and raphaelgavache via 45675bf September 16, 2021 06:38

raphaelgavache previously approved these changes Sep 16, 2021

View reviewed changes

trishankatdatadog reviewed Sep 16, 2021

View reviewed changes

Comment thread client/client_test.go

remove the 'previous' of test folders

d04805c

hosseinsia dismissed raphaelgavache’s stale review via d04805c September 16, 2021 16:44

trishankatdatadog approved these changes Sep 16, 2021

View reviewed changes

trishankatdatadog mentioned this pull request Sep 16, 2021

Sort signatures by keys IDs for deterministic signature order #155

Merged

raphaelgavache approved these changes Sep 17, 2021

View reviewed changes

asraa approved these changes Sep 17, 2021

View reviewed changes

hosseinsia merged commit 526f227 into theupdateframework:master Sep 17, 2021

hosseinsia deleted the updateroots branch September 17, 2021 17:03

trishankatdatadog mentioned this pull request Oct 6, 2021

Improve the CLI experience #160

Merged

ethan-lowman-dd mentioned this pull request Jan 18, 2022

[Delegations prereq 8] Make snapshot manifest list dynamic, and remove root.json from snapshot #198

Merged

Conversation

hosseinsia commented Aug 10, 2021

Uh oh!

coveralls commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1242433780

💛 - Coveralls

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trishankatdatadog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hosseinsia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raphaelgavache left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trishankatdatadog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

raphaelgavache left a comment

Choose a reason for hiding this comment

Uh oh!

trishankatdatadog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hosseinsia commented Sep 3, 2021

Uh oh!

Uh oh!

asraa left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 10, 2021 •

edited

Loading