Pass through any errors from signed.Sign

Instead of silently ignoring any signing errors, and failing with a
generic ErrInsufficientSignatures, just pass through any signing errors.

As long as the role's threshold is met, some primary keys may be
missing. But if any key does exist (whether primary or optional), failures
using that key are fatal. (This is why signed.Sign needs to know the
role's threshold; making any missing key fatal would break signatures
of roles with 2 valid keys and threshold of 1.)

As a side effect, this improves the “snapshot key is not available”
detection in NotaryRepository.Publish, to trigger only when a key is not
available, not also when it should be available but using it is failing.

Similarly, the ErrInsufficientSignatures error now mentions only the
missing keys, not the successfully used keys now that present but
failing keys are handled by using other error types.

(This removes the second ErrInsufficientSignatures check because it is
no longer necessary, which also fixes the fact that it was broken by
introducing optional keys. The first, now only, ErrInsufficientSignatures
check does correctly distinguish between primary and optional keys.)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

tuf/signed/errors.go

@@ -8,17 +8,17 @@ import (
 // ErrInsufficientSignatures - can not create enough signatures on a piece of
 // metadata
 type ErrInsufficientSignatures struct {
-	FoundKeys  int
-	NeededKeys int
-	KeyIDs     []string
+	FoundKeys     int
+	NeededKeys    int
+	MissingKeyIDs []string
 }
 
 func (e ErrInsufficientSignatures) Error() string {
-	candidates := strings.Join(e.KeyIDs, ", ")
+	candidates := strings.Join(e.MissingKeyIDs, ", ")
 	if e.FoundKeys == 0 {
 		return fmt.Sprintf("signing keys not available, need %d keys out of: %s", e.NeededKeys, candidates)
 	}
-	return fmt.Sprintf("not enough signing keys: got %d of %d needed keys out of: %s",
+	return fmt.Sprintf("not enough signing keys: got %d of %d needed keys, other candidates: %s",
 		e.FoundKeys, e.NeededKeys, candidates)
 }
 

tuf/signed/sign.go

@@ -15,6 +15,7 @@ import (
 	"crypto/rand"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/utils"
 )
@@ -32,21 +33,24 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
 	signingKeyIDs := make(map[string]struct{})
 	tufIDs := make(map[string]data.PublicKey)
-	ids := make([]string, 0, len(primaryKeys))
 
 	privKeys := make(map[string]data.PrivateKey)
 
 	// Get all the private key objects related to the public keys
+	missingKeyIDs := []string{}
 	for _, key := range primaryKeys {
 		canonicalID, err := utils.CanonicalKeyID(key)
-		ids = append(ids, canonicalID)
 		tufIDs[key.ID()] = key
 		if err != nil {
-			continue
+			return err
 		}
 		k, _, err := service.GetPrivateKey(canonicalID)
 		if err != nil {
-			continue
+			if _, ok := err.(trustmanager.ErrKeyNotFound); ok {
+				missingKeyIDs = append(missingKeyIDs, canonicalID)
+				continue
+			}
+			return err
 		}
 		privKeys[key.ID()] = k
 	}
@@ -54,7 +58,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 	// Check to ensure we have enough signing keys
 	if len(privKeys) < minSignatures {
 		return ErrInsufficientSignatures{FoundKeys: len(privKeys),
-			NeededKeys: minSignatures, KeyIDs: ids}
+			NeededKeys: minSignatures, MissingKeyIDs: missingKeyIDs}
 	}
 
 	for _, key := range optionalKeys {
@@ -78,7 +82,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 		sig, err := pk.Sign(rand.Reader, *s.Signed, nil)
 		if err != nil {
 			logrus.Debugf("Failed to sign with key: %s. Reason: %v", keyID, err)
-			continue
+			return err
 		}
 		signingKeyIDs[keyID] = struct{}{}
 		signatures = append(signatures, data.Signature{
@@ -88,12 +92,6 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 		})
 	}
 
-	// Check we produced enough signatures
-	if len(signatures) < minSignatures {
-		return ErrInsufficientSignatures{FoundKeys: len(signatures),
-			NeededKeys: minSignatures, KeyIDs: ids}
-	}
-
 	for _, sig := range s.Signatures {
 		if _, ok := signingKeyIDs[sig.KeyID]; ok {
 			// key is in the set of key IDs for which a signature has been created

tuf/signed/sign_test.go

@@ -1,8 +1,10 @@
 package signed
 
 import (
+	"crypto"
 	"crypto/rand"
 	"encoding/pem"
+	"io"
 	"testing"
 
 	"github.com/docker/go/canonical/json"
@@ -16,6 +18,22 @@ const (
 	testKeyPEM1 = "-----BEGIN PUBLIC KEY-----\nMIIBojANBgkqhkiG9w0BAQEFAAOCAY8AMIIBigKCAYEAnKuXZeefa2LmgxaL5NsM\nzKOHNe+x/nL6ik+lDBCTV6OdcwAhHQS+PONGhrChIUVR6Vth3hUCrreLzPO73Oo5\nVSCuRJ53UronENl6lsa5mFKP8StYLvIDITNvkoT3j52BJIjyNUK9UKY9As2TNqDf\nBEPIRp28ev/NViwGOEkBu2UAbwCIdnDXm8JQErCZA0Ydm7PKGgjLbFsFGrVzqXHK\n6pdzJXlhr9yap3UpgQ/iO9JtoEYB2EXsnSrPc9JRjR30bNHHtnVql3fvinXrAEwq\n3xmN4p+R4VGzfdQN+8Kl/IPjqWB535twhFYEG/B7Ze8IwbygBjK3co/KnOPqMUrM\nBI8ztvPiogz+MvXb8WvarZ6TMTh8ifZI96r7zzqyzjR1hJulEy3IsMGvz8XS2J0X\n7sXoaqszEtXdq5ef5zKVxkiyIQZcbPgmpHLq4MgfdryuVVc/RPASoRIXG4lKaTJj\n1ANMFPxDQpHudCLxwCzjCb+sVa20HBRPTnzo8LSZkI6jAgMBAAE=\n-----END PUBLIC KEY-----"
 )
 
+type FailingPrivateKeyErr struct {
+}
+
+func (err FailingPrivateKeyErr) Error() string {
+	return "FailingPrivateKey.Sign failed"
+}
+
+// A data.PrivateKey which fails signing with a recognizable error.
+type FailingPrivateKey struct {
+	data.PrivateKey
+}
+
+func (fpk FailingPrivateKey) Sign(rand io.Reader, msg []byte, opts crypto.SignerOpts) (signature []byte, err error) {
+	return nil, FailingPrivateKeyErr{}
+}
+
 // A CryptoService which does not contain any keys.
 type FailingCryptoService struct {
 }
@@ -337,3 +355,19 @@ func TestSignOptionalKeys(t *testing.T) {
 		require.IsType(t, ErrInsufficientSignatures{}, err)
 	}
 }
+
+func TestSignFailingKeys(t *testing.T) {
+	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
+	require.NoError(t, err)
+	cs := &MockCryptoService{FailingPrivateKey{privKey}}
+
+	testData := data.Signed{Signed: &json.RawMessage{}}
+	err = Sign(cs, &testData, []data.PublicKey{privKey}, 1, nil)
+	require.Error(t, err)
+	require.IsType(t, FailingPrivateKeyErr{}, err)
+
+	testData = data.Signed{Signed: &json.RawMessage{}}
+	err = Sign(cs, &testData, []data.PublicKey{}, 0, []data.PublicKey{privKey})
+	require.Error(t, err)
+	require.IsType(t, FailingPrivateKeyErr{}, err)
+}