Rotate remote key endpoint

client/client.go

@@ -929,7 +929,7 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 	)
 	switch serverManagesKey {
 	case true:
-		pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
+		pubKey, err = rotateRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
 		errFmtMsg = "unable to rotate remote key: %s"
 	default:
 		pubKey, err = r.CryptoService.Create(role, r.gun, data.ECDSAKey)

client/client_test.go

@@ -2580,9 +2580,17 @@ func TestRotateKeyInvalidRole(t *testing.T) {
 	require.Error(t, repo.RotateKey("targets/releases", false),
 		"Rotating a delegation key should fail")
 
+	// rotating a delegation key to the server also fails
+	require.Error(t, repo.RotateKey("targets/releases", true),
+		"Rotating a delegation key should fail")
+
 	// rotating a not a real role key fails
 	require.Error(t, repo.RotateKey("nope", false),
 		"Rotating a non-real role key should fail")
+
+	// rotating a not a real role key to the server also fails
+	require.Error(t, repo.RotateKey("nope", true),
+		"Rotating a non-real role key should fail")
 }
 
 // If remotely rotating key fails, the failure is propagated
@@ -2595,9 +2603,59 @@ func TestRemoteRotationError(t *testing.T) {
 	ts.Close()
 
 	// server has died, so this should fail
+	for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} {
+		err := repo.RotateKey(role, true)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "unable to rotate remote key")
+	}
+}
+
+// If remotely rotating key fails for any reason, fail the rotation entirely
+func TestRemoteRotationEndpointError(t *testing.T) {
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
+	defer os.RemoveAll(repo.baseDir)
+
+	// simpleTestServer has no rotate key endpoint, so this should fail
+	for _, role := range []string{data.CanonicalSnapshotRole, data.CanonicalTimestampRole} {
+		err := repo.RotateKey(role, true)
+		require.Error(t, err)
+		require.IsType(t, store.ErrMetaNotFound{}, err)
+	}
+}
+
+// The rotator is not the owner of the repository, they cannot rotate the remote
+// key
+func TestRemoteRotationNoRootKey(t *testing.T) {
+	ts := fullTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
+	defer os.RemoveAll(repo.baseDir)
+	require.NoError(t, repo.Publish())
+
+	newRepo, _ := newRepoToTestRepo(t, repo, true)
+	defer os.RemoveAll(newRepo.baseDir)
+	_, err := newRepo.ListTargets()
+	require.NoError(t, err)
+
+	err = newRepo.RotateKey(data.CanonicalSnapshotRole, true)
+	require.Error(t, err)
+	require.IsType(t, signed.ErrInsufficientSignatures{}, err)
+}
+
+// The repo hasn't been initialized, so we can't rotate
+func TestRemoteRotationNonexistentRepo(t *testing.T) {
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	repo := newBlankRepo(t, ts.URL)
+	defer os.RemoveAll(repo.baseDir)
+
 	err := repo.RotateKey(data.CanonicalTimestampRole, true)
 	require.Error(t, err)
-	require.Contains(t, err.Error(), "unable to rotate remote key")
+	require.IsType(t, ErrRepoNotInitialized{}, err)
 }
 
 // Rotates the keys.  After the rotation, downloading the latest metadata
@@ -2732,6 +2790,14 @@ func TestRotateKeyAfterPublishServerManagementChange(t *testing.T) {
 		data.CanonicalTargetsRole:  false,
 		data.CanonicalRootRole:     false,
 	})
+	// check that the snapshot remote rotation creates new keys
+	testRotateKeySuccess(t, true, map[string]bool{
+		data.CanonicalSnapshotRole: true,
+	})
+	// check that the timestamp remote rotation creates new keys
+	testRotateKeySuccess(t, false, map[string]bool{
+		data.CanonicalTimestampRole: true,
+	})
 	// reclaim snapshot key management from the server
 	testRotateKeySuccess(t, true, map[string]bool{
 		data.CanonicalSnapshotRole: false,
@@ -2761,13 +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)
-		}
-	}
 }
 
 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/helpers.go

@@ -236,6 +236,25 @@ func getRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey,
 	return pubKey, nil
 }
 
+// Rotates a private key in a remote store and returns the public key component
+func rotateRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey, error) {
+	remote, err := getRemoteStore(url, gun, rt)
+	if err != nil {
+		return nil, err
+	}
+	rawPubKey, err := remote.RotateKey(role)
+	if err != nil {
+		return nil, err
+	}
+
+	pubKey, err := data.UnmarshalPublicKey(rawPubKey)
+	if err != nil {
+		return nil, err
+	}
+
+	return pubKey, nil
+}
+
 // signs and serializes the metadata for a canonical role in a TUF repo to JSON
 func serializeCanonicalRole(tufRepo *tuf.Repo, role string) (out []byte, err error) {
 	var s *data.Signed

client/helpers_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/json"
+	"net/http"
 	"testing"
 	"time"
 
@@ -1019,3 +1020,15 @@ func TestAllNotNearExpiry(t *testing.T) {
 	require.NotContains(t, a.String(), "snapshot is nearing expiry, you should re-sign the role metadata", "Snapshot should not show near expiry")
 	require.NotContains(t, a.String(), "timestamp", "there should be no logrus warnings pertaining to timestamp")
 }
+
+func TestRotateRemoteKeyOffline(t *testing.T) {
+	// without a valid roundtripper, rotation should fail since we cannot initialize a HTTPStore
+	key, err := rotateRemoteKey("invalidURL", "gun", data.CanonicalSnapshotRole, nil)
+	require.Error(t, err)
+	require.Nil(t, key)
+
+	// if the underlying remote store is faulty and cannot rotate keys, we should get back the error
+	key, err = rotateRemoteKey("https://notary-server", "gun", data.CanonicalSnapshotRole, http.DefaultTransport)
+	require.Error(t, err)
+	require.Nil(t, key)
+}

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