From 85714490cf2e13620db5e6f140eac8c603918eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:30:15 +0200 Subject: [PATCH 1/9] feat(cloud): provider interface and server skeleton (#45) Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/allowlist.go | 27 ++++++++++++++ pkg/mcp/cloud/fake_test.go | 48 +++++++++++++++++++++++++ pkg/mcp/cloud/provider.go | 69 ++++++++++++++++++++++++++++++++++++ pkg/mcp/cloud/server.go | 48 +++++++++++++++++++++++++ pkg/mcp/cloud/server_test.go | 13 +++++++ 5 files changed, 205 insertions(+) create mode 100644 pkg/mcp/cloud/allowlist.go create mode 100644 pkg/mcp/cloud/fake_test.go create mode 100644 pkg/mcp/cloud/provider.go create mode 100644 pkg/mcp/cloud/server.go create mode 100644 pkg/mcp/cloud/server_test.go diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go new file mode 100644 index 0000000..2edd2c1 --- /dev/null +++ b/pkg/mcp/cloud/allowlist.go @@ -0,0 +1,27 @@ +package cloud + +// 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. +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 +// subcommand paths run_cli permits. +type CommandAllowlist struct { + Commands []Command `json:"commands"` +} + +// DenyFloor is the always-on set of subcommands, flags, and argument-value +// prefixes that the config can never re-enable. The base floor lives in this +// package; a Provider contributes provider-specific additions through +// DenyFloorAdditions, mirroring how k8s.LoadAllowlist always drops Secret. +type DenyFloor struct { + Subcommands []string `json:"subcommands,omitempty"` + Flags []string `json:"flags,omitempty"` + ArgPrefixes []string `json:"arg_prefixes,omitempty"` +} diff --git a/pkg/mcp/cloud/fake_test.go b/pkg/mcp/cloud/fake_test.go new file mode 100644 index 0000000..fc89e87 --- /dev/null +++ b/pkg/mcp/cloud/fake_test.go @@ -0,0 +1,48 @@ +package cloud + +import "context" + +// fakeProvider is the in-package test double for the Provider interface. +// Providers (gcp, aws) implement the same contract in their own subpackages; +// this fake exercises the parent package's harness, tools, and probe without +// shelling any real cloud CLI. +type fakeProvider struct { + name string + binary string + allowlist *CommandAllowlist + denyFloor DenyFloor + inventory Inventory + identity IdentityStatus + identityErr error +} + +func (f *fakeProvider) Name() string { + if f.name == "" { + return "fake" + } + return f.name +} + +func (f *fakeProvider) Binary() string { + if f.binary == "" { + return "/bin/true" + } + return f.binary +} + +func (f *fakeProvider) DefaultAllowlist() *CommandAllowlist { + if f.allowlist == nil { + return &CommandAllowlist{} + } + return f.allowlist +} + +func (f *fakeProvider) DenyFloorAdditions() DenyFloor { return f.denyFloor } + +func (f *fakeProvider) Inventory(context.Context, RunFunc) (Inventory, error) { + return f.inventory, nil +} + +func (f *fakeProvider) Identity(context.Context, RunFunc) (IdentityStatus, error) { + return f.identity, f.identityErr +} diff --git a/pkg/mcp/cloud/provider.go b/pkg/mcp/cloud/provider.go new file mode 100644 index 0000000..ad3f0f4 --- /dev/null +++ b/pkg/mcp/cloud/provider.go @@ -0,0 +1,69 @@ +// Package cloud implements the read-only cloud-context MCP server the +// triagent-mcp binary exposes to Claude. One package serves both GCP and AWS: +// the cloud-specific behaviour sits behind the Provider interface, selected at +// launch by --provider and plugged in from pkg/mcp/cloud/providers/. +// +// The server is read-only by construction. run_cli never touches a shell, every +// invocation is validated against a positive command allowlist plus a hardcoded +// deny floor the config can never re-enable, and the cloud identity is pinned by +// the deployment through harness-controlled env the agent cannot reach. +package cloud + +import "context" + +// Provider is the cloud-specific seam every tool calls through. Selecting +// --provider chooses the concrete gcp or aws implementation, injected behind +// this interface (the teleport DI pattern). Implementations live in +// pkg/mcp/cloud/providers/, never in this package. +type Provider interface { + // Name reports the provider identifier ("gcp" | "aws"). + Name() string + // Binary is the resolved absolute path to the provider CLI (gcloud/aws). + Binary() string + // DefaultAllowlist is the provider's embedded default command allowlist. + DefaultAllowlist() *CommandAllowlist + // DenyFloorAdditions contributes provider-specific subcommands and flags to + // the always-on deny floor. The base floor lives in this package; providers + // only add to it, never relax it. + DenyFloorAdditions() DenyFloor + // Inventory projects the provider's accessible scopes (projects for gcp, + // accounts for aws). It execs only through run, never directly. + Inventory(ctx context.Context, run RunFunc) (Inventory, error) + // Identity is the read-only whoami: which pinned identity is active and + // whether it is valid. It execs only through run, never directly. + Identity(ctx context.Context, run RunFunc) (IdentityStatus, error) +} + +// RunFunc is the harness exec core, injected into providers so they never exec +// directly. It carries the no-shell guarantee: argv tokens reach the provider +// binary via execve, never a shell. +type RunFunc func(ctx context.Context, argv []string) (CLIResult, error) + +// Inventory is the projected list of accessible scopes the agent uses to orient. +type Inventory struct { + Scopes []Scope `json:"scopes"` +} + +// Scope is one project (gcp) or account (aws) the pinned identity can read. +type Scope struct { + ID string `json:"id"` + Name string `json:"name"` +} + +// IdentityStatus is the single struct the identity probe returns. The +// connections array, the session_status tool, and the preflight gate all render +// from it, so they cannot disagree. JSON tags are a downstream contract. +type IdentityStatus struct { + Provider string `json:"provider"` + AssumedIdentity string `json:"assumed_identity"` + Valid bool `json:"valid"` + 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. +type CLIResult struct { + Stdout string `json:"stdout"` + Truncated bool `json:"truncated"` + ExitCode int `json:"exit_code"` +} diff --git a/pkg/mcp/cloud/server.go b/pkg/mcp/cloud/server.go new file mode 100644 index 0000000..9d6d3f1 --- /dev/null +++ b/pkg/mcp/cloud/server.go @@ -0,0 +1,48 @@ +package cloud + +import ( + "context" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// Options configures the cloud-context MCP server. +type Options struct { + // Provider is the cloud-specific backend (gcp or aws), injected behind the + // Provider interface. Required; New errors when nil. + Provider Provider +} + +// Server holds the configured cloud-context MCP server. +type Server struct { + impl *mcp.Server + provider Provider +} + +// New constructs a cloud-context MCP server. Provider is required. +func New(opts Options) (*Server, error) { + if opts.Provider == nil { + return nil, fmt.Errorf("cloud: Provider is required") + } + impl := mcp.NewServer(&mcp.Implementation{ + Name: "triagent-mcp-cloud", + Version: "0.1.0", + }, nil) + s := &Server{ + impl: impl, + provider: opts.Provider, + } + s.registerOn(impl) + return s, nil +} + +// Run serves MCP requests over stdio until the client disconnects or ctx is +// cancelled. +func (s *Server) Run(ctx context.Context) error { + return s.impl.Run(ctx, &mcp.StdioTransport{}) +} + +// registerOn wires the cloud tools onto impl. Called from New and from wire +// tests inside the package. +func (s *Server) registerOn(impl *mcp.Server) {} diff --git a/pkg/mcp/cloud/server_test.go b/pkg/mcp/cloud/server_test.go new file mode 100644 index 0000000..2a610df --- /dev/null +++ b/pkg/mcp/cloud/server_test.go @@ -0,0 +1,13 @@ +package cloud + +import "testing" + +func TestNewRequiresProvider(t *testing.T) { + t.Parallel() + if _, err := New(Options{}); err == nil { + t.Fatal("expected error when Provider is nil") + } + if _, err := New(Options{Provider: &fakeProvider{}}); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} From b2282ddf968fc217ea21124016a5679d1f36c2a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:31:30 +0200 Subject: [PATCH 2/9] feat(cloud): command allowlist with hardcoded deny floor (#45) Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/allowlist.go | 133 ++++++++++++++++++++++++++++ pkg/mcp/cloud/allowlist_test.go | 72 +++++++++++++++ pkg/mcp/cloud/default_commands.json | 3 + 3 files changed, 208 insertions(+) create mode 100644 pkg/mcp/cloud/allowlist_test.go create mode 100644 pkg/mcp/cloud/default_commands.json diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go index 2edd2c1..f9389e0 100644 --- a/pkg/mcp/cloud/allowlist.go +++ b/pkg/mcp/cloud/allowlist.go @@ -1,5 +1,21 @@ package cloud +import ( + _ "embed" + "encoding/json" + "fmt" + "os" + "strings" +) + +// defaultCommandsJSON is the parent package's embedded default allowlist. It is +// intentionally empty: provider command sets ship in each provider's own +// default_commands.json (pkg/mcp/cloud/providers/). This anchor lets the +// shared loader compile and gives LoadCommandAllowlist("", …) a valid document. +// +//go:embed default_commands.json +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 @@ -25,3 +41,120 @@ type DenyFloor struct { Flags []string `json:"flags,omitempty"` ArgPrefixes []string `json:"arg_prefixes,omitempty"` } + +// denyFloor is the base floor. Config can never re-enable these; they are +// filtered out of any loaded allowlist and rejected in argv validation. The +// floor covers credential-reading and identity/endpoint-redirecting subcommands +// and flags, plus argument prefixes that read local files or reach the network +// (local-file read and SSRF vectors). +var denyFloor = DenyFloor{ + Subcommands: []string{"secrets", "ssh", "scp", "cp", "sync", "auth", "config"}, + Flags: []string{ + "--impersonate-service-account", "--account", "--profile", + "--endpoint-url", "--cli-input-json", "--cli-input-yaml", "--configuration", + }, + ArgPrefixes: []string{"file://", "fileb://", "@", "http://", "https://"}, +} + +// mergeDenyFloor combines the base floor with provider additions into one floor. +func mergeDenyFloor(extra DenyFloor) DenyFloor { + return DenyFloor{ + Subcommands: append(append([]string{}, denyFloor.Subcommands...), extra.Subcommands...), + Flags: append(append([]string{}, denyFloor.Flags...), extra.Flags...), + ArgPrefixes: append(append([]string{}, denyFloor.ArgPrefixes...), extra.ArgPrefixes...), + } +} + +// LoadCommandAllowlist returns the command allowlist from path, or the embedded +// default when path is empty, then filters out every command whose subcommand +// path falls under the base deny floor plus the provider's extra additions. A +// too-broad override can never re-enable a floored command — the filter is +// applied identically regardless of input, the LoadAllowlist pattern. +func LoadCommandAllowlist(path string, extra DenyFloor) (*CommandAllowlist, error) { + data := defaultCommandsJSON + if path != "" { + b, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read command allowlist %q: %w", path, err) + } + data = b + } + + var list CommandAllowlist + if err := json.Unmarshal(data, &list); err != nil { + return nil, fmt.Errorf("parse command allowlist: %w", err) + } + + floor := mergeDenyFloor(extra) + filtered := make([]Command, 0, len(list.Commands)) + for _, c := range list.Commands { + if c.Path == "" { + return nil, fmt.Errorf("command allowlist entry missing path: %+v", c) + } + if floor.deniesSubcommand(normalizePath(c.Path)) { + continue + } + filtered = append(filtered, c) + } + list.Commands = filtered + return &list, nil +} + +// Allows reports whether argv's leading subcommand tokens match an allowlisted +// command path. Flag tokens and their values do not participate in the match; +// only the positional subcommand path does. +func (a *CommandAllowlist) Allows(argv []string) bool { + path := subcommandPath(argv) + for _, c := range a.Commands { + if pathHasPrefix(path, normalizePath(c.Path)) { + return true + } + } + return false +} + +// deniesSubcommand reports whether a normalized subcommand path falls under any +// floored subcommand. A floor entry matches when it is a token-wise prefix of +// the path, so "secrets" floors "secrets versions access" and "compute ssh" +// floors "compute ssh foo". +func (d DenyFloor) deniesSubcommand(path []string) bool { + for _, s := range d.Subcommands { + if pathHasPrefix(path, normalizePath(s)) { + return true + } + } + return false +} + +// subcommandPath returns the leading positional tokens of argv, stopping at the +// first flag (a token beginning with "-"). These tokens form the subcommand +// path the allowlist and deny floor match against. +func subcommandPath(argv []string) []string { + out := make([]string, 0, len(argv)) + for _, tok := range argv { + if strings.HasPrefix(tok, "-") { + break + } + out = append(out, tok) + } + return out +} + +// normalizePath splits a space-separated command path ("compute firewall-rules +// list") into its tokens. +func normalizePath(path string) []string { + return strings.Fields(path) +} + +// pathHasPrefix reports whether prefix is a token-wise prefix of path. +func pathHasPrefix(path, prefix []string) bool { + if len(prefix) == 0 || len(prefix) > len(path) { + return false + } + for i := range prefix { + if path[i] != prefix[i] { + return false + } + } + return true +} diff --git a/pkg/mcp/cloud/allowlist_test.go b/pkg/mcp/cloud/allowlist_test.go new file mode 100644 index 0000000..6bd66fa --- /dev/null +++ b/pkg/mcp/cloud/allowlist_test.go @@ -0,0 +1,72 @@ +package cloud + +import ( + "os" + "path/filepath" + "testing" +) + +func writeTemp(t *testing.T, body string) string { + t.Helper() + p := filepath.Join(t.TempDir(), "allowlist.json") + if err := os.WriteFile(p, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return p +} + +func TestLoadCommandAllowlistDropsDenyFloor(t *testing.T) { + t.Parallel() + // JSON that tries to allow a deny-floored subcommand. + path := writeTemp(t, `{"commands":[{"path":"projects list"},{"path":"secrets versions access"}]}`) + al, err := LoadCommandAllowlist(path, DenyFloor{}) + if err != nil { + t.Fatal(err) + } + if al.Allows([]string{"secrets", "versions", "access"}) { + t.Fatal("deny floor must drop secrets access regardless of config") + } + if !al.Allows([]string{"projects", "list"}) { + t.Fatal("projects list should be allowed") + } +} + +func TestLoadCommandAllowlistUsesEmbeddedDefaultWhenPathEmpty(t *testing.T) { + t.Parallel() + // The parent package ships no provider commands of its own; an empty path + // yields the empty embedded default, not an error. + al, err := LoadCommandAllowlist("", DenyFloor{}) + if err != nil { + t.Fatal(err) + } + if al == nil { + t.Fatal("expected a non-nil allowlist for the empty default") + } +} + +func TestLoadCommandAllowlistMergesProviderDenyFloorAdditions(t *testing.T) { + t.Parallel() + path := writeTemp(t, `{"commands":[{"path":"compute instances list"},{"path":"compute ssh foo"}]}`) + extra := DenyFloor{Subcommands: []string{"compute ssh"}} + al, err := LoadCommandAllowlist(path, extra) + if err != nil { + t.Fatal(err) + } + if al.Allows([]string{"compute", "ssh", "foo"}) { + t.Fatal("provider deny-floor addition must drop compute ssh") + } + if !al.Allows([]string{"compute", "instances", "list"}) { + t.Fatal("compute instances list should remain allowed") + } +} + +func TestAllowsMatchesLongestPathPrefix(t *testing.T) { + t.Parallel() + al := &CommandAllowlist{Commands: []Command{{Path: "compute firewall-rules list"}}} + if !al.Allows([]string{"compute", "firewall-rules", "list", "--project", "prod"}) { + t.Fatal("argv whose leading tokens match an allowed path should pass") + } + if al.Allows([]string{"compute", "firewall-rules", "delete"}) { + t.Fatal("a different verb under the same group must not be allowed") + } +} diff --git a/pkg/mcp/cloud/default_commands.json b/pkg/mcp/cloud/default_commands.json new file mode 100644 index 0000000..f2ae3f6 --- /dev/null +++ b/pkg/mcp/cloud/default_commands.json @@ -0,0 +1,3 @@ +{ + "commands": [] +} From 058c48ebb4cafa1c04bce563ab0c0fc33f57fbc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:33:21 +0200 Subject: [PATCH 3/9] feat(cloud): argv validation against allowlist, deny floor, and scope (#45) Exact-match the positional subcommand path so a surplus token (a shell metacharacter, an extra argument) cannot ride through on an allowed prefix. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/allowlist.go | 23 ++++-- pkg/mcp/cloud/validate.go | 125 +++++++++++++++++++++++++++++++++ pkg/mcp/cloud/validate_test.go | 69 ++++++++++++++++++ 3 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 pkg/mcp/cloud/validate.go create mode 100644 pkg/mcp/cloud/validate_test.go diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go index f9389e0..2fa72dc 100644 --- a/pkg/mcp/cloud/allowlist.go +++ b/pkg/mcp/cloud/allowlist.go @@ -100,13 +100,15 @@ func LoadCommandAllowlist(path string, extra DenyFloor) (*CommandAllowlist, erro return &list, nil } -// Allows reports whether argv's leading subcommand tokens match an allowlisted -// command path. Flag tokens and their values do not participate in the match; -// only the positional subcommand path does. +// Allows reports whether argv's positional subcommand path exactly equals an +// allowlisted command path. Flag tokens and their values do not participate; +// only the leading positionals do. The match is exact rather than prefix so a +// surplus positional — a shell metacharacter token, an extra argument — never +// rides through on the back of an allowed prefix. func (a *CommandAllowlist) Allows(argv []string) bool { path := subcommandPath(argv) for _, c := range a.Commands { - if pathHasPrefix(path, normalizePath(c.Path)) { + if pathEqual(path, normalizePath(c.Path)) { return true } } @@ -158,3 +160,16 @@ func pathHasPrefix(path, prefix []string) bool { } return true } + +// pathEqual reports whether two token paths are identical. +func pathEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/pkg/mcp/cloud/validate.go b/pkg/mcp/cloud/validate.go new file mode 100644 index 0000000..770b995 --- /dev/null +++ b/pkg/mcp/cloud/validate.go @@ -0,0 +1,125 @@ +package cloud + +import ( + "fmt" + "strings" +) + +// ScopeAllowlist is the deployment's set of cloud targets any run_cli argv may +// reference. An empty field means that target axis is unconstrained. The agent +// cannot pivot to an un-allowlisted project, account, or region. +type ScopeAllowlist struct { + Projects []string `json:"projects,omitempty"` + Accounts []string `json:"accounts,omitempty"` + Regions []string `json:"regions,omitempty"` +} + +// allowedFor maps a target-selecting flag to the ScopeAllowlist field whose +// membership a value of that flag must satisfy. The deny floor rejects identity +// flags (--account, --profile) before scope ever sees them, so scope governs +// only the location axes the agent is allowed to choose among. +func (s ScopeAllowlist) allowedFor(flag string) ([]string, bool) { + switch flag { + case "--project": + return s.Projects, true + case "--region", "--zone": + return s.Regions, true + default: + return nil, false + } +} + +// validateArgv enforces the no-bypass contract on one argv before exec: the +// positional subcommand path is on the allowlist, no token is a deny-floored +// flag or arg-prefix, and every target-selecting flag value is within scope. +// It runs entirely on argv tokens — there is no shell, so metacharacter tokens +// are inert positionals that simply fail the exact allowlist match. +func validateArgv(argv []string, allow *CommandAllowlist, scope ScopeAllowlist) error { + if len(argv) == 0 { + return fmt.Errorf("empty command") + } + if !allow.Allows(argv) { + return fmt.Errorf("subcommand not on the allowlist: %q", strings.Join(subcommandPath(argv), " ")) + } + + floor := denyFloor // base floor; provider additions are filtered at load time. + for i := 0; i < len(argv); i++ { + tok := argv[i] + flag, value, hasInlineValue := splitFlag(tok) + + if strings.HasPrefix(flag, "-") { + if floorDeniesFlag(floor, flag) { + return fmt.Errorf("flag is on the deny floor: %s", flag) + } + // Resolve the flag's value: inline (--flag=value) or the next token. + val := value + if !hasInlineValue && i+1 < len(argv) && !strings.HasPrefix(argv[i+1], "-") { + val = argv[i+1] + } + if val != "" { + if err := checkArgPrefix(floor, val); err != nil { + return err + } + if allowed, scoped := scope.allowedFor(flag); scoped { + if err := checkScope(flag, val, allowed); err != nil { + return err + } + } + } + continue + } + // Positional token: still subject to the arg-prefix floor. + if err := checkArgPrefix(floor, tok); err != nil { + return err + } + } + return nil +} + +// splitFlag separates a "--flag=value" token into its flag and value. For a +// bare "--flag" or a non-flag token it returns the token unchanged with no +// inline value. +func splitFlag(tok string) (flag, value string, hasInlineValue bool) { + if !strings.HasPrefix(tok, "-") { + return tok, "", false + } + if eq := strings.IndexByte(tok, '='); eq >= 0 { + return tok[:eq], tok[eq+1:], true + } + return tok, "", false +} + +// floorDeniesFlag reports whether flag matches a deny-floored flag name. +func floorDeniesFlag(floor DenyFloor, flag string) bool { + for _, f := range floor.Flags { + if flag == f { + return true + } + } + return false +} + +// checkArgPrefix rejects an argument value beginning with a deny-floored prefix +// (local-file read and SSRF vectors). +func checkArgPrefix(floor DenyFloor, val string) error { + for _, p := range floor.ArgPrefixes { + if strings.HasPrefix(val, p) { + return fmt.Errorf("argument value has a denied prefix %q: %s", p, val) + } + } + return nil +} + +// checkScope rejects a target-selecting flag value outside the allowlist. An +// empty allowlist means the axis is unconstrained. +func checkScope(flag, val string, allowed []string) error { + if len(allowed) == 0 { + return nil + } + for _, a := range allowed { + if val == a { + return nil + } + } + return fmt.Errorf("%s %q is outside the allowed scope", flag, val) +} diff --git a/pkg/mcp/cloud/validate_test.go b/pkg/mcp/cloud/validate_test.go new file mode 100644 index 0000000..9dfdbbc --- /dev/null +++ b/pkg/mcp/cloud/validate_test.go @@ -0,0 +1,69 @@ +package cloud + +import "testing" + +func TestValidateArgvRejectsDenyFloorAndScope(t *testing.T) { + t.Parallel() + al := &CommandAllowlist{Commands: []Command{{Path: "compute instances list"}}} + scope := ScopeAllowlist{Projects: []string{"prod"}, Regions: []string{"us-central1"}} + cases := []struct { + name string + argv []string + ok bool + }{ + {"allowed", []string{"compute", "instances", "list", "--project", "prod"}, true}, + {"allowed-region", []string{"compute", "instances", "list", "--project", "prod", "--region", "us-central1"}, true}, + {"bad-scope", []string{"compute", "instances", "list", "--project", "other"}, false}, + {"bad-region", []string{"compute", "instances", "list", "--project", "prod", "--region", "eu-west1"}, false}, + {"impersonate", []string{"compute", "instances", "list", "--impersonate-service-account", "x"}, false}, + {"account-flag", []string{"compute", "instances", "list", "--account", "evil"}, false}, + {"profile-flag", []string{"compute", "instances", "list", "--profile", "evil"}, false}, + {"endpoint-flag", []string{"compute", "instances", "list", "--endpoint-url", "http://evil"}, false}, + {"file-prefix", []string{"compute", "instances", "list", "--filter", "@/etc/passwd"}, false}, + {"fileurl-prefix", []string{"compute", "instances", "list", "--filter", "file:///etc/passwd"}, false}, + {"httpurl-prefix", []string{"compute", "instances", "list", "--filter", "https://evil"}, false}, + {"metachar-semicolon", []string{"compute", "instances", "list", ";", "rm", "-rf", "/"}, false}, + {"metachar-pipe", []string{"compute", "instances", "list", "|", "cat"}, false}, + {"metachar-subshell", []string{"compute", "instances", "list", "$(whoami)"}, false}, + {"not-allowed", []string{"iam", "service-accounts", "create"}, false}, + {"empty", []string{}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := validateArgv(tc.argv, al, scope) + if tc.ok && err != nil { + t.Fatalf("expected argv to validate, got error: %v", err) + } + if !tc.ok && err == nil { + t.Fatal("expected validation error, got nil") + } + }) + } +} + +func TestValidateArgvEqualsFormFlag(t *testing.T) { + t.Parallel() + al := &CommandAllowlist{Commands: []Command{{Path: "compute instances list"}}} + scope := ScopeAllowlist{Projects: []string{"prod"}} + // --project=other in equals form must be caught by the scope check, and a + // deny-floored flag in equals form must be caught by the floor. + if err := validateArgv([]string{"compute", "instances", "list", "--project=other"}, al, scope); err == nil { + t.Fatal("expected --project=other (equals form) to fail the scope check") + } + if err := validateArgv([]string{"compute", "instances", "list", "--impersonate-service-account=x"}, al, scope); err == nil { + t.Fatal("expected --impersonate-service-account=x (equals form) to be denied") + } + if err := validateArgv([]string{"compute", "instances", "list", "--project=prod"}, al, scope); err != nil { + t.Fatalf("expected --project=prod (equals form, in scope) to validate, got: %v", err) + } +} + +func TestValidateArgvEmptyScopeAllowsAnyTarget(t *testing.T) { + t.Parallel() + al := &CommandAllowlist{Commands: []Command{{Path: "projects list"}}} + // An empty scope means the deployment did not constrain targets; the scope + // check must not reject a --project then. + if err := validateArgv([]string{"projects", "list", "--project", "anything"}, al, ScopeAllowlist{}); err != nil { + t.Fatalf("empty scope should not reject a target, got: %v", err) + } +} From 1275739637d64c4c7ef86c55b26b544eaa533017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:34:28 +0200 Subject: [PATCH 4/9] feat(cloud): no-shell argv exec core with output truncation (#45) Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/harness.go | 42 ++++++++++++++ pkg/mcp/cloud/harness_security_test.go | 78 ++++++++++++++++++++++++++ pkg/mcp/cloud/harness_test.go | 20 +++++++ 3 files changed, 140 insertions(+) create mode 100644 pkg/mcp/cloud/harness.go create mode 100644 pkg/mcp/cloud/harness_security_test.go create mode 100644 pkg/mcp/cloud/harness_test.go diff --git a/pkg/mcp/cloud/harness.go b/pkg/mcp/cloud/harness.go new file mode 100644 index 0000000..c185be7 --- /dev/null +++ b/pkg/mcp/cloud/harness.go @@ -0,0 +1,42 @@ +package cloud + +import ( + "context" + "errors" + "os/exec" +) + +// defaultOutputLimit caps run_cli stdout so a raw provider response cannot blow +// the agent's context budget. Output beyond it is dropped and flagged. +const defaultOutputLimit = 64 * 1024 + +// 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. +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 + + out, err := cmd.Output() + res := CLIResult{} + if len(out) > limit { + out = out[:limit] + res.Truncated = true + } + res.Stdout = string(out) + + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + res.ExitCode = exitErr.ExitCode() + return res, nil + } + return CLIResult{}, err + } + return res, nil +} diff --git a/pkg/mcp/cloud/harness_security_test.go b/pkg/mcp/cloud/harness_security_test.go new file mode 100644 index 0000000..c071814 --- /dev/null +++ b/pkg/mcp/cloud/harness_security_test.go @@ -0,0 +1,78 @@ +package cloud + +import ( + "bytes" + "context" + "os" + "strings" + "testing" +) + +// TestExecCLINeverUsesShell is the source-level half of the no-shell guarantee: +// the exec core must never construct a shell command. There is no "-c" string, +// no "sh -c", no "bash -c" anywhere in harness.go. +func TestExecCLINeverUsesShell(t *testing.T) { + t.Parallel() + src, err := os.ReadFile("harness.go") + if err != nil { + t.Fatal(err) + } + for _, banned := range []string{`"-c"`, "sh -c", "bash -c", `"sh"`, `"bash"`} { + if bytes.Contains(src, []byte(banned)) { + t.Fatalf("harness.go must never construct a shell command; found %q", banned) + } + } +} + +// TestExecCLIMetacharactersAreInert is the behavioural half: shell +// metacharacters handed to a binary as argv tokens are literal arguments, never +// interpreted. Running /bin/echo with metacharacter tokens prints them verbatim +// and spawns no second process. +func TestExecCLIMetacharactersAreInert(t *testing.T) { + t.Parallel() + argv := []string{";", "echo", "pwned", "|", "$(whoami)", "&&", "`id`"} + r, err := execCLI(context.Background(), "/bin/echo", argv, nil, 4096) + if err != nil { + t.Fatal(err) + } + got := strings.TrimRight(r.Stdout, "\n") + want := strings.Join(argv, " ") + if got != want { + t.Fatalf("metacharacters were not inert: got %q want %q", got, want) + } + if strings.Contains(r.Stdout, "pwned\n") && got != want { + t.Fatal("a second process appears to have run") + } +} + +// TestExecCLITruncates caps output at the byte limit and flags truncation. +func TestExecCLITruncates(t *testing.T) { + t.Parallel() + r, err := execCLI(context.Background(), "/bin/echo", []string{strings.Repeat("x", 100)}, nil, 10) + if err != nil { + t.Fatal(err) + } + if !r.Truncated { + t.Fatalf("expected Truncated, got %+v", r) + } + if len(r.Stdout) > 10 { + t.Fatalf("output exceeded limit: got %d bytes", len(r.Stdout)) + } +} + +// TestExecCLIMinimalEnv confirms the subprocess runs with the caller's explicit +// env, not the parent process environment, so a poisoned PATH cannot redirect +// the resolved binary and ambient secrets do not leak in. +func TestExecCLIMinimalEnv(t *testing.T) { + t.Setenv("TRIAGENT_CLOUD_HARNESS_LEAK_CANARY", "should-not-appear") + r, err := execCLI(context.Background(), "/usr/bin/env", nil, []string{"FOO=bar"}, 4096) + if err != nil { + t.Fatal(err) + } + if strings.Contains(r.Stdout, "TRIAGENT_CLOUD_HARNESS_LEAK_CANARY") { + t.Fatal("subprocess inherited the parent environment; env must be explicit") + } + if !strings.Contains(r.Stdout, "FOO=bar") { + t.Fatalf("explicit env not applied: %q", r.Stdout) + } +} diff --git a/pkg/mcp/cloud/harness_test.go b/pkg/mcp/cloud/harness_test.go new file mode 100644 index 0000000..73c8afd --- /dev/null +++ b/pkg/mcp/cloud/harness_test.go @@ -0,0 +1,20 @@ +package cloud + +import ( + "context" + "testing" +) + +// TestExecCLIExitCode surfaces the child's exit code without treating a +// non-zero exit as a Go error: a CLI that exits 1 on "not found" is a normal +// result the agent should see, not a harness failure. +func TestExecCLIExitCode(t *testing.T) { + t.Parallel() + r, err := execCLI(context.Background(), "/bin/false", nil, nil, 4096) + if err != nil { + t.Fatalf("non-zero exit should not be a Go error: %v", err) + } + if r.ExitCode != 1 { + t.Fatalf("expected exit code 1, got %d", r.ExitCode) + } +} From e77b91f89256949405a0cd4e694001c4fb773872 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:35:02 +0200 Subject: [PATCH 5/9] feat(cloud): shared identity probe (#45) Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/probe.go | 36 ++++++++++++++++++++++++ pkg/mcp/cloud/probe_test.go | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 pkg/mcp/cloud/probe.go create mode 100644 pkg/mcp/cloud/probe_test.go diff --git a/pkg/mcp/cloud/probe.go b/pkg/mcp/cloud/probe.go new file mode 100644 index 0000000..54e722d --- /dev/null +++ b/pkg/mcp/cloud/probe.go @@ -0,0 +1,36 @@ +package cloud + +import "context" + +// Probe runs the read-only whoami for one provider: which pinned identity is +// active and whether it is valid. It is the single probe the launcher's +// connections panel, the session preflight gate, and the session_status tool +// all call, so those surfaces can never disagree. +// +// Probe never returns a Go error for an unreachable or invalid identity — that +// is a degrade, reported through IdentityStatus.Valid and Hint, so a stale cloud +// credential surfaces visibly instead of failing the caller. A Go error is +// reserved for a caller contract violation (a nil provider). +func Probe(ctx context.Context, p Provider) (IdentityStatus, error) { + run := func(ctx context.Context, argv []string) (CLIResult, error) { + return execCLI(ctx, p.Binary(), argv, nil, defaultOutputLimit) + } + + st, err := p.Identity(ctx, run) + if err != nil { + return IdentityStatus{ + Provider: p.Name(), + Valid: false, + Hint: err.Error(), + }, nil + } + if st.Provider == "" { + st.Provider = p.Name() + } + if st.AssumedIdentity == "" { + // A whoami that resolved no identity is not a valid session, whatever + // the provider reported. + st.Valid = false + } + return st, nil +} diff --git a/pkg/mcp/cloud/probe_test.go b/pkg/mcp/cloud/probe_test.go new file mode 100644 index 0000000..99c996d --- /dev/null +++ b/pkg/mcp/cloud/probe_test.go @@ -0,0 +1,56 @@ +package cloud + +import ( + "context" + "errors" + "testing" +) + +func TestProbeReturnsProviderIdentity(t *testing.T) { + t.Parallel() + p := &fakeProvider{ + name: "gcp", + identity: IdentityStatus{ + Provider: "gcp", + AssumedIdentity: "ro-sa@proj.iam.gserviceaccount.com", + Valid: true, + }, + } + st, err := Probe(context.Background(), p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !st.Valid || st.AssumedIdentity != "ro-sa@proj.iam.gserviceaccount.com" { + t.Fatalf("probe did not return the provider identity: %+v", st) + } +} + +func TestProbeSurfacesProviderErrorAsInvalid(t *testing.T) { + t.Parallel() + p := &fakeProvider{name: "aws", identityErr: errors.New("token expired")} + st, err := Probe(context.Background(), p) + if err != nil { + t.Fatalf("Probe should degrade, not error: %v", err) + } + if st.Valid { + t.Fatal("expected Valid=false when the provider errors") + } + if st.Provider != "aws" { + t.Fatalf("expected provider name carried through, got %q", st.Provider) + } + if st.Hint == "" { + t.Fatal("expected the provider error surfaced as a hint") + } +} + +func TestProbeInvalidWhenIdentityEmpty(t *testing.T) { + t.Parallel() + p := &fakeProvider{name: "gcp", identity: IdentityStatus{Provider: "gcp", Valid: true}} + st, err := Probe(context.Background(), p) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if st.Valid { + t.Fatal("an empty resolved identity must not be reported valid") + } +} From 951802d8d678c784596cdea6b372d07a4f7c3730 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:37:44 +0200 Subject: [PATCH 6/9] feat(cloud): list_inventory, session_status, run_cli, list_allowed_commands (#45) Wire the four tools onto the server, load the command allowlist through the deny floor at construction, and bind run_cli + the providers to a single validated no-shell run core. list_allowed_commands reads the same allowlist run_cli enforces, so advertised equals permitted. Add the TRIAGENT_CLOUD_* env-name constants the launcher injects through the subprocess env. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/allowlist.go | 21 ++++-- pkg/mcp/cloud/env.go | 17 +++++ pkg/mcp/cloud/server.go | 75 +++++++++++++++++--- pkg/mcp/cloud/specs.go | 36 ++++++++++ pkg/mcp/cloud/tools_cli.go | 57 ++++++++++++++++ pkg/mcp/cloud/tools_inventory.go | 28 ++++++++ pkg/mcp/cloud/tools_status.go | 28 ++++++++ pkg/mcp/cloud/tools_test.go | 113 +++++++++++++++++++++++++++++++ pkg/mcp/cloud/tools_wire_test.go | 51 ++++++++++++++ 9 files changed, 412 insertions(+), 14 deletions(-) create mode 100644 pkg/mcp/cloud/env.go create mode 100644 pkg/mcp/cloud/specs.go create mode 100644 pkg/mcp/cloud/tools_cli.go create mode 100644 pkg/mcp/cloud/tools_inventory.go create mode 100644 pkg/mcp/cloud/tools_status.go create mode 100644 pkg/mcp/cloud/tools_test.go create mode 100644 pkg/mcp/cloud/tools_wire_test.go diff --git a/pkg/mcp/cloud/allowlist.go b/pkg/mcp/cloud/allowlist.go index 2fa72dc..409a90b 100644 --- a/pkg/mcp/cloud/allowlist.go +++ b/pkg/mcp/cloud/allowlist.go @@ -85,19 +85,28 @@ func LoadCommandAllowlist(path string, extra DenyFloor) (*CommandAllowlist, erro return nil, fmt.Errorf("parse command allowlist: %w", err) } - floor := mergeDenyFloor(extra) - filtered := make([]Command, 0, len(list.Commands)) for _, c := range list.Commands { if c.Path == "" { return nil, fmt.Errorf("command allowlist entry missing path: %+v", c) } - if floor.deniesSubcommand(normalizePath(c.Path)) { + } + return filterAllowlist(&list, extra), nil +} + +// filterAllowlist returns a copy of list with every command whose subcommand +// path falls under the base deny floor plus extra dropped. Applied identically +// to a loaded file and to a provider's in-memory default, so neither source can +// advertise a floored command. +func filterAllowlist(list *CommandAllowlist, extra DenyFloor) *CommandAllowlist { + floor := mergeDenyFloor(extra) + out := &CommandAllowlist{Commands: make([]Command, 0, len(list.Commands))} + for _, c := range list.Commands { + if c.Path == "" || floor.deniesSubcommand(normalizePath(c.Path)) { continue } - filtered = append(filtered, c) + out.Commands = append(out.Commands, c) } - list.Commands = filtered - return &list, nil + return out } // Allows reports whether argv's positional subcommand path exactly equals an diff --git a/pkg/mcp/cloud/env.go b/pkg/mcp/cloud/env.go new file mode 100644 index 0000000..fcb57ae --- /dev/null +++ b/pkg/mcp/cloud/env.go @@ -0,0 +1,17 @@ +package cloud + +// Environment variable names the cloud-context MCP subprocess reads. The +// launcher sets these in the subprocess env (never argv); the agent supplies +// argv only and cannot reach them. Provider impersonation env vars +// (CLOUDSDK_AUTH_IMPERSONATE_SERVICE_ACCOUNT, AWS_PROFILE) are contributed by +// the provider packages, not here. +const ( + // EnvProvider selects the concrete provider ("gcp" | "aws"). + EnvProvider = "TRIAGENT_CLOUD_PROVIDER" + // EnvAllowlistPath points at a command-allowlist override file; empty uses + // the provider's embedded default. + EnvAllowlistPath = "TRIAGENT_CLOUD_ALLOWLIST_PATH" + // EnvScope carries the target scope allowlist the launcher froze for this + // session, as JSON the cloud package decodes into ScopeAllowlist. + EnvScope = "TRIAGENT_CLOUD_SCOPE" +) diff --git a/pkg/mcp/cloud/server.go b/pkg/mcp/cloud/server.go index 9d6d3f1..fa22e40 100644 --- a/pkg/mcp/cloud/server.go +++ b/pkg/mcp/cloud/server.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/sourcehawk/triagent/pkg/mcp/telemetry" ) // Options configures the cloud-context MCP server. @@ -12,37 +13,95 @@ type Options struct { // Provider is the cloud-specific backend (gcp or aws), injected behind the // Provider interface. Required; New errors when nil. Provider Provider + // AllowlistPath optionally overrides the provider's embedded default command + // allowlist. Empty means use the provider default. The launcher points this + // at the profile-configured override via TRIAGENT_CLOUD_ALLOWLIST_PATH. + AllowlistPath string + // Scope is the set of projects/accounts/regions any run_cli argv may target. + // Argv referencing a target outside the scope is rejected before exec. The + // launcher fills it from TRIAGENT_CLOUD_SCOPE. + Scope ScopeAllowlist } // Server holds the configured cloud-context MCP server. type Server struct { - impl *mcp.Server - provider Provider + impl *mcp.Server + provider Provider + allowlist *CommandAllowlist + scope ScopeAllowlist } -// New constructs a cloud-context MCP server. Provider is required. +// New constructs a cloud-context MCP server. Provider is required. The command +// allowlist loads from Options.AllowlistPath (or the provider default when +// empty), always filtered through the base deny floor plus the provider's +// additions, so a too-broad override can never re-enable a floored command. func New(opts Options) (*Server, error) { if opts.Provider == nil { return nil, fmt.Errorf("cloud: Provider is required") } + allow, err := loadAllowlist(opts.AllowlistPath, opts.Provider) + if err != nil { + return nil, fmt.Errorf("cloud: load command allowlist: %w", err) + } impl := mcp.NewServer(&mcp.Implementation{ Name: "triagent-mcp-cloud", Version: "0.1.0", }, nil) s := &Server{ - impl: impl, - provider: opts.Provider, + impl: impl, + provider: opts.Provider, + allowlist: allow, + scope: opts.Scope, } s.registerOn(impl) return s, nil } +// loadAllowlist resolves the command allowlist for a provider: the override path +// when given, else the provider's embedded default, always filtered through the +// base deny floor plus the provider's deny-floor additions. +func loadAllowlist(path string, p Provider) (*CommandAllowlist, error) { + if path != "" { + return LoadCommandAllowlist(path, p.DenyFloorAdditions()) + } + // Filter the provider's in-memory default through the floor the same way a + // loaded file would be, so the default can never advertise a floored command. + return filterAllowlist(p.DefaultAllowlist(), p.DenyFloorAdditions()), nil +} + // Run serves MCP requests over stdio until the client disconnects or ctx is // cancelled. func (s *Server) Run(ctx context.Context) error { return s.impl.Run(ctx, &mcp.StdioTransport{}) } -// registerOn wires the cloud tools onto impl. Called from New and from wire -// tests inside the package. -func (s *Server) registerOn(impl *mcp.Server) {} +// run is the harness exec core bound to this server's provider binary, scope, +// and allowlist. Providers and tools exec only through this RunFunc, never +// directly: it validates argv before handing it to the no-shell exec core. +func (s *Server) run(ctx context.Context, argv []string) (CLIResult, error) { + if err := validateArgv(argv, s.allowlist, s.scope); err != nil { + return CLIResult{}, err + } + return execCLI(ctx, s.provider.Binary(), argv, nil, defaultOutputLimit) +} + +// registerOn wires the cloud tools onto impl. Called from New and from the wire +// test inside the package. Registration order mirrors ToolSpecs(). +func (s *Server) registerOn(impl *mcp.Server) { + mcp.AddTool(impl, &mcp.Tool{ + Name: "list_inventory", + Description: descListInventory, + }, telemetry.Wrap("list_inventory", s.listInventory)) + mcp.AddTool(impl, &mcp.Tool{ + Name: "session_status", + Description: descSessionStatus, + }, telemetry.Wrap("session_status", s.sessionStatus)) + mcp.AddTool(impl, &mcp.Tool{ + Name: "run_cli", + Description: descRunCLI, + }, telemetry.Wrap("run_cli", s.runCLI)) + mcp.AddTool(impl, &mcp.Tool{ + Name: "list_allowed_commands", + Description: descListAllowedCommands, + }, telemetry.Wrap("list_allowed_commands", s.listAllowedCommands)) +} diff --git a/pkg/mcp/cloud/specs.go b/pkg/mcp/cloud/specs.go new file mode 100644 index 0000000..75e699b --- /dev/null +++ b/pkg/mcp/cloud/specs.go @@ -0,0 +1,36 @@ +package cloud + +import "github.com/sourcehawk/triagent/pkg/mcp/toolspec" + +// ToolSpecs returns the cloud server's tool catalog with each tool's input shape +// introspected from its Go struct (and its jsonschema tags). +// +// Order mirrors the registration order in server.go's registerOn(). +func ToolSpecs() []toolspec.ToolSpec { + return []toolspec.ToolSpec{ + { + Server: "triagent-cloud", + Name: "list_inventory", + Description: descListInventory, + Inputs: toolspec.FromStruct(ListInventoryInput{}), + }, + { + Server: "triagent-cloud", + Name: "session_status", + Description: descSessionStatus, + Inputs: toolspec.FromStruct(SessionStatusInput{}), + }, + { + Server: "triagent-cloud", + Name: "run_cli", + Description: descRunCLI, + Inputs: toolspec.FromStruct(RunCLIInput{}), + }, + { + Server: "triagent-cloud", + Name: "list_allowed_commands", + Description: descListAllowedCommands, + Inputs: toolspec.FromStruct(ListAllowedCommandsInput{}), + }, + } +} diff --git a/pkg/mcp/cloud/tools_cli.go b/pkg/mcp/cloud/tools_cli.go new file mode 100644 index 0000000..e0149b3 --- /dev/null +++ b/pkg/mcp/cloud/tools_cli.go @@ -0,0 +1,57 @@ +package cloud + +import ( + "context" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const descRunCLI = "Run one read-only provider CLI command (gcloud/aws). Supply the argument tokens as an array, never a single string — there is no shell. Only allowlisted subcommands run; identity flags, credential-reading subcommands, and out-of-scope targets are rejected. See list_allowed_commands for what is permitted." + +const descListAllowedCommands = "List the provider CLI subcommands run_cli permits, with the investigative axis each serves. This is exactly what run_cli enforces, so what is advertised is what is allowed." + +// RunCLIInput is the input schema for run_cli. Argv is a typed array of argument +// tokens, never a single command string: the harness never tokenizes, so there +// is no in-house splitter to fool and shell metacharacters are inert. +type RunCLIInput struct { + Argv []string `json:"argv" jsonschema:"The provider CLI argument tokens as an array (for example [\"compute\",\"firewall-rules\",\"list\",\"--project\",\"prod\"]). Do not include the binary name or pass a single string."` +} + +// RunCLIOutput is the response schema for run_cli: the shaped CLI result, never +// raw API JSON beyond the captured stdout the provider emitted. +type RunCLIOutput = CLIResult + +// runCLI validates the argv against the allowlist, deny floor, and scope, then +// execs it through the no-shell core. A rejected argv is a tool error returned +// before any exec; a non-zero CLI exit is a normal result the agent sees. +func (s *Server) runCLI(ctx context.Context, _ *mcp.CallToolRequest, in RunCLIInput) (*mcp.CallToolResult, RunCLIOutput, error) { + res, err := s.run(ctx, in.Argv) + if err != nil { + return errorResult(fmt.Sprintf("run_cli rejected: %v", err)), RunCLIOutput{}, nil + } + return nil, res, nil +} + +// ListAllowedCommandsInput is the input schema for list_allowed_commands. It +// takes no parameters. +type ListAllowedCommandsInput struct{} + +// ListAllowedCommandsOutput is the response schema for list_allowed_commands. +type ListAllowedCommandsOutput struct { + Commands []Command `json:"commands"` +} + +// listAllowedCommands returns the same CommandAllowlist run_cli enforces, so the +// catalog and the gate can never disagree. +func (s *Server) listAllowedCommands(_ context.Context, _ *mcp.CallToolRequest, _ ListAllowedCommandsInput) (*mcp.CallToolResult, ListAllowedCommandsOutput, error) { + return nil, ListAllowedCommandsOutput{Commands: s.allowlist.Commands}, nil +} + +// errorResult builds an MCP error result whose Content carries msg. +func errorResult(msg string) *mcp.CallToolResult { + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: msg}}, + } +} diff --git a/pkg/mcp/cloud/tools_inventory.go b/pkg/mcp/cloud/tools_inventory.go new file mode 100644 index 0000000..7d3cdfc --- /dev/null +++ b/pkg/mcp/cloud/tools_inventory.go @@ -0,0 +1,28 @@ +package cloud + +import ( + "context" + "fmt" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const descListInventory = "List the cloud projects (GCP) or accounts (AWS) the pinned read-only identity can see, so you can orient before drilling in. Read-only." + +// ListInventoryInput is the input schema for list_inventory. It takes no +// parameters: the accessible scope is fixed by the pinned identity. +type ListInventoryInput struct{} + +// ListInventoryOutput is the response schema for list_inventory: the provider's +// accessible scopes. +type ListInventoryOutput = Inventory + +// listInventory projects the provider's accessible scopes. The provider execs +// only through the server's validated run core. +func (s *Server) listInventory(ctx context.Context, _ *mcp.CallToolRequest, _ ListInventoryInput) (*mcp.CallToolResult, ListInventoryOutput, error) { + inv, err := s.provider.Inventory(ctx, s.run) + if err != nil { + return errorResult(fmt.Sprintf("list inventory: %v", err)), ListInventoryOutput{}, nil + } + return nil, inv, nil +} diff --git a/pkg/mcp/cloud/tools_status.go b/pkg/mcp/cloud/tools_status.go new file mode 100644 index 0000000..08ead5e --- /dev/null +++ b/pkg/mcp/cloud/tools_status.go @@ -0,0 +1,28 @@ +package cloud + +import ( + "context" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +const descSessionStatus = "Report the pinned read-only cloud identity this session acts as and whether it is currently valid. You cannot choose or change it. Read-only." + +// SessionStatusInput is the input schema for session_status. It takes no +// parameters: the identity is pinned by the deployment. +type SessionStatusInput struct{} + +// SessionStatusOutput is the response schema for session_status. It is the +// shared IdentityStatus the connections panel and preflight gate also render. +type SessionStatusOutput = IdentityStatus + +// sessionStatus runs the shared identity probe and returns its result. It never +// errors on an invalid identity — a stale credential surfaces as Valid:false +// with a Hint, the same visible-degrade contract the launcher renders. +func (s *Server) sessionStatus(ctx context.Context, _ *mcp.CallToolRequest, _ SessionStatusInput) (*mcp.CallToolResult, SessionStatusOutput, error) { + st, err := Probe(ctx, s.provider) + if err != nil { + return errorResult(err.Error()), SessionStatusOutput{}, nil + } + return nil, st, nil +} diff --git a/pkg/mcp/cloud/tools_test.go b/pkg/mcp/cloud/tools_test.go new file mode 100644 index 0000000..9a70571 --- /dev/null +++ b/pkg/mcp/cloud/tools_test.go @@ -0,0 +1,113 @@ +package cloud + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +func newTestServer(t *testing.T, p Provider, opts ...func(*Options)) *Server { + t.Helper() + o := Options{Provider: p} + for _, f := range opts { + f(&o) + } + srv, err := New(o) + require.NoError(t, err) + return srv +} + +func TestListInventoryReturnsProviderScopes(t *testing.T) { + t.Parallel() + p := &fakeProvider{inventory: Inventory{Scopes: []Scope{{ID: "prod", Name: "Production"}}}} + srv := newTestServer(t, p) + _, out, err := srv.listInventory(context.Background(), nil, ListInventoryInput{}) + require.NoError(t, err) + require.Len(t, out.Scopes, 1) + require.Equal(t, "prod", out.Scopes[0].ID) +} + +func TestSessionStatusReturnsProbeIdentity(t *testing.T) { + t.Parallel() + p := &fakeProvider{ + name: "gcp", + identity: IdentityStatus{Provider: "gcp", AssumedIdentity: "ro-sa@proj", Valid: true}, + } + srv := newTestServer(t, p) + _, out, err := srv.sessionStatus(context.Background(), nil, SessionStatusInput{}) + require.NoError(t, err) + require.True(t, out.Valid) + require.Equal(t, "ro-sa@proj", out.AssumedIdentity) +} + +func TestListAllowedCommandsReturnsLoadedAllowlist(t *testing.T) { + t.Parallel() + p := &fakeProvider{allowlist: &CommandAllowlist{Commands: []Command{ + {Path: "projects list", Description: "orient: list projects"}, + }}} + srv := newTestServer(t, p) + _, out, err := srv.listAllowedCommands(context.Background(), nil, ListAllowedCommandsInput{}) + require.NoError(t, err) + require.Len(t, out.Commands, 1) + require.Equal(t, "projects list", out.Commands[0].Path) +} + +func TestListAllowedCommandsDropsDenyFlooredEntries(t *testing.T) { + t.Parallel() + // Even if a provider default lists a floored command, the catalog the agent + // sees is exactly what run_cli enforces — the floored entry is absent. + p := &fakeProvider{allowlist: &CommandAllowlist{Commands: []Command{ + {Path: "projects list"}, + {Path: "secrets versions access"}, + }}} + srv := newTestServer(t, p) + _, out, err := srv.listAllowedCommands(context.Background(), nil, ListAllowedCommandsInput{}) + require.NoError(t, err) + for _, c := range out.Commands { + require.NotEqual(t, "secrets versions access", c.Path, "deny-floored command must not be advertised") + } +} + +func TestRunCLIRejectsDenyFlooredArgvBeforeExec(t *testing.T) { + t.Parallel() + p := &fakeProvider{ + binary: "/bin/echo", + allowlist: &CommandAllowlist{Commands: []Command{{Path: "compute instances list"}}}, + } + srv := newTestServer(t, p) + res, _, err := srv.runCLI(context.Background(), nil, RunCLIInput{ + Argv: []string{"compute", "instances", "list", "--impersonate-service-account", "evil"}, + }) + require.NoError(t, err) + require.NotNil(t, res) + require.True(t, res.IsError, "deny-floored argv must be rejected as a tool error before exec") +} + +func TestRunCLIShapesResultOnSuccess(t *testing.T) { + t.Parallel() + p := &fakeProvider{ + binary: "/bin/echo", + allowlist: &CommandAllowlist{Commands: []Command{{Path: "projects list"}}}, + } + srv := newTestServer(t, p) + _, out, err := srv.runCLI(context.Background(), nil, RunCLIInput{Argv: []string{"projects", "list"}}) + require.NoError(t, err) + require.Contains(t, out.Stdout, "projects list") +} + +func TestRunCLIRejectsOutOfScopeTarget(t *testing.T) { + t.Parallel() + p := &fakeProvider{ + binary: "/bin/echo", + allowlist: &CommandAllowlist{Commands: []Command{{Path: "projects list"}}}, + } + srv := newTestServer(t, p, func(o *Options) { + o.Scope = ScopeAllowlist{Projects: []string{"prod"}} + }) + res, _, err := srv.runCLI(context.Background(), nil, RunCLIInput{ + Argv: []string{"projects", "list", "--project", "other"}, + }) + require.NoError(t, err) + require.True(t, res.IsError, "out-of-scope target must be rejected") +} diff --git a/pkg/mcp/cloud/tools_wire_test.go b/pkg/mcp/cloud/tools_wire_test.go new file mode 100644 index 0000000..9cf246c --- /dev/null +++ b/pkg/mcp/cloud/tools_wire_test.go @@ -0,0 +1,51 @@ +package cloud + +import ( + "context" + "testing" + + sdkmcp "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTools_Registered confirms the four cloud tools are exposed and that the +// set registered on the server matches the ToolSpecs() catalog exactly — the +// wire test fails if registration drifts from the catalog. +func TestTools_Registered(t *testing.T) { + t.Parallel() + + srv, err := New(Options{Provider: &fakeProvider{}}) + require.NoError(t, err) + + serverT, clientT := sdkmcp.NewInMemoryTransports() + serverSession, err := srv.impl.Connect(context.Background(), serverT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = serverSession.Close() }) + + client := sdkmcp.NewClient(&sdkmcp.Implementation{Name: "test-client", Version: "v0"}, nil) + clientSession, err := client.Connect(context.Background(), clientT, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = clientSession.Close() }) + + list, err := clientSession.ListTools(context.Background(), &sdkmcp.ListToolsParams{}) + require.NoError(t, err) + + registered := map[string]bool{} + for _, tool := range list.Tools { + registered[tool.Name] = true + } + + cataloged := map[string]bool{} + for _, spec := range ToolSpecs() { + cataloged[spec.Name] = true + assert.True(t, registered[spec.Name], "tool %q in ToolSpecs() but not registered", spec.Name) + } + for name := range registered { + assert.True(t, cataloged[name], "tool %q registered but absent from ToolSpecs()", name) + } + + for _, want := range []string{"list_inventory", "session_status", "run_cli", "list_allowed_commands"} { + assert.True(t, registered[want], "%s not registered", want) + } +} From 95d9b39d5d02e5a333353bee188af7b6859704e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:41:42 +0200 Subject: [PATCH 7/9] feat(cloud): register --kind=cloud --provider in serve.go (#45) Parse --provider, decode the frozen scope and allowlist override from the subprocess env, and construct the server behind cloud.Provider. The gcp/aws implementations land in their own PRs; until then a known provider reports it is not yet built and an unknown one is named in the error. Also fold cloud.ToolSpecs() into the launcher tool catalog so the four tools surface in the MCP catalog view alongside every other server. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/triagent-mcp/serve.go | 116 +++++++++++++++++++++------ cmd/triagent-mcp/serve_cloud_test.go | 48 +++++++++++ internal/server/meta.go | 2 + 3 files changed, 141 insertions(+), 25 deletions(-) create mode 100644 cmd/triagent-mcp/serve_cloud_test.go diff --git a/cmd/triagent-mcp/serve.go b/cmd/triagent-mcp/serve.go index 5650b5e..1b81ecb 100644 --- a/cmd/triagent-mcp/serve.go +++ b/cmd/triagent-mcp/serve.go @@ -2,13 +2,16 @@ package main import ( "context" + "encoding/json" "fmt" "os" "os/signal" "strings" "syscall" + "github.com/charmbracelet/log" "github.com/sourcehawk/triagent/pkg/mcp/agentoperator" + "github.com/sourcehawk/triagent/pkg/mcp/cloud" "github.com/sourcehawk/triagent/pkg/mcp/git" "github.com/sourcehawk/triagent/pkg/mcp/incidentio" "github.com/sourcehawk/triagent/pkg/mcp/k8s" @@ -21,28 +24,27 @@ import ( "github.com/sourcehawk/triagent/pkg/mcp/strategies" "github.com/sourcehawk/triagent/pkg/mcp/teleport" "github.com/sourcehawk/triagent/pkg/mcp/wiki" - "github.com/charmbracelet/log" "github.com/spf13/cobra" ) // Environment variable names. Flags override env when both are set. const ( - envKubeconfig = "TRIAGENT_MCP_KUBECONFIG" - envCRDsFile = "TRIAGENT_MCP_CRDS_FILE" - envCrossplaneGroups = "TRIAGENT_MCP_CROSSPLANE_GROUPS" - envSessionDir = "TRIAGENT_MCP_SESSION_DIR" - envUserPlaybooksDir = "TRIAGENT_MCP_USER_PLAYBOOKS_DIR" - envPluginPlaybooksDir = "TRIAGENT_MCP_PLUGIN_PLAYBOOKS_DIR" - envSystemPlaybooksDir = "TRIAGENT_MCP_SYSTEM_PLAYBOOKS_DIR" - envStrategiesSubagentModel = "TRIAGENT_MCP_STRATEGIES_SUBAGENT_MODEL" - envMCPConfigPath = "TRIAGENT_MCP_CONFIG_PATH" - envGitRepo = "TRIAGENT_MCP_GIT_REPO" - envGitCacheDir = "TRIAGENT_MCP_GIT_CACHE_DIR" - envGitClaudeBinary = "TRIAGENT_MCP_GIT_CLAUDE_BINARY" - envGitFilterPrereleases = "TRIAGENT_MCP_GIT_FILTER_PRERELEASES" - envWikiServePath = "TRIAGENT_MCP_WIKI_PATH" - envWikiServeProposalsPath = "TRIAGENT_MCP_WIKI_PROPOSALS_PATH" - envWikiServeClaudeBinary = "TRIAGENT_MCP_WIKI_CLAUDE_BINARY" + envKubeconfig = "TRIAGENT_MCP_KUBECONFIG" + envCRDsFile = "TRIAGENT_MCP_CRDS_FILE" + envCrossplaneGroups = "TRIAGENT_MCP_CROSSPLANE_GROUPS" + envSessionDir = "TRIAGENT_MCP_SESSION_DIR" + envUserPlaybooksDir = "TRIAGENT_MCP_USER_PLAYBOOKS_DIR" + envPluginPlaybooksDir = "TRIAGENT_MCP_PLUGIN_PLAYBOOKS_DIR" + envSystemPlaybooksDir = "TRIAGENT_MCP_SYSTEM_PLAYBOOKS_DIR" + envStrategiesSubagentModel = "TRIAGENT_MCP_STRATEGIES_SUBAGENT_MODEL" + envMCPConfigPath = "TRIAGENT_MCP_CONFIG_PATH" + envGitRepo = "TRIAGENT_MCP_GIT_REPO" + envGitCacheDir = "TRIAGENT_MCP_GIT_CACHE_DIR" + envGitClaudeBinary = "TRIAGENT_MCP_GIT_CLAUDE_BINARY" + envGitFilterPrereleases = "TRIAGENT_MCP_GIT_FILTER_PRERELEASES" + envWikiServePath = "TRIAGENT_MCP_WIKI_PATH" + envWikiServeProposalsPath = "TRIAGENT_MCP_WIKI_PROPOSALS_PATH" + envWikiServeClaudeBinary = "TRIAGENT_MCP_WIKI_CLAUDE_BINARY" envSessionsProposalsPath = "TRIAGENT_MCP_SESSIONS_PROPOSALS_PATH" envSessionsClaudeBinary = "TRIAGENT_MCP_SESSIONS_CLAUDE_BINARY" @@ -71,10 +73,10 @@ type serveFlags struct { systemPlaybooksDir string // git flags - gitRepo string - gitCacheDir string - gitClaudeBinary string - gitFilterPrereleases bool + gitRepo string + gitCacheDir string + gitClaudeBinary string + gitFilterPrereleases bool // wiki flags wikiPath string @@ -95,6 +97,9 @@ type serveFlags struct { promURL string promBearer string promBasic string + + // cloud flags + cloudProvider string } func serveCmd() *cobra.Command { @@ -104,14 +109,14 @@ func serveCmd() *cobra.Command { Short: "Run one of the triagent-mcp MCP servers over stdio", Long: "Run one of the triagent-mcp MCP servers over stdio.\n\n" + "Select the server via --kind. Supported kinds:\n" + - " k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom", + " k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom, cloud", Hidden: true, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { return runServe(cmd.Context(), resolveFlags(f)) }, } - cmd.Flags().StringVar(&f.kind, "kind", "", "which MCP server to run: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, or prom (required)") + cmd.Flags().StringVar(&f.kind, "kind", "", "which MCP server to run: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom, or cloud (required)") // k8s flags cmd.Flags().StringVar(&f.kubeconfig, "kubeconfig", "", "path to kubeconfig (defaults to $"+envKubeconfig+", then $KUBECONFIG, then ~/.kube/config) [kind=k8s]") @@ -150,6 +155,9 @@ func serveCmd() *cobra.Command { cmd.Flags().StringVar(&f.promBearer, "prom-bearer", "", "Authorization: Bearer token for Prometheus (defaults to $"+envPromBearer+") [kind=prom]") cmd.Flags().StringVar(&f.promBasic, "prom-basic", "", "Basic auth credentials user:pass for Prometheus (defaults to $"+envPromBasic+") [kind=prom]") + // cloud flags + cmd.Flags().StringVar(&f.cloudProvider, "provider", "", "cloud provider to serve: gcp or aws; required (defaults to $"+cloud.EnvProvider+") [kind=cloud]") + return cmd } @@ -215,6 +223,9 @@ func resolveFlags(f *serveFlags) serveFlags { if out.promBasic == "" { out.promBasic = os.Getenv(envPromBasic) } + if out.cloudProvider == "" { + out.cloudProvider = os.Getenv(cloud.EnvProvider) + } // Bool env override: only consider when the operator hasn't passed // the flag explicitly. Cobra preserves the flag default (true) when // unset, so we can't distinguish "operator passed --filter-prereleases=true" @@ -263,10 +274,12 @@ func runServe(ctx context.Context, f serveFlags) error { return runParallel(ctx, f) case "prom": return runProm(ctx, f) + case "cloud": + return runCloud(ctx, f) case "": - return fmt.Errorf("--kind is required (one of: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom)") + return fmt.Errorf("--kind is required (one of: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom, cloud)") default: - return fmt.Errorf("unknown --kind %q (want one of: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom)", f.kind) + return fmt.Errorf("unknown --kind %q (want one of: k8s, teleport, strategies, git, wiki, slack, incidentio, sessions, meta, agent-operator, signal-ingest, parallel, prom, cloud)", f.kind) } } @@ -423,6 +436,59 @@ func runProm(ctx context.Context, f serveFlags) error { return srv.Run(ctx) } +// runCloud wires the read-only cloud-context MCP. --provider selects the +// concrete backend; New plugs it in behind cloud.Provider. The launcher passes +// the allowlist override path and target scope through the subprocess env +// (cloud.EnvAllowlistPath, cloud.EnvScope), never argv. +func runCloud(ctx context.Context, f serveFlags) error { + if f.cloudProvider == "" { + return fmt.Errorf("--provider is required (gcp or aws) (set --provider or $%s)", cloud.EnvProvider) + } + provider, err := newCloudProvider(f.cloudProvider) + if err != nil { + return err + } + srv, err := cloud.New(cloud.Options{ + Provider: provider, + AllowlistPath: os.Getenv(cloud.EnvAllowlistPath), + Scope: parseCloudScope(os.Getenv(cloud.EnvScope)), + }) + if err != nil { + return fmt.Errorf("build cloud mcp server: %w", err) + } + log.Info("mcp serve --kind=cloud starting", "provider", f.cloudProvider) + return srv.Run(ctx) +} + +// newCloudProvider constructs the cloud.Provider for the named provider. The +// gcp and aws implementations land in pkg/mcp/cloud/providers/ in their +// own PRs; until then a known provider reports that it is not yet built and an +// unknown one is named in the error. +func newCloudProvider(name string) (cloud.Provider, error) { + switch name { + case "gcp", "aws": + return nil, fmt.Errorf("cloud provider %q is not built yet", name) + default: + return nil, fmt.Errorf("unknown cloud --provider %q (want gcp or aws)", name) + } +} + +// parseCloudScope decodes the JSON-encoded target scope the launcher froze into +// a cloud.ScopeAllowlist. An empty value yields an empty scope, which leaves the +// target axes unconstrained; a malformed value is logged and treated the same, +// so a bad profile entry never silently widens scope. +func parseCloudScope(raw string) cloud.ScopeAllowlist { + var scope cloud.ScopeAllowlist + if raw == "" { + return scope + } + if err := json.Unmarshal([]byte(raw), &scope); err != nil { + log.Warn("mcp serve --kind=cloud: ignoring malformed scope", "error", err) + return cloud.ScopeAllowlist{} + } + return scope +} + func runGit(ctx context.Context, f serveFlags) error { if f.gitRepo == "" { return fmt.Errorf("--repo is required (owner/name) (set --repo or $%s)", envGitRepo) diff --git a/cmd/triagent-mcp/serve_cloud_test.go b/cmd/triagent-mcp/serve_cloud_test.go new file mode 100644 index 0000000..a427db1 --- /dev/null +++ b/cmd/triagent-mcp/serve_cloud_test.go @@ -0,0 +1,48 @@ +package main + +import ( + "context" + "strings" + "testing" +) + +func TestRunServe_CloudKindRequiresProvider(t *testing.T) { + t.Parallel() + err := runServe(context.Background(), serveFlags{kind: "cloud"}) + if err == nil { + t.Fatal("expected error when --provider is missing") + } + if !strings.Contains(err.Error(), "provider") { + t.Fatalf("error should mention --provider, got: %v", err) + } +} + +func TestRunServe_CloudKindRejectsUnknownProvider(t *testing.T) { + t.Parallel() + err := runServe(context.Background(), serveFlags{kind: "cloud", cloudProvider: "azure"}) + if err == nil { + t.Fatal("expected error for an unknown provider") + } + if !strings.Contains(err.Error(), "azure") { + t.Fatalf("error should name the rejected provider, got: %v", err) + } +} + +func TestRunServe_UnknownKindErrorListsCloud(t *testing.T) { + t.Parallel() + err := runServe(context.Background(), serveFlags{kind: "bogus"}) + if err == nil { + t.Fatal("expected error for unknown kind") + } + if !strings.Contains(err.Error(), "cloud") { + t.Fatalf("kind list should include cloud, got: %v", err) + } +} + +func TestServeCmd_KnowsCloudKind(t *testing.T) { + t.Parallel() + cmd := serveCmd() + if !strings.Contains(cmd.Long, "cloud") { + t.Fatalf("serve --help should list cloud, got: %q", cmd.Long) + } +} diff --git a/internal/server/meta.go b/internal/server/meta.go index 01f1c9f..5b9550a 100644 --- a/internal/server/meta.go +++ b/internal/server/meta.go @@ -5,6 +5,7 @@ import ( "fmt" "sync" + "github.com/sourcehawk/triagent/pkg/mcp/cloud" "github.com/sourcehawk/triagent/pkg/mcp/git" "github.com/sourcehawk/triagent/pkg/mcp/incidentio" "github.com/sourcehawk/triagent/pkg/mcp/k8s" @@ -107,6 +108,7 @@ func toolCatalog() []MetaTool { specs = append(specs, parallel.ToolSpecs()...) specs = append(specs, prom.ToolSpecs()...) specs = append(specs, teleport.ToolSpecs()...) + specs = append(specs, cloud.ToolSpecs()...) out := make([]MetaTool, 0, len(specs)) for _, s := range specs { ins := make([]MetaToolInput, 0, len(s.Inputs)) From de286d65625feb0bd64279eafeb09332b1df1ec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:55:31 +0200 Subject: [PATCH 8/9] fix(cloud): build an explicit minimal subprocess env instead of inheriting the parent (#45) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit s.run passed nil to execCLI, which makes Go's exec set cmd.Env = nil and inherit the entire parent process environment — violating the spec's minimal-env guarantee and harness.go's own no-leak doc comment. The existing TestExecCLIMinimalEnv passed only because it called execCLI directly with an explicit env; the real caller bypassed that. Add Provider.EnvPassthrough() so each provider declares the env var names its CLI needs forwarded, and build the subprocess env from os.Environ() filtered to the base set plus those names via Server.subprocessEnv. A new test exercises the server-built env: a parent-env canary is dropped while a declared passthrough var survives. Co-Authored-By: Claude Opus 4.8 (1M context) --- pkg/mcp/cloud/fake_test.go | 17 ++++++++++------- pkg/mcp/cloud/provider.go | 6 ++++++ pkg/mcp/cloud/server.go | 32 +++++++++++++++++++++++++++++++- pkg/mcp/cloud/server_test.go | 36 ++++++++++++++++++++++++++++++------ 4 files changed, 77 insertions(+), 14 deletions(-) diff --git a/pkg/mcp/cloud/fake_test.go b/pkg/mcp/cloud/fake_test.go index fc89e87..62593e8 100644 --- a/pkg/mcp/cloud/fake_test.go +++ b/pkg/mcp/cloud/fake_test.go @@ -7,13 +7,14 @@ import "context" // this fake exercises the parent package's harness, tools, and probe without // shelling any real cloud CLI. type fakeProvider struct { - name string - binary string - allowlist *CommandAllowlist - denyFloor DenyFloor - inventory Inventory - identity IdentityStatus - identityErr error + name string + binary string + allowlist *CommandAllowlist + denyFloor DenyFloor + inventory Inventory + identity IdentityStatus + identityErr error + envPassthrough []string } func (f *fakeProvider) Name() string { @@ -39,6 +40,8 @@ func (f *fakeProvider) DefaultAllowlist() *CommandAllowlist { func (f *fakeProvider) DenyFloorAdditions() DenyFloor { return f.denyFloor } +func (f *fakeProvider) EnvPassthrough() []string { return f.envPassthrough } + func (f *fakeProvider) Inventory(context.Context, RunFunc) (Inventory, error) { return f.inventory, nil } diff --git a/pkg/mcp/cloud/provider.go b/pkg/mcp/cloud/provider.go index ad3f0f4..8abf18b 100644 --- a/pkg/mcp/cloud/provider.go +++ b/pkg/mcp/cloud/provider.go @@ -26,6 +26,12 @@ type Provider interface { // the always-on deny floor. The base floor lives in this package; providers // only add to it, never relax it. DenyFloorAdditions() DenyFloor + // EnvPassthrough lists the environment variable NAMES this provider's CLI + // needs forwarded from the launcher-controlled process env to the subprocess + // (base credentials, the pinned-identity impersonation target, config dirs). + // The harness forwards only these plus a minimal base set; every other parent + // env var is dropped, so ambient launcher secrets never reach the CLI. + EnvPassthrough() []string // Inventory projects the provider's accessible scopes (projects for gcp, // accounts for aws). It execs only through run, never directly. Inventory(ctx context.Context, run RunFunc) (Inventory, error) diff --git a/pkg/mcp/cloud/server.go b/pkg/mcp/cloud/server.go index fa22e40..21f6549 100644 --- a/pkg/mcp/cloud/server.go +++ b/pkg/mcp/cloud/server.go @@ -3,11 +3,19 @@ package cloud import ( "context" "fmt" + "os" + "strings" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/sourcehawk/triagent/pkg/mcp/telemetry" ) +// baseEnvPassthrough is the minimal env every provider CLI needs regardless of +// cloud: PATH so the resolved binary can find its own dependencies, HOME so it +// can locate per-user config. Providers add their credential/impersonation +// names via Provider.EnvPassthrough. +var baseEnvPassthrough = []string{"PATH", "HOME"} + // Options configures the cloud-context MCP server. type Options struct { // Provider is the cloud-specific backend (gcp or aws), injected behind the @@ -82,7 +90,29 @@ func (s *Server) run(ctx context.Context, argv []string) (CLIResult, error) { if err := validateArgv(argv, s.allowlist, s.scope); err != nil { return CLIResult{}, err } - return execCLI(ctx, s.provider.Binary(), argv, nil, defaultOutputLimit) + return execCLI(ctx, s.provider.Binary(), argv, s.subprocessEnv(), defaultOutputLimit) +} + +// subprocessEnv builds the explicit, minimal environment for a provider CLI +// invocation: only the base names plus the provider's declared passthrough +// names, read from the launcher-controlled process env. Everything else is +// dropped, so the launcher's ambient secrets never reach the CLI. +func (s *Server) subprocessEnv() []string { + keep := make(map[string]bool, len(baseEnvPassthrough)) + for _, name := range baseEnvPassthrough { + keep[name] = true + } + for _, name := range s.provider.EnvPassthrough() { + keep[name] = true + } + var env []string + for _, kv := range os.Environ() { + name, _, ok := strings.Cut(kv, "=") + if ok && keep[name] { + env = append(env, kv) + } + } + return env } // registerOn wires the cloud tools onto impl. Called from New and from the wire diff --git a/pkg/mcp/cloud/server_test.go b/pkg/mcp/cloud/server_test.go index 2a610df..002cfb0 100644 --- a/pkg/mcp/cloud/server_test.go +++ b/pkg/mcp/cloud/server_test.go @@ -1,13 +1,37 @@ package cloud -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) func TestNewRequiresProvider(t *testing.T) { t.Parallel() - if _, err := New(Options{}); err == nil { - t.Fatal("expected error when Provider is nil") - } - if _, err := New(Options{Provider: &fakeProvider{}}); err != nil { - t.Fatalf("unexpected error: %v", err) + _, err := New(Options{}) + require.Error(t, err, "expected error when Provider is nil") + _, err = New(Options{Provider: &fakeProvider{}}) + require.NoError(t, err) +} + +// TestSubprocessEnvDropsParentSecretsKeepsPassthrough exercises the env the +// server actually builds for run_cli — the path the real harness takes, which +// the isolated execCLI test cannot cover. A parent-env canary must be dropped +// while a declared passthrough var survives, so ambient launcher secrets never +// reach the provider CLI. +func TestSubprocessEnvDropsParentSecretsKeepsPassthrough(t *testing.T) { + t.Setenv("TRIAGENT_CLOUD_LEAK_CANARY", "should-not-appear") + t.Setenv("CLOUDSDK_AUTH_IMPERSONATE_SERVICE_ACCOUNT", "ro-sa@proj.iam.gserviceaccount.com") + p := &fakeProvider{ + envPassthrough: []string{"CLOUDSDK_AUTH_IMPERSONATE_SERVICE_ACCOUNT"}, } + srv := newTestServer(t, p) + + env := srv.subprocessEnv() + + assert.NotContains(t, env, "TRIAGENT_CLOUD_LEAK_CANARY=should-not-appear", + "parent-env secret must be dropped from the subprocess env") + assert.Contains(t, env, "CLOUDSDK_AUTH_IMPERSONATE_SERVICE_ACCOUNT=ro-sa@proj.iam.gserviceaccount.com", + "declared passthrough var must be forwarded") } From 43390a190ac0ff53c76067e6d0ff108c7efe6f9b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 30 May 2026 04:57:44 +0200 Subject: [PATCH 9/9] test(cloud): assert with testify across the cloud package (#45) Convert the scaffold's tests from bare t.Fatal/t.Fatalf/t.Errorf to testify, the repo standard: require for preconditions a failure must stop at (a non-nil error before a dereference, setup that must succeed), assert for independent checks that should keep running. Assertion intent is preserved exactly; no security assertion is weakened, and the harness_security_test source-scan logic (reading harness.go bytes) stays intact. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/triagent-mcp/serve_cloud_test.go | 32 +++++----------- pkg/mcp/cloud/allowlist_test.go | 52 ++++++++++---------------- pkg/mcp/cloud/harness_security_test.go | 50 +++++++++---------------- pkg/mcp/cloud/harness_test.go | 11 +++--- pkg/mcp/cloud/probe_test.go | 36 ++++++------------ pkg/mcp/cloud/validate_test.go | 35 +++++++++-------- 6 files changed, 80 insertions(+), 136 deletions(-) diff --git a/cmd/triagent-mcp/serve_cloud_test.go b/cmd/triagent-mcp/serve_cloud_test.go index a427db1..32504a0 100644 --- a/cmd/triagent-mcp/serve_cloud_test.go +++ b/cmd/triagent-mcp/serve_cloud_test.go @@ -2,47 +2,35 @@ package main import ( "context" - "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRunServe_CloudKindRequiresProvider(t *testing.T) { t.Parallel() err := runServe(context.Background(), serveFlags{kind: "cloud"}) - if err == nil { - t.Fatal("expected error when --provider is missing") - } - if !strings.Contains(err.Error(), "provider") { - t.Fatalf("error should mention --provider, got: %v", err) - } + require.Error(t, err, "expected error when --provider is missing") + assert.Contains(t, err.Error(), "provider", "error should mention --provider") } func TestRunServe_CloudKindRejectsUnknownProvider(t *testing.T) { t.Parallel() err := runServe(context.Background(), serveFlags{kind: "cloud", cloudProvider: "azure"}) - if err == nil { - t.Fatal("expected error for an unknown provider") - } - if !strings.Contains(err.Error(), "azure") { - t.Fatalf("error should name the rejected provider, got: %v", err) - } + require.Error(t, err, "expected error for an unknown provider") + assert.Contains(t, err.Error(), "azure", "error should name the rejected provider") } func TestRunServe_UnknownKindErrorListsCloud(t *testing.T) { t.Parallel() err := runServe(context.Background(), serveFlags{kind: "bogus"}) - if err == nil { - t.Fatal("expected error for unknown kind") - } - if !strings.Contains(err.Error(), "cloud") { - t.Fatalf("kind list should include cloud, got: %v", err) - } + require.Error(t, err, "expected error for unknown kind") + assert.Contains(t, err.Error(), "cloud", "kind list should include cloud") } func TestServeCmd_KnowsCloudKind(t *testing.T) { t.Parallel() cmd := serveCmd() - if !strings.Contains(cmd.Long, "cloud") { - t.Fatalf("serve --help should list cloud, got: %q", cmd.Long) - } + assert.Contains(t, cmd.Long, "cloud", "serve --help should list cloud") } diff --git a/pkg/mcp/cloud/allowlist_test.go b/pkg/mcp/cloud/allowlist_test.go index 6bd66fa..1dbd512 100644 --- a/pkg/mcp/cloud/allowlist_test.go +++ b/pkg/mcp/cloud/allowlist_test.go @@ -4,14 +4,15 @@ import ( "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func writeTemp(t *testing.T, body string) string { t.Helper() p := filepath.Join(t.TempDir(), "allowlist.json") - if err := os.WriteFile(p, []byte(body), 0o600); err != nil { - t.Fatal(err) - } + require.NoError(t, os.WriteFile(p, []byte(body), 0o600)) return p } @@ -20,15 +21,10 @@ func TestLoadCommandAllowlistDropsDenyFloor(t *testing.T) { // JSON that tries to allow a deny-floored subcommand. path := writeTemp(t, `{"commands":[{"path":"projects list"},{"path":"secrets versions access"}]}`) al, err := LoadCommandAllowlist(path, DenyFloor{}) - if err != nil { - t.Fatal(err) - } - if al.Allows([]string{"secrets", "versions", "access"}) { - t.Fatal("deny floor must drop secrets access regardless of config") - } - if !al.Allows([]string{"projects", "list"}) { - t.Fatal("projects list should be allowed") - } + require.NoError(t, err) + assert.False(t, al.Allows([]string{"secrets", "versions", "access"}), + "deny floor must drop secrets access regardless of config") + assert.True(t, al.Allows([]string{"projects", "list"}), "projects list should be allowed") } func TestLoadCommandAllowlistUsesEmbeddedDefaultWhenPathEmpty(t *testing.T) { @@ -36,12 +32,8 @@ func TestLoadCommandAllowlistUsesEmbeddedDefaultWhenPathEmpty(t *testing.T) { // The parent package ships no provider commands of its own; an empty path // yields the empty embedded default, not an error. al, err := LoadCommandAllowlist("", DenyFloor{}) - if err != nil { - t.Fatal(err) - } - if al == nil { - t.Fatal("expected a non-nil allowlist for the empty default") - } + require.NoError(t, err) + assert.NotNil(t, al, "expected a non-nil allowlist for the empty default") } func TestLoadCommandAllowlistMergesProviderDenyFloorAdditions(t *testing.T) { @@ -49,24 +41,18 @@ func TestLoadCommandAllowlistMergesProviderDenyFloorAdditions(t *testing.T) { path := writeTemp(t, `{"commands":[{"path":"compute instances list"},{"path":"compute ssh foo"}]}`) extra := DenyFloor{Subcommands: []string{"compute ssh"}} al, err := LoadCommandAllowlist(path, extra) - if err != nil { - t.Fatal(err) - } - if al.Allows([]string{"compute", "ssh", "foo"}) { - t.Fatal("provider deny-floor addition must drop compute ssh") - } - if !al.Allows([]string{"compute", "instances", "list"}) { - t.Fatal("compute instances list should remain allowed") - } + require.NoError(t, err) + assert.False(t, al.Allows([]string{"compute", "ssh", "foo"}), + "provider deny-floor addition must drop compute ssh") + assert.True(t, al.Allows([]string{"compute", "instances", "list"}), + "compute instances list should remain allowed") } func TestAllowsMatchesLongestPathPrefix(t *testing.T) { t.Parallel() al := &CommandAllowlist{Commands: []Command{{Path: "compute firewall-rules list"}}} - if !al.Allows([]string{"compute", "firewall-rules", "list", "--project", "prod"}) { - t.Fatal("argv whose leading tokens match an allowed path should pass") - } - if al.Allows([]string{"compute", "firewall-rules", "delete"}) { - t.Fatal("a different verb under the same group must not be allowed") - } + assert.True(t, al.Allows([]string{"compute", "firewall-rules", "list", "--project", "prod"}), + "argv whose leading tokens match an allowed path should pass") + assert.False(t, al.Allows([]string{"compute", "firewall-rules", "delete"}), + "a different verb under the same group must not be allowed") } diff --git a/pkg/mcp/cloud/harness_security_test.go b/pkg/mcp/cloud/harness_security_test.go index c071814..13a6a98 100644 --- a/pkg/mcp/cloud/harness_security_test.go +++ b/pkg/mcp/cloud/harness_security_test.go @@ -6,6 +6,9 @@ import ( "os" "strings" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestExecCLINeverUsesShell is the source-level half of the no-shell guarantee: @@ -14,13 +17,10 @@ import ( func TestExecCLINeverUsesShell(t *testing.T) { t.Parallel() src, err := os.ReadFile("harness.go") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) for _, banned := range []string{`"-c"`, "sh -c", "bash -c", `"sh"`, `"bash"`} { - if bytes.Contains(src, []byte(banned)) { - t.Fatalf("harness.go must never construct a shell command; found %q", banned) - } + assert.False(t, bytes.Contains(src, []byte(banned)), + "harness.go must never construct a shell command; found %q", banned) } } @@ -32,32 +32,21 @@ func TestExecCLIMetacharactersAreInert(t *testing.T) { t.Parallel() argv := []string{";", "echo", "pwned", "|", "$(whoami)", "&&", "`id`"} r, err := execCLI(context.Background(), "/bin/echo", argv, nil, 4096) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) got := strings.TrimRight(r.Stdout, "\n") want := strings.Join(argv, " ") - if got != want { - t.Fatalf("metacharacters were not inert: got %q want %q", got, want) - } - if strings.Contains(r.Stdout, "pwned\n") && got != want { - t.Fatal("a second process appears to have run") - } + require.Equal(t, want, got, "metacharacters were not inert") + assert.False(t, strings.Contains(r.Stdout, "pwned\n") && got != want, + "a second process appears to have run") } // TestExecCLITruncates caps output at the byte limit and flags truncation. func TestExecCLITruncates(t *testing.T) { t.Parallel() r, err := execCLI(context.Background(), "/bin/echo", []string{strings.Repeat("x", 100)}, nil, 10) - if err != nil { - t.Fatal(err) - } - if !r.Truncated { - t.Fatalf("expected Truncated, got %+v", r) - } - if len(r.Stdout) > 10 { - t.Fatalf("output exceeded limit: got %d bytes", len(r.Stdout)) - } + require.NoError(t, err) + assert.True(t, r.Truncated, "expected Truncated, got %+v", r) + assert.LessOrEqual(t, len(r.Stdout), 10, "output exceeded limit") } // TestExecCLIMinimalEnv confirms the subprocess runs with the caller's explicit @@ -66,13 +55,8 @@ func TestExecCLITruncates(t *testing.T) { func TestExecCLIMinimalEnv(t *testing.T) { t.Setenv("TRIAGENT_CLOUD_HARNESS_LEAK_CANARY", "should-not-appear") r, err := execCLI(context.Background(), "/usr/bin/env", nil, []string{"FOO=bar"}, 4096) - if err != nil { - t.Fatal(err) - } - if strings.Contains(r.Stdout, "TRIAGENT_CLOUD_HARNESS_LEAK_CANARY") { - t.Fatal("subprocess inherited the parent environment; env must be explicit") - } - if !strings.Contains(r.Stdout, "FOO=bar") { - t.Fatalf("explicit env not applied: %q", r.Stdout) - } + require.NoError(t, err) + assert.NotContains(t, r.Stdout, "TRIAGENT_CLOUD_HARNESS_LEAK_CANARY", + "subprocess inherited the parent environment; env must be explicit") + assert.Contains(t, r.Stdout, "FOO=bar", "explicit env not applied") } diff --git a/pkg/mcp/cloud/harness_test.go b/pkg/mcp/cloud/harness_test.go index 73c8afd..9730e34 100644 --- a/pkg/mcp/cloud/harness_test.go +++ b/pkg/mcp/cloud/harness_test.go @@ -3,6 +3,9 @@ package cloud import ( "context" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestExecCLIExitCode surfaces the child's exit code without treating a @@ -11,10 +14,6 @@ import ( func TestExecCLIExitCode(t *testing.T) { t.Parallel() r, err := execCLI(context.Background(), "/bin/false", nil, nil, 4096) - if err != nil { - t.Fatalf("non-zero exit should not be a Go error: %v", err) - } - if r.ExitCode != 1 { - t.Fatalf("expected exit code 1, got %d", r.ExitCode) - } + require.NoError(t, err, "non-zero exit should not be a Go error") + assert.Equal(t, 1, r.ExitCode) } diff --git a/pkg/mcp/cloud/probe_test.go b/pkg/mcp/cloud/probe_test.go index 99c996d..e94aa39 100644 --- a/pkg/mcp/cloud/probe_test.go +++ b/pkg/mcp/cloud/probe_test.go @@ -4,6 +4,9 @@ import ( "context" "errors" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestProbeReturnsProviderIdentity(t *testing.T) { @@ -17,40 +20,25 @@ func TestProbeReturnsProviderIdentity(t *testing.T) { }, } st, err := Probe(context.Background(), p) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if !st.Valid || st.AssumedIdentity != "ro-sa@proj.iam.gserviceaccount.com" { - t.Fatalf("probe did not return the provider identity: %+v", st) - } + require.NoError(t, err) + assert.True(t, st.Valid) + assert.Equal(t, "ro-sa@proj.iam.gserviceaccount.com", st.AssumedIdentity) } func TestProbeSurfacesProviderErrorAsInvalid(t *testing.T) { t.Parallel() p := &fakeProvider{name: "aws", identityErr: errors.New("token expired")} st, err := Probe(context.Background(), p) - if err != nil { - t.Fatalf("Probe should degrade, not error: %v", err) - } - if st.Valid { - t.Fatal("expected Valid=false when the provider errors") - } - if st.Provider != "aws" { - t.Fatalf("expected provider name carried through, got %q", st.Provider) - } - if st.Hint == "" { - t.Fatal("expected the provider error surfaced as a hint") - } + require.NoError(t, err, "Probe should degrade, not error") + assert.False(t, st.Valid, "expected Valid=false when the provider errors") + assert.Equal(t, "aws", st.Provider, "expected provider name carried through") + assert.NotEmpty(t, st.Hint, "expected the provider error surfaced as a hint") } func TestProbeInvalidWhenIdentityEmpty(t *testing.T) { t.Parallel() p := &fakeProvider{name: "gcp", identity: IdentityStatus{Provider: "gcp", Valid: true}} st, err := Probe(context.Background(), p) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if st.Valid { - t.Fatal("an empty resolved identity must not be reported valid") - } + require.NoError(t, err) + assert.False(t, st.Valid, "an empty resolved identity must not be reported valid") } diff --git a/pkg/mcp/cloud/validate_test.go b/pkg/mcp/cloud/validate_test.go index 9dfdbbc..ba77a32 100644 --- a/pkg/mcp/cloud/validate_test.go +++ b/pkg/mcp/cloud/validate_test.go @@ -1,6 +1,10 @@ package cloud -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestValidateArgvRejectsDenyFloorAndScope(t *testing.T) { t.Parallel() @@ -31,11 +35,10 @@ func TestValidateArgvRejectsDenyFloorAndScope(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { err := validateArgv(tc.argv, al, scope) - if tc.ok && err != nil { - t.Fatalf("expected argv to validate, got error: %v", err) - } - if !tc.ok && err == nil { - t.Fatal("expected validation error, got nil") + if tc.ok { + assert.NoError(t, err, "expected argv to validate") + } else { + assert.Error(t, err, "expected validation error") } }) } @@ -47,15 +50,12 @@ func TestValidateArgvEqualsFormFlag(t *testing.T) { scope := ScopeAllowlist{Projects: []string{"prod"}} // --project=other in equals form must be caught by the scope check, and a // deny-floored flag in equals form must be caught by the floor. - if err := validateArgv([]string{"compute", "instances", "list", "--project=other"}, al, scope); err == nil { - t.Fatal("expected --project=other (equals form) to fail the scope check") - } - if err := validateArgv([]string{"compute", "instances", "list", "--impersonate-service-account=x"}, al, scope); err == nil { - t.Fatal("expected --impersonate-service-account=x (equals form) to be denied") - } - if err := validateArgv([]string{"compute", "instances", "list", "--project=prod"}, al, scope); err != nil { - t.Fatalf("expected --project=prod (equals form, in scope) to validate, got: %v", err) - } + assert.Error(t, validateArgv([]string{"compute", "instances", "list", "--project=other"}, al, scope), + "expected --project=other (equals form) to fail the scope check") + assert.Error(t, validateArgv([]string{"compute", "instances", "list", "--impersonate-service-account=x"}, al, scope), + "expected --impersonate-service-account=x (equals form) to be denied") + assert.NoError(t, validateArgv([]string{"compute", "instances", "list", "--project=prod"}, al, scope), + "expected --project=prod (equals form, in scope) to validate") } func TestValidateArgvEmptyScopeAllowsAnyTarget(t *testing.T) { @@ -63,7 +63,6 @@ func TestValidateArgvEmptyScopeAllowsAnyTarget(t *testing.T) { al := &CommandAllowlist{Commands: []Command{{Path: "projects list"}}} // An empty scope means the deployment did not constrain targets; the scope // check must not reject a --project then. - if err := validateArgv([]string{"projects", "list", "--project", "anything"}, al, ScopeAllowlist{}); err != nil { - t.Fatalf("empty scope should not reject a target, got: %v", err) - } + assert.NoError(t, validateArgv([]string{"projects", "list", "--project", "anything"}, al, ScopeAllowlist{}), + "empty scope should not reject a target") }