More testing, cleanup

Signed-off-by: Riyaz Faizullabhoy <[email protected]>

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)
+}

server/handlers/default.go

@@ -184,43 +184,12 @@ func GetKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request)
 }
 
 func getKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
-	gun, ok := vars["imageName"]
-	logger := ctxu.GetLoggerWithField(ctx, gun, "gun")
-	if !ok || gun == "" {
-		logger.Info("400 GET no gun in request")
-		return errors.ErrUnknown.WithDetail("no gun")
-	}
-
-	role, ok := vars["tufRole"]
-	if !ok || role == "" {
-		logger.Info("400 GET no role in request")
-		return errors.ErrUnknown.WithDetail("no role")
-	}
-
-	s := ctx.Value("metaStore")
-	store, ok := s.(storage.MetaStore)
-	if !ok || store == nil {
-		logger.Error("500 GET storage not configured")
-		return errors.ErrNoStorage.WithDetail(nil)
-	}
-	c := ctx.Value("cryptoService")
-	crypto, ok := c.(signed.CryptoService)
-	if !ok || crypto == nil {
-		logger.Error("500 GET crypto service not configured")
-		return errors.ErrNoCryptoService.WithDetail(nil)
-	}
-	algo := ctx.Value("keyAlgorithm")
-	keyAlgo, ok := algo.(string)
-	if !ok || keyAlgo == "" {
-		logger.Error("500 GET key algorithm not configured")
-		return errors.ErrNoKeyAlgorithm.WithDetail(nil)
+	role, gun, keyAlgorithm, store, crypto, err := setupKeyHandler(ctx, w, r, vars, http.MethodGet)
+	if err != nil {
+		return err
 	}
-	keyAlgorithm := keyAlgo
-
-	var (
-		key data.PublicKey
-		err error
-	)
+	var key data.PublicKey
+	logger := ctxu.GetLoggerWithField(ctx, gun, "gun")
 	switch role {
 	case data.CanonicalTimestampRole:
 		key, err = timestamp.GetOrCreateTimestampKey(gun, store, crypto, keyAlgorithm)
@@ -253,43 +222,12 @@ func RotateKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Reques
 }
 
 func rotateKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
-	gun, ok := vars["imageName"]
-	logger := ctxu.GetLoggerWithField(ctx, gun, "gun")
-	if !ok || gun == "" {
-		logger.Info("400 POST no gun in request")
-		return errors.ErrUnknown.WithDetail("no gun")
-	}
-
-	role, ok := vars["tufRole"]
-	if !ok || role == "" {
-		logger.Info("400 POST no role in request")
-		return errors.ErrUnknown.WithDetail("no role")
-	}
-
-	s := ctx.Value("metaStore")
-	store, ok := s.(storage.MetaStore)
-	if !ok || store == nil {
-		logger.Error("500 POST storage not configured")
-		return errors.ErrNoStorage.WithDetail(nil)
-	}
-	c := ctx.Value("cryptoService")
-	crypto, ok := c.(signed.CryptoService)
-	if !ok || crypto == nil {
-		logger.Error("500 POST crypto service not configured")
-		return errors.ErrNoCryptoService.WithDetail(nil)
-	}
-	algo := ctx.Value("keyAlgorithm")
-	keyAlgo, ok := algo.(string)
-	if !ok || keyAlgo == "" {
-		logger.Error("500 POST key algorithm not configured")
-		return errors.ErrNoKeyAlgorithm.WithDetail(nil)
+	role, gun, keyAlgorithm, store, crypto, err := setupKeyHandler(ctx, w, r, vars, http.MethodPost)
+	if err != nil {
+		return err
 	}
-	keyAlgorithm := keyAlgo
-
-	var (
-		key data.PublicKey
-		err error
-	)
+	var key data.PublicKey
+	logger := ctxu.GetLoggerWithField(ctx, gun, "gun")
 	switch role {
 	case data.CanonicalTimestampRole:
 		key, err = timestamp.RotateTimestampKey(gun, store, crypto, keyAlgorithm)
@@ -314,6 +252,43 @@ func rotateKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Reques
 	return nil
 }
 
+// To be called before getKeyHandler or rotateKeyHandler
+func setupKeyHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string, actionVerb string) (string, string, string, storage.MetaStore, signed.CryptoService, error) {
+	gun, ok := vars["imageName"]
+	logger := ctxu.GetLoggerWithField(ctx, gun, "gun")
+	if !ok || gun == "" {
+		logger.Infof("400 %s no gun in request", actionVerb)
+		return "", "", "", nil, nil, errors.ErrUnknown.WithDetail("no gun")
+	}
+
+	role, ok := vars["tufRole"]
+	if !ok || role == "" {
+		logger.Infof("400 %s no role in request", actionVerb)
+		return "", "", "", nil, nil, errors.ErrUnknown.WithDetail("no role")
+	}
+
+	s := ctx.Value("metaStore")
+	store, ok := s.(storage.MetaStore)
+	if !ok || store == nil {
+		logger.Errorf("500 %s storage not configured", actionVerb)
+		return "", "", "", nil, nil, errors.ErrNoStorage.WithDetail(nil)
+	}
+	c := ctx.Value("cryptoService")
+	crypto, ok := c.(signed.CryptoService)
+	if !ok || crypto == nil {
+		logger.Errorf("500 %s crypto service not configured", actionVerb)
+		return "", "", "", nil, nil, errors.ErrNoCryptoService.WithDetail(nil)
+	}
+	algo := ctx.Value("keyAlgorithm")
+	keyAlgo, ok := algo.(string)
+	if !ok || keyAlgo == "" {
+		logger.Errorf("500 %s key algorithm not configured", actionVerb)
+		return "", "", "", nil, nil, errors.ErrNoKeyAlgorithm.WithDetail(nil)
+	}
+
+	return role, gun, keyAlgo, store, crypto, nil
+}
+
 // NotFoundHandler is used as a generic catch all handler to return the ErrMetadataNotFound
 // 404 response
 func NotFoundHandler(ctx context.Context, w http.ResponseWriter, r *http.Request) error {

server/handlers/default_test.go

@@ -193,8 +193,8 @@ func TestRotateKeyHandlerInvalidRole(t *testing.T) {
 	}
 }
 
-// Rotating the key fails if we don't pass a valid key algorithm, or if it isn't Ed25519
-func TestRotateKeyHandlerNonED25519(t *testing.T) {
+// Rotating the key fails if we don't pass a valid key algorithm
+func TestRotateKeyHandlerInvalidKeyAlgo(t *testing.T) {
 	roles := []string{data.CanonicalTimestampRole, data.CanonicalSnapshotRole}
 	req := &http.Request{Body: ioutil.NopCloser(bytes.NewBuffer(nil))}
 
@@ -205,9 +205,6 @@ func TestRotateKeyHandlerNonED25519(t *testing.T) {
 		invalidKeyAlgoState.keyAlgo = "notactuallyakeyalgorithm"
 		err := rotateKeyHandler(getContext(invalidKeyAlgoState), recorder, req, vars)
 		require.Error(t, err)
-		invalidKeyAlgoState.keyAlgo = data.ECDSAKey
-		err = rotateKeyHandler(getContext(invalidKeyAlgoState), recorder, req, vars)
-		require.Error(t, err)
 	}
 }
 

server/snapshot/snapshot.go

@@ -31,7 +31,7 @@ func GetOrCreateSnapshotKey(gun string, store storage.MetaStore, crypto signed.C
 		return key, nil
 	}
 
-	// If we have a current root, parse out the public key ID for the snapshot role and get it from the underlying cryptoservice
+	// If we have a current root, parse out the public key for the snapshot role, and return it
 	repoSignedRoot := new(data.SignedRoot)
 	if err := json.Unmarshal(rootJSON, repoSignedRoot); err != nil {
 		logrus.Errorf("Failed to unmarshal existing root for GUN %s to retrieve snapshot key ID", gun)

server/snapshot/snapshot_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/hex"
+	"fmt"
 	"testing"
 	"time"
 
@@ -51,6 +52,46 @@ func TestGetSnapshotKeyCreate(t *testing.T) {
 	require.NotNil(t, k2, "Key should not be nil")
 }
 
+type FailingStore struct {
+	*storage.MemStorage
+}
+
+func (f FailingStore) GetCurrent(role, gun string) (*time.Time, []byte, error) {
+	return nil, nil, fmt.Errorf("failing store failed")
+}
+
+func TestGetSnapshotKeyCreateWithFailingStore(t *testing.T) {
+	store := FailingStore{storage.NewMemStorage()}
+	crypto := signed.NewEd25519()
+	k, err := GetOrCreateSnapshotKey("gun", store, crypto, data.ED25519Key)
+	require.Error(t, err, "Expected error")
+	require.Nil(t, k, "Key should be nil")
+}
+
+type CorruptedStore struct {
+	*storage.MemStorage
+}
+
+func (c CorruptedStore) GetCurrent(role, gun string) (*time.Time, []byte, error) {
+	return &time.Time{}, []byte("junk"), nil
+}
+
+func TestGetSnapshotKeyCreateWithCorruptedStore(t *testing.T) {
+	store := CorruptedStore{storage.NewMemStorage()}
+	crypto := signed.NewEd25519()
+	k, err := GetOrCreateSnapshotKey("gun", store, crypto, data.ED25519Key)
+	require.Error(t, err, "Expected error")
+	require.Nil(t, k, "Key should be nil")
+}
+
+func TestGetSnapshotKeyCreateWithInvalidAlgo(t *testing.T) {
+	store := storage.NewMemStorage()
+	crypto := signed.NewEd25519()
+	k, err := GetOrCreateSnapshotKey("gun", store, crypto, "notactuallyanalgorithm")
+	require.Error(t, err, "Expected error")
+	require.Nil(t, k, "Key should be nil")
+}
+
 func TestGetSnapshotKeyExisting(t *testing.T) {
 
 	repo, crypto, err := testutils.EmptyRepo("gun")

server/storage/rethinkdb.go

@@ -267,12 +267,10 @@ func (rdb RethinkDB) Bootstrap() error {
 
 // CheckHealth checks that all tables and databases exist and are query-able
 func (rdb RethinkDB) CheckHealth() error {
-	for _, table := range []string{TUFFilesRethinkTable.Name} {
-		res, err := gorethink.DB(rdb.dbName).Table(table).Info().Run(rdb.sess)
-		if err != nil {
-			return fmt.Errorf("%s is unavailable, or missing one or more tables, or permissions are incorrectly set", rdb.dbName)
-		}
-		defer res.Close()
+	res, err := gorethink.DB(rdb.dbName).Table(TUFFilesRethinkTable.Name).Info().Run(rdb.sess)
+	if err != nil {
+		return fmt.Errorf("%s is unavailable, or missing one or more tables, or permissions are incorrectly set", rdb.dbName)
 	}
+	defer res.Close()
 	return nil
 }

server/storage/sqldb.go

@@ -159,21 +159,15 @@ func (db *SQLStorage) Delete(gun string) error {
 	return db.Unscoped().Where(&TUFFile{Gun: gun}).Delete(TUFFile{}).Error
 }
 
-// CheckHealth asserts that both required tables are present
+// CheckHealth asserts that the tuf_files table is present
 func (db *SQLStorage) CheckHealth() error {
-	interfaces := []interface {
-		TableName() string
-	}{&TUFFile{}}
-
-	for _, model := range interfaces {
-		tableOk := db.HasTable(model)
-		if db.Error != nil {
-			return db.Error
-		}
-		if !tableOk {
-			return fmt.Errorf(
-				"Cannot access table: %s", model.TableName())
-		}
+	tableOk := db.HasTable(&TUFFile{})
+	if db.Error != nil {
+		return db.Error
+	}
+	if !tableOk {
+		return fmt.Errorf(
+			"Cannot access table: %s", TUFFile{}.TableName())
 	}
 	return nil
 }

server/storage/sqldb_test.go

@@ -24,11 +24,9 @@ func SetupSQLDB(t *testing.T, dbtype, dburl string) *SQLStorage {
 
 	// verify that the tables are empty
 	var count int
-	for _, model := range [1]interface{}{&TUFFile{}} {
-		query := dbStore.DB.Model(model).Count(&count)
-		require.NoError(t, query.Error)
-		require.Equal(t, 0, count)
-	}
+	query := dbStore.DB.Model(&TUFFile{}).Count(&count)
+	require.NoError(t, query.Error)
+	require.Equal(t, 0, count)
 	return dbStore
 }
 

server/timestamp/timestamp.go

@@ -36,7 +36,7 @@ func GetOrCreateTimestampKey(gun string, store storage.MetaStore, crypto signed.
 		return key, nil
 	}
 
-	// If we have a current root, parse out the public key ID for the snapshot role and get it from the underlying cryptoservice
+	// If we have a current root, parse out the public key for the timestamp role, and return it
 	repoSignedRoot := new(data.SignedRoot)
 	if err := json.Unmarshal(rootJSON, repoSignedRoot); err != nil {
 		logrus.Errorf("Failed to unmarshal existing root for GUN %s to retrieve timestamp key ID", gun)

server/timestamp/timestamp_test.go

@@ -2,16 +2,16 @@ package timestamp
 
 import (
 	"bytes"
+	"fmt"
 	"testing"
 	"time"
 
 	"github.com/docker/go/canonical/json"
+	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
 	"github.com/docker/notary/tuf/testutils"
 	"github.com/stretchr/testify/require"
-
-	"github.com/docker/notary/server/storage"
 )
 
 func TestTimestampExpired(t *testing.T) {
@@ -226,3 +226,43 @@ func TestCreateTimestampNoKeyInCrypto(t *testing.T) {
 	require.Error(t, err)
 	require.IsType(t, signed.ErrInsufficientSignatures{}, err)
 }
+
+type FailingStore struct {
+	*storage.MemStorage
+}
+
+func (f FailingStore) GetCurrent(role, gun string) (*time.Time, []byte, error) {
+	return nil, nil, fmt.Errorf("failing store failed")
+}
+
+func TestGetTimestampKeyCreateWithFailingStore(t *testing.T) {
+	store := FailingStore{storage.NewMemStorage()}
+	crypto := signed.NewEd25519()
+	k, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
+	require.Error(t, err, "Expected error")
+	require.Nil(t, k, "Key should be nil")
+}
+
+type CorruptedStore struct {
+	*storage.MemStorage
+}
+
+func (c CorruptedStore) GetCurrent(role, gun string) (*time.Time, []byte, error) {
+	return &time.Time{}, []byte("junk"), nil
+}
+
+func TestGetTimestampKeyCreateWithCorruptedStore(t *testing.T) {
+	store := CorruptedStore{storage.NewMemStorage()}
+	crypto := signed.NewEd25519()
+	k, err := GetOrCreateTimestampKey("gun", store, crypto, data.ED25519Key)
+	require.Error(t, err, "Expected error")
+	require.Nil(t, k, "Key should be nil")
+}
+
+func TestGetSnapshotKeyCreateWithInvalidAlgo(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")
+}