From 3f227005fc0f8203306dce21de01bf185717a5d0 Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Wed, 14 Sep 2022 14:56:12 +0200 Subject: [PATCH 1/6] fix(localMeta): Add delegated targets back to localMeta Signed-off-by: Baptiste Foy --- client/client.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/client/client.go b/client/client.go index f103d802..6b93d0a7 100644 --- a/client/client.go +++ b/client/client.go @@ -423,6 +423,16 @@ func (c *Client) getLocalMeta() error { c.loadTargets(targets.Targets) } } + + for fileName := range meta { + if fileName != "targets.json" && + fileName != "snapshot.json" && + fileName != "root.json" && + fileName != "timestamp.json" { + c.localMeta[fileName] = meta[fileName] + } + } + if loadFailed { // If any of the metadata failed to be verified, return the reason for that failure return retErr From f852e605d9ce707aa1d27ca6291bb667ea55f078 Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Wed, 14 Sep 2022 15:19:27 +0200 Subject: [PATCH 2/6] chore(tests): Add tests to validate the new behaviour Signed-off-by: Baptiste Foy --- client/delegations_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/client/delegations_test.go b/client/delegations_test.go index 47fdc229..6d5a7767 100644 --- a/client/delegations_test.go +++ b/client/delegations_test.go @@ -226,17 +226,27 @@ func TestPersistedMeta(t *testing.T) { p, err := c.local.GetMeta() assert.Nil(t, err) persisted := copyStore(p) + persistedLocal := copyStore(c.localMeta) // trim non targets metas for _, notTargets := range []string{"root.json", "snapshot.json", "timestamp.json"} { delete(persisted, notTargets) + delete(persistedLocal, notTargets) } for _, targets := range tt.targets { + // Test local store storedVersion, err := versionOfStoredTargets(targets.name, persisted) assert.Equal(t, targets.version, storedVersion) assert.Nil(t, err) delete(persisted, targets.name) + + // Test localMeta + storedVersion, err = versionOfStoredTargets(targets.name, persistedLocal) + assert.Equal(t, targets.version, storedVersion) + assert.Nil(t, err) + delete(persistedLocal, targets.name) } assert.Empty(t, persisted) + assert.Empty(t, persistedLocal) }) } } From 9634f0bc67b35199f31525ce5cd2882e280ea401 Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Thu, 15 Sep 2022 10:19:27 +0200 Subject: [PATCH 3/6] refactor(getLocalMeta): Use IsDelegatedTargetsManifest to filter out top metas Signed-off-by: Baptiste Foy --- client/client.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/client.go b/client/client.go index 6b93d0a7..87c6d6bc 100644 --- a/client/client.go +++ b/client/client.go @@ -7,6 +7,7 @@ import ( "io" "github.com/theupdateframework/go-tuf/data" + "github.com/theupdateframework/go-tuf/internal/roles" "github.com/theupdateframework/go-tuf/util" "github.com/theupdateframework/go-tuf/verify" ) @@ -425,10 +426,7 @@ func (c *Client) getLocalMeta() error { } for fileName := range meta { - if fileName != "targets.json" && - fileName != "snapshot.json" && - fileName != "root.json" && - fileName != "timestamp.json" { + if roles.IsDelegatedTargetsManifest(fileName) { c.localMeta[fileName] = meta[fileName] } } From b6e97733d65788bbe8697ad79db9c53f8318faf6 Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Thu, 22 Dec 2022 09:45:46 +0100 Subject: [PATCH 4/6] upgrade(localMeta): Verify delegated targets Adds verification of delegated targets when they go from the local store to the localMeta structure. The verification is done by selecting one target file per delegated targets that verify at least one, then build the delegation path from the top targets to that specific delegated file. Building the path is already verifying every targets in the path, so the whole path is safe to add to the localMeta. Doing so for every delegated targets lets us verify every delegated targets that is used at least once. Signed-off-by: Baptiste Foy --- client/client.go | 53 +++++++++++++++++++++++++++++++++++++-- client/delegations.go | 53 ++++++++++++++++++++++++++++++++------- pkg/targets/delegation.go | 7 ++++++ 3 files changed, 102 insertions(+), 11 deletions(-) diff --git a/client/client.go b/client/client.go index 87c6d6bc..e2701327 100644 --- a/client/client.go +++ b/client/client.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/hex" "encoding/json" + "fmt" "io" "github.com/theupdateframework/go-tuf/data" @@ -400,8 +401,8 @@ func (c *Client) getLocalMeta() error { } } + snapshot := &data.Snapshot{} if snapshotJSON, ok := meta["snapshot.json"]; ok { - snapshot := &data.Snapshot{} if err := c.db.UnmarshalTrusted(snapshotJSON, snapshot, "snapshot"); err != nil { loadFailed = true retErr = err @@ -425,12 +426,33 @@ func (c *Client) getLocalMeta() error { } } + if loadFailed { + // If any of the metadata failed to be verified, return the reason for that failure + // and fail fast before delegated targets + return retErr + } + + verifiedDelegatedTargets := make(map[string]bool) for fileName := range meta { if roles.IsDelegatedTargetsManifest(fileName) { - c.localMeta[fileName] = meta[fileName] + if delegations, err := c.getDelegationPathFromRaw(snapshot, meta[fileName]); err != nil { + loadFailed = true + retErr = err + } else { + for _, key := range delegations { + fileName := fmt.Sprintf("%s.json", key) + if !verifiedDelegatedTargets[fileName] { + verifiedDelegatedTargets[fileName] = true + } + } + } } } + for fileName := range verifiedDelegatedTargets { + c.localMeta[fileName] = meta[fileName] + } + if loadFailed { // If any of the metadata failed to be verified, return the reason for that failure return retErr @@ -438,6 +460,33 @@ func (c *Client) getLocalMeta() error { return nil } +// getDelegationPathFromRaw verifies a delegated targets against +// a given snapshot and returns an error if it's invalid +// +// Delegation must have targets to get a path, else an empty list +// will be returned: this is because the delegation iterator is leveraged. +func (c *Client) getDelegationPathFromRaw(snapshot *data.Snapshot, delegatedTargetsJSON json.RawMessage) ([]string, error) { + // unmarshal the delegated targets first without verifying as + // we need at least one targets file name to leverage the + // getTargetFileMetaDelegationPath method + s := &data.Signed{} + if err := json.Unmarshal(delegatedTargetsJSON, s); err != nil { + return nil, err + } + targets := &data.Targets{} + if err := json.Unmarshal(s.Signed, targets); err != nil { + return nil, err + } + for targetPath := range targets.Targets { + _, resp, err := c.getTargetFileMetaDelegationPath(targetPath, snapshot) + // We only need to test one targets file: + // - If it is valid, it means the delegated targets has been validated + // - If it is not, the delegated targets isn't valid + return resp, err + } + return nil, nil +} + // loadAndVerifyLocalRootMeta decodes and verifies root metadata from // local storage and loads the top-level keys. This method first clears // the DB for top-level keys and then loads the new keys. diff --git a/client/delegations.go b/client/delegations.go index de3e6647..cc0fc482 100644 --- a/client/delegations.go +++ b/client/delegations.go @@ -8,13 +8,23 @@ import ( // getTargetFileMeta searches for a verified TargetFileMeta matching a target // Requires a local snapshot to be loaded and is locked to the snapshot versions. -// Searches through delegated targets following TUF spec 1.0.19 section 5.6. func (c *Client) getTargetFileMeta(target string) (data.TargetFileMeta, error) { snapshot, err := c.loadLocalSnapshot() if err != nil { return data.TargetFileMeta{}, err } + targetFileMeta, _, err := c.getTargetFileMetaDelegationPath(target, snapshot) + if err != nil { + return data.TargetFileMeta{}, err + } + return targetFileMeta, nil +} + +// getTargetFileMetaDelegationPath searches for a verified TargetFileMeta matching a target +// Requires snapshot to be passed and is locked to that specific snapshot versions. +// Searches through delegated targets following TUF spec 1.0.19 section 5.6. +func (c *Client) getTargetFileMetaDelegationPath(target string, snapshot *data.Snapshot) (data.TargetFileMeta, []string, error) { // delegationsIterator covers 5.6.7 // - pre-order depth-first search starting with the top targets // - filter delegations with paths or path_hash_prefixes matching searched target @@ -22,50 +32,75 @@ func (c *Client) getTargetFileMeta(target string) (data.TargetFileMeta, error) { // - 5.6.7.2 terminations delegations, err := targets.NewDelegationsIterator(target, c.db) if err != nil { - return data.TargetFileMeta{}, err + return data.TargetFileMeta{}, nil, err } + targetFileMeta := data.TargetFileMeta{} + delegationRole := "" + for i := 0; i < c.MaxDelegations; i++ { d, ok := delegations.Next() if !ok { - return data.TargetFileMeta{}, ErrUnknownTarget{target, snapshot.Version} + return data.TargetFileMeta{}, nil, ErrUnknownTarget{target, snapshot.Version} } // covers 5.6.{1,2,3,4,5,6} targets, err := c.loadDelegatedTargets(snapshot, d.Delegatee.Name, d.DB) if err != nil { - return data.TargetFileMeta{}, err + return data.TargetFileMeta{}, nil, err } // stop when the searched TargetFileMeta is found if m, ok := targets.Targets[target]; ok { - return m, nil + delegationRole = d.Delegatee.Name + targetFileMeta = m + break } if targets.Delegations != nil { delegationsDB, err := verify.NewDBFromDelegations(targets.Delegations) if err != nil { - return data.TargetFileMeta{}, err + return data.TargetFileMeta{}, nil, err } err = delegations.Add(targets.Delegations.Roles, d.Delegatee.Name, delegationsDB) if err != nil { - return data.TargetFileMeta{}, err + return data.TargetFileMeta{}, nil, err } } } - return data.TargetFileMeta{}, ErrMaxDelegations{ + if len(delegationRole) > 0 { + return targetFileMeta, buildPath(delegations.Parent, delegationRole, ""), nil + } + + return data.TargetFileMeta{}, nil, ErrMaxDelegations{ Target: target, MaxDelegations: c.MaxDelegations, SnapshotVersion: snapshot.Version, } } +func buildPath(parent func(string) string, start string, end string) []string { + if start == end { + return nil + } + + path := []string{start} + current := start + for { + current = parent(current) + if current == end { + break + } + path = append(path, current) + } + return path +} + func (c *Client) loadLocalSnapshot() (*data.Snapshot, error) { if err := c.getLocalMeta(); err != nil { return nil, err } - rawS, ok := c.localMeta["snapshot.json"] if !ok { return nil, ErrNoLocalSnapshot diff --git a/pkg/targets/delegation.go b/pkg/targets/delegation.go index ccd52bae..dce61710 100644 --- a/pkg/targets/delegation.go +++ b/pkg/targets/delegation.go @@ -18,6 +18,7 @@ type delegationsIterator struct { stack []Delegation target string visitedRoles map[string]struct{} + parents map[string]string } var ErrTopLevelTargetsRoleMissing = errors.New("tuf: top level targets role missing from top level keys DB") @@ -43,6 +44,7 @@ func NewDelegationsIterator(target string, topLevelKeysDB *verify.DB) (*delegati }, }, visitedRoles: make(map[string]struct{}), + parents: make(map[string]string), } return i, nil } @@ -88,8 +90,13 @@ func (d *delegationsIterator) Add(roles []data.DelegatedRole, delegator string, DB: db, } d.stack = append(d.stack, delegation) + d.parents[r.Name] = delegator } } return nil } + +func (d *delegationsIterator) Parent(role string) string { + return d.parents[role] +} From 6b5e2f19ca22bce0a7df0e1016b729772b221bc7 Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Thu, 22 Dec 2022 18:28:10 +0100 Subject: [PATCH 5/6] docs(comments): Add more comments on tricky parts of the implementation Signed-off-by: Baptiste Foy --- client/client.go | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/client/client.go b/client/client.go index e2701327..f749bf1e 100644 --- a/client/client.go +++ b/client/client.go @@ -432,18 +432,19 @@ func (c *Client) getLocalMeta() error { return retErr } + // verifiedDelegatedTargets is a set of verified delegated targets verifiedDelegatedTargets := make(map[string]bool) for fileName := range meta { if roles.IsDelegatedTargetsManifest(fileName) { - if delegations, err := c.getDelegationPathFromRaw(snapshot, meta[fileName]); err != nil { + if delegationPath, err := c.getDelegationPathFromRaw(snapshot, meta[fileName]); err != nil { loadFailed = true retErr = err } else { - for _, key := range delegations { + // Every delegated targets in the path has been verified + // as a side effect of getDelegationPathFromRaw + for _, key := range delegationPath { fileName := fmt.Sprintf("%s.json", key) - if !verifiedDelegatedTargets[fileName] { - verifiedDelegatedTargets[fileName] = true - } + verifiedDelegatedTargets[fileName] = true } } } @@ -465,6 +466,20 @@ func (c *Client) getLocalMeta() error { // // Delegation must have targets to get a path, else an empty list // will be returned: this is because the delegation iterator is leveraged. +// +// Concrete example: +// targets +// └── a.json +//   └── b.json +//      └── c.json +//        └── target_file.txt +// +// If you try to use that function on "a.json" or "b.json", it'll return an empty list +// with no error, as neither of them declare a target file +// On the other hand, if you use that function on "c.json", it'll return & verify +// [c.json, b.json, a.json]. Running that function on every delegated targets +// guarantees that if a delegated targets is in the path of a target file, then it will +// appear at least once in the result func (c *Client) getDelegationPathFromRaw(snapshot *data.Snapshot, delegatedTargetsJSON json.RawMessage) ([]string, error) { // unmarshal the delegated targets first without verifying as // we need at least one targets file name to leverage the From 45234eed5d23a137386e8332d8d97015e400159c Mon Sep 17 00:00:00 2001 From: Baptiste Foy Date: Fri, 6 Jan 2023 11:20:50 +0100 Subject: [PATCH 6/6] fix(localMeta): Remove unnecessary calls to getDelegationPathFromRaw Signed-off-by: Baptiste Foy --- client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/client.go b/client/client.go index f749bf1e..978ebf1d 100644 --- a/client/client.go +++ b/client/client.go @@ -435,7 +435,7 @@ func (c *Client) getLocalMeta() error { // verifiedDelegatedTargets is a set of verified delegated targets verifiedDelegatedTargets := make(map[string]bool) for fileName := range meta { - if roles.IsDelegatedTargetsManifest(fileName) { + if !verifiedDelegatedTargets[fileName] && roles.IsDelegatedTargetsManifest(fileName) { if delegationPath, err := c.getDelegationPathFromRaw(snapshot, meta[fileName]); err != nil { loadFailed = true retErr = err