From d4013b8624930dc39ecb3593c7d78cb232734236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20Gro=C3=9Fmann?= Date: Fri, 9 Oct 2026 14:16:27 +0200 Subject: [PATCH] feat(pass)!: authorize before overwriting secrets set --force asks the engine for the secret it is about to overwrite and stores nothing when access is denied. SetCommand takes the same client options as get, rm and run and returns an error. --- plugins/pass/commands/command_test.go | 76 ++++++++++++++++++++++---- plugins/pass/commands/keychain_test.go | 2 +- plugins/pass/commands/set.go | 27 +++++---- 3 files changed, 80 insertions(+), 25 deletions(-) diff --git a/plugins/pass/commands/command_test.go b/plugins/pass/commands/command_test.go index a503e839..66ebfafb 100644 --- a/plugins/pass/commands/command_test.go +++ b/plugins/pass/commands/command_test.go @@ -52,21 +52,21 @@ func Test_SetCommand(t *testing.T) { t.Parallel() t.Run("ok", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, SetCommand(), mock, "foo=bar=bar=bar") + out, err := execute(t, mustSetCommand(t), mock, "foo=bar=bar=bar") assert.NoError(t, err) assert.Empty(t, out) assertStoredValue(t, mock, "bar=bar=bar") }) t.Run("from STDIN", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := executeWithStdin(t, SetCommand(), mock, "my\nmultiline\nvalue", "foo") + out, err := executeWithStdin(t, mustSetCommand(t), mock, "my\nmultiline\nvalue", "foo") assert.NoError(t, err) assert.Empty(t, out) assertStoredValue(t, mock, "my\nmultiline\nvalue") }) t.Run("with --metadata flag", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := execute(t, SetCommand(), mock, "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01") + out, err := execute(t, mustSetCommand(t), mock, "foo=bar", "--metadata", "name=bob", "--metadata", "expiry=2027-03-01") assert.NoError(t, err) assert.Empty(t, out) assertStoredValue(t, mock, "bar") @@ -74,7 +74,7 @@ func Test_SetCommand(t *testing.T) { }) t.Run("from STDIN JSON with value and metadata", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := executeWithStdin(t, SetCommand(), mock, `{"secret":"bar","metadata":{"name":"bob"}}`, "foo") + out, err := executeWithStdin(t, mustSetCommand(t), mock, `{"secret":"bar","metadata":{"name":"bob"}}`, "foo") assert.NoError(t, err) assert.Empty(t, out) assertStoredValue(t, mock, "bar") @@ -82,7 +82,7 @@ func Test_SetCommand(t *testing.T) { }) t.Run("from STDIN JSON merged with --metadata flag wins on collision", func(t *testing.T) { mock := teststore.NewMockStore() - out, err := executeWithStdin(t, SetCommand(), mock, `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "foo", "--metadata", "name=alice") + out, err := executeWithStdin(t, mustSetCommand(t), mock, `{"secret":"bar","metadata":{"name":"bob","extra":"thing"}}`, "foo", "--metadata", "name=alice") assert.NoError(t, err) assert.Empty(t, out) assertStoredValue(t, mock, "bar") @@ -90,20 +90,20 @@ func Test_SetCommand(t *testing.T) { }) t.Run("invalid --metadata flag (no =)", func(t *testing.T) { mock := teststore.NewMockStore() - _, err := execute(t, SetCommand(), mock, "foo=bar", "--metadata", "invalid") + _, err := execute(t, mustSetCommand(t), mock, "foo=bar", "--metadata", "invalid") assert.ErrorContains(t, err, "invalid metadata pair (expected key=value): invalid") }) t.Run("store error", func(t *testing.T) { errSave := errors.New("save error") mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave)) - out, err := execute(t, SetCommand(), mock, "foo=bar") + out, err := execute(t, mustSetCommand(t), mock, "foo=bar") assert.ErrorIs(t, err, errSave) assert.Equal(t, "Error: "+errSave.Error()+"\n", out) }) t.Run("invalid id", func(t *testing.T) { errSave := errors.New("save error") mock := teststore.NewMockStore(teststore.WithStoreSaveErr(errSave)) - out, err := execute(t, SetCommand(), mock, "/foo=bar") + out, err := execute(t, mustSetCommand(t), mock, "/foo=bar") errInvalidID := secrets.ErrInvalidID{ID: "/foo"} assert.ErrorIs(t, err, errInvalidID) assert.Equal(t, "Error: "+errInvalidID.Error()+"\n", out) @@ -112,12 +112,20 @@ func Test_SetCommand(t *testing.T) { mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ store.MustParseID("foo"): pass.NewPassValue([]byte("old")), })) - out, err := execute(t, SetCommand(), mock, "foo=new") + out, err := execute(t, mustSetCommand(t), mock, "foo=new") assert.ErrorIs(t, err, keychain.ErrDuplicateItem) assert.Equal(t, "Error: keychain item already exists\n\n"+duplicateItemHint+"\n", out) assertStoredValue(t, mock, "old") }) - t.Run("--force overwrites existing secret", func(t *testing.T) { + t.Run("without --force needs no engine", func(t *testing.T) { + mock := teststore.NewMockStore() + out, err := execute(t, mustSetCommand(t, WithSocketPath(deadSocket(t))), mock, "foo=bar") + assert.NoError(t, err) + assert.Empty(t, out) + assertStoredValue(t, mock, "bar") + }) + t.Run("--force overwrites existing secret once the engine allows", func(t *testing.T) { + engine := &mockEngine{allow: true} // Make Save return an error so the test fails if --force does not // route the call through Upsert. mock := teststore.NewMockStore( @@ -126,18 +134,55 @@ func Test_SetCommand(t *testing.T) { }), teststore.WithStoreSaveErr(errors.New("save should not be called when --force is set")), ) - out, err := execute(t, SetCommand(), mock, "foo=new", "--force") + out, err := execute(t, mustSetCommand(t, engineOpts(t, engine)...), mock, "foo=new", "--force") assert.NoError(t, err) assert.Empty(t, out) + assert.Equal(t, []string{"authorize foo"}, engine.recorded()) assertStoredValue(t, mock, "new") }) + t.Run("--force fails when the engine denies", func(t *testing.T) { + engine := &mockEngine{} + mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ + store.MustParseID("foo"): pass.NewPassValue([]byte("old")), + })) + out, err := execute(t, mustSetCommand(t, engineOpts(t, engine)...), mock, "foo=new", "--force") + assert.ErrorIs(t, err, client.ErrAccessDenied) + assert.Equal(t, "Error: authorizing: access denied\n", out) + assert.Equal(t, []string{"authorize foo"}, engine.recorded()) + assertStoredValue(t, mock, "old") + }) + t.Run("--force fails when the engine is unreachable", func(t *testing.T) { + mock := teststore.NewMockStore(teststore.WithStore(map[store.ID]store.Secret{ + store.MustParseID("foo"): pass.NewPassValue([]byte("old")), + })) + socket := deadSocket(t) + cmd := mustSetCommand(t, WithTimeout(time.Second), WithSocketPath(socket)) + out, err := execute(t, cmd, mock, "foo=new", "--force") + assert.ErrorIs(t, err, client.ErrSecretsEngineNotRunning) + assert.ErrorContains(t, err, "authorizing:") + assert.Contains(t, out, "Start the secrets engine and retry: nothing is listening on "+strconv.Quote(socket)+".") + assertStoredValue(t, mock, "old") + }) + t.Run("--force validates the input before asking the engine", func(t *testing.T) { + mock := teststore.NewMockStore() + cmd := mustSetCommand(t, WithTimeout(time.Second), WithSocketPath(deadSocket(t))) + _, err := execute(t, cmd, mock, "foo=bar", "--force", "--metadata", "invalid") + assert.ErrorContains(t, err, "invalid metadata pair (expected key=value): invalid") + assert.NotErrorIs(t, err, client.ErrSecretsEngineNotRunning) + }) t.Run("--force surfaces upsert error", func(t *testing.T) { errUpsert := errors.New("upsert error") mock := teststore.NewMockStore(teststore.WithStoreUpsertErr(errUpsert)) - out, err := execute(t, SetCommand(), mock, "foo=bar", "--force") + cmd := mustSetCommand(t, engineOpts(t, &mockEngine{allow: true})...) + out, err := execute(t, cmd, mock, "foo=bar", "--force") assert.ErrorIs(t, err, errUpsert) assert.Equal(t, "Error: "+errUpsert.Error()+"\n", out) }) + t.Run("rejects an invalid option", func(t *testing.T) { + cmd, err := SetCommand(WithTimeout(-time.Second)) + require.EqualError(t, err, "request timeout duration cannot be negative") + assert.Nil(t, cmd) + }) } func Test_ListCommand(t *testing.T) { @@ -362,6 +407,13 @@ func Test_GetCommand(t *testing.T) { }) } +func mustSetCommand(t *testing.T, options ...ClientOption) *cobra.Command { + t.Helper() + cmd, err := SetCommand(options...) + require.NoError(t, err) + return cmd +} + func mustGetCommand(t *testing.T, options ...ClientOption) *cobra.Command { t.Helper() cmd, err := GetCommand(options...) diff --git a/plugins/pass/commands/keychain_test.go b/plugins/pass/commands/keychain_test.go index 28d09969..60bbd875 100644 --- a/plugins/pass/commands/keychain_test.go +++ b/plugins/pass/commands/keychain_test.go @@ -99,7 +99,7 @@ func TestStoreCommandsHintOnLockedKeychain(t *testing.T) { args []string }{ {"get", mustGetCommand(t), teststore.NewMockStore(teststore.WithStoreGetErr(locked)), []string{"foo"}}, - {"set", SetCommand(), teststore.NewMockStore(teststore.WithStoreSaveErr(locked)), []string{"foo=bar"}}, + {"set", mustSetCommand(t), teststore.NewMockStore(teststore.WithStoreSaveErr(locked)), []string{"foo=bar"}}, {"ls", ListCommand(), teststore.NewMockStore(teststore.WithStoreGetAllErr(locked)), nil}, {"rm", mustRmCommand(t, engineOpts(t, &mockEngine{allow: true})...), teststore.NewMockStore( teststore.WithStore(map[store.ID]store.Secret{store.MustParseID("foo"): pass.NewPassValue([]byte("bar"))}), diff --git a/plugins/pass/commands/set.go b/plugins/pass/commands/set.go index 6b00c1ad..753eaa0a 100644 --- a/plugins/pass/commands/set.go +++ b/plugins/pass/commands/set.go @@ -37,18 +37,18 @@ var setExample string //go:embed set_long.md var setLong string -type setOpts struct { - metadata []string // raw "key=value" strings from --metadata flag - force bool // if true, overwrite existing setPayload instead of erroring -} - type stdinPayload struct { Secret string `json:"secret"` Metadata map[string]string `json:"metadata,omitempty"` } -func SetCommand() *cobra.Command { - opts := setOpts{} +func SetCommand(options ...ClientOption) (*cobra.Command, error) { + copts, err := parseClientOptions(options...) + if err != nil { + return nil, err + } + metadata := []string{} + var force bool cmd := &cobra.Command{ Use: "set id[=value]", Aliases: []string{"store", "save"}, @@ -66,7 +66,7 @@ func SetCommand() *cobra.Command { if err != nil { return err } - flagMeta, err := parseMetadataFlags(opts.metadata) + flagMeta, err := parseMetadataFlags(metadata) if err != nil { return err } @@ -104,16 +104,19 @@ func SetCommand() *cobra.Command { return err } } - if opts.force { + if force { + if err := authorizeAccess(cmd.Context(), copts, id); err != nil { + return err + } return kc.Upsert(cmd.Context(), id, pv) } return kc.Save(cmd.Context(), id, pv) }, } flags := cmd.Flags() - flags.StringArrayVar(&opts.metadata, "metadata", nil, "Non-sensitive key=value metadata (repeatable)") - flags.BoolVarP(&opts.force, "force", "f", false, "Overwrite existing secret if it already exists") - return wrapKeychainErrors(cmd) + flags.StringArrayVar(&metadata, "metadata", nil, "Non-sensitive key=value metadata (repeatable)") + flags.BoolVarP(&force, "force", "f", false, "Overwrite existing secret if it already exists") + return wrapEngineErrors(wrapKeychainErrors(cmd)), nil } func parseMetadataFlags(raw []string) (map[string]string, error) {