Do not sign the actual targets metadata unless it's dirty.

Previously we were always signing it, but we can't do that anymore
because then delegated users won't be able to publish ever (they
probably don't have the target key).

Some other related changes: when role keys are rotated, that role
needs to be marked as dirty now in order to be re-signed and
published.

Signed-off-by: Ying Li <ying.li@docker.com>

client/client.go

@@ -584,10 +584,9 @@ func (r *NotaryRepository) Publish() error {
 		updatedFiles[data.CanonicalRootRole] = rootJSON
 	}
 
-	// iterate through all the targets files - if they are dirty, or if they
-	// are the canonical target role, then sign and update
+	// iterate through all the targets files - if they are dirty, sign and update
 	for roleName, roleObj := range r.tufRepo.Targets {
-		if roleName == data.CanonicalTargetsRole || roleObj.Dirty {
+		if roleObj.Dirty {
 			targetsJSON, err := serializeCanonicalRole(r.tufRepo, roleName)
 			if err != nil {
 				return err

client/client_test.go

@@ -1550,7 +1550,7 @@ func TestPublishTargetsDelgationScopeNoFallbackIfNoKeys(t *testing.T) {
 	assert.Len(t, getChanges(t, repo), 1, "wrong number of changelist files found")
 
 	// Now Publish should fail
-	assert.NoError(t, repo.Publish())
+	assert.Error(t, repo.Publish())
 	assert.Len(t, getChanges(t, repo), 1, "wrong number of changelist files found")
 
 	targets, err := repo.ListTargets("targets", "targets/a", "targets/a/b")
@@ -1607,24 +1607,31 @@ func TestPublishTargetsDelgationSuccessNeedsToDownloadRoles(t *testing.T) {
 	defer ts.Close()
 
 	// this is the original repo - it owns the root/targets keys and creates
-	// the delegation
-	ownerRepo, _ := initializeRepo(t, data.ECDSAKey, tempDirs[0], gun, ts.URL, false)
+	// the delegation to which it doesn't have the key (so server snapshot
+	// signing would be required)
+	ownerRepo, _ := initializeRepo(t, data.ECDSAKey, tempDirs[0], gun, ts.URL, true)
 	// this is a user, or otherwise a repo that only has access to the delegation
 	// key so it can publish targets to the delegated role
 	delgRepo, err := NewNotaryRepository(tempDirs[1], gun, ts.URL,
 		http.DefaultTransport, passphraseRetriever)
 	assert.NoError(t, err, "error creating repository: %s", err)
 
-	// create delegated key on the delegated repo
-	delgKey, err := delgRepo.CryptoService.Create("targets/a", data.ECDSAKey)
+	// create a key on the owner repo
+	aKey, err := ownerRepo.CryptoService.Create("targets/a", data.ECDSAKey)
+	assert.NoError(t, err, "error creating delegation key")
+
+	// create a key on the delegated repo
+	bKey, err := delgRepo.CryptoService.Create("targets/a/b", data.ECDSAKey)
 	assert.NoError(t, err, "error creating delegation key")
 
 	// owner creates delegations, adds the delegated key to them, and publishes them
-	for _, delgName := range []string{"targets/a", "targets/a/b"} {
-		assert.NoError(t,
-			ownerRepo.AddDelegation(delgName, 1, []data.PublicKey{delgKey}),
-			"error creating delegation")
-	}
+	assert.NoError(t,
+		ownerRepo.AddDelegation("targets/a", 1, []data.PublicKey{aKey}),
+		"error creating delegation")
+	assert.NoError(t,
+		ownerRepo.AddDelegation("targets/a/b", 1, []data.PublicKey{bKey}),
+		"error creating delegation")
+
 	assert.NoError(t, ownerRepo.Publish())
 
 	// delegated repo now publishes to delegated roles, but it will need

tuf/tuf.go

@@ -99,8 +99,25 @@ func (tr *Repo) AddBaseKeys(role string, keys ...data.PublicKey) error {
 	}
 	tr.keysDB.AddRole(r)
 	tr.Root.Dirty = true
-	return nil
 
+	// also, whichever role was switched out needs to be re-signed
+	// root has already been marked dirty
+	switch role {
+	case data.CanonicalSnapshotRole:
+		if tr.Snapshot != nil {
+			tr.Snapshot.Dirty = true
+		}
+	case data.CanonicalTargetsRole:
+		target, ok := tr.Targets[data.CanonicalTargetsRole]
+		if ok {
+			target.Dirty = true
+		}
+	case data.CanonicalTimestampRole:
+		if tr.Timestamp != nil {
+			tr.Timestamp.Dirty = true
+		}
+	}
+	return nil
 }
 
 // ReplaceBaseKeys is used to replace all keys for the given role with the new keys

tuf/tuf_test.go

@@ -317,7 +317,7 @@ func TestUpdateDelegationsReplaceRole(t *testing.T) {
 
 	// create one now to assert that replacing the delegation doesn't delete the
 	// metadata
-	repo.Targets["targets/test"] = data.NewTargets()
+	repo.InitTargets("targets/test")
 
 	// create another role with the same name and ensure it replaces the
 	// previous role
@@ -404,7 +404,7 @@ func TestDeleteDelegations(t *testing.T) {
 	assert.Equal(t, testKey.ID(), keyIDs[0])
 
 	// ensure that the metadata is there
-	repo.Targets["targets/test"] = data.NewTargets()
+	repo.InitTargets("targets/test")
 
 	err = repo.DeleteDelegation(*role)
 	assert.Len(t, r.Signed.Delegations.Roles, 0)
@@ -551,7 +551,7 @@ func TestGetDelegationRoleAndMetadataExistDelegationDoesntExists(t *testing.T) {
 	assert.NoError(t, repo.UpdateDelegations(role, data.KeyList{testKey}))
 
 	// ensure metadata exists
-	repo.Targets["targets/level1"] = data.NewTargets()
+	repo.InitTargets("targets/level1")
 
 	_, err = repo.GetDelegation("targets/level1/level2")
 	assert.Error(t, err)
@@ -674,3 +674,33 @@ func TestAddTargetsRoleDoesntExist(t *testing.T) {
 	assert.Error(t, err)
 	assert.IsType(t, data.ErrInvalidRole{}, err)
 }
+
+// adding a key to a role marks root as dirty as well as the role
+func TestAddBaseKeysToRoot(t *testing.T) {
+	for role := range data.ValidRoles {
+		ed25519 := signed.NewEd25519()
+		keyDB := keys.NewDB()
+		repo := initRepo(t, ed25519, keyDB)
+
+		key, err := ed25519.Create(role, data.ED25519Key)
+		assert.NoError(t, err)
+
+		assert.Len(t, repo.Root.Signed.Roles[role].KeyIDs, 1)
+
+		assert.NoError(t, repo.AddBaseKeys(role, key))
+
+		_, ok := repo.Root.Signed.Keys[key.ID()]
+		assert.True(t, ok)
+		assert.Len(t, repo.Root.Signed.Roles[role].KeyIDs, 2)
+		assert.True(t, repo.Root.Dirty)
+
+		switch role {
+		case data.CanonicalSnapshotRole:
+			assert.True(t, repo.Snapshot.Dirty)
+		case data.CanonicalTargetsRole:
+			assert.True(t, repo.Targets[data.CanonicalTargetsRole].Dirty)
+		case data.CanonicalTimestampRole:
+			assert.True(t, repo.Timestamp.Dirty)
+		}
+	}
+}