From 83f7c758caadbc10703aeeb00cd2a1a5be9d038f Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Wed, 9 Mar 2016 12:01:48 -0800 Subject: [PATCH 1/2] Remove delegation role fallback when applying targets changes Signed-off-by: Riyaz Faizullabhoy --- client/client_test.go | 68 +++++++++++------------------------------- client/helpers.go | 30 +++---------------- client/helpers_test.go | 27 ++++++++--------- docs/advanced_usage.md | 2 +- 4 files changed, 36 insertions(+), 91 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 777901f38d..eb23713f75 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2046,8 +2046,7 @@ func createKey(t *testing.T, repo *NotaryRepository, role string, x509 bool) dat // Publishing delegations works so long as the delegation parent exists by the // time that delegation addition change is applied. Most of the tests for // applying delegation changes in in helpers_test.go (applyTargets tests), so -// this is just a sanity test to make sure Publish calls it correctly and -// no fallback happens. +// this is just a sanity test to make sure Publish calls it correctly func TestPublishDelegations(t *testing.T) { testPublishDelegations(t, true, false) testPublishDelegations(t, false, false) @@ -2140,44 +2139,13 @@ func testPublishDelegations(t *testing.T, clearCache, x509Keys bool) { } // If a changelist specifies a particular role to push targets to, and there -// is no such role, publish will try to publish to its parent. If the parent -// doesn't work, it falls back on its parent, and so forth, and eventually -// falls back on publishing to "target". This *only* falls back if the role -// doesn't exist, not if the user doesn't have a key. (different test) -func TestPublishTargetsDelgationScopeFallback(t *testing.T) { - testPublishTargetsDelgationScopeFallback(t, true) - testPublishTargetsDelgationScopeFallback(t, false) +// is a role but no key, publish should just fail outright. +func TestPublishTargetsDelegationScopeFailIfNoKeys(t *testing.T) { + testPublishTargetsDelegationScopeFailIfNoKeys(t, true) + testPublishTargetsDelegationScopeFailIfNoKeys(t, false) } -func testPublishTargetsDelgationScopeFallback(t *testing.T, clearCache bool) { - ts := fullTestServer(t) - defer ts.Close() - - repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false) - defer os.RemoveAll(repo.baseDir) - - var rec *passRoleRecorder - if clearCache { - repo, rec = newRepoToTestRepo(t, repo, false) - } - - requirePublishToRolesSucceeds(t, repo, []string{"targets/a/b", "targets/b/c"}, - []string{data.CanonicalTargetsRole}) - - if clearCache { - rec.requireAsked(t, []string{data.CanonicalTargetsRole, data.CanonicalSnapshotRole}) - rec.requireCreated(t, nil) - } -} - -// If a changelist specifies a particular role to push targets to, and there -// is a role but no key, publish not fall back and just fail. -func TestPublishTargetsDelgationScopeNoFallbackIfNoKeys(t *testing.T) { - testPublishTargetsDelgationScopeNoFallbackIfNoKeys(t, true) - testPublishTargetsDelgationScopeNoFallbackIfNoKeys(t, false) -} - -func testPublishTargetsDelgationScopeNoFallbackIfNoKeys(t *testing.T, clearCache bool) { +func testPublishTargetsDelegationScopeFailIfNoKeys(t *testing.T, clearCache bool) { ts := fullTestServer(t) defer ts.Close() @@ -2227,7 +2195,7 @@ func testPublishTargetsDelgationScopeNoFallbackIfNoKeys(t *testing.T, clearCache // not its parents. This tests the case where the local machine knows about // all the roles (in fact, the role creations will be applied before the // targets) -func TestPublishTargetsDelgationSuccessLocallyHasRoles(t *testing.T) { +func TestPublishTargetsDelegationSuccessLocallyHasRoles(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -2242,7 +2210,7 @@ func TestPublishTargetsDelgationSuccessLocallyHasRoles(t *testing.T) { } // just always check signing now, we've already established we can publish - // delgations with and without the metadata and key cache + // delegations with and without the metadata and key cache var rec *passRoleRecorder repo, rec = newRepoToTestRepo(t, repo, false) @@ -2257,7 +2225,7 @@ func TestPublishTargetsDelgationSuccessLocallyHasRoles(t *testing.T) { // If a changelist specifies a particular role to push targets to, and the role // is present, publish will write to that role only. The targets keys are not // needed. -func TestPublishTargetsDelgationNoTargetsKeyNeeded(t *testing.T) { +func TestPublishTargetsDelegationNoTargetsKeyNeeded(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -2272,7 +2240,7 @@ func TestPublishTargetsDelgationNoTargetsKeyNeeded(t *testing.T) { } // just always check signing now, we've already established we can publish - // delgations with and without the metadata and key cache + // delegations with and without the metadata and key cache var rec *passRoleRecorder repo, rec = newRepoToTestRepo(t, repo, false) @@ -2300,7 +2268,7 @@ func TestPublishTargetsDelgationNoTargetsKeyNeeded(t *testing.T) { // them before publish. // - owner of a repo may not have the delegated keys, so can't sign a delegated // role -func TestPublishTargetsDelgationSuccessNeedsToDownloadRoles(t *testing.T) { +func TestPublishTargetsDelegationSuccessNeedsToDownloadRoles(t *testing.T) { gun := "docker.com/notary" ts := fullTestServer(t) defer ts.Close() @@ -2350,7 +2318,7 @@ func TestPublishTargetsDelgationSuccessNeedsToDownloadRoles(t *testing.T) { // Ensure that two clients can publish delegations with two different keys and // the changes will not clobber each other. -func TestPublishTargetsDelgationFromTwoRepos(t *testing.T) { +func TestPublishTargetsDelegationFromTwoRepos(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -2421,7 +2389,7 @@ func TestPublishTargetsDelgationFromTwoRepos(t *testing.T) { // A client who could publish before can no longer publish once the owner // removes their delegation key from the delegation role. -func TestPublishRemoveDelgationKeyFromDelegationRole(t *testing.T) { +func TestPublishRemoveDelegationKeyFromDelegationRole(t *testing.T) { gun := "docker.com/notary" ts := fullTestServer(t) defer ts.Close() @@ -2480,7 +2448,7 @@ func TestPublishRemoveDelgationKeyFromDelegationRole(t *testing.T) { // A client who could publish before can no longer publish once the owner // deletes the delegation -func TestPublishRemoveDelgation(t *testing.T) { +func TestPublishRemoveDelegation(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -3107,12 +3075,12 @@ func TestBootstrapClientInvalidURL(t *testing.T) { require.EqualError(t, err, err2.Error()) } -func TestPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t *testing.T) { - testPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t, false) - testPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t, true) +func TestPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T) { + testPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t, false) + testPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t, true) } -func testPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t *testing.T, x509 bool) { +func testPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T, x509 bool) { gun := "docker.com/notary" ts := fullTestServer(t) defer ts.Close() diff --git a/client/helpers.go b/client/helpers.go index 46648a11b5..8a5e715206 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -4,7 +4,6 @@ import ( "encoding/json" "fmt" "net/http" - "path" "strings" "time" @@ -130,22 +129,6 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error { } -// applies a function repeatedly, falling back on the parent role, until it no -// longer can -func doWithRoleFallback(role string, doFunc func(string) error) error { - for role == data.CanonicalTargetsRole || data.IsDelegation(role) { - err := doFunc(role) - if err == nil { - return nil - } - if _, ok := err.(data.ErrInvalidRole); !ok { - return err - } - role = path.Dir(role) - } - return data.ErrInvalidRole{Role: role} -} - func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error { var err error switch c.Action() { @@ -158,21 +141,16 @@ func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error { } files := data.Files{c.Path(): *meta} - err = doWithRoleFallback(c.Scope(), func(role string) error { - _, e := repo.AddTargets(role, files) - return e - }) - if err != nil { + // Attempt to add the target to this role, with no fallback + if _, err = repo.AddTargets(c.Scope(), files); err != nil { logrus.Errorf("couldn't add target to %s: %s", c.Scope(), err.Error()) } case changelist.ActionDelete: logrus.Debug("changelist remove: ", c.Path()) - err = doWithRoleFallback(c.Scope(), func(role string) error { - return repo.RemoveTargets(role, c.Path()) - }) - if err != nil { + // Attempt to remove the target from this role, with no fallback + if err = repo.RemoveTargets(c.Scope(), c.Path()); err != nil { logrus.Errorf("couldn't remove target from %s: %s", c.Scope(), err.Error()) } diff --git a/client/helpers_test.go b/client/helpers_test.go index 3df3b46315..0bb8108101 100644 --- a/client/helpers_test.go +++ b/client/helpers_test.go @@ -864,8 +864,8 @@ func TestApplyChangelistTargetsToMultipleRoles(t *testing.T) { require.False(t, ok, "no change to targets/level2, so metadata not created") } -// ApplyTargets falls back to role that exists when adding or deleting a change -func TestApplyChangelistTargetsFallbackRoles(t *testing.T) { +// ApplyTargets does not fall back to role that exists when adding or deleting a change to a nonexistent delegation +func TestApplyChangelistTargetsDoesNotFallbackRoles(t *testing.T) { repo, _, err := testutils.EmptyRepo("docker.com/notary") require.NoError(t, err) @@ -887,12 +887,11 @@ func TestApplyChangelistTargetsFallbackRoles(t *testing.T) { ChangePath: "latest", Data: fjson, })) + err = applyChangelist(repo, cl) + require.Error(t, err) + require.IsType(t, data.ErrInvalidRole{}, err) - require.NoError(t, applyChangelist(repo, cl)) - _, ok := repo.Targets[data.CanonicalTargetsRole].Signed.Targets["latest"] - require.True(t, ok) - - // now delete and require it applies to + // now try a delete and assert the same error cl = changelist.NewMemChangelist() require.NoError(t, cl.Add(&changelist.TufChange{ Actn: changelist.ActionDelete, @@ -902,12 +901,13 @@ func TestApplyChangelistTargetsFallbackRoles(t *testing.T) { Data: nil, })) - require.NoError(t, applyChangelist(repo, cl)) - require.Empty(t, repo.Targets[data.CanonicalTargetsRole].Signed.Targets) + err = applyChangelist(repo, cl) + require.Error(t, err) + require.IsType(t, data.ErrInvalidRole{}, err) } -// changeTargetMeta fallback fails with ErrInvalidRole if role is invalid -func TestChangeTargetMetaFallbackFailsInvalidRole(t *testing.T) { +// changeTargetMeta fails with ErrInvalidRole if role is invalid +func TestChangeTargetMetaFailsInvalidRole(t *testing.T) { repo, _, err := testutils.EmptyRepo("docker.com/notary") require.NoError(t, err) @@ -932,9 +932,8 @@ func TestChangeTargetMetaFallbackFailsInvalidRole(t *testing.T) { require.IsType(t, data.ErrInvalidRole{}, err) } -// If applying a change fails due to a prefix error, it does not fall back -// on the parent. -func TestChangeTargetMetaDoesntFallbackIfPrefixError(t *testing.T) { +// If applying a change fails due to a prefix error, changeTargetMeta fails outright +func TestChangeTargetMetaFailsIfPrefixError(t *testing.T) { repo, cs, err := testutils.EmptyRepo("docker.com/notary") require.NoError(t, err) diff --git a/docs/advanced_usage.md b/docs/advanced_usage.md index b86d41262d..a2fe3e3ae7 100644 --- a/docs/advanced_usage.md +++ b/docs/advanced_usage.md @@ -227,7 +227,7 @@ other delegation roles will be supported in a future release. By default, Engine `targets/releases` role if it exists; if it does not exist, the Engine will fall back to retrieving images signed by the default `targets` role. -To use the `targets/releases` role for pushing and pulling images with content trust, follow the steps above to add and publish the delegation role with notary. When adding the delegation, the `--all-paths` flag should be used. By default, pushing with Docker Content Trust will attempt to add to the `targets/releases` role and will fallback to `targets` _only_ if the delegation does not exist. +To use the `targets/releases` role for pushing and pulling images with content trust, follow the steps above to add and publish the delegation role with notary. When adding the delegation, the `--all-paths` flag should be used. By default, pushing with Docker Content Trust will attempt to add to whichever delegations exist directly under targets (ex: `targets/releases`, but not `targets/nested/delegation`). # Files and state on disk From b65723fce32970756315a1bcf448e8673c54a232 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Wed, 9 Mar 2016 16:43:05 -0800 Subject: [PATCH 2/2] Remove mentions of fallback Signed-off-by: Riyaz Faizullabhoy --- client/helpers.go | 4 ++-- client/helpers_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/client/helpers.go b/client/helpers.go index 8a5e715206..38f1b43cda 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -141,7 +141,7 @@ func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error { } files := data.Files{c.Path(): *meta} - // Attempt to add the target to this role, with no fallback + // Attempt to add the target to this role if _, err = repo.AddTargets(c.Scope(), files); err != nil { logrus.Errorf("couldn't add target to %s: %s", c.Scope(), err.Error()) } @@ -149,7 +149,7 @@ func changeTargetMeta(repo *tuf.Repo, c changelist.Change) error { case changelist.ActionDelete: logrus.Debug("changelist remove: ", c.Path()) - // Attempt to remove the target from this role, with no fallback + // Attempt to remove the target from this role if err = repo.RemoveTargets(c.Scope(), c.Path()); err != nil { logrus.Errorf("couldn't remove target from %s: %s", c.Scope(), err.Error()) } diff --git a/client/helpers_test.go b/client/helpers_test.go index 0bb8108101..d10a39984d 100644 --- a/client/helpers_test.go +++ b/client/helpers_test.go @@ -864,8 +864,8 @@ func TestApplyChangelistTargetsToMultipleRoles(t *testing.T) { require.False(t, ok, "no change to targets/level2, so metadata not created") } -// ApplyTargets does not fall back to role that exists when adding or deleting a change to a nonexistent delegation -func TestApplyChangelistTargetsDoesNotFallbackRoles(t *testing.T) { +// ApplyTargets fails when adding or deleting a change to a nonexistent delegation +func TestApplyChangelistTargetsFailsNonexistentRole(t *testing.T) { repo, _, err := testutils.EmptyRepo("docker.com/notary") require.NoError(t, err)