Support delegations in client #137

raphaelgavache · 2021-07-12T11:28:24Z

This PR adds the support of delegations in the client

Notable changes

support delegations in client.Download, following spec 1.0.19 section 5.6
root is no longer required in the snapshot meta

Notes

ignore the commit Add TUF3 php test, this commit only imports test files
copied TUF3 files from php fixtures to test delegations logic

Follow ups

add chained root downloading to comply with spec and add other fixtures from php

Review walkthrough

1. `data/types.go data/types_test.go`

delegations fields are added to the structs with JSON support
MatchesPath matches a file to a delegation with the support of Paths and PathHashPrefixes

2. `verify/db.go`

reuse DB struct verifiers to verify delegated roles from a parent Targets keys
delegationsVerifier is added to bypass specific logic dedicated to the 4 top roles

3. `client/client.go`

client.Download will now try to fetch FileMeta through the graph of delegations if it's not already cached. Previous pattern was check cache, check local top level Targets. New pattern is check cache, check local top level Targets, check local delegations, download missing delegations.
root is no longer required in snapshot
MaxDelegations parameter is introduced to bound a search in the delegations

4. `client/delegations.go client/delegations_test.go`

getTargetFileMeta implements the delegation search logic

trishankatdatadog · 2021-07-12T16:25:32Z

Thanks, @raphaelgavache!

Would you please summarize the changes over the source code, so that we know what to expect while reviewing?

ethan-lowman-dd

Thanks Raphael. Here's a first batch of comments. I hope to complete a first pass by tomorrow.

data/hex_bytes.go

data/types.go

client/delegations_test.go

client/client_test.go

client/delegations.go

verify/db.go

verify/verify.go

verify/db.go

raphaelgavache · 2021-07-12T21:32:48Z

Thanks Ethan, the delegation verifier code is now a lot cleaner!
I'll apply your other comments tomorrow morning and add an error when both Paths and PathsPrefixes are set to stick to the spec

trishankatdatadog · 2021-07-12T21:40:33Z

Thanks for your review, @ethan-lowman-dd!

@raphaelgavache: does this implement a single-pass (i.e., look for only one target at a time) or a multi-pass (i.e., look for multiple targets at once) approach to delegations?

raphaelgavache · 2021-07-12T21:51:41Z

It's the single-pass one that searches for one target at a time

coveralls · 2021-07-12T21:54:50Z

Pull Request Test Coverage Report for Build 1046833689

200 of 235 (85.11%) changed or added relevant lines in 7 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+2.0%) to 69.354%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
client/client.go	23	24	95.83%
verify/db.go	26	28	92.86%
data/types.go	29	33	87.88%
verify/verify.go	6	11	54.55%
client/errors.go	0	7	0.0%
client/delegations.go	112	128	87.5%

Files with Coverage Reduction	New Missed Lines	%
verify/db.go	1	85.9%

Totals
Change from base Build 987132054:	2.0%
Covered Lines:	1772
Relevant Lines:	2555

💛 - Coveralls

trishankatdatadog · 2021-07-12T22:04:25Z

As a sanity-check, can we cross-check for correctness against this recursive implementation?

…n-client Iteration on "Support delegations in client"

ethan-lowman-dd

See this branch for the proposed changes from today.

verify/verify.go

data/types.go

client/delegations.go

client/delegations_test.go

data/types.go

* Clarify validation of signing role & metadata type * Add/update comments * Validation happens on both decode & encode * Bubble up an error if MatchesPath is called on an invalid DelegatedRole * Match spec for delegation traversal * Comment in the iterator * Add diamond test case * Revert "Match spec for delegation traversal" This reverts commit 15fee6b. * Rename IsTopLevelRole back to ValidRole to avoid breaking change

verify/db.go

trishankatdatadog

first set of comments

data/types.go

verify/db.go

verify/verify.go

client/client.go

data/types.go

client/delegations.go

* Add back lower case check * Initialize with size and comment * Simplify iterator initialization

client/delegations.go

trishankatdatadog

second round of comments

client/delegations.go

data/types.go

client/client.go

client/delegations.go

…ec (#6) (instead of true cycle detection with "edges seen"). This reverts commit cfbb024.

Co-authored-by: Ethan Lowman <[email protected]>

* Update name * Rename file to target * Nits * Move verifier in iterator and rename fields

trishankatdatadog

LGTM, the code is super nice!

@joshuagl and @asraa can you pls help review? 🙂

client/delegations.go

* Update delegations to err on top level role * Add simple cycle test * Remove duplicate check

client/client.go

client/delegations.go

trishankatdatadog · 2021-07-30T02:48:44Z

@raphaelgavache, one more thing: can you check whether you match paths using glob-style matching?

Meanwhile, I will read your tests to check for correctness...

hosseinsia · 2021-07-30T13:33:07Z

client/client.go

@@ -210,13 +218,17 @@ func (c *Client) update(latestRoot bool) (data.TargetFiles, error) {

 	// If we don't have the root.json, download it, save it in local


Shouldn't this be the responsibility of the user to download the very first root.json from a secure channel?

data/types.go

trishankatdatadog · 2021-08-04T17:17:18Z

Hmm, looks like @mnm678 and @asraa are not maintainers of go-tuf, which is not cool! We should fix this. I'm going to go ahead and merge this in the interesting of unblocking this PR, which has been through extensive review. Thanks again everyone, especially @raphaelgavache!

raphaelgavache · 2021-08-23T09:13:28Z

Thank you for the reviews!

* Add delegation client * Add TUF3 php test * Use ioutil for go 1.15 * Add more delegations tests * Cleanups * Check new paths * Add suggestion * Add struct tags * Add tags and remove duplicate types * Formatting * Fix order of asserts (should be want, got) * Clean up DelegatedRole validation * Fix root to top targets delegation * July 14 changes (theupdateframework#4) * Clarify validation of signing role & metadata type * Add/update comments * Validation happens on both decode & encode * Bubble up an error if MatchesPath is called on an invalid DelegatedRole * Match spec for delegation traversal * Comment in the iterator * Add diamond test case * Revert "Match spec for delegation traversal" This reverts commit 15fee6b. * Rename IsTopLevelRole back to ValidRole to avoid breaking change * Update after reviews * Add back lower case check * Initialize with size and comment * Simplify iterator initialization * Revert back to "nodes seen" interpretation of delegation traversal spec (theupdateframework#6) (instead of true cycle detection with "edges seen"). This reverts commit cfbb024. * Update client/delegations.go Co-authored-by: Ethan Lowman <[email protected]> * Update following reviews of 16th of july (theupdateframework#7) * Update name * Rename file to target * Nits * Move verifier in iterator and rename fields * Add tests * Update after 19th july review (theupdateframework#9) * Update delegations to err on top level role * Add simple cycle test * Remove duplicate check * Fix comment * Update comment Co-authored-by: Ethan Lowman <[email protected]> Co-authored-by: Ethan Lowman <[email protected]>

arbll approved these changes Jul 12, 2021

View reviewed changes

ethan-lowman-dd reviewed Jul 12, 2021

View reviewed changes

raphaelgavache added 2 commits July 12, 2021 23:27

Add delegation client

17ca5d9

Add TUF3 php test

b01e941

raphaelgavache force-pushed the raphael/support_delegations_client branch from e62e1b0 to b01e941 Compare July 12, 2021 21:28

Use ioutil for go 1.15

205252d

raphaelgavache and others added 9 commits July 13, 2021 13:51

Add more delegations tests

b1d9372

Cleanups

26d3dcd

Check new paths

8eac49a

Add suggestion

64a1e27

Add struct tags

e15f592

Add tags and remove duplicate types

c0b597c

Formatting

6b76252

Fix order of asserts (should be want, got)

14376fb

Clean up DelegatedRole validation

0c822ec

ethan-lowman-dd mentioned this pull request Jul 13, 2021

Iteration on "Support delegations in client" raphaelgavache/go-tuf#3

Merged

raphaelgavache added 2 commits July 14, 2021 15:27

Merge pull request #3 from ethan-lowman-dd/ethan.lowman/delegations-i…

22b1e63

…n-client Iteration on "Support delegations in client"

Fix root to top targets delegation

a8f8cb0

ethan-lowman-dd reviewed Jul 14, 2021

View reviewed changes

ethan-lowman-dd reviewed Jul 15, 2021

View reviewed changes

verify/db.go Show resolved Hide resolved

trishankatdatadog suggested changes Jul 16, 2021

View reviewed changes

raphaelgavache mentioned this pull request Jul 16, 2021

Delegations in client reviews july 14th, 15th raphaelgavache/go-tuf#5

Merged

Update after reviews

9631aeb

* Add back lower case check * Initialize with size and comment * Simplify iterator initialization

ethan-lowman-dd reviewed Jul 16, 2021

View reviewed changes

client/delegations.go Show resolved Hide resolved

trishankatdatadog suggested changes Jul 16, 2021

View reviewed changes

ethan-lowman-dd and others added 4 commits July 16, 2021 19:26

Revert back to "nodes seen" interpretation of delegation traversal sp…

80371ea

…ec (#6) (instead of true cycle detection with "edges seen"). This reverts commit cfbb024.

Update client/delegations.go

0334084

Co-authored-by: Ethan Lowman <[email protected]>

Update following reviews of 16th of july (#7)

f5a27f3

* Update name * Rename file to target * Nits * Move verifier in iterator and rename fields

Add tests

9c997b3

raphaelgavache mentioned this pull request Jul 19, 2021

Add delegations support DataDog/go-tuf#2

Merged

trishankatdatadog approved these changes Jul 19, 2021

View reviewed changes

client/delegations.go Outdated Show resolved Hide resolved

client/delegations.go Show resolved Hide resolved

client/delegations.go Show resolved Hide resolved

raphaelgavache added 2 commits July 19, 2021 18:59

Update after 19th july review (#9)

e9f9750

* Update delegations to err on top level role * Add simple cycle test * Remove duplicate check

Fix comment

e7cf8b7

asraa reviewed Jul 19, 2021

View reviewed changes

client/client.go Show resolved Hide resolved

client/client.go Show resolved Hide resolved

client/delegations.go Show resolved Hide resolved

Update comment

deff639

asraa approved these changes Jul 26, 2021

View reviewed changes

hosseinsia reviewed Jul 30, 2021

View reviewed changes

trishankatdatadog merged commit e95956b into theupdateframework:master Aug 4, 2021

ethan-lowman-dd mentioned this pull request Aug 16, 2021

Delegation #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support delegations in client #137

Support delegations in client #137

raphaelgavache commented Jul 12, 2021 •

edited

Loading

trishankatdatadog commented Jul 12, 2021

ethan-lowman-dd left a comment

raphaelgavache commented Jul 12, 2021

trishankatdatadog commented Jul 12, 2021

raphaelgavache commented Jul 12, 2021

coveralls commented Jul 12, 2021 •

edited

Loading

trishankatdatadog commented Jul 12, 2021

ethan-lowman-dd left a comment

trishankatdatadog left a comment

trishankatdatadog left a comment

trishankatdatadog left a comment

trishankatdatadog commented Jul 30, 2021

hosseinsia Jul 30, 2021

trishankatdatadog commented Aug 4, 2021

raphaelgavache commented Aug 23, 2021

		@@ -210,13 +218,17 @@ func (c *Client) update(latestRoot bool) (data.TargetFiles, error) {

		// If we don't have the root.json, download it, save it in local

Support delegations in client #137

Support delegations in client #137

Conversation

raphaelgavache commented Jul 12, 2021 • edited Loading

Notable changes

Notes

Follow ups

Review walkthrough

1. data/types.go data/types_test.go

2. verify/db.go

3. client/client.go

4. client/delegations.go client/delegations_test.go

trishankatdatadog commented Jul 12, 2021

ethan-lowman-dd left a comment

Choose a reason for hiding this comment

raphaelgavache commented Jul 12, 2021

trishankatdatadog commented Jul 12, 2021

raphaelgavache commented Jul 12, 2021

coveralls commented Jul 12, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1046833689

💛 - Coveralls

trishankatdatadog commented Jul 12, 2021

ethan-lowman-dd left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Jul 30, 2021

hosseinsia Jul 30, 2021

Choose a reason for hiding this comment

trishankatdatadog commented Aug 4, 2021

raphaelgavache commented Aug 23, 2021

raphaelgavache commented Jul 12, 2021 •

edited

Loading

1. `data/types.go data/types_test.go`

2. `verify/db.go`

3. `client/client.go`

4. `client/delegations.go client/delegations_test.go`

coveralls commented Jul 12, 2021 •

edited

Loading