From 839a1d076fae4a5e3772ece6ff9185f5597e4ee6 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 15 Apr 2016 14:54:20 -0700 Subject: [PATCH] Add support for thresholds, which means signed.VerifyRoot needs to be have just like signed.VerifySignatures. So remove signed.VerifyRoot and just use signed.VerifySignatures instead. Also, to fix some tests, add an additional check for version when validating metadata since versions can't be negative. Signed-off-by: Ying Li --- client/client_update_test.go | 15 +------------- server/handlers/validation.go | 4 ++-- trustpinning/certs.go | 11 ++++++++-- tuf/data/root.go | 5 +++++ tuf/data/root_test.go | 8 +++++++ tuf/data/snapshot.go | 5 +++++ tuf/data/snapshot_test.go | 8 +++++++ tuf/data/targets.go | 4 ++++ tuf/data/targets_test.go | 10 +++++++++ tuf/data/timestamp.go | 5 +++++ tuf/data/timestamp_test.go | 8 +++++++ tuf/signed/verify.go | 39 ----------------------------------- 12 files changed, 65 insertions(+), 57 deletions(-) diff --git a/client/client_update_test.go b/client/client_update_test.go index 1b62c1056a..1dbe64c0f8 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -800,7 +800,7 @@ var waysToMessUpServer = []swizzleExpectations{ swizzle: (*testutils.MetadataSwizzler).ExpireMetadata}, {desc: "lower metadata version", expectErrs: []interface{}{ - &trustpinning.ErrValidationFail{}, signed.ErrLowVersion{}}, + &trustpinning.ErrValidationFail{}, signed.ErrLowVersion{}, data.ErrInvalidMetadata{}}, swizzle: func(s *testutils.MetadataSwizzler, role string) error { return s.OffsetMetadataVersion(role, -3) }}, @@ -851,13 +851,6 @@ func TestUpdateRootRemoteCorruptedNoLocalCache(t *testing.T) { } for _, testData := range waysToMessUpServerRoot() { - if testData.desc == "insufficient signatures" { - // Currently if we download the root during the bootstrap phase, - // we don't check for enough signatures to meet the threshold. We - // are also not entirely sure if we want to support threshold. - continue - } - testUpdateRemoteCorruptValidChecksum(t, updateOpts{ forWrite: false, role: data.CanonicalRootRole, @@ -1209,12 +1202,6 @@ func TestUpdateLocalAndRemoteRootCorrupt(t *testing.T) { // against the previous if we can continue } - if serverExpt.desc == "insufficient signatures" { - // Currently if we download the root during the bootstrap phase, - // we don't check for enough signatures to meet the threshold. - // We are also not sure if we want to support thresholds. - continue - } testUpdateLocalAndRemoteRootCorrupt(t, true, localExpt, serverExpt) testUpdateLocalAndRemoteRootCorrupt(t, false, localExpt, serverExpt) } diff --git a/server/handlers/validation.go b/server/handlers/validation.go index cd853961a8..fa2b86b1e0 100644 --- a/server/handlers/validation.go +++ b/server/handlers/validation.go @@ -417,7 +417,7 @@ func validateRoot(gun string, oldRoot, newRoot []byte) ( } } - if err := signed.VerifyRoot(parsedNewSigned, newRootRole.Threshold, newRootRole.Keys); err != nil { + if err := signed.VerifySignatures(parsedNewSigned, newRootRole); err != nil { return nil, validation.ErrBadRoot{Msg: err.Error()} } @@ -440,7 +440,7 @@ func checkAgainstOldRoot(oldRoot []byte, newRootRole data.BaseRole, newSigned *d } // Always verify the new root against the old root - if err := signed.VerifyRoot(newSigned, oldRootRole.Threshold, oldRootRole.Keys); err != nil { + if err := signed.VerifySignatures(newSigned, oldRootRole); err != nil { return validation.ErrBadRoot{Msg: fmt.Sprintf( "rotation detected and new root was not signed with at least %d old keys", oldRootRole.Threshold)} diff --git a/trustpinning/certs.go b/trustpinning/certs.go index 4ad275efde..bcfb27201e 100644 --- a/trustpinning/certs.go +++ b/trustpinning/certs.go @@ -93,6 +93,11 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin return err } + rootRole, err := signedRoot.BuildBaseRole(data.CanonicalRootRole) + if err != nil { + return err + } + // Retrieve all the leaf and intermediate certificates in root for which the CN matches the GUN allLeafCerts, allIntCerts := parseAllCerts(signedRoot) certsFromRoot, err := validRootLeafCerts(allLeafCerts, gun) @@ -117,7 +122,8 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin if len(trustedCerts) != 0 { logrus.Debugf("found %d valid root certificates for %s: %s", len(trustedCerts), gun, prettyFormatCertIDs(trustedCerts)) - err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(trustedCerts, allIntCerts)) + err = signed.VerifySignatures( + root, data.BaseRole{Keys: trustmanager.CertsToKeys(trustedCerts, allIntCerts), Threshold: 1}) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) return &ErrValidationFail{Reason: "failed to validate data with current trusted certificates"} @@ -149,7 +155,8 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin // Validate the integrity of the new root (does it have valid signatures) // Note that certsFromRoot is guaranteed to be unchanged only if we had prior cert data for this GUN or enabled TOFUS // If we attempted to pin a certain certificate or CA, certsFromRoot could have been pruned accordingly - err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(certsFromRoot, allIntCerts)) + err = signed.VerifySignatures(root, data.BaseRole{ + Keys: trustmanager.CertsToKeys(certsFromRoot, allIntCerts), Threshold: rootRole.Threshold}) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) return &ErrValidationFail{Reason: "failed to validate integrity of roots"} diff --git a/tuf/data/root.go b/tuf/data/root.go index d16a9a77c9..75f0c1c9c6 100644 --- a/tuf/data/root.go +++ b/tuf/data/root.go @@ -31,6 +31,11 @@ func isValidRootStructure(r Root) error { role: CanonicalRootRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, r.Type)} } + if r.Version < 0 { + return ErrInvalidMetadata{ + role: CanonicalRootRole, msg: "version cannot be negative"} + } + // all the base roles MUST appear in the root.json - other roles are allowed, // but other than the mirror role (not currently supported) are out of spec for _, roleName := range BaseRoles { diff --git a/tuf/data/root_test.go b/tuf/data/root_test.go index 0f038f1bca..d00f1dd255 100644 --- a/tuf/data/root_test.go +++ b/tuf/data/root_test.go @@ -218,3 +218,11 @@ func TestRootFromSignedValidatesRoleType(t *testing.T) { require.NoError(t, err) require.Equal(t, "Root", sRoot.Signed.Type) } + +// The version cannot be negative +func TestRootFromSignedValidatesVersion(t *testing.T) { + root := validRootTemplate() + root.Signed.Version = -1 + _, err := rootToSignedAndBack(t, root) + require.IsType(t, ErrInvalidMetadata{}, err) +} diff --git a/tuf/data/snapshot.go b/tuf/data/snapshot.go index 2a402ef7bc..172a1465e9 100644 --- a/tuf/data/snapshot.go +++ b/tuf/data/snapshot.go @@ -32,6 +32,11 @@ func isValidSnapshotStructure(s Snapshot) error { role: CanonicalSnapshotRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, s.Type)} } + if s.Version < 0 { + return ErrInvalidMetadata{ + role: CanonicalSnapshotRole, msg: "version cannot be negative"} + } + for _, role := range []string{CanonicalRootRole, CanonicalTargetsRole} { // Meta is a map of FileMeta, so if the role isn't in the map it returns // an empty FileMeta, which has an empty map, and you can check on keys diff --git a/tuf/data/snapshot_test.go b/tuf/data/snapshot_test.go index 6fa4265b45..2b2b23e2a3 100644 --- a/tuf/data/snapshot_test.go +++ b/tuf/data/snapshot_test.go @@ -194,6 +194,14 @@ func TestSnapshotFromSignedValidatesRoleType(t *testing.T) { require.Equal(t, TUFTypes[CanonicalSnapshotRole], sSnapshot.Signed.Type) } +// The version cannot be negative +func TestSnapshotFromSignedValidatesVersion(t *testing.T) { + sn := validSnapshotTemplate() + sn.Signed.Version = -1 + _, err := snapshotToSignedAndBack(t, sn) + require.IsType(t, ErrInvalidMetadata{}, err) +} + // GetMeta returns the checksum, or an error if it is missing. func TestSnapshotGetMeta(t *testing.T) { ts := validSnapshotTemplate() diff --git a/tuf/data/targets.go b/tuf/data/targets.go index 43468d4015..a73c8b6283 100644 --- a/tuf/data/targets.go +++ b/tuf/data/targets.go @@ -38,6 +38,10 @@ func isValidTargetsStructure(t Targets, roleName string) error { role: roleName, msg: fmt.Sprintf("expected type %s, not %s", expectedType, t.Type)} } + if t.Version < 0 { + return ErrInvalidMetadata{role: roleName, msg: "version cannot be negative"} + } + for _, roleObj := range t.Delegations.Roles { if !IsDelegation(roleObj.Name) || path.Dir(roleObj.Name) != roleName { return ErrInvalidMetadata{ diff --git a/tuf/data/targets_test.go b/tuf/data/targets_test.go index 13c7a58e3c..868089a203 100644 --- a/tuf/data/targets_test.go +++ b/tuf/data/targets_test.go @@ -227,3 +227,13 @@ func TestTargetsFromSignedValidatesRoleName(t *testing.T) { require.IsType(t, ErrInvalidRole{}, err) } } + +// The version cannot be negative +func TestTargetsFromSignedValidatesVersion(t *testing.T) { + tg := validTargetsTemplate() + tg.Signed.Version = -1 + s, err := tg.ToSigned() + require.NoError(t, err) + _, err = TargetsFromSigned(s, "targets/a") + require.IsType(t, ErrInvalidMetadata{}, err) +} diff --git a/tuf/data/timestamp.go b/tuf/data/timestamp.go index 111c414dc0..b9632a1ad1 100644 --- a/tuf/data/timestamp.go +++ b/tuf/data/timestamp.go @@ -31,6 +31,11 @@ func isValidTimestampStructure(t Timestamp) error { role: CanonicalTimestampRole, msg: fmt.Sprintf("expected type %s, not %s", expectedType, t.Type)} } + if t.Version < 0 { + return ErrInvalidMetadata{ + role: CanonicalTimestampRole, msg: "version cannot be negative"} + } + // Meta is a map of FileMeta, so if the role isn't in the map it returns // an empty FileMeta, which has an empty map, and you can check on keys // from an empty map. diff --git a/tuf/data/timestamp_test.go b/tuf/data/timestamp_test.go index 5365c884ec..207f2a74a7 100644 --- a/tuf/data/timestamp_test.go +++ b/tuf/data/timestamp_test.go @@ -194,6 +194,14 @@ func TestTimestampFromSignedValidatesRoleType(t *testing.T) { require.Equal(t, tsType, sTimestamp.Signed.Type) } +// The version cannot be negative +func TestTimestampFromSignedValidatesVersion(t *testing.T) { + ts := validTimestampTemplate() + ts.Signed.Version = -1 + _, err := timestampToSignedAndBack(t, ts) + require.IsType(t, ErrInvalidMetadata{}, err) +} + // GetSnapshot returns the snapshot checksum, or an error if it is missing. func TestTimestampGetSnapshot(t *testing.T) { ts := validTimestampTemplate() diff --git a/tuf/signed/verify.go b/tuf/signed/verify.go index 2fca4d783d..59c707690e 100644 --- a/tuf/signed/verify.go +++ b/tuf/signed/verify.go @@ -21,45 +21,6 @@ var ( ErrWrongType = errors.New("tuf: meta file has wrong type") ) -// VerifyRoot checks if a given root file is valid against a known set of keys. -// Threshold is always assumed to be 1 -func VerifyRoot(s *data.Signed, minVersion int, keys map[string]data.PublicKey) error { - if len(s.Signatures) == 0 { - return ErrNoSignatures - } - - var decoded map[string]interface{} - if err := json.Unmarshal(*s.Signed, &decoded); err != nil { - return err - } - msg, err := json.MarshalCanonical(decoded) - if err != nil { - return err - } - for _, sig := range s.Signatures { - // 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 for verify root: %s\n", sig.Method) - continue - } - key, ok := keys[sig.KeyID] - if !ok { - logrus.Debugf("continuing b/c signing key isn't present in keys: %s\n", sig.KeyID) - continue - } - - if err := verifier.Verify(key, sig.Signature, msg); err != nil { - logrus.Debugf("continuing b/c signature was invalid\n") - continue - } - // threshold of 1 so return on first success - return verifyMeta(s, data.CanonicalRootRole, minVersion) - } - return ErrRoleThreshold{} -} - // Verify checks the signatures and metadata (expiry, version) for the signed role // data func Verify(s *data.Signed, role data.BaseRole, minVersion int) error {