Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/pass/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func Root(ctx context.Context, s store.Store, info commands.VersionInfo) *cobra.
cmd.AddCommand(wrapRunEWithSpan(commands.ListCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.RmCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.GetCommand(s)))
cmd.AddCommand(wrapRunEWithSpan(commands.RunCommand()))
cmd.AddCommand(wrapRunEWithSpan(commands.RunCommand(s)))
cmd.AddCommand(commands.VersionCommand(info))

return cmd
Expand Down
63 changes: 46 additions & 17 deletions plugins/pass/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/spf13/cobra"

"github.com/docker/secrets-engine/client"
"github.com/docker/secrets-engine/store"
"github.com/docker/secrets-engine/x/api"
"github.com/docker/secrets-engine/x/secrets"
)
Expand All @@ -52,24 +53,20 @@ func (e *ExitCodeError) Error() string {
//go:embed run_example.md
var runExample string

//go:embed run_long.md
var runLong string

type runOpts struct {
envFiles []string
envFiles []string
osKeychain bool
}

func RunCommand() *cobra.Command {
func RunCommand(s store.Store) *cobra.Command {
opts := runOpts{}
cmd := &cobra.Command{
Use: "run -- CMD [ARGS...]",
Short: "Run a command with `se://` environment references resolved.",
Long: "Scans the current environment (plus any `--env-file` inputs) for variables\n" +
"whose value is exactly `se://<ID|pattern>`. Each reference is resolved through the\n" +
"secrets-engine daemon and the resolved value is passed to the child process.\n" +
"The child inherits stdin, stdout, and stderr.\n" +
"\n" +
"Requires the secrets-engine daemon (Docker Desktop) to be running.\n" +
"\n" +
"If any reference cannot be resolved, the command fails before the child is\n" +
"started and exits non-zero.",
Use: "run -- CMD [ARGS...]",
Short: "Run a command with `se://` environment references resolved.",
Long: strings.Trim(runLong, "\n"),
Example: strings.Trim(runExample, "\n"),
Args: cobra.MinimumNArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -78,12 +75,18 @@ func RunCommand() *cobra.Command {
return err
}

c, err := client.New(client.WithSocketPath(api.DefaultSocketPath()))
if err != nil {
return err
var r secrets.Resolver
if opts.osKeychain {
r = &storeResolver{s: s}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Missing nil guard: --os-keychain with nil store panics at runtime

RunCommand now accepts a store.Store parameter. When --os-keychain is set, &storeResolver{s: s} is constructed and r.s.Filter(ctx, p) is immediately called (line 155). If s is nil, this is a nil-interface dereference → runtime panic.

In the normal production path (Root() → RunCommand(s)), s is always a real store from NewPassPlugin, so this is not reachable there. However, the tests already call RunCommand(nil) — if any future test adds --os-keychain it will panic rather than fail gracefully. A defensive early return would harden this:

if opts.osKeychain && s == nil {
    return fmt.Errorf("--os-keychain requires a configured store (s is nil)")
}

Consider adding this guard at line 79, before constructing storeResolver.

} else {
c, err := client.New(client.WithSocketPath(api.DefaultSocketPath()))
if err != nil {
return err
}
r = c
}

env, err := resolveEnv(cmd.Context(), c, merged)
env, err := resolveEnv(cmd.Context(), r, merged)
if err != nil {
return err
}
Expand Down Expand Up @@ -139,9 +142,35 @@ func RunCommand() *cobra.Command {
}
cmd.Flags().StringArrayVar(&opts.envFiles, "env-file", nil,
"Read environment variables from a dotenv-formatted file. Repeatable; later files override earlier files and the process environment.")
cmd.Flags().BoolVar(&opts.osKeychain, "os-keychain", false,
"Resolve `se://` references directly from the local OS keychain instead of the secrets-engine daemon.")
return cmd
}

type storeResolver struct {
s store.Store
}

func (r *storeResolver) GetSecrets(ctx context.Context, p secrets.Pattern) ([]secrets.Envelope, error) {
m, err := r.s.Filter(ctx, p)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] store.Filter may signal 'not found' inconsistently between error vs empty map

In storeResolver.GetSecrets, if store.Filter returns store.ErrCredentialNotFound (= secrets.ErrNotFound) as an error, it is propagated directly. But if Filter returns an empty map with nil error, the returned slice is empty and the caller (resolveRef) detects ErrNotFound from the empty result.

Both paths ultimately trigger ErrNotFound in the caller (via errors.Is wrapping), so correctness is preserved. However, the error message path differs: the error-return path will say "resolving $KEY: secret not found" while the empty-slice path generates a separate ErrNotFound. If store.Filter implementations diverge on this contract, the UX of --os-keychain will be inconsistent.

Consider normalising in GetSecrets:

if errors.Is(err, store.ErrCredentialNotFound) {
    return nil, nil  // let the caller's empty-slice ErrNotFound path handle it
}

if err != nil {
return nil, err
}
out := make([]secrets.Envelope, 0, len(m))
for id, sec := range m {
val, err := sec.Marshal()
if err != nil {
return nil, err
}
out = append(out, secrets.Envelope{
ID: id,
Value: val,
Metadata: sec.Metadata(),
})
}
return out, nil
}

// mergeEnv folds the process environment and any --env-file inputs into a
// single deterministic KEY=VALUE slice. Precedence: process env first, then
// each file in order; later entries override earlier ones.
Expand Down
8 changes: 8 additions & 0 deletions plugins/pass/commands/run_example.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,11 @@ $ docker pass run --env-file .env -- ./my-binary
```console
$ docker pass run --env-file .env --env-file .env.local -- ./my-binary
```

### Resolve directly from the local OS keychain (skip the daemon):

```console
$ SE_TOKEN=se://gh-token docker pass run --os-keychain -- gh repo list
```

References are read from the same store used by `docker pass set`/`docker pass get`, so the secrets-engine daemon does not need to be running.
10 changes: 10 additions & 0 deletions plugins/pass/commands/run_long.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Scans the current environment (plus any `--env-file` inputs) for variables
whose value is exactly `se://<ID|pattern>` and resolves each reference before
launching the child process. The child inherits stdin, stdout, and stderr.

By default, references are resolved through the secrets-engine daemon (Docker
Desktop must be running). Pass `--os-keychain` to resolve directly from the
local OS keychain instead, with no daemon required.

If any reference cannot be resolved, the command fails before the child is
started and exits non-zero.
4 changes: 2 additions & 2 deletions plugins/pass/commands/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func runAsWrapper() {
if err != nil {
os.Exit(2)
}
cmd := RunCommand()
cmd := RunCommand(nil)
cmd.SetArgs([]string{exe})
cmd.SetContext(context.Background())
cmd.SilenceUsage = true
Expand Down Expand Up @@ -241,7 +241,7 @@ func TestRunCommand(t *testing.T) {
require.NoError(t, err)

t.Run("no command given returns arg error", func(t *testing.T) {
cmd := RunCommand()
cmd := RunCommand(nil)
cmd.SetArgs([]string{})
cmd.SetContext(t.Context())
cmd.SetOut(testWriter{t})
Expand Down
Loading