Address review comments and improve docstrings

Signed-off-by: Ying Li <ying.li@docker.com>

client/client.go

@@ -160,6 +160,24 @@ 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) {
+	// 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
+	}
+
+	x509PublicKey := trustmanager.CertToKey(cert)
+	if x509PublicKey == nil {
+		return nil, nil, fmt.Errorf(
+			"cannot use regenerated certificate: format %s", cert.PublicKeyAlgorithm)
+	}
+
+	return cert, x509PublicKey, nil
+}
+
 // Initialize creates a new repository by using rootKey as the root Key for the
 // TUF repository. The server must be reachable (and is asked to generate a
 // timestamp key and possibly other serverManagedRoles), but the created repository
@@ -198,31 +216,12 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		}
 	}
 
-	// Hard-coded policy: the generated certificate expires in 10 years.
-	startTime := time.Now()
-	rootCert, err := cryptoservice.GenerateCertificate(
-		privKey, r.gun, startTime, startTime.AddDate(10, 0, 0))
-
+	rootCert, rootKey, err := rootCertKey(r.gun, privKey)
 	if err != nil {
 		return err
 	}
 	r.CertStore.AddCert(rootCert)
 
-	// The root key gets stored in the TUF metadata X509 encoded, linking
-	// the tuf root.json to our X509 PKI.
-	// If the key is RSA, we store it as type RSAx509, if it is ECDSA we store it
-	// as ECDSAx509 to allow the gotuf verifiers to correctly decode the
-	// key on verification of signatures.
-	var rootKey data.PublicKey
-	switch privKey.Algorithm() {
-	case data.RSAKey:
-		rootKey = data.NewRSAx509PublicKey(trustmanager.CertToPEM(rootCert))
-	case data.ECDSAKey:
-		rootKey = data.NewECDSAx509PublicKey(trustmanager.CertToPEM(rootCert))
-	default:
-		return fmt.Errorf("invalid format for root key: %s", privKey.Algorithm())
-	}
-
 	var (
 		rootRole = data.NewBaseRole(
 			data.CanonicalRootRole,
@@ -1010,13 +1009,13 @@ func (r *NotaryRepository) ListRootCerts() ([]*x509.Certificate, error) {
 
 // RotateRootCert generates a new certificate to replace oldCert and adds
 // a changelist entry to update the root role with this certificate replacement
-// shen r.Publish() is called.
+// when r.Publish() is called.
 func (r *NotaryRepository) RotateRootCert(oldCert *x509.Certificate) error {
 	oldCertX509PublicKey := trustmanager.CertToKey(oldCert)
 	if oldCertX509PublicKey == nil {
 		return fmt.Errorf("invalid format for root key: %s", oldCert.PublicKeyAlgorithm)
 	}
-	rootKeyID, err := trustmanager.X509PublicKeyID(oldCertX509PublicKey)
+	rootKeyID, err := utils.CanonicalKeyID(oldCertX509PublicKey)
 	if err != nil {
 		return err
 	}
@@ -1026,16 +1025,10 @@ func (r *NotaryRepository) RotateRootCert(oldCert *x509.Certificate) error {
 		return err
 	}
 
-	// Hard-coded policy: the generated certificate expires in 10 years.
-	startTime := time.Now()
-	newCert, err := cryptoservice.GenerateCertificate(privKey, r.gun, startTime, startTime.AddDate(10, 0, 0))
+	_, newCertX509PublicKey, err := rootCertKey(r.gun, privKey)
 	if err != nil {
 		return err
 	}
-	newCertX509PublicKey := trustmanager.CertToKey(newCert)
-	if newCertX509PublicKey == nil {
-		return fmt.Errorf("cannot use regenerated certificate?! format %s", newCert.PublicKeyAlgorithm)
-	}
 
 	cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist"))
 	if err != nil {

client/client_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"crypto/rand"
 	"crypto/sha256"
-	"crypto/x509"
 	"encoding/hex"
 	regJson "encoding/json"
 	"fmt"
@@ -3398,14 +3397,9 @@ func TestRotateRootCert(t *testing.T) {
 	// Rotate root certificate.
 	certs, err := authorRepo.ListRootCerts()
 	require.NoError(t, err)
-	var cert *x509.Certificate
 	require.Len(t, certs, 1)
-	// Just get the only certificate in there.
-	for _, c := range certs {
-		cert = c
-	}
 	logRepoTrustRoot(t, "original", authorRepo)
-	err = authorRepo.RotateRootCert(cert)
+	err = authorRepo.RotateRootCert(certs[0])
 	require.NoError(t, err)
 	logRepoTrustRoot(t, "post-rotate", authorRepo)
 
@@ -3457,7 +3451,9 @@ func TestRotateRootCert(t *testing.T) {
 	require.NoError(t, err)
 	logRepoTrustRoot(t, "author refresh 1", authorRepo)
 	require.Equal(t, newRootCertID, rootRoleCertID(t, authorRepo))
-	// verifyOnlyTrustedCertificate(t, authorRepo, newRootCertID)
+	// can't verify anything here, because the trusted certificates aren't
+	// rotated from the updated root
+
 	err = authorRepo.Update(false)
 	require.NoError(t, err)
 	logRepoTrustRoot(t, "author refresh 2", authorRepo)

tuf/signed/sign.go

@@ -20,18 +20,19 @@ import (
 	"github.com/docker/notary/tuf/utils"
 )
 
-// Sign takes a data.Signed and keys, calculates and adds at least
-// 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, validSigningKeys []data.PublicKey) error {
+// Sign takes a data.Signed and a cryptoservice containing private keys,
+// calculates and adds at least minSignature signatures using signingKeys the
+// data.Signed.  It will also clean up any signatures that are not in produced
+// by either a signingKey or an otherWhitelistedKey.
+// Note that in most cases, otherWhitelistedKeys should probably be null. They
+// are for keys you don't want to sign with, but you also don't want to remove
+// existing signatures by those keys.  For instance, if you want to call Sign
+// multiple times with different sets of signing keys without undoing removing
+// signatures produced by the previous call to Sign.
+func Sign(service CryptoService, s *data.Signed, signingKeys []data.PublicKey,
+	minSignatures int, otherWhitelistedKeys []data.PublicKey) error {
 
-	logrus.Debugf("sign called with %d/%d required keys", minSignatures, len(primaryKeys))
+	logrus.Debugf("sign called with %d/%d required keys", minSignatures, len(signingKeys))
 	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
 	signingKeyIDs := make(map[string]struct{})
 	tufIDs := make(map[string]data.PublicKey)
@@ -40,7 +41,7 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 
 	// Get all the private key objects related to the public keys
 	missingKeyIDs := []string{}
-	for _, key := range primaryKeys {
+	for _, key := range signingKeys {
 		canonicalID, err := utils.CanonicalKeyID(key)
 		tufIDs[key.ID()] = key
 		if err != nil {
@@ -57,8 +58,8 @@ func Sign(service CryptoService, s *data.Signed, primaryKeys []data.PublicKey,
 		privKeys[key.ID()] = k
 	}
 
-	// include the list of validSigningKeys
-	for _, key := range validSigningKeys {
+	// include the list of otherWhitelistedKeys
+	for _, key := range otherWhitelistedKeys {
 		if _, ok := tufIDs[key.ID()]; !ok {
 			tufIDs[key.ID()] = key
 		}

tuf/tuf.go

@@ -64,6 +64,11 @@ type Repo struct {
 	Timestamp     *data.SignedTimestamp
 	cryptoService signed.CryptoService
 
+	// Because Repo is a mutable structure, these keep track of what the root
+	// role was when a root is set on the repo (as opposed to what it might be
+	// after things like AddBaseKeys and RemoveBaseKeys have been called on it).
+	// If we know what the original was, we'll if and how to handle root
+	// rotations.
 	originalRootRole data.BaseRole
 	rootRoleDirty    bool
 }
@@ -552,6 +557,8 @@ func (tr *Repo) InitTimestamp() error {
 func (tr *Repo) SetRoot(s *data.SignedRoot) error {
 	tr.Root = s
 	var err error
+	// originalRootRole is the root role prior to any mutations that might
+	// occur on tr.Root.
 	tr.originalRootRole, err = tr.Root.BuildBaseRole(data.CanonicalRootRole)
 	return err
 }