Fix timestamp update logic

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

client/client_test.go

@@ -2827,18 +2827,6 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool,
 	// Get root.json and capture targets + snapshot key IDs
 	_, err := repo.GetTargetByName("latest")
 	require.NoError(t, err)
-
-	var keysToExpectCreated []string
-	for role, serverManaged := range keysToRotate {
-		if !serverManaged {
-			keysToExpectCreated = append(keysToExpectCreated, role)
-		}
-	}
-
-	for _, role := range keysToExpectCreated {
-		// We can't tell which ID in particular to expect from just the role, so check we have at least one key for this role on disk
-		require.True(t, len(repo.CryptoService.ListKeys(role)) > 0, fmt.Sprintf("could not find key on disk for role %s", role))
-	}
 }
 
 func logRepoTrustRoot(t *testing.T, prefix string, repo *NotaryRepository) {

client/client_update_test.go

@@ -1058,8 +1058,8 @@ func TestUpdateNonRootRemoteCorruptedNoLocalCache(t *testing.T) {
 // Having a local cache, if the server has the same data (timestamp has not changed),
 // should succeed in all cases if whether forWrite (force check) is true or not.
 // If the timestamp is fine, it hasn't changed and we don't have to download
-// anything. If it's broken, we used the cached timestamp and again download
-// nothing.
+// anything. If it's broken, we used the cached timestamp only if the error on
+// downloading the new one was not validation related
 func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping test in short mode")
@@ -1070,10 +1070,18 @@ func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) {
 			continue
 		}
 		for _, testData := range waysToMessUpServer {
-			testUpdateRemoteCorruptValidChecksum(t, updateOpts{
-				localCache: true,
-				role:       role,
-			}, testData, false)
+			// remote timestamp swizzling will fail the update
+			if role == data.CanonicalTimestampRole {
+				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
+					localCache: true,
+					role:       role,
+				}, testData, testData.desc != "insufficient signatures")
+			} else {
+				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
+					localCache: true,
+					role:       role,
+				}, testData, false)
+			}
 		}
 	}
 	for role, expectations := range waysToMessUpServerNonRootPerRole(t) {
@@ -1094,7 +1102,7 @@ func TestUpdateNonRootRemoteCorruptedCanUseLocalCache(t *testing.T) {
 				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
 					localCache: true,
 					role:       role,
-				}, testData, false)
+				}, testData, true)
 			case "targets/a":
 				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
 					localCache: true,
@@ -1137,19 +1145,6 @@ func TestUpdateNonRootRemoteCorruptedCannotUseLocalCache(t *testing.T) {
 		switch role {
 		case data.CanonicalRootRole:
 			break
-		case data.CanonicalTimestampRole:
-			for _, testData := range waysToMessUpServer {
-				// in general the cached timsestamp will always succeed, but if the threshold has been
-				// increased, it fails because when we download the new timestamp, it validates as per our
-				// previous root.  But the root hash doesn't match.  So we download a new root and
-				// try the update again.  In this case, both the old and new timestamps won't have enough
-				// signatures.
-				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
-					serverHasNewData: true,
-					localCache:       true,
-					role:             role,
-				}, testData, testData.desc == "insufficient signatures")
-			}
 		default:
 			for _, testData := range waysToMessUpServer {
 				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
@@ -1198,13 +1193,13 @@ func TestUpdateNonRootRemoteCorruptedCannotUseLocalCache(t *testing.T) {
 					role:             role,
 				}, testData, false)
 			case data.CanonicalTimestampRole:
-				// If the timestamp is invalid, we just default to the previous
-				// cached version of the timestamp, so the update succeeds
+				// we only default to the previous cached version of the timestamp if
+				// there is a network/storage error, so swizzling will fail the update
 				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
 					serverHasNewData: true,
 					localCache:       true,
 					role:             role,
-				}, testData, false)
+				}, testData, true)
 			case "targets/a":
 				testUpdateRemoteCorruptValidChecksum(t, updateOpts{
 					serverHasNewData: true,

client/tufclient.go

@@ -107,27 +107,34 @@ func (c *TUFClient) downloadTimestamp() error {
 	role := data.CanonicalTimestampRole
 	consistentInfo := c.newBuilder.GetConsistentInfo(role)
 
-	// get the cached timestamp, if it exists
+	// always get the remote timestamp, since it supersedes the local one
 	cachedTS, cachedErr := c.cache.GetSized(role, notary.MaxTimestampSize)
-	// always get the remote timestamp, since it supercedes the local one
 	_, remoteErr := c.tryLoadRemote(consistentInfo, cachedTS)
 
-	switch {
-	case remoteErr == nil:
+	// check that there was no remote error, or if there was a network problem
+	// If there was a validation error, we should error out so we can download a new root or fail the update
+	switch remoteErr.(type) {
+	case nil:
 		return nil
-	case cachedErr == nil:
-		logrus.Debug(remoteErr.Error())
-		logrus.Warn("Error while downloading remote metadata, using cached timestamp - this might not be the latest version available remotely")
-
-		err := c.newBuilder.Load(role, cachedTS, 1, false)
-		if err == nil {
-			logrus.Debug("successfully verified cached timestamp")
-		}
-		return err
+	case store.ErrMetaNotFound, store.ErrServerUnavailable:
+		break
 	default:
+		return remoteErr
+	}
+
+	// since it was a network error: get the cached timestamp, if it exists
+	if cachedErr != nil {
 		logrus.Debug("no cached or remote timestamp available")
 		return remoteErr
 	}
+
+	logrus.Warn("Error while downloading remote metadata, using cached timestamp - this might not be the latest version available remotely")
+	err := c.newBuilder.Load(role, cachedTS, 1, false)
+	if err == nil {
+		logrus.Debug("successfully verified cached timestamp")
+	}
+	return err
+
 }
 
 // downloadSnapshot is responsible for downloading the snapshot.json
@@ -220,7 +227,6 @@ func (c *TUFClient) tryLoadRemote(consistentInfo tuf.ConsistentInfo, old []byte)
 	// will be 1
 	c.oldBuilder.Load(consistentInfo.RoleName, old, 1, true)
 	minVersion := c.oldBuilder.GetLoadedVersion(consistentInfo.RoleName)
-
 	if err := c.newBuilder.Load(consistentInfo.RoleName, raw, minVersion, false); err != nil {
 		logrus.Debugf("downloaded %s is invalid: %s", consistentName, err)
 		return raw, err

cmd/notary/integration_test.go

@@ -1268,8 +1268,16 @@ func TestClientKeyGenerationRotation(t *testing.T) {
 	// publish using the new keys
 	output := assertSuccessfullyPublish(
 		t, tempDir, server.URL, "gun", target+"2", tempfiles[1])
-	// assert that the previous target is sitll there
+	// assert that the previous target is still there
 	require.True(t, strings.Contains(string(output), target))
+
+	// rotate the snapshot and timestamp keys on the server, multiple times
+	for i := 0; i < 10; i++ {
+		_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalSnapshotRole, "-r")
+		require.NoError(t, err)
+		_, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalTimestampRole, "-r")
+		require.NoError(t, err)
+	}
 }
 
 // Tests default root key generation

server/snapshot/snapshot.go

@@ -59,7 +59,7 @@ func RotateSnapshotKey(gun string, store storage.MetaStore, crypto signed.Crypto
 	if err != nil {
 		return nil, err
 	}
-	logrus.Debug("Created new pending snapshot key to rotate to for ", gun, ". With algo: ", key.Algorithm())
+	logrus.Debug("Created new pending snapshot key ", key.ID(), "to rotate to for ", gun, ". With algo: ", key.Algorithm())
 	return key, nil
 }
 

server/timestamp/timestamp.go

@@ -64,7 +64,7 @@ func RotateTimestampKey(gun string, store storage.MetaStore, crypto signed.Crypt
 	if err != nil {
 		return nil, err
 	}
-	logrus.Debug("Created new pending timestamp key to rotate to for ", gun, ". With algo: ", key.Algorithm())
+	logrus.Debug("Created new pending timestamp key ", key.ID(), "to rotate to for ", gun, ". With algo: ", key.Algorithm())
 	return key, nil
 }
 

tuf/builder.go

@@ -18,7 +18,7 @@ var ErrBuildDone = fmt.Errorf(
 	"the builder has finished building and cannot accept any more input or produce any more output")
 
 // ErrInvalidBuilderInput is returned when RepoBuilder.Load is called
-// with the wrong type of metadata for thes tate that it's in
+// with the wrong type of metadata for the state that it's in
 type ErrInvalidBuilderInput struct{ msg string }
 
 func (e ErrInvalidBuilderInput) Error() string {
@@ -349,7 +349,7 @@ func (rb *repoBuilder) GenerateTimestamp(prev *data.SignedTimestamp) ([]byte, in
 		return nil, 0, ErrInvalidBuilderInput{msg: "timestamp has already been loaded"}
 	}
 
-	// SignTimetamp always serializes the loaded snapshot and signs in the data, so we must always
+	// SignTimestamp always serializes the loaded snapshot and signs in the data, so we must always
 	// have the snapshot loaded first
 	if err := rb.checkPrereqsLoaded([]string{data.CanonicalRootRole, data.CanonicalSnapshotRole}); err != nil {
 		return nil, 0, err
@@ -422,7 +422,6 @@ func (rb *repoBuilder) loadRoot(content []byte, minVersion int, allowExpired boo
 	if err != nil { // this should never happen since the root has been validated
 		return err
 	}
-
 	rb.repo.Root = signedRoot
 	rb.repo.originalRootRole = rootRole
 	return nil