From 925e956fca7fbd16fa22280f0ed252037b850c50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 21 Nov 2015 07:02:42 +0100 Subject: [PATCH] Pass through any errors from signed.Sign MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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č --- tuf/signed/errors.go | 10 +++++----- tuf/signed/sign.go | 22 ++++++++++------------ tuf/signed/sign_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/tuf/signed/errors.go b/tuf/signed/errors.go index 79617a1eb4..07f6193394 100644 --- a/tuf/signed/errors.go +++ b/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) } diff --git a/tuf/signed/sign.go b/tuf/signed/sign.go index c3b680209c..2ffa26dab3 100644 --- a/tuf/signed/sign.go +++ b/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 diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go index bb840e4064..1483a8c76e 100644 --- a/tuf/signed/sign_test.go +++ b/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) +}