Merge pull request #294 from endophage/remove_cryptoservice_sign

CryptoService.Sign is now dead code. Remove it and update tests

cryptoservice/crypto_service.go

@@ -111,39 +111,6 @@ func (cs *CryptoService) RemoveKey(keyID string) (err error) {
 	return // returns whatever the final values were
 }
 
-// Sign returns the signatures for the payload with a set of keyIDs. It ignores
-// errors to sign and expects the called to validate if the number of returned
-// signatures is adequate.
-func (cs *CryptoService) Sign(keyIDs []string, payload []byte) ([]data.Signature, error) {
-	signatures := make([]data.Signature, 0, len(keyIDs))
-	for _, keyID := range keyIDs {
-		privKey, _, err := cs.GetPrivateKey(keyID)
-		if err != nil {
-			logrus.Debugf("error attempting to retrieve private key: %s, %v", keyID, err)
-			continue
-		}
-
-		sigAlgo := privKey.SignatureAlgorithm()
-		sig, err := privKey.Sign(rand.Reader, payload, nil)
-		if err != nil {
-			logrus.Debugf("ignoring error attempting to %s sign with keyID: %s, %v",
-				privKey.Algorithm(), keyID, err)
-			continue
-		}
-
-		logrus.Debugf("appending %s signature with Key ID: %s", privKey.Algorithm(), keyID)
-
-		// Append signatures to result array
-		signatures = append(signatures, data.Signature{
-			KeyID:     keyID,
-			Method:    sigAlgo,
-			Signature: sig[:],
-		})
-	}
-
-	return signatures, nil
-}
-
 // ListKeys returns a list of key IDs valid for the given role
 func (cs *CryptoService) ListKeys(role string) []string {
 	var res []string

cryptoservice/crypto_service_test.go

@@ -100,14 +100,17 @@ func (c CryptoServiceTester) TestSignWithKey(t *testing.T) {
 	assert.NoError(t, err, c.errorMsg("error creating key"))
 
 	// Test Sign
-	signatures, err := cryptoService.Sign([]string{tufKey.ID()}, content)
+	privKey, role, err := cryptoService.GetPrivateKey(tufKey.ID())
+	assert.NoError(t, err, c.errorMsg("failed to get private key"))
+	assert.Equal(t, c.role, role)
+
+	signature, err := privKey.Sign(rand.Reader, content, nil)
 	assert.NoError(t, err, c.errorMsg("signing failed"))
-	assert.Len(t, signatures, 1, c.errorMsg("wrong number of signatures"))
 
 	verifier, ok := signed.Verifiers[algoToSigType[c.keyAlgo]]
 	assert.True(t, ok, c.errorMsg("Unknown verifier for algorithm"))
 
-	err = verifier.Verify(tufKey, signatures[0].Signature, content)
+	err = verifier.Verify(tufKey, signature, content)
 	assert.NoError(t, err,
 		c.errorMsg("verification failed for %s key type", c.keyAlgo))
 }
@@ -115,36 +118,33 @@ func (c CryptoServiceTester) TestSignWithKey(t *testing.T) {
 // asserts that signing, if there are no matching keys, produces no signatures
 func (c CryptoServiceTester) TestSignNoMatchingKeys(t *testing.T) {
 	cryptoService := c.cryptoServiceFactory()
-	content := []byte("this is a secret")
 
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err, c.errorMsg("error creating key"))
 
-	// Test Sign with key that is not in the cryptoservice
-	signatures, err := cryptoService.Sign([]string{privKey.ID()}, content)
-	assert.NoError(t, err, c.errorMsg("signing failed"))
-	assert.Len(t, signatures, 0, c.errorMsg("wrong number of signatures"))
+	// Test Sign
+	_, _, err = cryptoService.GetPrivateKey(privKey.ID())
+	assert.Error(t, err, c.errorMsg("Should not have found private key"))
 }
 
-// If there are multiple keystores, even if all of them have the same key,
-// only one signature is returned.
-func (c CryptoServiceTester) TestSignWhenMultipleKeystores(t *testing.T) {
+// Test GetPrivateKey succeeds when multiple keystores have the same key
+func (c CryptoServiceTester) TestGetPrivateKeyMultipleKeystores(t *testing.T) {
 	cryptoService := c.cryptoServiceFactory()
 	cryptoService.keyStores = append(cryptoService.keyStores,
 		trustmanager.NewKeyMemoryStore(passphraseRetriever))
-	content := []byte("this is a secret")
 
 	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
 	assert.NoError(t, err, c.errorMsg("error creating key"))
 
 	for _, store := range cryptoService.keyStores {
-		err := store.AddKey(privKey.ID(), "root", privKey)
+		err := store.AddKey(privKey.ID(), c.role, privKey)
 		assert.NoError(t, err)
 	}
 
-	signatures, err := cryptoService.Sign([]string{privKey.ID()}, content)
-	assert.NoError(t, err, c.errorMsg("signing failed"))
-	assert.Len(t, signatures, 1, c.errorMsg("wrong number of signatures"))
+	foundKey, role, err := cryptoService.GetPrivateKey(privKey.ID())
+	assert.NoError(t, err, c.errorMsg("failed to get private key"))
+	assert.Equal(t, c.role, role)
+	assert.Equal(t, privKey.ID(), foundKey.ID())
 }
 
 // asserts that removing key that exists succeeds
@@ -274,7 +274,7 @@ func testCryptoService(t *testing.T, gun string) {
 			cst.TestGetNonexistentKey(t)
 			cst.TestSignWithKey(t)
 			cst.TestSignNoMatchingKeys(t)
-			cst.TestSignWhenMultipleKeystores(t)
+			cst.TestGetPrivateKeyMultipleKeystores(t)
 			cst.TestRemoveCreatedKey(t)
 			cst.TestRemoveFromMultipleKeystores(t)
 			cst.TestListFromMultipleKeystores(t)

signer/api/api.go

@@ -1,6 +1,7 @@
 package api
 
 import (
+	"crypto/rand"
 	"encoding/json"
 	"net/http"
 
@@ -178,8 +179,15 @@ func Sign(cryptoServices signer.CryptoServiceIndex) http.Handler {
 			return
 		}
 
-		signatures, err := cryptoService.Sign([]string{sigRequest.KeyID.ID}, sigRequest.Content)
-		if err != nil || len(signatures) != 1 {
+		privKey, _, err := cryptoService.GetPrivateKey(tufKey.ID())
+		if err != nil {
+			// We got an unexpected error
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte(err.Error()))
+			return
+		}
+		sig, err := privKey.Sign(rand.Reader, sigRequest.Content, nil)
+		if err != nil {
 			w.WriteHeader(http.StatusInternalServerError)
 			w.Write([]byte(err.Error()))
 			return
@@ -189,7 +197,8 @@ func Sign(cryptoServices signer.CryptoServiceIndex) http.Handler {
 				KeyID:     &pb.KeyID{ID: tufKey.ID()},
 				Algorithm: &pb.Algorithm{Algorithm: tufKey.Algorithm()},
 			},
-			Content: signatures[0].Signature,
+			Algorithm: &pb.Algorithm{Algorithm: privKey.SignatureAlgorithm().String()},
+			Content:   sig,
 		}
 
 		json.NewEncoder(w).Encode(signature)

signer/api/rpc_api.go

@@ -1,6 +1,7 @@
 package api
 
 import (
+	"crypto/rand"
 	"fmt"
 
 	ctxu "github.com/docker/distribution/context"
@@ -125,8 +126,13 @@ func (s *SignerServer) Sign(ctx context.Context, sr *pb.SignatureRequest) (*pb.S
 		return nil, grpc.Errorf(codes.NotFound, "key %s not found", sr.KeyID.ID)
 	}
 
-	signatures, err := service.Sign([]string{sr.KeyID.ID}, sr.Content)
-	if err != nil || len(signatures) != 1 {
+	privKey, _, err := service.GetPrivateKey(tufKey.ID())
+	if err != nil {
+		logger.Errorf("Sign: key %s not found", sr.KeyID.ID)
+		return nil, grpc.Errorf(codes.NotFound, "key %s not found", sr.KeyID.ID)
+	}
+	sig, err := privKey.Sign(rand.Reader, sr.Content, nil)
+	if err != nil {
 		logger.Errorf("Sign: signing failed for KeyID %s on hash %s", sr.KeyID.ID, sr.Content)
 		return nil, grpc.Errorf(codes.Internal, "Signing failed for KeyID %s on hash %s", sr.KeyID.ID, sr.Content)
 	}
@@ -138,8 +144,8 @@ func (s *SignerServer) Sign(ctx context.Context, sr *pb.SignatureRequest) (*pb.S
 			KeyID:     &pb.KeyID{ID: tufKey.ID()},
 			Algorithm: &pb.Algorithm{Algorithm: tufKey.Algorithm()},
 		},
-		Algorithm: &pb.Algorithm{Algorithm: signatures[0].Method.String()},
-		Content:   signatures[0].Signature,
+		Algorithm: &pb.Algorithm{Algorithm: privKey.SignatureAlgorithm().String()},
+		Content:   sig,
 	}
 
 	return signature, nil

signer/client/signer_trust.go

@@ -127,28 +127,6 @@ func NewNotarySigner(hostname string, port string, tlsConfig *tls.Config) *Notar
 	}
 }
 
-// Sign signs a byte string with a number of KeyIDs
-func (trust *NotarySigner) Sign(keyIDs []string, toSign []byte) ([]data.Signature, error) {
-	signatures := make([]data.Signature, 0, len(keyIDs))
-	for _, ID := range keyIDs {
-		keyID := pb.KeyID{ID: ID}
-		sr := &pb.SignatureRequest{
-			Content: toSign,
-			KeyID:   &keyID,
-		}
-		sig, err := trust.sClient.Sign(context.Background(), sr)
-		if err != nil {
-			return nil, err
-		}
-		signatures = append(signatures, data.Signature{
-			KeyID:     sig.KeyInfo.KeyID.ID,
-			Method:    data.SigAlgorithm(sig.Algorithm.Algorithm),
-			Signature: sig.Content,
-		})
-	}
-	return signatures, nil
-}
-
 // Create creates a remote key and returns the PublicKey associated with the remote private key
 func (trust *NotarySigner) Create(role, algorithm string) (data.PublicKey, error) {
 	publicKey, err := trust.kmClient.CreateKey(context.Background(), &pb.Algorithm{Algorithm: algorithm})

tuf/signed/ed25519.go

@@ -61,23 +61,6 @@ func (e *Ed25519) ListAllKeys() map[string]string {
 	return keys
 }
 
-// Sign generates an Ed25519 signature over the data
-func (e *Ed25519) Sign(keyIDs []string, toSign []byte) ([]data.Signature, error) {
-	signatures := make([]data.Signature, 0, len(keyIDs))
-	for _, keyID := range keyIDs {
-		priv := [ed25519.PrivateKeySize]byte{}
-		copy(priv[:], e.keys[keyID].privKey.Private())
-		sig := ed25519.Sign(&priv, toSign)
-		signatures = append(signatures, data.Signature{
-			KeyID:     keyID,
-			Method:    data.EDDSASignature,
-			Signature: sig[:],
-		})
-	}
-	return signatures, nil
-
-}
-
 // Create generates a new key and returns the public part
 func (e *Ed25519) Create(role, algorithm string) (data.PublicKey, error) {
 	if algorithm != data.ED25519Key {

tuf/signed/interface.go

@@ -5,14 +5,6 @@ import (
 	"io"
 )
 
-// SigningService defines the necessary functions to determine
-// if a user is able to sign with a key, and to perform signing.
-type SigningService interface {
-	// Sign takes a slice of keyIDs and a piece of data to sign
-	// and returns a slice of signatures and an error
-	Sign(keyIDs []string, data []byte) ([]data.Signature, error)
-}
-
 // KeyService provides management of keys locally. It will never
 // accept or provide private keys. Communication between the KeyService
 // and a SigningService happen behind the Create function.
@@ -44,10 +36,9 @@ type KeyService interface {
 	ImportRootKey(source io.Reader) error
 }
 
-// CryptoService defines a unified Signing and Key Service as this
-// will be most useful for most applications.
+// CryptoService is deprecated and all instances of its use should be
+// replaced with KeyService
 type CryptoService interface {
-	SigningService
 	KeyService
 }