Skip to content

feat: pass run command#522

Open
joe0BAB wants to merge 2 commits into
mainfrom
feat/pass-run
Open

feat: pass run command#522
joe0BAB wants to merge 2 commits into
mainfrom
feat/pass-run

Conversation

@joe0BAB
Copy link
Copy Markdown
Collaborator

@joe0BAB joe0BAB commented May 21, 2026

Adds a new docker pass run -- CMD [ARGS...] subcommand to the pass plugin. It scans the current environment for variables whose value is se://NAME, resolves each reference via the secrets-engine daemon (requires Docker Desktop to be running), and execs the child with the resolved environment. Mirrors the workflow of op run from 1Password.

Only values that exactly equal se://NAME are resolved; embedded references pass through unchanged and wildcards are rejected. Unresolvable references fail before exec, so the child never starts with a half-resolved environment. The child's exit code is propagated.

SIGINT, SIGTERM, and SIGHUP delivered to pass run are forwarded to the child instead of killing both via the foreground process group, and pass run waits for the child to exit on its own terms. When the child is killed by a signal, pass run exits 128 + signum instead of -1. On Windows only SIGINT is forwarded.

joe0BAB added 2 commits May 21, 2026 12:05
Adds a new `docker pass run -- CMD [ARGS...]` subcommand that scans the
current environment for variables whose value is `se://NAME`, resolves
each reference through the secrets-engine daemon, and execs the child
process with the resolved environment. Mirrors the workflow of
`op run` from 1Password.

Behavior:
- Exact-match `se://NAME` values only; embedded references are passed
  through unchanged (deferred to a later milestone).
- Hard-fails before exec when any reference cannot be resolved.
- Forwards the child's exit code via `os.Exit`.

Includes PASS_RUN.md with the milestone roadmap toward full `op run`
parity.

Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
Replace `child.Run()` with explicit `Start`/`Wait` bracketed by a signal
forwarder. SIGINT, SIGTERM, and SIGHUP delivered to `pass run` are now
relayed to the child instead of killing both processes through the
foreground process group. The wrapper waits for the child to exit on
its own terms before returning, so trapped cleanup handlers in the
child run to completion.

When the child is killed by a signal, the exit code reported by
`pass run` is now `128 + signum` instead of `-1`, matching shell
convention.

Platform split for the signal set and exit-code translation:
SIGINT/SIGTERM/SIGHUP on Unix, SIGINT only on Windows.
@joe0BAB joe0BAB marked this pull request as ready for review May 22, 2026 11:46
@docker-agent
Copy link
Copy Markdown

docker-agent Bot commented May 22, 2026

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

@joe0BAB
Copy link
Copy Markdown
Collaborator Author

joe0BAB commented May 22, 2026

/review

Copy link
Copy Markdown

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🔴 CRITICAL

Note: Verification was inconclusive (verifier returned empty response); findings below are from the drafter with manual line-number validation. Two medium-severity bugs were found in newly added code.

Severity Finding Location
🔴 MEDIUM os.Exit() inside wrapRunEWithSpan drops OTel span & metrics run.go:105
🔴 MEDIUM Signal handler registered after child.Start() — child can be orphaned run.go:85

if waitErr != nil {
var exitErr *exec.ExitError
if errors.As(waitErr, &exitErr) {
os.Exit(childExitCode(exitErr.ProcessState))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

os.Exit() inside wrapRunEWithSpan bypasses OTel span.End() and metrics

command.go wraps every subcommand with wrapRunEWithSpanwithOTEL, which calls defer span.End() and calledMetric() after runE() returns. But when the child exits non-zero, run.go calls os.Exit(childExitCode(exitErr.ProcessState)) directly — bypassing all deferred functions and never returning to the withOTEL wrapper.

Result: every non-zero child exit (the most operationally common case) produces an unclosed OTel span and a missing metric event. Only the success path (child exits 0, RunE returns nil) is correctly instrumented.

Fix: return a sentinel error (or a dedicated exit-code error type) from RunE, let withOTEL complete normally, then call os.Exit in a PersistentPostRunE or via a top-level error handler that runs outside the span wrapper.

}

sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, forwardableSignals()...)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Signal handler registered after child.Start() — signals in the gap orphan the child

child.Start() is called at line 80, but signal.Notify(sigCh, forwardableSignals()...) is not called until line 85. If SIGINT, SIGTERM, or SIGHUP arrives in that window, Go's default handler terminates the parent immediately. The child process, already running, is reparented to init/PID 1 and continues executing indefinitely — a process and secret leak.

Fix: call signal.Notify before child.Start():

sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, forwardableSignals()...)
defer signal.Stop(sigCh)

if err := child.Start(); err != nil {
    return fmt.Errorf("starting child: %w", err)
}

This eliminates the race entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant