Update update logic to error out on corrupted previous root metadata

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

client/backwards_compatibility_test.go

@@ -29,10 +29,6 @@ func requireValidFixture(t *testing.T, notaryRepo *NotaryRepository) {
 	for _, targetObj := range notaryRepo.tufRepo.Targets {
 		require.True(t, targetObj.Signed.Expires.After(tenYearsInFuture))
 	}
-
-	for _, cert := range notaryRepo.CertStore.GetCertificates() {
-		require.True(t, cert.NotAfter.After(tenYearsInFuture))
-	}
 }
 
 // recursively copies the contents of one directory into another - ignores

client/client.go

@@ -2,7 +2,6 @@ package client
 
 import (
 	"bytes"
-	"crypto/x509"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -87,7 +86,6 @@ type NotaryRepository struct {
 	CryptoService signed.CryptoService
 	tufRepo       *tuf.Repo
 	roundTrip     http.RoundTripper
-	CertStore     trustmanager.X509Store
 	trustPinning  trustpinning.TrustPinConfig
 }
 
@@ -97,15 +95,6 @@ type NotaryRepository struct {
 func repositoryFromKeystores(baseDir, gun, baseURL string, rt http.RoundTripper,
 	keyStores []trustmanager.KeyStore, trustPin trustpinning.TrustPinConfig) (*NotaryRepository, error) {
 
-	certPath := filepath.Join(baseDir, notary.TrustedCertsDir)
-	certStore, err := trustmanager.NewX509FilteredFileStore(
-		certPath,
-		trustmanager.FilterCertsExpiredSha1,
-	)
-	if err != nil {
-		return nil, err
-	}
-
 	cryptoService := cryptoservice.NewCryptoService(keyStores...)
 
 	nRepo := &NotaryRepository{
@@ -115,7 +104,6 @@ func repositoryFromKeystores(baseDir, gun, baseURL string, rt http.RoundTripper,
 		tufRepoPath:   filepath.Join(baseDir, tufDir, filepath.FromSlash(gun)),
 		CryptoService: cryptoService,
 		roundTrip:     rt,
-		CertStore:     certStore,
 		trustPinning:  trustPin,
 	}
 
@@ -162,22 +150,22 @@ func NewTarget(targetName string, targetPath string) (*Target, error) {
 	return &Target{Name: targetName, Hashes: meta.Hashes, Length: meta.Length}, nil
 }
 
-func rootCertKey(gun string, privKey data.PrivateKey) (*x509.Certificate, data.PublicKey, error) {
+func rootCertKey(gun string, privKey data.PrivateKey) (data.PublicKey, error) {
 	// Hard-coded policy: the generated certificate expires in 10 years.
 	startTime := time.Now()
 	cert, err := cryptoservice.GenerateCertificate(
 		privKey, gun, startTime, startTime.Add(notary.Year*10))
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	x509PublicKey := trustmanager.CertToKey(cert)
 	if x509PublicKey == nil {
-		return nil, nil, fmt.Errorf(
+		return nil, fmt.Errorf(
 			"cannot use regenerated certificate: format %s", cert.PublicKeyAlgorithm)
 	}
 
-	return cert, x509PublicKey, nil
+	return x509PublicKey, nil
 }
 
 // Initialize creates a new repository by using rootKey as the root Key for the
@@ -218,11 +206,10 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		}
 	}
 
-	rootCert, rootKey, err := rootCertKey(r.gun, privKey)
+	rootKey, err := rootCertKey(r.gun, privKey)
 	if err != nil {
 		return err
 	}
-	r.CertStore.AddCert(rootCert)
 
 	var (
 		rootRole = data.NewBaseRole(
@@ -790,9 +777,6 @@ func (r *NotaryRepository) Update(forWrite bool) error {
 // Partially populates r.tufRepo with this root metadata (only; use
 // tufclient.Client.Update to load the rest).
 //
-// As another side effect, r.CertManager's list of trusted certificates
-// is updated with data from the loaded root.json.
-//
 // Fails if the remote server is reachable and does not know the repo
 // (i.e. before the first r.Publish()), in which case the error is
 // store.ErrMetaNotFound, or if the root metadata (from whichever source is used)
@@ -883,19 +867,20 @@ func (r *NotaryRepository) validateRoot(rootJSON []byte, fromRemote bool) (*data
 	var prevRoot *data.SignedRoot
 	if fromRemote {
 		prevRootJSON, err := r.fileStore.GetMeta(data.CanonicalRootRole, -1)
+		// A previous root exists, so we attempt to use it
+		// If for some reason we can't extract it (ex: it's corrupted), we should error client-side to be conservative
 		if err == nil {
 			prevSignedRoot := &data.Signed{}
 			err = json.Unmarshal(prevRootJSON, prevSignedRoot)
-			if err == nil {
+			if err != nil {
-				prevRoot, err = data.RootFromSigned(prevSignedRoot)
+				return nil, &trustpinning.ErrValidationFail{fmt.Sprintf("unable to unmarshal previously trusted root from disk: %v", err)}
+			}
+			prevRoot, err = data.RootFromSigned(prevSignedRoot)
+			if err != nil {
+				return nil, &trustpinning.ErrValidationFail{fmt.Sprintf("error loading previously trusted root into valid role format: %v", err)}
 			}
 		}
 	}
-	// If we had any errors while trying to retrieve the previous root, just set it to nil
-	if err != nil {
-		prevRoot = nil
-	}
-
 	err = trustpinning.ValidateRoot(prevRoot, root, r.gun, r.trustPinning)
 	if err != nil {
 		return nil, err
@@ -947,7 +932,7 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 		if err != nil {
 			return err
 		}
-		_, pubKey, err = rootCertKey(r.gun, privKey)
+		pubKey, err = rootCertKey(r.gun, privKey)
 		if err != nil {
 			return err
 		}
@@ -982,26 +967,12 @@ func (r *NotaryRepository) rootFileKeyChange(cl changelist.Changelist, role, act
 	return cl.Add(c)
 }
 
-// DeleteTrustData removes the trust data stored for this repo in the TUF cache and certificate store on the client side
+// DeleteTrustData removes the trust data stored for this repo in the TUF cache on the client side
 func (r *NotaryRepository) DeleteTrustData() error {
 	// Clear TUF files and cache
 	if err := r.fileStore.RemoveAll(); err != nil {
 		return fmt.Errorf("error clearing TUF repo data: %v", err)
 	}
 	r.tufRepo = tuf.NewRepo(nil)
-	// Clear certificates
-	certificates, err := r.CertStore.GetCertificatesByCN(r.gun)
-	if err != nil {
-		// If there were no certificates to delete, we're done
-		if _, ok := err.(*trustmanager.ErrNoCertificatesFound); ok {
-			return nil
-		}
-		return fmt.Errorf("error retrieving certificates for %s: %v", r.gun, err)
-	}
-	for _, cert := range certificates {
-		if err := r.CertStore.RemoveCert(cert); err != nil {
-			return fmt.Errorf("error removing certificate: %v: %v", cert, err)
-		}
-	}
 	return nil
 }

client/client_root_validation_test.go

@@ -33,11 +33,6 @@ func validateRootSuccessfully(t *testing.T, rootType string) {
 	err := repo.tufRepo.InitTimestamp()
 	require.NoError(t, err, "error creating repository: %s", err)
 
-	// Initialize is supposed to have created new certificate for this repository
-	// Lets check for it and store it for later use
-	allCerts := repo.CertStore.GetCertificates()
-	require.Len(t, allCerts, 1)
-
 	fakeServerData(t, repo, mux, keys)
 
 	//

client/client_test.go

@@ -1911,17 +1911,15 @@ func TestPublishRootCorrupt(t *testing.T) {
 	defer os.RemoveAll(repo.baseDir)
 	testPublishBadMetadata(t, data.CanonicalRootRole, repo, false, false)
 
-	// publish first - publish should still succeed if root corrupt since the
+	// publish first - publish should still fail if the local root is corrupt since
-	// remote root is signed with the same key.
+	// we can't determine whether remote root is signed with the same key.
 	repo, _ = initializeRepo(t, data.ECDSAKey, "docker.com/notary2", ts.URL, false)
 	defer os.RemoveAll(repo.baseDir)
-	testPublishBadMetadata(t, data.CanonicalRootRole, repo, true, true)
+	testPublishBadMetadata(t, data.CanonicalRootRole, repo, true, false)
 }
 
 // When publishing snapshot, root, or target, if the repo hasn't been published
-// before, if the metadata is corrupt, it can't be published.  If it has been
+// before, if the metadata is corrupt, it can't be published.
-// published already, then the corrupt metadata can just be re-downloaded, so
-// publishing is successful.
 func testPublishBadMetadata(t *testing.T, roleName string, repo *NotaryRepository,
 	publishFirst, succeeds bool) {
 
@@ -1938,7 +1936,11 @@ func testPublishBadMetadata(t *testing.T, roleName string, repo *NotaryRepositor
 		require.NoError(t, err)
 	} else {
 		require.Error(t, err)
-		require.IsType(t, &regJson.SyntaxError{}, err)
+		if roleName == data.CanonicalRootRole && publishFirst {
+			require.IsType(t, &trustpinning.ErrValidationFail{}, err)
+		} else {
+			require.IsType(t, &regJson.SyntaxError{}, err)
+		}
 	}
 
 	// make an unreadable file by creating a directory instead of a file
@@ -1950,7 +1952,7 @@ func testPublishBadMetadata(t *testing.T, roleName string, repo *NotaryRepositor
 	defer os.RemoveAll(path)
 
 	err = repo.Publish()
-	if succeeds {
+	if succeeds || publishFirst {
 		require.NoError(t, err)
 	} else {
 		require.Error(t, err)

client/client_update_test.go

@@ -208,8 +208,9 @@ var waysToMessUpLocalMetadata = []swizzleExpectations{
 	// actively sabotage and break their own local repo (particularly the root.json)
 }
 
-// If a repo has corrupt metadata (in that the hash doesn't match the snapshot) or
+// If a repo has missing metadata, an update will replace all of it
-// missing metadata, an update will replace all of it
+// If a repo has corrupt metadata for root, the update will fail
+// For other roles, corrupt metadata will be replaced
 func TestUpdateReplacesCorruptOrMissingMetadata(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping test in short mode")
@@ -230,104 +231,115 @@ func TestUpdateReplacesCorruptOrMissingMetadata(t *testing.T) {
 	repoSwizzler := testutils.NewMetadataSwizzler("docker.com/notary", serverMeta, cs)
 	repoSwizzler.MetadataCache = repo.fileStore
 
+	origMeta := testutils.CopyRepoMetadata(serverMeta)
+
 	for _, role := range repoSwizzler.Roles {
 		for _, expt := range waysToMessUpLocalMetadata {
 			text, messItUp := expt.desc, expt.swizzle
 			for _, forWrite := range []bool{true, false} {
 				require.NoError(t, messItUp(repoSwizzler, role), "could not fuzz %s (%s)", role, text)
 				err := repo.Update(forWrite)
-				require.NoError(t, err)
+				// if this is a root role, we should error if it's corrupted data
-				for r, expected := range serverMeta {
+				if role == data.CanonicalRootRole && expt.desc == "invalid JSON" {
-					actual, err := repo.fileStore.GetMeta(r, -1)
+					require.Error(t, err)
-					require.NoError(t, err, "problem getting repo metadata for %s", role)
+					// revert our original metadata
-					require.True(t, bytes.Equal(expected, actual),
+					for role := range origMeta {
-						"%s for %s: expected to recover after update", text, role)
+						require.NoError(t, repo.fileStore.SetMeta(role, origMeta[role]))
+					}
+				} else {
+					require.NoError(t, err)
+					for r, expected := range serverMeta {
+						actual, err := repo.fileStore.GetMeta(r, -1)
+						require.NoError(t, err, "problem getting repo metadata for %s", role)
+						require.True(t, bytes.Equal(expected, actual),
+							"%s for %s: expected to recover after update", text, role)
+					}
 				}
 			}
 		}
 	}
 }
 
-// TODO(riyazdf): this test goes against the new root pinning because the certstore
+// If a repo has an invalid root (signed by wrong key, expired, invalid version,
-// no longer exists, so roots on the local filestore must be correct.
+// invalid number of signatures, etc.), the repo will just get the new root from
-// TODO(riyazdf): break this test up to only test valid rejections, not associated
+// the server, whether or not the update is for writing (forced update), but
-// with rotations -- such as expired metadata, etc.
+// it will refuse to update if the root key has changed and the new root is
-//// If a repo has an invalid root (signed by wrong key, expired, invalid version,
+// not signed by the old and new key
-//// invalid number of signatures, etc.), the repo will just get the new root from
+func TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign(t *testing.T) {
-//// the server, whether or not the update is for writing (forced update), but
+	if testing.Short() {
-//// it will refuse to update if the root key has changed and the new root is
+		t.Skip("skipping test in short mode")
-//// not signed by the old and new key
+	}
-//func TestUpdateFailsIfServerRootKeyChangedWithoutMultiSign(t *testing.T) {
+
-//	if testing.Short() {
+	serverMeta, serverSwizzler := newServerSwizzler(t)
-//		t.Skip("skipping test in short mode")
+	origMeta := testutils.CopyRepoMetadata(serverMeta)
-//	}
+
-//
+	ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound, "docker.com/notary")
-//	serverMeta, serverSwizzler := newServerSwizzler(t)
+	defer ts.Close()
-//	origMeta := testutils.CopyRepoMetadata(serverMeta)
+
-//
+	repo := newBlankRepo(t, ts.URL)
-//	ts := readOnlyServer(t, serverSwizzler.MetadataCache, http.StatusNotFound, "docker.com/notary")
+	defer os.RemoveAll(repo.baseDir)
-//	defer ts.Close()
+
-//
+	err := repo.Update(false) // ensure we have all metadata to start with
-//	repo := newBlankRepo(t, ts.URL)
+	require.NoError(t, err)
-//	defer os.RemoveAll(repo.baseDir)
+
-//
+	// rotate the server's root.json root key so that they no longer match trust anchors
-//	err := repo.Update(false) // ensure we have all metadata to start with
+	require.NoError(t, serverSwizzler.ChangeRootKey())
-//	require.NoError(t, err)
+	// bump versions, update snapshot and timestamp too so it will not fail on a hash
-//
+	bumpVersions(t, serverSwizzler, 1)
-//	// rotate the server's root.json root key so that they no longer match trust anchors
+
-//	require.NoError(t, serverSwizzler.ChangeRootKey())
+	// we want to swizzle the local cache, not the server, so create a new one
-//	// bump versions, update snapshot and timestamp too so it will not fail on a hash
+	repoSwizzler := &testutils.MetadataSwizzler{
-//	bumpVersions(t, serverSwizzler, 1)
+		MetadataCache: repo.fileStore,
-//
+		CryptoService: serverSwizzler.CryptoService,
-//	// we want to swizzle the local cache, not the server, so create a new one
+		Roles:         serverSwizzler.Roles,
-//	repoSwizzler := &testutils.MetadataSwizzler{
+	}
-//		MetadataCache: repo.fileStore,
+
-//		CryptoService: serverSwizzler.CryptoService,
+	for _, expt := range waysToMessUpLocalMetadata {
-//		Roles:         serverSwizzler.Roles,
+		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)
-//	for _, expt := range waysToMessUpLocalMetadata {
+			messedUpMeta, err := repo.fileStore.GetMeta(data.CanonicalRootRole, -1)
-//		text, messItUp := expt.desc, expt.swizzle
+
-//		for _, forWrite := range []bool{true, false} {
+			if _, ok := err.(store.ErrMetaNotFound); ok { // one of the ways to mess up is to delete metadata
-//			require.NoError(t, messItUp(repoSwizzler, data.CanonicalRootRole), "could not fuzz root (%s)", text)
+
-//			messedUpMeta, err := repo.fileStore.GetMeta(data.CanonicalRootRole, -1)
+				err = repo.Update(forWrite)
-//
+				// the new server has a different root key, but we don't have any way of pinning against a previous root
-//			if _, ok := err.(store.ErrMetaNotFound); ok { // one of the ways to mess up is to delete metadata
+				require.NoError(t, err)
-//
+				// revert our original metadata
-//				err = repo.Update(forWrite)
+				for role := range origMeta {
-//				require.Error(t, err) // the new server has a different root key, update fails
+					require.NoError(t, repo.fileStore.SetMeta(role, origMeta[role]))
-//
+				}
-//			} else {
+			} else {
-//
+
-//				require.NoError(t, err)
+				require.NoError(t, err)
-//
+
-//				err = repo.Update(forWrite)
+				err = repo.Update(forWrite)
-//				require.Error(t, err) // the new server has a different root, update fails
+				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
+				// 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
+				// have downloaded a new timestamp and maybe snapshot.  But the root should be the
-//				// same because it has failed to update.
+				// same because it has failed to update.
-//				for role, expected := range origMeta {
+				for role, expected := range origMeta {
-//					if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole {
+					if role != data.CanonicalTimestampRole && role != data.CanonicalSnapshotRole {
-//						actual, err := repo.fileStore.GetMeta(role, -1)
+						actual, err := repo.fileStore.GetMeta(role, -1)
-//						require.NoError(t, err, "problem getting repo metadata for %s", role)
+						require.NoError(t, err, "problem getting repo metadata for %s", role)
-//
+
-//						if role == data.CanonicalRootRole {
+						if role == data.CanonicalRootRole {
-//							expected = messedUpMeta
+							expected = messedUpMeta
-//						}
+						}
-//						require.True(t, bytes.Equal(expected, actual),
+						require.True(t, bytes.Equal(expected, actual),
-//							"%s for %s: expected to not have updated", text, role)
+							"%s for %s: expected to not have updated -- swizzle method %s", text, role, expt.desc)
-//					}
+					}
-//				}
+				}
-//
+
-//			}
+			}
-//
+
-//			// revert our original root metadata
+			// revert our original root metadata
-//			require.NoError(t,
+			require.NoError(t,
-//				repo.fileStore.SetMeta(data.CanonicalRootRole, origMeta[data.CanonicalRootRole]))
+				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
@@ -774,15 +786,15 @@ func testUpdateRemoteFileChecksumWrong(t *testing.T, opts updateOpts, errExpecte
 // this does not include delete, which is tested separately so we can try to get
 // 404s and 503s
 var waysToMessUpServer = []swizzleExpectations{
-	{desc: "invalid JSON", expectErrs: []interface{}{&json.SyntaxError{}},
+	{desc: "invalid JSON", expectErrs: []interface{}{&trustpinning.ErrValidationFail{}, &json.SyntaxError{}},
 		swizzle: (*testutils.MetadataSwizzler).SetInvalidJSON},
 
-	{desc: "an invalid Signed", expectErrs: []interface{}{&json.UnmarshalTypeError{}},
+	{desc: "an invalid Signed", expectErrs: []interface{}{&trustpinning.ErrValidationFail{}, &json.UnmarshalTypeError{}},
 		swizzle: (*testutils.MetadataSwizzler).SetInvalidSigned},
 
 	{desc: "an invalid SignedMeta",
 		// it depends which field gets unmarshalled first
-		expectErrs: []interface{}{&json.UnmarshalTypeError{}, &time.ParseError{}},
+		expectErrs: []interface{}{&trustpinning.ErrValidationFail{}, &json.UnmarshalTypeError{}, &time.ParseError{}},
 		swizzle:    (*testutils.MetadataSwizzler).SetInvalidSignedMeta},
 
 	// for the errors below, when we bootstrap root, we get cert.ErrValidationFail failures
@@ -1201,12 +1213,6 @@ func TestUpdateLocalAndRemoteRootCorrupt(t *testing.T) {
 	}
 	for _, localExpt := range waysToMessUpLocalMetadata {
 		for _, serverExpt := range waysToMessUpServer {
-			if localExpt.desc == "expired metadata" && serverExpt.desc == "lower metadata version" {
-				// TODO: bug right now where if the local metadata is invalid, we just download a
-				// new version - we verify the signatures and everything, but don't check the version
-				// against the previous if we can
-				continue
-			}
 			testUpdateLocalAndRemoteRootCorrupt(t, true, localExpt, serverExpt)
 			testUpdateLocalAndRemoteRootCorrupt(t, false, localExpt, serverExpt)
 		}

cmd/notary/cert.go

@@ -1,194 +0,0 @@
-package main
-
-import (
-	"crypto/x509"
-	"fmt"
-	"os"
-	"path/filepath"
-
-	"github.com/docker/notary"
-	notaryclient "github.com/docker/notary/client"
-	"github.com/docker/notary/passphrase"
-	"github.com/docker/notary/trustmanager"
-	"github.com/spf13/cobra"
-	"github.com/spf13/viper"
-)
-
-var cmdCertTemplate = usageTemplate{
-	Use:   "cert",
-	Short: "Operates on certificates.",
-	Long:  `Operations on certificates.`,
-}
-
-var cmdCertListTemplate = usageTemplate{
-	Use:   "list",
-	Short: "Lists certificates.",
-	Long:  "Lists root certificates known to notary.",
-}
-
-var cmdCertRemoveTemplate = usageTemplate{
-	Use:   "remove [ certID ]",
-	Short: "Removes the certificate with the given cert ID.",
-	Long:  "Remove the certificate with the given cert ID from the local host.",
-}
-
-type certCommander struct {
-	// these need to be set
-	configGetter func() (*viper.Viper, error)
-	retriever    passphrase.Retriever
-
-	// these are for command line parsing - no need to set
-	certRemoveGUN string
-	certRemoveYes bool
-}
-
-func (c *certCommander) GetCommand() *cobra.Command {
-	cmd := cmdCertTemplate.ToCommand(nil)
-	cmd.AddCommand(cmdCertListTemplate.ToCommand(c.certList))
-
-	cmdCertRemove := cmdCertRemoveTemplate.ToCommand(c.certRemove)
-	cmdCertRemove.Flags().StringVarP(
-		&c.certRemoveGUN, "gun", "g", "", "Globally unique name to delete certificates for")
-	cmdCertRemove.Flags().BoolVarP(
-		&c.certRemoveYes, "yes", "y", false, "Answer yes to the removal question (no confirmation)")
-
-	cmd.AddCommand(cmdCertRemove)
-
-	return cmd
-}
-
-// certRemove deletes a certificate given a cert ID or a gun
-// If given a gun, certRemove will also remove local TUF data
-func (c *certCommander) certRemove(cmd *cobra.Command, args []string) error {
-	// If the user hasn't provided -g with a gun, or a cert ID, show usage
-	// If the user provided -g and a cert ID, also show usage
-	if (len(args) < 1 && c.certRemoveGUN == "") || (len(args) > 0 && c.certRemoveGUN != "") {
-		cmd.Usage()
-		return fmt.Errorf("Must specify the cert ID or the GUN of the certificates to remove")
-	}
-	config, err := c.configGetter()
-	if err != nil {
-		return err
-	}
-
-	trustDir := config.GetString("trust_dir")
-	certPath := filepath.Join(trustDir, notary.TrustedCertsDir)
-	certStore, err := trustmanager.NewX509FilteredFileStore(
-		certPath,
-		trustmanager.FilterCertsExpiredSha1,
-	)
-	if err != nil {
-		return fmt.Errorf("Failed to create a new truststore with directory: %s", trustDir)
-	}
-
-	var certsToRemove []*x509.Certificate
-	var certFoundByID *x509.Certificate
-	var removeTrustData bool
-
-	// If there is no GUN, we expect a cert ID
-	if c.certRemoveGUN == "" {
-		certID := args[0]
-		// Attempt to find this certificate
-		certFoundByID, err = certStore.GetCertificateByCertID(certID)
-		if err != nil {
-			// This is an invalid ID, the user might have forgotten a character
-			if len(certID) != notary.Sha256HexSize {
-				return fmt.Errorf("Unable to retrieve certificate with invalid certificate ID provided: %s", certID)
-			}
-			return fmt.Errorf("Unable to retrieve certificate with cert ID: %s", certID)
-		}
-		// the GUN is the CN from the certificate
-		c.certRemoveGUN = certFoundByID.Subject.CommonName
-		certsToRemove = []*x509.Certificate{certFoundByID}
-	}
-
-	toRemove, err := certStore.GetCertificatesByCN(c.certRemoveGUN)
-	// We could not find any certificates matching the user's query, so propagate the error
-	if err != nil {
-		return fmt.Errorf("%v", err)
-	}
-
-	// If we specified a GUN or if the ID we specified is the only certificate with its CN, remove all GUN certs and trust data too
-	if certFoundByID == nil || len(toRemove) == 1 {
-		removeTrustData = true
-		certsToRemove = toRemove
-	}
-
-	// List all the certificates about to be removed
-	cmd.Printf("The following certificates will be removed:\n\n")
-	for _, cert := range certsToRemove {
-		// This error can't occur because we're getting certs off of an
-		// x509 store that indexes by ID.
-		certID, _ := trustmanager.FingerprintCert(cert)
-		cmd.Printf("%s - %s\n", cert.Subject.CommonName, certID)
-	}
-	// If we were given a GUN or the last ID for a GUN, inform the user that we'll also delete all TUF data
-	if removeTrustData {
-		cmd.Printf("\nAll local trust data will be removed for %s\n", c.certRemoveGUN)
-	}
-	cmd.Println("\nAre you sure you want to remove these certificates? (yes/no)")
-
-	// Ask for confirmation before removing certificates, unless -y is provided
-	if !c.certRemoveYes {
-		confirmed := askConfirm(os.Stdin)
-		if !confirmed {
-			return fmt.Errorf("Aborting action.")
-		}
-	}
-
-	if removeTrustData {
-		// Remove all TUF data, so call RemoveTrustData on a NotaryRepository with the GUN
-		// no online operations are performed so the transport argument is nil
-		trustPin, err := getTrustPinning(config)
-		if err != nil {
-			return err
-		}
-		nRepo, err := notaryclient.NewNotaryRepository(
-			trustDir, c.certRemoveGUN, getRemoteTrustServer(config), nil, c.retriever, trustPin)
-		if err != nil {
-			return fmt.Errorf("Could not establish trust data for GUN %s", c.certRemoveGUN)
-		}
-		// DeleteTrustData will pick up all of the same certificates by GUN (CN) and remove them
-		err = nRepo.DeleteTrustData()
-		if err != nil {
-			return fmt.Errorf("Failed to delete trust data for %s", c.certRemoveGUN)
-		}
-	} else {
-		for _, cert := range certsToRemove {
-			err = certStore.RemoveCert(cert)
-			if err != nil {
-				return fmt.Errorf("Failed to remove cert %s", cert)
-			}
-		}
-	}
-	return nil
-}
-
-func (c *certCommander) certList(cmd *cobra.Command, args []string) error {
-	if len(args) > 0 {
-		cmd.Usage()
-		return fmt.Errorf("")
-	}
-	config, err := c.configGetter()
-	if err != nil {
-		return err
-	}
-
-	trustDir := config.GetString("trust_dir")
-	certPath := filepath.Join(trustDir, notary.TrustedCertsDir)
-	// Load all individual (non-CA) certificates that aren't expired and don't use SHA1
-	certStore, err := trustmanager.NewX509FilteredFileStore(
-		certPath,
-		trustmanager.FilterCertsExpiredSha1,
-	)
-	if err != nil {
-		return fmt.Errorf("Failed to create a new truststore with directory: %s", trustDir)
-	}
-
-	trustedCerts := certStore.GetCertificates()
-
-	cmd.Println("")
-	prettyPrintCerts(trustedCerts, cmd.Out())
-	cmd.Println("")
-	return nil
-}

cmd/notary/integration_test.go

@@ -1346,98 +1346,6 @@ func TestClientKeyImportExportAllRoles(t *testing.T) {
 	}
 }
 
-func assertNumCerts(t *testing.T, tempDir string, expectedNum int) []string {
-	output, err := runCommand(t, tempDir, "cert", "list")
-	require.NoError(t, err)
-	lines := splitLines(strings.TrimSpace(output))
-
-	if expectedNum == 0 {
-		require.Len(t, lines, 1)
-		require.Equal(t, "No trusted root certificates present.", lines[0])
-		return []string{}
-	}
-
-	require.Len(t, lines, expectedNum+2)
-	return lines[2:]
-}
-
-// TestClientCertInteraction
-func TestClientCertInteraction(t *testing.T) {
-	// -- setup --
-	setUp(t)
-
-	tempDir := tempDirWithConfig(t, "{}")
-	defer os.RemoveAll(tempDir)
-
-	server := setupServer()
-	defer server.Close()
-
-	// -- tests --
-	_, err := runCommand(t, tempDir, "-s", server.URL, "init", "gun1")
-	require.NoError(t, err)
-	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2")
-	require.NoError(t, err)
-	certs := assertNumCerts(t, tempDir, 2)
-	// root is always on disk, because even if there's a yubikey a backup is created
-	assertNumKeys(t, tempDir, 1, 4, true)
-
-	// remove certs for one gun
-	_, err = runCommand(t, tempDir, "cert", "remove", "-g", "gun1", "-y")
-	require.NoError(t, err)
-	certs = assertNumCerts(t, tempDir, 1)
-	// assert that when we remove cert by gun, we do not remove repo signing keys
-	// (root is always on disk, because even if there's a yubikey a backup is created)
-	assertNumKeys(t, tempDir, 1, 4, true)
-	// assert that when we remove cert by gun, we also remove TUF metadata
-	_, err = os.Stat(filepath.Join(tempDir, "tuf", "gun1"))
-	require.Error(t, err)
-
-	// remove a single cert
-	certID := strings.Fields(certs[0])[1]
-	// passing an empty gun here because the string for the previous gun has
-	// has already been stored (a drawback of running these commands without)
-	// shelling out
-	_, err = runCommand(t, tempDir, "cert", "remove", certID, "-y", "-g", "")
-	require.NoError(t, err)
-	assertNumCerts(t, tempDir, 0)
-	// assert that when we remove the last cert ID for a gun, we also remove TUF metadata
-	_, err = os.Stat(filepath.Join(tempDir, "tuf", "gun2"))
-	require.Error(t, err)
-
-	// Setup certificate with nonexistent repo GUN
-	// Check that we can only remove one certificate when specifying one ID
-	startTime := time.Now()
-	privKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
-	require.NoError(t, err)
-	noGunCert, err := cryptoservice.GenerateCertificate(
-		privKey, "nonexistent", startTime, startTime.AddDate(10, 0, 0))
-	require.NoError(t, err)
-	certStore, err := trustmanager.NewX509FileStore(filepath.Join(tempDir, "trusted_certificates"))
-	require.NoError(t, err)
-	err = certStore.AddCert(noGunCert)
-	require.NoError(t, err)
-
-	certs = assertNumCerts(t, tempDir, 1)
-	certID = strings.Fields(certs[0])[1]
-
-	privKey, err = trustmanager.GenerateECDSAKey(rand.Reader)
-	require.NoError(t, err)
-	noGunCert2, err := cryptoservice.GenerateCertificate(
-		privKey, "nonexistent", startTime, startTime.AddDate(10, 0, 0))
-	require.NoError(t, err)
-	err = certStore.AddCert(noGunCert2)
-	require.NoError(t, err)
-
-	certs = assertNumCerts(t, tempDir, 2)
-
-	// passing an empty gun to overwrite previously stored gun
-	_, err = runCommand(t, tempDir, "cert", "remove", certID, "-y", "-g", "")
-	require.NoError(t, err)
-
-	// Since another cert with the same GUN exists, we didn't remove everything
-	assertNumCerts(t, tempDir, 1)
-}
-
 // Tests default root key generation
 func TestDefaultRootKeyGeneration(t *testing.T) {
 	// -- setup --

cmd/notary/main.go

@@ -180,11 +180,6 @@ func (n *notaryCommander) GetCommand() *cobra.Command {
 		retriever:    n.getRetriever(),
 	}
 
-	cmdCertGenerator := &certCommander{
-		configGetter: n.parseConfig,
-		retriever:    n.getRetriever(),
-	}
-
 	cmdTufGenerator := &tufCommander{
 		configGetter: n.parseConfig,
 		retriever:    n.getRetriever(),
@@ -192,7 +187,6 @@ func (n *notaryCommander) GetCommand() *cobra.Command {
 
 	notaryCmd.AddCommand(cmdKeyGenerator.GetCommand())
 	notaryCmd.AddCommand(cmdDelegationGenerator.GetCommand())
-	notaryCmd.AddCommand(cmdCertGenerator.GetCommand())
 
 	cmdTufGenerator.AddToCommand(&notaryCmd)
 

cmd/notary/main_test.go

@@ -164,8 +164,6 @@ var exampleValidCommands = []string{
 	"key import backup.pem",
 	"key remove e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
 	"key passwd e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
-	"cert list",
-	"cert remove e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
 	"delegation list repo",
 	"delegation add repo targets/releases path/to/pem/file.pem",
 	"delegation remove repo targets/releases",
@@ -205,8 +203,8 @@ func TestInsufficientArgumentsReturnsErrorAndPrintsUsage(t *testing.T) {
 		cmd.SetOutput(b)
 
 		arglist := strings.Fields(args)
-		if args == "key list" || args == "cert list" || args == "key generate rsa" {
+		if args == "key list" || args == "key generate rsa" {
-			// in these case, "key" or "cert" or "key generate" are valid commands, so add an arg to them instead
+			// in these case, "key" or "key generate" are valid commands, so add an arg to them instead
 			arglist = append(arglist, "extraArg")
 		} else {
 			arglist = arglist[:len(arglist)-1]
@@ -240,8 +238,8 @@ func TestBareCommandPrintsUsageAndNoError(t *testing.T) {
 	// usage is printed
 	require.Contains(t, b.String(), "Usage:", "expected usage when running `notary`")
 
-	// notary key, notary cert, and notary delegation
+	// notary key and notary delegation
-	for _, bareCommand := range []string{"key", "cert", "delegation"} {
+	for _, bareCommand := range []string{"key", "delegation"} {
 		b := new(bytes.Buffer)
 		cmd := NewNotaryCommand()
 		cmd.SetOutput(b)

cmd/notary/prettyprint.go

@@ -1,14 +1,11 @@
 package main
 
 import (
-	"crypto/x509"
 	"encoding/hex"
 	"fmt"
 	"io"
-	"math"
 	"sort"
 	"strings"
-	"time"
 
 	"github.com/docker/notary/client"
 	"github.com/docker/notary/trustmanager"
@@ -204,52 +201,3 @@ func prettyPrintPaths(paths []string) string {
 	}
 	return strings.Join(prettyPaths, "\n")
 }
-
-// --- pretty printing certs ---
-
-// cert by repo name then expiry time.  Don't bother sorting by fingerprint.
-type certSorter []*x509.Certificate
-
-func (t certSorter) Len() int      { return len(t) }
-func (t certSorter) Swap(i, j int) { t[i], t[j] = t[j], t[i] }
-func (t certSorter) Less(i, j int) bool {
-	if t[i].Subject.CommonName < t[j].Subject.CommonName {
-		return true
-	} else if t[i].Subject.CommonName > t[j].Subject.CommonName {
-		return false
-	}
-
-	return t[i].NotAfter.Before(t[j].NotAfter)
-}
-
-// Given a list of Ceritifcates in order of listing preference, pretty-prints
-// the cert common name, fingerprint, and expiry
-func prettyPrintCerts(certs []*x509.Certificate, writer io.Writer) {
-	if len(certs) == 0 {
-		writer.Write([]byte("\nNo trusted root certificates present.\n\n"))
-		return
-	}
-
-	sort.Stable(certSorter(certs))
-
-	table := getTable([]string{
-		"GUN", "Fingerprint of Trusted Root Certificate", "Expires In"}, writer)
-
-	for _, c := range certs {
-		days := math.Floor(c.NotAfter.Sub(time.Now()).Hours() / 24)
-		expiryString := "< 1 day"
-		if days == 1 {
-			expiryString = "1 day"
-		} else if days > 1 {
-			expiryString = fmt.Sprintf("%d days", int(days))
-		}
-
-		certID, err := trustmanager.FingerprintCert(c)
-		if err != nil {
-			fatalf("Could not fingerprint certificate: %v", err)
-		}
-
-		table.Append([]string{c.Subject.CommonName, certID, expiryString})
-	}
-	table.Render()
-}

cmd/notary/prettyprint_test.go

@@ -3,7 +3,6 @@ package main
 import (
 	"bytes"
 	"crypto/rand"
-	"crypto/x509"
 	"encoding/hex"
 	"fmt"
 	"io/ioutil"
@@ -11,10 +10,8 @@ import (
 	"sort"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/docker/notary/client"
-	"github.com/docker/notary/cryptoservice"
 	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
@@ -201,19 +198,6 @@ func TestPrettyPrintSortedTargets(t *testing.T) {
 	}
 }
 
-// --- tests for pretty printing certs ---
-
-func generateCertificate(t *testing.T, gun string, expireInHours int64) *x509.Certificate {
-	ecdsaPrivKey, err := trustmanager.GenerateECDSAKey(rand.Reader)
-	require.NoError(t, err)
-
-	startTime := time.Now()
-	endTime := startTime.Add(time.Hour * time.Duration(expireInHours))
-	cert, err := cryptoservice.GenerateCertificate(ecdsaPrivKey, gun, startTime, endTime)
-	require.NoError(t, err)
-	return cert
-}
-
 // --- tests for pretty printing roles ---
 
 // If there are no roles, no table is printed, only a line saying that there
@@ -268,53 +252,3 @@ func TestPrettyPrintSortedRoles(t *testing.T) {
 		require.Equal(t, expected[i], splitted)
 	}
 }
-
-// If there are no certs in the cert store store, a message that there are no
-// certs should be displayed.
-func TestPrettyPrintZeroCerts(t *testing.T) {
-	var b bytes.Buffer
-	prettyPrintCerts([]*x509.Certificate{}, &b)
-	text, err := ioutil.ReadAll(&b)
-	require.NoError(t, err)
-
-	lines := strings.Split(strings.TrimSpace(string(text)), "\n")
-	require.Len(t, lines, 1)
-	require.Equal(t, "No trusted root certificates present.", lines[0])
-}
-
-// Certificates are pretty-printed in table form sorted by gun and then expiry
-func TestPrettyPrintSortedCerts(t *testing.T) {
-	unsorted := []*x509.Certificate{
-		generateCertificate(t, "xylitol", 77),    // 3 days 5 hours
-		generateCertificate(t, "xylitol", 12),    // less than 1 day
-		generateCertificate(t, "cheesecake", 25), // a little more than 1 day
-		generateCertificate(t, "baklava", 239),   // almost 10 days
-	}
-
-	var b bytes.Buffer
-	prettyPrintCerts(unsorted, &b)
-	text, err := ioutil.ReadAll(&b)
-	require.NoError(t, err)
-
-	expected := [][]string{
-		{"baklava", "9 days"},
-		{"cheesecake", "1 day"},
-		{"xylitol", "< 1 day"},
-		{"xylitol", "3 days"},
-	}
-
-	lines := strings.Split(strings.TrimSpace(string(text)), "\n")
-	require.Len(t, lines, len(expected)+2)
-
-	// starts with headers
-	require.True(t, reflect.DeepEqual(strings.Fields(lines[0]), strings.Fields(
-		"GUN     FINGERPRINT OF TRUSTED ROOT CERTIFICATE      EXPIRES IN")))
-	require.Equal(t, "----", lines[1][:4])
-
-	for i, line := range lines[2:] {
-		splitted := strings.Fields(line)
-		require.True(t, len(splitted) >= 3)
-		require.Equal(t, expected[i][0], splitted[0])
-		require.Equal(t, expected[i][1], strings.Join(splitted[2:], " "))
-	}
-}

trustpinning/certs_test.go

@@ -568,12 +568,41 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string
 	origRootCert := certificates[0]
 	replRootCert := certificates[1]
 
-	// We need the PEM representation of the replacement key to put it into the TUF data
+	// Set up the previous root prior to rotating
+	// We need the PEM representation of the original key to put it into the TUF data
 	origRootPEMCert := trustmanager.CertToPEM(origRootCert)
-	replRootPEMCert := trustmanager.CertToPEM(replRootCert)
 
 	// Tuf key with PEM-encoded x509 certificate
 	origRootKey := data.NewPublicKey(rootKeyType, origRootPEMCert)
+
+	origRootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{origRootKey.ID()}, nil)
+	require.NoError(t, err)
+
+	origTestRoot, err := data.NewRoot(
+		map[string]data.PublicKey{origRootKey.ID(): origRootKey},
+		map[string]*data.RootRole{
+			data.CanonicalRootRole:      &origRootRole.RootRole,
+			data.CanonicalTargetsRole:   &origRootRole.RootRole,
+			data.CanonicalSnapshotRole:  &origRootRole.RootRole,
+			data.CanonicalTimestampRole: &origRootRole.RootRole,
+		},
+		false,
+	)
+	require.NoError(t, err, "Failed to create new root")
+
+	signedOrigTestRoot, err := origTestRoot.ToSigned()
+	require.NoError(t, err)
+
+	// We only sign with the new key, and not with the original one.
+	err = signed.Sign(cs, signedOrigTestRoot, []data.PublicKey{origRootKey}, 1, nil)
+	require.NoError(t, err)
+	prevRoot, err := data.RootFromSigned(signedOrigTestRoot)
+	require.NoError(t, err)
+
+	// We need the PEM representation of the replacement key to put it into the TUF data
+	replRootPEMCert := trustmanager.CertToPEM(replRootCert)
+
+	// Tuf key with PEM-encoded x509 certificate
 	replRootKey := data.NewPublicKey(rootKeyType, replRootPEMCert)
 
 	rootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{replRootKey.ID()}, nil)
@@ -598,7 +627,7 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string
 
 	// This call to ValidateRoot will succeed since we are using a valid PEM
 	// encoded certificate, and have no other certificates for this CN
-	err = ValidateRoot(nil, signedTestRoot, gun, TrustPinConfig{})
+	err = ValidateRoot(prevRoot, signedTestRoot, gun, TrustPinConfig{})
 	require.NoError(t, err)
 }