From 58f9de4805967c08485dc0c8799ac285f39d1893 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Mon, 8 Aug 2016 16:16:33 -0700 Subject: [PATCH] Address comments from group review: 0.3 rotation compatibility and cleanup, add tests Signed-off-by: Riyaz Faizullabhoy --- server/snapshot/snapshot.go | 18 +++++------ server/snapshot/snapshot_test.go | 11 +++++-- server/timestamp/timestamp.go | 18 +++++------ server/timestamp/timestamp_test.go | 41 ++++++++++++++++++++++++- signer/keydbstore/rethink_keydbstore.go | 1 + signer/keydbstore/sql_keydbstore.go | 2 +- tuf/signed/ed25519.go | 5 ++- tuf/signed/ed25519_test.go | 26 ++++++++++++++++ 8 files changed, 97 insertions(+), 25 deletions(-) diff --git a/server/snapshot/snapshot.go b/server/snapshot/snapshot.go index 5283dacf3c..d752cf0e25 100644 --- a/server/snapshot/snapshot.go +++ b/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 diff --git a/server/snapshot/snapshot_test.go b/server/snapshot/snapshot_test.go index 6f2d7bfc31..2eac5b5f38 100644 --- a/server/snapshot/snapshot_test.go +++ b/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, "Received same key when attempting to recreate.") + require.NotEqual(t, k2, k3, "Received 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 diff --git a/server/timestamp/timestamp.go b/server/timestamp/timestamp.go index 68f9093251..9a69804b1e 100644 --- a/server/timestamp/timestamp.go +++ b/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 diff --git a/server/timestamp/timestamp_test.go b/server/timestamp/timestamp_test.go index 61ad949241..8a0513e4a0 100644 --- a/server/timestamp/timestamp_test.go +++ b/server/timestamp/timestamp_test.go @@ -259,10 +259,49 @@ func TestGetTimestampKeyCreateWithCorruptedStore(t *testing.T) { require.Nil(t, k, "Key should be nil") } -func TestGetSnapshotKeyCreateWithInvalidAlgo(t *testing.T) { +func TestGetTimestampKeyCreateWithInvalidAlgo(t *testing.T) { store := storage.NewMemStorage() crypto := signed.NewEd25519() k, err := GetOrCreateTimestampKey("gun", store, crypto, "notactuallyanalgorithm") 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, "Received same key when attempting to recreate.") + require.NotEqual(t, k2, k3, "Received same key when attempting to recreate.") + require.NotNil(t, k3, "Key should not be nil") +} diff --git a/signer/keydbstore/rethink_keydbstore.go b/signer/keydbstore/rethink_keydbstore.go index ed340e6ec4..1198c8ee5d 100644 --- a/signer/keydbstore/rethink_keydbstore.go +++ b/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 diff --git a/signer/keydbstore/sql_keydbstore.go b/signer/keydbstore/sql_keydbstore.go index 7866217ff7..b6493e6157 100644 --- a/signer/keydbstore/sql_keydbstore.go +++ b/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 } diff --git a/tuf/signed/ed25519.go b/tuf/signed/ed25519.go index eef673b9da..7a70739e40 100644 --- a/tuf/signed/ed25519.go +++ b/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 diff --git a/tuf/signed/ed25519_test.go b/tuf/signed/ed25519_test.go index 6747903511..31a70ed5aa 100644 --- a/tuf/signed/ed25519_test.go +++ b/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) +}