diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go index f9f025c..ac5444c 100644 --- a/pkg/mcp/cloud/allowlist.go +++ b/pkg/mcp/cloud/allowlist.go @@ -19,11 +19,10 @@ var defaultCommandsJSON []byte // Command is one entry in the command allowlist. Path is the normalized // subcommand path the allowlist matches against (for example "projects list" or // "compute firewall-rules list"). Description carries the investigative axis the -// command serves (prose only). Redact marks output that needs secret-scrubbing. +// command serves (prose only). type Command struct { Path string `json:"path"` Description string `json:"description,omitempty"` - Redact bool `json:"redact,omitempty"` } // CommandAllowlist is the decoded allowlist document: the positive set of diff --git a/pkg/mcp/cloud/harness.go b/pkg/mcp/cloud/harness.go index 8d11dc0..8c2b11d 100644 --- a/pkg/mcp/cloud/harness.go +++ b/pkg/mcp/cloud/harness.go @@ -1,7 +1,6 @@ package cloud import ( - "bytes" "context" "errors" "os/exec" @@ -11,40 +10,63 @@ import ( // the agent's context budget. Output beyond it is dropped and flagged. const defaultOutputLimit = 64 * 1024 +// limitedWriter retains at most limit bytes of everything written to it and +// records whether any write pushed it past that cap. It never grows past limit, +// so a command emitting an arbitrarily large response cannot consume unbounded +// memory: bytes past the cap are counted for the overflow flag and discarded. +type limitedWriter struct { + buf []byte + limit int + overflow bool +} + +// Write retains up to the remaining capacity in the buffer and discards the +// rest, flagging overflow whenever a write carries more bytes than the buffer +// can still hold. It always reports the full length written so the child +// process is never blocked on a short write. +func (w *limitedWriter) Write(p []byte) (int, error) { + room := w.limit - len(w.buf) + if len(p) > room { + w.overflow = true + } + if room > 0 { + take := len(p) + if take > room { + take = room + } + w.buf = append(w.buf, p[:take]...) + } + return len(p), nil +} + // execCLI runs binPath with argv via execve — no shell, ever. The argv tokens // reach the binary as literal arguments, so shell metacharacters are inert. The // subprocess runs with exactly the supplied env (never the parent environment, // so a poisoned PATH cannot redirect the binary and ambient secrets do not -// leak), closed stdin (no interactive prompt), and stdout capped at limit. A -// non-zero exit is a normal result carried in ExitCode, not a Go error; a Go -// error means the process could not be run at all. Stderr — where gcloud/aws -// write their error context — is captured alongside stdout and capped at the -// same limit, so a non-zero exit carries an explanation instead of an empty -// result. +// leak), closed stdin (no interactive prompt), and stdout/stderr captured +// through bounded writers that retain at most limit bytes each — the cap is +// effective during the run, so a command emitting a very large response can +// never buffer it all in memory. A non-zero exit is a normal result carried in +// ExitCode, not a Go error; a Go error means the process could not be run at +// all. Stderr — where gcloud/aws write their error context — is captured +// alongside stdout and capped at the same limit, so a non-zero exit carries an +// explanation instead of an empty result. func execCLI(ctx context.Context, binPath string, argv []string, env []string, limit int) (CLIResult, error) { cmd := exec.CommandContext(ctx, binPath, argv...) cmd.Env = env cmd.Stdin = nil - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr + stdout := &limitedWriter{limit: limit} + stderr := &limitedWriter{limit: limit} + cmd.Stdout = stdout + cmd.Stderr = stderr err := cmd.Run() - res := CLIResult{} - out := stdout.Bytes() - if len(out) > limit { - out = out[:limit] - res.Truncated = true - } - res.Stdout = string(out) - - errOut := stderr.Bytes() - if len(errOut) > limit { - errOut = errOut[:limit] - res.Truncated = true + res := CLIResult{ + Stdout: string(stdout.buf), + Stderr: string(stderr.buf), + Truncated: stdout.overflow || stderr.overflow, } - res.Stderr = string(errOut) if err != nil { var exitErr *exec.ExitError diff --git a/pkg/mcp/cloud/harness_test.go b/pkg/mcp/cloud/harness_test.go index 0d4861f..79a0f5b 100644 --- a/pkg/mcp/cloud/harness_test.go +++ b/pkg/mcp/cloud/harness_test.go @@ -41,3 +41,19 @@ func TestExecCLITruncatesStderr(t *testing.T) { require.NoError(t, err) assert.LessOrEqual(t, len(r.Stderr), 10, "stderr exceeded limit") } + +// TestExecCLICapsLargeOutputWithoutBuffering drives a payload orders of +// magnitude past the limit through a shell-free command (head reading 8MB from +// /dev/zero) and asserts the captured stdout is capped at the limit with +// Truncated set, so a command emitting a very large response cannot retain +// unbounded bytes in memory. The cap is effective during the run, not a +// post-hoc slice of a fully buffered output. +func TestExecCLICapsLargeOutputWithoutBuffering(t *testing.T) { + t.Parallel() + const limit = 1024 + r, err := execCLI(context.Background(), "/usr/bin/head", + []string{"-c", "8388608", "/dev/zero"}, nil, limit) + require.NoError(t, err) + assert.True(t, r.Truncated, "an output far larger than limit must be flagged truncated") + assert.LessOrEqual(t, len(r.Stdout), limit, "captured stdout must be capped at limit, not the full 8MB payload") +} diff --git a/pkg/mcp/cloud/probe.go b/pkg/mcp/cloud/probe.go index 6a90fd3..38f65a8 100644 --- a/pkg/mcp/cloud/probe.go +++ b/pkg/mcp/cloud/probe.go @@ -30,9 +30,10 @@ func Probe(ctx context.Context, p Provider, expected string, env []string) (Iden st, err := p.Identity(ctx, run, expected) if err != nil { return IdentityStatus{ - Provider: p.Name(), - Valid: false, - Hint: err.Error(), + Provider: p.Name(), + AssumedIdentity: expected, + Valid: false, + Hint: err.Error(), }, nil } if st.Provider == "" { @@ -40,8 +41,11 @@ func Probe(ctx context.Context, p Provider, expected string, env []string) (Iden } if st.AssumedIdentity == "" { // A whoami that resolved no identity is not a valid session, whatever - // the provider reported. + // the provider reported. Report the pinned identity so the degraded + // session names which credential the operator must fix instead of an + // empty one. st.Valid = false + st.AssumedIdentity = expected } return st, nil } diff --git a/pkg/mcp/cloud/probe_test.go b/pkg/mcp/cloud/probe_test.go index a2cf8ae..4925233 100644 --- a/pkg/mcp/cloud/probe_test.go +++ b/pkg/mcp/cloud/probe_test.go @@ -111,3 +111,31 @@ func TestProbeInvalidWhenIdentityEmpty(t *testing.T) { require.NoError(t, err) assert.False(t, st.Valid, "an empty resolved identity must not be reported valid") } + +// TestProbeDegradedReportsPinnedIdentity proves a degraded probe still names +// WHICH pinned identity is degraded: when the provider errors and resolves no +// identity, Probe falls back to the expected identity the caller pinned, so +// session_status stays actionable instead of showing an empty identity. +func TestProbeDegradedReportsPinnedIdentity(t *testing.T) { + t.Parallel() + const pinned = "ro-sa@proj.iam.gserviceaccount.com" + p := &fakeProvider{name: "gcp", identityErr: errors.New("token expired")} + st, err := Probe(context.Background(), p, pinned, nil) + require.NoError(t, err, "Probe should degrade, not error") + assert.False(t, st.Valid) + assert.Equal(t, pinned, st.AssumedIdentity, + "a degraded probe must report the pinned identity so the operator knows what to fix") +} + +// TestProbeFallsBackToExpectedWhenProviderOmitsIdentity covers the valid path: +// a provider that resolves to valid but reports no identity (an unusual but +// possible projection gap) still shows the pinned identity rather than empty. +func TestProbeFallsBackToExpectedWhenProviderOmitsIdentity(t *testing.T) { + t.Parallel() + const pinned = "arn:aws:iam::111122223333:role/triage-ro" + p := &fakeProvider{name: "aws", identity: IdentityStatus{Provider: "aws", Valid: true}} + st, err := Probe(context.Background(), p, pinned, nil) + require.NoError(t, err) + assert.Equal(t, pinned, st.AssumedIdentity, + "an empty resolved identity must fall back to the pinned identity") +} diff --git a/pkg/mcp/cloud/provider.go b/pkg/mcp/cloud/provider.go index 111ebad..32d1bee 100644 --- a/pkg/mcp/cloud/provider.go +++ b/pkg/mcp/cloud/provider.go @@ -69,8 +69,10 @@ type IdentityStatus struct { Hint string `json:"hint,omitempty"` } -// CLIResult is the shaped result of one run_cli invocation. Raw provider JSON -// is never surfaced; the harness caps output and reports truncation. +// CLIResult is the result of one run_cli invocation. It carries the provider +// CLI's raw stdout (and stderr), each capped at the output limit with Truncated +// set when the output exceeded it. The bytes are not otherwise shaped or +// redacted; callers must not assume any projection beyond truncation. type CLIResult struct { Stdout string `json:"stdout"` Stderr string `json:"stderr,omitempty"` diff --git a/pkg/mcp/cloud/providers/aws/provider.go b/pkg/mcp/cloud/providers/aws/provider.go index d954adf..85da7af 100644 --- a/pkg/mcp/cloud/providers/aws/provider.go +++ b/pkg/mcp/cloud/providers/aws/provider.go @@ -13,8 +13,10 @@ package aws import ( _ "embed" "encoding/json" + "errors" "fmt" "os/exec" + "path/filepath" "github.com/sourcehawk/triagent/pkg/mcp/cloud" ) @@ -50,13 +52,20 @@ type Provider struct { } // New constructs the AWS provider, resolving aws to an absolute path once via -// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. +// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A +// PATH with relative entries makes LookPath return a relative path (flagged with +// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change +// cannot reinterpret it against a different working directory. func New() (*Provider, error) { bin, err := exec.LookPath("aws") - if err != nil { + if err != nil && !errors.Is(err, exec.ErrDot) { return nil, fmt.Errorf("aws: resolve aws binary: %w", err) } - return newWithBinary(bin) + abs, err := filepath.Abs(bin) + if err != nil { + return nil, fmt.Errorf("aws: resolve aws binary to absolute path: %w", err) + } + return newWithBinary(abs) } // newWithBinary builds the provider against an already-resolved binary path. It diff --git a/pkg/mcp/cloud/providers/aws/provider_test.go b/pkg/mcp/cloud/providers/aws/provider_test.go index 0df882a..b3efd36 100644 --- a/pkg/mcp/cloud/providers/aws/provider_test.go +++ b/pkg/mcp/cloud/providers/aws/provider_test.go @@ -23,6 +23,31 @@ func TestNewResolvesProvider(t *testing.T) { assert.Equal(t, "/usr/bin/aws", p.Binary()) } +// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path +// even when PATH resolution would yield a relative one, so a later subprocess +// env/PATH change cannot redirect what executes. The CLI is dropped into a temp +// dir reachable through a relative PATH entry; the resolved binary must come +// back absolute. +func TestNewResolvesBinaryToAbsolutePath(t *testing.T) { + dir := t.TempDir() + bin := filepath.Join(dir, "aws") + require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755)) + + cwd, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { _ = os.Chdir(cwd) }) + require.NoError(t, os.Chdir(dir)) + + // "." is a relative PATH entry; exec.LookPath("aws") resolves to "aws" + // (relative) under it. + t.Setenv("PATH", ".") + + p, err := New() + require.NoError(t, err) + assert.True(t, filepath.IsAbs(p.Binary()), + "New must store an absolute binary path, got %q", p.Binary()) +} + func TestDefaultAllowlistCoversReadOnlyAxes(t *testing.T) { p, err := newWithBinary("/usr/bin/aws") require.NoError(t, err) diff --git a/pkg/mcp/cloud/providers/gcp/provider.go b/pkg/mcp/cloud/providers/gcp/provider.go index 8f13cfd..3b819ea 100644 --- a/pkg/mcp/cloud/providers/gcp/provider.go +++ b/pkg/mcp/cloud/providers/gcp/provider.go @@ -8,8 +8,10 @@ package gcp import ( _ "embed" "encoding/json" + "errors" "fmt" "os/exec" + "path/filepath" "github.com/sourcehawk/triagent/pkg/mcp/cloud" ) @@ -36,13 +38,20 @@ type Provider struct { } // New constructs the gcp provider, resolving gcloud to an absolute path once via -// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. +// exec.LookPath so a poisoned PATH cannot redirect the binary at run time. A +// PATH with relative entries makes LookPath return a relative path (flagged with +// exec.ErrDot); the path is made absolute so a later subprocess env/PATH change +// cannot reinterpret it against a different working directory. func New() (*Provider, error) { bin, err := exec.LookPath("gcloud") - if err != nil { + if err != nil && !errors.Is(err, exec.ErrDot) { return nil, fmt.Errorf("gcp: resolve gcloud binary: %w", err) } - return newWithBinary(bin) + abs, err := filepath.Abs(bin) + if err != nil { + return nil, fmt.Errorf("gcp: resolve gcloud binary to absolute path: %w", err) + } + return newWithBinary(abs) } // newWithBinary builds the provider against an already-resolved binary path. It diff --git a/pkg/mcp/cloud/providers/gcp/provider_test.go b/pkg/mcp/cloud/providers/gcp/provider_test.go index 5c1f544..2544384 100644 --- a/pkg/mcp/cloud/providers/gcp/provider_test.go +++ b/pkg/mcp/cloud/providers/gcp/provider_test.go @@ -20,6 +20,31 @@ func TestNewResolvesBinaryAndName(t *testing.T) { assert.Equal(t, "/usr/bin/gcloud", p.Binary()) } +// TestNewResolvesBinaryToAbsolutePath proves New stores an absolute binary path +// even when PATH resolution would yield a relative one, so a later subprocess +// env/PATH change cannot redirect what executes. The provider's CLI is dropped +// into a temp dir reachable through a relative PATH entry; the resolved binary +// must come back absolute. +func TestNewResolvesBinaryToAbsolutePath(t *testing.T) { + dir := t.TempDir() + bin := filepath.Join(dir, "gcloud") + require.NoError(t, os.WriteFile(bin, []byte("#!/bin/sh\n"), 0o755)) + + cwd, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { _ = os.Chdir(cwd) }) + require.NoError(t, os.Chdir(dir)) + + // "." is a relative PATH entry; exec.LookPath("gcloud") resolves to "gcloud" + // (relative) under it. + t.Setenv("PATH", ".") + + p, err := New() + require.NoError(t, err) + assert.True(t, filepath.IsAbs(p.Binary()), + "New must store an absolute binary path, got %q", p.Binary()) +} + func TestDefaultAllowlistLoadsEmbeddedJSON(t *testing.T) { t.Parallel() p, err := newWithBinary("/usr/bin/gcloud") diff --git a/pkg/mcp/cloud/providers/probe.go b/pkg/mcp/cloud/providers/probe.go index 8106b69..2273a52 100644 --- a/pkg/mcp/cloud/providers/probe.go +++ b/pkg/mcp/cloud/providers/probe.go @@ -45,7 +45,12 @@ type Source struct { func ProbeSource(ctx context.Context, src Source) cloud.IdentityStatus { p, err := New(src.Provider) if err != nil { - return cloud.IdentityStatus{Provider: src.Provider, Valid: false, Hint: err.Error()} + return cloud.IdentityStatus{ + Provider: src.Provider, + AssumedIdentity: src.AssumedIdentity, + Valid: false, + Hint: err.Error(), + } } return probeProvider(ctx, p, src.AssumedIdentity, sourceEnvFor(p, src)) } diff --git a/pkg/mcp/cloud/providers/probe_test.go b/pkg/mcp/cloud/providers/probe_test.go index 4c484ac..8d54a8b 100644 --- a/pkg/mcp/cloud/providers/probe_test.go +++ b/pkg/mcp/cloud/providers/probe_test.go @@ -84,6 +84,21 @@ func TestProbeSourceUnknownProviderDegrades(t *testing.T) { assert.NotEmpty(t, st.Hint) } +// TestProbeSourceConstructionFailureKeepsPinnedIdentity proves a provider +// construction failure (here an unknown provider, which never reaches New's CLI +// lookup but exercises the same construction-error path) still reports the +// pinned identity, so preflight and connections name the degraded source's +// identity the operator must fix instead of an empty one. +func TestProbeSourceConstructionFailureKeepsPinnedIdentity(t *testing.T) { + const pinned = "arn:aws:iam::111122223333:role/triage-ro" + st := ProbeSource(context.Background(), Source{Provider: "azure", AssumedIdentity: pinned}) + assert.False(t, st.Valid) + assert.Equal(t, "azure", st.Provider) + assert.Equal(t, pinned, st.AssumedIdentity, + "a construction failure must still carry the pinned identity") + assert.NotEmpty(t, st.Hint) +} + // fakePassthroughProvider exposes a fixed EnvPassthrough so sourceEnv's // carry-and-overlay behaviour can be asserted without a real cloud CLI. type fakePassthroughProvider struct{ passthrough []string }