diff --git a/CLAUDE.md b/CLAUDE.md index 4a3e943..2ea9ff4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -114,6 +114,15 @@ This project follows DDD layered architecture with dependency injection **strict 2. Add the entry to `pkg/clients/builtins.go`. 3. Add a Dockerfile in `images//` and a Taskfile target `image-` (and include it in `image-all`). +### Custom (bring-your-own) agents — no Go code + +A custom agent can be declared entirely in global config under `agents:` (no `pkg/clients/` package, no Dockerfile in this repo). `config.AgentFromOverride` (pure, `pkg/domain/config`) maps the `AgentOverride` into an `agent.Agent`; `config.ValidateCustomAgent` runs the load-time checks (the image-ref parser is injected as a closure from the composition root to keep the domain free of go-containerregistry). Manage them with `bbox agents list|inspect|doctor`. + +- **Safer defaults than built-ins**: `env_forward` empty (forward nothing), egress profile `standard` (must declare `egress_hosts` for it or set `permissive`), MCP authz `safe-tools` when MCP is enabled. +- **Universal `BBOX_*` env** (`pkg/domain/agent.BuildUniversalEnv`) is injected into *every* VM — `BBOX_AGENT_NAME`, `BBOX_WORKSPACE`, `BBOX_HOME`, `BBOX_SESSION_ID`, `BBOX_GIT_TOKEN_AVAILABLE`, `BBOX_SSH_AGENT_AVAILABLE`, plus `BBOX_MCP_URL`/`BBOX_MCP_AUTHZ_PROFILE` when MCP is active. Applied **after** forwarded host vars so it is authoritative (the env_forward allowlist can't clobber it). `BBOX_MCP_URL` uses `config.MCPEndpointPath` (`/mcp`) — the single source of truth shared by the proxy and all clients. +- **`mcp.mode: env`**: enables the proxy but runs no config-file injector; the agent discovers it via `BBOX_MCP_URL`. (`mcp.mode: config` is not yet supported.) +- **Security**: workspace-local `.broodbox.yaml` can **never** add a custom agent or introduce new credential paths, and can't repoint an existing agent's image/command or widen its `env_forward` — local config is tighten-only (`mergeAgentOverride`). + ## Conventions - **SPDX headers required** on every `.go` and `.yaml` file: diff --git a/cmd/bbox/agents.go b/cmd/bbox/agents.go new file mode 100644 index 0000000..43dc728 --- /dev/null +++ b/cmd/bbox/agents.go @@ -0,0 +1,466 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "errors" + "fmt" + "io" + "log/slog" + "os" + "sort" + "strings" + + "github.com/spf13/cobra" + + "github.com/stacklok/brood-box/pkg/clients" + domainagent "github.com/stacklok/brood-box/pkg/domain/agent" + domainconfig "github.com/stacklok/brood-box/pkg/domain/config" + "github.com/stacklok/brood-box/pkg/domain/egress" + "github.com/stacklok/brood-box/pkg/domain/settings" +) + +// agentsCmd is the `bbox agents` command group: list, inspect, doctor. +func agentsCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "agents", + Short: "Inspect and validate available agents", + } + cmd.AddCommand(agentsListCmd()) + cmd.AddCommand(agentsInspectCmd()) + cmd.AddCommand(agentsDoctorCmd()) + return cmd +} + +func agentsListCmd() *cobra.Command { + var cfgPath string + cmd := &cobra.Command{ + Use: "list", + Short: "List available agents (built-in and custom)", + RunE: func(cmd *cobra.Command, _ []string) error { + return runAgentsList(cmd, cfgPath) + }, + } + cmd.Flags().StringVar(&cfgPath, "config", "", "Config file path (default: ~/.config/broodbox/config.yaml)") + return cmd +} + +// runAgentsList resolves the registry (built-ins + custom agents from config) +// and prints each agent's name and image. Shared by `bbox list` and +// `bbox agents list`. +func runAgentsList(cmd *cobra.Command, cfgPath string) error { + ws, err := os.Getwd() + if err != nil { + return fmt.Errorf("getting current directory: %w", err) + } + resolved, err := buildResolvedRegistry(cfgPath, ws, slog.Default(), io.Discard) + if err != nil { + return err + } + out := cmd.OutOrStdout() + for _, e := range resolved.registry.List() { + _, _ = fmt.Fprintf(out, "%-15s %s\n", e.Agent.Name, e.Agent.Image) + } + return nil +} + +func agentsInspectCmd() *cobra.Command { + var cfgPath string + cmd := &cobra.Command{ + Use: "inspect ", + Short: "Show an agent's resolved configuration and field sources", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return runAgentsInspect(cmd, cfgPath, args[0]) + }, + } + cmd.Flags().StringVar(&cfgPath, "config", "", "Config file path (default: ~/.config/broodbox/config.yaml)") + return cmd +} + +func runAgentsInspect(cmd *cobra.Command, cfgPath, agentName string) error { + ws, err := os.Getwd() + if err != nil { + return fmt.Errorf("getting current directory: %w", err) + } + resolved, err := buildResolvedRegistry(cfgPath, ws, slog.Default(), io.Discard) + if err != nil { + return err + } + + entry, err := resolved.registry.Get(agentName) + if err != nil { + // Not registered. If it is a custom agent declared in the merged config + // (skipped during registration because it failed validation), build a + // best-effort view from the override so the user can still see what they + // declared. Direct them to `agents doctor` for the failure detail. + override, declared := resolved.merged.Agents[agentName] + if !declared { + return fmt.Errorf("unknown agent %q", agentName) + } + ag, mapErr := domainconfig.AgentFromOverride(agentName, override, resolved.merged.Defaults) + if mapErr != nil { + return fmt.Errorf("agent %q is misconfigured: %w (run 'bbox agents doctor %s' for details)", agentName, mapErr, agentName) + } + out := cmd.OutOrStdout() + _, _ = fmt.Fprintf(out, "Warning: agent %q is not registered (failed validation) — run 'bbox agents doctor %s' for details\n\n", agentName, agentName) + renderInspect(out, ag, resolved, agentName) + return nil + } + + out := cmd.OutOrStdout() + renderInspect(out, entry.Agent, resolved, agentName) + return nil +} + +// renderInspect prints the resolved agent fields with provenance. Security: +// environment VALUES are never printed — only variable names/patterns and a +// present/missing indicator for required vars. +func renderInspect(w io.Writer, ag domainagent.Agent, resolved *resolvedRegistry, agentName string) { + _, isBuiltin := builtinNames()[agentName] + src := func() string { + if isBuiltin { + return "built-in" + } + return "global" + } + + override := resolved.merged.Agents[agentName] + + _, _ = fmt.Fprintf(w, "Name: %s\n", ag.Name) + _, _ = fmt.Fprintf(w, "Image: %s [%s]\n", ag.Image, fieldSourceImage(agentName, resolved, isBuiltin)) + _, _ = fmt.Fprintf(w, "Command: %s [%s]\n", strings.Join(ag.Command, " "), src()) + if override.Description != "" { + _, _ = fmt.Fprintf(w, "Description: %s\n", override.Description) + } + + // Env: names/patterns only. + _, _ = fmt.Fprintf(w, "Env forward: %s\n", joinOrNone(ag.EnvForward)) + _, _ = fmt.Fprintf(w, "Default env: %s\n", joinOrNone(sortedKeys(ag.DefaultEnv))) + if len(override.EnvRequired) > 0 { + _, _ = fmt.Fprintf(w, "Required env:\n") + for _, name := range override.EnvRequired { + _, present := os.LookupEnv(name) + status := "MISSING" + if present { + status = "present" + } + _, _ = fmt.Fprintf(w, " %-30s %s\n", name, status) + } + } + + // Resources. + _, _ = fmt.Fprintf(w, "Resources: %d CPUs, %s memory, %s /tmp\n", + ag.DefaultCPUs, ag.DefaultMemory, ag.DefaultTmpSize) + + // Egress. + _, _ = fmt.Fprintf(w, "Egress: profile=%s\n", ag.DefaultEgressProfile) + for _, profile := range sortedProfiles(ag.EgressHosts) { + names := make([]string, 0, len(ag.EgressHosts[profile])) + for _, h := range ag.EgressHosts[profile] { + names = append(names, h.Name) + } + _, _ = fmt.Fprintf(w, " %-12s %s\n", string(profile)+":", strings.Join(names, ", ")) + } + + // MCP. + mode := "" + authz := "" + enabled := "inherit" + mcpDisabled := false + if override.MCP != nil { + mode = override.MCP.Mode + if override.MCP.Enabled != nil { + if *override.MCP.Enabled { + enabled = "true" + } else { + enabled = "false" + mcpDisabled = true + } + } + if override.MCP.Authz != nil { + authz = override.MCP.Authz.Profile + } + } + // A custom agent with MCP enabled and no explicit authz runs under the + // safe-tools default applied at the composition root (built-ins keep + // full-access). Surface that effective default here so inspect reflects + // the profile the agent will actually run with. + if authz == "" && !isBuiltin && !mcpDisabled { + authz = domainconfig.DefaultCustomAgentMCPAuthzProfile + " (default)" + } + _, _ = fmt.Fprintf(w, "MCP: enabled=%s mode=%q authz=%q\n", enabled, mode, authz) + + // Credential paths. + _, _ = fmt.Fprintf(w, "Credentials: %s\n", joinOrNone(ag.CredentialPaths)) + + // Settings mappings. + if ag.SettingsManifest != nil && len(ag.SettingsManifest.Entries) > 0 { + _, _ = fmt.Fprintf(w, "Settings:\n") + for _, e := range ag.SettingsManifest.Entries { + _, _ = fmt.Fprintf(w, " [%s] %s -> %s (%s%s%s)\n", + e.Category, e.HostPath, e.GuestPath, kindString(e.Kind), + formatSuffix(e.Format), optionalSuffix(e.Optional)) + } + } else { + _, _ = fmt.Fprintf(w, "Settings: (none)\n") + } +} + +func agentsDoctorCmd() *cobra.Command { + var cfgPath string + cmd := &cobra.Command{ + Use: "doctor ", + Short: "Validate an agent's configuration", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return runAgentsDoctor(cmd, cfgPath, args[0]) + }, + } + cmd.Flags().StringVar(&cfgPath, "config", "", "Config file path (default: ~/.config/broodbox/config.yaml)") + return cmd +} + +func runAgentsDoctor(cmd *cobra.Command, cfgPath, agentName string) error { + ws, err := os.Getwd() + if err != nil { + return fmt.Errorf("getting current directory: %w", err) + } + resolved, err := buildResolvedRegistry(cfgPath, ws, slog.Default(), io.Discard) + if err != nil { + return err + } + + // A misconfigured custom agent is SKIPPED during registration (registerCustomAgents + // warns and drops it), so it is absent from the registry. Diagnosing exactly + // that case is the whole point of doctor — so do not hard-gate on the registry. + // Only reject names that are neither registered nor declared in the merged config. + _, regErr := resolved.registry.Get(agentName) + _, declared := resolved.merged.Agents[agentName] + if regErr != nil && !declared { + return fmt.Errorf("unknown agent %q", agentName) + } + + override := resolved.merged.Agents[agentName] + localAddedAgent := didLocalAddAgent(resolved, agentName) + localAddedCreds := didLocalAddCredentials(resolved, agentName) + _, isBuiltin := builtinNames()[agentName] + + results, ok := runDoctor(agentName, override, isBuiltin, os.LookupEnv, imageRefValidator, localAddedAgent, localAddedCreds) + + out := cmd.OutOrStdout() + for _, r := range results { + status := "PASS" + if !r.ok { + status = "FAIL" + } + _, _ = fmt.Fprintf(out, "[%s] %s\n", status, r.message) + } + if !ok { + return errors.New("agent configuration has problems (see above)") + } + return nil +} + +// checkResult is a single doctor check outcome. +type checkResult struct { + ok bool + message string +} + +// runDoctor performs all agent health checks against the resolved override. It +// is pure (the environment and image-ref lookups are injected) so it can be +// exercised directly in tests without touching the real config tree. It +// returns the per-check results and an overall ok flag (false if any FAIL). +// +// The custom-agent identity checks (name/image/command/paths/egress via +// ValidateCustomAgent) only apply to custom (bring-your-own) agents declared +// in config. Built-in agents carry their image/command in the registry entry, +// not in the override, so feeding them through ValidateCustomAgent would +// spuriously fail on the empty Image. For built-ins we skip those checks and +// only run the override-applicable checks (required env, blocked local +// additions). +// +// lookupEnv mirrors os.LookupEnv; imageValidator mirrors imageRefValidator. +// localAddedAgent / localAddedCredentials report whether the workspace-local +// config attempted the (now-blocked) unsafe additions, surfaced as FAILs. +func runDoctor( + agentName string, + override domainconfig.AgentOverride, + isBuiltin bool, + lookupEnv func(string) (string, bool), + imageValidator func(string) error, + localAddedAgent bool, + localAddedCredentials bool, +) ([]checkResult, bool) { + var results []checkResult + add := func(ok bool, msg string) { + results = append(results, checkResult{ok: ok, message: msg}) + } + + if isBuiltin { + // Built-in agents are valid registered agents whose identity comes from + // their plugin/registry entry, not from config. There is no custom-agent + // definition to validate. + add(true, "built-in agent: identity provided by the registry (no custom config to validate)") + } else if err := domainconfig.ValidateCustomAgent(agentName, override, imageValidator); err != nil { + // The bulk of validation is the same pure check the loader runs. + add(false, fmt.Sprintf("config validation: %s", err)) + } else { + add(true, "config validation: name, image, command, paths, egress all valid") + } + + // Required env presence (names only — values never read into output). + for _, name := range override.EnvRequired { + _, present := lookupEnv(name) + if present { + add(true, fmt.Sprintf("required env %s present", name)) + } else { + add(false, fmt.Sprintf("required env %s is missing", name)) + } + } + + // Security #7 surfacing. + if localAddedAgent { + add(false, "workspace .broodbox.yaml attempted to add this agent (ignored)") + } + if localAddedCredentials { + add(false, "workspace .broodbox.yaml attempted to add credential paths (ignored)") + } + + ok := true + for _, r := range results { + if !r.ok { + ok = false + } + } + return results, ok +} + +// fieldSource returns a provenance label by comparing the layered values for a +// field. The layers are checked most-specific first: a non-zero CLI value wins, +// then workspace-local, then global, otherwise built-in. The caller passes +// whether each layer set the field (the comparison is field-specific). +func fieldSource(setByCLI, setByWorkspace, setByGlobal bool) string { + switch { + case setByCLI: + return "CLI" + case setByWorkspace: + return "workspace" + case setByGlobal: + return "global" + default: + return "built-in" + } +} + +// fieldSourceImage attributes the image field for the inspect output. +func fieldSourceImage(agentName string, resolved *resolvedRegistry, isBuiltin bool) string { + setByGlobal := false + if o, ok := resolved.global.Agents[agentName]; ok && o.Image != "" { + setByGlobal = true + } + setByWorkspace := false + if resolved.local != nil { + if o, ok := resolved.local.Agents[agentName]; ok && o.Image != "" { + setByWorkspace = true + } + } + if !setByGlobal && !setByWorkspace && isBuiltin { + return "built-in" + } + return fieldSource(false, setByWorkspace, setByGlobal) +} + +// didLocalAddAgent reports whether the workspace-local config declared an +// agent key that is absent from the global config (a blocked attempt to add a +// new custom agent). +func didLocalAddAgent(resolved *resolvedRegistry, agentName string) bool { + if resolved.local == nil { + return false + } + _, inLocal := resolved.local.Agents[agentName] + if !inLocal { + return false + } + _, inGlobal := resolved.global.Agents[agentName] + _, isBuiltin := builtinNames()[agentName] + return !inGlobal && !isBuiltin +} + +// didLocalAddCredentials reports whether the workspace-local config declared +// credential paths for an agent (a blocked attempt — local credentials are +// always ignored during merge). +func didLocalAddCredentials(resolved *resolvedRegistry, agentName string) bool { + if resolved.local == nil { + return false + } + o, ok := resolved.local.Agents[agentName] + if !ok { + return false + } + return o.Credentials != nil && len(o.Credentials.Persist) > 0 +} + +// builtinNames returns the set of built-in agent names so the inspect/doctor +// provenance logic can tell built-in agents apart from custom ones. +func builtinNames() map[string]struct{} { + names := make(map[string]struct{}) + for _, e := range clients.Builtins(slog.Default()) { + names[e.Agent.Name] = struct{}{} + } + return names +} + +// --- small rendering helpers --- + +func joinOrNone(items []string) string { + if len(items) == 0 { + return "(none)" + } + return strings.Join(items, ", ") +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +func sortedProfiles(m map[egress.ProfileName][]egress.Host) []egress.ProfileName { + profiles := make([]egress.ProfileName, 0, len(m)) + for p := range m { + profiles = append(profiles, p) + } + sort.Slice(profiles, func(i, j int) bool { return profiles[i] < profiles[j] }) + return profiles +} + +func kindString(k settings.EntryKind) string { + switch k { + case settings.KindDirectory: + return "directory" + case settings.KindMergeFile: + return "merge-file" + default: + return "file" + } +} + +func formatSuffix(format string) string { + if format == "" { + return "" + } + return ", " + format +} + +func optionalSuffix(optional bool) string { + if optional { + return ", optional" + } + return "" +} diff --git a/cmd/bbox/agents_test.go b/cmd/bbox/agents_test.go new file mode 100644 index 0000000..db65bff --- /dev/null +++ b/cmd/bbox/agents_test.go @@ -0,0 +1,356 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + domainagent "github.com/stacklok/brood-box/pkg/domain/agent" + domainconfig "github.com/stacklok/brood-box/pkg/domain/config" + "github.com/stacklok/brood-box/pkg/domain/egress" + "github.com/stacklok/brood-box/pkg/domain/settings" +) + +func TestFieldSource(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cli, workspace, global, wantLabel string + setCLI, setWS, setGlobal bool + want string + }{ + {name: "cli wins", setCLI: true, setWS: true, setGlobal: true, want: "CLI"}, + {name: "workspace beats global", setWS: true, setGlobal: true, want: "workspace"}, + {name: "global", setGlobal: true, want: "global"}, + {name: "built-in fallback", want: "built-in"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, fieldSource(tt.setCLI, tt.setWS, tt.setGlobal)) + }) + } +} + +func TestRunDoctor(t *testing.T) { + t.Parallel() + + validOverride := domainconfig.AgentOverride{ + Image: "ghcr.io/acme/agent:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + EnvRequired: []string{"ACME_API_KEY"}, + } + + t.Run("clean config with present env passes", func(t *testing.T) { + t.Parallel() + lookup := func(string) (string, bool) { return "secret", true } + results, ok := runDoctor("agent", validOverride, false, lookup, imageRefValidator, false, false) + assert.True(t, ok) + require.NotEmpty(t, results) + }) + + t.Run("missing required env fails with clear message", func(t *testing.T) { + t.Parallel() + lookup := func(string) (string, bool) { return "", false } + results, ok := runDoctor("agent", validOverride, false, lookup, imageRefValidator, false, false) + assert.False(t, ok) + assert.True(t, containsMessage(results, "required env ACME_API_KEY is missing"), + "missing required env must report 'is missing', not 'present'") + assert.False(t, containsMessage(results, "required env ACME_API_KEY present")) + }) + + t.Run("present required env reports present", func(t *testing.T) { + t.Parallel() + lookup := func(string) (string, bool) { return "v", true } + results, _ := runDoctor("agent", validOverride, false, lookup, imageRefValidator, false, false) + assert.True(t, containsMessage(results, "required env ACME_API_KEY present")) + }) + + t.Run("invalid image ref fails", func(t *testing.T) { + t.Parallel() + bad := validOverride + bad.Image = "::::not a ref::::" + lookup := func(string) (string, bool) { return "v", true } + _, ok := runDoctor("agent", bad, false, lookup, imageRefValidator, false, false) + assert.False(t, ok) + }) + + t.Run("local-added agent flagged", func(t *testing.T) { + t.Parallel() + lookup := func(string) (string, bool) { return "v", true } + results, ok := runDoctor("agent", validOverride, false, lookup, imageRefValidator, true, false) + assert.False(t, ok) + assert.True(t, containsMessage(results, "attempted to add this agent")) + }) + + t.Run("local-added credentials flagged", func(t *testing.T) { + t.Parallel() + lookup := func(string) (string, bool) { return "v", true } + results, ok := runDoctor("agent", validOverride, false, lookup, imageRefValidator, false, true) + assert.False(t, ok) + assert.True(t, containsMessage(results, "credential paths")) + }) + + t.Run("built-in agent passes despite empty override", func(t *testing.T) { + t.Parallel() + // A built-in agent carries its image/command in the registry entry, + // not in the override. ValidateCustomAgent must be skipped so the + // empty Image does not trigger a spurious failure. + lookup := func(string) (string, bool) { return "", false } + results, ok := runDoctor("claude-code", domainconfig.AgentOverride{}, true, lookup, imageRefValidator, false, false) + assert.True(t, ok, "built-in agent with empty override must not fail validation") + assert.False(t, containsMessage(results, "image is required")) + assert.True(t, containsMessage(results, "built-in agent")) + }) + + t.Run("built-in agent still flags missing required env", func(t *testing.T) { + t.Parallel() + override := domainconfig.AgentOverride{EnvRequired: []string{"ACME_API_KEY"}} + lookup := func(string) (string, bool) { return "", false } + results, ok := runDoctor("claude-code", override, true, lookup, imageRefValidator, false, false) + assert.False(t, ok) + assert.True(t, containsMessage(results, "required env ACME_API_KEY is missing")) + }) +} + +// TestRenderInspect_NeverLeaksEnvValues asserts that inspect output never +// contains an environment variable's VALUE — only names/patterns. +func TestRenderInspect_NeverLeaksEnvValues(t *testing.T) { + const secret = "super-secret-token-value" + t.Setenv("ACME_API_KEY", secret) + + ag := domainagent.Agent{ + Name: "acme", + Image: "ghcr.io/acme/agent:latest", + Command: []string{"run"}, + EnvForward: []string{"ACME_API_KEY", "ACME_*"}, + DefaultEnv: map[string]string{"ACME_MODE": secret}, + DefaultEgressProfile: egress.ProfileStandard, + EgressHosts: map[egress.ProfileName][]egress.Host{ + egress.ProfileStandard: {{Name: "api.acme.dev"}}, + }, + SettingsManifest: &settings.Manifest{ + Entries: []settings.Entry{ + {Category: "settings", HostPath: ".acme/s.json", GuestPath: ".acme/s.json", Kind: settings.KindMergeFile, Format: "json", Optional: true}, + }, + }, + } + + resolved := &resolvedRegistry{ + merged: &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + "acme": { + Image: "ghcr.io/acme/agent:latest", + Description: "ACME agent", + EnvRequired: []string{"ACME_API_KEY"}, + DefaultEnv: map[string]string{"ACME_MODE": secret}, + }, + }, + }, + global: &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + "acme": {Image: "ghcr.io/acme/agent:latest"}, + }, + }, + local: &domainconfig.Config{}, + } + + var buf bytes.Buffer + renderInspect(&buf, ag, resolved, "acme") + out := buf.String() + + assert.NotContains(t, out, secret, "inspect output must never contain env values") + // Sanity: names/patterns are present. + assert.Contains(t, out, "ACME_API_KEY") + assert.Contains(t, out, "ACME_MODE") + assert.Contains(t, out, "present") // required env present indicator +} + +func TestDidLocalAddAgent(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + local *domainconfig.Config + global *domainconfig.Config + agent string + want bool + }{ + { + name: "local-only agent flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"sneaky": {Image: "x"}}}, + global: &domainconfig.Config{}, + agent: "sneaky", + want: true, + }, + { + name: "agent present in global not flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"known": {Image: "x"}}}, + global: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"known": {Image: "g"}}}, + agent: "known", + want: false, + }, + { + name: "built-in name in local not flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"claude-code": {Image: "x"}}}, + global: &domainconfig.Config{}, + agent: "claude-code", + want: false, + }, + { + name: "nil local returns false", + local: nil, + global: &domainconfig.Config{}, + agent: "anything", + want: false, + }, + { + name: "agent not in local returns false", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"other": {Image: "x"}}}, + global: &domainconfig.Config{}, + agent: "missing", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + resolved := &resolvedRegistry{global: tt.global, local: tt.local} + assert.Equal(t, tt.want, didLocalAddAgent(resolved, tt.agent)) + }) + } +} + +func TestDidLocalAddCredentials(t *testing.T) { + t.Parallel() + + creds := func(paths ...string) *domainconfig.AgentCredentialsConfig { + return &domainconfig.AgentCredentialsConfig{Persist: paths} + } + + tests := []struct { + name string + local *domainconfig.Config + agent string + want bool + }{ + { + name: "persist set flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"a": {Credentials: creds(".x/creds")}}}, + agent: "a", + want: true, + }, + { + name: "nil credentials not flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"a": {Image: "x"}}}, + agent: "a", + want: false, + }, + { + name: "empty persist not flagged", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"a": {Credentials: creds()}}}, + agent: "a", + want: false, + }, + { + name: "nil local returns false", + local: nil, + agent: "a", + want: false, + }, + { + name: "agent not in local returns false", + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"other": {Credentials: creds(".x")}}}, + agent: "a", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + resolved := &resolvedRegistry{local: tt.local} + assert.Equal(t, tt.want, didLocalAddCredentials(resolved, tt.agent)) + }) + } +} + +func TestFieldSourceImage(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + global *domainconfig.Config + local *domainconfig.Config + isBuiltin bool + agent string + want string + }{ + { + name: "image set only in global", + global: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"a": {Image: "g"}}}, + agent: "a", + want: "global", + }, + { + name: "image set in workspace", + global: &domainconfig.Config{}, + local: &domainconfig.Config{Agents: map[string]domainconfig.AgentOverride{"a": {Image: "w"}}}, + agent: "a", + want: "workspace", + }, + { + name: "unset plus builtin falls back to built-in", + global: &domainconfig.Config{}, + isBuiltin: true, + agent: "claude-code", + want: "built-in", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + resolved := &resolvedRegistry{global: tt.global, local: tt.local} + assert.Equal(t, tt.want, fieldSourceImage(tt.agent, resolved, tt.isBuiltin)) + }) + } +} + +// TestRunDoctor_MisconfiguredCustomAgent confirms doctor surfaces a custom +// agent's ValidateCustomAgent failure (bad image + unsafe credential path) as a +// FAIL, rather than the "unknown agent" hard-gate it would hit if it required +// the agent to be registered first. +func TestRunDoctor_MisconfiguredCustomAgent(t *testing.T) { + t.Parallel() + + override := domainconfig.AgentOverride{ + Image: "::::bad ref::::", + Command: []string{"run"}, + EgressProfile: "permissive", + Credentials: &domainconfig.AgentCredentialsConfig{Persist: []string{"../escape.json"}}, + } + lookup := func(string) (string, bool) { return "", false } + results, ok := runDoctor("badagent", override, false, lookup, imageRefValidator, false, false) + assert.False(t, ok, "a misconfigured custom agent must FAIL doctor") + assert.True(t, containsMessage(results, "config validation"), + "doctor must surface the ValidateCustomAgent failure, got: %+v", results) +} + +func containsMessage(results []checkResult, substr string) bool { + for _, r := range results { + if bytes.Contains([]byte(r.message), []byte(substr)) { + return true + } + } + return false +} diff --git a/cmd/bbox/main.go b/cmd/bbox/main.go index dff45ee..673ed12 100644 --- a/cmd/bbox/main.go +++ b/cmd/bbox/main.go @@ -24,6 +24,7 @@ import ( "unicode" "github.com/adrg/xdg" + "github.com/google/go-containerregistry/pkg/name" "github.com/spf13/cobra" "github.com/stacklok/go-microvm/extract" "github.com/stacklok/go-microvm/image" @@ -228,6 +229,7 @@ Example: // Add subcommands. cmd.AddCommand(listCmd()) + cmd.AddCommand(agentsCmd()) cmd.AddCommand(authCmd()) cmd.AddCommand(configCmd()) cmd.AddCommand(cacheCmd()) @@ -236,18 +238,16 @@ Example: } func listCmd() *cobra.Command { - return &cobra.Command{ + var cfgPath string + cmd := &cobra.Command{ Use: "list", - Short: "List available agents", - RunE: func(_ *cobra.Command, _ []string) error { - registry := infraagent.NewRegistry(clients.Builtins(slog.Default())...) - entries := registry.List() - for _, e := range entries { - fmt.Printf("%-15s %s\n", e.Agent.Name, e.Agent.Image) - } - return nil + Short: "List available agents (alias for 'agents list')", + RunE: func(cmd *cobra.Command, _ []string) error { + return runAgentsList(cmd, cfgPath) }, } + cmd.Flags().StringVar(&cfgPath, "config", "", "Config file path (default: ~/.config/broodbox/config.yaml)") + return cmd } func authCmd() *cobra.Command { @@ -524,67 +524,14 @@ func run(parentCtx context.Context, agentName string, flags runFlags) error { infravm.CleanupStaleLogs(filepath.Join(home, ".config", "broodbox", "vms"), logger) } - // Build registry: start with built-in clients, then layer in any - // data-only custom agents declared in config. - registry := infraagent.NewRegistry(clients.Builtins(logger)...) - cfgLoader := infraconfig.NewLoader(flags.cfgPath) - - cfg, err := cfgLoader.Load() - if err != nil { - // Fail fast on global config errors. Silently discarding every - // override because one line fails validation (or the file is - // unreadable) is worse UX than a hard error naming the file and - // the exact problem — the user owns this file and wants to know. - return fmt.Errorf("loading global config %s: %w", cfgLoader.Path(), err) - } - if cfg == nil { - cfg = &domainconfig.Config{} - } - - // Load per-workspace config and merge. Unlike the global config, a - // bad workspace config is warn-and-skip: a malicious or corrupted - // `.broodbox.yaml` shipped by an untrusted repo must not be able to - // DOS the user's session — the operator can always run from a - // different directory or delete the file. - localCfgPath := filepath.Join(ws, domainconfig.LocalConfigFile) - localCfg, err := infraconfig.LoadFromPath(localCfgPath) + // Build registry and resolve config (global + workspace-local merge, + // custom-agent registration). Shared with the `bbox agents` commands. + resolved, err := buildResolvedRegistry(flags.cfgPath, ws, logger, os.Stderr) if err != nil { - _, _ = fmt.Fprintf(os.Stderr, "Warning: failed to load local config %s: %s (ignoring)\n", localCfgPath, err) - logger.Warn("failed to load local config, ignoring", "error", err) - } - warnLocalConfigOverrides(os.Stderr, localCfg, cfg) - cfg = domainconfig.MergeConfigs(cfg, localCfg) - if cfg == nil { - cfg = &domainconfig.Config{} - } - - if cfg.Agents != nil { - // Register custom agents from config (only those not already built-in). - // Warnings use fmt.Fprintf(os.Stderr) instead of slog because the logger - // writes to a file — users would not see invalid agent name warnings - // unless they manually inspected the log. - for name, override := range cfg.Agents { - if err := agent.ValidateName(name); err != nil { - _, _ = fmt.Fprintf(os.Stderr, "Warning: skipping custom agent %q: %s\n", name, err) - continue - } - if _, err := registry.Get(name); err != nil { - // Not a built-in agent — register as custom if it has an image. - if override.Image != "" { - if addErr := registry.Add(agent.Agent{ - Name: name, - Image: override.Image, - Command: override.Command, - EnvForward: override.EnvForward, - DefaultCPUs: override.CPUs, - DefaultMemory: override.Memory, - }); addErr != nil { - _, _ = fmt.Fprintf(os.Stderr, "Warning: skipping custom agent %q: %s\n", name, addErr) - } - } - } - } + return err } + registry := resolved.registry + cfg := resolved.merged // Resolve effective workspace mode. CLI wins over global config; unset // means snapshot. Workspace-local .broodbox.yaml was already merged with @@ -857,6 +804,13 @@ func run(parentCtx context.Context, agentName string, flags runFlags) error { } } + // Default custom (bring-your-own) agents with MCP enabled to the + // safe-tools authz profile when no explicit profile was supplied via + // CLI flag, global config, inferred custom policies, or per-agent + // override. Built-ins keep full-access. This is subordinate to every + // explicit source above, so an operator can still widen or tighten it. + authzCfg = applyCustomAgentAuthzDefault(authzCfg, isCustomAgent(registry, agentName)) + // Resolve session TTL: flag (non-zero) > config > default (12h). sessionTTL := flags.mcpSessionTTL if sessionTTL == 0 && cfg != nil { @@ -1131,6 +1085,138 @@ func chooseObserver(terminal *infraterminal.OSTerminal) progress.Observer { return infraprogress.NewSimpleObserver(os.Stderr) } +// resolvedRegistry bundles the registry and the layered configs produced by +// buildResolvedRegistry. The individual global/local configs are kept +// alongside the merged result so commands like `bbox agents inspect` can +// attribute each field to its source (built-in / global / workspace). +type resolvedRegistry struct { + registry *infraagent.Registry + merged *domainconfig.Config + global *domainconfig.Config + local *domainconfig.Config +} + +// imageRefValidator parses an OCI image reference using go-containerregistry. +// It performs no network I/O — only syntactic validation. It is passed into +// the domain-layer config.ValidateCustomAgent so the domain stays free of the +// registry dependency. +func imageRefValidator(ref string) error { + if _, err := name.ParseReference(ref); err != nil { + return err + } + return nil +} + +// buildResolvedRegistry loads the global config, merges the per-workspace +// .broodbox.yaml (with the usual warn-and-skip semantics), registers any +// custom (data-only) agents declared in the merged config, and returns the +// populated registry plus the layered configs. +// +// It is the single resolution path shared by `run()` and the `bbox agents` +// subcommands so they all observe the same agent set and validation behavior. +// warnWriter receives the workspace-config override warnings; pass io.Discard +// to suppress them (e.g. for `agents inspect`). +func buildResolvedRegistry(cfgPath, ws string, logger *slog.Logger, warnWriter io.Writer) (*resolvedRegistry, error) { + registry := infraagent.NewRegistry(clients.Builtins(logger)...) + cfgLoader := infraconfig.NewLoader(cfgPath) + + global, err := cfgLoader.Load() + if err != nil { + // Fail fast on global config errors. Silently discarding every + // override because one line fails validation (or the file is + // unreadable) is worse UX than a hard error naming the file and + // the exact problem — the user owns this file and wants to know. + return nil, fmt.Errorf("loading global config %s: %w", cfgLoader.Path(), err) + } + if global == nil { + global = &domainconfig.Config{} + } + + // Load per-workspace config and merge. Unlike the global config, a bad + // workspace config is warn-and-skip: a malicious or corrupted + // `.broodbox.yaml` shipped by an untrusted repo must not be able to DOS + // the user's session — the operator can always run from a different + // directory or delete the file. + localCfgPath := filepath.Join(ws, domainconfig.LocalConfigFile) + local, err := infraconfig.LoadFromPath(localCfgPath) + if err != nil { + _, _ = fmt.Fprintf(warnWriter, "Warning: failed to load local config %s: %s (ignoring)\n", localCfgPath, err) + logger.Warn("failed to load local config, ignoring", "error", err) + } + warnLocalConfigOverrides(warnWriter, local, global) + merged := domainconfig.MergeConfigs(global, local) + if merged == nil { + merged = &domainconfig.Config{} + } + + registerCustomAgents(registry, merged, warnWriter) + + return &resolvedRegistry{registry: registry, merged: merged, global: global, local: local}, nil +} + +// isCustomAgent reports whether agentName resolves to a custom (bring-your-own) +// agent rather than a built-in. Custom agents are registered as data-only +// entries with a nil Plugin (registerCustomAgents), whereas built-ins always +// carry a Plugin. Unknown agents return false (treated as non-custom). +func isCustomAgent(registry *infraagent.Registry, agentName string) bool { + entry, err := registry.Get(agentName) + if err != nil { + return false + } + return entry.Plugin == nil +} + +// applyCustomAgentAuthzDefault returns the effective MCP authz config for an +// agent with MCP enabled. When no authz profile was resolved from any explicit +// source (CLI flag, global config, inferred custom policies, or the per-agent +// tighten-only override) and the agent is a custom (bring-your-own) agent, it +// defaults to DefaultCustomAgentMCPAuthzProfile (safe-tools). Built-in agents +// and any agent with an already-resolved authz config are returned unchanged +// (built-ins keep full-access). It is pure so the default can be table-tested. +func applyCustomAgentAuthzDefault(authzCfg *domainconfig.MCPAuthzConfig, isCustom bool) *domainconfig.MCPAuthzConfig { + if authzCfg != nil { + return authzCfg + } + if !isCustom { + return nil + } + return &domainconfig.MCPAuthzConfig{Profile: domainconfig.DefaultCustomAgentMCPAuthzProfile} +} + +// registerCustomAgents adds data-only custom agents from the merged config to +// the registry. Agents already present (built-ins) are skipped. Each custom +// agent is validated and mapped via the pure domain functions; invalid or +// unmappable agents are warned and skipped rather than aborting the run. +// +// Warnings use the supplied writer (typically os.Stderr) instead of slog +// because the logger writes to a file — users would not see invalid agent +// warnings unless they manually inspected the log. +func registerCustomAgents(registry *infraagent.Registry, merged *domainconfig.Config, warnWriter io.Writer) { + for name, override := range merged.Agents { + if _, err := registry.Get(name); err == nil { + // Already a built-in agent — overrides are applied at run time. + continue + } + if override.Image == "" { + // Not a custom-agent definition (e.g. an override for an agent + // that is not currently registered). Skip silently. + continue + } + if err := domainconfig.ValidateCustomAgent(name, override, imageRefValidator); err != nil { + _, _ = fmt.Fprintf(warnWriter, "Warning: skipping custom agent %q: %s\n", name, err) + continue + } + ag, err := domainconfig.AgentFromOverride(name, override, merged.Defaults) + if err != nil { + _, _ = fmt.Fprintf(warnWriter, "Warning: skipping custom agent %q: %s\n", name, err) + continue + } + if addErr := registry.Add(ag); addErr != nil { + _, _ = fmt.Fprintf(warnWriter, "Warning: skipping custom agent %q: %s\n", name, addErr) + } + } +} + // warnLocalConfigOverrides prints warnings to w for each security-sensitive // field set in the local per-workspace config. This makes supply-chain-style // overrides visible to the user without blocking execution. @@ -1270,14 +1356,31 @@ func warnLocalConfigOverrides(w io.Writer, localCfg, globalCfg *domainconfig.Con for _, name := range slices.Sorted(maps.Keys(localCfg.Agents)) { safeName := sanitizeValue(name) override := localCfg.Agents[name] + + // Security #7: a workspace config cannot ADD a new custom agent or + // introduce new credential paths — both are dropped during merge. + // Surface the blocked attempt explicitly. + if _, inGlobal := globalCfg.Agents[name]; !inGlobal { + warnings = append(warnings, fmt.Sprintf( + "attempts to define a new agent %q — ignored (custom agents must be declared in global config)", safeName)) + } + if override.Credentials != nil && len(override.Credentials.Persist) > 0 { + warnings = append(warnings, fmt.Sprintf( + "attempts to add %s credential paths — ignored (credentials must be declared in global config)", safeName)) + } + if override.Image != "" { - warnings = append(warnings, fmt.Sprintf("overrides %s image: %s", safeName, sanitizeValue(override.Image))) + warnings = append(warnings, fmt.Sprintf( + "attempts to override %s image: %s — ignored (image must be declared in global config)", + safeName, sanitizeValue(override.Image))) } if len(override.Command) > 0 { - warnings = append(warnings, fmt.Sprintf("overrides %s command", safeName)) + warnings = append(warnings, fmt.Sprintf( + "attempts to override %s command — ignored (command must be declared in global config)", safeName)) } if len(override.EnvForward) > 0 { - warnings = append(warnings, fmt.Sprintf("overrides %s env forwarding", safeName)) + warnings = append(warnings, fmt.Sprintf( + "narrows %s env forwarding (can only restrict, not widen)", safeName)) } if len(override.AllowHosts) > 0 { names := make([]string, len(override.AllowHosts)) diff --git a/cmd/bbox/main_test.go b/cmd/bbox/main_test.go index c9747bb..2ebfedd 100644 --- a/cmd/bbox/main_test.go +++ b/cmd/bbox/main_test.go @@ -6,11 +6,15 @@ package main import ( "bytes" "fmt" + "io" + "log/slog" "os" "path/filepath" "strings" "testing" + infraagent "github.com/stacklok/brood-box/internal/infra/agent" + "github.com/stacklok/brood-box/pkg/clients" "github.com/stacklok/brood-box/pkg/domain/bytesize" domainconfig "github.com/stacklok/brood-box/pkg/domain/config" ) @@ -198,7 +202,7 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, global: defaultGlobal, expected: wrapWarnings( - "overrides myagent image: ghcr.io/evil/image:latest", + "attempts to override myagent image: ghcr.io/evil/image:latest — ignored (image must be declared in global config)", ), }, { @@ -209,7 +213,7 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, }, global: defaultGlobal, - expected: wrapWarnings("overrides myagent command"), + expected: wrapWarnings("attempts to override myagent command — ignored (command must be declared in global config)"), }, { name: "agent env forwarding", @@ -219,7 +223,7 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, }, global: defaultGlobal, - expected: wrapWarnings("overrides myagent env forwarding"), + expected: wrapWarnings("narrows myagent env forwarding (can only restrict, not widen)"), }, { name: "agent allow_hosts", @@ -321,9 +325,9 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, global: defaultGlobal, expected: wrapWarnings( - "overrides a-agent image: img-a", - "overrides m-agent image: img-m", - "overrides z-agent image: img-z", + "attempts to override a-agent image: img-a — ignored (image must be declared in global config)", + "attempts to override m-agent image: img-m — ignored (image must be declared in global config)", + "attempts to override z-agent image: img-z — ignored (image must be declared in global config)", ), }, // --- ANSI escape sanitization (CRITICAL-2 fix) --- @@ -348,7 +352,7 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, }, global: defaultGlobal, - contains: []string{"overrides evil[0magent image: img"}, + contains: []string{"attempts to override evil[0magent image: img"}, notContai: []string{"\x1b", "\x07"}, }, { @@ -359,7 +363,7 @@ func TestWarnLocalConfigOverrides(t *testing.T) { }, }, global: defaultGlobal, - contains: []string{"overrides myagent image: ghcr.io/evil[2J/image"}, + contains: []string{"attempts to override myagent image: ghcr.io/evil[2J/image"}, notContai: []string{"\x1b"}, }, // --- Combined: all fields set --- @@ -413,9 +417,9 @@ func TestWarnLocalConfigOverrides(t *testing.T) { "sets default memory: 4g", "adds egress hosts: global-extra.com", "sets git token forwarding: false", - "overrides myagent image: custom:v1", - "overrides myagent command", - "overrides myagent env forwarding", + "attempts to override myagent image: custom:v1 — ignored (image must be declared in global config)", + "attempts to override myagent command — ignored (command must be declared in global config)", + "narrows myagent env forwarding (can only restrict, not widen)", "adds myagent egress hosts: agent-extra.com", "sets myagent egress profile: locked", "sets myagent MCP authz profile: observe (can only tighten, not widen)", @@ -427,8 +431,32 @@ func TestWarnLocalConfigOverrides(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() + // These cases exercise the per-field OVERRIDE warnings for agents + // that already exist globally. To isolate that behavior from the + // separately tested "local adds a new agent" / "local adds + // credentials" warnings, ensure every agent key present locally is + // also present in the global config (with no credential paths). + global := tt.global + if tt.local != nil && len(tt.local.Agents) > 0 { + merged := &domainconfig.Config{} + if tt.global != nil { + *merged = *tt.global + } + agents := map[string]domainconfig.AgentOverride{} + for k, v := range merged.Agents { + agents[k] = v + } + for k := range tt.local.Agents { + if _, ok := agents[k]; !ok { + agents[k] = domainconfig.AgentOverride{Image: "ghcr.io/global/base:latest"} + } + } + merged.Agents = agents + global = merged + } + var buf bytes.Buffer - warnLocalConfigOverrides(&buf, tt.local, tt.global) + warnLocalConfigOverrides(&buf, tt.local, global) got := buf.String() if tt.expected != "" || (tt.contains == nil && tt.notContai == nil) { @@ -450,6 +478,128 @@ func TestWarnLocalConfigOverrides(t *testing.T) { } } +func TestWarnLocalConfigOverrides_BlockedAgentAdditions(t *testing.T) { + t.Parallel() + + t.Run("local adds a new agent", func(t *testing.T) { + t.Parallel() + local := &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + "sneaky": {Image: "ghcr.io/evil/agent:latest"}, + }, + } + var buf bytes.Buffer + warnLocalConfigOverrides(&buf, local, &domainconfig.Config{}) + got := buf.String() + if !strings.Contains(got, `attempts to define a new agent "sneaky"`) { + t.Errorf("expected new-agent warning, got:\n%s", got) + } + }) + + t.Run("local adds credential paths to existing agent", func(t *testing.T) { + t.Parallel() + local := &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + "existing": {Credentials: &domainconfig.AgentCredentialsConfig{Persist: []string{".x/creds"}}}, + }, + } + global := &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + "existing": {Image: "ghcr.io/global/agent:latest"}, + }, + } + var buf bytes.Buffer + warnLocalConfigOverrides(&buf, local, global) + got := buf.String() + if !strings.Contains(got, "attempts to add existing credential paths") { + t.Errorf("expected credential warning, got:\n%s", got) + } + }) +} + +func TestRegisterCustomAgents(t *testing.T) { + t.Parallel() + + registry := infraagent.NewRegistry(clients.Builtins(slog.New(slog.NewTextHandler(io.Discard, nil)))...) + + merged := &domainconfig.Config{ + Agents: map[string]domainconfig.AgentOverride{ + // Valid custom agent — should be registered. + "good": { + Image: "ghcr.io/acme/good:latest", + Command: []string{"run"}, + EgressProfile: "permissive", + }, + // Invalid custom agent (bad image ref + escaping credential path) — + // must be warned and skipped, not abort the run. + "badagent": { + Image: "::::bad ref::::", + Command: []string{"run"}, + EgressProfile: "permissive", + Credentials: &domainconfig.AgentCredentialsConfig{Persist: []string{"../escape.json"}}, + }, + // Override with empty Image (an override for an unregistered agent) — + // skipped silently (no warning). + "emptyimage": { + Command: []string{"run"}, + }, + // A built-in name present in config — must be skipped (kept as built-in). + "claude-code": { + Image: "ghcr.io/evil/replacement:latest", + }, + }, + } + + var warn bytes.Buffer + registerCustomAgents(registry, merged, &warn) + + // Valid custom agent is registered as a data-only entry (nil Plugin). + entry, err := registry.Get("good") + if err != nil { + t.Fatalf("expected custom agent %q to be registered: %v", "good", err) + } + if entry.Plugin != nil { + t.Errorf("custom agent %q should have a nil Plugin (data-only entry)", "good") + } + if entry.Agent.Image != "ghcr.io/acme/good:latest" { + t.Errorf("custom agent image = %q, want %q", entry.Agent.Image, "ghcr.io/acme/good:latest") + } + + // Invalid agent is not registered. + if _, err := registry.Get("badagent"); err == nil { + t.Errorf("invalid custom agent %q must not be registered", "badagent") + } + + // Empty-image override is not registered. + if _, err := registry.Get("emptyimage"); err == nil { + t.Errorf("empty-image override %q must not be registered", "emptyimage") + } + + // Built-in keeps its own plugin entry — not replaced by the local override. + cc, err := registry.Get("claude-code") + if err != nil { + t.Fatalf("built-in claude-code must remain registered: %v", err) + } + if cc.Plugin == nil { + t.Errorf("built-in claude-code must keep its Plugin (override must not replace it)") + } + if cc.Agent.Image == "ghcr.io/evil/replacement:latest" { + t.Errorf("built-in claude-code image must not be replaced by a local override") + } + + got := warn.String() + if !strings.Contains(got, `skipping custom agent "badagent"`) { + t.Errorf("expected warning for invalid agent, got:\n%s", got) + } + // Empty-image override and built-in skip are silent. + if strings.Contains(got, "emptyimage") { + t.Errorf("empty-image override should be skipped silently, got:\n%s", got) + } + if strings.Contains(got, "claude-code") { + t.Errorf("built-in name in config should be skipped silently, got:\n%s", got) + } +} + func TestSanitizeValue(t *testing.T) { t.Parallel() @@ -699,3 +849,98 @@ func TestCacheDirFunctions(t *testing.T) { }) } } + +// TestApplyCustomAgentAuthzDefault asserts the safe-tools default is applied +// to custom agents with MCP enabled and no explicit authz, while built-ins and +// already-resolved configs are left unchanged. +func TestApplyCustomAgentAuthzDefault(t *testing.T) { + t.Parallel() + + explicit := &domainconfig.MCPAuthzConfig{Profile: domainconfig.MCPAuthzProfileObserve} + + tests := []struct { + name string + in *domainconfig.MCPAuthzConfig + isCustom bool + want *domainconfig.MCPAuthzConfig + }{ + { + name: "custom agent with no authz defaults to safe-tools", + in: nil, + isCustom: true, + want: &domainconfig.MCPAuthzConfig{Profile: domainconfig.DefaultCustomAgentMCPAuthzProfile}, + }, + { + name: "built-in agent with no authz stays full-access (nil)", + in: nil, + isCustom: false, + want: nil, + }, + { + name: "custom agent with explicit authz is unchanged", + in: explicit, + isCustom: true, + want: explicit, + }, + { + name: "built-in agent with explicit authz is unchanged", + in: explicit, + isCustom: false, + want: explicit, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := applyCustomAgentAuthzDefault(tt.in, tt.isCustom) + if tt.want == nil { + if got != nil { + t.Fatalf("expected nil authz config, got %+v", got) + } + return + } + if got == nil { + t.Fatalf("expected %+v, got nil", tt.want) + } + if got.Profile != tt.want.Profile { + t.Errorf("profile = %q, want %q", got.Profile, tt.want.Profile) + } + }) + } + + // Confirm the default constant really is safe-tools (the secure default). + if domainconfig.DefaultCustomAgentMCPAuthzProfile != domainconfig.MCPAuthzProfileSafeTools { + t.Errorf("DefaultCustomAgentMCPAuthzProfile = %q, want %q", + domainconfig.DefaultCustomAgentMCPAuthzProfile, domainconfig.MCPAuthzProfileSafeTools) + } +} + +// TestIsCustomAgent asserts built-ins (Plugin != nil) are not custom, while +// data-only registered agents (nil Plugin) are. +func TestIsCustomAgent(t *testing.T) { + t.Parallel() + + registry := infraagent.NewRegistry(clients.Builtins(slog.New(slog.NewTextHandler(io.Discard, nil)))...) + customAgent, err := domainconfig.AgentFromOverride("my-custom", domainconfig.AgentOverride{ + Image: "ghcr.io/acme/my-custom:latest", + Command: []string{"run"}, + EgressProfile: "permissive", + }, domainconfig.DefaultsConfig{}) + if err != nil { + t.Fatalf("building custom agent: %v", err) + } + if err := registry.Add(customAgent); err != nil { + t.Fatalf("adding custom agent: %v", err) + } + + if isCustomAgent(registry, "claude-code") { + t.Error("claude-code is a built-in, isCustomAgent should be false") + } + if !isCustomAgent(registry, "my-custom") { + t.Error("my-custom is data-only, isCustomAgent should be true") + } + if isCustomAgent(registry, "does-not-exist") { + t.Error("unknown agent should not be reported as custom") + } +} diff --git a/internal/infra/config/writer.go b/internal/infra/config/writer.go index 00cd572..3c33ce6 100644 --- a/internal/infra/config/writer.go +++ b/internal/infra/config/writer.go @@ -167,6 +167,11 @@ var defaultConfigTemplate = `# Brood Box configuration # Per-agent configuration overrides. Keys are agent names # (e.g. claude-code, codex, opencode, or custom agent names). +# +# SECURITY: Custom (bring-your-own) agents may ONLY be declared here in the +# global config (or via CLI). A per-workspace .broodbox.yaml CANNOT add a new +# custom agent or introduce new credential paths — it may only tighten an +# agent that already exists in this file. # agents: # # claude-code: # # # Override the OCI image reference. @@ -197,7 +202,7 @@ var defaultConfigTemplate = `# Brood Box configuration # # allow_hosts: [] # # # # # Override MCP proxy settings for this agent. -# # # Only enabled (gate) and authz (tighten-only) are supported. +# # # Only enabled (gate), mode, and authz (tighten-only) are supported. # # mcp: # # enabled: true # # authz: @@ -210,6 +215,69 @@ var defaultConfigTemplate = `# Brood Box configuration # # categories: # # rules: true # # skills: true +# +# # Custom (bring-your-own) agent declared entirely in config. +# # Custom agents are SAFER by default than built-ins: env_forward is empty +# # (no host env is forwarded), the egress profile defaults to "standard" +# # (so you must declare egress_hosts or set it to "permissive"), and when +# # MCP is enabled the authz profile defaults to "safe-tools". +# # my-agent: +# # image: "ghcr.io/acme/my-agent:latest" +# # command: ["my-agent", "--interactive"] +# # description: "ACME's in-house coding agent" +# # +# # # VM resources. If omitted (and no global defaults set), a safe +# # # minimum of 2 cpus / 4096 MiB is applied so the agent can boot. +# # cpus: 2 +# # memory: 4096 +# # +# # # Forwarded host env (empty by default — opt in explicitly). +# # env_forward: +# # - "ACME_API_KEY" +# # - "ACME_*" +# # # Env always set in the VM. +# # default_env: +# # ACME_MODE: "interactive" +# # # Env names that must be present on the host (surfaced by +# # # 'bbox agents inspect' and 'bbox agents doctor'; values never read). +# # env_required: +# # - "ACME_API_KEY" +# # +# # # Egress: default profile "standard" requires hosts for it. +# # egress_profile: "standard" +# # egress_hosts: +# # standard: +# # - name: "api.acme.dev" +# # ports: [443] +# # protocol: 6 # TCP +# # - name: "*.acme.dev" +# # locked: +# # - name: "api.acme.dev" +# # ports: [443] +# # +# # # MCP mode "env": the proxy is enabled and the agent learns it only +# # # via the BBOX_MCP_URL env var (no config-file injection). +# # mcp: +# # enabled: true +# # mode: "env" +# # authz: +# # profile: "safe-tools" +# # +# # # Credential persistence (OFF unless declared; relative paths only). +# # credentials: +# # persist: +# # - ".acme/credentials.json" +# # - ".config/acme/" +# # +# # # Settings injection (OFF unless declared; relative paths only). +# # settings: +# # - category: "settings" +# # host_path: ".config/acme/settings.json" +# # guest_path: ".config/acme/settings.json" +# # kind: "merge-file" # file | directory | merge-file +# # format: "json" # json | toml | jsonc (merge-file only) +# # optional: true +# # allow_keys: ["theme", "editor"] ` // WriteDefault writes the default config template to the given path. diff --git a/internal/infra/mcp/provider.go b/internal/infra/mcp/provider.go index adc27e2..bbcafc0 100644 --- a/internal/infra/mcp/provider.go +++ b/internal/infra/mcp/provider.go @@ -173,7 +173,7 @@ func (p *VMCPProvider) Services(ctx context.Context) ([]hostservice.Service, err Name: vmcpServerName, GroupRef: p.group, Port: int(p.port), - EndpointPath: "/mcp", + EndpointPath: domainconfig.MCPEndpointPath, SessionTTL: p.sessionTTL, AuthMiddleware: authMiddleware, AuthzMiddleware: authzMiddleware, diff --git a/internal/infra/vm/gatewayip_guard_test.go b/internal/infra/vm/gatewayip_guard_test.go new file mode 100644 index 0000000..0558a5a --- /dev/null +++ b/internal/infra/vm/gatewayip_guard_test.go @@ -0,0 +1,24 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package vm + +import ( + "testing" + + "github.com/stacklok/go-microvm/net/topology" +) + +// sandboxGatewayIP duplicates the literal hardcoded in pkg/sandbox so the +// application layer does not have to import go-microvm's topology package +// (which would violate the "sandbox depends only on domain" rule). This test +// fails if go-microvm ever changes its gateway IP, catching the drift before +// BBOX_MCP_URL silently points at the wrong address. +const sandboxGatewayIP = "192.168.127.1" + +func TestSandboxGatewayIPMatchesTopology(t *testing.T) { + if topology.GatewayIP != sandboxGatewayIP { + t.Fatalf("go-microvm topology.GatewayIP = %q, but pkg/sandbox hardcodes %q; update sandbox.gatewayIP", + topology.GatewayIP, sandboxGatewayIP) + } +} diff --git a/pkg/clients/claudecode/mcp.go b/pkg/clients/claudecode/mcp.go index cfdd982..2db45ef 100644 --- a/pkg/clients/claudecode/mcp.go +++ b/pkg/clients/claudecode/mcp.go @@ -11,6 +11,7 @@ import ( "github.com/stacklok/brood-box/pkg/clients/internal/configio" "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/config" ) // Ref: https://code.claude.com/docs/en/mcp @@ -35,7 +36,7 @@ func (mcpInjector) Inject(rootfsPath, gatewayIP string, port uint16, chown agent servers := map[string]claudeCodeServer{ "sandbox-tools": { Type: "http", - URL: fmt.Sprintf("http://%s:%d/mcp", gatewayIP, port), + URL: fmt.Sprintf("http://%s:%d%s", gatewayIP, port, config.MCPEndpointPath), }, } diff --git a/pkg/clients/codex/mcp.go b/pkg/clients/codex/mcp.go index 8e7fa1b..5eee5e4 100644 --- a/pkg/clients/codex/mcp.go +++ b/pkg/clients/codex/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/stacklok/brood-box/pkg/clients/internal/configio" "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/config" ) // Ref: https://developers.openai.com/codex/config-reference/ @@ -19,7 +20,7 @@ type mcpInjector struct{} // Inject merges an MCP server entry into ~/.codex/config.toml, preserving // any pre-existing TOML sections. func (mcpInjector) Inject(rootfsPath, gatewayIP string, port uint16, chown agent.ChownFunc) error { - mcpURL := fmt.Sprintf("http://%s:%d/mcp", gatewayIP, port) + mcpURL := fmt.Sprintf("http://%s:%d%s", gatewayIP, port, config.MCPEndpointPath) codexDir := filepath.Join(rootfsPath, configio.SandboxHome, ".codex") if err := configio.MkdirAndChown(codexDir, chown); err != nil { diff --git a/pkg/clients/gemini/mcp.go b/pkg/clients/gemini/mcp.go index 17325fc..6b3f773 100644 --- a/pkg/clients/gemini/mcp.go +++ b/pkg/clients/gemini/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/stacklok/brood-box/pkg/clients/internal/configio" "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/config" ) // Ref: https://github.com/google-gemini/gemini-cli/blob/main/docs/tools/mcp-server.md @@ -29,7 +30,7 @@ func (mcpInjector) Inject(rootfsPath, gatewayIP string, port uint16, chown agent return configio.MergeJSONMapEntries(geminiDir, "settings.json", "mcpServers", map[string]any{ "sandbox-tools": map[string]any{ - "httpUrl": fmt.Sprintf("http://%s:%d/mcp", gatewayIP, port), + "httpUrl": fmt.Sprintf("http://%s:%d%s", gatewayIP, port, config.MCPEndpointPath), }, }, chown) } diff --git a/pkg/clients/hermes/mcp.go b/pkg/clients/hermes/mcp.go index c2f646d..a0c757b 100644 --- a/pkg/clients/hermes/mcp.go +++ b/pkg/clients/hermes/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/stacklok/brood-box/pkg/clients/internal/configio" "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/config" ) // Ref: https://github.com/NousResearch/hermes-agent @@ -21,7 +22,7 @@ type mcpInjector struct{} // Inject merges an MCP server entry into ~/.hermes/config.yaml, preserving // any pre-existing YAML keys (provider config, skills, cron, etc.). func (mcpInjector) Inject(rootfsPath, gatewayIP string, port uint16, chown agent.ChownFunc) error { - mcpURL := fmt.Sprintf("http://%s:%d/mcp", gatewayIP, port) + mcpURL := fmt.Sprintf("http://%s:%d%s", gatewayIP, port, config.MCPEndpointPath) hermesDir := filepath.Join(rootfsPath, configio.SandboxHome, ".hermes") if err := configio.MkdirAndChown(hermesDir, chown); err != nil { diff --git a/pkg/clients/opencode/mcp.go b/pkg/clients/opencode/mcp.go index db4f348..0ce8f4e 100644 --- a/pkg/clients/opencode/mcp.go +++ b/pkg/clients/opencode/mcp.go @@ -9,6 +9,7 @@ import ( "github.com/stacklok/brood-box/pkg/clients/internal/configio" "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/config" ) // Ref: https://opencode.ai/docs/mcp-servers/ @@ -29,7 +30,7 @@ func (mcpInjector) Inject(rootfsPath, gatewayIP string, port uint16, chown agent servers := map[string]openCodeServer{ "sandbox-tools": { Type: "remote", - URL: fmt.Sprintf("http://%s:%d/mcp", gatewayIP, port), + URL: fmt.Sprintf("http://%s:%d%s", gatewayIP, port, config.MCPEndpointPath), Enabled: true, }, } diff --git a/pkg/domain/agent/env_bbox.go b/pkg/domain/agent/env_bbox.go new file mode 100644 index 0000000..db3fd1b --- /dev/null +++ b/pkg/domain/agent/env_bbox.go @@ -0,0 +1,102 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package agent + +// Universal BBOX_* environment variable names injected into every sandbox VM. +// These give an agent (especially a custom/bring-your-own agent with no +// built-in plugin) a stable, documented way to discover its runtime context +// without parsing config or relying on forwarded host variables. +const ( + // EnvBBOXAgentName is the resolved agent name. + EnvBBOXAgentName = "BBOX_AGENT_NAME" + + // EnvBBOXWorkspace is the guest path to the mounted workspace. + EnvBBOXWorkspace = "BBOX_WORKSPACE" + + // EnvBBOXHome is the sandbox user's home directory inside the guest. + EnvBBOXHome = "BBOX_HOME" + + // EnvBBOXSessionID is the unique session identifier for this run. + EnvBBOXSessionID = "BBOX_SESSION_ID" + + // EnvBBOXGitTokenAvailable is "1" when a GitHub token is available to the + // guest's git credential helper, "0" otherwise. + EnvBBOXGitTokenAvailable = "BBOX_GIT_TOKEN_AVAILABLE" + + // EnvBBOXSSHAgentAvailable is "1" when SSH agent forwarding is active, + // "0" otherwise. + EnvBBOXSSHAgentAvailable = "BBOX_SSH_AGENT_AVAILABLE" + + // EnvBBOXMCPURL is the base URL of the in-VM MCP proxy. Set only when the + // MCP proxy is enabled. + EnvBBOXMCPURL = "BBOX_MCP_URL" + + // EnvBBOXMCPAuthzProfile is the effective MCP authorization profile. Set + // only when the MCP proxy is enabled. + EnvBBOXMCPAuthzProfile = "BBOX_MCP_AUTHZ_PROFILE" +) + +// UniversalEnvInput carries the primitive inputs needed to build the universal +// BBOX_* environment variables. Keeping it a flat value type keeps +// BuildUniversalEnv pure and trivially table-testable. +type UniversalEnvInput struct { + // AgentName is the resolved agent name. + AgentName string + + // Workspace is the guest path to the mounted workspace. + Workspace string + + // Home is the sandbox user's home directory inside the guest. + Home string + + // SessionID is the unique session identifier for this run. + SessionID string + + // GitTokenAvailable reports whether a GitHub token reaches the guest. + GitTokenAvailable bool + + // SSHAgentAvailable reports whether SSH agent forwarding is active. + SSHAgentAvailable bool + + // MCPURL is the base URL of the in-VM MCP proxy. Empty means the MCP + // proxy is disabled, in which case the BBOX_MCP_* keys are omitted. + MCPURL string + + // MCPAuthzProfile is the effective MCP authorization profile. Only used + // when MCPURL is non-empty. + MCPAuthzProfile string +} + +// BuildUniversalEnv returns the universal BBOX_* environment variables for a +// sandbox run. It is a pure function: the returned map contains exactly the +// BBOX_* keys and nothing else. +// +// The MCP keys (BBOX_MCP_URL, BBOX_MCP_AUTHZ_PROFILE) are included only when +// in.MCPURL is non-empty, so a disabled MCP proxy produces no MCP env vars. +// +// Callers should apply this map authoritatively — after forwarded host +// variables — so an untrusted host env cannot clobber BBOX_* values. +func BuildUniversalEnv(in UniversalEnvInput) map[string]string { + env := map[string]string{ + EnvBBOXAgentName: in.AgentName, + EnvBBOXWorkspace: in.Workspace, + EnvBBOXHome: in.Home, + EnvBBOXSessionID: in.SessionID, + EnvBBOXGitTokenAvailable: boolEnv(in.GitTokenAvailable), + EnvBBOXSSHAgentAvailable: boolEnv(in.SSHAgentAvailable), + } + if in.MCPURL != "" { + env[EnvBBOXMCPURL] = in.MCPURL + env[EnvBBOXMCPAuthzProfile] = in.MCPAuthzProfile + } + return env +} + +// boolEnv renders a bool as the canonical "1"/"0" env string. +func boolEnv(b bool) string { + if b { + return "1" + } + return "0" +} diff --git a/pkg/domain/agent/env_bbox_test.go b/pkg/domain/agent/env_bbox_test.go new file mode 100644 index 0000000..357f0d7 --- /dev/null +++ b/pkg/domain/agent/env_bbox_test.go @@ -0,0 +1,99 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package agent + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBuildUniversalEnv(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in UniversalEnvInput + want map[string]string + }{ + { + name: "mcp disabled, git and ssh off", + in: UniversalEnvInput{ + AgentName: "my-agent", + Workspace: "/workspace", + Home: "/home/sandbox", + SessionID: "abc123", + }, + want: map[string]string{ + EnvBBOXAgentName: "my-agent", + EnvBBOXWorkspace: "/workspace", + EnvBBOXHome: "/home/sandbox", + EnvBBOXSessionID: "abc123", + EnvBBOXGitTokenAvailable: "0", + EnvBBOXSSHAgentAvailable: "0", + }, + }, + { + name: "git and ssh on", + in: UniversalEnvInput{ + AgentName: "a", + Workspace: "/w", + Home: "/h", + SessionID: "s", + GitTokenAvailable: true, + SSHAgentAvailable: true, + }, + want: map[string]string{ + EnvBBOXAgentName: "a", + EnvBBOXWorkspace: "/w", + EnvBBOXHome: "/h", + EnvBBOXSessionID: "s", + EnvBBOXGitTokenAvailable: "1", + EnvBBOXSSHAgentAvailable: "1", + }, + }, + { + name: "mcp enabled adds url and authz", + in: UniversalEnvInput{ + AgentName: "a", + Workspace: "/w", + Home: "/h", + SessionID: "s", + MCPURL: "http://192.168.127.1:4483/mcp", + MCPAuthzProfile: "safe-tools", + }, + want: map[string]string{ + EnvBBOXAgentName: "a", + EnvBBOXWorkspace: "/w", + EnvBBOXHome: "/h", + EnvBBOXSessionID: "s", + EnvBBOXGitTokenAvailable: "0", + EnvBBOXSSHAgentAvailable: "0", + EnvBBOXMCPURL: "http://192.168.127.1:4483/mcp", + EnvBBOXMCPAuthzProfile: "safe-tools", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := BuildUniversalEnv(tt.in) + // Assert exact key set (and nothing else). + assert.Equal(t, tt.want, got) + }) + } +} + +func TestBuildUniversalEnv_NoMCPKeysWhenURLEmpty(t *testing.T) { + t.Parallel() + got := BuildUniversalEnv(UniversalEnvInput{ + AgentName: "a", + MCPAuthzProfile: "safe-tools", // must be ignored when URL is empty + }) + _, hasURL := got[EnvBBOXMCPURL] + _, hasAuthz := got[EnvBBOXMCPAuthzProfile] + assert.False(t, hasURL, "BBOX_MCP_URL must be absent when MCP is disabled") + assert.False(t, hasAuthz, "BBOX_MCP_AUTHZ_PROFILE must be absent when MCP is disabled") +} diff --git a/pkg/domain/config/config.go b/pkg/domain/config/config.go index 1c4c79e..fa07fca 100644 --- a/pkg/domain/config/config.go +++ b/pkg/domain/config/config.go @@ -481,19 +481,57 @@ func StricterMCPAuthzProfile(a, b string) string { } // MCPAgentOverride holds the subset of MCP settings that can be -// overridden per-agent. Only Enabled (gate) and Authz (tighten-only) -// have runtime effect at the per-agent level. +// overridden per-agent. Only Enabled (gate), Mode, and Authz +// (tighten-only) have runtime effect at the per-agent level. type MCPAgentOverride struct { // Enabled controls whether the MCP proxy is active for this agent. // nil means "no override" (inherit global setting), unlike // MCPConfig.Enabled where nil defaults to true. Enabled *bool `yaml:"enabled,omitempty"` + // Mode controls how the agent learns about the MCP proxy. + // "" (default) preserves legacy behavior: a config-file injector runs + // if the agent's plugin supplies one. "env" enables the proxy but runs + // NO config-file injector — the agent only learns the proxy via the + // universal BBOX_MCP_URL environment variable. "config" (declarative + // file injection) is reserved for a future release and rejected today. + Mode string `yaml:"mode,omitempty"` + // Authz overrides the MCP authorization profile for this agent. // Tighten-only: can only make the profile stricter, not more permissive. Authz *MCPAuthzConfig `yaml:"authz,omitempty"` } +// MCPEndpointPath is the HTTP path the vmcp proxy serves the MCP endpoint on. +// It is the single source of truth shared by the proxy server +// (internal/infra/mcp), the universal BBOX_MCP_URL env var (pkg/sandbox), and +// every built-in client's config injector (pkg/clients/*) so the served path +// can never drift between producers and consumers. +const MCPEndpointPath = "/mcp" + +const ( + // MCPModeEnv enables the MCP proxy and exposes it to the agent only via + // the universal BBOX_MCP_URL environment variable (no config-file + // injection). + MCPModeEnv = "env" + + // MCPModeConfig requests declarative config-file injection. Reserved for + // a future release; rejected by ValidateCustomAgent today. + MCPModeConfig = "config" +) + +// IsValidMCPMode reports whether the given MCP mode is accepted in this +// release. The empty string (legacy behavior) and "env" are valid. +// "config" is recognized as a future value but is NOT yet supported. +func IsValidMCPMode(mode string) bool { + switch mode { + case "", MCPModeEnv: + return true + default: + return false + } +} + // IsEnabled returns whether the MCP proxy is enabled. // Defaults to true when Enabled is nil. func (m MCPConfig) IsEnabled() bool { @@ -634,17 +672,92 @@ type EgressHostConfig struct { Protocol uint8 `yaml:"protocol,omitempty"` } -// AgentOverride allows users to override built-in agent settings. +// AgentCredentialsConfig configures credential persistence for a custom +// agent. Persist lists relative paths (from the sandbox user's home) whose +// contents are saved between sessions for authentication. +type AgentCredentialsConfig struct { + // Persist lists relative home-directory paths to persist between + // sessions. Paths must be relative and must not escape the home + // directory (no leading "/" and no ".." element). + Persist []string `yaml:"persist,omitempty"` +} + +// AgentSettingsEntryConfig is the YAML representation of a single settings +// injection entry for a custom agent. It mirrors settings.Entry. +type AgentSettingsEntryConfig struct { + // Category groups entries for enable/disable control (e.g. "settings", + // "rules", "skills"). + Category string `yaml:"category,omitempty"` + + // HostPath is relative to $HOME on the host. + HostPath string `yaml:"host_path"` + + // GuestPath is relative to the guest home directory. Defaults to HostPath + // when empty. + GuestPath string `yaml:"guest_path,omitempty"` + + // Kind selects how the entry is processed: "file", "directory", or + // "merge-file". + Kind string `yaml:"kind,omitempty"` + + // Format identifies the file format for "merge-file" entries: + // "json", "toml", or "jsonc". + Format string `yaml:"format,omitempty"` + + // Optional means skip silently if the host source does not exist. + Optional bool `yaml:"optional,omitempty"` + + // AllowKeys lists top-level keys to keep for "merge-file" entries. Empty + // means copy all keys. + AllowKeys []string `yaml:"allow_keys,omitempty"` +} + +// AgentOverride allows users to override built-in agent settings and to +// declare a brand-new custom (bring-your-own) agent entirely from config. +// +// For built-in agents, the zero value of each field means "inherit". For a +// custom agent (a key absent from the built-in registry), the fields below +// fully describe the agent — see AgentFromOverride for the pure mapping into +// an agent.Agent and ValidateCustomAgent for the load-time checks. +// +// Security note: custom agents may ONLY be declared in the global config or +// via CLI. Workspace-local .broodbox.yaml cannot add a new custom agent or +// introduce new credential paths — see MergeConfigs. type AgentOverride struct { // Image overrides the OCI image reference. Image string `yaml:"image,omitempty"` + // Description is a human-readable description of a custom agent. Purely + // informational (surfaced by `bbox agents inspect`). + Description string `yaml:"description,omitempty"` + // Command overrides the entrypoint command. Command []string `yaml:"command,omitempty"` - // EnvForward overrides the env forwarding patterns. + // EnvForward overrides the env forwarding patterns. Custom agents default + // to an EMPTY list (no host env is forwarded) to avoid accidental secret + // leakage; operators opt in explicitly. EnvForward []string `yaml:"env_forward,omitempty"` + // DefaultEnv contains environment variables always set in the VM for this + // agent. Applied before forwarded variables, so a forwarded variable with + // the same name takes precedence. + DefaultEnv map[string]string `yaml:"default_env,omitempty"` + + // EnvRequired lists environment variable names that must be present in the + // host environment for this agent to run. Used by `bbox agents inspect` + // and `bbox agents doctor` to surface missing configuration; the values + // themselves are never read or printed. + EnvRequired []string `yaml:"env_required,omitempty"` + + // Credentials configures credential persistence for this agent. + // OFF unless explicitly declared. + Credentials *AgentCredentialsConfig `yaml:"credentials,omitempty"` + + // Settings declares host settings to inject into the VM for this agent. + // OFF unless explicitly declared. + Settings []AgentSettingsEntryConfig `yaml:"settings,omitempty"` + // CPUs overrides the vCPU count. CPUs uint32 `yaml:"cpus,omitempty"` @@ -656,10 +769,18 @@ type AgentOverride struct { // human-readable values like "512m" or "2g". TmpSize bytesize.ByteSize `yaml:"tmp_size,omitempty"` - // EgressProfile overrides the agent's default egress profile. + // EgressProfile overrides the agent's default egress profile. Custom + // agents default to "standard" when this is empty (built-ins keep their + // own built-in default). EgressProfile string `yaml:"egress_profile,omitempty"` - // AllowHosts are additional egress hosts for this agent. + // EgressHosts maps egress profile names ("standard", "locked", ...) to the + // hosts allowed for that profile. Required for a custom agent whose + // effective profile is non-permissive (egress.Resolve needs hosts). + EgressHosts map[string][]EgressHostConfig `yaml:"egress_hosts,omitempty"` + + // AllowHosts are additional egress hosts for this agent (additive on top + // of the profile's host list). AllowHosts []EgressHostConfig `yaml:"allow_hosts,omitempty"` // MCP overrides the global MCP proxy settings for this agent. @@ -698,7 +819,10 @@ func clampTmpSize(s bytesize.ByteSize) bytesize.ByteSize { // - Defaults.EgressProfile: local can only tighten (not widen). // - Network.AllowHosts: additive (global + local). // - MCP.SessionTTL: local value is IGNORED (operational, not policy). -// - Agents map: local extends/overrides global per key. +// - Agents map: local may only TIGHTEN existing global agents. A +// local-only agent key (a new custom agent) is DROPPED. Local Image, +// Command, and credential paths are ignored, and local EnvForward may +// only narrow (subset of) the global allowlist (security #7). // // Returns global unchanged when local is nil. func MergeConfigs(global, local *Config) *Config { @@ -800,29 +924,28 @@ func MergeConfigs(global, local *Config) *Config { // authoritative. // (result.MCP.SessionTTL already carries the global value via *global copy.) - // Agents: local extends/overrides global per key. - // Security fields (EgressProfile, MCP.Authz) use tighten-only merge. - // Resource/identity fields use local-overrides-when-non-zero. - // AllowHosts are additive. - if len(local.Agents) > 0 { - if result.Agents == nil { - result.Agents = make(map[string]AgentOverride) - } else { - // Copy the global map to avoid mutating the original. - merged := make(map[string]AgentOverride, len(result.Agents)+len(local.Agents)) - for k, v := range result.Agents { - merged[k] = v - } - result.Agents = merged + // Agents: local may only TIGHTEN existing global agents. + // + // Security (#7): workspace-local config must NOT be able to introduce a + // brand-new custom agent (a key absent from the global config) — a + // repository could otherwise ship a malicious agent definition. Such keys + // are DROPPED here. For existing agents, mergeAgentOverride applies + // tighten-only/additive semantics and additionally ignores any local + // Image, Command, and credential paths, and narrows (never widens) + // EnvForward (local config cannot widen credential persistence or + // repoint/forward additional host env into the agent). + if len(local.Agents) > 0 && len(result.Agents) > 0 { + // Copy the global map to avoid mutating the original. + merged := make(map[string]AgentOverride, len(result.Agents)) + for k, v := range result.Agents { + merged[k] = v } - for k, v := range local.Agents { - if ga, ok := result.Agents[k]; ok { - v = mergeAgentOverride(ga, v) - } else { - // New agent from local — still sanitize security fields. - v = sanitizeNewAgentOverride(v) + result.Agents = merged + for k, lv := range local.Agents { + if gv, ok := result.Agents[k]; ok { + result.Agents[k] = mergeAgentOverride(gv, lv) } - result.Agents[k] = v + // else: local-only agent key is dropped (cannot add a new agent). } } @@ -997,26 +1120,38 @@ func TightenSettingsCategories(global, local *SettingsCategoryConfig) *SettingsC // mergeAgentOverride merges a local (workspace) agent override into a global one. // Rules mirror the top-level MergeConfigs patterns: -// - Resource/identity fields (Image, Command, EnvForward, CPUs, Memory, TmpSize): -// local overrides global when non-zero. +// - Resource fields (CPUs, Memory, TmpSize): local overrides global when non-zero. // - EgressProfile: tighten-only via egress.Stricter. // - AllowHosts: additive (global + local). -// - MCP.Enabled: local overrides global. +// - MCP.Enabled / MCP.Mode: local overrides global. // - MCP.Authz: tighten-only via MergeMCPAuthzConfig. // - SettingsImport: tighten-only via mergeSettingsImportConfig. +// - EnvForward: tighten-only — local may only NARROW (subset of) the global +// allowlist, never add new names/prefixes. +// +// Security (#7): the following identity/credential fields from local config +// are IGNORED entirely — they are sourced from the global config only: +// - Image, Command (a repository must not be able to repoint a built-in or +// declared agent at an arbitrary image or change its entrypoint) +// - Credentials.Persist (local cannot add new credential paths) +// - Description, DefaultEnv, EnvRequired, Settings, EgressHosts +// (declarative custom-agent identity must come from the trusted global +// config; a repository must not redefine an agent's settings/credentials) func mergeAgentOverride(global, local AgentOverride) AgentOverride { result := global - // Resource/identity: local overrides when non-zero. - if local.Image != "" { - result.Image = local.Image - } - if len(local.Command) > 0 { - result.Command = local.Command - } + // Image/Command: IGNORED from local config (sourced from global only). + // Allowing local to override these would let an untrusted repository + // repoint an agent at an arbitrary image or change its command. + + // EnvForward: tighten-only. Local may only NARROW the global allowlist + // (keep a subset), never add new names/prefixes — otherwise a repository's + // .broodbox.yaml could exfiltrate host secrets (e.g. GITHUB_TOKEN) into the + // VM by widening the forwarded-env set. if len(local.EnvForward) > 0 { - result.EnvForward = local.EnvForward + result.EnvForward = intersectEnvForward(result.EnvForward, local.EnvForward) } + if local.CPUs > 0 { result.CPUs = local.CPUs } @@ -1059,6 +1194,9 @@ func mergeAgentOverride(global, local AgentOverride) AgentOverride { if local.MCP.Enabled != nil { result.MCP.Enabled = local.MCP.Enabled } + if local.MCP.Mode != "" { + result.MCP.Mode = local.MCP.Mode + } result.MCP.Authz = MergeMCPAuthzConfig(result.MCP.Authz, local.MCP.Authz) } @@ -1079,16 +1217,84 @@ func mergeAgentOverride(global, local AgentOverride) AgentOverride { return result } -// sanitizeNewAgentOverride strips security-sensitive fields that a workspace -// config must not set on a brand-new agent (one with no global counterpart). -// "custom" MCP authz profile is blocked (same as MergeMCPAuthzConfig). -func sanitizeNewAgentOverride(ao AgentOverride) AgentOverride { - if ao.MCP != nil && ao.MCP.Authz != nil && ao.MCP.Authz.Profile == MCPAuthzProfileCustom { - sanitized := *ao.MCP - sanitized.Authz = nil - ao.MCP = &sanitized +// intersectEnvForward returns the subset of local patterns that are "covered" +// by the global allowlist — i.e. every host env var a local pattern could match +// is also matched by some global pattern. This enforces tighten-only semantics: +// workspace-local config may only NARROW the forwarded-env set, never add new +// names or prefixes. A local pattern that is not fully covered by global is +// dropped, so a malicious .broodbox.yaml cannot exfiltrate host secrets (e.g. +// GITHUB_TOKEN, AWS_*) that the trusted global config never forwarded. +func intersectEnvForward(global, local []string) []string { + if len(global) == 0 { + // Global forwards nothing; local cannot widen, so the result is empty. + return nil + } + var result []string + for _, lp := range local { + if envPatternCoveredBy(lp, global) { + result = append(result, lp) + } + } + return result +} + +// envPatternCoveredBy reports whether every key matched by pattern is also +// matched by at least one pattern in global. Patterns are exact names ("HOME") +// or trailing-star prefixes ("AWS_*"); leading-star and bare-"*" patterns are +// treated as matching nothing (mirroring matchPattern in pkg/domain/agent). +func envPatternCoveredBy(pattern string, global []string) bool { + for _, gp := range global { + if envPatternSubset(pattern, gp) { + return true + } + } + return false +} + +// envPatternSubset reports whether the set of keys matched by sub is a subset +// of the keys matched by sup. +// +// - sup is an exact name: sub is covered only if sub is the same exact name. +// - sup is a prefix glob "P*": sub is covered if sub is an exact name with +// prefix P, or a prefix glob "Q*" whose prefix Q starts with P (Q is at +// least as specific as P). +func envPatternSubset(sub, sup string) bool { + sub = strings.TrimSpace(sub) + sup = strings.TrimSpace(sup) + if sub == "" || sub == "*" || sup == "" || sup == "*" { + return false + } + if strings.HasPrefix(sub, "*") || strings.HasPrefix(sup, "*") { + // Leading-star patterns match nothing (see matchPattern); a pattern + // that matches nothing is trivially a subset, but we conservatively + // drop it rather than silently keep a useless pattern. + return false + } + + subGlob := strings.HasSuffix(sub, "*") + supGlob := strings.HasSuffix(sup, "*") + + switch { + case !supGlob: + // sup is an exact name: only the identical exact name is a subset. + return !subGlob && sub == sup + default: + // sup is a prefix glob "P*". + supPrefix := strings.TrimSuffix(sup, "*") + if supPrefix == "" { + return false + } + if subGlob { + subPrefix := strings.TrimSuffix(sub, "*") + if subPrefix == "" { + return false + } + // "Q*" ⊆ "P*" iff Q starts with P. + return strings.HasPrefix(subPrefix, supPrefix) + } + // Exact name ⊆ "P*" iff the name starts with P. + return strings.HasPrefix(sub, supPrefix) } - return ao } // ToEgressHosts converts config host entries to domain egress hosts. diff --git a/pkg/domain/config/config_test.go b/pkg/domain/config/config_test.go index d16803c..093de7c 100644 --- a/pkg/domain/config/config_test.go +++ b/pkg/domain/config/config_test.go @@ -108,7 +108,7 @@ func TestMergeConfigs(t *testing.T) { }, }, { - name: "agents map merge — local extends global", + name: "agents map merge — local cannot add a new agent (security #7)", global: &Config{ Agents: map[string]AgentOverride{ "a": {Image: "img-a"}, @@ -119,15 +119,16 @@ func TestMergeConfigs(t *testing.T) { "b": {Image: "img-b"}, }, }, + // Local-only key "b" is dropped: workspace config must not be + // able to introduce a brand-new custom agent. want: &Config{ Agents: map[string]AgentOverride{ "a": {Image: "img-a"}, - "b": {Image: "img-b"}, }, }, }, { - name: "agents map merge — local overrides global per key", + name: "agents map merge — local image override is ignored (security #7)", global: &Config{ Agents: map[string]AgentOverride{ "a": {Image: "old"}, @@ -138,25 +139,23 @@ func TestMergeConfigs(t *testing.T) { "a": {Image: "new"}, }, }, + // Image is sourced from global only; local cannot repoint it. want: &Config{ Agents: map[string]AgentOverride{ - "a": {Image: "new"}, + "a": {Image: "old"}, }, }, }, { - name: "local agents into nil global agents", + name: "local agents into nil global agents are dropped (security #7)", global: &Config{}, local: &Config{ Agents: map[string]AgentOverride{ "x": {Image: "img-x"}, }, }, - want: &Config{ - Agents: map[string]AgentOverride{ - "x": {Image: "img-x"}, - }, - }, + // No global agents to tighten, and local cannot add new ones. + want: &Config{}, }, { name: "egress profile tighten-only — local locked tightens global standard", @@ -1344,8 +1343,14 @@ func TestMergeConfigs_AgentOverridesMCPAuthzTightenOnly(t *testing.T) { wantAuthz: &MCPAuthzConfig{Profile: MCPAuthzProfileObserve}, }, { - name: "local sets per-agent authz when global has none", - global: &Config{}, + name: "local sets per-agent authz when global agent has none", + // Security: the agent key must already exist in global; a + // local-only key would be dropped (cannot add a new agent). + global: &Config{ + Agents: map[string]AgentOverride{ + "codex": {Image: "ghcr.io/example/codex:latest"}, + }, + }, local: &Config{ Agents: map[string]AgentOverride{ "codex": { @@ -1359,8 +1364,12 @@ func TestMergeConfigs_AgentOverridesMCPAuthzTightenOnly(t *testing.T) { wantAuthz: &MCPAuthzConfig{Profile: MCPAuthzProfileSafeTools}, }, { - name: "custom from local per-agent config is ignored", - global: &Config{}, + name: "custom from local per-agent config is ignored", + global: &Config{ + Agents: map[string]AgentOverride{ + "codex": {Image: "ghcr.io/example/codex:latest"}, + }, + }, local: &Config{ Agents: map[string]AgentOverride{ "codex": { @@ -1592,7 +1601,7 @@ func TestMergeAgentOverride_FieldByField(t *testing.T) { }, }, { - name: "all resource fields override when non-zero", + name: "resource fields override when non-zero; image/command/env are not widened", global: AgentOverride{ Image: "old:v1", Command: []string{"old-cmd"}, @@ -1610,14 +1619,55 @@ func TestMergeAgentOverride_FieldByField(t *testing.T) { TmpSize: bytesize.ByteSize(1024), }, want: AgentOverride{ - Image: "new:v2", - Command: []string{"new-cmd", "--flag"}, - EnvForward: []string{"NEW_*"}, + // Image/Command are sourced from global only (local ignored). + Image: "old:v1", + Command: []string{"old-cmd"}, + // NEW_* is not covered by OLD_*, so it is dropped entirely. + EnvForward: nil, CPUs: 8, Memory: bytesize.ByteSize(8192), TmpSize: bytesize.ByteSize(1024), }, }, + { + name: "env_forward narrows to subset of global allowlist", + global: AgentOverride{ + EnvForward: []string{"ACME_*", "HOME", "PATH"}, + }, + local: AgentOverride{ + // Keep only ACME_API_KEY (covered by ACME_*) and HOME (exact); + // AWS_SECRET is not covered and is dropped. + EnvForward: []string{"ACME_API_KEY", "HOME", "AWS_SECRET"}, + }, + want: AgentOverride{ + EnvForward: []string{"ACME_API_KEY", "HOME"}, + }, + }, + { + name: "env_forward cannot widen — uncovered local names dropped", + global: AgentOverride{ + EnvForward: []string{"ACME_*"}, + }, + local: AgentOverride{ + EnvForward: []string{"GITHUB_TOKEN", "AWS_*", "OPENAI_API_KEY"}, + }, + want: AgentOverride{ + EnvForward: nil, + }, + }, + { + name: "env_forward local star cannot widen global exact name", + global: AgentOverride{ + EnvForward: []string{"ACME_KEY"}, + }, + local: AgentOverride{ + // "ACME_*" matches more than "ACME_KEY", so it is not a subset. + EnvForward: []string{"ACME_*"}, + }, + want: AgentOverride{ + EnvForward: nil, + }, + }, { name: "zero local is a no-op", global: AgentOverride{Image: "keep", CPUs: 4}, @@ -2030,3 +2080,136 @@ func TestMergeConfigs_ImagePullPolicy(t *testing.T) { }) } } + +func TestMergeConfigs_LocalCannotAddCustomAgent(t *testing.T) { + t.Parallel() + + global := &Config{ + Agents: map[string]AgentOverride{ + "existing": {Image: "ghcr.io/example/existing:latest"}, + }, + } + local := &Config{ + Agents: map[string]AgentOverride{ + "sneaky": {Image: "ghcr.io/evil/agent:latest", Command: []string{"pwn"}}, + }, + } + + result := MergeConfigs(global, local) + _, ok := result.Agents["sneaky"] + assert.False(t, ok, "workspace-local config must not be able to add a new custom agent") + _, ok = result.Agents["existing"] + assert.True(t, ok, "existing global agent must remain") +} + +func TestMergeConfigs_LocalCannotAddCredentialPaths(t *testing.T) { + t.Parallel() + + global := &Config{ + Agents: map[string]AgentOverride{ + "a": { + Image: "img", + Credentials: &AgentCredentialsConfig{Persist: []string{".global/creds"}}, + }, + }, + } + local := &Config{ + Agents: map[string]AgentOverride{ + "a": { + Credentials: &AgentCredentialsConfig{Persist: []string{".steal/creds"}}, + }, + }, + } + + result := MergeConfigs(global, local) + got := result.Agents["a"] + require.NotNil(t, got.Credentials) + assert.Equal(t, []string{".global/creds"}, got.Credentials.Persist, + "local config must not add or replace credential paths") +} + +// TestMergeConfigs_LocalCannotAddDeclarativeIdentityFields asserts security +// rule #7: an untrusted workspace-local .broodbox.yaml cannot inject the +// declarative-identity fields (Description, DefaultEnv, EnvRequired, Settings, +// EgressHosts) onto an existing global agent. These are sourced from global +// only; mergeAgentOverride simply never copies them from local. +func TestMergeConfigs_LocalCannotAddDeclarativeIdentityFields(t *testing.T) { + t.Parallel() + + global := &Config{ + Agents: map[string]AgentOverride{ + "a": {Image: "img"}, + }, + } + local := &Config{ + Agents: map[string]AgentOverride{ + "a": { + Description: "evil agent", + DefaultEnv: map[string]string{"INJECTED": "1"}, + EnvRequired: []string{"STEAL_TOKEN"}, + Settings: []AgentSettingsEntryConfig{ + {Category: "settings", HostPath: ".evil/cfg", Kind: "merge-file"}, + }, + EgressHosts: map[string][]EgressHostConfig{ + "standard": {{Name: "c2.evil.org"}}, + }, + }, + }, + } + + result := MergeConfigs(global, local) + got := result.Agents["a"] + assert.Empty(t, got.Description, "local must not inject Description") + assert.Nil(t, got.DefaultEnv, "local must not inject DefaultEnv") + assert.Nil(t, got.EnvRequired, "local must not inject EnvRequired") + assert.Nil(t, got.Settings, "local must not inject Settings") + assert.Nil(t, got.EgressHosts, "local must not inject EgressHosts") +} + +func TestMergeConfigs_LocalCanTightenExisting(t *testing.T) { + t.Parallel() + + global := &Config{ + Agents: map[string]AgentOverride{ + "a": { + Image: "img", + EgressProfile: string(egress.ProfileStandard), + MCP: &MCPAgentOverride{Authz: &MCPAuthzConfig{Profile: MCPAuthzProfileSafeTools}}, + }, + }, + } + local := &Config{ + Agents: map[string]AgentOverride{ + "a": { + EgressProfile: string(egress.ProfileLocked), + MCP: &MCPAgentOverride{Authz: &MCPAuthzConfig{Profile: MCPAuthzProfileObserve}}, + }, + }, + } + + result := MergeConfigs(global, local) + got := result.Agents["a"] + assert.Equal(t, string(egress.ProfileLocked), got.EgressProfile, "egress tightened") + require.NotNil(t, got.MCP) + require.NotNil(t, got.MCP.Authz) + assert.Equal(t, MCPAuthzProfileObserve, got.MCP.Authz.Profile, "authz tightened") +} + +func TestMergeConfigs_LocalMCPModeOverride(t *testing.T) { + t.Parallel() + + global := &Config{ + Agents: map[string]AgentOverride{ + "a": {Image: "img", MCP: &MCPAgentOverride{Mode: ""}}, + }, + } + local := &Config{ + Agents: map[string]AgentOverride{ + "a": {MCP: &MCPAgentOverride{Mode: MCPModeEnv}}, + }, + } + + result := MergeConfigs(global, local) + require.NotNil(t, result.Agents["a"].MCP) + assert.Equal(t, MCPModeEnv, result.Agents["a"].MCP.Mode) +} diff --git a/pkg/domain/config/custom_agent.go b/pkg/domain/config/custom_agent.go new file mode 100644 index 0000000..f2cf0b1 --- /dev/null +++ b/pkg/domain/config/custom_agent.go @@ -0,0 +1,314 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "fmt" + "path/filepath" + "strings" + + "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/bytesize" + "github.com/stacklok/brood-box/pkg/domain/egress" + "github.com/stacklok/brood-box/pkg/domain/settings" +) + +const ( + // DefaultCustomAgentEgressProfile is the egress profile applied to a + // custom (bring-your-own) agent when the override does not set one. + // Custom agents are restricted by default, so they must also declare + // egress_hosts for this profile (or override it to "permissive"). + DefaultCustomAgentEgressProfile = egress.ProfileStandard + + // DefaultCustomAgentMCPAuthzProfile is the MCP authorization profile + // applied to a custom agent when MCP is enabled and no explicit profile + // is set. Custom agents default to "safe-tools" (built-ins keep + // "full-access"). + DefaultCustomAgentMCPAuthzProfile = MCPAuthzProfileSafeTools + + // DefaultCustomAgentCPUs is the CPU floor applied to a custom agent when + // neither the override nor the global defaults specify a value. Mirrors the + // built-in agents' DefaultCPUs so a copy-pasted custom agent boots instead + // of being passed 0 vCPUs (which the runner rejects). + DefaultCustomAgentCPUs = 2 + + // DefaultCustomAgentMemory is the memory floor (in MiB) applied to a custom + // agent when neither the override nor the global defaults specify a value. + // Mirrors the built-in agents' DefaultMemory. + DefaultCustomAgentMemory bytesize.ByteSize = 4096 +) + +// AgentFromOverride builds a fully-populated agent.Agent from a custom-agent +// override. It is a pure function (no I/O) so it can be table-tested and +// reused by both the CLI registration path and the `bbox agents` commands. +// +// Resource defaults (CPUs, Memory, TmpSize) follow the same precedence as +// config.Merge: the override value wins when non-zero, otherwise the global +// default applies. EnvForward is copied verbatim (an empty list stays empty, +// per the custom-agent default of forwarding nothing). EgressProfile defaults +// to DefaultCustomAgentEgressProfile when unset. Settings and CredentialPaths +// are only populated when the override declares them. +// +// MCP authorization is not resolved here. The safe-tools default for custom +// agents (DefaultCustomAgentMCPAuthzProfile) is applied at the composition +// root (cmd/bbox/main.go) where the MCP provider is wired, because authz is +// resolved against CLI flags, global config, inferred custom policies, and the +// tighten-only per-agent override — none of which are available in this pure +// mapping function. +func AgentFromOverride(name string, o AgentOverride, defaults DefaultsConfig) (agent.Agent, error) { + result := agent.Agent{ + Name: name, + Image: o.Image, + Command: append([]string(nil), o.Command...), + EnvForward: append([]string(nil), o.EnvForward...), + DefaultEnv: copyStringMap(o.DefaultEnv), + DefaultCPUs: o.CPUs, + } + + // Resources: override > global default > zero (go-microvm/agent default). + if o.CPUs == 0 && defaults.CPUs > 0 { + result.DefaultCPUs = defaults.CPUs + } + if o.Memory > 0 { + result.DefaultMemory = o.Memory + } else if defaults.Memory > 0 { + result.DefaultMemory = defaults.Memory + } + if o.TmpSize > 0 { + result.DefaultTmpSize = o.TmpSize + } else if defaults.TmpSize > 0 { + result.DefaultTmpSize = defaults.TmpSize + } + + // Safe minimum fallback: when neither the override nor the global defaults + // provide CPUs/Memory, apply the built-in floor so the agent can actually + // boot. Passing 0 to the runner is rejected ("num_vcpus must be > 0" / + // "ram_mib must be > 0"). + if result.DefaultCPUs == 0 { + result.DefaultCPUs = DefaultCustomAgentCPUs + } + if result.DefaultMemory == 0 { + result.DefaultMemory = DefaultCustomAgentMemory + } + + // Clamp to configured maximums, mirroring config.Merge. + result.DefaultCPUs, result.DefaultMemory = clampResources(result.DefaultCPUs, result.DefaultMemory) + result.DefaultTmpSize = clampTmpSize(result.DefaultTmpSize) + + // Egress profile: default to "standard" for custom agents when unset. + if o.EgressProfile != "" { + result.DefaultEgressProfile = egress.ProfileName(o.EgressProfile) + } else { + result.DefaultEgressProfile = DefaultCustomAgentEgressProfile + } + + // Egress hosts per profile. + if len(o.EgressHosts) > 0 { + hosts := make(map[egress.ProfileName][]egress.Host, len(o.EgressHosts)) + for profile, entries := range o.EgressHosts { + converted, err := ToEgressHosts(entries) + if err != nil { + return agent.Agent{}, fmt.Errorf("egress_hosts[%q]: %w", profile, err) + } + hosts[egress.ProfileName(profile)] = converted + } + result.EgressHosts = hosts + } + + // Credential paths (off unless declared). + if o.Credentials != nil && len(o.Credentials.Persist) > 0 { + result.CredentialPaths = append([]string(nil), o.Credentials.Persist...) + } + + // Settings manifest (off unless declared). + if len(o.Settings) > 0 { + manifest, err := settingsManifestFromConfig(o.Settings) + if err != nil { + return agent.Agent{}, err + } + result.SettingsManifest = manifest + } + + return result, nil +} + +// settingsManifestFromConfig maps the YAML settings entries into a +// settings.Manifest. Returns an error on an unknown kind. +func settingsManifestFromConfig(entries []AgentSettingsEntryConfig) (*settings.Manifest, error) { + out := make([]settings.Entry, 0, len(entries)) + for i, e := range entries { + kind, err := entryKindFromString(e.Kind) + if err != nil { + return nil, fmt.Errorf("settings[%d]: %w", i, err) + } + guestPath := e.GuestPath + if guestPath == "" { + guestPath = e.HostPath + } + entry := settings.Entry{ + Category: e.Category, + HostPath: e.HostPath, + GuestPath: guestPath, + Kind: kind, + Optional: e.Optional, + Format: e.Format, + } + if kind == settings.KindMergeFile && len(e.AllowKeys) > 0 { + entry.Filter = &settings.FieldFilter{ + AllowKeys: append([]string(nil), e.AllowKeys...), + } + } + out = append(out, entry) + } + return &settings.Manifest{Entries: out}, nil +} + +// entryKindFromString maps a YAML kind string to a settings.EntryKind. +func entryKindFromString(kind string) (settings.EntryKind, error) { + switch kind { + case "", "file": + return settings.KindFile, nil + case "directory": + return settings.KindDirectory, nil + case "merge-file": + return settings.KindMergeFile, nil + default: + return 0, fmt.Errorf("unknown settings kind %q: valid values are file, directory, merge-file", kind) + } +} + +// ValidateCustomAgent runs all load-time, pure checks for a custom agent +// declared in config. It never performs I/O. The optional imageRefValidator +// is invoked on the image reference when non-nil — the composition root +// supplies a closure wrapping go-containerregistry's name.ParseReference so +// the domain layer stays free of that dependency. +// +// Checks: valid agent name; non-empty image (and parseable when a validator +// is supplied); non-empty command; valid env_forward patterns; well-formed +// env_required names; safe relative credential and settings paths; valid +// settings kinds/formats; valid egress hostnames; a hosts list for any +// non-permissive effective egress profile; and a supported MCP mode. +func ValidateCustomAgent(name string, o AgentOverride, imageRefValidator func(string) error) error { + if err := agent.ValidateName(name); err != nil { + return err + } + if o.Image == "" { + return fmt.Errorf("agent %q: image is required", name) + } + if imageRefValidator != nil { + if err := imageRefValidator(o.Image); err != nil { + return fmt.Errorf("agent %q: invalid image reference %q: %w", name, o.Image, err) + } + } + if len(o.Command) == 0 { + return fmt.Errorf("agent %q: command is required", name) + } + if err := agent.ValidateEnvForwardPatterns(o.EnvForward); err != nil { + return fmt.Errorf("agent %q: %w", name, err) + } + for i, envName := range o.EnvRequired { + if strings.TrimSpace(envName) == "" { + return fmt.Errorf("agent %q: env_required[%d]: empty name", name, i) + } + if strings.Contains(envName, "=") { + return fmt.Errorf("agent %q: env_required[%d]: name %q must not contain '='", name, i, envName) + } + } + + // MCP mode. + if o.MCP != nil { + if o.MCP.Mode == MCPModeConfig { + return fmt.Errorf("agent %q: mcp.mode: config not supported in this version", name) + } + if !IsValidMCPMode(o.MCP.Mode) { + return fmt.Errorf("agent %q: mcp.mode %q: valid values are %q", name, o.MCP.Mode, MCPModeEnv) + } + } + + // Credential paths must be safe relative paths. + if o.Credentials != nil { + for i, p := range o.Credentials.Persist { + if err := safeRelPath(p); err != nil { + return fmt.Errorf("agent %q: credentials.persist[%d]: %w", name, i, err) + } + } + } + + // Settings entries: safe paths + valid kind/format. + for i, e := range o.Settings { + if err := safeRelPath(e.HostPath); err != nil { + return fmt.Errorf("agent %q: settings[%d].host_path: %w", name, i, err) + } + guestPath := e.GuestPath + if guestPath == "" { + guestPath = e.HostPath + } + if err := safeRelPath(guestPath); err != nil { + return fmt.Errorf("agent %q: settings[%d].guest_path: %w", name, i, err) + } + if _, err := entryKindFromString(e.Kind); err != nil { + return fmt.Errorf("agent %q: settings[%d]: %w", name, i, err) + } + } + + // Egress hosts: validate every hostname. + for profile, entries := range o.EgressHosts { + if _, err := ToEgressHosts(entries); err != nil { + return fmt.Errorf("agent %q: egress_hosts[%q]: %w", name, profile, err) + } + } + + // Effective egress profile must have hosts unless permissive. + effectiveProfile := DefaultCustomAgentEgressProfile + if o.EgressProfile != "" { + effectiveProfile = egress.ProfileName(o.EgressProfile) + } + if effectiveProfile != egress.ProfilePermissive { + if len(o.EgressHosts[string(effectiveProfile)]) == 0 { + return fmt.Errorf( + "agent %q: egress profile %q requires egress_hosts[%q] (or set egress_profile: permissive)", + name, effectiveProfile, effectiveProfile) + } + } + + return nil +} + +// safeRelPath validates that p is a non-empty relative path that does not +// escape the sandbox home directory: no leading "/", no ".." element, and not +// "." after cleaning. This is the single source of truth for path safety on +// custom-agent credential and settings paths. +func safeRelPath(p string) error { + if p == "" { + return fmt.Errorf("empty path") + } + if strings.HasPrefix(p, "/") { + return fmt.Errorf("absolute path %q not allowed (must be relative to home)", p) + } + cleaned := filepath.Clean(p) + if cleaned == "." { + return fmt.Errorf("path %q resolves to the home directory itself", p) + } + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return fmt.Errorf("path %q escapes the home directory", p) + } + for _, seg := range strings.Split(filepath.ToSlash(cleaned), "/") { + if seg == ".." { + return fmt.Errorf("path %q contains a %q element", p, "..") + } + } + return nil +} + +// copyStringMap returns a shallow copy of m, or nil when m is empty. +func copyStringMap(m map[string]string) map[string]string { + if len(m) == 0 { + return nil + } + out := make(map[string]string, len(m)) + for k, v := range m { + out[k] = v + } + return out +} diff --git a/pkg/domain/config/custom_agent_test.go b/pkg/domain/config/custom_agent_test.go new file mode 100644 index 0000000..7a65536 --- /dev/null +++ b/pkg/domain/config/custom_agent_test.go @@ -0,0 +1,368 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package config + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/brood-box/pkg/domain/agent" + "github.com/stacklok/brood-box/pkg/domain/bytesize" + "github.com/stacklok/brood-box/pkg/domain/egress" + "github.com/stacklok/brood-box/pkg/domain/settings" +) + +func TestAgentFromOverride(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + agent string + override AgentOverride + defaults DefaultsConfig + assert func(t *testing.T, a agent.Agent) + }{ + { + name: "full mapping", + agent: "my-agent", + override: AgentOverride{ + Image: "ghcr.io/acme/my-agent:latest", + Command: []string{"my-agent", "--interactive"}, + Description: "ACME agent", + EnvForward: []string{"ACME_API_KEY", "ACME_*"}, + DefaultEnv: map[string]string{"ACME_MODE": "interactive"}, + CPUs: 4, + Memory: 4096, + TmpSize: 2048, + EgressProfile: string(egress.ProfileStandard), + EgressHosts: map[string][]EgressHostConfig{ + "standard": {{Name: "api.acme.dev", Ports: []uint16{443}, Protocol: 6}}, + }, + Credentials: &AgentCredentialsConfig{Persist: []string{".acme/credentials.json", ".config/acme/"}}, + Settings: []AgentSettingsEntryConfig{ + { + Category: "settings", + HostPath: ".config/acme/settings.json", + GuestPath: ".config/acme/settings.json", + Kind: "merge-file", + Format: "json", + Optional: true, + AllowKeys: []string{"theme", "editor"}, + }, + }, + }, + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, "my-agent", a.Name) + assert.Equal(t, "ghcr.io/acme/my-agent:latest", a.Image) + assert.Equal(t, []string{"my-agent", "--interactive"}, a.Command) + assert.Equal(t, []string{"ACME_API_KEY", "ACME_*"}, a.EnvForward) + assert.Equal(t, map[string]string{"ACME_MODE": "interactive"}, a.DefaultEnv) + assert.Equal(t, uint32(4), a.DefaultCPUs) + assert.Equal(t, bytesize.ByteSize(4096), a.DefaultMemory) + assert.Equal(t, bytesize.ByteSize(2048), a.DefaultTmpSize) + assert.Equal(t, egress.ProfileStandard, a.DefaultEgressProfile) + require.Len(t, a.EgressHosts[egress.ProfileStandard], 1) + assert.Equal(t, "api.acme.dev", a.EgressHosts[egress.ProfileStandard][0].Name) + assert.Equal(t, []string{".acme/credentials.json", ".config/acme/"}, a.CredentialPaths) + require.NotNil(t, a.SettingsManifest) + require.Len(t, a.SettingsManifest.Entries, 1) + e := a.SettingsManifest.Entries[0] + assert.Equal(t, settings.KindMergeFile, e.Kind) + require.NotNil(t, e.Filter) + assert.Equal(t, []string{"theme", "editor"}, e.Filter.AllowKeys) + }, + }, + { + name: "settings guest_path defaults to host_path and kinds map correctly", + agent: "settingsmap", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + Settings: []AgentSettingsEntryConfig{ + { + Category: "settings", + HostPath: ".config/acme/", + Kind: "directory", + // GuestPath omitted on purpose: must default to HostPath. + }, + { + Category: "settings", + HostPath: ".acme-rc", + // Kind omitted on purpose: must default to KindFile. + }, + }, + }, + assert: func(t *testing.T, a agent.Agent) { + require.NotNil(t, a.SettingsManifest) + require.Len(t, a.SettingsManifest.Entries, 2) + + dir := a.SettingsManifest.Entries[0] + assert.Equal(t, settings.KindDirectory, dir.Kind) + assert.Equal(t, ".config/acme/", dir.GuestPath, + "empty guest_path must default to host_path") + + file := a.SettingsManifest.Entries[1] + assert.Equal(t, settings.KindFile, file.Kind, + "unset kind must default to file") + assert.Equal(t, ".acme-rc", file.GuestPath, + "empty guest_path must default to host_path") + }, + }, + { + name: "empty env_forward stays empty", + agent: "minimal", + override: AgentOverride{ + Image: "ghcr.io/acme/minimal:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + }, + assert: func(t *testing.T, a agent.Agent) { + assert.Empty(t, a.EnvForward) + assert.Nil(t, a.DefaultEnv) + assert.Nil(t, a.CredentialPaths) + assert.Nil(t, a.SettingsManifest) + }, + }, + { + name: "egress profile defaults to standard when unset", + agent: "defprofile", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + }, + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, egress.ProfileStandard, a.DefaultEgressProfile) + }, + }, + { + name: "global defaults apply when override resources unset", + agent: "useglobal", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + }, + defaults: DefaultsConfig{CPUs: 2, Memory: 1024, TmpSize: 512}, + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, uint32(2), a.DefaultCPUs) + assert.Equal(t, bytesize.ByteSize(1024), a.DefaultMemory) + assert.Equal(t, bytesize.ByteSize(512), a.DefaultTmpSize) + }, + }, + { + name: "override resources win over global defaults", + agent: "overrideres", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + CPUs: 8, + Memory: 8192, + TmpSize: 4096, + }, + defaults: DefaultsConfig{CPUs: 2, Memory: 1024, TmpSize: 512}, + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, uint32(8), a.DefaultCPUs) + assert.Equal(t, bytesize.ByteSize(8192), a.DefaultMemory) + assert.Equal(t, bytesize.ByteSize(4096), a.DefaultTmpSize) + }, + }, + { + name: "safe minimum cpus/memory applied when override and defaults are zero", + agent: "barebones", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + }, + // No global defaults set either. + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, uint32(DefaultCustomAgentCPUs), a.DefaultCPUs, + "a custom agent with no resources must fall back to a bootable CPU floor") + assert.Equal(t, DefaultCustomAgentMemory, a.DefaultMemory, + "a custom agent with no resources must fall back to a bootable memory floor") + assert.NotZero(t, a.DefaultCPUs) + assert.NotZero(t, a.DefaultMemory) + }, + }, + { + name: "resources clamped to configured maximums", + agent: "greedy", + override: AgentOverride{ + Image: "ghcr.io/acme/x:latest", + Command: []string{"run"}, + EgressProfile: string(egress.ProfilePermissive), + CPUs: MaxCPUs + 64, + Memory: MaxMemory + 1024, + TmpSize: MaxTmpSize + 1024, + }, + assert: func(t *testing.T, a agent.Agent) { + assert.Equal(t, MaxCPUs, a.DefaultCPUs) + assert.Equal(t, MaxMemory, a.DefaultMemory) + assert.Equal(t, MaxTmpSize, a.DefaultTmpSize) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ag, err := AgentFromOverride(tt.agent, tt.override, tt.defaults) + require.NoError(t, err) + tt.assert(t, ag) + }) + } +} + +func TestAgentFromOverride_UnknownSettingsKind(t *testing.T) { + t.Parallel() + _, err := AgentFromOverride("x", AgentOverride{ + Image: "img", + Command: []string{"run"}, + Settings: []AgentSettingsEntryConfig{ + {HostPath: "a", Kind: "bogus"}, + }, + }, DefaultsConfig{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown settings kind") +} + +func TestValidateCustomAgent(t *testing.T) { + t.Parallel() + + valid := AgentOverride{ + Image: "ghcr.io/acme/my-agent:latest", + Command: []string{"my-agent"}, + EnvForward: []string{"ACME_*"}, + EgressProfile: string(egress.ProfileStandard), + EgressHosts: map[string][]EgressHostConfig{ + "standard": {{Name: "api.acme.dev", Ports: []uint16{443}}}, + }, + } + + tests := []struct { + name string + agentName string + override AgentOverride + wantErr string + }{ + {name: "valid", agentName: "my-agent", override: valid}, + {name: "invalid name", agentName: "../evil", override: valid, wantErr: "invalid agent name"}, + {name: "missing image", agentName: "x", override: AgentOverride{Command: []string{"run"}, EgressProfile: "permissive"}, wantErr: "image is required"}, + {name: "missing command", agentName: "x", override: AgentOverride{Image: "img", EgressProfile: "permissive"}, wantErr: "command is required"}, + { + name: "bare star env_forward", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EnvForward: []string{"*"}, EgressProfile: "permissive"}, + wantErr: "bare", + }, + { + name: "leading star env_forward", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EnvForward: []string{"*_KEY"}, EgressProfile: "permissive"}, + wantErr: "leading-star", + }, + { + name: "env_required with equals", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EnvRequired: []string{"FOO=bar"}, EgressProfile: "permissive"}, + wantErr: "must not contain", + }, + { + name: "env_required empty name", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EnvRequired: []string{""}, EgressProfile: "permissive"}, + wantErr: "empty name", + }, + { + name: "env_required whitespace name", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EnvRequired: []string{" "}, EgressProfile: "permissive"}, + wantErr: "empty name", + }, + { + name: "absolute credential path", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + Credentials: &AgentCredentialsConfig{Persist: []string{"/etc/passwd"}}}, + wantErr: "absolute path", + }, + { + name: "dotdot credential path", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + Credentials: &AgentCredentialsConfig{Persist: []string{"../escape"}}}, + wantErr: "escapes", + }, + { + name: "empty credential path", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + Credentials: &AgentCredentialsConfig{Persist: []string{""}}}, + wantErr: "empty path", + }, + { + name: "settings absolute host_path", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + Settings: []AgentSettingsEntryConfig{{HostPath: "/abs"}}}, + wantErr: "absolute path", + }, + { + name: "settings dotdot guest_path", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + Settings: []AgentSettingsEntryConfig{{HostPath: "ok", GuestPath: "a/../../b"}}}, + wantErr: "escapes", + }, + { + name: "bad egress hostname", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "standard", + EgressHosts: map[string][]EgressHostConfig{"standard": {{Name: "1.2.3.4"}}}}, + wantErr: "IP address", + }, + { + name: "non-permissive profile without hosts", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "standard"}, + wantErr: "requires egress_hosts", + }, + { + name: "default profile (standard) without hosts", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}}, + wantErr: "requires egress_hosts", + }, + { + name: "mcp mode config rejected", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + MCP: &MCPAgentOverride{Mode: MCPModeConfig}}, + wantErr: "not supported in this version", + }, + { + name: "mcp mode env accepted", agentName: "x", + override: AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive", + MCP: &MCPAgentOverride{Mode: MCPModeEnv}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := ValidateCustomAgent(tt.agentName, tt.override, nil) + if tt.wantErr == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +func TestValidateCustomAgent_ImageRefValidator(t *testing.T) { + t.Parallel() + + o := AgentOverride{Image: "img", Command: []string{"run"}, EgressProfile: "permissive"} + + // nil validator: skipped. + require.NoError(t, ValidateCustomAgent("x", o, nil)) + + // failing validator surfaces. + fakeErr := errors.New("bad ref") + err := ValidateCustomAgent("x", o, func(string) error { return fakeErr }) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid image reference") +} diff --git a/pkg/sandbox/sandbox.go b/pkg/sandbox/sandbox.go index fdbda6d..584af6a 100644 --- a/pkg/sandbox/sandbox.go +++ b/pkg/sandbox/sandbox.go @@ -32,6 +32,21 @@ import ( "github.com/stacklok/brood-box/pkg/domain/workspace" ) +const ( + // gatewayIP is the host-side gateway address inside the go-microvm + // virtual network. It mirrors go-microvm's topology.GatewayIP. The + // application layer must not import go-microvm's topology package (it + // would break the "sandbox depends only on domain" rule), so the value + // is duplicated here as a stable contract. An infra-layer test + // (internal/infra/vm) asserts topology.GatewayIP equals this literal so + // drift is caught at build time. + gatewayIP = "192.168.127.1" + + // guestHomeDir is the sandbox user's home directory inside the guest VM. + // The guest runs as the "sandbox" user (see go-microvm guest setup). + guestHomeDir = "/home/sandbox" +) + // SnapshotOpts groups snapshot isolation options. type SnapshotOpts struct { // Enabled controls whether snapshot isolation is active. @@ -370,7 +385,22 @@ func (s *SandboxRunner) Prepare(ctx context.Context, agentName string, opts RunO ) } - // 3. Collect env vars: agent defaults → forwarded host vars → git identity. + // Resolve the effective MCP config early so the universal BBOX_MCP_URL is + // known while assembling the env map below. The host service itself is + // started later (step 4). + mcpCfg := s.resolveMCPConfig(cfg, agentName) + + // Honor the per-agent MCP mode. mode:"env" enables the proxy but runs NO + // config-file injection — the agent learns the proxy only via the + // universal BBOX_MCP_URL env var. Suppress the plugin's config-file + // injector in that case (it is a no-op for data-only custom agents whose + // Plugin is nil, but for built-ins the injector must be actively dropped). + if mcpMode(cfg, agentName) == config.MCPModeEnv { + mcpInjector = nil + } + + // 3. Collect env vars: agent defaults → forwarded host vars → git identity + // → authoritative universal BBOX_* vars. // Agent defaults are applied first so host-side overrides take precedence. envVars := make(map[string]string) for k, v := range ag.DefaultEnv { @@ -448,9 +478,13 @@ func (s *SandboxRunner) Prepare(ctx context.Context, agentName string, opts RunO // Determine if a GitHub token is available for credential helper injection. hasGitToken := opts.GitTokenEnabled && (envVars["GITHUB_TOKEN"] != "" || envVars["GH_TOKEN"] != "") - // 4. Set up MCP host services if enabled. + // 4. Set up MCP host services if enabled. This runs BEFORE the universal + // BBOX_* env injection so the MCP env vars (BBOX_MCP_URL / authz profile) + // are only injected when the proxy is actually available. Otherwise a BYO + // agent that trusts BBOX_MCP_URL would hit a dead endpoint when discovery + // fails. var hostServices []domvm.HostService - mcpCfg := s.resolveMCPConfig(cfg, agentName) + mcpAvailable := false if mcpCfg.IsEnabled() && s.mcpProvider != nil { _, mcpSpan := tracer.Start(ctx, "bbox.ConfigureMCP") s.observer.Start(progress.PhaseConfiguringMCP, "Discovering MCP servers...") @@ -464,11 +498,40 @@ func (s *SandboxRunner) Prepare(ctx context.Context, agentName string, opts RunO Name: svc.Name, Port: svc.Port, Handler: svc.Handler, }) } + mcpAvailable = len(hostServices) > 0 s.observer.Complete(fmt.Sprintf("MCP proxy ready on port %d", mcpCfg.Port)) } mcpSpan.End() } + // Inject the universal BBOX_* env vars AFTER forwarded host vars so they + // are authoritative: an untrusted host env (or env_forward allowlist) + // cannot clobber e.g. BBOX_AGENT_NAME. The MCP URL points at the in-VM + // gateway; see gatewayIP for the source of the literal. It is only set + // when MCP discovery actually produced services (mcpAvailable) so the + // guest never receives a URL for a proxy that is not running. + var mcpURL, mcpAuthzProfile string + if mcpAvailable { + mcpURL = fmt.Sprintf("http://%s:%d%s", gatewayIP, mcpCfg.Port, config.MCPEndpointPath) + mcpAuthzProfile = config.MCPAuthzProfileFullAccess + if mcpCfg.Authz != nil && mcpCfg.Authz.Profile != "" { + mcpAuthzProfile = mcpCfg.Authz.Profile + } + } + universalEnv := agent.BuildUniversalEnv(agent.UniversalEnvInput{ + AgentName: ag.Name, + Workspace: opts.Workspace, + Home: guestHomeDir, + SessionID: opts.SessionID, + GitTokenAvailable: hasGitToken, + SSHAgentAvailable: opts.SSHAgentForward, + MCPURL: mcpURL, + MCPAuthzProfile: mcpAuthzProfile, + }) + for k, v := range universalEnv { + envVars[k] = v + } + // 5. Set up workspace path (possibly with snapshot isolation). workspacePath := opts.Workspace var snap *workspace.Snapshot @@ -882,6 +945,19 @@ func (s *SandboxRunner) resolveSettingsManifest( return &settings.Manifest{Entries: filtered} } +// mcpMode returns the effective per-agent MCP mode from the agent override, +// or "" when no override sets it. It is read at runtime to decide whether the +// config-file injector must be suppressed (mode:"env"). +func mcpMode(cfg *SandboxConfig, agentName string) string { + if cfg == nil { + return "" + } + if override, ok := cfg.AgentOverrides[agentName]; ok && override.MCP != nil { + return override.MCP.Mode + } + return "" +} + // resolveMCPConfig returns the effective MCP configuration by merging // global config with any agent-specific Enabled override. // Per-agent authz is handled at the composition root (cmd/bbox/main.go) @@ -893,6 +969,15 @@ func (s *SandboxRunner) resolveMCPConfig(cfg *SandboxConfig, agentName string) c if override, ok := cfg.AgentOverrides[agentName]; ok && override.MCP != nil { if override.MCP.Enabled != nil { mcpCfg.Enabled = override.MCP.Enabled + } else if override.MCP.Mode == config.MCPModeEnv { + // mode:"env" implies the proxy must be enabled — that is the whole + // point of the mode (BBOX_MCP_URL is the agent's only discovery + // mechanism). Honor the documented invariant ("when set, MCP is + // enabled") so a custom agent that sets only mode:env still gets + // the proxy even when MCP is disabled globally. An explicit + // enabled:false is still respected (handled by the branch above). + enabled := true + mcpCfg.Enabled = &enabled } } diff --git a/pkg/sandbox/sandbox_test.go b/pkg/sandbox/sandbox_test.go index 9d19784..8ab2406 100644 --- a/pkg/sandbox/sandbox_test.go +++ b/pkg/sandbox/sandbox_test.go @@ -23,6 +23,7 @@ import ( "github.com/stacklok/brood-box/pkg/domain/agent" "github.com/stacklok/brood-box/pkg/domain/bytesize" domainconfig "github.com/stacklok/brood-box/pkg/domain/config" + "github.com/stacklok/brood-box/pkg/domain/credential" "github.com/stacklok/brood-box/pkg/domain/egress" "github.com/stacklok/brood-box/pkg/domain/hostservice" "github.com/stacklok/brood-box/pkg/domain/session" @@ -107,6 +108,10 @@ func (m *mockEnvProvider) Environ() []string { return m.vars } // require Plugin behavior, so the tests do not provide one. type mockRegistry struct { agents map[string]agent.Agent + // plugins optionally attaches a Plugin to a registered agent (keyed by + // name) so tests can exercise built-in behavior such as MCP config + // injection. Agents without an entry here get a nil Plugin (data-only). + plugins map[string]agent.Plugin } func (m *mockRegistry) Get(name string) (agent.ClientEntry, error) { @@ -114,17 +119,30 @@ func (m *mockRegistry) Get(name string) (agent.ClientEntry, error) { if !ok { return agent.ClientEntry{}, &agent.ErrNotFound{Name: name} } - return agent.ClientEntry{Agent: a}, nil + return agent.ClientEntry{Agent: a, Plugin: m.plugins[name]}, nil } func (m *mockRegistry) List() []agent.ClientEntry { result := make([]agent.ClientEntry, 0, len(m.agents)) for _, a := range m.agents { - result = append(result, agent.ClientEntry{Agent: a}) + result = append(result, agent.ClientEntry{Agent: a, Plugin: m.plugins[a.Name]}) } return result } +// mockPlugin is a test Plugin that returns a fixed MCPInjector. +type mockPlugin struct { + injector agent.MCPInjector +} + +func (p *mockPlugin) MCPConfig() agent.MCPInjector { return p.injector } +func (p *mockPlugin) Seeder() credential.Seeder { return nil } + +// mockMCPInjector is a no-op MCPInjector used to assert injector selection. +type mockMCPInjector struct{} + +func (mockMCPInjector) Inject(_, _ string, _ uint16, _ agent.ChownFunc) error { return nil } + // mockWorkspaceCloner records calls and returns a snapshot pointing to a temp dir. type mockWorkspaceCloner struct { createCalled bool @@ -250,7 +268,16 @@ func TestSandboxRunner_Run(t *testing.T) { assert.Equal(t, uint32(2), vmRunner.startCfg.CPUs) assert.Equal(t, bytesize.ByteSize(2048), vmRunner.startCfg.Memory) assert.Equal(t, "/tmp/workspace", vmRunner.startCfg.WorkspacePath) - assert.Equal(t, map[string]string{"TEST_KEY": "secret123", "GIT_TERMINAL_PROMPT": "0"}, vmRunner.startCfg.EnvVars) + assert.Equal(t, map[string]string{ + "TEST_KEY": "secret123", + "GIT_TERMINAL_PROMPT": "0", + agent.EnvBBOXAgentName: "test-agent", + agent.EnvBBOXWorkspace: "/tmp/workspace", + agent.EnvBBOXHome: guestHomeDir, + agent.EnvBBOXSessionID: "abcd1234", + agent.EnvBBOXGitTokenAvailable: "0", + agent.EnvBBOXSSHAgentAvailable: "0", + }, vmRunner.startCfg.EnvVars) // Verify terminal session was started. assert.Equal(t, "127.0.0.1", sessionRunner.runOpts.Host) @@ -848,7 +875,17 @@ func TestSandboxRunner_Prepare_Success(t *testing.T) { assert.Equal(t, VMName("test-agent", snapshotDir, "abcd1234"), sb.VMConfig.Name) assert.Equal(t, snapshotDir, sb.WorkspacePath) assert.NotNil(t, sb.Snapshot) - assert.Equal(t, map[string]string{"TEST_KEY": "secret123", "GIT_TERMINAL_PROMPT": "0"}, sb.EnvVars) + assert.Equal(t, map[string]string{ + "TEST_KEY": "secret123", + "GIT_TERMINAL_PROMPT": "0", + // Universal BBOX_* vars are always injected (MCP disabled here). + agent.EnvBBOXAgentName: "test-agent", + agent.EnvBBOXWorkspace: workspaceDir, + agent.EnvBBOXHome: guestHomeDir, + agent.EnvBBOXSessionID: "abcd1234", + agent.EnvBBOXGitTokenAvailable: "0", + agent.EnvBBOXSSHAgentAvailable: "0", + }, sb.EnvVars) assert.Equal(t, snapshot.NopMatcher, sb.DiffMatcher) assert.Equal(t, snapshotDir, vmRunner.startCfg.WorkspacePath) assert.True(t, cloner.createCalled) @@ -932,6 +969,292 @@ func TestSandboxRunner_Prepare_EnvForwardExtra(t *testing.T) { } } +func TestSandboxRunner_Prepare_InjectsUniversalBBOXEnv(t *testing.T) { + t.Parallel() + + testAgent := agent.Agent{ + Name: "test-agent", + Image: "img:latest", + Command: []string{"cmd"}, + DefaultCPUs: 2, + DefaultMemory: bytesize.ByteSize(2048), + } + + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + mcpProvider := &mockMCPProvider{ + services: []hostservice.Service{{Name: "mcp", Port: 4483}}, + } + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{agents: map[string]agent.Agent{"test-agent": testAgent}}, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{ + MCP: domainconfig.MCPConfig{ + Authz: &domainconfig.MCPAuthzConfig{Profile: domainconfig.MCPAuthzProfileSafeTools}, + }, + }, + EnvProvider: &mockEnvProvider{vars: []string{"GITHUB_TOKEN=ghp_secret"}}, + Logger: testLogger(), + MCPProvider: mcpProvider, + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + GitTokenEnabled: true, + EnvForwardExtra: []string{"GITHUB_TOKEN"}, + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + env := vmRunner.startCfg.EnvVars + assert.Equal(t, "test-agent", env[agent.EnvBBOXAgentName]) + assert.Equal(t, "/tmp/ws", env[agent.EnvBBOXWorkspace]) + assert.Equal(t, guestHomeDir, env[agent.EnvBBOXHome]) + assert.Equal(t, "abcd1234", env[agent.EnvBBOXSessionID]) + assert.Equal(t, "1", env[agent.EnvBBOXGitTokenAvailable], "git token was forwarded") + assert.Equal(t, "0", env[agent.EnvBBOXSSHAgentAvailable]) + assert.Equal(t, "http://"+gatewayIP+":4483"+domainconfig.MCPEndpointPath, env[agent.EnvBBOXMCPURL]) + assert.Equal(t, domainconfig.MCPAuthzProfileSafeTools, env[agent.EnvBBOXMCPAuthzProfile]) +} + +// TestSandboxRunner_Prepare_MCPModeEnv_SuppressesInjector asserts that when a +// built-in agent (with a plugin supplying an MCP config injector) has the +// per-agent mcp.mode set to "env", the config-file injector is dropped while +// the proxy stays enabled (BBOX_MCP_URL is still set). +func TestSandboxRunner_Prepare_MCPModeEnv_SuppressesInjector(t *testing.T) { + t.Parallel() + + testAgent := agent.Agent{ + Name: "test-agent", Image: "img:latest", Command: []string{"cmd"}, + DefaultCPUs: 2, DefaultMemory: bytesize.ByteSize(2048), + } + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + mcpProvider := &mockMCPProvider{services: []hostservice.Service{{Name: "mcp", Port: 4483}}} + injector := mockMCPInjector{} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{ + agents: map[string]agent.Agent{"test-agent": testAgent}, + plugins: map[string]agent.Plugin{"test-agent": &mockPlugin{injector: injector}}, + }, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{ + AgentOverrides: map[string]domainconfig.AgentOverride{ + "test-agent": {MCP: &domainconfig.MCPAgentOverride{Mode: domainconfig.MCPModeEnv}}, + }, + }, + EnvProvider: &mockEnvProvider{}, + Logger: testLogger(), + MCPProvider: mcpProvider, + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + assert.Nil(t, vmRunner.startCfg.MCPConfigInjector, + "mcp.mode:env must suppress the config-file injector") + assert.Equal(t, "http://"+gatewayIP+":4483"+domainconfig.MCPEndpointPath, vmRunner.startCfg.EnvVars[agent.EnvBBOXMCPURL], + "mcp.mode:env must still enable the proxy via BBOX_MCP_URL") +} + +// TestSandboxRunner_Prepare_MCPDefaultMode_UsesInjector is the companion to the +// mode:env test: with no mode override, a plugin's injector is used. +func TestSandboxRunner_Prepare_MCPDefaultMode_UsesInjector(t *testing.T) { + t.Parallel() + + testAgent := agent.Agent{ + Name: "test-agent", Image: "img:latest", Command: []string{"cmd"}, + DefaultCPUs: 2, DefaultMemory: bytesize.ByteSize(2048), + } + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + mcpProvider := &mockMCPProvider{services: []hostservice.Service{{Name: "mcp", Port: 4483}}} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{ + agents: map[string]agent.Agent{"test-agent": testAgent}, + plugins: map[string]agent.Plugin{"test-agent": &mockPlugin{injector: mockMCPInjector{}}}, + }, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{}, + EnvProvider: &mockEnvProvider{}, + Logger: testLogger(), + MCPProvider: mcpProvider, + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + assert.NotNil(t, vmRunner.startCfg.MCPConfigInjector, + "default mode must use the plugin's config-file injector") +} + +func TestSandboxRunner_Prepare_BBOXMCPURLAbsentWhenMCPDisabled(t *testing.T) { + t.Parallel() + + testAgent := agent.Agent{ + Name: "test-agent", Image: "img:latest", Command: []string{"cmd"}, + DefaultCPUs: 2, DefaultMemory: bytesize.ByteSize(2048), + } + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{agents: map[string]agent.Agent{"test-agent": testAgent}}, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{}, + EnvProvider: &mockEnvProvider{}, + Logger: testLogger(), + // No MCPProvider wired => MCP effectively disabled. + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + _, ok := vmRunner.startCfg.EnvVars[agent.EnvBBOXMCPURL] + assert.False(t, ok, "BBOX_MCP_URL must be absent when MCP is disabled") +} + +// TestSandboxRunner_Prepare_BBOXMCPURLAbsentWhenDiscoveryFails asserts that the +// MCP env vars are NOT injected when MCP is enabled but service discovery fails +// (or yields no services) — otherwise a BYO agent would trust a dead endpoint. +func TestSandboxRunner_Prepare_BBOXMCPURLAbsentWhenDiscoveryFails(t *testing.T) { + t.Parallel() + + testAgent := agent.Agent{ + Name: "test-agent", Image: "img:latest", Command: []string{"cmd"}, + DefaultCPUs: 2, DefaultMemory: bytesize.ByteSize(2048), + } + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + // MCP enabled by default (empty MCPConfig), provider returns an error. + mcpProvider := &mockMCPProvider{err: fmt.Errorf("discovery failed")} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{agents: map[string]agent.Agent{"test-agent": testAgent}}, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{}, + EnvProvider: &mockEnvProvider{}, + Logger: testLogger(), + MCPProvider: mcpProvider, + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + require.True(t, mcpProvider.called, "MCP discovery must have been attempted") + _, hasURL := vmRunner.startCfg.EnvVars[agent.EnvBBOXMCPURL] + assert.False(t, hasURL, "BBOX_MCP_URL must be absent when MCP discovery fails") + _, hasAuthz := vmRunner.startCfg.EnvVars[agent.EnvBBOXMCPAuthzProfile] + assert.False(t, hasAuthz, "BBOX_MCP_AUTHZ_PROFILE must be absent when MCP discovery fails") + assert.Empty(t, vmRunner.startCfg.HostServices, "no host services when discovery fails") +} + +func TestSandboxRunner_Prepare_BBOXEnvNotOverridableByHost(t *testing.T) { + t.Parallel() + + // The agent forwards a wildcard that would match BBOX_AGENT_NAME, and the + // host sets BBOX_AGENT_NAME=evil. The universal vars are applied AFTER + // forwarding, so the real agent name must win. + testAgent := agent.Agent{ + Name: "test-agent", + Image: "img:latest", + Command: []string{"cmd"}, + EnvForward: []string{"BBOX_*"}, + DefaultCPUs: 2, + DefaultMemory: bytesize.ByteSize(2048), + } + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{agents: map[string]agent.Agent{"test-agent": testAgent}}, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{}, + EnvProvider: &mockEnvProvider{vars: []string{"BBOX_AGENT_NAME=evil"}}, + Logger: testLogger(), + }) + + sb, err := runner.Prepare(context.Background(), "test-agent", RunOpts{ + Workspace: "/tmp/ws", + EgressProfile: string(egress.ProfilePermissive), + SessionID: "abcd1234", + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + assert.Equal(t, "test-agent", vmRunner.startCfg.EnvVars[agent.EnvBBOXAgentName], + "host-forwarded BBOX_AGENT_NAME must not override the real agent name") +} + +func TestSandboxRunner_Prepare_CustomAgentEgressDefaultsStandard(t *testing.T) { + t.Parallel() + + // A data-only custom agent (nil Plugin) declaring standard egress hosts. + customAgent, err := domainconfig.AgentFromOverride("custom", domainconfig.AgentOverride{ + Image: "ghcr.io/acme/custom:latest", + Command: []string{"run"}, + EgressHosts: map[string][]domainconfig.EgressHostConfig{ + "standard": {{Name: "api.acme.dev", Ports: []uint16{443}}}, + }, + }, domainconfig.DefaultsConfig{}) + require.NoError(t, err) + + mvm := &mockVM{sshPort: 2222, sshKeyPath: "/tmp/key"} + vmRunner := &mockVMRunner{vm: mvm} + + runner := NewSandboxRunner(SandboxDeps{ + Registry: &mockRegistry{agents: map[string]agent.Agent{"custom": customAgent}}, + VMRunner: vmRunner, + SessionRunner: &mockSessionRunner{}, + Config: &SandboxConfig{}, + EnvProvider: &mockEnvProvider{}, + Logger: testLogger(), + }) + + sb, err := runner.Prepare(context.Background(), "custom", RunOpts{ + Workspace: "/tmp/ws", + SessionID: "abcd1234", + // No EgressProfile override => uses the agent's default "standard". + }) + require.NoError(t, err) + defer func() { _ = sb.Cleanup() }() + + require.NotNil(t, vmRunner.startCfg.EgressPolicy, "standard profile must produce a restricted policy") + require.Len(t, vmRunner.startCfg.EgressPolicy.AllowedHosts, 1) + assert.Equal(t, "api.acme.dev", vmRunner.startCfg.EgressPolicy.AllowedHosts[0].Name) +} + func TestSandboxRunner_Prepare_AgentNotFound(t *testing.T) { t.Parallel() @@ -1603,6 +1926,48 @@ func TestResolveMCPConfig(t *testing.T) { wantPort: 5555, wantEnabled: true, }, + { + name: "mode:env enables MCP even when globally disabled", + cfg: &SandboxConfig{ + MCP: domainconfig.MCPConfig{ + Enabled: boolPtr(false), + Group: "global-group", + Port: 5555, + }, + AgentOverrides: map[string]domainconfig.AgentOverride{ + "test": { + MCP: &domainconfig.MCPAgentOverride{ + Mode: domainconfig.MCPModeEnv, + }, + }, + }, + }, + agentName: "test", + wantGroup: "global-group", + wantPort: 5555, + wantEnabled: true, + }, + { + name: "explicit enabled:false wins over mode:env", + cfg: &SandboxConfig{ + MCP: domainconfig.MCPConfig{ + Group: "global-group", + Port: 5555, + }, + AgentOverrides: map[string]domainconfig.AgentOverride{ + "test": { + MCP: &domainconfig.MCPAgentOverride{ + Enabled: boolPtr(false), + Mode: domainconfig.MCPModeEnv, + }, + }, + }, + }, + agentName: "test", + wantGroup: "global-group", + wantPort: 5555, + wantEnabled: false, + }, { name: "agent not in map uses global", cfg: &SandboxConfig{