From 816c1c980c9961a905b2ae0d25cb2ad72a61b8dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 1 Dec 2015 13:21:55 +0100 Subject: [PATCH] Modify signed.Sign to replace, not add, signatures. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only thing depending on signed.Sign keeping old signatures was two tests; all real users were modifying the signed data without clearing the old signatures, and therefore implicitly relying on signing with the keys which were used for the old signatures as well. This broke signing an updated root with a new certificate when the old certificate was no longer available. It could have been fixed by keeping signed.Sign as is and adding the clearing to all users, but noting actually needs the appending semantics, the appending semantics is surprising, and switching to replacing signatures is less code. Signed-off-by: Miloslav Trmač --- tuf/signed/sign.go | 34 +++------------------------------- tuf/signed/sign_test.go | 7 ++----- tuf/testutils/swizzler.go | 4 ---- 3 files changed, 5 insertions(+), 40 deletions(-) diff --git a/tuf/signed/sign.go b/tuf/signed/sign.go index 2ffa26dab3..7b395d5743 100644 --- a/tuf/signed/sign.go +++ b/tuf/signed/sign.go @@ -20,19 +20,14 @@ import ( "github.com/docker/notary/tuf/utils" ) -// Sign takes a data.Signed and keys, calculates and adds at least +// Sign takes a data.Signed and replaces data.Signature with at least // minPrimarySignature signatures using primaryKeys and any possible -// signatures using optionalKeys to the data.Signed -// N.B. All public keys for a role should be passed so that this function -// can correctly clean up signatures that are no longer valid. +// signatures using optionalKeys func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, minSignatures int, optionalKeys []data.PublicKey) error { logrus.Debugf("sign called with %d/%d required + %d optional keys", minSignatures, len(primaryKeys), len(optionalKeys)) - signatures := make([]data.Signature, 0, len(s.Signatures)+1) - signingKeyIDs := make(map[string]struct{}) - tufIDs := make(map[string]data.PublicKey) privKeys := make(map[string]data.PrivateKey) @@ -40,7 +35,6 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, missingKeyIDs := []string{} for _, key := range primaryKeys { canonicalID, err := utils.CanonicalKeyID(key) - tufIDs[key.ID()] = key if err != nil { return err } @@ -65,7 +59,6 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, if _, ok := privKeys[key.ID()]; ok { continue } - tufIDs[key.ID()] = key canonicalID, err := utils.CanonicalKeyID(key) if err != nil { return err @@ -78,13 +71,13 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, } // Do signing and generate list of signatures + signatures := make([]data.Signature, 0, len(privKeys)) for keyID, pk := range privKeys { sig, err := pk.Sign(rand.Reader, *s.Signed, nil) if err != nil { logrus.Debugf("Failed to sign with key: %s. Reason: %v", keyID, err) return err } - signingKeyIDs[keyID] = struct{}{} signatures = append(signatures, data.Signature{ KeyID: keyID, Method: pk.SignatureAlgorithm(), @@ -92,27 +85,6 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, }) } - 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 - continue - } - var ( - k data.PublicKey - ok bool - ) - if k, ok = tufIDs[sig.KeyID]; !ok { - // key is no longer a valid signing key - continue - } - if err := VerifySignature(*s.Signed, sig, k); err != nil { - // signature is no longer valid - continue - } - // keep any signatures that still represent valid keys and are - // themselves valid - signatures = append(signatures, sig) - } s.Signatures = signatures return nil } diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go index 1483a8c76e..61447064aa 100644 --- a/tuf/signed/sign_test.go +++ b/tuf/signed/sign_test.go @@ -154,7 +154,7 @@ func TestReSign(t *testing.T) { } -// Should not remove signatures for valid keys that were not resigned with +// Resigning drops all obsolete signatures func TestMultiSign(t *testing.T) { cs := NewEd25519() testData := data.Signed{ @@ -166,10 +166,7 @@ func TestMultiSign(t *testing.T) { require.NoError(t, Sign(cs, &testData, []data.PublicKey{key1}, 1, nil)) - // reinitializing cs means it won't know about key1. We want - // to attempt to sign passing both key1 and key2, while expecting - // that the signature for key1 is left intact and the signature - // for key2 is added + // reinitializing cs means it won't know about key1. cs = NewEd25519() key2, err := cs.Create(data.CanonicalRootRole, "", data.ED25519Key) require.NoError(t, err) diff --git a/tuf/testutils/swizzler.go b/tuf/testutils/swizzler.go index 4079ba1215..e7a54e65ae 100644 --- a/tuf/testutils/swizzler.go +++ b/tuf/testutils/swizzler.go @@ -64,10 +64,6 @@ func getPubKeys(cs signed.CryptoService, s *data.Signed, role string) ([]data.Pu // signs the new metadata, replacing whatever signature was there func serializeMetadata(cs signed.CryptoService, s *data.Signed, role string, pubKeys ...data.PublicKey) ([]byte, error) { - - // delete the existing signatures - s.Signatures = []data.Signature{} - if len(pubKeys) < 1 { return nil, ErrNoKeyForRole{role} }