refactoring and adding tests for EC key types

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

cli/command/trust/key_generate.go

@@ -19,15 +19,24 @@ import (
 	"github.com/spf13/cobra"
 )
 
+type keyGenerateOptions struct {
+	name      string
+	directory string
+}
+
 func newKeyGenerateCommand(dockerCli command.Streams) *cobra.Command {
+	options := keyGenerateOptions{}
 	cmd := &cobra.Command{
 		Use:   "generate NAME [NAME...]",
 		Short: "Generate and load a signing key-pair",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return setupPassphraseAndGenerateKeys(dockerCli, args[0])
+			options.name = args[0]
+			return setupPassphraseAndGenerateKeys(dockerCli, options)
 		},
 	}
+	flags := cmd.Flags()
+	flags.StringVarP(&options.directory, "dir", "d", "", "Directory to generate key in, defaults to current directory")
 	return cmd
 }
 
@@ -48,60 +57,74 @@ func validateKeyArgs(keyName string, cwdPath string) error {
 	return nil
 }
 
-func setupPassphraseAndGenerateKeys(streams command.Streams, keyName string) error {
+func setupPassphraseAndGenerateKeys(streams command.Streams, opts keyGenerateOptions) error {
-	// always use a fresh passphrase for each key generation
+	targetDir := opts.directory
-	freshPassRetGetter := func() notary.PassRetriever { return trust.GetPassphraseRetriever(streams.In(), streams.Out()) }
+	// if the target dir is empty, default to CWD
-	cwd, err := os.Getwd()
+	if targetDir == "" {
-	if err != nil {
+		cwd, err := os.Getwd()
-		return err
+		if err != nil {
+			return err
+		}
+		targetDir = cwd
 	}
-	return validateAndGenerateKey(streams, keyName, cwd, freshPassRetGetter)
+	return validateAndGenerateKey(streams, opts.name, targetDir)
 }
 
-func validateAndGenerateKey(streams command.Streams, keyName string, workingDir string, passphraseGetter func() notary.PassRetriever) error {
+func validateAndGenerateKey(streams command.Streams, keyName string, workingDir string) error {
+	freshPassRetGetter := func() notary.PassRetriever { return trust.GetPassphraseRetriever(streams.In(), streams.Out()) }
 	if err := validateKeyArgs(keyName, workingDir); err != nil {
 		return err
 	}
 	fmt.Fprintf(streams.Out(), "\nGenerating key for %s...\n", keyName)
-	freshPassRet := passphraseGetter()
+	// Automatically load the private key to local storage for use
-	if err := generateKey(keyName, workingDir, trust.GetTrustDirectory(), freshPassRet); err != nil {
+	privKeyFileStore, err := trustmanager.NewKeyFileStore(trust.GetTrustDirectory(), freshPassRetGetter())
+	if err != nil {
+		return err
+	}
+
+	pubPEM, err := generateKeyAndOutputPubPEM(keyName, privKeyFileStore)
+	if err != nil {
 		fmt.Fprintf(streams.Out(), err.Error())
 		return fmt.Errorf("Error generating key for: %s", keyName)
 	}
-	pubFileName := strings.Join([]string{keyName, "pub"}, ".")
+
-	fmt.Fprintf(streams.Out(), "Successfully generated and loaded private key. Corresponding public key available: %s\n", pubFileName)
+	// Output the public key to a file in the CWD or specified dir
+	writtenPubFile, err := writePubKeyPEMToDir(pubPEM, keyName, workingDir)
+	if err != nil {
+		return err
+	}
+	fmt.Fprintf(streams.Out(), "Successfully generated and loaded private key. Corresponding public key available: %s\n", writtenPubFile)
 
 	return nil
 }
 
-func generateKey(keyName, pubDir, privTrustDir string, passRet notary.PassRetriever) error {
+func generateKeyAndOutputPubPEM(keyName string, privKeyStore trustmanager.KeyStore) (pem.Block, error) {
 	privKey, err := tufutils.GenerateKey(data.ECDSAKey)
 	if err != nil {
-		return err
+		return pem.Block{}, err
 	}
 
-	// Automatically load the private key to local storage for use
+	privKeyStore.AddKey(trustmanager.KeyInfo{Role: data.RoleName(keyName)}, privKey)
-	privKeyFileStore, err := trustmanager.NewKeyFileStore(privTrustDir, passRet)
 	if err != nil {
-		return err
+		return pem.Block{}, err
-	}
-
-	privKeyFileStore.AddKey(trustmanager.KeyInfo{Role: data.RoleName(keyName)}, privKey)
-	if err != nil {
-		return err
 	}
 
 	pubKey := data.PublicKeyFromPrivate(privKey)
-	pubPEM := pem.Block{
+	return pem.Block{
 		Type: "PUBLIC KEY",
 		Headers: map[string]string{
 			"role": keyName,
 		},
 		Bytes: pubKey.Public(),
-	}
+	}, nil
-
+}
-	// Output the public key to a file in the CWD
+
-	pubFileName := strings.Join([]string{keyName, "pub"}, ".")
+func writePubKeyPEMToDir(pubPEM pem.Block, keyName, workingDir string) (string, error) {
-	pubFilePath := filepath.Join(pubDir, pubFileName)
+	// Output the public key to a file in the CWD or specified dir
-	return ioutil.WriteFile(pubFilePath, pem.EncodeToMemory(&pubPEM), notary.PrivNoExecPerms)
+	pubFileName := strings.Join([]string{keyName, "pub"}, ".")
+	pubFilePath := filepath.Join(workingDir, pubFileName)
+	if err := ioutil.WriteFile(pubFilePath, pem.EncodeToMemory(&pubPEM), notary.PrivNoExecPerms); err != nil {
+		return "", fmt.Errorf("Error writing public key to location: %s", pubFilePath)
+	}
+	return pubFilePath, nil
 }

cli/command/trust/key_generate_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/docker/cli/internal/test/testutil"
 	"github.com/docker/notary"
 	"github.com/docker/notary/passphrase"
+	"github.com/docker/notary/trustmanager"
 	tufutils "github.com/docker/notary/tuf/utils"
 	"github.com/stretchr/testify/assert"
 )
@@ -60,27 +61,17 @@ func TestGenerateKeySuccess(t *testing.T) {
 	cannedPasswordRetriever := passphrase.ConstantRetriever(passwd)
 	// generate a single key
 	keyName := "alice"
-	assert.NoError(t, generateKey(keyName, pubKeyCWD, privKeyStorageDir, cannedPasswordRetriever))
+	privKeyFileStore, err := trustmanager.NewKeyFileStore(privKeyStorageDir, cannedPasswordRetriever)
-
-	// check that the public key exists:
-	expectedPubKeyPath := filepath.Join(pubKeyCWD, keyName+".pub")
-	_, err = os.Stat(expectedPubKeyPath)
 	assert.NoError(t, err)
-	// check that the public key is the only file output in CWD
-	cwdKeyFiles, err := ioutil.ReadDir(pubKeyCWD)
-	assert.NoError(t, err)
-	assert.Len(t, cwdKeyFiles, 1)
 
-	// verify the key header is set with the specified name
+	pubKeyPEM, err := generateKeyAndOutputPubPEM(keyName, privKeyFileStore)
-	from, _ := os.OpenFile(expectedPubKeyPath, os.O_RDONLY, notary.PrivExecPerms)
+	assert.NoError(t, err)
-	defer from.Close()
+
-	fromBytes, _ := ioutil.ReadAll(from)
+	assert.Equal(t, keyName, pubKeyPEM.Headers["role"])
-	keyPEM, _ := pem.Decode(fromBytes)
-	assert.Equal(t, keyName, keyPEM.Headers["role"])
 	// the default GUN is empty
-	assert.Equal(t, "", keyPEM.Headers["gun"])
+	assert.Equal(t, "", pubKeyPEM.Headers["gun"])
 	// assert public key header
-	assert.Equal(t, "PUBLIC KEY", keyPEM.Type)
+	assert.Equal(t, "PUBLIC KEY", pubKeyPEM.Type)
 
 	// check that an appropriate ~/<trust_dir>/private/<key_id>.key file exists
 	expectedPrivKeyDir := filepath.Join(privKeyStorageDir, notary.PrivDir)
@@ -95,16 +86,28 @@ func TestGenerateKeySuccess(t *testing.T) {
 	// verify the key content
 	privFrom, _ := os.OpenFile(privKeyFilePath, os.O_RDONLY, notary.PrivExecPerms)
 	defer privFrom.Close()
-	fromBytes, _ = ioutil.ReadAll(privFrom)
+	fromBytes, _ := ioutil.ReadAll(privFrom)
-	keyPEM, _ = pem.Decode(fromBytes)
+	privKeyPEM, _ := pem.Decode(fromBytes)
-	assert.Equal(t, keyName, keyPEM.Headers["role"])
+	assert.Equal(t, keyName, privKeyPEM.Headers["role"])
 	// the default GUN is empty
-	assert.Equal(t, "", keyPEM.Headers["gun"])
+	assert.Equal(t, "", privKeyPEM.Headers["gun"])
 	// assert encrypted header
-	assert.Equal(t, "ENCRYPTED PRIVATE KEY", keyPEM.Type)
+	assert.Equal(t, "ENCRYPTED PRIVATE KEY", privKeyPEM.Type)
 	// check that the passphrase matches
-	_, err = tufutils.ParsePKCS8ToTufKey(keyPEM.Bytes, []byte(passwd))
+	_, err = tufutils.ParsePKCS8ToTufKey(privKeyPEM.Bytes, []byte(passwd))
 	assert.NoError(t, err)
+
+	// check that the public key exists at the correct path if we use the helper:
+	returnedPath, err := writePubKeyPEMToDir(pubKeyPEM, keyName, pubKeyCWD)
+	assert.NoError(t, err)
+	expectedPubKeyPath := filepath.Join(pubKeyCWD, keyName+".pub")
+	assert.Equal(t, returnedPath, expectedPubKeyPath)
+	_, err = os.Stat(expectedPubKeyPath)
+	assert.NoError(t, err)
+	// check that the public key is the only file output in CWD
+	cwdKeyFiles, err := ioutil.ReadDir(pubKeyCWD)
+	assert.NoError(t, err)
+	assert.Len(t, cwdKeyFiles, 1)
 }
 
 func TestValidateKeyArgs(t *testing.T) {

cli/command/trust/key_load.go

@@ -1,10 +1,10 @@
 package trust
 
 import (
+	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
-	"path/filepath"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
@@ -42,7 +42,7 @@ func newKeyLoadCommand(dockerCli command.Streams) *cobra.Command {
 
 func loadPrivKey(streams command.Streams, keyPath string, options keyLoadOptions) error {
 	trustDir := trust.GetTrustDirectory()
-	keyFileStore, err := storage.NewPrivateKeyFileStorage(filepath.Join(trustDir, notary.PrivDir), notary.KeyExtension)
+	keyFileStore, err := storage.NewPrivateKeyFileStorage(trustDir, notary.KeyExtension)
 	if err != nil {
 		return err
 	}
@@ -52,39 +52,43 @@ func loadPrivKey(streams command.Streams, keyPath string, options keyLoadOptions
 
 	// Always use a fresh passphrase retriever for each import
 	passRet := trust.GetPassphraseRetriever(streams.In(), streams.Out())
-	if err := loadPrivKeyFromPath(privKeyImporters, keyPath, options.keyName, passRet); err != nil {
+	keyBytes, err := getPrivKeyBytesFromPath(keyPath)
+	if err != nil {
+		return fmt.Errorf("error reading key from %s: %s", keyPath, err)
+	}
+	if err := loadPrivKeyBytesToStore(keyBytes, privKeyImporters, keyPath, options.keyName, passRet); err != nil {
 		return fmt.Errorf("error importing key from %s: %s", keyPath, err)
 	}
 	fmt.Fprintf(streams.Out(), "Successfully imported key from %s\n", keyPath)
 	return nil
 }
 
-func loadPrivKeyFromPath(privKeyImporters []utils.Importer, keyPath, keyName string, passRet notary.PassRetriever) error {
+func getPrivKeyBytesFromPath(keyPath string) ([]byte, error) {
 	fileInfo, err := os.Stat(keyPath)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if fileInfo.Mode() != ownerReadOnlyPerms && fileInfo.Mode() != ownerReadAndWritePerms {
-		return fmt.Errorf("private key permission from %s should be set to 400 or 600", keyPath)
+		return nil, fmt.Errorf("private key permission from %s should be set to 400 or 600", keyPath)
 	}
 
 	from, err := os.OpenFile(keyPath, os.O_RDONLY, notary.PrivExecPerms)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	defer from.Close()
 
 	keyBytes, err := ioutil.ReadAll(from)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	if _, _, err := tufutils.ExtractPrivateKeyAttributes(keyBytes); err != nil {
+	return keyBytes, nil
+}
+
+func loadPrivKeyBytesToStore(privKeyBytes []byte, privKeyImporters []utils.Importer, keyPath, keyName string, passRet notary.PassRetriever) 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)
 	}
-	// Rewind the file pointer
+	// Make a reader, rewind the file pointer
-	if _, err := from.Seek(0, 0); err != nil {
+	return utils.ImportKeys(bytes.NewReader(privKeyBytes), privKeyImporters, keyName, "", passRet)
-		return err
-	}
-
-	return utils.ImportKeys(from, privKeyImporters, keyName, "", passRet)
 }

cli/command/trust/key_load_test.go

@@ -40,7 +40,7 @@ func TestTrustKeyLoadErrors(t *testing.T) {
 		{
 			name:           "not-a-key",
 			args:           []string{"iamnotakey"},
-			expectedError:  "error importing key from iamnotakey: stat iamnotakey: no such file or directory",
+			expectedError:  "error reading key from iamnotakey: stat iamnotakey: no such file or directory",
 			expectedOutput: "\nLoading key from \"iamnotakey\"...\n",
 		},
 	}
@@ -59,7 +59,7 @@ func TestTrustKeyLoadErrors(t *testing.T) {
 	}
 }
 
-var privKeyFixture = []byte(`-----BEGIN RSA PRIVATE KEY-----
+var rsaPrivKeyFixture = []byte(`-----BEGIN RSA PRIVATE KEY-----
 MIIEpAIBAAKCAQEAs7yVMzCw8CBZPoN+QLdx3ZzbVaHnouHIKu+ynX60IZ3stpbb
 6rowu78OWON252JcYJqe++2GmdIgbBhg+mZDwhX0ZibMVztJaZFsYL+Ch/2J9KqD
 A5NtE1s/XdhYoX5hsv7W4ok9jLFXRYIMj+T4exJRlR4f4GP9p0fcqPWd9/enPnlJ
@@ -87,9 +87,30 @@ GIhbizeGJ3h4cUdozKmt8ZWIt6uFDEYCqEA7XF4RH75dW25x86mpIPO7iRl9eisY
 IsLeMYqTIwXAwGx6Ka9v5LOL1kzcHQ2iVj6+QX+yoptSft1dYa9jOA==
 -----END RSA PRIVATE KEY-----`)
 
-const privKeyID = "ee69e8e07a14756ad5ff0aca2336b37f86b0ac1710d1f3e94440081e080aecd7"
+const rsaPrivKeyID = "ee69e8e07a14756ad5ff0aca2336b37f86b0ac1710d1f3e94440081e080aecd7"
+
+var ecPrivKeyFixture = []byte(`-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEINfxKtDH3ug7ZIQPDyeAzujCdhw36D+bf9ToPE1A7YEyoAoGCCqGSM49
+AwEHoUQDQgAEUIH9AYtrcDFzZrFJBdJZkn21d+4cH3nzy2O6Q/ct4BjOBKa+WCdR
+tPo78bA+C/7t81ADQO8Jqaj59W50rwoqDQ==
+-----END EC PRIVATE KEY-----`)
+
+const ecPrivKeyID = "46157cb0becf9c72c3219e11d4692424fef9bf4460812ccc8a71a3dfcafc7e60"
+
+var testKeys = map[string][]byte{
+	ecPrivKeyID:  ecPrivKeyFixture,
+	rsaPrivKeyID: rsaPrivKeyFixture,
+}
 
 func TestLoadKeyFromPath(t *testing.T) {
+	for keyID, keyBytes := range testKeys {
+		t.Run(fmt.Sprintf("load-key-id-%s-from-path", keyID), func(t *testing.T) {
+			testLoadKeyFromPath(t, keyID, keyBytes)
+		})
+	}
+}
+
+func testLoadKeyFromPath(t *testing.T, privKeyID string, privKeyFixture []byte) {
 	privKeyDir, err := ioutil.TempDir("", "key-load-test-")
 	assert.NoError(t, err)
 	defer os.RemoveAll(privKeyDir)
@@ -106,8 +127,12 @@ func TestLoadKeyFromPath(t *testing.T) {
 	assert.NoError(t, err)
 	privKeyImporters := []utils.Importer{keyFileStore}
 
+	// get the privKeyBytes
+	privKeyBytes, err := getPrivKeyBytesFromPath(privKeyFilepath)
+	assert.NoError(t, err)
+
 	// import the key to our keyStorageDir
-	assert.NoError(t, loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer-name", cannedPasswordRetriever))
+	assert.NoError(t, loadPrivKeyBytesToStore(privKeyBytes, privKeyImporters, privKeyFilepath, "signer-name", cannedPasswordRetriever))
 
 	// check that the appropriate ~/<trust_dir>/private/<key_id>.key file exists
 	expectedImportKeyPath := filepath.Join(keyStorageDir, notary.PrivDir, privKeyID+"."+notary.KeyExtension)
@@ -132,6 +157,14 @@ func TestLoadKeyFromPath(t *testing.T) {
 }
 
 func TestLoadKeyTooPermissive(t *testing.T) {
+	for keyID, keyBytes := range testKeys {
+		t.Run(fmt.Sprintf("load-key-id-%s-too-permissive", keyID), func(t *testing.T) {
+			testLoadKeyTooPermissive(t, keyBytes)
+		})
+	}
+}
+
+func testLoadKeyTooPermissive(t *testing.T, privKeyFixture []byte) {
 	privKeyDir, err := ioutil.TempDir("", "key-load-test-")
 	assert.NoError(t, err)
 	defer os.RemoveAll(privKeyDir)
@@ -142,41 +175,35 @@ func TestLoadKeyTooPermissive(t *testing.T) {
 	assert.NoError(t, err)
 	defer os.RemoveAll(keyStorageDir)
 
-	passwd := "password"
-	cannedPasswordRetriever := passphrase.ConstantRetriever(passwd)
-	keyFileStore, err := storage.NewPrivateKeyFileStorage(keyStorageDir, notary.KeyExtension)
-	assert.NoError(t, err)
-	privKeyImporters := []utils.Importer{keyFileStore}
-
 	// import the key to our keyStorageDir
-	err = loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer", cannedPasswordRetriever)
+	_, 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())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey667.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0677))
 
-	err = loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer", cannedPasswordRetriever)
+	_, 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())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey777.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0777))
 
-	err = loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer", cannedPasswordRetriever)
+	_, 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())
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey400.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0400))
 
-	err = loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer", cannedPasswordRetriever)
+	_, err = getPrivKeyBytesFromPath(privKeyFilepath)
 	assert.NoError(t, err)
 
 	privKeyFilepath = filepath.Join(privKeyDir, "privkey600.pem")
 	assert.NoError(t, ioutil.WriteFile(privKeyFilepath, privKeyFixture, 0600))
 
-	err = loadPrivKeyFromPath(privKeyImporters, privKeyFilepath, "signer", cannedPasswordRetriever)
+	_, err = getPrivKeyBytesFromPath(privKeyFilepath)
 	assert.NoError(t, err)
 }
 
@@ -201,8 +228,11 @@ func TestLoadPubKeyFailure(t *testing.T) {
 	assert.NoError(t, err)
 	privKeyImporters := []utils.Importer{keyFileStore}
 
+	pubKeyBytes, err := getPrivKeyBytesFromPath(pubKeyFilepath)
+	assert.NoError(t, err)
+
 	// import the key to our keyStorageDir - it should fail
-	err = loadPrivKeyFromPath(privKeyImporters, pubKeyFilepath, "signer", cannedPasswordRetriever)
+	err = loadPrivKeyBytesToStore(pubKeyBytes, privKeyImporters, pubKeyFilepath, "signer-name", cannedPasswordRetriever)
 	assert.Error(t, err)
 	assert.Contains(t, fmt.Sprintf("provided file %s is not a supported private key - to add a signer's public key use docker trust signer add", pubKeyFilepath), err.Error())
 }

cli/command/trust/sign.go

@@ -183,7 +183,9 @@ func initNotaryRepoWithSigners(notaryRepo client.Repository, newSigner data.Role
 	if err != nil {
 		return err
 	}
-	addStagedSigner(notaryRepo, newSigner, []data.PublicKey{signerKey})
+	if err := addStagedSigner(notaryRepo, newSigner, []data.PublicKey{signerKey}); err != nil {
+		return errors.Wrapf(err, "could not add signer to repo: %s", strings.TrimPrefix(newSigner.String(), "targets/"))
+	}
 
 	return notaryRepo.Publish()
 }
@@ -216,12 +218,21 @@ func getOrGenerateNotaryKey(notaryRepo client.Repository, role data.RoleName) (d
 }
 
 // stages changes to add a signer with the specified name and key(s).  Adds to targets/<name> and targets/releases
-func addStagedSigner(notaryRepo client.Repository, newSigner data.RoleName, signerKeys []data.PublicKey) {
+func addStagedSigner(notaryRepo client.Repository, newSigner data.RoleName, signerKeys []data.PublicKey) error {
 	// create targets/<username>
-	notaryRepo.AddDelegationRoleAndKeys(newSigner, signerKeys)
+	if err := notaryRepo.AddDelegationRoleAndKeys(newSigner, signerKeys); err != nil {
-	notaryRepo.AddDelegationPaths(newSigner, []string{""})
+		return err
+	}
+	if err := notaryRepo.AddDelegationPaths(newSigner, []string{""}); err != nil {
+		return err
+	}
 
 	// create targets/releases
-	notaryRepo.AddDelegationRoleAndKeys(trust.ReleasesRole, signerKeys)
+	if err := notaryRepo.AddDelegationRoleAndKeys(trust.ReleasesRole, signerKeys); err != nil {
-	notaryRepo.AddDelegationPaths(trust.ReleasesRole, []string{""})
+		return err
+	}
+	if err := notaryRepo.AddDelegationPaths(trust.ReleasesRole, []string{""}); err != nil {
+		return err
+	}
+	return nil
 }

cli/command/trust/sign_test.go

@@ -140,7 +140,8 @@ func TestAddStageSigners(t *testing.T) {
 	// stage targets/user
 	userRole := data.RoleName("targets/user")
 	userKey := data.NewPublicKey("algoA", []byte("a"))
-	addStagedSigner(notaryRepo, userRole, []data.PublicKey{userKey})
+	err = addStagedSigner(notaryRepo, userRole, []data.PublicKey{userKey})
+	assert.NoError(t, err)
 	// check the changelist for four total changes: two on targets/releases and two on targets/user
 	cl, err := notaryRepo.GetChangelist()
 	assert.NoError(t, err)

cli/command/trust/signer_add.go

@@ -18,6 +18,7 @@ import (
 	"github.com/docker/notary/client"
 	"github.com/docker/notary/tuf/data"
 	tufutils "github.com/docker/notary/tuf/utils"
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 )
 
@@ -36,7 +37,7 @@ func newSignerAddCommand(dockerCli command.Cli) *cobra.Command {
 		RunE: func(cmd *cobra.Command, args []string) error {
 			options.signer = args[0]
 			options.images = args[1:]
-			return addSigner(dockerCli, &options)
+			return addSigner(dockerCli, options)
 		},
 	}
 	flags := cmd.Flags()
@@ -47,7 +48,7 @@ func newSignerAddCommand(dockerCli command.Cli) *cobra.Command {
 
 var validSignerName = regexp.MustCompile(`^[a-z0-9]+[a-z0-9\_\-]*$`).MatchString
 
-func addSigner(cli command.Cli, options *signerAddOptions) error {
+func addSigner(cli command.Cli, options signerAddOptions) error {
 	signerName := options.signer
 	if !validSignerName(signerName) {
 		return fmt.Errorf("signer name \"%s\" must not contain uppercase or special characters", signerName)
@@ -111,7 +112,9 @@ func addSignerToImage(cli command.Cli, signerName string, imageName string, keyP
 	if err != nil {
 		return err
 	}
-	addStagedSigner(notaryRepo, newSignerRoleName, signerPubKeys)
+	if err := addStagedSigner(notaryRepo, newSignerRoleName, signerPubKeys); err != nil {
+		return errors.Wrapf(err, "could not add signer to repo: %s", strings.TrimPrefix(newSignerRoleName.String(), "targets/"))
+	}
 
 	return notaryRepo.Publish()
 }

cli/command/trust/signer_remove.go

@@ -17,6 +17,8 @@ import (
 )
 
 type signerRemoveOptions struct {
+	signer   string
+	images   []string
 	forceYes bool
 }
 
@@ -27,18 +29,20 @@ func newSignerRemoveCommand(dockerCli command.Cli) *cobra.Command {
 		Short: "Remove a signer",
 		Args:  cli.RequiresMinArgs(2),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return removeSigner(dockerCli, args[0], args[1:], &options)
+			options.signer = args[0]
+			options.images = args[1:]
+			return removeSigner(dockerCli, options)
 		},
 	}
 	flags := cmd.Flags()
-	flags.BoolVarP(&options.forceYes, "yes", "y", false, "Answer yes to removing most recent signer (no confirmation)")
+	flags.BoolVarP(&options.forceYes, "yes", "y", false, "Do not prompt for confirmation before removing the most recent signer")
 	return cmd
 }
 
-func removeSigner(cli command.Cli, signer string, images []string, options *signerRemoveOptions) error {
+func removeSigner(cli command.Cli, options signerRemoveOptions) error {
 	var errImages []string
-	for _, image := range images {
+	for _, image := range options.images {
-		if err := removeSingleSigner(cli, image, signer, options.forceYes); err != nil {
+		if err := removeSingleSigner(cli, image, options.signer, options.forceYes); err != nil {
 			fmt.Fprintln(cli.Out(), err.Error())
 			errImages = append(errImages, image)
 		}

cli/command/trust/signer_remove_test.go

@@ -81,7 +81,7 @@ func TestRemoveSingleSigner(t *testing.T) {
 func TestRemoveMultipleSigners(t *testing.T) {
 	cli := test.NewFakeCli(&fakeClient{})
 	cli.SetNotaryClient(getLoadedNotaryRepository)
-	err := removeSigner(cli, "test", []string{"signed-repo", "signed-repo"}, &signerRemoveOptions{forceYes: true})
+	err := removeSigner(cli, signerRemoveOptions{signer: "test", images: []string{"signed-repo", "signed-repo"}, forceYes: true})
 	assert.EqualError(t, err, "Error removing signer from: signed-repo, signed-repo")
 	assert.Contains(t, cli.OutBuffer().String(),
 		"\nRemoving signer \"test\" from signed-repo...\nNo signer test for image signed-repo\n\nRemoving signer \"test\" from signed-repo...\nNo signer test for image signed-repo")
@@ -89,7 +89,8 @@ func TestRemoveMultipleSigners(t *testing.T) {
 func TestRemoveLastSignerWarning(t *testing.T) {
 	cli := test.NewFakeCli(&fakeClient{})
 	cli.SetNotaryClient(getLoadedNotaryRepository)
-	err := removeSigner(cli, "alice", []string{"signed-repo"}, &signerRemoveOptions{forceYes: false})
+
+	err := removeSigner(cli, signerRemoveOptions{signer: "alice", images: []string{"signed-repo"}, forceYes: false})
 	assert.NoError(t, err)
 	assert.Contains(t, cli.OutBuffer().String(),
 		"The signer \"alice\" signed the last released version of signed-repo. "+