diff --git a/cli/command/trust/key_generate.go b/cli/command/trust/key_generate.go index c0473f18c2..23b3f121b9 100644 --- a/cli/command/trust/key_generate.go +++ b/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 { - // always use a fresh passphrase for each key generation - freshPassRetGetter := func() notary.PassRetriever { return trust.GetPassphraseRetriever(streams.In(), streams.Out()) } - cwd, err := os.Getwd() - if err != nil { - return err +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 { + 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() - if err := generateKey(keyName, workingDir, trust.GetTrustDirectory(), freshPassRet); err != nil { + // Automatically load the private key to local storage for use + 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 - privKeyFileStore, err := trustmanager.NewKeyFileStore(privTrustDir, passRet) + privKeyStore.AddKey(trustmanager.KeyInfo{Role: data.RoleName(keyName)}, privKey) if err != nil { - return err - } - - privKeyFileStore.AddKey(trustmanager.KeyInfo{Role: data.RoleName(keyName)}, privKey) - if err != nil { - return err + return pem.Block{}, err } pubKey := data.PublicKeyFromPrivate(privKey) - pubPEM := pem.Block{ + return pem.Block{ Type: "PUBLIC KEY", Headers: map[string]string{ "role": keyName, }, Bytes: pubKey.Public(), - } - - // Output the public key to a file in the CWD - pubFileName := strings.Join([]string{keyName, "pub"}, ".") - pubFilePath := filepath.Join(pubDir, pubFileName) - return ioutil.WriteFile(pubFilePath, pem.EncodeToMemory(&pubPEM), notary.PrivNoExecPerms) + }, nil +} + +func writePubKeyPEMToDir(pubPEM pem.Block, keyName, workingDir string) (string, error) { + // Output the public key to a file in the CWD or specified dir + 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 } diff --git a/cli/command/trust/key_generate_test.go b/cli/command/trust/key_generate_test.go index 05afee342a..2aa0d256a9 100644 --- a/cli/command/trust/key_generate_test.go +++ b/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)) - - // check that the public key exists: - expectedPubKeyPath := filepath.Join(pubKeyCWD, keyName+".pub") - _, err = os.Stat(expectedPubKeyPath) + privKeyFileStore, err := trustmanager.NewKeyFileStore(privKeyStorageDir, cannedPasswordRetriever) 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 - from, _ := os.OpenFile(expectedPubKeyPath, os.O_RDONLY, notary.PrivExecPerms) - defer from.Close() - fromBytes, _ := ioutil.ReadAll(from) - keyPEM, _ := pem.Decode(fromBytes) - assert.Equal(t, keyName, keyPEM.Headers["role"]) + pubKeyPEM, err := generateKeyAndOutputPubPEM(keyName, privKeyFileStore) + assert.NoError(t, err) + + assert.Equal(t, keyName, pubKeyPEM.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 ~//private/.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) - keyPEM, _ = pem.Decode(fromBytes) - assert.Equal(t, keyName, keyPEM.Headers["role"]) + fromBytes, _ := ioutil.ReadAll(privFrom) + privKeyPEM, _ := pem.Decode(fromBytes) + 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) { diff --git a/cli/command/trust/key_load.go b/cli/command/trust/key_load.go index 9d8ec915c4..4207c3a957 100644 --- a/cli/command/trust/key_load.go +++ b/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 - if _, err := from.Seek(0, 0); err != nil { - return err - } - - return utils.ImportKeys(from, privKeyImporters, keyName, "", passRet) + // Make a reader, rewind the file pointer + return utils.ImportKeys(bytes.NewReader(privKeyBytes), privKeyImporters, keyName, "", passRet) } diff --git a/cli/command/trust/key_load_test.go b/cli/command/trust/key_load_test.go index 86d773d1e2..1a20dd55e3 100644 --- a/cli/command/trust/key_load_test.go +++ b/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 ~//private/.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()) } diff --git a/cli/command/trust/sign.go b/cli/command/trust/sign.go index e67373659a..af40e206e7 100644 --- a/cli/command/trust/sign.go +++ b/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/ 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/ - notaryRepo.AddDelegationRoleAndKeys(newSigner, signerKeys) - notaryRepo.AddDelegationPaths(newSigner, []string{""}) + if err := notaryRepo.AddDelegationRoleAndKeys(newSigner, signerKeys); err != nil { + return err + } + if err := notaryRepo.AddDelegationPaths(newSigner, []string{""}); err != nil { + return err + } // create targets/releases - notaryRepo.AddDelegationRoleAndKeys(trust.ReleasesRole, signerKeys) - notaryRepo.AddDelegationPaths(trust.ReleasesRole, []string{""}) + if err := notaryRepo.AddDelegationRoleAndKeys(trust.ReleasesRole, signerKeys); err != nil { + return err + } + if err := notaryRepo.AddDelegationPaths(trust.ReleasesRole, []string{""}); err != nil { + return err + } + return nil } diff --git a/cli/command/trust/sign_test.go b/cli/command/trust/sign_test.go index ec9a5dc59f..5334004397 100644 --- a/cli/command/trust/sign_test.go +++ b/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) diff --git a/cli/command/trust/signer_add.go b/cli/command/trust/signer_add.go index 6393371821..0b67fbee1c 100644 --- a/cli/command/trust/signer_add.go +++ b/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() } diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index 590cff8944..82b384fbb1 100644 --- a/cli/command/trust/signer_remove.go +++ b/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 { - if err := removeSingleSigner(cli, image, signer, options.forceYes); err != nil { + for _, image := range options.images { + if err := removeSingleSigner(cli, image, options.signer, options.forceYes); err != nil { fmt.Fprintln(cli.Out(), err.Error()) errImages = append(errImages, image) } diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go index 7ad841a82d..cde0c0b4ab 100644 --- a/cli/command/trust/signer_remove_test.go +++ b/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. "+