feat(pass)!: authorize before removing secrets

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...)
+}

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,15 +310,15 @@ 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)
 		assert.Equal(t, []string{"authorize foo"}, engine.recorded())
 	})
 	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

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

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"}},

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)

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".