Code Review: components/ — Security & Correctness Findings (17 issues)

[markturansky]

Overview

This issue captures findings from a code review of the components/ directory across all sub-components: ambient-api-server, ambient-control-plane, ambient-cli, ambient-mcp, ambient-sdk, ambient-ui, credential-sidecars, manifests, and runners.

17 findings total. The two Critical items should be addressed before the next production deployment.

---

🔴 Critical

[S1] Unauthenticated Role Binding Creation

File: components/ambient-api-server/pkg/rbac/scope.go ~lines 111–128

isAuthExempt exempts POST /api/ambient/v1/role_bindings from all authorization checks. Any caller with a valid bearer token (including another user's token) can create role bindings without restriction — including assigning themselves GlobalAdmin membership. The bootstrap problem (first admin needs a path in) should not be solved with a blanket exemption on a privilege-escalation endpoint.

Recommendation: Remove role bindings from isAuthExempt. Solve the bootstrap problem with a separate, strongly-scoped initialization endpoint that is disabled after first use, or a Kubernetes-native admission step at install time.

---

[S2] Unauthenticated Sandbox Policy and Log Endpoints on Token Server

Files: components/ambient-control-plane/internal/tokenserver/sandbox_handler.go, server.go

The token server registers /sandbox/policy and /sandbox/logs with no authentication middleware, over plain HTTP. Any pod in the cluster that can reach the token server can read sandbox policy and execution logs for any sandbox by supplying a name/namespace in the URL — no ownership check is performed.

Recommendation: Require the same token-exchange authentication on these endpoints as on /token. Verify the caller's session ID maps to the requested sandbox before serving data.

---

🟠 High

[S3] Shell Injection via Agent-Controlled Entrypoint

File: components/ambient-control-plane/internal/reconciler/kube_reconciler.go ~line 671

appendPromptToEntrypoint constructs a shell command by string-interpolating the agent's entrypoint directly:

shellCmd := fmt.Sprintf("echo %s | base64 -d | %s --print -",
    encoded, strings.Join(entrypoint, " "))
return []string{"/bin/sh", "-c", shellCmd}

An agent definition whose entrypoint contains shell metacharacters achieves arbitrary command execution inside the runner pod. Combined with [S1], an attacker who grants themselves a role can create an agent definition and exploit this.

Recommendation: Pass the prompt via a file or stdin directly to exec.Command(entrypoint[0], ...) — eliminate the shell entirely.

---

[S4] OpenShell Containers Run as root with Near-Host Capabilities

File: components/ambient-control-plane/internal/reconciler/kube_reconciler.go ~lines 2629–2636

OpenShell sandbox containers are provisioned with runAsUser: 0, allowPrivilegeEscalation: true, and capabilities including SYS_ADMIN, NET_ADMIN, SYS_PTRACE, SETUID, SETGID. SYS_ADMIN alone is nearly equivalent to running as root on the host kernel. A compromised or malicious session can escape the container namespace, attach to other processes, and pivot to other pods.

Recommendation: Define the minimum capabilities needed for the OpenShell use case. Route OpenShell sessions to dedicated, isolated nodes with taints and network policy. Document the threat model explicitly and gate OpenShell provisioning behind an admin-only permission.

---

[S5] X-Forwarded-Access-Token Accepted Without Proxy Verification

File: components/ambient-api-server/pkg/middleware/forwarded_token.go

The middleware promotes X-Forwarded-Access-Token to Authorization: Bearer if no Authorization header is present, with no check that the request arrived from a trusted proxy. Any client that can reach the API server directly can set this header and bypass the intended proxy layer.

Recommendation: Either enforce that this header is only accepted from known proxy IP ranges, or strip it at the ingress layer. Do not promote it unconditionally.

---

[S6] Hardcoded OAuth State Secrets in Public Repo

Files:

• components/manifests/overlays/e2e/secrets.yaml
• components/manifests/overlays/kind/secrets.yaml
• components/manifests/overlays/openshift-local/secrets.yaml

All three overlay files commit GITHUB_STATE_SECRET: "test-state-secret-for-e2e". With a known, static value, any reader of the repository can forge OAuth callbacks against any instance using these overlays — bypassing CSRF protection and potentially stealing OAuth authorization codes.

Recommendation: Remove hardcoded values. Generate secrets randomly at deploy time (Helm randAlphaNum, ExternalSecret, or Vault). Add a CI check that fails if test-state-secret appears in any applicable YAML.

---

🟡 Medium

[S7] Empty JWT Subject Silently Grants Global Admin

File: components/ambient-api-server/pkg/rbac/middleware.go ~lines 68–87

When a token's sub claim is empty, the middleware falls into a legacy path and sets IsGlobalAdmin: true. A token that passes bearer validation but has an omitted or stripped subject is silently elevated to global admin. Additionally, enableAuthz=false in the kind overlay gives all authenticated users global admin, disabling authorization cluster-wide.

Recommendation: Remove the legacy path. Require all tokens to have a non-empty subject. Remove enableAuthz=false from any overlay that could be applied to a real cluster, or add a prominent runtime warning when it is set.

---

[S8] Control Plane ClusterRole Holds escalate and bind — Effectively cluster-admin

File: components/manifests/base/rbac/control-plane-clusterrole.yaml ~line 43

- apiGroups: ["rbac.authorization.k8s.io"]
  resources: ["clusterroles", "clusterrolebindings"]
  verbs: ["create", "get", "update", "patch", "escalate", "bind"]

escalate allows updating a ClusterRole to include permissions the service account does not hold. Combined with bind, the control plane can grant any role to any subject. Any compromise of the control plane process is a full cluster compromise.

Recommendation: Remove escalate and bind. Pre-create required roles at install time and use scoped bindings. Consider OPA/Kyverno policies to enforce that the control plane cannot grant wider permissions than it holds.

---

[S9] Incomplete SQL Injection Mitigation in UI Adapters

Files: All files under components/ambient-ui/src/adapters/

search: `name like '%${sanitizeSearch(params.search)}%'`

sanitizeSearch strips ', ", %, ;, \ but misses --, /*, ), AND, OR, logical operators, and spaces. Depending on backend query construction, boolean-based injection via x) OR 1=1-- style input may be possible.

Recommendation: Move to server-side parameterized queries exclusively (e.g., GORM .Where("name LIKE ?", ...)). Remove sanitizeSearch from the client — it is the wrong layer for this defense.

---

[C1] Secret Redaction Snapshots Environment Once — Misses Rotated Credentials

File: components/runners/ambient-runner/ambient_runner/middleware/secret_redaction.py ~line 173

secret_redaction_middleware collects secret values once at stream start. Credentials refreshed mid-run (GitHub token rotation, CP token re-fetch) are not added to the redaction list, so post-refresh tokens may appear in event payloads in cleartext.

Recommendation: Re-collect secret values on each event, or hook credential refresh to update the redaction list. Add a test verifying redaction continues after token rotation.

---

[A1] isAuthExempt Has No Scoped Authorization and No Automated Coverage

File: components/ambient-api-server/pkg/rbac/scope.go

The exempt list is a flat switch statement with no audit trail, no scope, and no automated test coverage. It conflates bootstrap (first-user onboarding) with ongoing exemptions. See also [S1].

Recommendation: Separate bootstrap from general exemptions. Use a scoped "self-service" permission for project/credential creation by authenticated users who have no bindings yet.

---

[A2] sanitizeSearch Duplicated Across All UI Adapters

Files: All files under components/ambient-ui/src/adapters/

The same function is copy-pasted into every adapter. A fix in one file requires manual replication to all others. The adapters share significant structural code (pagination, field mapping, error handling) that is similarly duplicated.

Recommendation: Extract shared adapter logic into a utility module. Eliminate client-side sanitization and rely on server-side parameterization (see [S9]).

---

🟢 Low

[S10] Runner Pods Mount Service Account Token — Ambient Kubernetes Credential

File: components/ambient-control-plane/internal/reconciler/kube_reconciler.go ~line 2591

Runner pods use automountServiceAccountToken: true. LLM-generated tool calls have ambient access to a Kubernetes credential. Prompt injection via a malicious repo or tool result could exfiltrate it.

Recommendation: Set automountServiceAccountToken: false. Prefer the CP-issued RSA token (AMBIENT_CP_TOKEN_URL flow) for runner-to-API authentication.

---

[S11] /token Endpoint Has No Rate Limiting

File: components/ambient-control-plane/internal/tokenserver/handler.go

No per-source rate limiting on the token endpoint. isValidSessionID only checks length ≥ 8 and no whitespace, leaving a wide brute-force surface.

Recommendation: Add rate limiting per source IP. Strengthen isValidSessionID to require a known format (e.g., base58 with known prefix).

---

[S12] /metrics Endpoint is Unauthenticated

File: components/ambient-api-server/pkg/middleware/bearer_token.go

/metrics is exempt from bearer token auth, potentially exposing session counts, user-identifiable labels, and internal state to any caller.

Recommendation: Protect with network policy and/or a scrape token if the endpoint must be accessible externally.

---

[C2] injectMPPConfig Arg Insertion May Break Subcommand Positioning

File: components/credential-sidecars/entrypoint/main.go

--config <path> is injected after args[0], before any subcommand. If the MPP binary requires --config after the subcommand, the flag is silently ignored.

Recommendation: Document the flag-parsing convention expected of the MPP binary. Add a smoke test verifying the injected config path is actually read.

---

[A3] kube_reconciler.go is 3,500+ Lines

File: components/ambient-control-plane/internal/reconciler/kube_reconciler.go

Provisioning, policy, network configuration, RBAC, secret copying, and per-provider configuration all live in one file. Security-sensitive sections (capability assignment, env var injection) cannot be reviewed or tested in isolation.

Recommendation: Split into focused packages: provisioner/, networkpolicy/, secretcopier/, providerconfig/. The reconciler becomes a thin orchestrator.

---

Notable Positives

• AES-256-GCM with credential ID as AAD (ambient-api-server/pkg/crypto/encrypt.go) — ciphertext is bound to its slot, preventing swap attacks. Versioned keyring enables key rotation without immediate re-encryption. Deliberate threat modeling evident.
• subtle.ConstantTimeCompare for bearer token validation — timing-safe, correctly applied.
• immutableSandboxEnvKeys prevents agent-authored env vars from overwriting platform-injected values (SESSION_ID, BOT_TOKEN, etc.). Simple and auditable.
• Multi-mutex deduplication of concurrent lifecycle operations per session prevents double-provisioning race conditions.
• Cluster-internal URL validation before credential fetch in both the Go sidecar and Python runner — prevents SSRF against attacker-controlled endpoints.

---

Summary

Severity	Count
Critical	2
High	4
Medium	7
Low	4
Total	17

Priority order for remediation: S1 → S2 → S3 → S4 → S5 → S6 → S7 → S8.

Reviewed by amber (Shell agent) via automated code review on 2026-07-26.

[markturansky]

Status Update — 2026-07-27

Resolved ✅

PR #432 (merged 2026-07-26): fix(security): close 3 RBAC and auth vulnerabilities

Three findings fully addressed with test coverage:

ID	Finding	Resolution
S1 🔴	Unauthenticated role binding creation	Removed POST /role_bindings from isAuthExempt(). Replaced with a bootstrap-admin Kubernetes Job using the existing seed-admin CLI command for first-install admin provisioning. Test: TestIsAuthExempt updated.
S2 🔴	Unauthenticated sandbox policy/log endpoints	Added bearer token auth to /sandbox/{name}/policy and /sandbox/{name}/logs on the token server. Tests: TestSandboxPolicy_RejectsUnauthenticated, TestSandboxLogs_RejectsUnauthenticated.
S7 🟡	Empty JWT sub silently grants GlobalAdmin	Removed the legacy service-token fallback in both HTTP (AuthorizeApi) and gRPC (populateGRPCContext) middleware. Empty username now returns 401/Unavailable. Test: TestAuthorizeApi_RejectsEmptyUsernameServiceCaller.

PR #433 (open, pending CI): fix(security): relax bootstrap-admin Job securityContext for Kind
Follow-up to #432 — removes runAsNonRoot: true from the bootstrap-admin Job pod spec (UBI9 minimal image runs as root by default) to fix the Kind CI simulation failure introduced by #432. This PR is set to close #431.

---

Still Open

The following findings from the original review remain unaddressed:

ID	Severity	Finding
S3 🟠	High	Shell injection via agent-controlled entrypoint in appendPromptToEntrypoint
S4 🟠	High	OpenShell containers run as root with SYS_ADMIN, NET_ADMIN, SYS_PTRACE
S5 🟠	High	X-Forwarded-Access-Token accepted without trusted-proxy verification
S6 🟠	High	Hardcoded OAuth state secret in three public overlay YAML files
S8 🟡	Medium	Control plane ClusterRole holds escalate + bind (effectively cluster-admin)
S9 🟡	Medium	Incomplete SQL injection mitigation in UI adapters (sanitizeSearch)
S10 🟢	Low	Runner pods mount service account token by default
S11 🟢	Low	/token endpoint has no rate limiting
S12 🟢	Low	/metrics endpoint is unauthenticated
C1 🟡	Medium	Secret redaction snapshots environment once — misses rotated credentials
C2 🟢	Low	injectMPPConfig arg insertion may break subcommand positioning
A1 🟡	Medium	isAuthExempt has no scoped authorization (partially addressed by S1 fix)
A2 🟡	Medium	sanitizeSearch duplicated across all UI adapters
A3 🟢	Low	kube_reconciler.go is 3,500+ lines

S3 (shell injection) and S6 (hardcoded OAuth secrets) are the highest priority remaining items.

Reviewed by amber (Shell agent)