added skip import if a key is imported to a non root canonical role w…

…ithout a gun

Signed-off-by: Avi Vaid <[email protected]>

cmd/notary/integration_test.go

@@ -2098,7 +2098,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 0, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 0, !rootOnHardware())
 
 	// test 3, no path no role included with unencrypted key
 
@@ -2115,7 +2115,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	nBytes, err = tempFile3.Write(pemBytes)
 	require.NoError(t, err)
-	tempFile2.Close()
+	tempFile3.Close()
 	require.Equal(t, len(pemBytes), nBytes)
 
 	// import the key
@@ -2124,7 +2124,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 1, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 1, !rootOnHardware())
 	file, err := os.OpenFile(filepath.Join(tempDir, "private", notary.NonRootKeysSubdir, privKey.ID()+".key"), os.O_RDONLY, notary.PrivKeyPerms)
 	require.NoError(t, err)
 	filebytes, _ := ioutil.ReadAll(file)
@@ -2155,7 +2155,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 2, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 2, !rootOnHardware())
 	_, err = os.Open(filepath.Join(tempDir, "private", notary.NonRootKeysSubdir, newKeyID+".key"))
 	require.NoError(t, err)
 
@@ -2184,7 +2184,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 3, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 3, !rootOnHardware())
 	_, err = os.Open(filepath.Join(tempDir, "private", notary.NonRootKeysSubdir, "somegun", newKeyID+".key"))
 	require.NoError(t, err)
 
@@ -2212,7 +2212,7 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 3, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 3, !rootOnHardware())
 
 	// test7, non root key with no path with no gun
 
@@ -2239,9 +2239,38 @@ func TestClientKeyImport(t *testing.T) {
 
 	// if there is hardware available, root will only be on hardware, and not
 	// on disk
-	_, _ = assertNumKeys(t, tempDir, 2, 4, !rootOnHardware())
+	assertNumKeys(t, tempDir, 2, 4, !rootOnHardware())
 	_, err = os.Open(filepath.Join(tempDir, "private", notary.NonRootKeysSubdir, newKeyID+".key"))
 	require.NoError(t, err)
+
+	// test 8, non root canonical key with no gun
+
+	tempFile8, err := ioutil.TempFile("", "pemfile")
+	require.NoError(t, err)
+	// close later, because we might need to write to it
+	defer os.Remove(tempFile8.Name())
+
+	privKey, err = utils.GenerateECDSAKey(rand.Reader)
+	require.NoError(t, err)
+
+	pemBytes, err = utils.EncryptPrivateKey(privKey, data.CanonicalSnapshotRole, "", "")
+	require.NoError(t, err)
+
+	nBytes, err = tempFile8.Write(pemBytes)
+	require.NoError(t, err)
+	tempFile8.Close()
+	require.Equal(t, len(pemBytes), nBytes)
+	newKeyID = privKey.ID()
+
+	// import the key
+	_, err = runCommand(t, tempDir, "key", "import", tempFile8.Name())
+	require.NoError(t, err)
+
+	// if there is hardware available, root will only be on hardware, and not
+	// on disk
+	assertNumKeys(t, tempDir, 2, 4, !rootOnHardware())
+	_, err = os.Open(filepath.Join(tempDir, "private", notary.NonRootKeysSubdir, newKeyID+".key"))
+	require.Error(t, err)
 }
 
 func TestAddDelImportKeyPublishFlow(t *testing.T) {
@@ -2487,7 +2516,6 @@ func TestExportImportFlow(t *testing.T) {
 	snapString := string(snapBytes)
 	require.Contains(t, snapString, "gun: gun")
 	require.True(t, strings.Contains(snapString, "role: snapshot") || strings.Contains(snapString, "role: target"))
-	require.Contains(t, snapString, "role: snapshot")
 
 	// validate targets is imported correctly
 	targKey, err := os.OpenFile(filepath.Join(newTempDir, "private", notary.NonRootKeysSubdir, "gun", signing[1]+".key"), os.O_RDONLY, notary.PrivKeyPerms)

cmd/notary/keys.go

@@ -415,8 +415,7 @@ func (k *keyCommander) importKeys(cmd *cobra.Command, args []string) error {
 	for _, file := range args {
 		from, err := os.OpenFile(file, os.O_RDONLY, notary.PrivKeyPerms)
 		defer from.Close()
-		passRetriever := k.getRetriever()
-		if err = utils.ImportKeys(from, importers, k.keysImportRole, k.keysImportGUN, passRetriever); err != nil {
+		if err = utils.ImportKeys(from, importers, k.keysImportRole, k.keysImportGUN, k.getRetriever()); err != nil {
 			return err
 		}
 	}

utils/keys.go

@@ -2,6 +2,7 @@ package utils
 
 import (
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary"
@@ -140,14 +141,6 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 				continue
 			}
 			keyID := decodedKey.ID()
-			if block.Headers["role"] == tufdata.CanonicalRootRole {
-				// 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 {
-				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/
@@ -165,6 +158,12 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 		// 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")
+		} else {
+			// check if the key is missing a gun header or has an empty gun and error out since we don't know where to import this key to
+			if block.Headers["gun"] == "" {
+				logrus.Info("failed to import key to store: Cannot have canonical role key without a gun, don't know where to import it")
+				continue
+			}
 		}
 
 		// the path header is not of any use once we've imported the key so strip it away
@@ -179,15 +178,17 @@ func ImportKeys(from io.Reader, to []Importer, fallbackRole string, fallbackGun
 			for attempts := 0; ; attempts++ {
 				var giveup bool
 				chosenPassphrase, giveup, err = passRet(loc, block.Headers["role"], true, attempts)
-				if err != nil {
-					continue
+				if err == nil {
+					break
 				}
 				if giveup || attempts > 10 {
-					fmt.Println("exceeded attempts")
+					return errors.New("maximum number of passphrase attempts exceeded")
 				}
-				break
 			}
-			blockBytes, _ = utils.EncryptPrivateKey(privKey, block.Headers["role"], block.Headers["gun"], chosenPassphrase)
+			blockBytes, err = utils.EncryptPrivateKey(privKey, block.Headers["role"], block.Headers["gun"], chosenPassphrase)
+			if err != nil {
+				return errors.New("failed to encrypt key with given passphrase")
+			}
 		}
 
 		if loc != writeTo {

utils/keys_test.go

@@ -431,10 +431,12 @@ func TestImportKeys2InOneFileNoPath(t *testing.T) {
 	defer from.Close()
 	fromBytes, _ := ioutil.ReadAll(from)
 	b, _ := pem.Decode(fromBytes)
+	b.Headers["gun"] = "testgun"
 	b.Headers["role"] = data.CanonicalSnapshotRole
 	bBytes := pem.EncodeToMemory(b)
 
 	b2, _ := pem.Decode(fromBytes)
+	b2.Headers["gun"] = "testgun"
 	b2.Headers["role"] = data.CanonicalSnapshotRole
 	b2Bytes := pem.EncodeToMemory(b2)
 
@@ -455,10 +457,12 @@ func TestImportKeys2InOneFileNoPath(t *testing.T) {
 	err := ImportKeys(in, []Importer{s}, "", "", passphraseRetriever)
 	require.NoError(t, err)
 
-	bFinal, bRest := pem.Decode(s.data[filepath.Join(notary.NonRootKeysSubdir, "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497")])
+	bFinal, bRest := pem.Decode(s.data[filepath.Join(notary.NonRootKeysSubdir, "testgun", "12ba0e0a8e05e177bc2c3489bdb6d28836879469f078e68a4812fc8a2d521497")])
+	require.Equal(t, b.Headers["gun"], bFinal.Headers["gun"])
 	require.Equal(t, b.Headers["role"], bFinal.Headers["role"])
 
 	b2Final, b2Rest := pem.Decode(bRest)
+	require.Equal(t, b2.Headers["gun"], b2Final.Headers["gun"])
 	require.Equal(t, b2.Headers["role"], b2Final.Headers["role"])
 	require.Len(t, b2Rest, 0)