diff --git a/CHANGELOG.md b/CHANGELOG.md index 254ac13..2c15d88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ repository still gets a decision, never by following the link; no release carrie - :lock: fix(cmd): discriminate absent provider declaration from forge failure (REL-03) - :lock: fix(release): pin cosign signer identity and issuer in install.sh (SEC-03) - :lock: fix(release): widen the cosign identity pin to the real signer casing (SEC-03) +- :lock: fix(provider): bound exec stdout, set WaitDelay, capture stderr ### Testing - :white_check_mark: test(cmd): assert the REL-03 error wrap as one contiguous substring diff --git a/internal/provider/transport.go b/internal/provider/transport.go index ed7b724..3cd1eac 100644 --- a/internal/provider/transport.go +++ b/internal/provider/transport.go @@ -140,9 +140,92 @@ func CallHTTP(ctx context.Context, url string, q FactQuery, timeout time.Duratio return readBounded(resp.Body, MaxResponseBytes) } +// maxStderrExcerptBytes bounds how much of an exec provider's stderr is kept to +// explain a failure (REL-07). It is an ERROR-LEGIBILITY bound, not a response +// bound: MaxResponseBytes above remains the single declared bound on the bytes a +// provider may answer with, and stderr never becomes an answer (see CallExec). +// The excerpt exists so an operator debugging a fail-closed REVIEW reads the +// provider's own diagnostic instead of a bare "exit status 1"; 4 KiB is a few +// dozen lines, past which more text stops helping and starts flooding CI logs. +const maxStderrExcerptBytes = 4 << 10 + +// boundedCapture is an io.Writer that accumulates a child process stream in +// memory under a HARD cap, and then applies the readBounded verdict to what it +// kept (AUD2-S01, finding REL-01). It exists because a plain bytes.Buffer as +// cmd.Stdout is unbounded: opts.Timeout bounds wall clock, not memory, so a +// runaway provider could exhaust the runner before any deadline fired. +// +// The cap and the verdict are deliberately the same object: removing the exec +// bound means deleting this type's use, not silently loosening one half of it +// while the other still looks correct. +type boundedCapture struct { + limit int64 // bytes ALLOWED through; limit+1 is refused + buf bytes.Buffer +} + +func newBoundedCapture(limit int64) *boundedCapture { return &boundedCapture{limit: limit} } + +// Write keeps at most limit+1 bytes — exactly what readBounded needs to tell +// "at the limit" (legitimate) from "over the limit" (refused) — and DISCARDS +// the rest. It reports a full write for the discarded remainder on purpose: an +// io.ErrShortWrite here would abort os/exec's copier and surface as a confusing +// I/O error instead of the limit error the caller must see. +func (c *boundedCapture) Write(p []byte) (int, error) { + n := len(p) + if room := c.limit + 1 - int64(c.buf.Len()); room > 0 { + if int64(n) > room { + p = p[:room] + } + c.buf.Write(p) // bytes.Buffer.Write never returns an error + } + return n, nil +} + +// overflowed reports whether the stream had more bytes to give than the cap. +func (c *boundedCapture) overflowed() bool { return int64(c.buf.Len()) > c.limit } + +// bytesOrError applies the shared bound semantics: at-limit is returned intact, +// over-limit is an error with NO bytes so nothing can parse a truncated +// document. Identical treatment to CallHTTP's response read, by construction. +func (c *boundedCapture) bytesOrError() ([]byte, error) { + return readBounded(bytes.NewReader(c.buf.Bytes()), c.limit) +} + +// excerpt renders the captured stream for an error message, marking truncation +// so a reader never mistakes a cut-off diagnostic for the whole story. +func (c *boundedCapture) excerpt() string { + raw := c.buf.Bytes() + truncated := c.overflowed() + if truncated { + raw = raw[:c.limit] + } + text := strings.TrimSpace(string(raw)) + if text == "" { + return "" + } + if truncated { + text += " …(truncated)" + } + return text +} + // CallExec runs an exec provider with the FactQuery on stdin, a scrubbed // environment/argv, and a verified digest pin. Refuses before spawn when the // pin is missing or does not match the binary bytes (REQ-E5-S03-02). +// +// Three containment properties, all fail-closed (AUD2-S01): +// - stdout is BOUNDED at MaxResponseBytes exactly as CallHTTP's body read is +// (REL-01) — an over-limit provider yields an error and NO bytes, never a +// truncated parse, and cannot grow the runner's heap without limit; +// - cmd.WaitDelay is the operator's own timeout (REL-02), so a provider that +// forks a background grandchild inheriting stdout cannot hold cmd.Run open +// past ~2x the deadline; the resulting exec.ErrWaitDelay is an error like +// any other and classifies as unavailable; +// - stderr is captured into its OWN bounded buffer and folded into the +// returned error (REL-07), so a failure explains itself. It is NEVER +// concatenated into the returned bytes: ResolveFacts parses stdout as the +// provider's answer, and mixing the streams would let a chatty provider +// corrupt a decision input. func CallExec(ctx context.Context, opts ExecOpts, q FactQuery) ([]byte, error) { if err := VerifyExecDigest(opts.Binary, opts.Digest); err != nil { return nil, err @@ -158,12 +241,29 @@ func CallExec(ctx context.Context, opts ExecOpts, q FactQuery) ([]byte, error) { cmd := exec.CommandContext(ctx, opts.Binary, args...) cmd.Env = ScrubEnv(opts.Env) cmd.Stdin = bytes.NewReader(body) - var out bytes.Buffer - cmd.Stdout = &out + stdout := newBoundedCapture(MaxResponseBytes) + stderr := newBoundedCapture(maxStderrExcerptBytes) + cmd.Stdout = stdout + cmd.Stderr = stderr + // The single operator-declared timeout is also the wait bound: killing the + // child does not close a pipe its grandchildren still hold, so without this + // Wait blocks indefinitely (REL-02). + cmd.WaitDelay = opts.Timeout if err := cmd.Run(); err != nil { - return out.Bytes(), err + return nil, execFailure(err, stderr) + } + return stdout.bytesOrError() +} + +// execFailure folds the provider's own stderr diagnostic into the run error, +// preserving the wrapped sentinel (exec.ErrWaitDelay, *exec.ExitError, …) so +// callers can still discriminate with errors.Is/As. +func execFailure(err error, stderr *boundedCapture) error { + excerpt := stderr.excerpt() + if excerpt == "" { + return err } - return out.Bytes(), nil + return fmt.Errorf("%w: provider stderr: %s", err, excerpt) } // FileDigestSHA256 returns the sha256: digest of the file at path. diff --git a/internal/provider/transport_internal_test.go b/internal/provider/transport_internal_test.go new file mode 100644 index 0000000..02ab262 --- /dev/null +++ b/internal/provider/transport_internal_test.go @@ -0,0 +1,99 @@ +package provider + +import ( + "bytes" + "strings" + "testing" +) + +// TestBoundedCaptureRetainsAtMostLimitPlusOne is the WHITE-BOX half of REL-01, +// and it is the only test that can see the finding as stated. +// +// REL-01 is *unbounded memory*, not "a missing error". The black-box exec tests +// in transport_test.go observe the error, which readBounded produces from the +// bytes that were kept — so they stay green even if the capture itself grows +// without limit and the runner OOMs before the verdict is ever reached. That is +// exactly how this finding survived three audits: the tests measured the wrong +// surface. This one measures retained bytes. +func TestBoundedCaptureRetainsAtMostLimitPlusOne(t *testing.T) { + c := newBoundedCapture(MaxResponseBytes) + + const chunks = 3 + chunk := bytes.Repeat([]byte("x"), MaxResponseBytes) // 3x the limit in total + for i := 0; i < chunks; i++ { + n, err := c.Write(chunk) + if err != nil { + // os/exec's copier aborts on a writer error and reports it instead of + // the limit error the caller must see, so a short write is a defect. + t.Fatalf("write %d: %v", i, err) + } + if n != len(chunk) { + t.Fatalf("write %d reported %d of %d bytes — a short write aborts os/exec's copier", i, n, len(chunk)) + } + } + + // limit+1 is the whole point: it is what readBounded needs to tell an + // at-limit response (legitimate) from an over-limit one (refused). + if got, want := int64(c.buf.Len()), int64(MaxResponseBytes)+1; got != want { + t.Fatalf("retained %d bytes after writing %d — the capture must hold exactly %d (the bound plus the one byte that proves it was exceeded)", + got, chunks*len(chunk), want) + } + + raw, err := c.bytesOrError() + if err == nil { + t.Fatal("an over-limit capture must fail closed") + } + if raw != nil { + t.Fatalf("an over-limit capture must yield no bytes, got %d", len(raw)) + } + if !c.overflowed() { + t.Fatal("overflowed() must report an over-limit stream") + } +} + +// TestBoundedCaptureAtLimitIsIntact pins the boundary from the inside: a stream +// of exactly the limit is legitimate traffic, returned byte-for-byte. +func TestBoundedCaptureAtLimitIsIntact(t *testing.T) { + c := newBoundedCapture(16) + if _, err := c.Write([]byte("0123456789abcdef")); err != nil { + t.Fatalf("write: %v", err) + } + raw, err := c.bytesOrError() + if err != nil { + t.Fatalf("a stream of exactly the limit is legitimate: %v", err) + } + if string(raw) != "0123456789abcdef" { + t.Fatalf("at-limit capture = %q, want it intact", raw) + } + if c.overflowed() { + t.Fatal("an at-limit stream has not overflowed") + } +} + +// TestBoundedCaptureExcerptTruncates covers the stderr side (REL-07): the +// diagnostic buffer is bounded too — REL-01 must not be reopened through the +// back door — and a cut-off excerpt says that it was cut off. +func TestBoundedCaptureExcerptTruncates(t *testing.T) { + c := newBoundedCapture(maxStderrExcerptBytes) + if _, err := c.Write(bytes.Repeat([]byte("N"), 4<<20)); err != nil { + t.Fatalf("write: %v", err) + } + if got := int64(c.buf.Len()); got != maxStderrExcerptBytes+1 { + t.Fatalf("stderr capture retained %d bytes — it must stay bounded at %d", got, maxStderrExcerptBytes+1) + } + excerpt := c.excerpt() + if !strings.Contains(excerpt, "truncated") { + t.Fatalf("a truncated excerpt must say so, got %d bytes", len(excerpt)) + } + if len(excerpt) > maxStderrExcerptBytes+64 { + t.Fatalf("excerpt is %d bytes, want at most the bound plus the marker", len(excerpt)) + } + + quiet := newBoundedCapture(maxStderrExcerptBytes) + if _, err := quiet.Write([]byte(" \n ")); err != nil { + t.Fatalf("write: %v", err) + } + if quiet.excerpt() != "" { + t.Fatalf("whitespace-only stderr must not decorate an error, got %q", quiet.excerpt()) + } +} diff --git a/internal/provider/transport_test.go b/internal/provider/transport_test.go index 9f3e696..a98abc1 100644 --- a/internal/provider/transport_test.go +++ b/internal/provider/transport_test.go @@ -1,11 +1,16 @@ package provider_test import ( + "bytes" "context" + "errors" "fmt" "io" "net/http" "net/http/httptest" + "os" + "os/exec" + "path/filepath" "strings" "testing" "time" @@ -182,3 +187,275 @@ func TestBoundedReadUnderLimitUnaffected(t *testing.T) { "legitimate provider payloads", provider.MaxResponseBytes) } } + +// --- AUD2-S01: exec transport trio (REL-01 bound stdout / REL-02 WaitDelay / +// --- REL-07 stderr capture). The three exec bound tests below deliberately +// --- mirror the three HTTP ones above: an exec provider and an HTTP provider +// --- answer the same FactQuery and feed the same resolver, so a containment +// --- asymmetry between them is the finding, not a design choice. + +// execStub writes `script` as an executable stub provider under t.TempDir() and +// returns ExecOpts pinned to its real digest (CallExec refuses to spawn an +// unpinned binary, REQ-E5-S03-02). A shell script is the provider "binary": +// nothing in these tests needs Go, and building four child binaries would cost +// more than the behaviour under test. +func execStub(t *testing.T, script string, timeout time.Duration) provider.ExecOpts { + t.Helper() + bin := filepath.Join(t.TempDir(), "stub-provider") + if err := os.WriteFile(bin, []byte(script), 0o600); err != nil { + t.Fatalf("write stub provider: %v", err) + } + // #nosec G302 -- a stub provider under t.TempDir() must be executable to be + // spawned at all; it is created by this test, not by an untrusted party. + if err := os.Chmod(bin, 0o700); err != nil { + t.Fatalf("chmod stub provider: %v", err) + } + digest, err := provider.FileDigestSHA256(bin) + if err != nil { + t.Fatalf("digest stub provider: %v", err) + } + return provider.ExecOpts{Binary: bin, Digest: digest, Timeout: timeout} +} + +// writeStubPayload writes `payload` next to the stub and returns a script that +// cats it verbatim — exact byte counts are produced in Go, where they can be +// computed, rather than in shell. +func writeStubPayload(t *testing.T, payload []byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), "payload") + if err := os.WriteFile(path, payload, 0o600); err != nil { + t.Fatalf("write stub payload: %v", err) + } + return "#!/bin/sh\nexec cat " + path + "\n" +} + +// TestExecBoundedStdoutOverLimitFailsClosed — REQ-AUD2-S01-01 / -07 (finding +// REL-01, byte-identical across three audits). Child stdout larger than +// provider.MaxResponseBytes is an ERROR with NO bytes, exactly as for HTTP: a +// runaway exec provider must not OOM the runner, and the prefix it did write +// must never be mistaken for a complete FactSet. Driven through ResolveFacts +// too, so an unwired bound fails the test. +func TestExecBoundedStdoutOverLimitFailsClosed(t *testing.T) { + q := isolationQuery() + // Syntactically OPEN JSON: a truncating read would still be handed to the + // decoder, so only the byte bound can produce the limit error. + payload := []byte(`{"facts":{"groups":{"state":"resolved","value":"`) + payload = append(payload, bytes.Repeat([]byte("x"), provider.MaxResponseBytes+1-len(payload))...) + opts := execStub(t, writeStubPayload(t, payload), execTestTimeout) + + raw, err := provider.CallExec(t.Context(), opts, q) + if err == nil { + t.Fatal("over-limit exec provider stdout must fail closed, got nil error") + } + if raw != nil { + t.Fatalf("over-limit exec read must return no bytes, got %d", len(raw)) + } + if !strings.Contains(err.Error(), "response body exceeds") { + t.Fatalf("error must name the byte limit, got %v", err) + } + if !strings.Contains(err.Error(), fmt.Sprint(provider.MaxResponseBytes)) { + t.Fatalf("error must state the limit %d, got %v", provider.MaxResponseBytes, err) + } + + call := func(ctx context.Context) ([]byte, error) { return provider.CallExec(ctx, opts, q) } + got := provider.ResolveFacts(t.Context(), call, q, fixedAsOf) + f, ok := got.Facts["groups"] + if !ok { + t.Fatal("fact key silently absent — must never happen") + } + if f.State != provider.StateUnavailable { + t.Fatalf("over-limit exec state = %q, want unavailable (reason: %q)", f.State, f.Reason) + } + if f.Value != nil { + t.Fatal("an over-limit exec response must never carry a value") + } + if got.AutoMergeEligible() { + t.Fatal("over-limit exec path must keep auto-merge disarmed") + } +} + +// TestExecBoundedStdoutAtLimitStillSucceeds — REQ-AUD2-S01-02. The BOUNDARY +// positive control, mirroring the HTTP one: stdout of EXACTLY the limit is +// legitimate traffic and must both read and resolve. Only this case kills the +// `>` → `>=` mutant. +func TestExecBoundedStdoutAtLimitStillSucceeds(t *testing.T) { + q := isolationQuery() + doc := resolvedResponseBytes(t, q, fixedAsOf.Add(time.Hour)) + pad := provider.MaxResponseBytes - len(doc) + if pad <= 0 { + t.Fatalf("MaxResponseBytes %d is too small for this fixture", provider.MaxResponseBytes) + } + // Pad with INSIGNIFICANT TRAILING WHITESPACE so the document stays decodable + // at the boundary and the at-limit response can be asserted to RESOLVE. + payload := append(doc, bytes.Repeat([]byte(" "), pad)...) + opts := execStub(t, writeStubPayload(t, payload), execTestTimeout) + + raw, err := provider.CallExec(t.Context(), opts, q) + if err != nil { + t.Fatalf("stdout of exactly the limit is legitimate traffic: %v", err) + } + if len(raw) != provider.MaxResponseBytes { + t.Fatalf("read %d bytes, want exactly the limit %d", len(raw), provider.MaxResponseBytes) + } + + call := func(ctx context.Context) ([]byte, error) { return provider.CallExec(ctx, opts, q) } + got := provider.ResolveFacts(t.Context(), call, q, fixedAsOf) + f, ok := got.Facts["groups"] + if !ok { + t.Fatal("fact key silently absent — must never happen") + } + if f.State != provider.StateResolved { + t.Fatalf("at-limit exec state = %q, want resolved (reason: %q)", f.State, f.Reason) + } +} + +// TestExecBoundedStdoutUnderLimitUnaffected is the POSITIVE CONTROL for the +// exec bound: without it, a limit of zero would pass the over-limit test. +func TestExecBoundedStdoutUnderLimitUnaffected(t *testing.T) { + q := isolationQuery() + opts := execStub(t, writeStubPayload(t, resolvedResponseBytes(t, q, fixedAsOf.Add(time.Hour))), execTestTimeout) + + raw, err := provider.CallExec(t.Context(), opts, q) + if err != nil { + t.Fatalf("a legitimate exec payload must be unaffected by the bound: %v", err) + } + if len(raw) == 0 { + t.Fatal("legitimate exec payload read returned no bytes") + } + if provider.MaxResponseBytes < 1<<20 { + t.Fatalf("MaxResponseBytes = %d — the bound must stay MB-order, generously above "+ + "legitimate provider payloads", provider.MaxResponseBytes) + } +} + +// TestExecWaitDelayBoundsOrphanedStdout — REQ-AUD2-S01-04 / -05 / -07 (finding +// REL-02). A provider that forks a background grandchild inheriting stdout and +// then exits leaves the stdout pipe open: without cmd.WaitDelay, cmd.Run blocks +// until every writer closes it — here 60 seconds, long past the operator's +// declared deadline, with no decision and no diagnostic. +// +// The assertion is wall-clock: the call must return in ~2x opts.Timeout, an +// order of magnitude below the grandchild's lifetime. Removing the WaitDelay +// assignment makes this test wait the full 60s and fail on the elapsed bound +// (it does not merely rely on `go test -timeout`). +func TestExecWaitDelayBoundsOrphanedStdout(t *testing.T) { + const ( + timeout = 2 * time.Second + grandkid = 60 * time.Second // how long the orphan holds the pipe + mustBeUnder = 30 * time.Second + ) + q := isolationQuery() + opts := execStub(t, "#!/bin/sh\nsleep 60 &\nprintf 'partial'\nexit 0\n", timeout) + + start := time.Now() + raw, err := provider.CallExec(t.Context(), opts, q) + elapsed := time.Since(start) + + if elapsed >= mustBeUnder { + t.Fatalf("CallExec took %s — a grandchild holding stdout for %s blocked the call past "+ + "its %s deadline (cmd.WaitDelay unset?)", elapsed, grandkid, timeout) + } + if err == nil { + t.Fatalf("a provider killed by the deadline/wait-delay must fail closed, got nil error (raw %q)", raw) + } + if raw != nil { + t.Fatalf("a wait-delay-killed provider must return no bytes, got %d", len(raw)) + } + // The exact sentinel depends on which timer fired first (process exit vs the + // context deadline) and a loaded machine can reorder them, so accept either + // documented shape — but never a success. + if !errors.Is(err, exec.ErrWaitDelay) && !errors.Is(err, context.DeadlineExceeded) && + !strings.Contains(err.Error(), "killed") { + t.Fatalf("unexpected error shape for a wait-delay/deadline kill: %v", err) + } + + call := func(ctx context.Context) ([]byte, error) { return provider.CallExec(ctx, opts, q) } + got := provider.ResolveFacts(t.Context(), call, q, fixedAsOf) + f, ok := got.Facts["groups"] + if !ok { + t.Fatal("fact key silently absent — must never happen") + } + if f.State != provider.StateUnavailable { + t.Fatalf("wait-delay state = %q, want unavailable (reason: %q)", f.State, f.Reason) + } + if got.AutoMergeEligible() { + t.Fatal("wait-delay path must keep auto-merge disarmed") + } +} + +// TestExecStderrFoldedIntoError — REQ-AUD2-S01-06 (finding REL-07). A failing +// provider used to yield a bare "exit status 1": nothing for the operator +// debugging a fail-closed REVIEW to read. Its stderr now reaches the error — +// and, per judgment call (c), NEVER the fact bytes: the resolver parses stdout +// as the provider's answer, so merging the streams would let a chatty provider +// corrupt a decision input. +func TestExecStderrFoldedIntoError(t *testing.T) { + const canary = "upstream-ldap-unreachable-canary" + q := isolationQuery() + + t.Run("nonzero_exit_reports_stderr", func(t *testing.T) { + opts := execStub(t, "#!/bin/sh\necho '"+canary+"' >&2\nexit 7\n", execTestTimeout) + raw, err := provider.CallExec(t.Context(), opts, q) + if err == nil { + t.Fatal("a provider exiting non-zero must fail closed") + } + if !strings.Contains(err.Error(), canary) { + t.Fatalf("error must carry the provider's stderr diagnostic, got %v", err) + } + if !strings.Contains(err.Error(), "exit status 7") { + t.Fatalf("error must still name the exit status, got %v", err) + } + if raw != nil { + t.Fatalf("a failed provider call must return no bytes, got %q", raw) + } + }) + + t.Run("stderr_never_merged_into_facts", func(t *testing.T) { + // Exit 0 with a VALID response on stdout and noise on stderr: the noise + // must not appear in the returned bytes (which would break decoding) and + // the fact must still resolve. Without this case the "streams are not + // merged" half of the requirement is vacuous, because the failure path + // returns nil bytes anyway. + doc := resolvedResponseBytes(t, q, fixedAsOf.Add(time.Hour)) + payload := filepath.Join(t.TempDir(), "resp.json") + if err := os.WriteFile(payload, doc, 0o600); err != nil { + t.Fatalf("write payload: %v", err) + } + opts := execStub(t, "#!/bin/sh\necho '"+canary+"' >&2\nexec cat "+payload+"\n", execTestTimeout) + + raw, err := provider.CallExec(t.Context(), opts, q) + if err != nil { + t.Fatalf("a chatty but successful provider must succeed: %v", err) + } + if strings.Contains(string(raw), canary) { + t.Fatalf("stderr leaked into the fact bytes: %q", raw) + } + call := func(ctx context.Context) ([]byte, error) { return provider.CallExec(ctx, opts, q) } + got := provider.ResolveFacts(t.Context(), call, q, fixedAsOf) + if f := got.Facts["groups"]; f.State != provider.StateResolved { + t.Fatalf("chatty provider state = %q, want resolved (reason: %q)", f.State, f.Reason) + } + }) + + t.Run("runaway_stderr_is_bounded_and_truncated", func(t *testing.T) { + // REL-01 must not be reopened through the back door: the stderr buffer is + // bounded too, so a provider spewing megabytes of diagnostics can neither + // exhaust memory nor produce an unreadable error. + noise := filepath.Join(t.TempDir(), "noise") + if err := os.WriteFile(noise, bytes.Repeat([]byte("N"), 4<<20), 0o600); err != nil { + t.Fatalf("write noise: %v", err) + } + opts := execStub(t, "#!/bin/sh\ncat "+noise+" >&2\nexit 3\n", execTestTimeout) + + _, err := provider.CallExec(t.Context(), opts, q) + if err == nil { + t.Fatal("a provider exiting non-zero must fail closed") + } + if len(err.Error()) > 64<<10 { + t.Fatalf("stderr capture is unbounded: error message is %d bytes", len(err.Error())) + } + if !strings.Contains(err.Error(), "truncated") { + t.Fatalf("a truncated stderr excerpt must say so, got %d bytes: %.200v", len(err.Error()), err) + } + }) +}