diff --git a/certs/certs_test.go b/certs/certs_test.go index 46bdbdbed1..516d4b96c7 100644 --- a/certs/certs_test.go +++ b/certs/certs_test.go @@ -297,10 +297,7 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string signedTestRoot, err := testRoot.ToSigned() assert.NoError(t, err) - err = signed.Sign(cs, signedTestRoot, replRootKey) - assert.NoError(t, err) - - err = signed.Sign(cs, signedTestRoot, origRootKey) + err = signed.Sign(cs, signedTestRoot, replRootKey, origRootKey) assert.NoError(t, err) // This call to ValidateRoot will succeed since we are using a valid PEM diff --git a/tuf/signed/sign.go b/tuf/signed/sign.go index 9c840de6da..14e29bcb27 100644 --- a/tuf/signed/sign.go +++ b/tuf/signed/sign.go @@ -22,10 +22,13 @@ import ( // Sign takes a data.Signed and a key, calculated and adds the signature // 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. func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error { logrus.Debugf("sign called with %d keys", len(keys)) 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(keys)) privKeys := make(map[string]data.PrivateKey) @@ -34,6 +37,7 @@ func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error { for _, key := range keys { canonicalID, err := utils.CanonicalKeyID(key) ids = append(ids, canonicalID) + tufIDs[key.ID()] = key if err != nil { continue } @@ -78,6 +82,20 @@ func Sign(service CryptoService, s *data.Signed, keys ...data.PublicKey) error { // 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 diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go index 7d65f1c6eb..99d8b57c4a 100644 --- a/tuf/signed/sign_test.go +++ b/tuf/signed/sign_test.go @@ -21,11 +21,6 @@ type FailingCryptoService struct { testKey data.PrivateKey } -func (mts *FailingCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) { - sigs := make([]data.Signature, 0, len(keyIDs)) - return sigs, nil -} - func (mts *FailingCryptoService) Create(_, _ string) (data.PublicKey, error) { return mts.testKey, nil } @@ -36,10 +31,10 @@ func (mts *FailingCryptoService) ListKeys(role string) []string { func (mts *FailingCryptoService) ListAllKeys() map[string]string { return map[string]string{ - mts.testKey.ID(): "root", - mts.testKey.ID(): "targets", - mts.testKey.ID(): "snapshot", - mts.testKey.ID(): "timestamp", + mts.testKey.ID(): data.CanonicalRootRole, + mts.testKey.ID(): data.CanonicalTargetsRole, + mts.testKey.ID(): data.CanonicalSnapshotRole, + mts.testKey.ID(): data.CanonicalTimestampRole, } } @@ -69,14 +64,6 @@ type MockCryptoService struct { testKey data.PrivateKey } -func (mts *MockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) { - sigs := make([]data.Signature, 0, len(keyIDs)) - for _, keyID := range keyIDs { - sigs = append(sigs, data.Signature{KeyID: keyID}) - } - return sigs, nil -} - func (mts *MockCryptoService) Create(_ string, _ string) (data.PublicKey, error) { return mts.testKey, nil } @@ -94,10 +81,10 @@ func (mts *MockCryptoService) ListKeys(role string) []string { func (mts *MockCryptoService) ListAllKeys() map[string]string { return map[string]string{ - mts.testKey.ID(): "root", - mts.testKey.ID(): "targets", - mts.testKey.ID(): "snapshot", - mts.testKey.ID(): "timestamp", + mts.testKey.ID(): data.CanonicalRootRole, + mts.testKey.ID(): data.CanonicalTargetsRole, + mts.testKey.ID(): data.CanonicalSnapshotRole, + mts.testKey.ID(): data.CanonicalTimestampRole, } } @@ -119,16 +106,6 @@ type StrictMockCryptoService struct { MockCryptoService } -func (mts *StrictMockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) { - sigs := make([]data.Signature, 0, len(keyIDs)) - for _, keyID := range keyIDs { - if keyID == mts.testKey.ID() { - sigs = append(sigs, data.Signature{KeyID: keyID}) - } - } - return sigs, nil -} - func (mts *StrictMockCryptoService) GetKey(keyID string) data.PublicKey { if keyID == mts.testKey.ID() { return data.PublicKeyFromPrivate(mts.testKey) @@ -142,10 +119,10 @@ func (mts *StrictMockCryptoService) ListKeys(role string) []string { func (mts *StrictMockCryptoService) ListAllKeys() map[string]string { return map[string]string{ - mts.testKey.ID(): "root", - mts.testKey.ID(): "targets", - mts.testKey.ID(): "snapshot", - mts.testKey.ID(): "timestamp", + mts.testKey.ID(): data.CanonicalRootRole, + mts.testKey.ID(): data.CanonicalTargetsRole, + mts.testKey.ID(): data.CanonicalSnapshotRole, + mts.testKey.ID(): data.CanonicalTimestampRole, } } @@ -156,7 +133,7 @@ func (mts *StrictMockCryptoService) ImportRootKey(r io.Reader) error { // Test signing and ensure the expected signature is added func TestBasicSign(t *testing.T) { cs := NewEd25519() - key, err := cs.Create("root", data.ED25519Key) + key, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) require.NoError(t, err) testData := data.Signed{} @@ -176,7 +153,7 @@ func TestBasicSign(t *testing.T) { // with the same key ID func TestReSign(t *testing.T) { cs := NewEd25519() - key, err := cs.Create("root", data.ED25519Key) + key, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) require.NoError(t, err) testData := data.Signed{} @@ -198,8 +175,9 @@ func TestMultiSign(t *testing.T) { cs := NewEd25519() testData := data.Signed{} - key1, err := cs.Create("root", data.ED25519Key) + key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) require.NoError(t, err) + Sign(cs, &testData, key1) // reinitializing cs means it won't know about key1. We want @@ -207,8 +185,9 @@ func TestMultiSign(t *testing.T) { // that the signature for key1 is left intact and the signature // for key2 is added cs = NewEd25519() - key2, err := cs.Create("root", data.ED25519Key) + key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) require.NoError(t, err) + Sign( cs, &testData, @@ -229,7 +208,6 @@ func TestMultiSign(t *testing.T) { } } require.Equal(t, 2, count) - } func TestSignReturnsNoSigs(t *testing.T) { @@ -270,3 +248,59 @@ func TestSignWithX509(t *testing.T) { require.Len(t, testData.Signatures, 1) require.Equal(t, tufRSAx509Key.ID(), testData.Signatures[0].KeyID) } + +func TestSignRemovesValidSigByInvalidKey(t *testing.T) { + cs := NewEd25519() + testData := data.Signed{} + + key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) + require.NoError(t, err) + + Sign(cs, &testData, key1) + require.Len(t, testData.Signatures, 1) + require.Equal(t, key1.ID(), testData.Signatures[0].KeyID) + + key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) + require.NoError(t, err) + + // should remove key1 sig even though it's valid. It no longer appears + // in the list of signing keys for the role + Sign( + cs, + &testData, + key2, + ) + + require.Len(t, testData.Signatures, 1) + require.Equal(t, key2.ID(), testData.Signatures[0].KeyID) +} + +func TestSignRemovesInvalidSig(t *testing.T) { + cs := NewEd25519() + testData := data.Signed{} + + key1, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) + require.NoError(t, err) + + Sign(cs, &testData, key1) + require.Len(t, testData.Signatures, 1) + require.Equal(t, key1.ID(), testData.Signatures[0].KeyID) + + // we need cs to "forget" key1 so we can't sign with it + cs = NewEd25519() + key2, err := cs.Create(data.CanonicalRootRole, data.ED25519Key) + require.NoError(t, err) + + // modify test data to invalidate key1 sig + testData.Signed = []byte{0xff} + // should remove key1 sig because it's out of date + Sign( + cs, + &testData, + key1, + key2, + ) + + require.Len(t, testData.Signatures, 1) + require.Equal(t, key2.ID(), testData.Signatures[0].KeyID) +} diff --git a/tuf/signed/verify.go b/tuf/signed/verify.go index 4869b17074..f49d1280f4 100644 --- a/tuf/signed/verify.go +++ b/tuf/signed/verify.go @@ -2,6 +2,7 @@ package signed import ( "errors" + "fmt" "strings" "time" @@ -124,16 +125,8 @@ func VerifySignatures(s *data.Signed, roleData data.BaseRole) error { logrus.Debugf("continuing b/c keyid lookup was nil: %s\n", sig.KeyID) continue } - // method lookup is consistent due to Unmarshal JSON doing lower case for us. - method := sig.Method - verifier, ok := Verifiers[method] - if !ok { - logrus.Debugf("continuing b/c signing method is not supported: %s\n", sig.Method) - continue - } - - if err := verifier.Verify(key, sig.Signature, msg); err != nil { - logrus.Debugf("continuing b/c signature was invalid\n") + if err := VerifySignature(msg, sig, key); err != nil { + logrus.Debugf("continuing b/c %s", err.Error()) continue } valid[sig.KeyID] = struct{}{} @@ -145,3 +138,18 @@ func VerifySignatures(s *data.Signed, roleData data.BaseRole) error { return nil } + +// VerifySignature checks a single signature and public key against a payload +func VerifySignature(msg []byte, sig data.Signature, pk data.PublicKey) error { + // method lookup is consistent due to Unmarshal JSON doing lower case for us. + method := sig.Method + verifier, ok := Verifiers[method] + if !ok { + return fmt.Errorf("signing method is not supported: %s\n", sig.Method) + } + + if err := verifier.Verify(pk, sig.Signature, msg); err != nil { + return fmt.Errorf("signature was invalid\n") + } + return nil +} diff --git a/tuf/tuf.go b/tuf/tuf.go index e8d0638674..0cedf74e93 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -919,12 +919,7 @@ func (tr *Repo) SignTimestamp(expires time.Time) (*data.Signed, error) { } func (tr Repo) sign(signedData *data.Signed, role data.BaseRole) (*data.Signed, error) { - ks := role.ListKeys() - if len(ks) < 1 { - return nil, signed.ErrNoKeys{} - } - err := signed.Sign(tr.cryptoService, signedData, ks...) - if err != nil { + if err := signed.Sign(tr.cryptoService, signedData, role.ListKeys()...); err != nil { return nil, err } return signedData, nil