From d4349c6ba67f628b4d9b29393cca7028240d3088 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Mon, 5 Oct 2026 17:06:50 +0200 Subject: [PATCH] feat(pass)!: authorize before removing secrets rm asks the engine once for every secret it is about to delete and removes nothing when access is denied. RmCommand takes the same client options as get and run and returns an error. The Long description moves into rm_long.md, and get shares the new authorizeAccess helper. --- plugins/pass/commands/client.go | 21 +++++ plugins/pass/commands/command_test.go | 119 ++++++++++++++++++------- plugins/pass/commands/get.go | 21 +---- plugins/pass/commands/keychain_test.go | 2 +- plugins/pass/commands/rm.go | 34 +++++-- plugins/pass/commands/rm_long.md | 5 ++ 6 files changed, 139 insertions(+), 63 deletions(-) create mode 100644 plugins/pass/commands/rm_long.md 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".