Merge pull request #616 from docker/remove-role-fallback

Remove delegation role fallback when applying targets changes

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
+// this is just a sanity test to make sure Publish calls it correctly
-// no fallback happens.
 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
+// is a role but no key, publish should just fail outright.
-// doesn't work, it falls back on its parent, and so forth, and eventually
+func TestPublishTargetsDelegationScopeFailIfNoKeys(t *testing.T) {
-// falls back on publishing to "target".  This *only* falls back if the role
+	testPublishTargetsDelegationScopeFailIfNoKeys(t, true)
-// doesn't exist, not if the user doesn't have a key.  (different test)
+	testPublishTargetsDelegationScopeFailIfNoKeys(t, false)
-func TestPublishTargetsDelgationScopeFallback(t *testing.T) {
-	testPublishTargetsDelgationScopeFallback(t, true)
-	testPublishTargetsDelgationScopeFallback(t, false)
 }
 
-func testPublishTargetsDelgationScopeFallback(t *testing.T, clearCache bool) {
+func testPublishTargetsDelegationScopeFailIfNoKeys(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) {
 	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) {
+func TestPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T) {
-	testPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t, false)
+	testPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t, false)
-	testPublishTargetsDelgationCanUseUserKeyWithArbitraryRole(t, true)
+	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()

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 {
+		// Attempt to add the target to this role
-			_, e := repo.AddTargets(role, files)
+		if _, err = repo.AddTargets(c.Scope(), files); err != nil {
-			return e
-		})
-		if 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 {
+		// Attempt to remove the target from this role
-			return repo.RemoveTargets(role, c.Path())
+		if err = repo.RemoveTargets(c.Scope(), c.Path()); err != nil {
-		})
-		if err != nil {
 			logrus.Errorf("couldn't remove target from %s: %s", c.Scope(), err.Error())
 		}
 

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
+// ApplyTargets fails when adding or deleting a change to a nonexistent delegation
-func TestApplyChangelistTargetsFallbackRoles(t *testing.T) {
+func TestApplyChangelistTargetsFailsNonexistentRole(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))
+	// now try a delete and assert the same error
-	_, ok := repo.Targets[data.CanonicalTargetsRole].Signed.Targets["latest"]
-	require.True(t, ok)
-
-	// now delete and require it applies to
 	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))
+	err = applyChangelist(repo, cl)
-	require.Empty(t, repo.Targets[data.CanonicalTargetsRole].Signed.Targets)
+	require.Error(t, err)
+	require.IsType(t, data.ErrInvalidRole{}, err)
 }
 
-// changeTargetMeta fallback fails with ErrInvalidRole if role is invalid
+// changeTargetMeta fails with ErrInvalidRole if role is invalid
-func TestChangeTargetMetaFallbackFailsInvalidRole(t *testing.T) {
+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
+// If applying a change fails due to a prefix error, changeTargetMeta fails outright
-// on the parent.
+func TestChangeTargetMetaFailsIfPrefixError(t *testing.T) {
-func TestChangeTargetMetaDoesntFallbackIfPrefixError(t *testing.T) {
 	repo, cs, err := testutils.EmptyRepo("docker.com/notary")
 	require.NoError(t, err)
 

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