diff --git a/plugins/pass/commands/client.go b/plugins/pass/commands/client.go index 4778f782..6f74b721 100644 --- a/plugins/pass/commands/client.go +++ b/plugins/pass/commands/client.go @@ -125,3 +125,24 @@ func authorize(ctx context.Context, a secrets.Authorizer, patterns ...secrets.Pa } return nil } + +func authorizeAccess(ctx context.Context, opts clientOpts, ids ...secrets.ID) error { + patterns := make([]secrets.Pattern, 0, len(ids)) + for _, id := range ids { + pattern, err := secrets.ParsePattern(id.String()) + if err != nil { + return err + } + patterns = append(patterns, pattern) + } + c, err := newClient(opts) + if err != nil { + return err + } + if opts.isUnbound() { + if err := preflightPing(ctx, c, defaultPreflightPingTimeout); err != nil { + return err + } + } + return authorize(ctx, c, patterns...) +} diff --git a/plugins/pass/commands/command_test.go b/plugins/pass/commands/command_test.go index b4a41b1a..09644bdd 100644 --- a/plugins/pass/commands/command_test.go +++ b/plugins/pass/commands/command_test.go @@ -159,29 +159,62 @@ func Test_ListCommand(t *testing.T) { func Test_RmCommand(t *testing.T) { t.Parallel() - t.Run("ok (two secrets)", func(t *testing.T) { - mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ + twoSecrets := func() store.Store { + return teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ store.MustParseID("foo"): pass.NewPassValue([]byte("bar")), store.MustParseID("baz"): pass.NewPassValue([]byte("0")), })) - out, err := execute(t, RmCommand(), mock, "foo", "baz") + } + remaining := func(t *testing.T, kc store.Store) int { + t.Helper() + l, err := kc.GetAllMetadata(t.Context()) + require.NoError(t, err) + return len(l) + } + t.Run("ok (two secrets)", func(t *testing.T) { + engine := &mockEngine{allow: true} + mock := twoSecrets() + out, err := execute(t, mustRmCommand(t, engineOpts(t, engine)...), mock, "foo", "baz") assert.NoError(t, err) assert.Equal(t, "RM: baz\nRM: foo\n", out) - l, err := mock.GetAllMetadata(t.Context()) - require.NoError(t, err) - assert.Empty(t, l) + assert.Equal(t, []string{"authorize baz,foo"}, engine.recorded()) + assert.Equal(t, 0, remaining(t, mock)) }) t.Run("--all", func(t *testing.T) { - mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ - store.MustParseID("foo"): pass.NewPassValue([]byte("bar")), - store.MustParseID("baz"): pass.NewPassValue([]byte("0")), - })) - out, err := execute(t, RmCommand(), mock, "--all") + engine := &mockEngine{allow: true} + mock := twoSecrets() + out, err := execute(t, mustRmCommand(t, engineOpts(t, engine)...), mock, "--all") assert.NoError(t, err) assert.Equal(t, "RM: baz\nRM: foo\n", out) - l, err := mock.GetAllMetadata(t.Context()) - require.NoError(t, err) - assert.Empty(t, l) + assert.Equal(t, []string{"authorize baz,foo"}, engine.recorded()) + assert.Equal(t, 0, remaining(t, mock)) + }) + t.Run("denied removes nothing", func(t *testing.T) { + engine := &mockEngine{} + mock := twoSecrets() + out, err := execute(t, mustRmCommand(t, engineOpts(t, engine)...), mock, "--all") + assert.ErrorIs(t, err, client.ErrAccessDenied) + assert.Equal(t, "Error: authorizing: access denied\n", out) + assert.Equal(t, []string{"authorize baz,foo"}, engine.recorded()) + assert.Equal(t, 2, remaining(t, mock)) + }) + t.Run("denied reports missing secrets and removes nothing", func(t *testing.T) { + engine := &mockEngine{} + mock := twoSecrets() + out, err := execute(t, mustRmCommand(t, engineOpts(t, engine)...), mock, "foo", "missing") + assert.ErrorIs(t, err, client.ErrAccessDenied) + assert.ErrorIs(t, err, store.ErrCredentialNotFound) + assert.Equal(t, "ERR: missing: secret not found\nError: missing: secret not found\nauthorizing: access denied\n", out) + assert.Equal(t, []string{"authorize foo"}, engine.recorded()) + assert.Equal(t, 2, remaining(t, mock)) + }) + t.Run("unreachable engine removes nothing", func(t *testing.T) { + mock := twoSecrets() + cmd := mustRmCommand(t, WithTimeout(time.Second), WithSocketPath(deadSocket(t))) + out, err := execute(t, cmd, mock, "foo") + assert.ErrorIs(t, err, client.ErrSecretsEngineNotAvailable) + assert.NotContains(t, out, "RM:") + assert.Equal(t, 2, remaining(t, mock)) }) t.Run("store error", func(t *testing.T) { errRemove := errors.New("remove error") @@ -191,59 +224,64 @@ func Test_RmCommand(t *testing.T) { }), teststore.WithStoreDeleteErr(errRemove), ) - out, err := execute(t, RmCommand(), mock, "foo") + out, err := execute(t, mustRmCommand(t, engineOpts(t, &mockEngine{allow: true})...), mock, "foo") assert.ErrorIs(t, err, errRemove) assert.Equal(t, "ERR: foo: remove error\nError: "+errRemove.Error()+"\n", out) }) - t.Run("missing secret errors", func(t *testing.T) { + t.Run("missing secret errors without asking the engine", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, RmCommand(), mock, "foo") + out, err := execute(t, mustRmCommand(t, WithSocketPath(deadSocket(t))), mock, "foo") assert.ErrorIs(t, err, store.ErrCredentialNotFound) assert.Equal(t, "ERR: foo: secret not found\nError: foo: secret not found\n", out) }) t.Run("missing secret among existing ones", func(t *testing.T) { + engine := &mockEngine{allow: true} mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ store.MustParseID("foo"): pass.NewPassValue([]byte("bar")), })) - out, err := execute(t, RmCommand(), mock, "foo", "baz") + out, err := execute(t, mustRmCommand(t, engineOpts(t, engine)...), mock, "foo", "baz") assert.ErrorIs(t, err, store.ErrCredentialNotFound) assert.Equal(t, "ERR: baz: secret not found\nRM: foo\nError: baz: secret not found\n", out) - l, err := mock.GetAllMetadata(t.Context()) - require.NoError(t, err) - assert.Empty(t, l) + assert.Equal(t, []string{"authorize foo"}, engine.recorded()) + assert.Equal(t, 0, remaining(t, mock)) }) t.Run("metadata listing error", func(t *testing.T) { errList := errors.New("list error") mock := teststore.NewMockStore(teststore.WithStoreGetAllErr(errList)) - out, err := execute(t, RmCommand(), mock, "foo") + out, err := execute(t, mustRmCommand(t, WithSocketPath(deadSocket(t))), mock, "foo") assert.ErrorIs(t, err, errList) assert.Equal(t, "Error: "+errList.Error()+"\n", out) }) - t.Run("--all with empty store", func(t *testing.T) { + t.Run("--all with empty store needs no engine", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, RmCommand(), mock, "--all") + out, err := execute(t, mustRmCommand(t, WithSocketPath(deadSocket(t))), mock, "--all") assert.NoError(t, err) assert.Empty(t, out) }) t.Run("invalid id", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, RmCommand(), mock, "/foo") + out, err := execute(t, mustRmCommand(t), mock, "/foo") errInvalidID := secrets.ErrInvalidID{ID: "/foo"} assert.ErrorIs(t, err, errInvalidID) assert.Equal(t, "Error: "+errInvalidID.Error()+"\n", out) }) t.Run("cannot mix --all with explicit list", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, RmCommand(), mock, "--all", "foo") + out, err := execute(t, mustRmCommand(t), mock, "--all", "foo") assert.ErrorContains(t, err, "either provide a secret name or use --all to remove all secrets") assert.Equal(t, "Error: either provide a secret name or use --all to remove all secrets\n", out) }) t.Run("no args or --all", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, RmCommand(), mock) + out, err := execute(t, mustRmCommand(t), mock) assert.ErrorContains(t, err, "either provide a secret name or use --all to remove all secrets") assert.Equal(t, "Error: either provide a secret name or use --all to remove all secrets\n", out) }) + t.Run("rejects an invalid option", func(t *testing.T) { + cmd, err := RmCommand(WithTimeout(-time.Second)) + require.EqualError(t, err, "request timeout duration cannot be negative") + assert.Nil(t, cmd) + }) } func Test_GetCommand(t *testing.T) { @@ -253,10 +291,6 @@ func Test_GetCommand(t *testing.T) { store.MustParseID("foo"): pass.NewPassValue([]byte("bar")), })) } - deadSocket := func(t *testing.T) string { - t.Helper() - return filepath.Join(t.TempDir(), "dead.sock") - } t.Run("ok", func(t *testing.T) { out, err := execute(t, mustGetCommand(t), fooStore(), "foo") assert.NoError(t, err) @@ -276,7 +310,7 @@ func Test_GetCommand(t *testing.T) { }) t.Run("--reveal prints the value once the engine allows", func(t *testing.T) { engine := &mockEngine{allow: true} - cmd := mustGetCommand(t, WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + cmd := mustGetCommand(t, engineOpts(t, engine)...) out, err := execute(t, cmd, fooStore(), "--reveal", "foo") assert.NoError(t, err) assert.Equal(t, "ID: foo\nValue: bar\n", out) @@ -284,7 +318,7 @@ func Test_GetCommand(t *testing.T) { }) t.Run("--reveal fails when the engine denies", func(t *testing.T) { engine := &mockEngine{} - cmd := mustGetCommand(t, WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + cmd := mustGetCommand(t, engineOpts(t, engine)...) out, err := execute(t, cmd, fooStore(), "--reveal", "foo") assert.ErrorIs(t, err, client.ErrAccessDenied) assert.Equal(t, "Error: authorizing: access denied\n", out) @@ -294,7 +328,7 @@ func Test_GetCommand(t *testing.T) { engine := &mockEngine{allow: true} errGet := errors.New("get error") mock := teststore.NewMockStore(teststore.WithStoreGetErr(errGet)) - cmd := mustGetCommand(t, WithTimeout(time.Second), WithSocketPath(engine.serve(t))) + cmd := mustGetCommand(t, engineOpts(t, engine)...) out, err := execute(t, cmd, mock, "--reveal", "foo") assert.ErrorIs(t, err, errGet) assert.Equal(t, "Error: "+errGet.Error()+"\n", out) @@ -328,6 +362,23 @@ func mustGetCommand(t *testing.T, options ...ClientOption) *cobra.Command { return cmd } +func mustRmCommand(t *testing.T, options ...ClientOption) *cobra.Command { + t.Helper() + cmd, err := RmCommand(options...) + require.NoError(t, err) + return cmd +} + +func deadSocket(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "dead.sock") +} + +func engineOpts(t *testing.T, engine *mockEngine) []ClientOption { + t.Helper() + return []ClientOption{WithTimeout(time.Second), WithSocketPath(engine.serve(t))} +} + // execute runs cmd as if it were the root command: it attaches mock to the // command context so RunE bodies can pull it via StoreFrom, captures stdout // and stderr into one buffer (mirroring how cobra collapses both onto the diff --git a/plugins/pass/commands/get.go b/plugins/pass/commands/get.go index d77c8950..92e80d70 100644 --- a/plugins/pass/commands/get.go +++ b/plugins/pass/commands/get.go @@ -15,7 +15,6 @@ package commands import ( - "context" _ "embed" "errors" "fmt" @@ -26,7 +25,6 @@ import ( pass "github.com/docker/secrets-engine/plugins/pass/store" "github.com/docker/secrets-engine/store" - "github.com/docker/secrets-engine/x/secrets" ) //go:embed get_example.md @@ -69,7 +67,7 @@ func GetCommand(options ...ClientOption) (*cobra.Command, error) { if !reveal { return printSecret(cmd.OutOrStdout(), id, []byte(maskedValue)) } - if err := authorizeReveal(cmd.Context(), clientOpts, id); err != nil { + if err := authorizeAccess(cmd.Context(), clientOpts, id); err != nil { return err } value, err := pv.Marshal() @@ -84,23 +82,6 @@ func GetCommand(options ...ClientOption) (*cobra.Command, error) { return wrapKeychainErrors(cmd), nil } -func authorizeReveal(ctx context.Context, opts clientOpts, id store.ID) error { - pattern, err := secrets.ParsePattern(id.String()) - if err != nil { - return err - } - c, err := newClient(opts) - if err != nil { - return err - } - if opts.isUnbound() { - if err := preflightPing(ctx, c, defaultPreflightPingTimeout); err != nil { - return err - } - } - return authorize(ctx, c, pattern) -} - func printSecret(w io.Writer, id store.ID, value []byte) error { _, err := fmt.Fprintf(w, "ID: %s\nValue: %s\n", id, value) return err diff --git a/plugins/pass/commands/keychain_test.go b/plugins/pass/commands/keychain_test.go index 26e472bf..28d09969 100644 --- a/plugins/pass/commands/keychain_test.go +++ b/plugins/pass/commands/keychain_test.go @@ -101,7 +101,7 @@ func TestStoreCommandsHintOnLockedKeychain(t *testing.T) { {"get", mustGetCommand(t), teststore.NewMockStore(teststore.WithStoreGetErr(locked)), []string{"foo"}}, {"set", SetCommand(), teststore.NewMockStore(teststore.WithStoreSaveErr(locked)), []string{"foo=bar"}}, {"ls", ListCommand(), teststore.NewMockStore(teststore.WithStoreGetAllErr(locked)), nil}, - {"rm", RmCommand(), teststore.NewMockStore( + {"rm", mustRmCommand(t, engineOpts(t, &mockEngine{allow: true})...), teststore.NewMockStore( teststore.WithStore(map[store.ID]store.Secret{store.MustParseID("foo"): pass.NewPassValue([]byte("bar"))}), teststore.WithStoreDeleteErr(locked), ), []string{"foo"}}, diff --git a/plugins/pass/commands/rm.go b/plugins/pass/commands/rm.go index 09747186..fd91119a 100644 --- a/plugins/pass/commands/rm.go +++ b/plugins/pass/commands/rm.go @@ -31,17 +31,25 @@ import ( //go:embed rm_example.md var rmExample string +//go:embed rm_long.md +var rmLong string + type rmOpts struct { - All bool + clientOpts + all bool } -func RmCommand() *cobra.Command { - opts := rmOpts{} +func RmCommand(options ...ClientOption) (*cobra.Command, error) { + copts, err := parseClientOptions(options...) + if err != nil { + return nil, err + } + opts := rmOpts{clientOpts: copts} cmd := &cobra.Command{ Use: "rm name1 name2 ...", Aliases: []string{"delete", "erase", "remove"}, Short: "Remove secrets from local keychain.", - Long: "Removes one or more named secrets from the local OS keychain. Use `--all` to remove every stored secret at once.", + Long: strings.Trim(rmLong, "\n"), Example: strings.Trim(rmExample, "\n"), RunE: func(cmd *cobra.Command, args []string) error { idList, err := validateArgs(args, opts) @@ -56,12 +64,12 @@ func RmCommand() *cobra.Command { }, } flags := cmd.Flags() - flags.BoolVar(&opts.All, "all", false, "Remove all secrets") - return wrapKeychainErrors(cmd) + flags.BoolVar(&opts.all, "all", false, "Remove all secrets") + return wrapKeychainErrors(cmd), nil } func validateArgs(args []string, opts rmOpts) ([]store.ID, error) { - if (len(args) == 0 && !opts.All) || (len(args) > 0 && opts.All) { + if (len(args) == 0 && !opts.all) || (len(args) > 0 && opts.all) { return nil, fmt.Errorf("either provide a secret name or use --all to remove all secrets") } var result []store.ID @@ -80,19 +88,29 @@ func runRm(ctx context.Context, out io.Writer, kc store.Store, idList []store.ID if err != nil { return err } - if opts.All && len(idList) == 0 { + if opts.all && len(idList) == 0 { for k := range existing { idList = append(idList, k) } } slices.SortFunc(idList, func(a, b store.ID) int { return strings.Compare(a.String(), b.String()) }) var errs []error + var toDelete []store.ID for _, id := range idList { if _, ok := existing[id]; !ok { errs = append(errs, fmt.Errorf("%s: %w", id, store.ErrCredentialNotFound)) fmt.Fprintf(out, "ERR: %s: %s\n", id, store.ErrCredentialNotFound) continue } + toDelete = append(toDelete, id) + } + if len(toDelete) == 0 { + return errors.Join(errs...) + } + if err := authorizeAccess(ctx, opts.clientOpts, toDelete...); err != nil { + return errors.Join(append(errs, err)...) + } + for _, id := range toDelete { if err := kc.Delete(ctx, id); err != nil { errs = append(errs, err) fmt.Fprintf(out, "ERR: %s: %s\n", id, err) diff --git a/plugins/pass/commands/rm_long.md b/plugins/pass/commands/rm_long.md new file mode 100644 index 00000000..e846f59e --- /dev/null +++ b/plugins/pass/commands/rm_long.md @@ -0,0 +1,5 @@ +Removes one or more named secrets from the local OS keychain. Use `--all` to +remove every stored secret at once. + +The secrets engine must authorize the removal first and may prompt you. Denied +access removes nothing and fails with "access denied".