Address comments from group review: 0.3 rotation compatibility and cl…

…eanup, add tests

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

server/snapshot/snapshot.go

@@ -24,11 +24,7 @@ func GetOrCreateSnapshotKey(gun string, store storage.MetaStore, crypto signed.C
 			logrus.Errorf("Error when retrieving root role for GUN %s: %v", gun, err)
 			return nil, err
 		}
-		key, err := crypto.Create(data.CanonicalSnapshotRole, gun, createAlgorithm)
-		if err != nil {
-			return nil, err
-		}
-		return key, nil
+		return crypto.Create(data.CanonicalSnapshotRole, gun, createAlgorithm)
 	}
 
 	// If we have a current root, parse out the public key for the snapshot role, and return it
@@ -44,12 +40,14 @@ func GetOrCreateSnapshotKey(gun string, store storage.MetaStore, crypto signed.C
 		return nil, err
 	}
 
-	// We currently only support single keys for snapshot and timestamp, so we can return the first and only key in the map
-	for _, pubKey := range snapshotRole.Keys {
-		return pubKey, nil
+	// We currently only support single keys for snapshot and timestamp, so we can return the first and only key in the map if the signer has it
+	for keyID := range snapshotRole.Keys {
+		if pubKey := crypto.GetKey(keyID); pubKey != nil {
+			return pubKey, nil
+		}
 	}
-	logrus.Errorf("Failed to find any snapshot keys in saved root for GUN %s", gun)
-	return nil, err
+	logrus.Debugf("Failed to find any snapshot keys in cryptosigner from root for GUN %s, generating new key", gun)
+	return crypto.Create(data.CanonicalSnapshotRole, gun, createAlgorithm)
 }
 
 // RotateSnapshotKey attempts to rotate a snapshot key in the signer, but might be rate-limited by the signer

server/snapshot/snapshot_test.go

@@ -92,8 +92,7 @@ func TestGetSnapshotKeyCreateWithInvalidAlgo(t *testing.T) {
 	require.Nil(t, k, "Key should be nil")
 }
 
-func TestGetSnapshotKeyExisting(t *testing.T) {
-
+func TestGetSnapshotKeyExistingMetadata(t *testing.T) {
 	repo, crypto, err := testutils.EmptyRepo("gun")
 	require.NoError(t, err)
 
@@ -122,6 +121,14 @@ func TestGetSnapshotKeyExisting(t *testing.T) {
 
 	require.Equal(t, k, k2, "Did not receive same key when attempting to recreate.")
 	require.NotNil(t, k2, "Key should not be nil")
+
+	// try wiping out the cryptoservice data, and ensure we create a new key because the signer doesn't hold the key specified by TUF
+	crypto = signed.NewEd25519()
+	k3, err := GetOrCreateSnapshotKey("gun", store, crypto, data.ED25519Key)
+	require.Nil(t, err, "Expected nil error")
+	require.NotEqual(t, k, k3, "Did not receive same key when attempting to recreate.")
+	require.NotEqual(t, k2, k3, "Did not receive same key when attempting to recreate.")
+	require.NotNil(t, k3, "Key should not be nil")
 }
 
 // If there is no previous snapshot or the previous snapshot is corrupt, then

server/timestamp/timestamp.go

@@ -29,11 +29,7 @@ func GetOrCreateTimestampKey(gun string, store storage.MetaStore, crypto signed.
 			logrus.Errorf("Error when retrieving root role for GUN %s: %v", gun, err)
 			return nil, err
 		}
-		key, err := crypto.Create(data.CanonicalTimestampRole, gun, createAlgorithm)
-		if err != nil {
-			return nil, err
-		}
-		return key, nil
+		return crypto.Create(data.CanonicalTimestampRole, gun, createAlgorithm)
 	}
 
 	// If we have a current root, parse out the public key for the timestamp role, and return it
@@ -49,12 +45,14 @@ func GetOrCreateTimestampKey(gun string, store storage.MetaStore, crypto signed.
 		return nil, err
 	}
 
-	// We currently only support single keys for snapshot and timestamp, so we can return the first and only key in the map
-	for _, pubKey := range timestampRole.Keys {
-		return pubKey, nil
+	// We currently only support single keys for snapshot and timestamp, so we can return the first and only key in the map if the signer has it
+	for keyID := range timestampRole.Keys {
+		if pubKey := crypto.GetKey(keyID); pubKey != nil {
+			return pubKey, nil
+		}
 	}
-	logrus.Errorf("Failed to find any timestamp keys in saved root for GUN %s", gun)
-	return nil, err
+	logrus.Debugf("Failed to find any timestamp keys in cryptosigner from root for GUN %s, generating new key", gun)
+	return crypto.Create(data.CanonicalTimestampRole, gun, createAlgorithm)
 }
 
 // RotateTimestampKey attempts to rotate a timestamp key in the signer, but might be rate-limited by the signer

server/timestamp/timestamp_test.go

@@ -266,3 +266,42 @@ func TestGetSnapshotKeyCreateWithInvalidAlgo(t *testing.T) {
 	require.Error(t, err, "Expected error")
 	require.Nil(t, k, "Key should be nil")
 }
+
+func TestGetTimestampKeyExistingMetadata(t *testing.T) {
+	repo, crypto, err := testutils.EmptyRepo("gun")
+	require.NoError(t, err)
+
+	sgnd, err := repo.SignRoot(data.DefaultExpires(data.CanonicalRootRole))
+	require.NoError(t, err)
+	rootJSON, err := json.Marshal(sgnd)
+	require.NoError(t, err)
+	store := storage.NewMemStorage()
+	require.NoError(t,
+		store.UpdateCurrent("gun", storage.MetaUpdate{Role: data.CanonicalRootRole, Version: 0, Data: rootJSON}))
+
+	timestampRole, err := repo.Root.BuildBaseRole(data.CanonicalTimestampRole)
+	require.NoError(t, err)
+	key, ok := timestampRole.Keys[repo.Root.Signed.Roles[data.CanonicalTimestampRole].KeyIDs[0]]
+	require.True(t, ok)
+
+	k, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
+	require.Nil(t, err, "Expected nil error")
+	require.NotNil(t, k, "Key should not be nil")
+	require.Equal(t, key, k, "Did not receive same key when attempting to recreate.")
+	require.NotNil(t, k, "Key should not be nil")
+
+	k2, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
+
+	require.Nil(t, err, "Expected nil error")
+
+	require.Equal(t, k, k2, "Did not receive same key when attempting to recreate.")
+	require.NotNil(t, k2, "Key should not be nil")
+
+	// try wiping out the cryptoservice data, and ensure we create a new key because the signer doesn't hold the key specified by TUF
+	crypto = signed.NewEd25519()
+	k3, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
+	require.Nil(t, err, "Expected nil error")
+	require.NotEqual(t, k, k3, "Did not receive same key when attempting to recreate.")
+	require.NotEqual(t, k2, k3, "Did not receive same key when attempting to recreate.")
+	require.NotNil(t, k3, "Key should not be nil")
+}

signer/keydbstore/rethink_keydbstore.go

@@ -283,6 +283,7 @@ func (rdb RethinkDBKeyStore) Create(role, gun, algorithm string) (data.PublicKey
 		Filter(gorethink.Row.Field("role").Eq(role)).
 		Filter(gorethink.Row.Field("algorithm").Eq(algorithm)).
 		Filter(gorethink.Row.Field("last_used").Eq(time.Time{})).
+		OrderBy(gorethink.Row.Field("key_id")).
 		Run(rdb.sess)
 	if err != nil {
 		return nil, err

signer/keydbstore/sql_keydbstore.go

@@ -202,7 +202,7 @@ func (s *SQLKeyDBStore) markActive(keyID string) error {
 func (s *SQLKeyDBStore) Create(role, gun, algorithm string) (data.PublicKey, error) {
 	// If an unused key exists, simply return it.  Else, error because SQL can't make keys
 	dbPrivateKey := GormPrivateKey{}
-	if !s.db.Model(GormPrivateKey{}).Where("role = ? AND gun = ? AND algorithm = ? AND last_used IS NULL", role, gun, algorithm).First(&dbPrivateKey).RecordNotFound() {
+	if !s.db.Model(GormPrivateKey{}).Where("role = ? AND gun = ? AND algorithm = ? AND last_used IS NULL", role, gun, algorithm).Order("key_id").First(&dbPrivateKey).RecordNotFound() {
 		// Just return the public key component if we found one
 		return data.NewPublicKey(dbPrivateKey.Algorithm, []byte(dbPrivateKey.Public)), nil
 	}

tuf/signed/ed25519.go

@@ -96,7 +96,10 @@ func (e *Ed25519) PublicKeys(keyIDs ...string) (map[string]data.PublicKey, error
 
 // GetKey returns a single public key based on the ID
 func (e *Ed25519) GetKey(keyID string) data.PublicKey {
-	return data.PublicKeyFromPrivate(e.keys[keyID].privKey)
+	if privKey, _, err := e.GetPrivateKey(keyID); err == nil {
+		return data.PublicKeyFromPrivate(privKey)
+	}
+	return nil
 }
 
 // GetPrivateKey returns a single private key and role if present, based on the ID

tuf/signed/ed25519_test.go

@@ -22,3 +22,29 @@ func TestListKeys(t *testing.T) {
 
 	require.Len(t, c.ListKeys(data.CanonicalTargetsRole), 0)
 }
+
+// GetKey and GetPrivateKey only gets keys that we've added to this service
+func TestGetKeys(t *testing.T) {
+	c := NewEd25519()
+	tskey, err := c.Create(data.CanonicalTimestampRole, "", data.ED25519Key)
+	require.NoError(t, err)
+
+	pubKey := c.GetKey(tskey.ID())
+	require.NotNil(t, pubKey)
+	require.Equal(t, tskey.Public(), pubKey.Public())
+	require.Equal(t, tskey.Algorithm(), pubKey.Algorithm())
+	require.Equal(t, tskey.ID(), pubKey.ID())
+
+	privKey, role, err := c.GetPrivateKey(tskey.ID())
+	require.NoError(t, err)
+	require.Equal(t, data.CanonicalTimestampRole, role)
+	require.Equal(t, tskey.Public(), privKey.Public())
+	require.Equal(t, tskey.Algorithm(), privKey.Algorithm())
+	require.Equal(t, tskey.ID(), privKey.ID())
+
+	// if the key doesn't exist, GetKey returns nil and GetPrivateKey errors out
+	randomKey := c.GetKey("someID")
+	require.Nil(t, randomKey)
+	_, _, err = c.GetPrivateKey("someID")
+	require.Error(t, err)
+}