diff --git a/tuf/signed/sign.go b/tuf/signed/sign.go index 2ffa26dab3..3650f0109e 100644 --- a/tuf/signed/sign.go +++ b/tuf/signed/sign.go @@ -21,15 +21,17 @@ import ( ) // Sign takes a data.Signed and keys, calculates and adds at least -// minPrimarySignature signatures using primaryKeys and any possible -// signatures using optionalKeys to the data.Signed +// minSignature signatures using primaryKeys the data.Signed. It will also +// clean up any signatures produced by a key not in the primaryKeys or +// validSigningKeys list. +// (validSigningKeys can be either a superset of or a completely disjoint set from +// primaryKeys) // 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, primaryKeys []data.PublicKey, - minSignatures int, optionalKeys []data.PublicKey) error { + minSignatures int, validSigningKeys []data.PublicKey) error { - logrus.Debugf("sign called with %d/%d required + %d optional keys", - minSignatures, len(primaryKeys), len(optionalKeys)) + logrus.Debugf("sign called with %d/%d required keys", minSignatures, len(primaryKeys)) signatures := make([]data.Signature, 0, len(s.Signatures)+1) signingKeyIDs := make(map[string]struct{}) tufIDs := make(map[string]data.PublicKey) @@ -55,28 +57,20 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, privKeys[key.ID()] = k } + // include the list of validSigningKeys + for _, key := range validSigningKeys { + if _, ok := tufIDs[key.ID()]; !ok { + tufIDs[key.ID()] = key + } + } + // Check to ensure we have enough signing keys if len(privKeys) < minSignatures { return ErrInsufficientSignatures{FoundKeys: len(privKeys), NeededKeys: minSignatures, MissingKeyIDs: missingKeyIDs} } - for _, key := range optionalKeys { - if _, ok := privKeys[key.ID()]; ok { - continue - } - tufIDs[key.ID()] = key - canonicalID, err := utils.CanonicalKeyID(key) - if err != nil { - return err - } - k, _, err := service.GetPrivateKey(canonicalID) - if err != nil { - continue - } - privKeys[key.ID()] = k - } - + emptyStruct := struct{}{} // Do signing and generate list of signatures for keyID, pk := range privKeys { sig, err := pk.Sign(rand.Reader, *s.Signed, nil) @@ -84,7 +78,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey, logrus.Debugf("Failed to sign with key: %s. Reason: %v", keyID, err) return err } - signingKeyIDs[keyID] = struct{}{} + signingKeyIDs[keyID] = emptyStruct signatures = append(signatures, data.Signature{ KeyID: keyID, Method: pk.SignatureAlgorithm(), diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go index 38095aea5c..f59cf57b8d 100644 --- a/tuf/signed/sign_test.go +++ b/tuf/signed/sign_test.go @@ -240,19 +240,31 @@ func TestSignRemovesValidSigByInvalidKey(t *testing.T) { key1, err := cs.Create(data.CanonicalRootRole, "", data.ED25519Key) require.NoError(t, err) - Sign(cs, &testData, []data.PublicKey{key1}, 1, nil) - 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, []data.PublicKey{key2}, 1, nil) + require.NoError(t, Sign(cs, &testData, []data.PublicKey{key1, key2}, 1, nil)) + require.Len(t, testData.Signatures, 2) + var signatureKeys []string + for _, sig := range testData.Signatures { + signatureKeys = append(signatureKeys, sig.KeyID) + } + require.Contains(t, signatureKeys, key1.ID()) + require.Contains(t, signatureKeys, key2.ID()) - require.Len(t, testData.Signatures, 1) - require.Equal(t, key2.ID(), testData.Signatures[0].KeyID) + key3, 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 or valid signing keys for the role + require.NoError(t, Sign(cs, &testData, []data.PublicKey{key3}, 1, []data.PublicKey{key2})) + require.Len(t, testData.Signatures, 2) + signatureKeys = nil + for _, sig := range testData.Signatures { + signatureKeys = append(signatureKeys, sig.KeyID) + } + require.Contains(t, signatureKeys, key2.ID()) + require.Contains(t, signatureKeys, key3.ID()) } func TestSignRemovesInvalidSig(t *testing.T) { @@ -264,7 +276,7 @@ func TestSignRemovesInvalidSig(t *testing.T) { key1, err := cs.Create(data.CanonicalRootRole, "", data.ED25519Key) require.NoError(t, err) - Sign(cs, &testData, []data.PublicKey{key1}, 1, nil) + require.NoError(t, Sign(cs, &testData, []data.PublicKey{key1}, 1, nil)) require.Len(t, testData.Signatures, 1) require.Equal(t, key1.ID(), testData.Signatures[0].KeyID) @@ -321,42 +333,6 @@ func TestSignMinSignatures(t *testing.T) { } } -func TestSignOptionalKeys(t *testing.T) { - csA := NewEd25519() - keyA1, err := csA.Create("keyA", "", data.ED25519Key) - require.NoError(t, err) - keyA2, err := csA.Create("keyA", "", data.ED25519Key) - require.NoError(t, err) - // csB is only used to create public keys which are unavailable from csA. - csB := NewEd25519() - keyB, err := csB.Create("keyB", "", data.ED25519Key) - require.NoError(t, err) - - // Both primary and optional keys are used. - testData := data.Signed{Signed: &json.RawMessage{}} - err = Sign(csA, &testData, []data.PublicKey{keyA1, keyB}, 1, []data.PublicKey{keyA2}) - require.NoError(t, err) - require.Len(t, testData.Signatures, 2) - - // Missing optional keys are not an error. - testData = data.Signed{Signed: &json.RawMessage{}} - err = Sign(csA, &testData, []data.PublicKey{keyA1}, 1, []data.PublicKey{keyB}) - require.NoError(t, err) - require.Len(t, testData.Signatures, 1) - - // minSignatures is only counting primary keys. - testData = data.Signed{Signed: &json.RawMessage{}} - err = Sign(csA, &testData, []data.PublicKey{keyA1, keyB}, 2, []data.PublicKey{keyA2}) - require.Error(t, err) - if err2, ok := err.(ErrInsufficientSignatures); ok { - require.Equal(t, err2.FoundKeys, 1) - require.Equal(t, err2.NeededKeys, 2) - } else { - // We know this will fail if !ok - require.IsType(t, ErrInsufficientSignatures{}, err) - } -} - func TestSignFailingKeys(t *testing.T) { privKey, err := trustmanager.GenerateECDSAKey(rand.Reader) require.NoError(t, err) @@ -366,9 +342,4 @@ func TestSignFailingKeys(t *testing.T) { 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) } diff --git a/tuf/signed/verify_test.go b/tuf/signed/verify_test.go index 9d2c0d26bc..ba90065be8 100644 --- a/tuf/signed/verify_test.go +++ b/tuf/signed/verify_test.go @@ -76,7 +76,7 @@ func TestValidSigWithIncorrectKeyID(t *testing.T) { b, err := json.MarshalCanonical(meta) require.NoError(t, err) s := &data.Signed{Signed: (*json.RawMessage)(&b)} - Sign(cs, s, []data.PublicKey{k1}, 1) + Sign(cs, s, []data.PublicKey{k1}, 1, nil) require.Equal(t, 1, len(s.Signatures)) s.Signatures[0].KeyID = "invalidIDA" err = Verify(s, roleWithKeys, 1) diff --git a/tuf/tuf.go b/tuf/tuf.go index cd69c7db56..8913b3f47c 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -939,8 +939,12 @@ func (tr Repo) sign(signedData *data.Signed, role data.BaseRole, optionalKeyIDs } optionalKeys = append(optionalKeys, k) } - if err := signed.Sign(tr.cryptoService, signedData, role.ListKeys(), role.Threshold, optionalKeys); err != nil { + validKeys := append(role.ListKeys(), optionalKeys...) + if err := signed.Sign(tr.cryptoService, signedData, role.ListKeys(), role.Threshold, validKeys); err != nil { return nil, err } + // Attempt to sign with the optional keys, but ignore any errors, because these keys are optional + signed.Sign(tr.cryptoService, signedData, optionalKeys, 0, validKeys) + return signedData, nil }