review feedback: updating for windows, error paths

Signed-off-by: Riyaz Faizullabhoy <[email protected]>
Upstream-commit: e189a21a2511d25590f650eba5fef25e02fa2405
Component: cli

components/cli/cli/command/trust/helpers.go

@@ -11,12 +11,13 @@ import (
 const releasedRoleName = "Repo Admin"
 const releasesRoleTUFName = "targets/releases"
 
-// check if a role name is "released": either targets/releases or targets TUF roles
+// isReleasedTarget checks if a role name is "released":
+// either targets/releases or targets TUF roles
 func isReleasedTarget(role data.RoleName) bool {
 	return role == data.CanonicalTargetsRole || role == trust.ReleasesRole
 }
 
-// convert TUF role name to a human-understandable signer name
+// notaryRoleToSigner converts TUF role name to a human-understandable signer name
 func notaryRoleToSigner(tufRole data.RoleName) string {
 	//  don't show a signer for "targets" or "targets/releases"
 	if isReleasedTarget(data.RoleName(tufRole.String())) {
@@ -25,6 +26,7 @@ func notaryRoleToSigner(tufRole data.RoleName) string {
 	return strings.TrimPrefix(tufRole.String(), "targets/")
 }
 
+// clearChangelist clears the notary staging changelist.
 func clearChangeList(notaryRepo client.Repository) error {
 	cl, err := notaryRepo.GetChangelist()
 	if err != nil {
@@ -33,12 +35,13 @@ func clearChangeList(notaryRepo client.Repository) error {
 	return cl.Clear("")
 }
 
+// getOrGenerateRootKeyAndInitRepo initializes the notary repository
+// with a remotely managed snapshot key. The initialization will use
+// an existing root key if one is found, else a new one will be generated.
 func getOrGenerateRootKeyAndInitRepo(notaryRepo client.Repository) error {
 	rootKey, err := getOrGenerateNotaryKey(notaryRepo, data.CanonicalRootRole)
 	if err != nil {
 		return err
 	}
-	// Initialize the notary repository with a remotely managed snapshot
-	// key
 	return notaryRepo.Initialize([]string{rootKey.ID()}, data.CanonicalSnapshotRole)
 }

components/cli/cli/command/trust/key_generate.go

@@ -28,7 +28,7 @@ type keyGenerateOptions struct {
 func newKeyGenerateCommand(dockerCli command.Streams) *cobra.Command {
 	options := keyGenerateOptions{}
 	cmd := &cobra.Command{
-		Use:   "generate NAME [NAME...]",
+		Use:   "generate NAME",
 		Short: "Generate and load a signing key-pair",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -37,30 +37,33 @@ func newKeyGenerateCommand(dockerCli command.Streams) *cobra.Command {
 		},
 	}
 	flags := cmd.Flags()
-	flags.StringVarP(&options.directory, "dir", "d", "", "Directory to generate key in, defaults to current directory")
+	flags.StringVar(&options.directory, "dir", "", "Directory to generate key in, defaults to current directory")
 	return cmd
 }
 
-// key names can use alphanumeric + _ + - characters
-var validKeyName = regexp.MustCompile(`^[a-zA-Z0-9\_]+[a-zA-Z0-9\_\-]*$`).MatchString
+// key names can use lowercase alphanumeric + _ + - characters
+var validKeyName = regexp.MustCompile(`^[a-z0-9][a-z0-9\_\-]*$`).MatchString
 
 // validate that all of the key names are unique and are alphanumeric + _ + -
-// and that we do not already have public key files in the current dir on disk
-func validateKeyArgs(keyName string, cwdPath string) error {
+// and that we do not already have public key files in the target dir on disk
+func validateKeyArgs(keyName string, targetDir string) error {
 	if !validKeyName(keyName) {
-		return fmt.Errorf("key name \"%s\" must not contain special characters", keyName)
+		return fmt.Errorf("key name \"%s\" must start with lowercase alphanumeric characters and can include \"-\" or \"_\" after the first character", keyName)
 	}
 
 	pubKeyFileName := keyName + ".pub"
-	if _, err := os.Stat(filepath.Join(cwdPath, pubKeyFileName)); err == nil {
-		return fmt.Errorf("public key file already exists: \"%s\"", pubKeyFileName)
+	if _, err := os.Stat(targetDir); err != nil {
+		return fmt.Errorf("public key path does not exist: \"%s\"", targetDir)
+	}
+	targetPath := filepath.Join(targetDir, pubKeyFileName)
+	if _, err := os.Stat(targetPath); err == nil {
+		return fmt.Errorf("public key file already exists: \"%s\"", targetPath)
 	}
 	return nil
 }
 
 func setupPassphraseAndGenerateKeys(streams command.Streams, opts keyGenerateOptions) error {
 	targetDir := opts.directory
-	// if the target dir is empty, default to CWD
 	if targetDir == "" {
 		cwd, err := os.Getwd()
 		if err != nil {
@@ -76,7 +79,7 @@ func validateAndGenerateKey(streams command.Streams, keyName string, workingDir
 	if err := validateKeyArgs(keyName, workingDir); err != nil {
 		return err
 	}
-	fmt.Fprintf(streams.Out(), "\nGenerating key for %s...\n", keyName)
+	fmt.Fprintf(streams.Out(), "Generating key for %s...\n", keyName)
 	// Automatically load the private key to local storage for use
 	privKeyFileStore, err := trustmanager.NewKeyFileStore(trust.GetTrustDirectory(), freshPassRetGetter())
 	if err != nil {

components/cli/cli/command/trust/key_generate_test.go

@@ -2,6 +2,7 @@ package trust
 
 import (
 	"encoding/pem"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -120,14 +121,18 @@ func TestValidateKeyArgs(t *testing.T) {
 
 	err = validateKeyArgs("a/b", pubKeyCWD)
 	assert.Error(t, err)
-	assert.Equal(t, err.Error(), "key name \"a/b\" must not contain special characters")
+	assert.Equal(t, err.Error(), "key name \"a/b\" must start with lowercase alphanumeric characters and can include \"-\" or \"_\" after the first character")
 
 	err = validateKeyArgs("-", pubKeyCWD)
 	assert.Error(t, err)
-	assert.Equal(t, err.Error(), "key name \"-\" must not contain special characters")
+	assert.Equal(t, err.Error(), "key name \"-\" must start with lowercase alphanumeric characters and can include \"-\" or \"_\" after the first character")
 
 	assert.NoError(t, ioutil.WriteFile(filepath.Join(pubKeyCWD, "a.pub"), []byte("abc"), notary.PrivExecPerms))
 	err = validateKeyArgs("a", pubKeyCWD)
 	assert.Error(t, err)
-	assert.Equal(t, err.Error(), "public key file already exists: \"a.pub\"")
+	assert.Equal(t, err.Error(), fmt.Sprintf("public key file already exists: \"%s/a.pub\"", pubKeyCWD))
+
+	err = validateKeyArgs("a", "/random/dir/")
+	assert.Error(t, err)
+	assert.Equal(t, err.Error(), "public key path does not exist: \"/random/dir/\"")
 }

components/cli/cli/command/trust/key_load.go

@@ -2,6 +2,7 @@ package trust
 
 import (
 	"bytes"
+	"encoding/pem"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -18,8 +19,7 @@ import (
 )
 
 const (
-	ownerReadOnlyPerms     = 0400
-	ownerReadAndWritePerms = 0600
+	nonOwnerReadWriteMask = 0077
 )
 
 type keyLoadOptions struct {
@@ -29,33 +29,37 @@ type keyLoadOptions struct {
 func newKeyLoadCommand(dockerCli command.Streams) *cobra.Command {
 	var options keyLoadOptions
 	cmd := &cobra.Command{
-		Use:   "load [OPTIONS] KEY",
+		Use:   "load [OPTIONS] KEYFILE",
 		Short: "Load a private key file for signing",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return loadPrivKey(dockerCli, args[0], options)
 		},
 	}
 	flags := cmd.Flags()
-	flags.StringVarP(&options.keyName, "name", "n", "signer", "Name for the loaded key")
+	flags.StringVar(&options.keyName, "name", "signer", "Name for the loaded key")
 	return cmd
 }
 
 func loadPrivKey(streams command.Streams, keyPath string, options keyLoadOptions) error {
+	// validate the key name if provided
+	if options.keyName != "" && !validKeyName(options.keyName) {
+		return fmt.Errorf("key name \"%s\" must start with lowercase alphanumeric characters and can include \"-\" or \"_\" after the first character", options.keyName)
+	}
 	trustDir := trust.GetTrustDirectory()
 	keyFileStore, err := storage.NewPrivateKeyFileStorage(trustDir, notary.KeyExtension)
 	if err != nil {
 		return err
 	}
 	privKeyImporters := []trustmanager.Importer{keyFileStore}
 
-	fmt.Fprintf(streams.Out(), "\nLoading key from \"%s\"...\n", keyPath)
+	fmt.Fprintf(streams.Out(), "Loading key from \"%s\"...\n", keyPath)
 
 	// Always use a fresh passphrase retriever for each import
 	passRet := trust.GetPassphraseRetriever(streams.In(), streams.Out())
 	keyBytes, err := getPrivKeyBytesFromPath(keyPath)
 	if err != nil {
-		return errors.Wrapf(err, "error reading key from %s", keyPath)
+		return errors.Wrapf(err, "refusing to load key from %s", keyPath)
 	}
 	if err := loadPrivKeyBytesToStore(keyBytes, privKeyImporters, keyPath, options.keyName, passRet); err != nil {
 		return errors.Wrapf(err, "error importing key from %s", keyPath)
@@ -69,8 +73,8 @@ func getPrivKeyBytesFromPath(keyPath string) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	if fileInfo.Mode() != ownerReadOnlyPerms && fileInfo.Mode() != ownerReadAndWritePerms {
-		return nil, fmt.Errorf("private key permission from %s should be set to 400 or 600", keyPath)
+	if fileInfo.Mode()&nonOwnerReadWriteMask != 0 {
+		return nil, fmt.Errorf("private key file %s must not be readable or writable by others", keyPath)
 	}
 
 	from, err := os.OpenFile(keyPath, os.O_RDONLY, notary.PrivExecPerms)
@@ -83,9 +87,29 @@ func getPrivKeyBytesFromPath(keyPath string) ([]byte, error) {
 }
 
 func loadPrivKeyBytesToStore(privKeyBytes []byte, privKeyImporters []trustmanager.Importer, keyPath, keyName string, passRet notary.PassRetriever) error {
-	if _, _, err := tufutils.ExtractPrivateKeyAttributes(privKeyBytes); err != nil {
+	var err error
+	if _, _, err = tufutils.ExtractPrivateKeyAttributes(privKeyBytes); err != nil {
 		return fmt.Errorf("provided file %s is not a supported private key - to add a signer's public key use docker trust signer add", keyPath)
 	}
+	if privKeyBytes, err = decodePrivKeyIfNecessary(privKeyBytes, passRet); err != nil {
+		return errors.Wrapf(err, "cannot load key from provided file %s", keyPath)
+	}
 	// Make a reader, rewind the file pointer
 	return trustmanager.ImportKeys(bytes.NewReader(privKeyBytes), privKeyImporters, keyName, "", passRet)
 }
+
+func decodePrivKeyIfNecessary(privPemBytes []byte, passRet notary.PassRetriever) ([]byte, error) {
+	pemBlock, _ := pem.Decode(privPemBytes)
+	_, containsDEKInfo := pemBlock.Headers["DEK-Info"]
+	if containsDEKInfo || pemBlock.Type == "ENCRYPTED PRIVATE KEY" {
+		// if we do not have enough information to properly import, try to decrypt the key
+		if _, ok := pemBlock.Headers["path"]; !ok {
+			privKey, _, err := trustmanager.GetPasswdDecryptBytes(passRet, privPemBytes, "", "encrypted")
+			if err != nil {
+				return []byte{}, fmt.Errorf("could not decrypt key")
+			}
+			privPemBytes = privKey.Private()
+		}
+	}
+	return privPemBytes, nil
+}

components/cli/cli/command/trust/key_load_test.go

@@ -40,8 +40,14 @@ func TestTrustKeyLoadErrors(t *testing.T) {
 		{
 			name:           "not-a-key",
 			args:           []string{"iamnotakey"},
-			expectedError:  "error reading key from iamnotakey: stat iamnotakey: no such file or directory",
-			expectedOutput: "\nLoading key from \"iamnotakey\"...\n",
+			expectedError:  "refusing to load key from iamnotakey: stat iamnotakey: no such file or directory",
+			expectedOutput: "Loading key from \"iamnotakey\"...\n",
+		},
+		{
+			name:           "bad-key-name",
+			args:           []string{"iamnotakey", "--name", "KEYNAME"},
+			expectedError:  "key name \"KEYNAME\" must start with lowercase alphanumeric characters and can include \"-\" or \"_\" after the first character",
+			expectedOutput: "",
 		},
 	}
 	tmpDir, err := ioutil.TempDir("", "docker-key-load-test-")
@@ -178,21 +184,21 @@ func testLoadKeyTooPermissive(t *testing.T, privKeyFixture []byte) {
 	// import the key to our keyStorageDir
 	_, err = getPrivKeyBytesFromPath(privKeyFilepath)
 	assert.Error(t, err)
-	assert.Contains(t, fmt.Sprintf("private key permission from %s should be set to 400 or 600", privKeyFilepath), err.Error())
+	assert.Contains(t, fmt.Sprintf("private key file %s must not be readable or writable by others", privKeyFilepath), err.Error())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey667.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0677))
 
 	_, err = getPrivKeyBytesFromPath(privKeyFilepath)
 	assert.Error(t, err)
-	assert.Contains(t, fmt.Sprintf("private key permission from %s should be set to 400 or 600", privKeyFilepath), err.Error())
+	assert.Contains(t, fmt.Sprintf("private key file %s must not be readable or writable by others", privKeyFilepath), err.Error())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey777.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0777))
 
 	_, err = getPrivKeyBytesFromPath(privKeyFilepath)
 	assert.Error(t, err)
-	assert.Contains(t, fmt.Sprintf("private key permission from %s should be set to 400 or 600", privKeyFilepath), err.Error())
+	assert.Contains(t, fmt.Sprintf("private key file %s must not be readable or writable by others", privKeyFilepath), err.Error())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey400.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0400))
@@ -208,9 +214,9 @@ func testLoadKeyTooPermissive(t *testing.T, privKeyFixture []byte) {
 }
 
 var pubKeyFixture = []byte(`-----BEGIN PUBLIC KEY-----
-	MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEUIH9AYtrcDFzZrFJBdJZkn21d+4c
-	H3nzy2O6Q/ct4BjOBKa+WCdRtPo78bA+C/7t81ADQO8Jqaj59W50rwoqDQ==
-	-----END PUBLIC KEY-----`)
+MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEUIH9AYtrcDFzZrFJBdJZkn21d+4c
+H3nzy2O6Q/ct4BjOBKa+WCdRtPo78bA+C/7t81ADQO8Jqaj59W50rwoqDQ==
+-----END PUBLIC KEY-----`)
 
 func TestLoadPubKeyFailure(t *testing.T) {
 	pubKeyDir, err := ioutil.TempDir("", "key-load-test-pubkey-")