diff --git a/client/client_root_validation_test.go b/client/client_root_validation_test.go index 4ecfbeb6f8..92e7e56252 100644 --- a/client/client_root_validation_test.go +++ b/client/client_root_validation_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/Sirupsen/logrus" - "github.com/docker/notary/trustpinning" "github.com/docker/notary/tuf/data" "github.com/stretchr/testify/require" ) @@ -42,37 +41,9 @@ func validateRootSuccessfully(t *testing.T, rootType string) { fakeServerData(t, repo, mux, keys) // - // Test TOFUS logic. We remove all certs and expect a new one to be added after ListTargets + // Test TOFUS logic. // - err = repo.CertStore.RemoveAll() - require.NoError(t, err) - require.Len(t, repo.CertStore.GetCertificates(), 0) - // This list targets is expected to succeed and the certificate store to have the new certificate _, err = repo.ListTargets(data.CanonicalTargetsRole) require.NoError(t, err) - require.Len(t, repo.CertStore.GetCertificates(), 1) - - // - // Test certificate mismatch logic. We remove all certs, add a different cert to the - // same CN, and expect ValidateRoot to fail - // - - // First, remove all certs - err = repo.CertStore.RemoveAll() - require.NoError(t, err) - require.Len(t, repo.CertStore.GetCertificates(), 0) - - // Add a previously generated certificate with CN=docker.com/notary - err = repo.CertStore.AddCertFromFile( - "../fixtures/self-signed_docker.com-notary.crt") - require.NoError(t, err) - - // This list targets is expected to fail, since there already exists a certificate - // in the store for the dnsName docker.com/notary, so TOFUS doesn't apply - _, err = repo.ListTargets(data.CanonicalTargetsRole) - require.Error(t, err, "An error was expected") - require.Equal(t, err, &trustpinning.ErrValidationFail{ - Reason: "failed to validate data with current trusted certificates", - }) } diff --git a/client/client_test.go b/client/client_test.go index 38879d7eee..f902d6650a 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -425,26 +425,6 @@ func requireRepoHasExpectedKeys(t *testing.T, repo *NotaryRepository, "there should be no timestamp key because the server manages it") } -// This creates a new certificate store in the repo's base directory and -// makes sure the repo has the right certificates -func requireRepoHasExpectedCerts(t *testing.T, repo *NotaryRepository) { - // The repo should have a certificate store and have created certs using - // it, so create a new store, and check that the certs do exist and - // are valid - trustPath := filepath.Join(repo.baseDir, notary.TrustedCertsDir) - certStore, err := trustmanager.NewX509FilteredFileStore( - trustPath, - trustmanager.FilterCertsExpiredSha1, - ) - require.NoError(t, err) - certificates := certStore.GetCertificates() - require.Len(t, certificates, 1, "unexpected number of trusted certificates") - - certID, err := trustmanager.FingerprintCert(certificates[0]) - require.NoError(t, err, "unable to fingerprint the trusted certificate") - require.NotEqual(t, certID, "") -} - // Sanity check the TUF metadata files. Verify that it exists for a particular // role, the JSON is well-formed, and the signatures exist. // For the root.json file, also check that the root, snapshot, and @@ -515,7 +495,6 @@ func testInitRepoMetadata(t *testing.T, rootType string, serverManagesSnapshot b defer os.RemoveAll(repo.baseDir) requireRepoHasExpectedKeys(t, repo, rootKeyID, !serverManagesSnapshot) - requireRepoHasExpectedCerts(t, repo) requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, @@ -2757,14 +2736,6 @@ func logRepoTrustRoot(t *testing.T, prefix string, repo *NotaryRepository) { for _, k := range root.Signed.Roles[data.CanonicalRootRole].KeyIDs { logrus.Debugf("\t%s", k) } - logrus.Debugf("All trusted certs:") - certs, err := repo.CertStore.GetCertificatesByCN(repo.gun) - require.NoError(t, err) - for _, cert := range certs { - id, err := trustmanager.FingerprintCert(cert) - require.NoError(t, err) - logrus.Debugf("\t%s", id) - } } // ID of the (only) certificate trusted by the root role metadata @@ -2774,15 +2745,6 @@ func rootRoleCertID(t *testing.T, repo *NotaryRepository) string { return rootKeys[0] } -func verifyOnlyTrustedCertificate(t *testing.T, repo *NotaryRepository, certID string) { - certs, err := repo.CertStore.GetCertificatesByCN(repo.gun) - require.NoError(t, err) - require.Len(t, certs, 1) - id, err := trustmanager.FingerprintCert(certs[0]) - require.NoError(t, err) - require.Equal(t, certID, id) -} - func TestRotateRootKey(t *testing.T) { ts := fullTestServer(t) defer ts.Close() @@ -2845,7 +2807,6 @@ func TestRotateRootKey(t *testing.T) { err = authorRepo.Publish() require.NoError(t, err) logRepoTrustRoot(t, "post-publish", authorRepo) - verifyOnlyTrustedCertificate(t, authorRepo, newRootCertID) // Verify the user can use the rotated repo, and see the added target. _, err = userRepo.GetTargetByName("current") @@ -2859,7 +2820,6 @@ func TestRotateRootKey(t *testing.T) { _, err = freshUserRepo.GetTargetByName("current") require.NoError(t, err) require.Equal(t, newRootCertID, rootRoleCertID(t, freshUserRepo)) - verifyOnlyTrustedCertificate(t, freshUserRepo, newRootCertID) logRepoTrustRoot(t, "fresh client", freshUserRepo) // Verify that the user initialized with the original certificate eventually @@ -3307,7 +3267,6 @@ func TestDeleteRepo(t *testing.T) { // Assert initialization was successful before we delete requireRepoHasExpectedKeys(t, repo, rootKeyID, true) - requireRepoHasExpectedCerts(t, repo) requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true) @@ -3322,12 +3281,6 @@ func TestDeleteRepo(t *testing.T) { requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, false) requireRepoHasExpectedMetadata(t, repo, data.CanonicalTimestampRole, false) - // Assert no certs for this repo exist locally - _, err = repo.CertStore.GetCertificatesByCN(gun) - require.Error(t, err) - require.IsType(t, &trustmanager.ErrNoCertificatesFound{}, err) - require.NotNil(t, err) - // Assert keys for this repo exist locally requireRepoHasExpectedKeys(t, repo, rootKeyID, true) } @@ -3352,7 +3305,6 @@ func TestDeleteRepoBadFilestore(t *testing.T) { // Assert initialization was successful before we delete requireRepoHasExpectedKeys(t, repo, rootKeyID, true) - requireRepoHasExpectedCerts(t, repo) requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true) @@ -3365,50 +3317,6 @@ func TestDeleteRepoBadFilestore(t *testing.T) { require.Error(t, err) } -// TestDeleteRepoNoCerts tests that local repo data is deleted successfully without an error even when we do not have certificates -func TestDeleteRepoNoCerts(t *testing.T) { - gun := "docker.com/notary" - - ts, _, _ := simpleTestServer(t) - defer ts.Close() - - repo, rootKeyID := initializeRepo(t, data.ECDSAKey, gun, ts.URL, false) - defer os.RemoveAll(repo.baseDir) - - // Assert initialization was successful before we delete - requireRepoHasExpectedKeys(t, repo, rootKeyID, true) - requireRepoHasExpectedCerts(t, repo) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true) - - // Delete the certificate store contents and require it has been fully deleted - repo.CertStore.RemoveAll() - _, err := repo.CertStore.GetCertificatesByCN(gun) - require.Error(t, err) - require.IsType(t, &trustmanager.ErrNoCertificatesFound{}, err) - require.NotNil(t, err) - - // Delete all client trust data for repo - err = repo.DeleteTrustData() - require.NoError(t, err) - - // Assert no metadata for this repo exists locally - requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, false) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, false) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, false) - requireRepoHasExpectedMetadata(t, repo, data.CanonicalTimestampRole, false) - - // Assert no certs for this repo exist locally - _, err = repo.CertStore.GetCertificatesByCN(gun) - require.Error(t, err) - require.IsType(t, &trustmanager.ErrNoCertificatesFound{}, err) - require.NotNil(t, err) - - // Assert keys for this repo exist locally - requireRepoHasExpectedKeys(t, repo, rootKeyID, true) -} - // Test that we get a correct list of roles with keys and signatures func TestListRoles(t *testing.T) { ts := fullTestServer(t) diff --git a/client/client_update_test.go b/client/client_update_test.go index 30eda07445..bc351d9e52 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -248,82 +248,86 @@ func TestUpdateReplacesCorruptOrMissingMetadata(t *testing.T) { } } -// If a repo has an invalid root (signed by wrong key, expired, invalid version, -// invalid number of signatures, etc.), the repo will just get the new root from -// the server, whether or not the update is for writing (forced update), but -// it will refuse to update if the root key has changed and the new root is -// not signed by the old and new key -func TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign(t *testing.T) { - if testing.Short() { - t.Skip("skipping test in short mode") - } - - serverMeta, serverSwizzler := newServerSwizzler(t) - origMeta := testutils.CopyRepoMetadata(serverMeta) - - ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound, "docker.com/notary") - defer ts.Close() - - repo := newBlankRepo(t, ts.URL) - defer os.RemoveAll(repo.baseDir) - - err := repo.Update(false) // ensure we have all metadata to start with - require.NoError(t, err) - - // rotate the server's root.json root key so that they no longer match trust anchors - require.NoError(t, serverSwizzler.ChangeRootKey()) - // bump versions, update snapshot and timestamp too so it will not fail on a hash - bumpVersions(t, serverSwizzler, 1) - - // we want to swizzle the local cache, not the server, so create a new one - repoSwizzler := &testutils.MetadataSwizzler{ - MetadataCache: repo.fileStore, - CryptoService: serverSwizzler.CryptoService, - Roles: serverSwizzler.Roles, - } - - for _, expt := range waysToMessUpLocalMetadata { - text, messItUp := expt.desc, expt.swizzle - for _, forWrite := range []bool{true, false} { - require.NoError(t, messItUp(repoSwizzler, data.CanonicalRootRole), "could not fuzz root (%s)", text) - messedUpMeta, err := repo.fileStore.GetMeta(data.CanonicalRootRole, -1) - - if _, ok := err.(store.ErrMetaNotFound); ok { // one of the ways to mess up is to delete metadata - - err = repo.Update(forWrite) - require.Error(t, err) // the new server has a different root key, update fails - - } else { - - require.NoError(t, err) - - err = repo.Update(forWrite) - require.Error(t, err) // the new server has a different root, update fails - - // we can't test that all the metadata is the same, because we probably would - // have downloaded a new timestamp and maybe snapshot. But the root should be the - // same because it has failed to update. - for role, expected := range origMeta { - if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole { - actual, err := repo.fileStore.GetMeta(role, -1) - require.NoError(t, err, "problem getting repo metadata for %s", role) - - if role == data.CanonicalRootRole { - expected = messedUpMeta - } - require.True(t, bytes.Equal(expected, actual), - "%s for %s: expected to not have updated", text, role) - } - } - - } - - // revert our original root metadata - require.NoError(t, - repo.fileStore.SetMeta(data.CanonicalRootRole, origMeta[data.CanonicalRootRole])) - } - } -} +// TODO(riyazdf): this test goes against the new root pinning because the certstore +// no longer exists, so roots on the local filestore must be correct. +// TODO(riyazdf): break this test up to only test valid rejections, not associated +// with rotations -- such as expired metadata, etc. +//// If a repo has an invalid root (signed by wrong key, expired, invalid version, +//// invalid number of signatures, etc.), the repo will just get the new root from +//// the server, whether or not the update is for writing (forced update), but +//// it will refuse to update if the root key has changed and the new root is +//// not signed by the old and new key +//func TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign(t *testing.T) { +// if testing.Short() { +// t.Skip("skipping test in short mode") +// } +// +// serverMeta, serverSwizzler := newServerSwizzler(t) +// origMeta := testutils.CopyRepoMetadata(serverMeta) +// +// ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound, "docker.com/notary") +// defer ts.Close() +// +// repo := newBlankRepo(t, ts.URL) +// defer os.RemoveAll(repo.baseDir) +// +// err := repo.Update(false) // ensure we have all metadata to start with +// require.NoError(t, err) +// +// // rotate the server's root.json root key so that they no longer match trust anchors +// require.NoError(t, serverSwizzler.ChangeRootKey()) +// // bump versions, update snapshot and timestamp too so it will not fail on a hash +// bumpVersions(t, serverSwizzler, 1) +// +// // we want to swizzle the local cache, not the server, so create a new one +// repoSwizzler := &testutils.MetadataSwizzler{ +// MetadataCache: repo.fileStore, +// CryptoService: serverSwizzler.CryptoService, +// Roles: serverSwizzler.Roles, +// } +// +// for _, expt := range waysToMessUpLocalMetadata { +// text, messItUp := expt.desc, expt.swizzle +// for _, forWrite := range []bool{true, false} { +// require.NoError(t, messItUp(repoSwizzler, data.CanonicalRootRole), "could not fuzz root (%s)", text) +// messedUpMeta, err := repo.fileStore.GetMeta(data.CanonicalRootRole, -1) +// +// if _, ok := err.(store.ErrMetaNotFound); ok { // one of the ways to mess up is to delete metadata +// +// err = repo.Update(forWrite) +// require.Error(t, err) // the new server has a different root key, update fails +// +// } else { +// +// require.NoError(t, err) +// +// err = repo.Update(forWrite) +// require.Error(t, err) // the new server has a different root, update fails +// +// // we can't test that all the metadata is the same, because we probably would +// // have downloaded a new timestamp and maybe snapshot. But the root should be the +// // same because it has failed to update. +// for role, expected := range origMeta { +// if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole { +// actual, err := repo.fileStore.GetMeta(role, -1) +// require.NoError(t, err, "problem getting repo metadata for %s", role) +// +// if role == data.CanonicalRootRole { +// expected = messedUpMeta +// } +// require.True(t, bytes.Equal(expected, actual), +// "%s for %s: expected to not have updated", text, role) +// } +// } +// +// } +// +// // revert our original root metadata +// require.NoError(t, +// repo.fileStore.SetMeta(data.CanonicalRootRole, origMeta[data.CanonicalRootRole])) +// } +// } +//} type updateOpts struct { notFoundCode int // what code to return when the cache doesn't have the metadata diff --git a/trustpinning/certs.go b/trustpinning/certs.go index fc9432de40..e327e51514 100644 --- a/trustpinning/certs.go +++ b/trustpinning/certs.go @@ -268,43 +268,3 @@ func parseAllCerts(signedRoot *data.SignedRoot) (map[string]*x509.Certificate, m return leafCerts, intCerts } - -// certsToRemove returns all the certificates from oldCerts that aren't present -// in newCerts. Note that newCerts should never be empty, else this function will error. -// We expect newCerts to come from validateRootLeafCerts, which does not return empty sets. -func certsToRemove(oldCerts, newCerts []*x509.Certificate) (map[string]*x509.Certificate, error) { - certsToRemove := make(map[string]*x509.Certificate) - - // Populate a map with all the IDs from newCert - var newCertMap = make(map[string]struct{}) - for _, cert := range newCerts { - certID, err := trustmanager.FingerprintCert(cert) - if err != nil { - logrus.Debugf("error while fingerprinting root certificate with keyID: %s, %v", certID, err) - continue - } - newCertMap[certID] = struct{}{} - } - - // We don't want to "rotate" certificates to an empty set, nor keep old certificates if the - // new root does not trust them. newCerts should come from validRootLeafCerts, which refuses - // to return an empty set, and they should all be fingerprintable, so this should never happen - // - fail just to be sure. - if len(newCertMap) == 0 { - return nil, &ErrRootRotationFail{Reason: "internal error, got no certificates to rotate to"} - } - - // Iterate over all the old certificates and check to see if we should remove them - for _, cert := range oldCerts { - certID, err := trustmanager.FingerprintCert(cert) - if err != nil { - logrus.Debugf("error while fingerprinting root certificate with certID: %s, %v", certID, err) - continue - } - if _, ok := newCertMap[certID]; !ok { - certsToRemove[certID] = cert - } - } - - return certsToRemove, nil -} diff --git a/trustpinning/certs_test.go b/trustpinning/certs_test.go index d2f5a1fee3..f1081da66e 100644 --- a/trustpinning/certs_test.go +++ b/trustpinning/certs_test.go @@ -43,86 +43,6 @@ const rootPubKeyID = "49cf5c6404a35fa41d5a5aa2ce539dfee0d7a2176d0da488914a38603b const targetsPubKeyID = "1fc4fdc38f66558658c5c59b67f1716bdc6a74ef138b023ae5931db69f51d670" -func TestCertsToRemove(t *testing.T) { - // Get a few certificates to test with - cert1, err := trustmanager.LoadCertFromFile("../fixtures/secure.example.com.crt") - require.NoError(t, err) - cert1KeyID, err := trustmanager.FingerprintCert(cert1) - require.NoError(t, err) - - // Get intermediate certificate - cert2, err := trustmanager.LoadCertFromFile("../fixtures/self-signed_secure.example.com.crt") - require.NoError(t, err) - cert2KeyID, err := trustmanager.FingerprintCert(cert2) - require.NoError(t, err) - - // Get leaf certificate - cert3, err := trustmanager.LoadCertFromFile("../fixtures/self-signed_docker.com-notary.crt") - require.NoError(t, err) - cert3KeyID, err := trustmanager.FingerprintCert(cert3) - require.NoError(t, err) - - // Call CertsToRemove with only one old and one new - oldCerts := []*x509.Certificate{cert1} - newCerts := []*x509.Certificate{cert2} - - certificates, err := certsToRemove(oldCerts, newCerts) - require.NoError(t, err) - require.Len(t, certificates, 1) - _, ok := certificates[cert1KeyID] - require.True(t, ok) - - // Call CertsToRemove with two old and one new - oldCerts = []*x509.Certificate{cert1, cert2} - newCerts = []*x509.Certificate{cert3} - - certificates, err = certsToRemove(oldCerts, newCerts) - require.NoError(t, err) - require.Len(t, certificates, 2) - _, ok = certificates[cert1KeyID] - require.True(t, ok) - _, ok = certificates[cert2KeyID] - require.True(t, ok) - _, ok = certificates[cert3KeyID] - require.False(t, ok) - - // Call CertsToRemove with two new and one old - oldCerts = []*x509.Certificate{cert3} - newCerts = []*x509.Certificate{cert2, cert1} - - certificates, err = certsToRemove(oldCerts, newCerts) - require.NoError(t, err) - require.Len(t, certificates, 1) - _, ok = certificates[cert3KeyID] - require.True(t, ok) - _, ok = certificates[cert1KeyID] - require.False(t, ok) - _, ok = certificates[cert2KeyID] - require.False(t, ok) - - // Call CertsToRemove with three old certificates and no new - oldCerts = []*x509.Certificate{cert1, cert2, cert3} - newCerts = []*x509.Certificate{} - - certificates, err = certsToRemove(oldCerts, newCerts) - require.Error(t, err) - - // Call CertsToRemove with three new certificates and no old - oldCerts = []*x509.Certificate{} - newCerts = []*x509.Certificate{cert1, cert2, cert3} - - certificates, err = certsToRemove(oldCerts, newCerts) - require.NoError(t, err) - require.Len(t, certificates, 0) - _, ok = certificates[cert1KeyID] - require.False(t, ok) - _, ok = certificates[cert2KeyID] - require.False(t, ok) - _, ok = certificates[cert3KeyID] - require.False(t, ok) - -} - func TestValidateRoot(t *testing.T) { var testSignedRoot data.Signed var signedRootBytes bytes.Buffer