diff --git a/client/internal/certproof/helper_run.go b/client/internal/certproof/helper_run.go index 9345b50f3..cc221669b 100644 --- a/client/internal/certproof/helper_run.go +++ b/client/internal/certproof/helper_run.go @@ -7,6 +7,7 @@ import ( "fmt" "os/exec" "strings" + "time" log "github.com/sirupsen/logrus" @@ -16,6 +17,13 @@ import ( const ( maxHelperStdout = 1 << 20 maxHelperStderr = 4 << 10 + + // helperWaitDelay bounds how long Wait keeps reading the helper's output after the + // process we launched is gone. Killing that process does not close the pipe its own + // children inherited, and on macOS they are the ones doing the work: we launch + // launchctl, which launches sudo, which launches the helper. Without this, a helper + // stuck on a keychain prompt leaves Wait blocked with no deadline at all. + helperWaitDelay = time.Second ) var errHelperOutputTooLarge = errors.New("helper output exceeds the size limit") @@ -34,6 +42,7 @@ func runHelperCmd(cmd *exec.Cmd, req HelperRequest) ([]certposture.Proof, error) cmd.Stdin = bytes.NewReader(payload) cmd.Stdout = stdout cmd.Stderr = stderr + cmd.WaitDelay = helperWaitDelay if err := cmd.Run(); err != nil { return nil, fmt.Errorf("%w: %s", err, strings.TrimSpace(stderr.String())) diff --git a/client/internal/certproof/helper_run_test.go b/client/internal/certproof/helper_run_test.go index be4de9cbf..8a893f903 100644 --- a/client/internal/certproof/helper_run_test.go +++ b/client/internal/certproof/helper_run_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -64,6 +65,30 @@ func TestRunHelperCmd_CapsStderrInError(t *testing.T) { assert.True(t, strings.Contains(err.Error(), "exit status 3"), "the exit status is kept: %v", err) } +func TestRunHelperCmd_ReturnsWhenAGrandchildHoldsTheOutputPipe(t *testing.T) { + req := HelperRequest{Challenges: []HelperChallenge{{Nonce: []byte("asked")}}} + resp := HelperResponse{Proofs: []certposture.Proof{{Nonce: []byte("asked"), Signature: []byte("sig")}}} + + // The helper answers and exits, but leaves a background process holding the stdout + // it inherited. This is what a wedged `netbird posture cert-proof` behind a keychain + // prompt looks like from here: killing the process we launched does not close the + // pipe, so the copy out of it never sees EOF. + script := printJSON(t, resp) + "; sleep 10 &" + + done := make(chan error, 1) + go func() { + _, err := runHelperCmd(fakeHelper(t, script), req) + done <- err + }() + + select { + case err := <-done: + assert.ErrorIs(t, err, exec.ErrWaitDelay, "the output is incomplete, so the run must fail rather than report proofs") + case <-time.After(5 * time.Second): + t.Fatal("runHelperCmd never returned while a grandchild held the output pipe, so the collector's busy latch would stay set for the life of the daemon") + } +} + func TestRunHelperCmd_RejectsGarbage(t *testing.T) { req := HelperRequest{Challenges: []HelperChallenge{{Nonce: []byte("asked")}}}