cleaned up a lot of the control flow in key imports and added a bunch…

… of tests covering more cases, added a delegation add- import and publish flow test

Signed-off-by: Avi Vaid <avaid1996@gmail.com>

fixtures/precedence.example.com.key

@@ -1,5 +1,6 @@
 -----BEGIN RSA PRIVATE KEY-----
-role: root
+role: snapshot
+gun: anothergun
 
 MIIEowIBAAKCAQEAmLYiYCTAWJBWAuxZLqVmV4FiUdGgEqoQvCbN73zF/mQfhq0C
 ITo6xSxs1QiGDOzUtkpzXzziSj4J5+et4JkFleeEKaMcHadeIsSlHGvVtXDv93oR

tuf/utils/x509.go

@@ -443,6 +443,7 @@ func EncryptPrivateKey(key data.PrivateKey, role, gun, passphrase string) ([]byt
 		return nil, fmt.Errorf("unable to encrypt key - invalid PEM file produced")
 	}
 	encryptedPEMBlock.Headers["role"] = role
+
 	if gun != "" {
 		encryptedPEMBlock.Headers["gun"] = gun
 	}

tuf/utils/x509_test.go

@@ -139,16 +139,19 @@ func TestKeyOperations(t *testing.T) {
 	stringEncryptedEDKey := string(encryptedEDKey)
 	require.True(t, strings.Contains(stringEncryptedEDKey, "-----BEGIN ED25519 PRIVATE KEY-----"))
 	require.True(t, strings.Contains(stringEncryptedEDKey, "Proc-Type: 4,ENCRYPTED"))
+	require.True(t, strings.Contains(stringEncryptedEDKey, "role: root"))
 
 	// Check to see if EC key it is encrypted
 	stringEncryptedECKey := string(encryptedECKey)
 	require.True(t, strings.Contains(stringEncryptedECKey, "-----BEGIN EC PRIVATE KEY-----"))
 	require.True(t, strings.Contains(stringEncryptedECKey, "Proc-Type: 4,ENCRYPTED"))
+	require.True(t, strings.Contains(stringEncryptedECKey, "role: root"))
 
 	// Check to see if RSA key it is encrypted
 	stringEncryptedRSAKey := string(encryptedRSAKey)
 	require.True(t, strings.Contains(stringEncryptedRSAKey, "-----BEGIN RSA PRIVATE KEY-----"))
 	require.True(t, strings.Contains(stringEncryptedRSAKey, "Proc-Type: 4,ENCRYPTED"))
+	require.True(t, strings.Contains(stringEncryptedRSAKey, "role: root"))
 
 	// Decrypt our ED Key
 	decryptedEDKey, err := ParsePEMPrivateKey(encryptedEDKey, "ponies")
@@ -165,6 +168,19 @@ func TestKeyOperations(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, rsaKey.Private(), decryptedRSAKey.Private())
 
+	// quick test that gun headers are being added appropriately
+	// Encrypt our RSA Key, one type of key should be enough since headers are treated the same
+	testGunKey, err := EncryptPrivateKey(rsaKey, "root", "ilove", "ponies")
+	require.NoError(t, err)
+
+	testNoGunKey, err := EncryptPrivateKey(rsaKey, "root", "", "ponies")
+	require.NoError(t, err)
+
+	stringTestGunKey := string(testGunKey)
+	require.True(t, strings.Contains(stringTestGunKey, "gun: ilove"))
+
+	stringTestNoGunKey := string(testNoGunKey)
+	require.False(t, strings.Contains(stringTestNoGunKey, "gun:"))
 }
 
 // X509PublickeyID returns the public key ID of a RSA X509 key rather than the

utils/keys.go

@@ -105,10 +105,7 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 		toWrite []byte
 	)
 	for block, rest := pem.Decode(data); block != nil; block, rest = pem.Decode(rest) {
-		if block.Headers["role"] == "" {
-			// no worries about if check as for GUN here because empty roles will get a role:notary.DefaultImportRole
-			block.Headers["role"] = fallbackRole
-		}
+		// if there is a path then we set the gun header from this path
 		if rawPath := block.Headers["path"]; rawPath != "" {
 			pathWOFileName := strings.TrimSuffix(rawPath, filepath.Base(rawPath))
 			if strings.HasPrefix(pathWOFileName, notary.NonRootKeysSubdir) {
@@ -121,54 +118,59 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 				block.Headers["gun"] = fallbackGun
 			}
 		}
+		if block.Headers["role"] == "" {
+			if fallbackRole == "" {
+				block.Headers["role"] = notary.DefaultImportRole
+			} else {
+				block.Headers["role"] = fallbackRole
+			}
+		}
 		loc, ok := block.Headers["path"]
 		// only if the path isn't specified do we get into this parsing path logic
 		if !ok || loc == "" {
-			if block.Headers["role"] == "" {
-				// now we have no clue where to copy this key so we skip it since we have no path or role
-				logrus.Info("failed to import key to store: PEM headers did not contain import path")
-				continue
-			}
 			// if the path isn't specified, we will try to infer the path rel to trust dir from the role (and then gun)
-
+			// parse key for the keyID which we will save it by.
+			// if the key is encrypted at this point, we will generate an error and continue since we don't know the ID to save it by
 			decodedKey, err := utils.ParsePEMPrivateKey(pem.EncodeToMemory(block), "")
 			if err != nil {
 				logrus.Info("failed to import key to store: Invalid key generated, key may be encrypted and does not contain path header")
 				continue
 			}
 			keyID := decodedKey.ID()
-
 			if block.Headers["role"] == tufdata.CanonicalRootRole {
-				// does not make sense for root keys to have GUNs, so import it without the GUN even if specified
+				// this is a root key so import it to trustDir/root_keys/
 				loc = filepath.Join(notary.RootKeysSubdir, keyID)
+			} else if block.Headers["role"] == tufdata.CanonicalSnapshotRole || block.Headers["role"] == tufdata.CanonicalTargetsRole || block.Headers["role"] == tufdata.CanonicalTimestampRole {
+				loc = filepath.Join(notary.NonRootKeysSubdir, block.Headers["gun"], keyID)
 			} else {
-				// additional path inference from gun
+				loc = filepath.Join(notary.NonRootKeysSubdir, keyID)
+			}
+			switch block.Headers["role"] {
+			case tufdata.CanonicalRootRole:
+				// this is a root key so import it to trustDir/root_keys/
+				loc = filepath.Join(notary.RootKeysSubdir, keyID)
+			case tufdata.CanonicalSnapshotRole, tufdata.CanonicalTargetsRole, tufdata.CanonicalTimestampRole:
+				// this is a canonical key
 				loc = filepath.Join(notary.NonRootKeysSubdir, block.Headers["gun"], keyID)
+			default:
+				//this is a delegation key
+				loc = filepath.Join(notary.NonRootKeysSubdir, keyID)
 			}
 		}
-		// this is the path + no-role case where we assume the key is a delegation key
-		if block.Headers["role"] == "" {
-			block.Headers["role"] = notary.DefaultImportRole
-		}
 
-		if loc != writeTo {
-			// next location is different from previous one. We've finished aggregating
-			// data for the previous file. If we have data, write the previous file,
-			// the clear toWrite and set writeTo to the next path we're going to write
-			if toWrite != nil {
-				if err = importToStores(to, writeTo, toWrite); err != nil {
-					return err
-				}
-			}
-			// set up for aggregating next file's data
-			toWrite = nil
-			writeTo = loc
+		// A root key or a delegations key should not have a gun
+		// Note that a key that is not any of the canonical roles (except root) is a delegations key and should not have a gun
+		if block.Headers["role"] != tufdata.CanonicalSnapshotRole && block.Headers["role"] != tufdata.CanonicalTargetsRole && block.Headers["role"] != tufdata.CanonicalTimestampRole {
+			delete(block.Headers, "gun")
 		}
+
+		// the path header is not of any use once we've imported the key so strip it away
 		delete(block.Headers, "path")
 
-		// check if a key is not encrypted- if it isn't then ask for a passphrase and encrypt it
-		toSave := pem.EncodeToMemory(block)
-		if privKey, err := utils.ParsePEMPrivateKey(toSave, ""); err == nil {
+		// we are now all set for import but let's first encrypt the key
+		blockBytes := pem.EncodeToMemory(block)
+		// check if key is encrypted, note: if it is encrypted at this point, it will have had a path header
+		if privKey, err := utils.ParsePEMPrivateKey(blockBytes, ""); err == nil {
 			// Key is not encrypted- ask for a passphrase and encrypt this key
 			var chosenPassphrase string
 			for attempts := 0; ; attempts++ {
@@ -182,10 +184,24 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 				}
 				break
 			}
-			toSave, _ = utils.EncryptPrivateKey(privKey, block.Headers["role"], block.Headers["gun"], chosenPassphrase)
+			blockBytes, _ = utils.EncryptPrivateKey(privKey, block.Headers["role"], block.Headers["gun"], chosenPassphrase)
+		}
+
+		if loc != writeTo {
+			// next location is different from previous one. We've finished aggregating
+			// data for the previous file. If we have data, write the previous file,
+			// the clear toWrite and set writeTo to the next path we're going to write
+			if toWrite != nil {
+				if err = importToStores(to, writeTo, toWrite); err != nil {
+					return err
+				}
+			}
+			// set up for aggregating next file's data
+			toWrite = nil
+			writeTo = loc
 		}
 
-		toWrite = append(toWrite, toSave...)
+		toWrite = append(toWrite, blockBytes...)
 	}
 	if toWrite != nil { // close out final iteration if there's data left
 		return importToStores(to, writeTo, toWrite)

utils/keys_test.go

@@ -6,6 +6,7 @@ import (
 	"encoding/pem"
 	"errors"
 	"github.com/docker/notary"
+	"github.com/docker/notary/tuf/data"
 	"github.com/stretchr/testify/require"
 	"io/ioutil"
 	"os"
@@ -238,6 +239,8 @@ func TestImportKeys(t *testing.T) {
 	c.Bytes, _ = ioutil.ReadAll(from)
 	rand.Read(c.Bytes)
 	c.Headers["path"] = "morpork"
+	c.Headers["role"] = data.CanonicalSnapshotRole
+	c.Headers["gun"] = "somegun"
 
 	bBytes := pem.EncodeToMemory(b)
 	cBytes := pem.EncodeToMemory(c)
@@ -260,6 +263,8 @@ func TestImportKeys(t *testing.T) {
 	require.Equal(t, c.Bytes, cFinal.Bytes)
 	_, ok = cFinal.Headers["path"]
 	require.False(t, ok, "expected no path header, should have been removed at import")
+	require.Equal(t, data.CanonicalSnapshotRole, cFinal.Headers["role"])
+	require.Equal(t, "somegun", cFinal.Headers["gun"])
 	require.Len(t, cRest, 0)
 }
 
@@ -301,11 +306,11 @@ func TestNonRootPathInference(t *testing.T) {
 
 	in := bytes.NewBuffer(b.Bytes)
 
-	err := ImportKeys(in, []Importer{s}, "somerole", "somegun", passphraseRetriever)
+	err := ImportKeys(in, []Importer{s}, data.CanonicalSnapshotRole, "somegun", passphraseRetriever)
 	require.NoError(t, err)
 
 	for key := range s.data {
-		// no path but role included should work
+		// no path but role included should work and 12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497 is the key ID of the fixture
 		require.Equal(t, key, filepath.Join(notary.NonRootKeysSubdir, "somegun", "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497"))
 	}
 }
@@ -326,8 +331,12 @@ func TestBlockHeaderPrecedence(t *testing.T) {
 	require.NoError(t, err)
 
 	for key := range s.data {
-		// block header role should take precedence over command line flag
-		require.Equal(t, key, filepath.Join(notary.RootKeysSubdir, "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497"))
+		// block header role= root should take precedence over command line flag
+		require.Equal(t, key, filepath.Join(notary.NonRootKeysSubdir, "anothergun", "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497"))
+		final, rest := pem.Decode(s.data[key])
+		require.Len(t, rest, 0)
+		require.Equal(t, final.Headers["role"], "snapshot")
+		require.Equal(t, final.Headers["gun"], "anothergun")
 	}
 }