Address review comments

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

client/client.go

@@ -881,12 +881,7 @@ func (r *NotaryRepository) validateRoot(rootJSON []byte, fromRemote bool) (*data
 			}
 		}
 	}
-	err = trustpinning.ValidateRoot(prevRoot, root, r.gun, r.trustPinning)
+	return trustpinning.ValidateRoot(prevRoot, root, r.gun, r.trustPinning)
-	if err != nil {
-		return nil, err
-	}
-
-	return data.RootFromSigned(root)
 }
 
 // RotateKey removes all existing keys associated with the role, and either

client/client_update_test.go

@@ -1374,7 +1374,7 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	keyIDs := make([]string, len(threeKeys))
 	for i := 0; i < len(threeKeys); i++ {
 		threeKeys[i], err = testutils.CreateKey(
-			serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
+			serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole, data.ECDSAKey)
 		require.NoError(t, err)
 		keyIDs[i] = threeKeys[i].ID()
 		signedRoot.Signed.Keys[keyIDs[i]] = threeKeys[i]
@@ -1392,7 +1392,7 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	// --- threshold back to 1
 
 	replacementKey, err := testutils.CreateKey(
-		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole)
+		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalRootRole, data.ECDSAKey)
 	require.NoError(t, err)
 	signedRoot.Signed.Version++
 	signedRoot.Signed.Keys[replacementKey.ID()] = replacementKey
@@ -1416,7 +1416,7 @@ func TestValidateRootRotationWithOldRole(t *testing.T) {
 	// --- latest root role)
 	signedRoot.Signed.Version++
 	snapKey, err := testutils.CreateKey(
-		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalSnapshotRole)
+		serverSwizzler.CryptoService, "docker.com/notary", data.CanonicalSnapshotRole, data.ECDSAKey)
 	require.NoError(t, err)
 	signedRoot.Signed.Keys[snapKey.ID()] = snapKey
 	signedRoot.Signed.Roles[data.CanonicalSnapshotRole].KeyIDs = []string{snapKey.ID()}

trustpinning/certs.go

@@ -53,8 +53,9 @@ func prettyFormatCertIDs(certs []*x509.Certificate) string {
 ValidateRoot receives a new root, validates its correctness and attempts to
 do root key rotation if needed.
 
-First we list the current trusted certificates we have for a particular GUN. If
+First we check if we have any trusted certificates for a particular GUN in
-that list is non-empty means that we've already seen this repository before, and
+a previous root, if we have one. If the previous root is not nil and we find
+certificates for this GUN, we've already seen this repository before, and
 have a list of trusted certificates for it. In this case, we use this list of
 certificates to attempt to validate this root file.
 
@@ -86,16 +87,16 @@ We shall call this: TOFUS.
 
 Validation failure at any step will result in an ErrValidationFailed error.
 */
-func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trustPinning TrustPinConfig) error {
+func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trustPinning TrustPinConfig) (*data.SignedRoot, error) {
 	logrus.Debugf("entered ValidateRoot with dns: %s", gun)
 	signedRoot, err := data.RootFromSigned(root)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	rootRole, err := signedRoot.BuildBaseRole(data.CanonicalRootRole)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// Retrieve all the leaf and intermediate certificates in root for which the CN matches the GUN
@@ -103,29 +104,38 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus
 	certsFromRoot, err := validRootLeafCerts(allLeafCerts, gun, true)
 	if err != nil {
 		logrus.Debugf("error retrieving valid leaf certificates for: %s, %v", gun, err)
-		return &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"}
+		return nil, &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"}
 	}
 
-	// Retrieve all the trusted certificates from our previous root
+	// If we have a previous root, let's try to use it to validate that this new root is valid.
-	// Note that we do not validate expiries here since our originally trusted root might have expired certs
+	if prevRoot != nil {
-	allTrustedLeafCerts, allTrustedIntCerts := parseAllCerts(prevRoot)
+		// Retrieve all the trusted certificates from our previous root
-	trustedLeafCerts, err := validRootLeafCerts(allTrustedLeafCerts, gun, false)
+		// Note that we do not validate expiries here since our originally trusted root might have expired certs
-	// If we have certificates that match this specific GUN, let's make sure to
+		allTrustedLeafCerts, allTrustedIntCerts := parseAllCerts(prevRoot)
-	// use them first to validate that this new root is valid.
+		trustedLeafCerts, err := validRootLeafCerts(allTrustedLeafCerts, gun, false)
-	if len(trustedLeafCerts) != 0 {
+
+		// Use the certificates we found in the previous root for the GUN to verify its signatures
+		// This could potentially be an empty set, in which case we will fail to verify
 		logrus.Debugf("found %d valid root leaf certificates for %s: %s", len(trustedLeafCerts), gun,
 			prettyFormatCertIDs(trustedLeafCerts))
+
+		// Extract the previous root's threshold for signature verification
+		prevRootRoleData, ok := prevRoot.Signed.Roles[data.CanonicalRootRole]
+		if !ok {
+			return nil, &ErrValidationFail{Reason: "could not retrieve previous root role data"}
+		}
+
 		err = signed.VerifySignatures(
-			root, data.BaseRole{Keys: trustmanager.CertsToKeys(trustedLeafCerts, allTrustedIntCerts), Threshold: 1})
+			root, data.BaseRole{Keys: trustmanager.CertsToKeys(trustedLeafCerts, allTrustedIntCerts), Threshold: prevRootRoleData.Threshold})
 		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"}
+			return nil, &ErrValidationFail{Reason: "failed to validate data with current trusted certificates"}
 		}
 	} else {
 		logrus.Debugf("found no currently valid root certificates for %s, using trust_pinning config to bootstrap trust", gun)
 		trustPinCheckFunc, err := NewTrustPinChecker(trustPinning, gun)
 		if err != nil {
-			return &ErrValidationFail{Reason: err.Error()}
+			return nil, &ErrValidationFail{Reason: err.Error()}
 		}
 
 		validPinnedCerts := []*x509.Certificate{}
@@ -140,7 +150,7 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus
 			validPinnedCerts = append(validPinnedCerts, cert)
 		}
 		if len(validPinnedCerts) == 0 {
-			return &ErrValidationFail{Reason: "unable to match any certificates to trust_pinning config"}
+			return nil, &ErrValidationFail{Reason: "unable to match any certificates to trust_pinning config"}
 		}
 		certsFromRoot = validPinnedCerts
 	}
@@ -152,11 +162,11 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus
 		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"}
+		return nil, &ErrValidationFail{Reason: "failed to validate integrity of roots"}
 	}
 
 	logrus.Debugf("Root validation succeeded for %s", gun)
-	return nil
+	return signedRoot, nil
 }
 
 // validRootLeafCerts returns a list of possibly (if checkExpiry is true) non-expired, non-sha1 certificates

trustpinning/certs_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
+	"github.com/docker/notary/tuf/testutils"
 	"github.com/stretchr/testify/require"
 )
 
@@ -61,12 +62,12 @@ func TestValidateRoot(t *testing.T) {
 
 	// 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, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
 	require.NoError(t, err)
 
 	// This call to ValidateRoot will fail since we are passing in a dnsName that
 	// doesn't match the CN of the certificate.
-	err = ValidateRoot(nil, &testSignedRoot, "diogomonica.com/notary", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "diogomonica.com/notary", TrustPinConfig{})
 	require.Error(t, err, "An error was expected")
 	require.Equal(t, err, &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"})
 
@@ -80,7 +81,7 @@ func TestValidateRoot(t *testing.T) {
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
 
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
 	require.Error(t, err, "illegal base64 data at input byte")
 
 	//
@@ -93,7 +94,7 @@ func TestValidateRoot(t *testing.T) {
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
 
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
 	require.Error(t, err, "An error was expected")
 	require.Equal(t, err, &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"})
 
@@ -108,7 +109,7 @@ func TestValidateRoot(t *testing.T) {
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
 
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{})
 	require.Error(t, err, "An error was expected")
 	require.Equal(t, err, &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"})
 
@@ -127,7 +128,7 @@ func TestValidateRoot(t *testing.T) {
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
 
-	err = ValidateRoot(nil, &testSignedRoot, "secure.example.com", TrustPinConfig{})
+	_, err = ValidateRoot(nil, &testSignedRoot, "secure.example.com", TrustPinConfig{})
 	require.Error(t, err, "An error was expected")
 	require.Equal(t, err, &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"})
 }
@@ -149,7 +150,7 @@ func TestValidateRootWithoutTOFUS(t *testing.T) {
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
 
 	// This call to ValidateRoot will fail since we are explicitly disabling TOFU and have no local certs
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{DisableTOFU: true})
 	require.Error(t, err)
 }
 
@@ -168,14 +169,18 @@ func TestValidateRootWithPinnedCert(t *testing.T) {
 
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
+	typedSignedRoot, err := data.RootFromSigned(&testSignedRoot)
+	require.NoError(t, err)
 
 	// This call to ValidateRoot should succeed with the correct Cert ID (same as root public key ID)
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {rootPubKeyID}}, DisableTOFU: true})
+	validatedSignedRoot, err := ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {rootPubKeyID}}, DisableTOFU: true})
 	require.NoError(t, err)
+	require.Equal(t, validatedSignedRoot, typedSignedRoot)
 
 	// This call to ValidateRoot should also succeed with the correct Cert ID (same as root public key ID), even though we passed an extra bad one
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {rootPubKeyID, "invalidID"}}, DisableTOFU: true})
+	validatedSignedRoot, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {rootPubKeyID, "invalidID"}}, DisableTOFU: true})
 	require.NoError(t, err)
+	require.Equal(t, validatedSignedRoot, typedSignedRoot)
 }
 
 func TestValidateRootWithPinnerCertAndIntermediates(t *testing.T) {
@@ -328,11 +333,14 @@ func TestValidateRootWithPinnerCertAndIntermediates(t *testing.T) {
 	err = signed.Sign(cs, signedRoot, []data.PublicKey{ecdsax509Key}, 1, nil)
 	require.NoError(t, err)
 
+	typedSignedRoot, err := data.RootFromSigned(signedRoot)
+	require.NoError(t, err)
+
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
 	require.NoError(t, err, "failed to create a temporary directory: %s", err)
 
-	err = ValidateRoot(
+	validatedRoot, err := ValidateRoot(
 		nil,
 		signedRoot,
 		"docker.io/notary/test",
@@ -344,6 +352,7 @@ func TestValidateRootWithPinnerCertAndIntermediates(t *testing.T) {
 		},
 	)
 	require.NoError(t, err, "failed to validate certID with intermediate")
+	require.Equal(t, typedSignedRoot, validatedRoot)
 }
 
 func TestValidateRootFailuresWithPinnedCert(t *testing.T) {
@@ -361,26 +370,29 @@ func TestValidateRootFailuresWithPinnedCert(t *testing.T) {
 
 	// Unmarshal our signedroot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
+	typedSignedRoot, err := data.RootFromSigned(&testSignedRoot)
+	require.NoError(t, err)
 
 	// This call to ValidateRoot should fail due to an incorrect cert ID
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {"ABSOLUTELY NOT A CERT ID"}}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {"ABSOLUTELY NOT A CERT ID"}}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// This call to ValidateRoot should fail due to an empty cert ID
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {""}}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {""}}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// This call to ValidateRoot should fail due to an invalid GUN (even though the cert ID is correct), and TOFUS is set to false
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"not_a_gun": {rootPubKeyID}}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"not_a_gun": {rootPubKeyID}}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// This call to ValidateRoot should fail due to an invalid cert ID, even though it's a valid key ID for targets
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {targetsPubKeyID}}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {targetsPubKeyID}}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// This call to ValidateRoot should succeed because we fall through to TOFUS because we have no matching GUNs under Certs
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"not_a_gun": {rootPubKeyID}}, DisableTOFU: false})
+	validatedRoot, err := ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"not_a_gun": {rootPubKeyID}}, DisableTOFU: false})
 	require.NoError(t, err)
+	require.Equal(t, typedSignedRoot, validatedRoot)
 }
 
 func TestValidateRootWithPinnedCA(t *testing.T) {
@@ -396,31 +408,34 @@ func TestValidateRootWithPinnedCA(t *testing.T) {
 	templ.Execute(&signedRootBytes, SignedRSARootTemplate{RootPem: validPEMEncodedRSARoot})
 	// Unmarshal our signedRoot
 	json.Unmarshal(signedRootBytes.Bytes(), &testSignedRoot)
+	typedSignedRoot, err := data.RootFromSigned(&testSignedRoot)
+	require.NoError(t, err)
 
 	// This call to ValidateRoot will fail because we have an invalid path for the CA
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"docker.com/notary": filepath.Join(tempBaseDir, "nonexistent")}})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"docker.com/notary": filepath.Join(tempBaseDir, "nonexistent")}})
 	require.Error(t, err)
 
 	// This call to ValidateRoot will fail because we have no valid GUNs to use, and TOFUS is disabled
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"othergun": filepath.Join(tempBaseDir, "nonexistent")}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"othergun": filepath.Join(tempBaseDir, "nonexistent")}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// This call to ValidateRoot will succeed because we have no valid GUNs to use and we fall back to enabled TOFUS
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"othergun": filepath.Join(tempBaseDir, "nonexistent")}, DisableTOFU: false})
+	validatedRoot, err := ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"othergun": filepath.Join(tempBaseDir, "nonexistent")}, DisableTOFU: false})
 	require.NoError(t, err)
+	require.Equal(t, typedSignedRoot, validatedRoot)
 
 	// Write an invalid CA cert (not even a PEM) to the tempDir and ensure validation fails when using it
 	invalidCAFilepath := filepath.Join(tempBaseDir, "invalid.ca")
 	require.NoError(t, ioutil.WriteFile(invalidCAFilepath, []byte("ABSOLUTELY NOT A PEM"), 0644))
 
 	// Using this invalid CA cert should fail on ValidateRoot
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"docker.com/notary": invalidCAFilepath}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{CA: map[string]string{"docker.com/notary": invalidCAFilepath}, DisableTOFU: true})
 	require.Error(t, err)
 
 	validCAFilepath := "../fixtures/root-ca.crt"
 
 	// If we pass an invalid Certs entry in addition to this valid CA entry, since Certs has priority for pinning we will fail
-	err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {"invalidID"}}, CA: map[string]string{"docker.com/notary": validCAFilepath}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, &testSignedRoot, "docker.com/notary", TrustPinConfig{Certs: map[string][]string{"docker.com/notary": {"invalidID"}}, CA: map[string]string{"docker.com/notary": validCAFilepath}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// Now construct a new root with a valid cert chain, such that signatures are correct over the 'notary-signer' GUN.  Pin the root-ca and validate
@@ -469,10 +484,14 @@ func TestValidateRootWithPinnedCA(t *testing.T) {
 	err = signed.Sign(cs, newTestSignedRoot, []data.PublicKey{newRootKey}, 1, nil)
 	require.NoError(t, err)
 
-	// Check that we validate correctly against a pinned CA and provided bundle
+	newTypedSignedRoot, err := data.RootFromSigned(newTestSignedRoot)
-	err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": validCAFilepath}, DisableTOFU: true})
 	require.NoError(t, err)
 
+	// Check that we validate correctly against a pinned CA and provided bundle
+	validatedRoot, err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": validCAFilepath}, DisableTOFU: true})
+	require.NoError(t, err)
+	require.Equal(t, newTypedSignedRoot, validatedRoot)
+
 	// Add an expired CA for the same gun to our previous pinned bundle, ensure that we still validate correctly
 	goodRootCABundle, err := trustmanager.LoadCertBundleFromFile(validCAFilepath)
 	require.NoError(t, err)
@@ -490,8 +509,9 @@ func TestValidateRootWithPinnedCA(t *testing.T) {
 	require.NoError(t, ioutil.WriteFile(bundleWithExpiredCertPath, bundleWithExpiredCert, 0644))
 
 	// Check that we validate correctly against a pinned CA and provided bundle
-	err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": bundleWithExpiredCertPath}, DisableTOFU: true})
+	validatedRoot, err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": bundleWithExpiredCertPath}, DisableTOFU: true})
 	require.NoError(t, err)
+	require.Equal(t, newTypedSignedRoot, validatedRoot)
 
 	testPubKey2, err := cryptoService.Create("root", "notary-signer", data.ECDSAKey)
 	require.NoError(t, err)
@@ -504,7 +524,7 @@ func TestValidateRootWithPinnedCA(t *testing.T) {
 	allExpiredCertPath := filepath.Join(tempBaseDir, "all_expired_cert.pem")
 	require.NoError(t, ioutil.WriteFile(allExpiredCertPath, allExpiredCertBundle, 0644))
 	// Now only use expired certs in the bundle, we should fail
-	err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": allExpiredCertPath}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": allExpiredCertPath}, DisableTOFU: true})
 	require.Error(t, err)
 
 	// Add a CA cert for a that won't validate against the root leaf certificate
@@ -518,7 +538,7 @@ func TestValidateRootWithPinnedCA(t *testing.T) {
 	require.NoError(t, err)
 	bundleWithWrongCertPath := filepath.Join(tempBaseDir, "bundle_with_expired_cert.pem")
 	require.NoError(t, ioutil.WriteFile(bundleWithWrongCertPath, bundleWithWrongCert, 0644))
-	err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": bundleWithWrongCertPath}, DisableTOFU: true})
+	_, err = ValidateRoot(nil, newTestSignedRoot, "notary-signer", TrustPinConfig{CA: map[string]string{"notary-signer": bundleWithWrongCertPath}, DisableTOFU: true})
 	require.Error(t, err)
 }
 
@@ -531,49 +551,16 @@ func TestValidateSuccessfulRootRotation(t *testing.T) {
 	}
 }
 
-// Generates certificates for two keys which have been added to the keystore.
-// Also returns the temporary directory so it can be cleaned up.
-func generateTwoCerts(t *testing.T, gun, keyAlg string) (
-	string, *cryptoservice.CryptoService, []*x509.Certificate) {
-	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
-	require.NoError(t, err, "failed to create a temporary directory: %s", err)
-
-	fileKeyStore, err := trustmanager.NewKeyFileStore(tempBaseDir, passphraseRetriever)
-	require.NoError(t, err)
-
-	cryptoService := cryptoservice.NewCryptoService(fileKeyStore)
-
-	certificates := make([]*x509.Certificate, 2)
-	for i := 0; i < 2; i++ {
-		pubKey, err := cryptoService.Create("root", gun, keyAlg)
-		require.NoError(t, err)
-
-		key, _, err := fileKeyStore.GetKey(pubKey.ID())
-		require.NoError(t, err)
-
-		cert, err := generateTestingCertificate(key, gun)
-		require.NoError(t, err)
-
-		certificates[i] = cert
-	}
-	return tempBaseDir, cryptoService, certificates
-}
-
 func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string) {
 	// The gun to test
 	gun := "docker.com/notary"
 
-	tempBaseDir, cs, certificates := generateTwoCerts(t, gun, keyAlg)
+	memKeyStore := trustmanager.NewKeyMemoryStore(passphraseRetriever)
-	defer os.RemoveAll(tempBaseDir)
+	cs := cryptoservice.NewCryptoService(memKeyStore)
-	origRootCert := certificates[0]
-	replRootCert := certificates[1]
-
-	// 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)
 
 	// Tuf key with PEM-encoded x509 certificate
-	origRootKey := data.NewPublicKey(rootKeyType, origRootPEMCert)
+	origRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	origRootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{origRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -593,17 +580,14 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string
 	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)
+	replRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	rootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{replRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -625,10 +609,14 @@ func testValidateSuccessfulRootRotation(t *testing.T, keyAlg, rootKeyType string
 	err = signed.Sign(cs, signedTestRoot, []data.PublicKey{replRootKey, origRootKey}, 2, nil)
 	require.NoError(t, err)
 
+	typedSignedRoot, err := data.RootFromSigned(signedTestRoot)
+	require.NoError(t, err)
+
 	// 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(prevRoot, signedTestRoot, gun, TrustPinConfig{})
+	validatedRoot, err := ValidateRoot(prevRoot, signedTestRoot, gun, TrustPinConfig{})
 	require.NoError(t, err)
+	require.Equal(t, typedSignedRoot, validatedRoot)
 }
 
 // TestValidateRootRotationMissingOrigSig runs through a full root certificate rotation
@@ -644,18 +632,12 @@ func TestValidateRootRotationMissingOrigSig(t *testing.T) {
 func testValidateRootRotationMissingOrigSig(t *testing.T, keyAlg, rootKeyType string) {
 	gun := "docker.com/notary"
 
-	tempBaseDir, cryptoService, certificates := generateTwoCerts(
+	memKeyStore := trustmanager.NewKeyMemoryStore(passphraseRetriever)
-		t, gun, keyAlg)
+	cs := cryptoservice.NewCryptoService(memKeyStore)
-	defer os.RemoveAll(tempBaseDir)
-	origRootCert := certificates[0]
-	replRootCert := certificates[1]
-
-	// 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)
 
 	// Tuf key with PEM-encoded x509 certificate
-	origRootKey := data.NewPublicKey(rootKeyType, origRootPEMCert)
+	origRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	origRootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{origRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -675,17 +657,14 @@ func testValidateRootRotationMissingOrigSig(t *testing.T, keyAlg, rootKeyType st
 	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)
-	err = signed.Sign(cryptoService, 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)
+	replRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	rootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{replRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -706,12 +685,12 @@ func testValidateRootRotationMissingOrigSig(t *testing.T, keyAlg, rootKeyType st
 	require.NoError(t, err)
 
 	// We only sign with the new key, and not with the original one.
-	err = signed.Sign(cryptoService, signedTestRoot, []data.PublicKey{replRootKey}, 1, nil)
+	err = signed.Sign(cs, signedTestRoot, []data.PublicKey{replRootKey}, 1, nil)
 	require.NoError(t, err)
 
 	// 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(prevRoot, signedTestRoot, gun, TrustPinConfig{})
+	_, err = ValidateRoot(prevRoot, signedTestRoot, gun, TrustPinConfig{})
 	require.Error(t, err, "insuficient signatures on root")
 }
 
@@ -728,21 +707,12 @@ func TestValidateRootRotationMissingNewSig(t *testing.T) {
 func testValidateRootRotationMissingNewSig(t *testing.T, keyAlg, rootKeyType string) {
 	gun := "docker.com/notary"
 
-	tempBaseDir, cryptoService, certificates := generateTwoCerts(
+	memKeyStore := trustmanager.NewKeyMemoryStore(passphraseRetriever)
-		t, gun, keyAlg)
+	cs := cryptoservice.NewCryptoService(memKeyStore)
-	defer os.RemoveAll(tempBaseDir)
-	origRootCert := certificates[0]
-	replRootCert := certificates[1]
-
-	// We need the PEM representation of the replacement key to put it into the TUF data
-	replRootPEMCert := trustmanager.CertToPEM(replRootCert)
-
-	// 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)
 
 	// Tuf key with PEM-encoded x509 certificate
-	origRootKey := data.NewPublicKey(rootKeyType, origRootPEMCert)
+	origRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	origRootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{origRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -762,14 +732,14 @@ func testValidateRootRotationMissingNewSig(t *testing.T, keyAlg, rootKeyType str
 	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)
-	err = signed.Sign(cryptoService, signedOrigTestRoot, []data.PublicKey{origRootKey}, 1, nil)
 	require.NoError(t, err)
 	prevRoot, err := data.RootFromSigned(signedOrigTestRoot)
 	require.NoError(t, err)
 
 	// Tuf key with PEM-encoded x509 certificate
-	replRootKey := data.NewPublicKey(rootKeyType, replRootPEMCert)
+	replRootKey, err := testutils.CreateKey(cs, gun, data.CanonicalRootRole, keyAlg)
+	require.NoError(t, err)
 
 	rootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{replRootKey.ID()}, nil)
 	require.NoError(t, err)
@@ -790,12 +760,12 @@ func testValidateRootRotationMissingNewSig(t *testing.T, keyAlg, rootKeyType str
 	require.NoError(t, err)
 
 	// We only sign with the old key, and not with the new one
-	err = signed.Sign(cryptoService, signedTestRoot, []data.PublicKey{origRootKey}, 1, nil)
+	err = signed.Sign(cs, signedTestRoot, []data.PublicKey{origRootKey}, 1, nil)
 	require.NoError(t, err)
 
 	// 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(prevRoot, signedTestRoot, gun, TrustPinConfig{})
+	_, err = ValidateRoot(prevRoot, signedTestRoot, gun, TrustPinConfig{})
 	require.Error(t, err, "insuficient signatures on root")
 }
 

tuf/testutils/repo.go

@@ -19,8 +19,8 @@ import (
 
 // CreateKey creates a new key inside the cryptoservice for the given role and gun,
 // returning the public key.  If the role is a root role, create an x509 key.
-func CreateKey(cs signed.CryptoService, gun, role string) (data.PublicKey, error) {
+func CreateKey(cs signed.CryptoService, gun, role, keyAlgorithm string) (data.PublicKey, error) {
-	key, err := cs.Create(role, gun, data.ECDSAKey)
+	key, err := cs.Create(role, gun, keyAlgorithm)
 	if err != nil {
 		return nil, err
 	}
@@ -36,7 +36,13 @@ func CreateKey(cs signed.CryptoService, gun, role string) (data.PublicKey, error
 		if err != nil {
 			return nil, err
 		}
-		key = data.NewECDSAx509PublicKey(trustmanager.CertToPEM(cert))
+		// Keep the x509 key type consistent with the key's algorithm
+		if keyAlgorithm == data.RSAKey {
+			key = data.NewRSAx509PublicKey(trustmanager.CertToPEM(cert))
+		} else {
+			key = data.NewECDSAx509PublicKey(trustmanager.CertToPEM(cert))
+		}
+
 	}
 	return key, nil
 }
@@ -50,7 +56,7 @@ func EmptyRepo(gun string, delegationRoles ...string) (*tuf.Repo, signed.CryptoS
 
 	baseRoles := map[string]data.BaseRole{}
 	for _, role := range data.BaseRoles {
-		key, err := CreateKey(cs, gun, role)
+		key, err := CreateKey(cs, gun, role, data.ECDSAKey)
 		if err != nil {
 			return nil, nil, err
 		}
@@ -77,7 +83,7 @@ func EmptyRepo(gun string, delegationRoles ...string) (*tuf.Repo, signed.CryptoS
 	sort.Strings(delegationRoles)
 	for _, delgName := range delegationRoles {
 		// create a delegations key and a delegation in the tuf repo
-		delgKey, err := CreateKey(cs, gun, delgName)
+		delgKey, err := CreateKey(cs, gun, delgName, data.ECDSAKey)
 		if err != nil {
 			return nil, nil, err
 		}

tuf/testutils/swizzler.go

@@ -269,7 +269,7 @@ func (m *MetadataSwizzler) SignMetadataWithInvalidKey(role string) error {
 
 	// create an invalid key, but not in the existing CryptoService
 	cs := cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(passphrase.ConstantRetriever("")))
-	key, err := CreateKey(cs, m.Gun, role)
+	key, err := CreateKey(cs, m.Gun, role, data.ECDSAKey)
 	if err != nil {
 		return err
 	}
@@ -466,7 +466,7 @@ func (m *MetadataSwizzler) RotateKey(role string, key data.PublicKey) error {
 // ChangeRootKey swaps out the root key with a new key, and re-signs the metadata
 // with the new key
 func (m *MetadataSwizzler) ChangeRootKey() error {
-	key, err := CreateKey(m.CryptoService, m.Gun, data.CanonicalRootRole)
+	key, err := CreateKey(m.CryptoService, m.Gun, data.CanonicalRootRole, data.ECDSAKey)
 	if err != nil {
 		return err
 	}