Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward spiffe-helper's stdin to the 'cmd' invoked in daemon_mode #245

Merged
merged 3 commits into from
Feb 9, 2025

Conversation

ringerc
Copy link
Contributor

@ringerc ringerc commented Jan 31, 2025

Presently it's difficult to use spiffe-helper to wrap another command, because:

  • it doesn't forward its stdin to the command it runs
  • it doesn't write the child command's pid anywhere, nor does it forward signals, so there's no easy way to reliably identify and signal the wrapped process
  • it uses a stringified argument list when invoking the child process, which is difficult to reliably and correctly escape for all argument variations

(See related: #243, #244)

This PR makes one step toward making it more practical to use the helper as a wrapper command, by forwarding stdin to the cmd invoked.

Other improvements are needed to make wrapper use reliable, including

  • a way to run the cmd one-shot and exit with the cmd's exit code when the cmd exits; and
  • safer argument handling that preserves the original argument vector by using a HCL array of args not a string and supports CLI invocation like spiffe-helper -c some_config.hcl -- my_cmd "my argument" "here" too.

(WIP for these changes can be seen in this commit: a696295, and notes in this comment: #245 (comment))

WARNING This PR introduces a corner case behaviour change.

If spiffe-helper is invoked in a context where consuming stdin will have an effect on the caller, and it runs a 'cmd' that can optionally consume from stdin but ignores it if stdin is closed, then this change will cause spiffe-helper invocations to consume from stdin that would otherwise go to the caller.

E.g. this contrived bash code

    echo -n $'a\nb\nc\nd\n' | {
      spiffe-helper -config some_config.hcl & ;
      spiffe_helper_pid=$! ;
      while read -r SOME_VAR ; do
        echo "Got SOME_VAR: ${SOME_VAR}" ;
      done
    }

would have previously echoed one line for each of a, b, c, and d. Now, if spiffe-helper's cmd configured in some_config.hcl consumes stdin if it's attached, it'll instead produce no output (and the unexpectedly connected stdin may confuse the cmd.)

This is an unlikely corner case so this change is being made unconditionally, not gated behind a feature flag or configuration option. If anyone is actually affected by this, they can close the stdin file descriptor they pass when invoking spiffe-helper, e.g. in bash run spiffe-helper <&-.

@ringerc ringerc marked this pull request as draft January 31, 2025 00:57
@ringerc ringerc changed the title Omnibus commit to be split up Enable spiffe-helper to be used as a wrapper command Jan 31, 2025
pkg/sidecar/config.go Outdated Show resolved Hide resolved
@ringerc
Copy link
Contributor Author

ringerc commented Feb 2, 2025

I'm increasingly leaning toward dropping the idea of using spiffe-helper as a passthrough at all, in favour of leaning harder on pid_file_name to signal an external process. For cases where that's not suitable, cmd can be used to call a cert-rotation helper script or command, using the existing undocumented behaviour that cmd only starts and restarts the command when a cert is rotated, not continually.

That way there's no need to try to make spiffe-helper into a general purpose daemon manager with signal and arbitrary FD passthrough, restart back-off, etc etc etc.

Given that, I simplified this PR to include only forwarding stdin.

I will raise an issue for improving support for pid_file_name based signalling and readiness detection to solve some issues with using it as a separate helper process.


If someone does want to further improve support for using spiffe-helper as a wrapper, the next step would likely be to remove the current Run() function and call RunDaemon() for all code paths. If daemon_mode is false, then when the watcher is notified of the first cert renewal:

  • if cmd is empty then the cert watcher would os.Exit(0) after the first cert renewal
  • if cmd is non-empty, spiffe-helper would run the cmd and wait for it to exit, then exit with the same exit status as the controlled process when it terminates. It could continue to watch for cert renewals while this cmd runs, and if renew_signal is not "" it could signal it to reload certs, in case of uses like long-running batch jobs.

daemon_mode=false with a cmd set would thus mean "run the command as a one-shot command, renewing certs if required while it runs, and return with the same exit status as that command".

Something like this could use code like this to cause spiffe-helper to exit with the same exit code as the cmd it was managing:

// Propagate the child process's exit code to the spiffe-helper process so that
// spiffe-helper can be used as a wrapper process. Only to be called when the child
// process has terminated.
func (s *Sidecar) exitWithChildProcessExitCode(pState *os.ProcessState) {
       // If the child process was terminated by a signal, exit with a
       // non-zero code. This is the same behavior as the bourne shell. The
       // downside is that exit statuses of >127 of the child process cannot
       // be differentiated from signal exits, but this is a limitation of the
       // unix process interface as there's no way for a process to exit with
       // a "signal exit" status without actually signalling itself to do so.
       if !pState.Exited() && runtime.GOOS != "windows" {
               if status, ok := pState.Sys().(syscall.WaitStatus); ok {
                       os.Exit(128 + int(status.Signal()))
               }
       }
       // Propagate the child process's exit code
       os.Exit(pState.ExitCode())
}

... though I recommend using a func (*os.ProcessStatus) wrapper in the Sidecar state to indirect the call to os.Exit so it can be trapped for unit testing, like this

type Sidecar struct {
   // ...
  exitFunc     func(*os.ProcessState, int)
}

// ...

func New(config *Config) *Sidecar {
  // ...
  sidecar := &Sidecar{
    // ...
    exitFunc: func(_ *os.ProcessState, code int) { os.Exit(code) },
  }
  //...
}

@ringerc ringerc marked this pull request as ready for review February 2, 2025 22:09
@ringerc ringerc changed the title Enable spiffe-helper to be used as a wrapper command Forward spiffe-helper's stdin to the 'cmd' invoked in daemon_mode Feb 2, 2025
spiffe-helper was attaching stdout and stderr to the child process
launched by 'cmd' but was not attaching stdin.

Attach stdin too, so it's possible to pass a pipeline of data to
`spiffe-helper` for `cmd` to consume. It can then be used in  pipeline
or as a co-process to communicate with a `cmd` that requires
spiffe-helper to manage certificates.

This presents a corner case behaviour change for callers of
spiffe-helper. If it is invoked in a context where consuming stdin will
have an effect on the caller, and it runs a 'cmd' that can optionally
consume from stdin but ignores it if stdin is closed, then this change
will cause spiffe-helper invocations to consume from stdin that would
otherwise go to the caller.

E.g. this contrived bash code

    echo -n $'a\nb\nc\nd\n' | {
      spiffe-helper -config some_config.hcl & ;
      spiffe_helper_pid=$! ;
      while read -r SOME_VAR ; do
        echo "Got SOME_VAR: ${SOME_VAR}" ;
      done
    }

would have previously echoed one line for each of a b c and d. Now, if
`spiffe-helper`'s `cmd` configured in `some_config.hcl` consumes stdin
if it's attached, it'll instead produce no output (and the unexpectedly
connected `stdin` may confuse the `cmd`.)

This is an unlikely corner case so this change is being made
unconditionally, not gated behind a feature flag or configuration
option. If anyone is actually affected by this, they can close the
`stdin` file descriptor they pass when invoking `spiffe-helper`, e.g. in
bash run `spiffe-helper <&-`.

Signed-off-by: Craig Ringer <[email protected]>
@ringerc
Copy link
Contributor Author

ringerc commented Feb 2, 2025

PR simplified and un-marked draft. Follow-up PRs can address the safety of argument splitting, and supporting one-shot execution with exit-code forwarding.

@faisal-memon faisal-memon merged commit 5d7b5d5 into spiffe:main Feb 9, 2025
13 checks passed
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.

2 participants