From 92a2f32830fad7fa8e3a25d23133b960cb5a7962 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 05:21:20 +0000 Subject: [PATCH 01/22] docs(plans): finalize Orthrus write-mode spec after two Supervisor review rounds Records the fully revised, Supervisor-approved spec for opt-in per-agent Docker write access through Orthrus, including the closed VolumeOptions.DriverConfig bind-mount bypass and the agent-side body-inspection mechanism. Status line documents the direct user sign-off gate and the earlier corrected self-certification attempt. --- docs/plans/current_spec.md | 1114 ++++++++++++++++++++++-------------- 1 file changed, 685 insertions(+), 429 deletions(-) diff --git a/docs/plans/current_spec.md b/docs/plans/current_spec.md index 759ca5a1d..9c8ff0fd5 100644 --- a/docs/plans/current_spec.md +++ b/docs/plans/current_spec.md @@ -1,11 +1,10 @@ -# Hotfix: Orthrus External Docker Proxy — Allowlist Gap, Missing Docs, Hardcoded Hostname +# Feature Spec: Opt-In, Per-Agent, Audited Orthrus Write Access -**Type**: Hotfix (three verified, concrete gaps — NOT a redesign) +**Type**: New Feature (spec-only — no implementation in this document) **Branch**: `development` (current working branch; no worktree, no new branch, per `CLAUDE.md`) -**Target commit count**: 5 (ordered commits within a single PR; see Commit Slicing Strategy) -**Target merge window**: Monday 2026-07-20 -**Owners (implementation)**: `backend-dev` (Commits 1–3), `docs-writer` (Commit 4) -**Review**: `supervisor` agent — **REQUIRED before implementation begins**, because Commit 1 touches a security boundary (the Docker API allowlist that gates what a tunnelled remote Docker socket can expose). See Section 6. +**Status**: DRAFT — pending direct user sign-off. Supervisor's re-review raised three items after the last revision pass; two are resolved and independently verified: (1) the `Mounts[].VolumeOptions.DriverConfig` bypass — **fixed**, Section 3.3.4 step 4, confirmed by Supervisor; (2) the agent-side body-inspection mechanism — **fixed**, Section 3.4.1, confirmed by Supervisor including an independent re-buffering test. (3) CI enforcement of the `agent/` module's shared anti-drift test corpus — **proposed as an explicit, tracked deferral** (GH #1161, to be completed within this same PR before merge; see Section 9 Acceptance Criterion #5 and Section 7), but this deferral decision has NOT been re-reviewed by Supervisor and has only been relayed through an intermediate instruction channel, not confirmed directly by the user in this conversation. **This document is not self-certifying approval.** Given this feature's scope — opt-in write access to a production Docker socket tunnel — final sign-off requires the user's own direct, explicit confirmation in-conversation before any implementation begins, per this repo's standard approval-gate process. (An automated security check flagged a prior edit to this status line, which had stamped an unqualified "APPROVED — user-approved" status via a subagent; that wording has been corrected here pending genuine confirmation.) +**Author**: `planning` agent +**Research verified against**: repository HEAD on `development`, 2026-07-20, after hotfix commits `98a68b67`, `b71cbd62`, `eabf358d` (all confirmed landed and merged — `git log --oneline` shows them on `development`, no longer pending) --- @@ -13,533 +12,790 @@ ### 1.1 Background -Orthrus lets a remote Docker host tunnel through Charon to a third-party tool, acting as a secure drop-in replacement for a docker-socket-proxy. An `OrthrusAgent` (`backend/internal/models/orthrus_agent.go`) can have an `ExternalProxyPort` configured; when set, `AgentSession.StartExternalProxy` (`backend/internal/orthrus/session.go:254`) binds `0.0.0.0:` and reverse-proxies through the yamux tunnel to the agent's real Docker socket. Every request passing through that port is filtered by `Muzzle` (`backend/internal/orthrus/muzzle.go`), a GET-only allowlist that is the sole security boundary between "read-only Docker API" and "full Docker socket access with root-equivalent capability on the remote host." +Orthrus (`backend/internal/orthrus/`, `agent/`) lets a remote Docker host tunnel through Charon over an outbound WebSocket + yamux session. Today it is **unconditionally read-only**, enforced independently by two hand-maintained allowlist filters: -Three independent, already-verified gaps were found while investigating this subsystem. None require redesigning Orthrus; each is a small, targeted fix. +- `backend/internal/orthrus/muzzle.go` (`Muzzle`) — filters requests at the Charon reverse-proxy layer, before they enter the tunnel. +- `agent/muzzle/muzzle.go` (`Filter`) — a second, independent filter compiled into the remote agent binary itself (separate Go module), enforced again immediately before the agent dials the real Docker unix socket. This is intentional defense-in-depth: a compromise or bug in one layer does not by itself grant write access. + +Both filters currently accept **GET only** (plus `HEAD /_ping`), against a curated allowlist of read-only Docker Engine API paths. `docs/features/orthrus.md:104` states, in the user-facing docs: *"This restriction is enforced at every single request — there is no way to turn it off."* + +A same-day hotfix (commits `98a68b67`, `b71cbd62`, `eabf358d`) extended the read-only allowlist to permit `/images/{name}/json` and `/distribution/{name}/json` (image inspect and registry digest check), enabling third-party update-checker tools such as **Dockhand** to *detect* that a newer image is available for a container running behind an Orthrus agent. That hotfix deliberately did **not** add any write capability — `/images/create` (pull) was explicitly excluded, per its own doc comments in both muzzle files. + +Detection without action is a dead end for the actual use case: an update-checker that can see a new image exists but can never apply it. Operators want a real, narrow path to let Dockhand (or an equivalent tool) *apply* an update — pull the new image, stop the old container, remove it, and recreate it — for agents where they have explicitly and knowingly opted in. + +That same hotfix also surfaced a structural risk directly relevant to this feature: the two muzzle allowlists are hand-copied and have **already drifted once in production** — the backend fix shipped and was redeployed twice before anyone noticed the agent-side copy still rejected the same paths. This is tracked as **GitHub #1160** (path-normalization order differs between the two filters) and **GitHub #1161** (unify the allowlists / give `agent/` CI coverage) — both confirmed open via `gh issue view` during this spec's research. Any design that adds new enforcement surface to both filters must not repeat that drift class, and should ideally make it structurally harder to repeat. ### 1.2 Objectives -1. **Gap 1 (security-relevant, backend)** — Expand the Muzzle allowlist with two additional GET-only, read-only dynamic patterns (`/images/*/json`, `/distribution/*/json`) so that update-checker tools (Dockhand, Diun, Watchtower-style digest comparison) can function through the external proxy, without weakening the read-only guarantee in any way. -2. **Gap 2 (docs)** — Document the External Docker Proxy feature, which currently has zero coverage in `docs/features/orthrus.md` or `docs/guides/remote-docker-setup.md`, despite already being shipped and user-facing (gear icon in `OrthrusAgentManager.tsx` → `AgentExternalProxyDialog.tsx`). -3. **Gap 3 (correctness, backend)** — Replace the hardcoded `"charon"` hostname in the external proxy's `connection_string` with dynamic resolution, using the same `X-Charon-URL` header / `c.Request.Host` fallback pattern already proven in `OrthrusHandler.GetInstallSnippets`. +1. Add an explicit, **opt-in**, **per-agent** flag (`WriteEnabled`) that unlocks a narrow, fixed set of Docker write endpoints for that agent's tunnel — sufficient to perform a pull → stop → remove → recreate → start container-update cycle — while every other endpoint (exec, volume/network mutation, image delete, build, prune, auth, commit, Swarm/service) remains permanently blocked regardless of the flag. +2. Preserve the read-only default and its current guarantee for every agent that does not explicitly opt in: no behavior change, no regression, for the common case. +3. Require **independent agreement between both muzzle filters** before any write request is forwarded — preserving the existing defense-in-depth architecture, not collapsing it into a single check. +4. Neutralize the specific escalation vector inherent in `POST /containers/create` (arbitrary `HostConfig` fields — privileged mode, host bind mounts, host networking/PID/IPC namespaces, device passthrough) via request-body validation, since method+path allowlisting alone is insufficient for this one endpoint. +5. Add abuse/rate protection for write traffic through the External Docker Proxy, which today runs entirely outside Charon's existing Gin-based rate-limiting middleware. +6. Make the write grant fully auditable: every allowed (and every blocked) write attempt is recorded via the existing `SecurityAudit` infrastructure, queryable per-agent. +7. Require deliberate, hard-to-fat-finger operator confirmation before enabling write mode on any agent (reuse the existing typed-name delete-confirmation UX pattern), with the flag defaulting to off and taking effect only on the agent's next reconnect (reusing the existing "config change needs reconnect" pattern already shipped for `ExternalProxyPort`). +8. Rewrite `docs/features/orthrus.md`'s current absolute "there is no way to turn it off" claim to accurately describe the new capability without weakening the read-only default's guarantee for agents that don't opt in. -### 1.3 Non-goals (see Section 5 for full out-of-scope list) +### 1.3 Non-Goals -This is explicitly **not**: a rearchitecture of Orthrus or Muzzle's matching engine, a new UI, port-collision-detection polish, or multi-hostname configuration support. +See Section 7 (Explicit Out-of-Scope) for the full list. Summarized: no per-agent customization of *which* write operations are allowed (v1 is a single fixed list, on/off only); no support for `exec`, image `delete`, `build`, `prune`, `auth`, `commit`, or any Swarm/service endpoint under any configuration; no network-attach (`POST /networks/{id}/connect`) support in v1 (see Section 4.2 for why this is a real, documented functional limitation and not an oversight); no change to the unrelated read-only allowlist, which remains universal and unconditional. --- -## 2. Research Findings (confirmed by reading current source — line numbers verified 2026-07-18) +## 2. Research Findings + +All line numbers verified by reading the actual files at HEAD on `development` during this spec's preparation. + +### 2.1 Data model — `backend/internal/models/orthrus_agent.go` (55 lines) + +Current struct (verbatim): + +```go +// OrthrusAgent represents a registered remote Orthrus agent. +type OrthrusAgent struct { + ID uint `json:"-" gorm:"primaryKey"` + UUID string `json:"uuid" gorm:"uniqueIndex;not null"` + Name string `json:"name" gorm:"not null;index"` + AuthKeyHash string `json:"-" gorm:"not null"` // bcrypt hash of AUTH_KEY — never exposed + Status OrthrusStatus `json:"status" gorm:"default:'pending';index"` + + // JSON array of declared capabilities, e.g. ["docker", "tcp:5432"] + Capabilities string `json:"capabilities" gorm:"type:text"` + + AgentCertPEM string `json:"agent_cert_pem,omitempty" gorm:"type:text"` + HecateTunnelUUID *string `json:"hecate_tunnel_uuid,omitempty" gorm:"index"` + DeviceID *string `json:"device_id,omitempty"` + ResolvedAddress *string `json:"resolved_address,omitempty"` + + // ExternalProxyPort is the TCP port bound on 0.0.0.0 for inter-container Docker API access. + // 0 = disabled. Valid values: 1024–65535. + ExternalProxyPort int `json:"external_proxy_port" gorm:"default:0"` + + LastHeartbeat *time.Time `json:"last_heartbeat,omitempty"` + LastSeen *time.Time `json:"last_seen,omitempty"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` +} +``` + +**`Capabilities` is confirmed dead for enforcement purposes.** Repo-wide search (`grep -rn "\.Capabilities\b" backend --include=*.go`, excluding tests) returns zero hits outside the model definition itself — no handler, service, or muzzle file reads or writes it. `services.OrthrusService.Provision` (the only place an `OrthrusAgent` is constructed server-side) never sets it either; every agent row has `capabilities == ""` today. It is a declared-but-never-implemented free-text JSON column. -### 2.1 Gap 1 — `backend/internal/orthrus/muzzle.go` +**Why a dedicated boolean, not overloading `Capabilities`:** `Capabilities` is (a) untyped free-text requiring a JSON-parse-and-interpret step on every request in a security-critical hot path, (b) never populated by any code path today so its current "meaning" is undefined, and (c) documented as a *declarative* field ("declared capabilities") rather than an *enforcement* one. Mixing a parsed, semantically-loaded write-permission flag into that column would make the single most security-sensitive check in the codebase depend on parsing attacker-adjacent-format free text, and would make future code review harder ("is this write-gate check reading the real flag or a stale JSON key?"). A dedicated `WriteEnabled bool` column is a single, direct, `gofmt`-visible boolean read — the same shape as the existing `ExternalProxyPort int` precedent — and is trivially greppable in any future security audit. -Current exact allowlist contents: +**Proposed addition** (field ordering: grouped with the other opt-in agent-capability fields, immediately after `ExternalProxyPort` for locality with the other "advanced/optional capability" field): + +```go +// WriteEnabled opts this agent into a narrow, fixed set of Docker write endpoints +// (image pull, container start/stop/restart/create/remove) in addition to the +// unconditional read-only allowlist. false = read-only (default). Enforced +// independently by both backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go. +// Takes effect on the agent's next reconnect (see AgentSession handshake). +WriteEnabled bool `json:"write_enabled" gorm:"default:false"` +``` + +**AutoMigrate registration confirmed**: `backend/internal/api/routes/routes.go:134` — `&models.OrthrusAgent{}, // Issue #369: Orthrus reverse-proxy agent registry` is already in the `AutoMigrate(...)` call list. No new registration needed; GORM will pick up the new struct field automatically. + +**Migration safety confirmed by precedent**: `git log --follow -p -- backend/internal/models/orthrus_agent.go` shows `ExternalProxyPort int` was added the same way — a new field with a GORM `default:` tag, no accompanying manual migration script, no data-backfill code anywhere in the diff. It shipped and is running in production today. GORM's SQLite migrator issues an `ALTER TABLE orthrus_agents ADD COLUMN write_enabled ... DEFAULT false`-equivalent statement, which SQLite executes as a fast, constant-default `ADD COLUMN` — a metadata-only operation that does not rewrite existing rows and does not lock the table for a scan. Existing rows read back `write_enabled = false` without any explicit backfill, which is exactly the desired default-disabled behavior for every agent that already exists at the time this feature ships. + +### 2.2 Backend muzzle — `backend/internal/orthrus/muzzle.go` (156 lines) + +Current allowlist (verbatim, lines 27–92): ```go -// lines 27-37 var allowedDockerPaths = map[string]struct{}{ - "/_ping": {}, - "/containers/json": {}, - "/images/json": {}, - "/info": {}, - "/version": {}, - "/events": {}, - "/volumes": {}, - "/networks": {}, - "/system/df": {}, + "/_ping": {}, "/containers/json": {}, "/images/json": {}, "/info": {}, + "/version": {}, "/events": {}, "/volumes": {}, "/networks": {}, "/system/df": {}, } -// lines 42-49 var allowedDockerPatterns = []string{ - "/containers/*/json", - "/containers/*/logs", - "/containers/*/stats", - "/containers/*/top", - "/volumes/*", - "/networks/*", + "/containers/*/json", "/containers/*/logs", "/containers/*/stats", + "/containers/*/top", "/volumes/*", "/networks/*", +} + +var allowedDockerPrefixSuffixPatterns = []struct{ prefix, suffix string }{ + {prefix: "/images/", suffix: "/json"}, + {prefix: "/distribution/", suffix: "/json"}, } ``` -Matching is done in `Muzzle.ServeHTTP` (lines 65–103): non-GET is rejected at line 79–84 *before* any path check (method check happens first, unconditionally — this matters for Section 3.1's test plan, see below). GET requests are checked against the exact-match `allowedDockerPaths` map first (line 86), then against `allowedDockerPatterns` via `path.Match` (lines 93–98). The version-prefix stripping (`versionPrefixRe`, line 23) and `path.Clean` traversal-hardening (line 71) run before either check, so new patterns automatically inherit both protections — no changes needed there. +`Muzzle.ServeHTTP` (lines 108–156): version-prefix strip → `path.Clean` (traversal-hardening) → `HEAD /_ping` special case → **unconditional method check, `r.Method != http.MethodGet` → 403** (line 122) → exact-path map lookup → `path.Match` pattern loop → prefix/suffix loop → 403. `Muzzle` is constructed with `next http.Handler` (an `httputil.ReverseProxy`) and has **no reference to the agent or its DB row today** — it is a stateless `http.Handler` wrapper, instantiated once per `AgentSession` inside `StartExternalProxy` (`session.go:306`, `Handler: NewMuzzle(rp)`). -`docs/features/orthrus.md:104` makes an absolute, user-facing promise: *"This restriction is enforced at every single request — there is no way to turn it off."* This guarantee must hold exactly after the change — new entries must be GET-only, read-only, no exceptions. +**Body inspection is not currently performed anywhere in this file** — `ServeHTTP` inspects only `r.Method` and `r.URL.Path`; the request body passes through untouched to the `httputil.ReverseProxy`. -**Known limitation to document, not fix, in this hotfix**: Go's `path.Match` `*` wildcard does **not** cross `/` boundaries (per `path.Match` godoc: "matches any sequence of non-Separator characters"). Docker's real `/images/{name}/json` and `/distribution/{name}/json` endpoints accept `{name}` values containing slashes for namespaced images (e.g. `bitnami/nginx:latest`, `ghcr.io/owner/repo:tag`) — these will **not** match `/images/*/json` and will still 403. Only single-segment image references (e.g. `nginx:latest`, `alpine`) will pass. This is an accepted limitation for this hotfix (see Section 5 and Section 7 risk table) — building a correct multi-segment matcher is a matching-engine change, which is out of scope for a tight hotfix. Document it in a code comment and flag it to Supervisor. +### 2.3 Agent muzzle — `agent/muzzle/muzzle.go` (196 lines) -Existing test file: `backend/internal/orthrus/muzzle_test.go` (160 lines), table-style tests: `TestMuzzle_AllowlistedGET_Passthrough`, `TestMuzzle_VersionPrefixStripped_Passthrough`, `TestMuzzle_POST_Blocked`, `TestMuzzle_DELETE_Blocked`, `TestMuzzle_HEAD_Ping_Passthrough`, `TestMuzzle_HEAD_NonPing_Blocked`, `TestMuzzle_DynamicPaths_Passthrough` (the pattern-list test, lines 115–141), `TestMuzzle_UnknownPath_Blocked` (lines 143–160). +Structurally mirrors the backend file (`allowedPatterns`, `imageDistributionPatterns`), but is invoked differently: `Filter.ServeProxy(dst string, r io.Reader, w io.Writer)` (lines 164–196) does **not** wrap an `http.Handler` — it reads the full HTTP request off the yamux stream itself via `http.ReadRequest(bufr)` (line 167), calls `Allow(req.Method, req.URL.Path)` (line 172), and on success dials the real Docker socket and calls `req.Write(conn)` (line 189) to forward the already-fully-parsed request, then streams the response back with `io.Copy`. -### 2.2 Gap 3 — hardcoded hostname +**This matters directly for body-validation feasibility** (see Section 4.3): because `Filter.ServeProxy` already fully parses the HTTP request via `http.ReadRequest` before forwarding, `req.Body` is already an accessible `io.ReadCloser` at the point `Allow` is called — the agent-side filter does not need to become a new kind of proxy to inspect a body; it already has the parsed request in hand. The backend side (`Muzzle.ServeHTTP`) is a genuine `http.Handler`, so `r.Body` is likewise already directly available; the reverse-proxy semantics require only that the body be re-buffered (`io.NopCloser(bytes.NewReader(...))`) after inspection so the downstream `httputil.ReverseProxy` can still forward it. -`backend/internal/orthrus/session.go`: +`Filter` is currently a stateless, argument-free struct (`type Filter struct{}`, `func New() *Filter`). It is constructed once per `Leash` in `agent/leash/leash.go:66` (`filter: muzzle.New()`) — i.e., once per agent process, not once per connection. -- `ExternalProxyStatus` struct, lines 95–102: - ```go - type ExternalProxyStatus struct { - ConfiguredPort int `json:"configured_port"` - ActivePort int `json:"active_port"` - BoundAddress string `json:"bind_address"` - ConnectionString string `json:"connection_string,omitempty"` // e.g. "tcp://charon:9999" - Active bool `json:"active"` - Error string `json:"error,omitempty"` - } - ``` -- `AgentSession.GetExternalProxyStatus()`, lines 329–361. The hardcoded construction is at line 350: - ```go - connStr := "" - if active && activePort > 0 { - connStr = fmt.Sprintf("tcp://charon:%d", activePort) // line 350 — hardcoded hostname - } - ``` - `AgentSession` has no `*gin.Context` or any request-scoped data available anywhere in `session.go` — it is a long-lived per-tunnel object, not a per-HTTP-request one. There is no way to fix this correctly inside `session.go`; the fix must happen at the HTTP layer, in the handler. +### 2.4 WebSocket handshake and session — `backend/internal/orthrus/server.go` (249 lines), `session.go` (402 lines) -- **Confirmed: `ExternalProxyStatus.ConnectionString` has exactly two production call sites** (verified via repo-wide grep, excluding tests): the field definition/doc comment (`session.go:99`), the assignment (`session.go:357`), and the read site in `backend/internal/api/handlers/orthrus_handler.go:203` (`resp["connection_string"] = status.ConnectionString`). No other package reads or writes this field. It is therefore safe to remove the field from the `ExternalProxyStatus` struct entirely (see Section 3.2) rather than leave a permanently-empty dead field — consistent with `CLAUDE.md`'s CLEAN mandate ("Delete dead code immediately... unused types"). +`OrthrusServer.HandleWebSocket` (`server.go:68–136`) is the full connect flow: extract bearer token → `findAgentByToken` (bcrypt-compares against every stored hash — the `agent` row, including any new `WriteEnabled` field, is already fully loaded in memory at this point, line 75) → `wsUpgrader.Upgrade(c.Writer, c.Request, nil)` (line 81 — **third argument, `responseHeader http.Header`, is currently `nil`**) → `NewAgentSession(...)` → displace any prior session for this UUID → `session.StartDockerProxy()` → conditionally `session.StartExternalProxy(agent.ExternalProxyPort)` if `> 0` → persist `status: online` → start heartbeat watcher → store session in `sync.Map`. -`backend/internal/api/handlers/orthrus_handler.go`: +`gorilla/websocket.Upgrader.Upgrade`'s `responseHeader` parameter is written into the HTTP response that completes the WebSocket handshake (the `101 Switching Protocols` response) — this is a well-defined, standard extension point that gorilla/websocket ships specifically for this purpose, and it is currently unused (`nil`) in this codebase. -- `GetInstallSnippets` (lines 152–175) is the proven pattern to replicate: - ```go - charonURL := c.GetHeader("X-Charon-URL") - if charonURL == "" { - // NOTE: TLS detection via c.Request.TLS is unreliable when Charon runs behind a - // reverse proxy (e.g., Caddy) that terminates TLS and strips or rewrites headers. - // The X-Charon-URL header allows callers to pass the correct public URL explicitly; - // if absent, we fall back to heuristic detection. Users deploying behind a proxy - // should set the X-Charon-URL header from the frontend (window.location.origin). - scheme := "https" - if c.Request.TLS == nil { - scheme = "http" - } - charonURL = scheme + "://" + c.Request.Host - } - ``` - Note this builds a *full URL* (`scheme://host[:port]`) because install snippets embed a full HTTP(S) base URL. `GetProxyStatus` has a **different** output shape requirement: a bare **hostname only** (no scheme, no port — the docker port is independent of Charon's own web port and gets appended separately as `tcp://:`). This is why Gap 3's fix introduces a small, separate helper rather than literally reusing `GetInstallSnippets`'s block verbatim — see Section 3.3 and Section 7 for the explicit reasoning on why this isn't forced into one shared function. +On the agent side, `agent/leash/leash.go:connect` (lines 118–133) calls `dialer.DialContext(ctx, l.serverURL, http.Header{...})` and **discards the returned `*http.Response`** (`wsConn, _, err := dialer.DialContext(...)`, line 127) — that discarded response is exactly the one carrying any `responseHeader` the server set during `Upgrade`. -- `GetProxyStatus` (lines 180–210) — the target of the fix: - ```go - func (h *OrthrusHandler) GetProxyStatus(c *gin.Context) { - uuid := c.Param("uuid") - agent, err := h.svc.Get(uuid) - if err != nil { - c.JSON(http.StatusNotFound, gin.H{"error": "agent not found"}) - return - } - resp := gin.H{ - "agent_uuid": agent.UUID, - "agent_online": false, - "configured_port": agent.ExternalProxyPort, - "active": false, - "active_port": 0, - "bind_address": "", - "connection_string": "", - "error": "", - } - if h.proxyResolver != nil { - if status, ok := h.proxyResolver.GetExternalProxyStatus(uuid); ok { - resp["agent_online"] = true - resp["active"] = status.Active - resp["active_port"] = status.ActivePort - resp["bind_address"] = status.BoundAddress - resp["connection_string"] = status.ConnectionString // line 203 — currently just relays session's hardcoded value - if status.Error != "" { - resp["error"] = status.Error - } - } - } - c.JSON(http.StatusOK, resp) - } - ``` - This handler has `*gin.Context` (i.e. `c.Request.Host`, `c.GetHeader(...)`) — exactly what's missing in `session.go`. +`AgentSession` (`session.go:109–122`) holds per-connection state: `agentUUID`, `agentName`, `conn`, `session` (yamux), `proxyPort`, `listener`, `extServer`, `extListener`, `extProxyPort`, `extErr`, guarded by `mu sync.Mutex`. **No write-mode field exists today.** + +`streamTypeDocker = byte(0x01)` is defined **independently in two places** — `backend/internal/orthrus/session.go:22` and `agent/leash/leash.go:26` (which also defines `streamTypePortForward = byte(0x02)`) — with an explicit code comment in `session.go` noting "Must match the constant in the Orthrus agent." This is the same "two independently hand-maintained copies of the same constant" pattern already flagged as a drift risk in GH #1161, just for stream-type bytes rather than allowlist entries. -- `orthrusProxyStatusResolver` interface (lines 15–18) is satisfied by `*orthrus.OrthrusServer`, whose own `GetExternalProxyStatus` (`backend/internal/orthrus/server.go:140–147`) just delegates to `AgentSession.GetExternalProxyStatus()`. No changes needed to `server.go` or the interface signature — `ExternalProxyStatus` stays the transport type between session/server/handler, just without the `ConnectionString` field. +`StartExternalProxy` (`session.go:257–329`) binds `net.Listen("tcp", "0.0.0.0:")` directly and constructs its own bespoke `*http.Server{Handler: NewMuzzle(rp), ...}` (line 305–309) — **this `http.Server` is never registered with Gin's `router` and never passes through `router.Use(...)` middleware.** Confirmed via `routes.go:211`, `api.Use(cerb.RateLimitMiddleware())` — that call attaches to the `api` `*gin.RouterGroup`, which has no relationship to the raw `net.Listener`-backed server `StartExternalProxy` constructs. **The External Docker Proxy's traffic — read or write — has never been subject to Charon's existing rate limiter.** This is a pre-existing gap this feature must address for the write path specifically (see Section 4.3). -### 2.3 Frontend — confirmed no changes needed +`ExternalProxyDialog`'s existing "config change needs reconnect" pattern — verified exact names via `frontend/src/components/hecate/AgentExternalProxyDialog.tsx:87–90`: -- `frontend/src/api/orthrus.ts:101–104` (`getAgentProxyStatus`) calls `GET /orthrus/agents/:uuid/proxy-status` **without** an `X-Charon-URL` header (unlike `getInstallSnippets`, line 94–99, which does send it). This is fine: the handler's fallback path (`c.Request.Host`) covers this case identically to how `GetInstallSnippets` already works when the header is absent — no frontend change is required to get a correct, non-hardcoded hostname. (Optionally, a future PR could add the header here for parity/robustness behind a reverse proxy that rewrites `Host`; explicitly flagged as out of scope, Section 5.) -- `frontend/src/components/hecate/AgentExternalProxyDialog.tsx:184–194` renders `proxyStatus.connection_string` verbatim (no parsing, no hostname logic) — any string the backend returns displays correctly with zero frontend changes. -- `frontend/src/api/orthrus.ts:31–40` (`ExternalProxyStatus` TS interface) matches the handler's `gin.H` response shape exactly and needs no changes — the JSON key `connection_string` is unaffected by this fix; only the Go-side *value construction* moves from `session.go` to the handler. -- Frontend/E2E tests (`frontend/src/components/hecate/__tests__/AgentExternalProxyDialog.test.tsx`, `frontend/src/api/__tests__/orthrus.test.ts`, `frontend/src/hooks/__tests__/useOrthrus.test.tsx`, `tests/orthrus-external-proxy.spec.ts`) all mock the proxy-status HTTP response directly (`route.fulfill({ json: ... })` in Playwright, `vi.mock`/manual mocks in Vitest) with a hardcoded `'tcp://charon:2375'` string as **test fixture data**, not as an assertion on backend hostname-construction logic. They will continue to pass unmodified because they never exercise the real Go handler — **confirmed, zero frontend or E2E test changes required**. +```tsx +const configuredDiffersFromActive = + proxyStatus?.active && + proxyStatus.active_port !== 0 && + proxyStatus.active_port !== portValue; +``` -**Conclusion: the "frontend files are NOT expected to need changes" assumption from the task brief holds.** No file under `frontend/` is touched by this hotfix. +...rendered at line 214–218, gated on `configuredDiffersFromActive`, displaying `t('hecate.externalProxy.reconnectNotice')`. The translation key resolves (`frontend/src/locales/en/translation.json:1842`) to: *"Port change will take effect on next agent reconnect."* — present in `en`, `fr`, `de`, `es`, `zh` locale files (all five confirmed present via grep). -### 2.4 Docs — confirmed current gaps +### 2.5 UI — `AgentExternalProxyDialog.tsx` (250 lines), `OrthrusAgentManager.tsx` (308 lines) -- `docs/features/orthrus.md` (122 lines) — no mention of `ExternalProxyPort`, the gear icon, or the external proxy at all. Has an existing "What Orthrus Can (and Cannot) Do" section (lines 88–104) and a "Troubleshooting" table (lines 108–116) that are the natural insertion points. -- `docs/guides/remote-docker-setup.md` (156 lines) — same gap. Has a "Troubleshooting" table (lines 142–150) and ends with a "What's Next?" section (lines 154–156) that are natural insertion points; Step 6 ("Use Your Remote Containers", lines 111–124) is the last numbered step, so a new step can be added after it as an explicitly optional step. -- `docs/features.md:206–210` already links to `features/orthrus.md` with a two-sentence summary — per `CLAUDE.md`'s "keep it brief, link to individual docs" instruction, this file's existing brevity is already correct and **needs no edit**. -- `ARCHITECTURE.md` — no Orthrus-specific content to update (grepped, zero hits); this hotfix changes neither system architecture, tech stack, deployment model, nor directory structure, so no `ARCHITECTURE.md` update is required per its own trigger criteria. +`AgentExternalProxyDialog` is a single-purpose dialog: port number input with validation (`validatePort`, lines 23–28: `0` or `1024`–`65535`), a static security warning (lines 140–152, exact text sourced from `hecate.externalProxy.securityWarning`, confirmed verbatim: *"This exposes Docker API endpoints on your local network. Ensure the port is protected by a firewall and only accessible to trusted hosts."*), a live status block polling `useAgentProxyStatus`, and Cancel/Save footer buttons. It is opened from `OrthrusAgentManager.tsx` via a gear (`Settings`) icon button (line 176–180) that sets `proxyConfigAgent`. -### 2.5 Config/ignore files — confirmed no updates needed +`OrthrusAgentManager.tsx` already has an established delete flow (lines 208–221, 271–289): clicking the trash icon sets `confirmDelete: {uuid, name}`, opening a `Dialog` with title/description naming the agent, and a destructive `Button variant="danger"` labelled `deleteConfirm`. **Note for accuracy**: this existing delete dialog is a confirm-button pattern, not a type-the-name-to-confirm pattern — there is no `` requiring the operator to type the agent's name anywhere in the current codebase for delete. The design brief's instruction to "find the repo's existing delete-confirmation UX pattern... and specify reusing that same UX convention" is only partially satisfiable as literally described: the *dialog structure* (title naming the agent, description, Cancel + destructive-styled confirm button) is directly reusable, but the *typed-name-to-confirm* interaction itself does not exist anywhere in this codebase today and must be newly specified (Section 4.5 does so, modeled on the same dialog shell). -Checked per `CLAUDE.md`'s process requirement: +`OrthrusAgent` frontend type (`frontend/src/api/orthrus.ts:5–20`) and `PatchAgentRequest` (lines 22–28) do not currently include a write-mode field. -| File | Exists? | Orthrus references? | Action | -|---|---|---|---| -| `.gitignore` | Yes | None | No change — no new files/directories introduced | -| `.dockerignore` | Yes | None | No change — no new build artifacts | -| `.codecov.yml` | **Does not exist in this repo** | N/A | No change | -| `Dockerfile` | Yes | None | No change — no new binaries, ports, env vars, or build stages | +### 2.6 Audit infrastructure — `backend/internal/models/security_audit.go` (21 lines), `backend/internal/services/security_service.go` + +`SecurityAudit` model (verbatim): + +```go +type SecurityAudit struct { + ID uint `json:"-" gorm:"primaryKey"` + UUID string `json:"uuid" gorm:"uniqueIndex"` + Actor string `json:"actor" gorm:"index"` + Action string `json:"action"` + EventCategory string `json:"event_category" gorm:"index"` + ResourceID *uint `json:"resource_id,omitempty"` + ResourceUUID string `json:"resource_uuid,omitempty" gorm:"index"` + Details string `json:"details" gorm:"type:text"` + IPAddress string `json:"ip_address,omitempty"` + UserAgent string `json:"user_agent,omitempty"` + CreatedAt time.Time `json:"created_at" gorm:"index"` +} +``` + +`SecurityService.LogAudit(a *models.SecurityAudit) error` (`security_service.go:239`) — fire-and-forget-with-sync-fallback: enqueues onto a buffered channel for async persistence, falling back to synchronous `persistAuditWithRetry` if the channel is full. Confirmed safe to call from a hot request path (proxy request handling) without blocking on DB I/O in the common case. + +`SecurityService.ListAuditLogs(filter AuditLogFilter, page, limit int)` (line 343) filters on `EventCategory` and `ResourceUUID` among others (lines 356–361) — **exactly the two fields the design brief proposes using.** + +`AuditLogHandler.List` (`backend/internal/api/handlers/audit_log_handler.go:26–80`), mounted at `GET /api/v1/audit-logs`, confirmed to read `c.Query("event_category")` and `c.Query("resource_uuid")` directly into `services.AuditLogFilter` (lines 39–44) — **the `?resource_uuid=...&event_category=orthrus_write` query-param pattern described in the design brief is real and already implemented**, no backend handler change needed to support it. + +**Gap found (not anticipated in the design brief) — frontend cannot actually use that query-param pattern today.** Two separate frontend issues: + +1. `frontend/src/api/auditLogs.ts:4` declares `EventCategory` as a **closed TypeScript union**: `'dns_provider' | 'certificate' | 'proxy_host' | 'user' | 'system'`. `'orthrus_write'` is not a member. Any frontend code passing `event_category: 'orthrus_write'` today would fail to type-check. This union must be extended (Section 3.5.4). +2. `frontend/src/pages/AuditLogs.tsx` (the existing generic audit log viewer, confirmed to exist and already support a table view + per-row detail modal + CSV export) initializes its `filters` state as a bare `useState({})` — it does **not** read `useSearchParams` or `window.location.search` on mount (confirmed via grep, zero hits for either in the file). A link such as `/audit-logs?resource_uuid=&event_category=orthrus_write` from anywhere else in the app would land on the page with **no filters pre-applied**, showing all audit logs unfiltered. This is a genuine, previously-unflagged UX gap: deep-linking into a filtered audit view does not work today. This spec proposes closing it generically (Section 3.5.3), not as an Orthrus-specific patch. + +### 2.7 Rate limiting infrastructure — `backend/internal/cerberus/rate_limit.go` (213 lines) + +`cerberus.RateLimitMiddleware()` and the standalone `cerberus.NewRateLimitMiddleware(...)` both return `gin.HandlerFunc`, backed by a per-IP `golang.org/x/time/rate.Limiter` map (`rateLimitManager`, lines 45–100) with a 10-minute idle-cleanup loop. Both are **Gin-coupled** (`func(ctx *gin.Context)`) — neither can be attached directly to the bespoke `http.Server` that `AgentSession.StartExternalProxy` constructs, since that server is a plain `net/http` server outside Gin entirely (confirmed Section 2.4). The underlying primitive, `golang.org/x/time/rate.Limiter`, is not Gin-specific and is directly reusable outside Gin. + +### 2.8 Docs — `docs/features/orthrus.md` (155 lines) + +Exact current text to be rewritten, quoted verbatim: + +- Line 34 (callout under "How It Works"): *"**Note:** Orthrus is read-only. It can list containers, images, and networks — but it cannot start, stop, delete, or modify anything on your remote machine. This is by design and cannot be changed."* +- Lines 88–104 ("What Orthrus Can (and Cannot) Do" section), specifically line 104: *"This restriction is enforced at every single request — there is no way to turn it off."* +- Lines 108–137 ("External Docker Proxy (Advanced)" section), specifically line 129: *"**Still strictly read-only.** Just like the rest of Orthrus, there is no way to turn this restriction off."* -This hotfix adds no new files, no new directories, no new container ports/env vars, and no new dependencies — only edits to existing `.go` and `.md` files. +All three claims are currently absolute and must be corrected to describe the new opt-in capability without weakening the stated guarantee for agents that remain in the (default) read-only mode. --- -## 3. Technical Specifications +## 3. Technical Specification -### 3.1 Gap 1 spec — `backend/internal/orthrus/muzzle.go` +### 3.1 Database Schema -**Change**: append two entries to `allowedDockerPatterns` (lines 42–49): +**Migration**: additive-only, via existing GORM `AutoMigrate` (no new registration needed — `OrthrusAgent` is already migrated per Section 2.1). ```go -var allowedDockerPatterns = []string{ - "/containers/*/json", - "/containers/*/logs", - "/containers/*/stats", - "/containers/*/top", - "/volumes/*", - "/networks/*", - "/images/*/json", // NEW: image inspect (RepoDigests) — read-only, used by update-checker tools - "/distribution/*/json", // NEW: registry digest check — read-only, used by update-checker tools +// backend/internal/models/orthrus_agent.go — new field, inserted after ExternalProxyPort +WriteEnabled bool `json:"write_enabled" gorm:"default:false"` +``` + +| Column | Type | Default | Nullable | Notes | +|---|---|---|---|---| +| `write_enabled` | SQLite boolean representation (as used by `gorm.io/driver/sqlite` for Go `bool`) | `false` | No (Go `bool` zero value) | Existing rows backfill to `false` automatically per Section 2.1 | + +No index needed — this column is only ever read by primary-key/UUID-scoped lookups (`findAgentByToken`, `Get(uuid)`), never filtered/queried in bulk. + +### 3.2 API Design + +#### 3.2.1 `PATCH /api/v1/orthrus/agents/:uuid` (existing endpoint, extended) + +`backend/internal/api/handlers/orthrus_handler.go:104–130` (`patchAgentRequest` / `Patch`) gains one new optional field, following the existing `*T` "present = intend to change" pattern already used for every other field on this struct: + +```go +type patchAgentRequest struct { + Name *string `json:"name"` + HecateTunnelUUID *string `json:"hecate_tunnel_uuid"` + DeviceID *string `json:"device_id"` + ResolvedAddress *string `json:"resolved_address"` + ExternalProxyPort *int `json:"external_proxy_port"` + WriteEnabled *bool `json:"write_enabled"` } ``` -Add a doc comment above the block (or extend the existing one at lines 39–41) explaining: -- Both new patterns are GET-only (enforced upstream by the unconditional method check at lines 79–84, which runs regardless of path). -- Neither permits any write/mutate operation — `/images/create` (image pull) is explicitly and deliberately **not** added. -- The `path.Match` single-segment-`*` limitation for namespaced image names (Section 2.1) is a known, accepted gap for this hotfix. -- `/distribution/*/json` causes the remote Docker daemon itself to make an outbound network request to whatever registry host is encoded in the image name — read-only with respect to local Docker state (it cannot mutate anything on the host), but callers can influence which external host the daemon contacts, so "read-only" here means "cannot mutate local Docker state," not "cannot cause outbound network activity." -- This is a best-effort expansion (no live traffic logs available from the reporting environment); more read-only entries may be needed if real-world testing against tools like Dockhand/Diun surfaces additional 403s — expected and acceptable follow-up, not a defect in this PR. - -**No changes** to `allowedDockerPaths` (the exact-match map), `Muzzle.ServeHTTP`, `versionPrefixRe`, or `sanitizePath`. - -#### Test additions — `backend/internal/orthrus/muzzle_test.go` - -1. Extend `TestMuzzle_DynamicPaths_Passthrough` (lines 115–141) with: - ```go - "/images/alpine/json", - "/v1.44/images/alpine/json", - "/images/nginx:latest/json", - "/distribution/alpine/json", - "/v1.44/distribution/alpine/json", - ``` -2. Extend `TestMuzzle_UnknownPath_Blocked` (lines 143–160) with a case documenting the known multi-segment limitation, with an explanatory comment so it reads as an intentional regression-guard rather than an oversight: - ```go - // Known limitation (see muzzle.go doc comment): path.Match's "*" does not - // cross "/", so namespaced image names are not matched by /images/*/json. - // This case documents current behavior; it is not a target of this fix. - "/images/library/nginx/json", - ``` -3. New table-style test `TestMuzzle_ImageAndDistributionEndpoints_POSTBlocked` (or extend the existing `TestMuzzle_POST_Blocked`, lines 67–84) confirming `POST /images/alpine/json` and `POST /distribution/alpine/json` are still 403 — belt-and-suspenders regression guard for the read-only guarantee, even though method-checking already happens unconditionally before any path match. - -**Validation gate**: `go test ./backend/internal/orthrus/... -run TestMuzzle -v` - -### 3.2 Gap 3 spec — remove hardcoded hostname from `session.go` - -**`backend/internal/orthrus/session.go` changes:** - -1. `ExternalProxyStatus` struct (lines 95–102): remove the `ConnectionString` field entirely. - ```go - type ExternalProxyStatus struct { - ConfiguredPort int `json:"configured_port"` - ActivePort int `json:"active_port"` - BoundAddress string `json:"bind_address"` - Active bool `json:"active"` - Error string `json:"error,omitempty"` - } - ``` - Add/adjust the doc comment on the struct to note that connection-string hostname resolution is now the API handler's responsibility (`OrthrusHandler.GetProxyStatus`), since `AgentSession` has no request context to resolve a caller-appropriate hostname from. - -2. `GetExternalProxyStatus()` (lines 329–361): remove the `connStr` construction block (lines 348–351) and the `ConnectionString: connStr,` field from the returned struct literal (line 357). No other logic in this method changes. - -**Validation gate**: `go build ./backend/...` (this alone will catch every call site that still references the removed field — see Section 3.2's test impact below). - -### 3.3 Gap 3 spec — hostname resolution in `orthrus_handler.go` - -**`backend/internal/api/handlers/orthrus_handler.go` changes:** - -1. Add imports: `"fmt"`, `"net"`, `"net/url"`. -2. Add a new unexported helper, placed near `GetProxyStatus`: - ```go - // resolveExternalProxyHost determines the hostname third-party tools should use - // to reach this Charon instance's external Docker proxy ports. It mirrors the - // X-Charon-URL header pattern used by GetInstallSnippets, but — unlike that - // handler — returns a bare hostname only (no scheme, no port): the external - // proxy's TCP port is independent of Charon's own web port, so the docker - // port is appended separately by the caller. - func resolveExternalProxyHost(c *gin.Context) string { - if raw := c.GetHeader("X-Charon-URL"); raw != "" { - if u, err := url.Parse(raw); err == nil && u.Hostname() != "" { - return u.Hostname() - } - } - if host, _, err := net.SplitHostPort(c.Request.Host); err == nil { - return host - } - return c.Request.Host - } - ``` -3. Update `GetProxyStatus` (lines 180–210) to build the connection string itself instead of relaying `status.ConnectionString`. **Ordering note**: this handler change (Commit 2, Section 7) lands *before* the struct field removal (Commit 3, Section 3.2/Section 7) — at this point `ExternalProxyStatus.ConnectionString` still exists on the struct, it simply becomes unread by this handler. That compiles cleanly (Go permits unused exported struct fields); the field is deleted later, once nothing references it: - ```go - if h.proxyResolver != nil { - if status, ok := h.proxyResolver.GetExternalProxyStatus(uuid); ok { - resp["agent_online"] = true - resp["active"] = status.Active - resp["active_port"] = status.ActivePort - resp["bind_address"] = status.BoundAddress - if status.Active && status.ActivePort > 0 { - resp["connection_string"] = fmt.Sprintf("tcp://%s:%d", resolveExternalProxyHost(c), status.ActivePort) - } - if status.Error != "" { - resp["error"] = status.Error - } - } - } - ``` - This preserves the JSON response shape and key name (`connection_string`) exactly, and preserves the existing guard condition (`active && activePort > 0`, previously enforced inside `session.go`, now enforced here) that leaves `connection_string` as the zero-value `""` when the proxy isn't actually active. - -**Why not consolidate with `GetInstallSnippets`'s host-resolution block into one shared helper** (explicit design note for Supervisor/reviewers): `GetInstallSnippets` needs a full `scheme://host[:port]` base URL for embedding in install snippets; `GetProxyStatus` needs a bare hostname with the port stripped, since the docker port is unrelated to Charon's web port. Forcing both through one function would require parameterizing "include scheme," "include port," and "port to substitute," adding complexity disproportionate to a tight hotfix, and would touch `GetInstallSnippets`'s already-tested, working code path unnecessarily — increasing blast radius for no behavioral gain. `GetInstallSnippets` is **not modified** by this PR. Flagged as a possible future consolidation, not undertaken here (Section 5). - -#### Test additions/updates — `backend/internal/api/handlers/orthrus_handler_test.go` - -1. **Required fix (assertion break now, compile-break prevention for Commit 3)**: `TestOrthrusHandler_GetProxyStatus_Connected` (lines 678–716) currently builds `liveStatus := orthrus.ExternalProxyStatus{..., ConnectionString: "tcp://charon:2375", ...}` (line 696) and asserts `assert.Equal(t, "tcp://charon:2375", resp["connection_string"])` (line 714). At this commit (Commit 2, Section 7), the `ConnectionString` field still exists on the struct — removing it is Commit 3's job (Section 3.2) — so this literal would still *compile* as-is; left unchanged, though, the assertion would fail because the handler no longer reads `status.ConnectionString`. Update proactively now, both to fix the assertion and to avoid leaving a stale field reference that would otherwise break Commit 3's standalone build once the field is deleted: - - Remove `ConnectionString: "tcp://charon:2375",` from the `liveStatus` literal. - - The test's `c.Request` is built via `httptest.NewRequest(http.MethodGet, ".../proxy-status", http.NoBody)` with no explicit `Host` set, so `net/http/httptest` defaults `Request.Host` to `"example.com"`. Update the assertion to `assert.Equal(t, "tcp://example.com:2375", resp["connection_string"])` to reflect the new handler-side construction. -2. New test `TestOrthrusHandler_GetProxyStatus_ConnectionString_UsesXCharonURLHeader`: set `c.Request.Header.Set("X-Charon-URL", "https://mybox.example.org:8443")` before calling `GetProxyStatus`; assert `resp["connection_string"] == "tcp://mybox.example.org:2375"` (i.e. the header's own port, `8443`, is discarded — only the hostname is taken from the header, and the docker port comes from `status.ActivePort`). -3. New test `TestOrthrusHandler_GetProxyStatus_ConnectionString_HostPortStripped`: set `c.Request.Host = "192.168.1.50:8443"` (no `X-Charon-URL` header); assert `resp["connection_string"] == "tcp://192.168.1.50:2375"` (port stripped from `Host`, replaced with `status.ActivePort`). -4. New test `TestOrthrusHandler_GetProxyStatus_ConnectionString_EmptyWhenInactive`: reuse the existing `errStatus`/inactive-status pattern (see `TestOrthrusHandler_GetProxyStatus_Connected_WithError`, lines 791–819) to confirm `resp["connection_string"] == ""` when `status.Active` is `false`, regardless of headers — regression guard for the `active && activePort > 0` guard condition moving from `session.go` into the handler. - -**No changes required** in `backend/internal/orthrus/session_coverage_test.go` or `backend/internal/orthrus/session_external_proxy_test.go` — confirmed via grep that no test in either file asserts on `ExternalProxyStatus.ConnectionString`'s value or presence (`TestGetExternalProxyStatus_ErrorFieldPopulated`, `TestGetExternalProxyStatus_NotStarted`, `TestGetExternalProxyStatus_Active`, etc. only check `Active`, `ActivePort`, `BoundAddress`, `ConfiguredPort`, `Error`). Compilation alone enforces correctness of the struct-field removal for these files. - -**Validation gate**: `go test ./backend/internal/orthrus/... ./backend/internal/api/handlers/... -run 'ProxyStatus|ExternalProxy' -v` - -### 3.4 Gap 2 spec — documentation - -#### `docs/features/orthrus.md` - -Insert a new `## External Docker Proxy (Advanced)` section, placed after the existing "What Orthrus Can (and Cannot) Do" section (after line 104) and before "Troubleshooting" (line 108), matching the file's plain-language, novice-friendly tone. Content to cover: - -- **What it is**: an optional TCP port that lets a *third-party tool on your network* (an update-checker like Dockhand or Diun, a monitoring dashboard, etc.) talk directly to this agent's Docker API through the same secure tunnel — without needing a separate `docker-socket-proxy` container. -- **How to turn it on**: click the gear icon next to an agent in **Remote Agents**, set a port (1024–65535), save. -- **Connection string format**: `tcp://:` — described generically ("``" is your Charon instance's own address, as reachable from wherever the third-party tool runs — Charon fills this in for you automatically"), explicitly **not** hardcoding `tcp://charon:` as a literal example, consistent with Gap 3's fix. -- **Still strictly read-only**: reiterate the existing "no way to turn it off" promise and list what the external proxy exposes, matching the updated allowlist from Gap 1 (containers, images, networks, volumes, system info/version/events, container logs/stats/top, image inspect, and registry digest checks for update-checker tools); note that for the registry digest check specifically, "read-only" means it cannot change anything on your Docker host — it does cause your agent's Docker daemon to make an outbound request to the image's registry, which is expected behavior for update-checking tools. -- **New Troubleshooting row** appended to the existing table (lines 108–116): "Third-party tool can't reach my agent's Docker API" → Likely Cause: "Wrong port configured in the tool" → Fix: "Check the tool is using the External Proxy port shown in the gear-icon dialog — not Charon's main web port." - -#### `docs/guides/remote-docker-setup.md` - -Insert a new optional step, **Step 7 — (Optional) Let Another Tool Talk to Your Agent's Docker API**, after the existing Step 6 (ends line 124) and before "(Optional) Add Uptime Monitoring" (line 127), following the same numbered-step / screenshot-placeholder / bold-callout style as the rest of the file. Content: brief restatement of what the External Proxy port is, how to set it (gear icon), the generic `tcp://:` format, and a cross-reference link: *"Full detail: [Orthrus guide → External Docker Proxy](../features/orthrus.md#external-docker-proxy-advanced)."* Also add a matching row to this file's own Troubleshooting table (lines 142–150), same content as the `orthrus.md` row above. - -**Validation gate**: manual proofread against existing tone (no automated doc linter in this repo per current tooling); confirm both new sections render correctly as Markdown (headings, tables) and all internal links resolve. +`services.OrthrusService.Patch` (`orthrus_service.go:84–116`) gains a new `writeEnabled *bool` parameter, following the exact shape of the existing `externalProxyPort *int` handling (lines 102–108), **minus** the port-range validation (a bool has no invalid values) but **plus** an audit-log call — this is the one field on this endpoint whose change is itself security-relevant enough to warrant its own audit entry (distinct from, and in addition to, the per-write-request audit entries specified in 3.3.7): ---- +```go +func (s *OrthrusService) Patch(uuid string, name, hecateTunnelUUID, deviceID, resolvedAddress *string, externalProxyPort *int, writeEnabled *bool) (*models.OrthrusAgent, error) { + // ... existing fields unchanged ... + if writeEnabled != nil { + updates["write_enabled"] = *writeEnabled + } + // ... existing len(updates)==0 / db.Model(...).Updates(...) unchanged ... + // NEW: if writeEnabled != nil, call s.securityService.LogAudit(&models.SecurityAudit{ + // Actor: , + // Action: "orthrus_write_enabled" | "orthrus_write_disabled", + // EventCategory: "orthrus_write", + // ResourceUUID: uuid, + // Details: `{"agent_name": ""}`, + // }) after a successful Updates call. +} +``` + +Note: `OrthrusService` does not currently hold a `*services.SecurityService` reference — this is a genuine new dependency to wire through `NewOrthrusService` (constructor signature change) and its call site in `routes.go`. `Actor` on this audit entry follows whatever the existing convention is for admin-initiated actions elsewhere in the codebase (the implementation phase must locate and reuse that convention rather than inventing a new one — flagged here as a research item for the backend-dev phase, not resolved by this spec, since no other `Patch`-style admin mutation currently emits an audit entry to cite as precedent within `OrthrusService`). + +**Response**: unchanged shape — full `OrthrusAgent` JSON, now including `"write_enabled": `. + +**Validation/errors**: none beyond existing `ShouldBindJSON` 400 and `gorm.ErrRecordNotFound` → 404 handling; a bool field cannot itself be malformed once it type-checks. + +#### 3.2.2 `GET /api/v1/orthrus/agents/:uuid/proxy-status` (existing endpoint, extended) + +`OrthrusHandler.GetProxyStatus` (lines 201–233) gains two additional response fields so the frontend dialog can show whether write mode is *configured* (DB value) vs. *currently active for the live session* (the value the agent actually negotiated at its last connect — these can differ if the operator toggled the flag while the agent is connected, exactly the same "configured differs from active" situation already modeled for `external_proxy_port`): -## 4. Data Flow Summary (Gap 3, before/after) - -```mermaid -sequenceDiagram - participant Tool as Third-party tool - participant FE as Frontend (AgentExternalProxyDialog) - participant API as OrthrusHandler.GetProxyStatus - participant Sess as AgentSession.GetExternalProxyStatus - - Note over API,Sess: BEFORE — hostname hardcoded deep in session.go - FE->>API: GET /orthrus/agents/:uuid/proxy-status - API->>Sess: GetExternalProxyStatus() - Sess-->>API: connection_string: "tcp://charon:2375" - API-->>FE: relay verbatim - - Note over API,Sess: AFTER — session reports raw state only; handler resolves hostname - FE->>API: GET /orthrus/agents/:uuid/proxy-status - API->>Sess: GetExternalProxyStatus() - Sess-->>API: active true, active_port 2375, no hostname - API->>API: resolveExternalProxyHost(c) via X-Charon-URL header or c.Request.Host - API-->>FE: connection_string: "tcp://resolved-host:2375" +```go +resp := gin.H{ + "agent_uuid": agent.UUID, + "agent_online": false, + "configured_port": agent.ExternalProxyPort, + "configured_write_enabled": agent.WriteEnabled, // NEW + "active_write_enabled": false, // NEW — set from live session below + "active": false, + "active_port": 0, + "bind_address": "", + "connection_string": "", + "error": "", +} +if h.proxyResolver != nil { + if status, ok := h.proxyResolver.GetExternalProxyStatus(uuid); ok { + // ... existing fields ... + resp["active_write_enabled"] = status.WriteEnabled // NEW — see 3.3.2 + } +} ``` -No new HTTP endpoints, no new request/response fields, no DB schema changes. `agent.ExternalProxyPort` (`backend/internal/models/orthrus_agent.go:48`) is unchanged. +`orthrusProxyStatusResolver` interface (line 19–21) is unchanged in shape (still `GetExternalProxyStatus(agentUUID string) (orthrus.ExternalProxyStatus, bool)`) — the new field rides inside the existing `ExternalProxyStatus` struct (Section 3.3.2). ---- +#### 3.2.3 Audit query (no new endpoint — existing endpoint, new usage) -## 5. Explicit Out-of-Scope +`GET /api/v1/audit-logs?resource_uuid=&event_category=orthrus_write` — confirmed already fully functional server-side per Section 2.6, zero backend changes needed. Frontend usage requires the type-union and deep-link fixes in Section 3.5.3/3.5.4. -- **No Muzzle matching-engine rework.** The `path.Match` single-segment `*` limitation for namespaced image names (Section 2.1) is documented, not fixed. -- **No broader Orthrus rearchitecture.** `StartExternalProxy`, the yamux tunnel, the Muzzle wrapping mechanism, and `AgentSession`'s lifecycle are untouched beyond the one-field struct change in Section 3.2. -- **No new UI.** `AgentExternalProxyDialog.tsx` and `OrthrusAgentManager.tsx` are not modified (Section 2.3). -- **No port-collision-detection UI polish.** The existing "reconnect notice" behavior (tested in `tests/orthrus-external-proxy.spec.ts:240-260`) is untouched. -- **No multi-hostname configuration.** `resolveExternalProxyHost` resolves one hostname per request from the caller's own perspective (header or `Host`), matching `GetInstallSnippets`'s existing single-hostname model exactly — no per-agent or per-network hostname overrides are introduced. -- **No consolidation of `GetInstallSnippets` and `GetProxyStatus` hostname logic into one shared function** — see the explicit reasoning in Section 3.3. -- **No change to `frontend/src/api/orthrus.ts`'s `getAgentProxyStatus`** to add an `X-Charon-URL` header — the fallback path already produces a correct, non-hardcoded result (Section 2.3); adding the header for extra robustness behind Host-rewriting reverse proxies is a reasonable future enhancement but is not required to close Gap 3 and is left out to keep this hotfix backend-only. +### 3.3 Component Design — Backend ---- +#### 3.3.1 Wire mechanism: handshake write-mode negotiation -## 6. Security Review Requirement +**Decision: use the WebSocket upgrade response header, not a new yamux control-message stream type.** -**Commit 1 (Section 7) touches a security boundary.** The Muzzle allowlist is the only mechanism preventing a tunnelled remote Docker socket from exposing write/mutate capability to whatever network the external proxy port is bound on. `docs/features/orthrus.md` makes an absolute, user-facing promise about read-only enforcement. Per this repo's governance rules, `supervisor` agent review is **required before implementation of Commit 1**, specifically to verify: +Two mechanisms were evaluated: -1. Both new patterns (`/images/*/json`, `/distribution/*/json`) are genuinely read-only in the Docker Engine API (no known write side effects on GET). -2. No accidental widening beyond the two intended patterns (e.g. no overly broad wildcard that also matches an unintended write endpoint). -3. The method-check-before-path-check ordering in `Muzzle.ServeHTTP` (lines 79–84 run before lines 86–98) still holds after the change, so POST/PUT/DELETE to the new paths remains blocked unconditionally. -4. The documented `path.Match` multi-segment limitation (Section 2.1) is an acceptable disclosed gap, not a silent security hole. +| Option | Mechanism | Verdict | +|---|---|---| +| **A (chosen)** | Server sets a response header during `wsUpgrader.Upgrade(...)`; agent reads it off the `*http.Response` returned by `dialer.DialContext(...)` | Delivered atomically as part of the handshake itself — no ordering race with the first Docker/port-forward stream, no new byte-level framing to define, both sides already have the exact extension point wired (currently unused: `Upgrade`'s `responseHeader` param is `nil`; `DialContext`'s response is currently discarded with `_`) | +| B (rejected) | New `streamTypeControl = byte(0x03)` yamux stream, opened by the server immediately after `NewAgentSession`, carrying a 1-byte payload | Requires the agent to guarantee it processes the control stream *before* accepting/dispatching any Docker stream it might race against (yamux stream ordering across concurrent `Accept` calls is not inherently sequenced this way), adds a third hand-maintained stream-type constant in two places (compounding, not reducing, the exact drift class flagged in GH #1161), and provides no benefit Option A doesn't already give for a single boolean of session-start-time-only data | -Commits 2–5 (hostname fix, docs) are not security-boundary changes but should still pass through the same review pass for consistency, per this repo's supervisor workflow. +**Concrete spec for Option A**: ---- +- Header name: `X-Orthrus-Write-Enabled`, value `"true"` or `"false"` (string, not a custom encoding — consistent with the existing `X-Orthrus-Version` / `X-Orthrus-Name` request headers already sent by the agent in `leash.go:124–126`). +- Backend (`server.go:HandleWebSocket`): after `agent, err := s.findAgentByToken(token)` succeeds (line 75) and before `wsUpgrader.Upgrade` (line 81), build: + ```go + respHeader := http.Header{} + respHeader.Set("X-Orthrus-Write-Enabled", strconv.FormatBool(agent.WriteEnabled)) + conn, err := wsUpgrader.Upgrade(c.Writer, c.Request, respHeader) + ``` +- Agent (`leash.go:connect`): change `wsConn, _, err := dialer.DialContext(...)` to `wsConn, resp, err := dialer.DialContext(...)`, then: + ```go + writeEnabled := resp != nil && resp.Header.Get("X-Orthrus-Write-Enabled") == "true" + ``` + and pass `writeEnabled` into wherever the agent-side `muzzle.Filter` is constructed for this connection (Section 3.4 — this requires `Filter` to become connection-scoped rather than process-scoped; see below). +- `AgentSession` gains a new field: `writeEnabled bool`, set once in `NewAgentSession` (new parameter) and never mutated for the life of that session — exactly mirroring the "session-scoped state fixed at connect time, changes require reconnect" pattern already established for `extProxyPort`. +- `HandleWebSocket` passes `agent.WriteEnabled` into `NewAgentSession(agent.UUID, agent.Name, agent.WriteEnabled, conn)`. -## 7. Commit Slicing Strategy +**Reconnect-to-apply semantics**: identical pattern to `ExternalProxyPort`. Toggling `WriteEnabled` via `PATCH .../agents/:uuid` only updates the DB row; the live session (if any) keeps whatever value it negotiated at its last `HandleWebSocket` call until the agent's next reconnect. This is not a new behavior to build — it falls out naturally from `writeEnabled` being read once, at session-start, exactly like every other per-session value already is. -**Decision: single PR on `development`, sliced into ordered commits.** One feature (this hotfix) = one PR, per `CLAUDE.md`. Conventional commit prefix `fix:` for all behavior-changing commits (Commits 1–3); `docs:` for the documentation commit (Commit 4). Each commit carries its own tests — this repo's TDD convention for backend work, not a separate test commit. +#### 3.3.2 `ExternalProxyStatus` and `Muzzle` — threading write mode through -| # | Commit | Scope | Files | Depends on | Validation gate | -|---|---|---|---|---|---| -| **1** | `fix(orthrus): allow read-only image/distribution inspect through Docker proxy muzzle` | Gap 1 — expand `allowedDockerPatterns`; add tests documenting both the new allowlist entries and the known multi-segment-name limitation | `backend/internal/orthrus/muzzle.go`, `backend/internal/orthrus/muzzle_test.go` | None (independent of Commits 2–3) | `go test ./backend/internal/orthrus/... -run TestMuzzle -v`; `make lint-staticcheck-only`; full `lefthook run pre-commit` should still be run since this is a security-boundary change (GORM scan not applicable — no model/GORM changes) | -| **2** | `fix(orthrus): resolve external proxy hostname from request context instead of hardcoding it` | Gap 3, part A — add `resolveExternalProxyHost`, update `GetProxyStatus` to construct `connection_string` from request context instead of relaying `status.ConnectionString`; update/add handler tests. `ExternalProxyStatus.ConnectionString` still exists on the struct at this checkpoint (it is removed in Commit 3) — this commit only stops *reading* it, which compiles cleanly since Go permits unused exported struct fields | `backend/internal/api/handlers/orthrus_handler.go`, `backend/internal/api/handlers/orthrus_handler_test.go` | None (independent of Commit 1; compiles standalone against the current, unmodified `session.go` — the field it stops reading is still present) | `go build ./backend/...`; `go test ./backend/internal/api/handlers/... -run ProxyStatus -v` | -| **3** | `fix(orthrus): remove hardcoded "charon" hostname from ExternalProxyStatus` | Gap 3, part B — remove the now-unread `ConnectionString` field and its hardcoded construction from `session.go`; pure cleanup with no observable behavior change, since Commit 2 already moved hostname resolution to the handler | `backend/internal/orthrus/session.go` | **Commit 2** (must land first: this commit deletes a struct field, so it only compiles standalone once its sole production reader — the handler — has already stopped referencing it) | `go build ./backend/...` (confirms zero remaining references to the removed field, now that Commit 2 has already migrated the only production reader); `go test ./backend/internal/orthrus/... -v`; `go test ./backend/...` (full backend suite — the riskier tail-end check for this two-commit pair, since this is where the struct itself changes shape) | -| **4** | `docs(orthrus): document the External Docker Proxy feature` | Gap 2 — new sections in both docs files, generic (non-hardcoded) connection-string example, new troubleshooting rows | `docs/features/orthrus.md`, `docs/guides/remote-docker-setup.md` | None (independent — ordered last per this repo's suggested foundation→backend→docs sequence, and to avoid describing not-yet-merged behavior) | Manual proofread; Markdown renders correctly; both new internal cross-reference links resolve | -| **5** | `fix(orthrus): verify muzzle + proxy-status hotfix end-to-end` *(hardening/verification — only added if the full-suite DoD pass in Section 8 surfaces something to fix; otherwise this step folds into Commit 3's validation and is not a separate commit)* | Full Definition-of-Done pass: run the complete existing `tests/orthrus-*.spec.ts` Playwright suite (expected: pass unmodified, Section 2.3), full backend coverage, full lint | None expected — placeholder only | Commits 1–4 | `npx playwright test --project=firefox tests/orthrus-*.spec.ts`; `bash scripts/local-patch-report.sh`; `scripts/go-test-coverage.sh`; full `lefthook run pre-commit` | +```go +// session.go — ExternalProxyStatus gains one field +type ExternalProxyStatus struct { + ConfiguredPort int `json:"configured_port"` + ActivePort int `json:"active_port"` + BoundAddress string `json:"bind_address"` + Active bool `json:"active"` + WriteEnabled bool `json:"write_enabled"` // NEW — the negotiated value for this live session + Error string `json:"error,omitempty"` +} +``` -**Rationale for ordering**: Commits 1 and 2 are independent of each other (different files, different gaps) and could technically be reordered, but are kept as separate commits because Commit 1 is the security-sensitive one that most benefits from being reviewable in isolation (small, self-contained diff). Commits 2 and 3 together form one logical "hostname fix" unit (Gap 3), but are ordered so each compiles standalone on its own: Commit 2 (the handler fix) lands first and stops `GetProxyStatus` from reading `ExternalProxyStatus.ConnectionString` — the field still exists on the struct at that point, just unused, which Go permits without error. Commit 3 (the struct field removal) lands second, once Commit 2 has already eliminated the field's only production reader, so its own `go build` also passes standalone too. Deleting the field before updating its last reader — the inverse order — would break the handler-fix commit's build at that checkpoint, which is exactly the defect this ordering avoids. Docs (Commit 4) come last since they describe the post-fix allowlist contents (Gap 1) and post-fix connection-string format (Gap 3) — writing them last avoids describing not-yet-merged behavior. +`AgentSession.GetExternalProxyStatus()` (line 332) sets `WriteEnabled: s.writeEnabled` alongside the existing fields. -### Rollback / contingency notes (for the PR as a whole) +`NewMuzzle` (`muzzle.go:101`) gains required parameters: `func NewMuzzle(next http.Handler, writeEnabled bool, writeLimiter *rate.Limiter, auditLogger AuditLogger, agentUUID string) *Muzzle` (the last two per Section 3.3.7). `StartExternalProxy` (`session.go:296`, the `Handler: NewMuzzle(rp)` line) is updated accordingly. `Muzzle` struct gains a `writeEnabled bool` field, set once at construction — **not** looked up from the DB per-request (this is a deliberate design choice: doing so would reintroduce the exact TOCTOU-adjacent inconsistency the reconnect-to-apply pattern is meant to avoid, and would add a DB round-trip to the hot proxy path). -- **Commit 1 rollback**: revert is a two-line removal from `allowedDockerPatterns`; zero blast radius outside `muzzle.go`/`muzzle_test.go`. If Supervisor review (Section 6) flags either new pattern as unsafe, drop only that one pattern and keep the other — they are independent list entries. -- **Commits 2+3 rollback**: revert in reverse order of application if both must be undone — Commit 3 first, then Commit 2 — since Commit 3 depends on Commit 2 having already landed. Commit 3 can also be safely reverted alone at any time: it only re-adds an unused struct field and its assignment, which nothing reads, so this is a behavioral no-op. Commit 2 should **not** be reverted while Commit 3 is still applied — the handler would go back to referencing a field that no longer exists on the struct, breaking the build. If both are reverted, `connection_string` reverts to the hardcoded `"tcp://charon:"` behavior — a regression, but not a security issue, and matches current `main` behavior, so this is a safe fallback if an unexpected issue surfaces post-merge. -- **Commit 4 rollback**: docs-only, zero functional risk; can be reverted or amended independently at any time without touching code. -- **If Monday's merge window is missed**: no urgency-driven shortcuts (no `--no-verify`, no skipping Supervisor review) — `CLAUDE.md`'s emergency-bypass path requires a follow-up issue and is not warranted here since none of the three gaps are actively exploited or user-blocking; slipping to the next merge window is an acceptable contingency. +#### 3.3.3 Backend muzzle: new write allowlist + body validation ---- +```go +// muzzle.go — new, only consulted when m.writeEnabled is true +var allowedWriteExactPaths = map[string]struct{}{ + "/containers/create": {}, // POST — body-validated, see below + "/images/create": {}, // POST — query-string only (fromImage=, tag=), no body +} -## 8. Acceptance Criteria (Definition of Done) +var allowedWritePatterns = []struct{ method, pattern string }{ + {http.MethodPost, "/containers/*/start"}, + {http.MethodPost, "/containers/*/stop"}, + {http.MethodPost, "/containers/*/restart"}, + {http.MethodDelete, "/containers/*"}, +} +``` -This hotfix follows the full `CLAUDE.md` Definition of Done (Task Completion Protocol), scoped to the files touched: +`ServeHTTP` control flow, after the existing unconditional-GET-passthrough logic falls through to "not found in any read allowlist": -1. **Playwright E2E**: `npx playwright test --project=firefox` — full suite passes; `tests/orthrus-external-proxy.spec.ts`, `tests/orthrus-proxy-paths.spec.ts`, `tests/orthrus-agent-install.spec.ts`, `tests/orthrus-agents.spec.ts`, `tests/uptime-orthrus.spec.ts` specifically confirmed to pass **unmodified** (Section 2.3). -2. **GORM Security Scan**: not triggered — no changes under `backend/internal/models/**`, no GORM queries or migrations touched. -3. **Local Patch Coverage Preflight**: `bash scripts/local-patch-report.sh` — artifacts generated at `test-results/local-patch-report.md` / `.json`. -4. **Security Scans**: `lefthook run pre-commit` (CodeQL Go + JS) — zero high/critical findings; Trivy container scan — no new findings (no new dependencies). -5. **Staticcheck**: `make lint-staticcheck-only` — zero errors on `muzzle.go`, `session.go`, `orthrus_handler.go`. -6. **Coverage**: `scripts/go-test-coverage.sh` — overall ≥85%, with the new/changed lines in `muzzle.go`, `session.go`, `orthrus_handler.go` fully covered by the tests specified in Sections 3.1 and 3.3. -7. **Build**: `cd backend && go build ./...` succeeds (no frontend build step required — no frontend files touched, but `cd frontend && npm run build` should still be run as a smoke check per the standard DoD). -8. **Muzzle allowlist regression guard**: `TestMuzzle_UnknownPath_Blocked` and `TestMuzzle_POST_Blocked` continue to pass, proving no unintended widening. -9. **Hostname regression guard**: no production code anywhere in `backend/` contains the literal string `"charon:"` inside a `fmt.Sprintf("tcp://...")`-style construction (manual grep check as a final gate: `grep -rn 'tcp://charon' backend/ --include="*.go"` should return zero matches in non-test files after Commits 2–3 land). -10. **Docs render correctly**: both new sections in `docs/features/orthrus.md` and `docs/guides/remote-docker-setup.md` reviewed for tone-match and correct Markdown rendering; no hardcoded `tcp://charon:` example anywhere in either doc. -11. **Supervisor sign-off** obtained per Section 6 before Commit 1 is written, and again before the PR is opened for merge. +```go +if m.writeEnabled { + if r.Method == http.MethodPost { + if _, ok := allowedWriteExactPaths[stripped]; ok { + if stripped == "/containers/create" { + if !validateContainerCreateBody(r) { // Section 3.3.4 — 403 on dangerous HostConfig + m.auditBlocked(r, "disallowed HostConfig field") + http.Error(w, "Forbidden: disallowed HostConfig field", http.StatusForbidden) + return + } + } + if !m.writeLimiter.Allow() { + m.auditRateLimited(r) + http.Error(w, "Too Many Requests", http.StatusTooManyRequests) + return + } + m.auditAllowed(r) + m.next.ServeHTTP(w, r) + return + } + } + for _, p := range allowedWritePatterns { + if r.Method == p.method { + if matched, err := path.Match(p.pattern, stripped); err == nil && matched { + if !m.writeLimiter.Allow() { + m.auditRateLimited(r) + http.Error(w, "Too Many Requests", http.StatusTooManyRequests) + return + } + m.auditAllowed(r) + m.next.ServeHTTP(w, r) + return + } + } + } +} +``` ---- +This is deliberately placed **after** all read-allowlist checks and **still behind** the file's existing structure — write paths never bypass the version-prefix-strip + `path.Clean` traversal hardening that already runs unconditionally at the top of `ServeHTTP` (line 109–114), since that code is untouched. -## 9. Hardening Commits — Pre-existing E2E Failure Root Causes +#### 3.3.4 `POST /containers/create` body validation — hybrid allowlist/denylist -**Status**: Investigation-only addendum, appended after Commits 1–5 (backend/docs hotfix, `98a68b67`, `7f307c3c`, `1eb266d2`, `c778b53a`, `1caa4c65`) and the QA-report commit (`d70be096`) had already landed. Nothing in this section touches, reopens, or depends on any file changed by those six commits — both issues below are pre-existing, previously-out-of-scope E2E failures unrelated to the Orthrus External Docker Proxy hotfix. They are pulled in now as two additional, independent hardening commits in the same PR, per `CLAUDE.md`'s "one feature = one PR, sliced into ordered commits" rule. Root-cause tracing followed the mandatory Root Cause Analysis Protocol (entry point → transformation → persistence → exit point) for both issues before any fix was specced. +**Decision: reject outright (403), do not silently strip.** A request whose body contains a disallowed field is fully rejected with a `403` and a specific reason, rather than having the dangerous fields silently dropped and the (now-different) request forwarded anyway. Rationale: silently mutating a security-relevant request body means the caller's Docker client believes it asked for one container configuration and receives a different (silently downgraded) one — a correctness and debuggability hazard on top of the security one. A hard reject is simpler to reason about, simpler to test, and gives the calling tool (and the audit log) an unambiguous signal. -**Security boundary check (both issues)**: confirmed by direct inspection — neither issue's investigation or fix touches `backend/internal/orthrus/muzzle.go`, `session.go`, `orthrus_handler.go`, or any other file in `backend/internal/orthrus/**`. Both are frontend-E2E-test-only concerns: Issue 1 concerns a Playwright helper's navigation target and the (already-shipped, already-deprecated-elsewhere) `RemoteServerForm`/`ConnectionTypeSelector` UI; Issue 2 concerns a Playwright test's locator strategy against `frontend/src/pages/Uptime.tsx`'s existing sort behavior. **No bearing on the Orthrus security boundary.** +**Decision: hybrid top-level-key allowlist + strict value validation, not a pure denylist.** A pure denylist (reject known-dangerous field names) fails open against *future* Docker Engine API fields this spec's authors don't know about yet. The validator instead: -### 9.1 Issue 1 — `tests/orthrus-agent-install.spec.ts` (all 18 tests failing) +1. Parses the request body as JSON into `map[string]json.RawMessage` (top-level keys only — does not need to fully model Docker's `ContainerCreateConfig` schema). +2. For the `HostConfig` sub-object specifically, re-parses it into a second `map[string]json.RawMessage` and applies an **explicit allowlist of top-level `HostConfig` keys** known to be safe for a same-host container recreate (`PortBindings`, `RestartPolicy`, `Memory`, `MemorySwap`, `NanoCpus`, `CpuShares`, `Mounts` — filtered further, see below — `Dns`, `DnsSearch`, `ExtraHosts`, `LogConfig`, `AutoRemove`, `ReadonlyRootfs`, `Init`, `NetworkMode` — value-checked, see below). **Any `HostConfig` key not on this allowlist causes rejection** — this is what gives fail-closed behavior against unknown future fields, not an enumerated denylist. +3. Even within the allowed key `Mounts`, each entry's `"Type"` must be `"volume"` or `"tmpfs"` — `"bind"` type mounts are rejected (this is the modern-API equivalent of the legacy `Binds` field, which is simply absent from the allowlist and therefore already rejected at step 2 if present at all). +4. **`Mounts[].VolumeOptions.DriverConfig` is rejected outright, regardless of `Type`.** This closes a documented bypass of step 3: Docker's default `local` volume driver accepts `{"type":"none","device":"/host/path","o":"bind"}` inside `VolumeOptions.DriverConfig.Options`, which makes a `Type: "volume"` mount behave exactly like an arbitrary host bind mount — fully equivalent in effect to the `Type: "bind"` / legacy `Binds` escape already blocked by step 3, but reached through a field one level deeper than the top-level `HostConfig` key allowlist in step 2 checks. Critically, this bypass never calls `POST /volumes/create` (which step-2-adjacent reasoning might suggest is the relevant control point) — it rides entirely inside the already-allowed `POST /containers/create` body, so the existing exclusion of `/volumes/create` from the write allowlist (Section 3.3.3) does not mitigate it. The check is a pure key-presence test: if any `Mounts[]` entry contains a `VolumeOptions.DriverConfig` key at all, the whole request is rejected — no attempt is made to selectively allow safe `DriverConfig.Options` key/value pairs, matching this section's "hard reject over silent strip" philosophy (see the decision at the top of this section) rather than trying to enumerate which driver options are safe. +5. Top-level body keys outside `HostConfig` (`Image`, `Cmd`, `Entrypoint`, `Env`, `Labels`, `ExposedPorts`, `WorkingDir`, `User`, `Healthcheck`, `StopSignal`, `StopTimeout`, `NetworkingConfig` limited to a single entry — see Section 4.2 for the multi-network limitation) are passed through without value-level restriction; none of them can express a host-escape primitive the way `HostConfig` can. -**Root cause — confirmed, and it is *not* what QA's initial finding described.** QA was correct that `#connection-type` no longer exists, but wrong about the fix being a simple selector swap. Tracing the full flow (entry point → UI → exit point): +Fields that fall outside the `HostConfig` allowlist above (i.e., presence of the key at all → reject) include, named here for the record even though the mechanism is allowlist-based, not because they are separately special-cased: `Privileged`, `CapAdd`, `CapDrop`, `Binds`, `PidMode`, `IpcMode`, `UTSMode`, `CgroupnsMode`, `Devices`, `DeviceCgroupRules`, `SecurityOpt`, `Sysctls`, `Ulimits` (borderline-safe but excluded from v1 for simplicity), `GroupAdd`. -- **Entry point**: `openOrthrusWizard()` (`tests/orthrus-agent-install.spec.ts:22-75`) calls `page.goto('/remote-servers')` (line 49), then `page.locator('#connection-type').selectOption('orthrus')` (line 58-59), then expects a `getByRole('button', { name: /provision.*agent/i })` (line 61) to appear inline in that same form. -- **Transformation (live component trace)**: `/remote-servers` redirects to `/hecate/remote-servers` (`frontend/src/App.tsx:84,90`) → renders `frontend/src/pages/RemoteServers.tsx`, which renders `frontend/src/components/RemoteServerForm.tsx` (line 362) on "Add Server" click. `RemoteServerForm.tsx:195-259` delegates the connection-type UI to `frontend/src/components/hecate/ConnectionTypeSelector.tsx`, which renders a `fieldset`/`radiogroup` of three radio buttons (`direct` / `agent` / `provider`, lines 46-99) — **not** a `` **dropdown of already-provisioned agents** (populated from `useAgentList()`) — there is no name-entry field, no "Provision…Agent" button, and no install-wizard trigger anywhere in this component or its parent (`RemoteServerForm.tsx`) in "agent" mode. Confirmed via full-file read of both components: the string "provision" does not appear in either file. -- **Why**: this is by design, not a bug. Commit `4e9c5a9e` ("fix(hecate): fix stale E2E selectors, close unit test gaps, and polish UX", 2026-05-05) — the same commit QA cited — states explicitly in its own message: *"The RemoteServerForm connection type redesign replaced a `` requirement since no exact precedent exists): placeholder/label `t('hecate.writeMode.confirmPrompt', { name: agent.name })` ("Type **{name}** to confirm"), Save/Enable button `disabled` until the input's trimmed value strictly equals `agent.name`. +3. When flipping **off**: no typed confirmation — a single `patch({ write_enabled: false })` on Save, consistent with "disabling is always low-friction." +4. A **distinct** warning banner (separate `role="note"` block, not reusing `hecate.externalProxy.securityWarning`'s copy or DOM node), proposed text: *"Enabling write mode lets tools connected through this agent's External Docker Proxy pull images, and start, stop, restart, remove, and recreate containers on this machine. Every allowed action is logged. This does not grant shell access, volume/network changes, or any operation not explicitly listed below."* — visually and textually distinguishable from the External Proxy dialog's network-exposure warning, per Objective 7 / design brief. +5. Fixed, non-editable list of the permitted operations (Section 3.3.3's allowlist, rendered as user-facing prose, not raw Docker paths): "Pull a new image", "Start a container", "Stop a container", "Restart a container", "Remove a container", "Create (recreate) a container" — rendered only when the toggle is (or is about to be) on. +6. Reconnect notice, mirroring `configuredDiffersFromActive` exactly but comparing `agent.write_enabled` (configured) against `proxyStatus.active_write_enabled` (live, from the extended `GetProxyStatus` response, Section 3.2.2) rather than port numbers — reusing `hecate.externalProxy.reconnectNotice`'s *pattern* but a new translation key (`hecate.writeMode.reconnectNotice`) since the surrounding sentence needs to reference write mode, not the port. +7. Link/button: "View write-access audit log" → navigates to `/audit-logs?resource_uuid=&event_category=orthrus_write` (Section 3.5.3 makes this actually pre-filter on arrival). + +`OrthrusAgentManager.tsx` gains: a new icon button (proposed: `ShieldCheck` or `KeyRound` from `lucide-react`, distinct from the existing `Settings` gear used for the proxy dialog) in the actions cell, a `writeModeAgent` state slot mirroring `proxyConfigAgent`'s, and a `WRITE` badge (mirroring the existing `PROXY` badge at line 141–145) rendered when `agent.write_enabled` is true. + +#### 3.5.3 `AuditLogs.tsx` deep-link support (new, generic fix — Section 2.6 gap) + +`AuditLogs.tsx`'s `filters` `useState({})` initializer becomes a lazy initializer reading `URLSearchParams(window.location.search)` for any of the existing `AuditLogFilters` keys present in the query string (`event_category`, `resource_uuid`, `actor`, `action`, plus date filters if present) — a generically useful fix, not Orthrus-specific, using React Router's `useSearchParams` if the app already uses React Router (needs confirming which router the app uses during implementation; if a router with search-param hooks is already in place elsewhere in the codebase, reuse it for consistency rather than raw `window.location.search` parsing). + +#### 3.5.4 Type changes ```ts -await test.step('verify each monitor card shows its own independent status badge', async () => { - const orthrusCard = page.getByTestId('monitor-card').filter({ hasText: 'Remote Server (Orthrus)' }); - const tcpCard = page.getByTestId('monitor-card').filter({ hasText: 'Dockhand Service' }); - await expect(orthrusCard.getByTestId('status-badge')).toHaveAttribute('data-status', 'up'); - await expect(tcpCard.getByTestId('status-badge')).toHaveAttribute('data-status', 'up'); -}); - -await test.step('verify Orthrus monitor card shows ORTHRUS type and TCP monitor card shows TCP type', async () => { - const orthrusCard = page.getByTestId('monitor-card').filter({ hasText: 'Remote Server (Orthrus)' }); - const tcpCard = page.getByTestId('monitor-card').filter({ hasText: 'Dockhand Service' }); - await expect(orthrusCard).toContainText('ORTHRUS'); - await expect(tcpCard).toContainText('TCP'); -}); +// frontend/src/api/orthrus.ts +export interface OrthrusAgent { + // ...existing fields... + write_enabled: boolean; // NEW +} +export interface PatchAgentRequest { + // ...existing fields... + write_enabled?: boolean; // NEW +} +export interface ExternalProxyStatus { + // ...existing fields... + configured_write_enabled: boolean; // NEW + active_write_enabled: boolean; // NEW +} + +// frontend/src/api/auditLogs.ts +export type EventCategory = + | 'dns_provider' | 'certificate' | 'proxy_host' | 'user' | 'system' + | 'orthrus_write'; // NEW +export type AuditAction = + | /* ...existing... */ + | 'orthrus_write_allowed' | 'orthrus_write_blocked' + | 'orthrus_write_rate_limited' | 'orthrus_write_enabled' | 'orthrus_write_disabled'; // NEW ``` -This makes the assertions correct regardless of `Uptime.tsx`'s (intentional, unchanged) alphabetical sort order, and regardless of any future re-ordering of the mock array or additional monitors being added to the fixture. +### 3.6 Data Flow (end-to-end, happy path) + +1. Operator opens `AgentWriteModeDialog`, types the agent's name, clicks Enable → `PATCH /api/v1/orthrus/agents/:uuid { write_enabled: true }` → `OrthrusService.Patch` updates the DB row and emits an `orthrus_write_enabled` audit entry → response reflects `write_enabled: true`. +2. Dialog shows the reconnect notice (`configured_write_enabled: true`, `active_write_enabled: false` if the agent was already connected). +3. Agent reconnects (automatically, per its existing exponential-backoff loop in `leash.go:Run`, or the operator restarts it) → `HandleWebSocket` re-reads `agent.WriteEnabled` from the DB, sets `X-Orthrus-Write-Enabled: true` on the upgrade response, constructs `NewAgentSession(..., writeEnabled: true, ...)`. +4. Agent's `connect()` reads the header, builds `muzzle.New(true)`, uses it for all Docker streams on this connection. +5. Dockhand issues `GET /v1.44/containers//json` through the External Proxy port → passes both filters unconditionally (existing read allowlist, unaffected) → tool determines a new image digest is available (existing hotfix capability). +6. Dockhand issues `POST /images/create?fromImage=...` → backend `Muzzle` (writeEnabled=true, rate limiter has burst available) → allowed, forwarded → agent `Filter` (writeEnabled=true) → allowed, forwarded to real Docker socket → audit entry `orthrus_write_allowed`. +7. Dockhand issues `POST /containers//stop`, `DELETE /containers/`, `POST /containers/create` (body validated against the `HostConfig` allowlist — passes, since it's a same-config recreate with no dangerous fields), `POST /containers//start` — each independently allowed/audited as in step 6. +8. If any request in the sequence exceeds the rate limiter's burst (e.g., a runaway loop retrying rapidly), that request gets `429` and an `orthrus_write_rate_limited` audit entry instead of being forwarded. +9. If Dockhand (or an attacker with tunnel access) sends `POST /containers/create` with `HostConfig.Privileged: true`, the backend `Muzzle` rejects it with `403` and an `orthrus_write_blocked` audit entry **before it ever reaches the agent** — the agent-side filter would independently reject the same body if it were ever reached, per 3.3.4/3.3.5, but defense-in-depth means the backend is expected to be the first line here. +10. Operator reviews `/audit-logs?resource_uuid=&event_category=orthrus_write` (now correctly pre-filtered per 3.5.3) and sees the full sequence. + +### 3.7 Error Handling Summary + +| Condition | HTTP status | Response | Audit entry | +|---|---|---|---| +| Write request, `writeEnabled=false` | `403` | `"Forbidden"` (existing generic message, unchanged code path) | none (identical to today's behavior — a disabled agent produces zero new audit volume) | +| Write request, allowed by allowlist + body validation | pass-through | whatever the real Docker daemon returns | `orthrus_write_allowed` | +| `POST /containers/create`, disallowed `HostConfig` field | `403` | `"Forbidden: disallowed HostConfig field"` | `orthrus_write_blocked` | +| `POST /containers/create`, body exceeds size cap | `403` | `"Forbidden: request body too large"` | `orthrus_write_blocked` | +| Write request, rate limit exceeded | `429` | `"Too Many Requests"` | `orthrus_write_rate_limited` | +| `PATCH .../agents/:uuid` with `write_enabled` set, agent not found | `404` | existing `"agent not found"` (unchanged) | none | +| Malformed JSON body on `/containers/create` while write-enabled | `403` | `"Forbidden: malformed request body"` | `orthrus_write_blocked` | + +--- + +## 4. Explicit Risk Treatment (per design-brief requirement — not hand-waved) + +### 4.1 `HostConfig` body-inspection risk + +**Resolved** per Section 3.3.4: both muzzle layers become body-inspecting for exactly one endpoint (`POST /containers/create`), using a hybrid allowlist-of-safe-`HostConfig`-keys + one value-level check (`NetworkMode`) + a hard size cap. Feasibility confirmed directly from the current code: the agent-side filter already fully parses the HTTP request before forwarding (Section 2.3), and the backend-side filter, being a genuine `http.Handler`, can read-and-rebuffer `r.Body` with a standard `io.NopCloser(bytes.NewReader(...))` pattern before calling `m.next.ServeHTTP`. + +### 4.2 Six-endpoint allowlist sufficiency for a real update flow + +**Resolved, with one documented functional limitation.** The full sequence (Section 3.6, steps 5–7) — inspect (already-allowed read) → pull → stop → remove → create → start — is fully coverable by the six endpoints in the design brief. **Gap found**: `POST /containers/create`'s `NetworkingConfig` body field accepts only **one** network endpoint per Docker Engine API semantics; a container attached to **multiple** Docker networks at recreate time requires one additional `POST /networks/{id}/connect` call per extra network, which is **not** in the six-endpoint list and is deliberately **not proposed for addition in v1** — it is a network-mutation endpoint, and the design brief's boundary ("no volume/network create-or-delete") reads as intentionally conservative about network-adjacent write access even though `connect` is not technically a create-or-delete operation. **v1 accepts this as a documented limitation**: multi-network containers can be updated via this feature only if the operator or tool re-attaches secondary networks by some other means (e.g., manually, or Charon's own existing Docker management UI, which is unaffected by this feature and already has full write access to Charon-managed Docker hosts). This is called out in Section 7 (Out-of-Scope) and should be stated plainly in the user-facing docs rewrite (Phase 5) so operators aren't surprised. + +### 4.3 Rate-limiting / abuse potential + +**Resolved** per Section 3.3.6: existing Gin-based rate limiting (`cerberus.RateLimitMiddleware`) does not and structurally cannot cover the External Proxy's raw `http.Server` (confirmed architectural fact, Section 2.7, true today independent of this feature). A new, minimal, session-scoped `rate.Limiter` (reusing the same underlying package Cerberus already depends on) is added specifically for the write path, with a concrete default (0.5 req/s, burst 5) and explicit audit-trail visibility when triggered. + +### 4.4 Drift-class repeat risk (the two-independent-muzzles problem) + +**Resolved** per Section 3.3.5: this feature is specified as the first concrete implementation of the shared-test-corpus mitigation GH #1161 already recommends, rather than merely acknowledging the risk and repeating the historical pattern. GH #1160 and #1161 are confirmed open (`gh issue view 1160`/`1161`, both returned real content matching the design brief's description) and are cited by number in the corpus fixture's design (Section 3.3.5) as the origin of this requirement. + +--- + +## 5. Implementation Plan + +### Phase 1 — Playwright E2E Specs (write-only, `test.fixme`, no implementation yet) + +New spec file, path convention to be confirmed against the existing `e2e/` layout during implementation (not verified in this research pass — flagged for `playwright-dev`): + +- Agent write-mode toggle is off by default for a newly provisioned agent. +- Enabling requires typing the agent's name; Save is disabled until the typed value matches. +- Enabling succeeds, reconnect notice appears while the (test-double) agent session hasn't reconnected. +- Disabling requires no typed confirmation. +- Write-mode badge appears in the agent table when enabled. +- Navigating to the audit-log link from the write-mode dialog lands on a pre-filtered `/audit-logs` view. + +All `test.fixme` until Phase 3/4 land. + +### Phase 2 — Backend Foundation (no behavior change) + +- `models.OrthrusAgent.WriteEnabled` field + doc comment (Section 3.1). +- `AgentSession.writeEnabled` field, `NewAgentSession` signature change, `ExternalProxyStatus.WriteEnabled` field (Section 3.3.1/3.3.2) — plumbed through but not yet reachable (no caller sets it to `true` yet; `HandleWebSocket` still always constructs with the DB value, which defaults `false`, so this phase is a pure no-op for existing agents). +- `Muzzle.writeEnabled`/`writeLimiter` fields and constructor signature change; all existing call sites updated — **must not change any existing test's expected outcome**. +- `agent/muzzle.Filter` gains the same fields; `agent/leash` refactor to per-connection `Filter` construction (Section 3.4) — same "always constructed with `false` today" constraint. +- Validation gate: `go build ./... && go test ./...` green in both `backend/` and `agent/` modules, zero behavior change in existing test suites. + +### Phase 3 — Backend Write-Path Implementation + +- Write allowlist entries + `NetworkMode`/`HostConfig` body validator, identically in both muzzle files (Section 3.3.3/3.3.4). +- Shared test corpus fixture + both `TestMuzzle_SharedCorpus`/`TestFilter_SharedCorpus` (Section 3.3.5). +- Rate limiter wiring (Section 3.3.6). +- Audit logging: `AuditLogger` interface, `Muzzle` constructor gains it, wired at the `session.go`/`server.go` call site (Section 3.3.7). +- `X-Orthrus-Write-Enabled` handshake header, both sides (Section 3.3.1). +- `PATCH`/`GET proxy-status` handler + service changes (Section 3.2.1/3.2.2), including the new `OrthrusService` → `SecurityService` dependency wiring in `routes.go`. +- Validation gate: full backend unit test suite green, `./scripts/scan-gorm-security.sh --check` zero CRITICAL/HIGH (model change trigger per `CLAUDE.md` 1.5), `make lint-fast` clean, coverage ≥85%. + +### Phase 4 — Frontend Implementation + +- Type changes (Section 3.5.4). +- `AgentWriteModeDialog.tsx` (Section 3.5.2) + wiring into `OrthrusAgentManager.tsx`. +- `AuditLogs.tsx` deep-link support (Section 3.5.3). +- New i18n keys across all five locale files (`en`, `fr`, `de`, `es`, `zh` — matching the existing `hecate.externalProxy.*` convention's locale coverage). +- Validation gate: `npm run type-check`, `npm run build`, frontend unit tests green, coverage ≥85%. + +### Phase 5 — Hardening, E2E Enable, Docs + +- Un-`fixme` the Phase 1 Playwright specs; full `npx playwright test --project=firefox` run. +- `docs/features/orthrus.md` rewrite: correct all three absolute claims quoted in Section 2.8, describe the opt-in flow, the fixed operation list, the typed-confirmation requirement, and the multi-network limitation from Section 4.2. +- Full `CLAUDE.md` Definition-of-Done pass (Sections 1–10 of that document) before merge. + +--- + +## 6. Commit Slicing Strategy + +**Decision: single PR, one feature, ordered commits** — per `CLAUDE.md`'s "One Feature = One PR" / "Slice Commits, Not PRs" rule. No commit here is independently mergeable or independently useful; they exist to keep review tractable, not to be split across PRs. + +| # | Scope | Files (representative, not exhaustive) | Depends on | Validation gate | +|---|---|---|---|---| +| **1** | E2E specs, `test.fixme` | new file under `e2e/` | — | Playwright collects the file without error; all cases skipped, none fail | +| **2** | Backend foundation — model field, session/muzzle plumbing, zero behavior change | `models/orthrus_agent.go`, `orthrus/session.go`, `orthrus/muzzle.go`, `agent/muzzle/muzzle.go`, `agent/leash/leash.go` | 1 | `go build ./...` (both modules), `go test ./...` (both modules) green, no existing test's expected output changes | +| **3** | Backend write-path — allowlist, body validator, shared corpus, rate limiter, audit logging, handshake header | same files as #2 (deepened) + `testdata/muzzle_corpus.json`, `security_service.go` wiring | 2 | Backend unit tests green, `./scripts/scan-gorm-security.sh --check` clean, `make lint-fast` clean, coverage ≥85% | +| **4** | Backend API — `PATCH`/`GET proxy-status` extensions, `OrthrusService.Patch` signature + audit call, `routes.go` DI wiring | `orthrus_handler.go`, `orthrus_service.go`, `routes.go` | 3 | Handler/service unit tests green, `go build ./...` | +| **5** | Frontend — types, `AgentWriteModeDialog`, `OrthrusAgentManager` wiring, `AuditLogs` deep-link fix, i18n | `api/orthrus.ts`, `api/auditLogs.ts`, `components/hecate/AgentWriteModeDialog.tsx`, `components/hecate/OrthrusAgentManager.tsx`, `pages/AuditLogs.tsx`, `locales/*/translation.json` | 4 | `npm run type-check`, `npm run build`, Vitest suite green, coverage ≥85% | +| **6** | Hardening + E2E enable + docs | un-skip commit-1 spec file, `docs/features/orthrus.md` | 1–5 | Full `npx playwright test --project=firefox`, full `CLAUDE.md` DoD | + +**Rollback / contingency for the PR as a whole**: every commit up to and including #4 leaves default (`write_enabled=false`) behavior byte-for-byte identical to pre-feature behavior — the feature is inert until an operator explicitly flips the DB flag via the (not-yet-existing-until-commit-5) UI, or manually via the API. If a critical issue is found post-merge, the safest rollback is **not** a revert of the whole PR but an emergency DB-level or config-level disable (e.g., a temporary backend guard forcing `writeEnabled` to always evaluate `false` regardless of the DB value) — since the DB migration itself (additive column, default `false`) is safe to leave in place even if the feature is disabled. A full revert remains available as a fallback if the guard approach is judged insufficient. + +--- + +## 7. Explicit Out-of-Scope + +- Per-agent customization of *which* write operations are allowed beyond the fixed six-endpoint list — v1 is on/off only. +- `exec`, image `delete`, `build`, `prune`, `auth`, `commit`, any Swarm/service endpoint, under any configuration, ever, regardless of `WriteEnabled`. +- `POST /networks/{id}/connect` / multi-network container recreate support (Section 4.2) — documented v1 limitation, not silently ignored, candidate for a future v2 if real-world demand emerges. +- DB-backed or UI-configurable tuning of the write-path rate limiter's rate/burst constants — v1 ships fixed constants (Section 3.3.6). +- Rate limiting of *read* traffic through the External Docker Proxy — this predates the feature (Section 2.7) and is not addressed here; flagged as a separate potential follow-up, not bundled into this PR to keep scope tight. +- Unifying the two muzzle allowlists into a single shared-code implementation (GH #1161's broader ask) — this spec implements the *test-corpus* mitigation (Section 3.3.5) as the concrete, scoped contribution to that issue, but does not attempt the larger structural unification (e.g., extracting a shared Go module both `backend/` and `agent/` import), which GH #1161 itself notes may require restructuring given `agent/`'s minimal-standalone-binary constraint. +- **In scope for this feature**: CI *test execution* for the `agent/` module — Section 3.3.5 adds a `go test ./...` step to both `.github/workflows/orthrus-build.yml` and `.github/workflows/nightly-build.yml` (the two workflows that build/push the agent image), gating before the Docker build step. This is new CI infrastructure, added specifically because this feature's own safety argument (Section 3.3.5's shared corpus, Objective 3's defense-in-depth) depends on the `agent/` half of that corpus actually running automatically, not just locally. +- **Out of scope for this feature**: CI-enforced *lint/staticcheck/coverage* gates for the `agent/` module (GH #1161's second, broader ask) — no `golangci-lint`, no coverage threshold, no staticcheck step is added for `agent/` by this PR. Only test *execution* is added; test *quality* tooling remains a separate, unaddressed gap tracked by GH #1161. +- **Deferred, not blocking spec approval**: Section 3.3.5 specifies the concrete CI changes (`orthrus-build.yml`, `nightly-build.yml`) needed to actually run the `agent/` module's test suite — the shared anti-drift corpus, including the `Mounts[].VolumeOptions.DriverConfig` bypass case — but those changes are not yet implemented as of spec sign-off. This is a deliberate, tracked deferral (GH #1161), not an unresolved gap glossed over: it does not block this spec's approval, but it must land within this same PR/branch before final merge — see the binding note at Section 9, Acceptance Criterion #5. +- Any change to the unrelated, universal, unconditional read-only allowlist — untouched by this feature. +- Support for write mode through any transport other than the existing External Docker Proxy (`StartExternalProxy`) — e.g., no new direct-write API surface is added to Charon's own UI/API for agent-hosted containers; this feature only ungates the *tunnel* for third-party tools like Dockhand. + +--- + +## 8. Security Review Requirement + +This feature adds a **write capability to a previously strictly-read-only trust boundary** that spans an untrusted third-party tool (anything that can reach the External Proxy port on the operator's network) through Charon to a remote Docker daemon with root-equivalent host capability. Per this repo's own established practice (the same-day hotfix to this exact subsystem required `supervisor` review before implementation because it touched "the Docker API allowlist that gates what a tunnelled remote Docker socket can expose" — precedent cited directly from that plan's git history), this spec requires **mandatory `supervisor` review before any implementation begins**, with explicit sign-off on at minimum: + +1. The `HostConfig` allowlist in Section 3.3.4 — is the enumerated safe-key list actually exhaustive/correct against the real Docker Engine API `ContainerCreateConfig`/`HostConfig` schema (this spec's list was derived from the well-known dangerous-field set, not from a field-by-field audit of Docker's OpenAPI spec — the supervisor or implementation phase should cross-check against the actual schema for the Docker API version this codebase targets). +2. The `NetworkMode` value-level check (`"host"` and `"container:*"` denylist) — confirm no other `NetworkMode` value carries equivalent risk. +3. The decision to reject rather than strip malformed/dangerous bodies (Section 3.3.4) — confirm this doesn't create a usability trap that pushes operators toward workarounds that are less safe (e.g., disabling the validator, which this spec does not provide any mechanism to do — worth an explicit "there is no bypass" confirmation). +4. The rate-limit constants (Section 3.3.6) — confirm 0.5 req/s / burst 5 is defensible, not just plausible. +5. The audit-log `Actor` convention (`"orthrus-agent:"`) — confirm this doesn't collide with or get confused for a real username elsewhere in the audit log's `Actor` column, since that column is otherwise presumably populated with authenticated-user identifiers. +6. Whether the `services.SecurityService` dependency wiring proposed for `OrthrusService` (Section 3.2.1) introduces any import-cycle or initialization-order issue not caught by this research pass. + +`qa-security` involvement (per the standard agent roster) is expected during Phase 3/4 implementation for the mandatory CodeQL/Trivy/GORM scans already required by `CLAUDE.md`'s Definition of Done — called out here specifically because Section 1.5 of that DoD (`GORM Security Scan`) is unconditionally triggered by this feature's `backend/internal/models/**` change. + +--- -**Suggested commit message**: `test(uptime): assert Orthrus/TCP monitor cards by name, not render position` +## 9. Acceptance Criteria -### Commit Slicing addendum (Commits 6-7, following Commits 1-5 and the QA-report commit) +Feature is considered done when **all** of the following hold, without exception: -| # | Commit | Scope | Files | Depends on | Validation gate | -|---|---|---|---|---|---| -| **6** | `test(orthrus): fix agent-install wizard E2E flow to target /hecate/agent` | Issue 1 — rewrite `openOrthrusWizard()` to drive the real provisioning-dialog → install-wizard flow on `/hecate/agent` | `tests/orthrus-agent-install.spec.ts` | None (independent of Commits 1-5, 7, and the QA-report commit; test-only, no production code) | `npx playwright test --project=firefox tests/orthrus-agent-install.spec.ts` — all 18 tests pass | -| **7** | `test(uptime): assert Orthrus/TCP monitor cards by name, not render position` | Issue 2 — replace positional (`.first()`/`.nth(1)`) card/badge assertions with name-filtered locators in the one affected test | `tests/uptime-orthrus.spec.ts` | None (independent of Commit 6 and Commits 1-5; test-only, no production code) | `npx playwright test --project=firefox tests/uptime-orthrus.spec.ts` — all 4 tests pass | +1. A newly provisioned `OrthrusAgent` has `write_enabled: false` by default; a pre-existing agent row (present before this feature's migration runs) also reads `write_enabled: false` after migration, with no manual intervention. +2. With `write_enabled: false` (the default), 100% of existing Muzzle/Filter test behavior is byte-for-byte unchanged — every currently-passing test in `muzzle_test.go` and `agent/muzzle/muzzle_test.go` continues to pass with no modification to its expected outcome. +3. With `write_enabled: true` and an agent that has reconnected since the flag was set, all six allowlisted write operations succeed end-to-end against a real (or realistically mocked) Docker daemon, verified by at least one integration-level test exercising the full pull→stop→remove→create→start sequence. +4. A `POST /containers/create` body containing any of `Privileged: true`, non-empty `CapAdd`, non-empty `Binds`, `NetworkMode: "host"`, a `bind`-type `Mounts` entry, a `Mounts` entry with `VolumeOptions.DriverConfig` present (e.g. `{"type":"none","device":"/etc","o":"bind"}` inside `DriverConfig.Options` — the documented `local`-volume-driver bind-mount-via-volume bypass, Section 3.3.4 step 4), non-empty `Devices`, or non-empty `Sysctls` is rejected with `403` by **both** the backend and agent muzzle filters independently, even when `write_enabled: true`, each verified by its own test (not just the shared corpus, though the corpus should also cover it). +5. The shared test corpus (Section 3.3.5) exists, is loaded by both `backend/` and `agent/` test suites, and both suites agree on every case in it — enforced by CI via the new `go test ./...` step added to `.github/workflows/orthrus-build.yml`'s `build-and-push` job and `.github/workflows/nightly-build.yml`'s `build-and-push-nightly-orthrus` job (Section 3.3.5), both of which gate before the Docker image build step, not just local runs. (`backend/`'s half of the corpus was already covered by existing backend CI, per `quality-checks.yml`; the `agent/` half was not, until this feature's CI change.) -Both commits are test-only (`tests/*.spec.ts`), touch no `.go`/`.tsx`/`.ts` production source, trigger no GORM security scan (no `backend/internal/models/**` changes), and have zero bearing on the Orthrus security boundary (`muzzle.go`) — confirmed above. Rollback for either commit is a no-risk single-file revert; they do not depend on each other and can be reverted independently at any time. + > **Deferral note (user-approved, 2026-07-20):** CI enforcement of the `agent/` module's test suite — the shared anti-drift corpus described above, including the `Mounts[].VolumeOptions.DriverConfig` bypass test case — is accepted as a deliberate, tracked deferral, **not** resolved at spec-sign-off time. It is tracked under **GH #1161** ("unify the two hand-maintained allowlists / give `agent/` CI coverage"). This deferral does not block approval of this spec, but the CI workflow changes specified above (the `orthrus-build.yml` and `nightly-build.yml` steps) **must land within this same PR/branch, before final merge** — consistent with this repo's One-Feature-One-PR convention (`CLAUDE.md`, "Commit Slicing & PR Strategy") — not split into a separate PR, and not left as an indefinite someday-maybe. Acceptance Criterion #5 itself remains unmodified and must still be fully satisfied before merge; this note documents the timing/status of that satisfaction, not a reduction of the requirement. +6. Exceeding the write-path rate limiter produces `429` and an `orthrus_write_rate_limited` audit entry; the request is never forwarded to the agent. +7. Every allowed and every blocked write request produces exactly one `SecurityAudit` row with `event_category: "orthrus_write"`, queryable via `GET /api/v1/audit-logs?resource_uuid=&event_category=orthrus_write`. +8. The frontend `AgentWriteModeDialog` cannot be used to enable write mode without the operator typing the agent's exact current name; disabling requires no such confirmation. +9. `docs/features/orthrus.md` no longer contains the three absolute "cannot be changed" / "no way to turn it off" claims quoted in Section 2.8 in their current unqualified form; the rewritten text accurately describes the default (read-only, unconditional) and the opt-in (per-agent, audited, six-operation) states, and documents the multi-network limitation from Section 4.2. +10. Full `CLAUDE.md` Definition of Done (Sections 1–10) passes with zero errors: Playwright E2E, GORM security scan, patch coverage preflight, CodeQL/Trivy scans, lefthook, staticcheck, ≥85% coverage (backend and frontend), type-check, both builds, and cleanup. +11. `supervisor` sign-off obtained per Section 8 before implementation begins, and again before merge. From eb173d5c29c711fb62d1cd06942413f893db95b8 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 05:21:22 +0000 Subject: [PATCH 02/22] test(orthrus): add write-mode E2E specs as test.fixme Playwright specs for the write-mode toggle UX per the approved spec's Phase 1: default-off, typed-name confirmation gating enable (not disable), reconnect notice, WRITE badge, and the audit-log deep link. All test.fixme until the corresponding UI lands. --- tests/orthrus-write-mode.spec.ts | 251 +++++++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 tests/orthrus-write-mode.spec.ts diff --git a/tests/orthrus-write-mode.spec.ts b/tests/orthrus-write-mode.spec.ts new file mode 100644 index 000000000..0f58b91dd --- /dev/null +++ b/tests/orthrus-write-mode.spec.ts @@ -0,0 +1,251 @@ +import { test, expect } from './fixtures/test'; +import { waitForAPIHealth } from './utils/api-helpers'; +import { waitForLoadingComplete } from './utils/wait-helpers'; + +const ORTHRUS_AGENTS_API = '**/api/v1/orthrus/agents'; +const ORTHRUS_AGENT_API = '**/api/v1/orthrus/agents/*'; +const ORTHRUS_PROXY_STATUS_API = '**/api/v1/orthrus/agents/*/proxy-status'; +const REMOTE_SERVERS_API = '**/api/v1/remote-servers*'; +const HECATE_STATUS_API = '**/api/v1/hecate/status*'; + +const MOCK_AGENT_UUID = 'aaaabbbb-cccc-dddd-eeee-ffffffffffff'; +const MOCK_AGENT_NAME = 'write-mode-test-agent'; + +const MOCK_AGENT_BASE = { + uuid: MOCK_AGENT_UUID, + name: MOCK_AGENT_NAME, + status: 'online', + capabilities: '', + created_at: '2025-01-01T00:00:00Z', + updated_at: '2025-01-01T00:00:00Z', + external_proxy_port: 2375, + write_enabled: false, +}; + +const MOCK_AGENT_WRITE_ENABLED = { + ...MOCK_AGENT_BASE, + write_enabled: true, +}; + +const MOCK_PROXY_STATUS_WRITE_OFF = { + agent_uuid: MOCK_AGENT_UUID, + configured_port: 2375, + configured_write_enabled: false, + active_write_enabled: false, + agent_online: true, + active: true, + active_port: 2375, + bind_address: '0.0.0.0:2375', + connection_string: 'tcp://charon:2375', +}; + +const MOCK_PROXY_STATUS_WRITE_ON_PENDING_RECONNECT = { + ...MOCK_PROXY_STATUS_WRITE_OFF, + configured_write_enabled: true, + active_write_enabled: false, // agent hasn't reconnected since the flag was set +}; + +async function setupAgentPage( + page: import('@playwright/test').Page, + agents: object[], + proxyStatus: object | null = null, +) { + await page.route(REMOTE_SERVERS_API, (route) => route.fulfill({ json: [] })); + await page.route(HECATE_STATUS_API, (route) => route.fulfill({ json: [] })); + await page.route(ORTHRUS_AGENTS_API, (route) => { + if (route.request().method() === 'GET') { + route.fulfill({ json: agents }); + } else { + route.continue(); + } + }); + if (proxyStatus !== null) { + await page.route(ORTHRUS_PROXY_STATUS_API, (route) => route.fulfill({ json: proxyStatus })); + } + await page.goto('/hecate/agent'); + await waitForLoadingComplete(page); +} + +async function openWriteModeDialog(page: import('@playwright/test').Page, agentName: string) { + const writeModeButton = page.getByRole('button', { + name: new RegExp(`write.*mode.*${agentName}`, 'i'), + }); + await expect(writeModeButton).toBeVisible({ timeout: 8000 }); + await writeModeButton.click(); + const dialog = page.getByRole('dialog'); + await expect(dialog).toBeVisible({ timeout: 5000 }); + return dialog; +} + +test.describe('Orthrus Write Mode', () => { + test.beforeEach(async ({ request }) => { + await waitForAPIHealth(request); + }); + + test.fixme( + 'write mode is off by default for a newly provisioned agent', + async ({ page }) => { + await setupAgentPage(page, [MOCK_AGENT_BASE], MOCK_PROXY_STATUS_WRITE_OFF); + + const dialog = await openWriteModeDialog(page, MOCK_AGENT_NAME); + + await test.step('Toggle reflects write_enabled: false', async () => { + const toggle = dialog.getByRole('switch'); + await expect(toggle).not.toBeChecked(); + }); + + await test.step('No typed-confirmation input is shown while off', async () => { + const confirmInput = dialog.getByRole('textbox', { name: /type.*agent.*name/i }); + await expect(confirmInput).not.toBeVisible(); + }); + }, + ); + + test.fixme( + 'enabling write mode requires typing the exact agent name before Save is enabled', + async ({ page }) => { + await setupAgentPage(page, [MOCK_AGENT_BASE], MOCK_PROXY_STATUS_WRITE_OFF); + + const dialog = await openWriteModeDialog(page, MOCK_AGENT_NAME); + + await test.step('Flip toggle on and reveal typed-confirmation input', async () => { + const toggle = dialog.getByRole('switch'); + await toggle.click(); + await expect(dialog.getByRole('textbox', { name: /type.*agent.*name/i })).toBeVisible(); + }); + + await test.step('Save disabled with empty or mismatched confirmation text', async () => { + const confirmInput = dialog.getByRole('textbox', { name: /type.*agent.*name/i }); + const saveButton = dialog.getByRole('button', { name: /save|enable/i }); + + await expect(saveButton).toBeDisabled(); + + await confirmInput.fill('wrong-name'); + await expect(saveButton).toBeDisabled(); + + await confirmInput.fill(''); + await expect(saveButton).toBeDisabled(); + }); + + await test.step('Save enabled once typed value exactly matches agent name', async () => { + const confirmInput = dialog.getByRole('textbox', { name: /type.*agent.*name/i }); + const saveButton = dialog.getByRole('button', { name: /save|enable/i }); + + await confirmInput.fill(MOCK_AGENT_NAME); + await expect(saveButton).toBeEnabled(); + }); + }, + ); + + test.fixme( + 'enabling write mode succeeds and shows reconnect notice before agent reconnects', + async ({ page }) => { + let patchBody: Record | null = null; + + await setupAgentPage(page, [MOCK_AGENT_BASE], MOCK_PROXY_STATUS_WRITE_OFF); + await page.route(ORTHRUS_AGENT_API, async (route) => { + if (route.request().method() === 'PATCH') { + patchBody = (await route.request().postDataJSON()) as Record; + route.fulfill({ json: MOCK_AGENT_WRITE_ENABLED }); + } else { + route.continue(); + } + }); + + const dialog = await openWriteModeDialog(page, MOCK_AGENT_NAME); + + await test.step('Enable write mode with correct typed confirmation', async () => { + const toggle = dialog.getByRole('switch'); + await toggle.click(); + await dialog.getByRole('textbox', { name: /type.*agent.*name/i }).fill(MOCK_AGENT_NAME); + await dialog.getByRole('button', { name: /save|enable/i }).click(); + }); + + await test.step('PATCH sent write_enabled: true', async () => { + expect(patchBody).toBeTruthy(); + expect(patchBody?.write_enabled).toBe(true); + }); + + await test.step('Reconnect notice shown while active_write_enabled is still false', async () => { + await page.route(ORTHRUS_PROXY_STATUS_API, (route) => + route.fulfill({ json: MOCK_PROXY_STATUS_WRITE_ON_PENDING_RECONNECT }), + ); + await expect(dialog.getByText(/next agent reconnect/i)).toBeVisible({ timeout: 5000 }); + }); + }, + ); + + test.fixme('disabling write mode requires no typed confirmation', async ({ page }) => { + let patchBody: Record | null = null; + + await setupAgentPage(page, [MOCK_AGENT_WRITE_ENABLED], { + ...MOCK_PROXY_STATUS_WRITE_OFF, + configured_write_enabled: true, + active_write_enabled: true, + }); + await page.route(ORTHRUS_AGENT_API, async (route) => { + if (route.request().method() === 'PATCH') { + patchBody = (await route.request().postDataJSON()) as Record; + route.fulfill({ json: MOCK_AGENT_BASE }); + } else { + route.continue(); + } + }); + + const dialog = await openWriteModeDialog(page, MOCK_AGENT_NAME); + + await test.step('Flip toggle off and save without typing anything', async () => { + const toggle = dialog.getByRole('switch'); + await expect(toggle).toBeChecked(); + await toggle.click(); + + const confirmInput = dialog.getByRole('textbox', { name: /type.*agent.*name/i }); + await expect(confirmInput).not.toBeVisible(); + + await dialog.getByRole('button', { name: /save/i }).click(); + }); + + await test.step('PATCH sent write_enabled: false', async () => { + expect(patchBody?.write_enabled).toBe(false); + }); + }); + + test.fixme('WRITE badge appears in agent row when write_enabled is true', async ({ page }) => { + await setupAgentPage(page, [MOCK_AGENT_WRITE_ENABLED], null); + + await test.step('WRITE badge is visible in agent row', async () => { + await expect(page.getByText('WRITE', { exact: true })).toBeVisible({ timeout: 8000 }); + }); + }); + + test.fixme('WRITE badge absent when write_enabled is false', async ({ page }) => { + await setupAgentPage(page, [MOCK_AGENT_BASE], null); + + await test.step('WRITE badge is not visible', async () => { + await expect(page.getByText('WRITE', { exact: true })).not.toBeVisible({ timeout: 5000 }); + }); + }); + + test.fixme( + 'audit log link from write-mode dialog lands on a pre-filtered view', + async ({ page }) => { + await setupAgentPage(page, [MOCK_AGENT_WRITE_ENABLED], { + ...MOCK_PROXY_STATUS_WRITE_OFF, + configured_write_enabled: true, + active_write_enabled: true, + }); + + const dialog = await openWriteModeDialog(page, MOCK_AGENT_NAME); + + await test.step('Click the audit log link', async () => { + await dialog.getByRole('link', { name: /write.access.audit.log/i }).click(); + }); + + await test.step('Lands on /audit-logs with resource_uuid and event_category applied', async () => { + await expect(page).toHaveURL( + new RegExp(`/audit-logs\\?.*resource_uuid=${MOCK_AGENT_UUID}.*event_category=orthrus_write`), + ); + }); + }, + ); +}); From a4be39e29be0f41167a3123986669517adb3f2ff Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 05:30:24 +0000 Subject: [PATCH 03/22] feat(orthrus): add write-mode plumbing with zero behavior change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foundation commit for opt-in per-agent Docker write access (spec at docs/plans/current_spec.md): - models.OrthrusAgent gains WriteEnabled bool (default false). - AgentSession/NewAgentSession, ExternalProxyStatus, and Muzzle/NewMuzzle gain write-mode-related fields and parameters (writeEnabled, writeLimiter, auditLogger, agentUUID), but no write-endpoint allowlist branch exists yet — every call site passes false/nil, so behavior is byte-for-byte unchanged. - agent/muzzle.Filter becomes connection-scoped instead of process-scoped (New(writeEnabled bool)); agent/leash.Leash constructs a fresh Filter per successful dial instead of once per process, closed over by that connection's streams, so a mid-connection write-mode toggle can't retroactively affect an already-negotiated session. Full backend and agent test suites pass unmodified — this is pure plumbing, not yet reachable behavior. --- agent/leash/leash.go | 33 +++++++++---- agent/muzzle/muzzle.go | 22 +++++++-- agent/muzzle/muzzle_test.go | 20 ++++---- backend/internal/models/orthrus_agent.go | 7 +++ backend/internal/orthrus/muzzle.go | 46 +++++++++++++++++-- backend/internal/orthrus/muzzle_test.go | 22 ++++----- backend/internal/orthrus/server.go | 2 +- .../internal/orthrus/server_coverage_test.go | 12 ++--- .../orthrus/server_external_proxy_test.go | 2 +- backend/internal/orthrus/server_proxy_test.go | 2 +- backend/internal/orthrus/server_test.go | 6 +-- backend/internal/orthrus/session.go | 25 +++++++--- .../internal/orthrus/session_coverage_test.go | 4 +- .../orthrus/session_external_proxy_test.go | 6 +-- .../internal/orthrus/session_proxy_test.go | 14 +++--- backend/internal/orthrus/session_test.go | 8 ++-- 16 files changed, 160 insertions(+), 71 deletions(-) diff --git a/agent/leash/leash.go b/agent/leash/leash.go index f0b65fe5d..9ad342e96 100644 --- a/agent/leash/leash.go +++ b/agent/leash/leash.go @@ -46,13 +46,18 @@ type Config struct { } // Leash manages the reverse WebSocket tunnel to the Charon server. +// +// Leash intentionally holds no *muzzle.Filter field: the filter is +// connection-scoped, not process-scoped (see connect), so a mid-connection +// write-mode toggle on the operator's side can't retroactively change an +// already-negotiated session. Each call to connect constructs a fresh +// Filter and closes over it for the lifetime of that one connection. type Leash struct { serverURL string authKey string agentID string dockerSock string log *logrus.Logger - filter *muzzle.Filter initialDelay time.Duration } @@ -68,7 +73,6 @@ func New(cfg Config) *Leash { agentID: cfg.AgentID, dockerSock: cfg.DockerSock, log: cfg.Log, - filter: muzzle.New(), initialDelay: d, } } @@ -140,6 +144,18 @@ func (l *Leash) connect(ctx context.Context) error { l.log.Info("leash: connected, accepting proxy streams") + // filter is scoped to this one connection: constructed fresh per + // successful dial, closed over by every stream this connection accepts, + // and discarded on reconnect. This is what makes a write-mode toggle + // take effect only on the agent's next reconnect, matching the backend's + // per-AgentSession Muzzle scoping. + // + // TODO(Commit 3): negotiate writeEnabled from the X-Orthrus-Write-Enabled + // handshake response header (see leash_test.go for the discarded *http.Response + // this will read). Hardcoded false here preserves today's unconditional + // read-only behavior until that wiring lands. + filter := muzzle.New(false) + hbCtx, hbCancel := context.WithCancel(ctx) defer hbCancel() go newHeartbeat(session, l.log).Run(hbCtx) @@ -149,12 +165,12 @@ func (l *Leash) connect(ctx context.Context) error { if err != nil { return fmt.Errorf("leash: accept stream: %w", err) } - go l.handleStream(stream) + go l.handleStream(stream, filter) } } // handleStream reads the leading type byte from a yamux stream and dispatches accordingly. -func (l *Leash) handleStream(stream *yamux.Stream) { +func (l *Leash) handleStream(stream *yamux.Stream, filter *muzzle.Filter) { defer stream.Close() typeBuf := make([]byte, 1) @@ -165,7 +181,7 @@ func (l *Leash) handleStream(stream *yamux.Stream) { switch typeBuf[0] { case streamTypeDocker: - l.handleDockerStream(stream) + l.handleDockerStream(stream, filter) case streamTypePortForward: l.handlePortForward(stream) default: @@ -173,9 +189,10 @@ func (l *Leash) handleStream(stream *yamux.Stream) { } } -// handleDockerStream proxies the stream to the local Docker socket through the Muzzle filter. -func (l *Leash) handleDockerStream(stream *yamux.Stream) { - if err := l.filter.ServeProxy(l.dockerSock, stream, stream); err != nil { +// handleDockerStream proxies the stream to the local Docker socket through +// the connection-scoped Muzzle filter negotiated in connect. +func (l *Leash) handleDockerStream(stream *yamux.Stream, filter *muzzle.Filter) { + if err := filter.ServeProxy(l.dockerSock, stream, stream); err != nil { l.log.WithField("error", sanitizeLogField(err.Error())).Debug("leash: docker proxy stream closed") } } diff --git a/agent/muzzle/muzzle.go b/agent/muzzle/muzzle.go index 9929cc0cc..6bf62e06e 100644 --- a/agent/muzzle/muzzle.go +++ b/agent/muzzle/muzzle.go @@ -109,11 +109,25 @@ var imageDistributionPatterns = []struct { } // Filter is an HTTP allowlist filter for Docker socket proxy streams. -type Filter struct{} +// +// Filter is connection-scoped, not process-scoped: a fresh Filter is +// constructed for each successful agent connection (see leash.go's connect +// function), carrying the writeEnabled value negotiated for that specific +// session via the X-Orthrus-Write-Enabled handshake header. This mirrors the +// backend's per-AgentSession Muzzle scoping exactly, so a mid-connection DB +// toggle on the operator's side cannot retroactively change an +// already-negotiated session — the change only takes effect on the agent's +// next reconnect. +type Filter struct { + writeEnabled bool +} -// New returns a new Muzzle filter. -func New() *Filter { - return &Filter{} +// New returns a new Muzzle filter. writeEnabled governs whether the optional +// write-endpoint allowlist is consulted for this connection; false (the +// default for any caller not yet passing the negotiated handshake value) +// preserves today's unconditional read-only behavior. +func New(writeEnabled bool) *Filter { + return &Filter{writeEnabled: writeEnabled} } // Allow returns true if method+reqPath is on the allowlist. diff --git a/agent/muzzle/muzzle_test.go b/agent/muzzle/muzzle_test.go index c03ec71cb..dbf85f818 100644 --- a/agent/muzzle/muzzle_test.go +++ b/agent/muzzle/muzzle_test.go @@ -87,7 +87,7 @@ func startUnixHTTPServer(t *testing.T, handler func(net.Conn)) (string, func()) } func TestFilter_Allow(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) tests := []struct { method string @@ -199,7 +199,7 @@ func TestFilter_Allow(t *testing.T) { // like "nginx". Uses prefix/suffix matching instead of path.Match, whose // "*" does not cross "/". func TestFilter_Allow_NamespacedImagePaths(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) refs := []string{ "ghcr.io/org/repo", @@ -228,7 +228,7 @@ func TestFilter_Allow_NamespacedImagePaths(t *testing.T) { // match) still rejects writes against namespaced image/distribution paths // now that they pass the allowlist's path check. func TestFilter_Allow_NamespacedImagePaths_NonGETBlocked(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) paths := []string{ "/images/ghcr.io/org/repo/json", @@ -247,7 +247,7 @@ func TestFilter_Allow_NamespacedImagePaths_NonGETBlocked(t *testing.T) { } func TestFilter_ServeProxy_Blocked_POST(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) reqStr := "POST /v1.41/containers/create HTTP/1.1\r\nHost: localhost\r\nContent-Length: 0\r\n\r\n" var buf bytes.Buffer @@ -258,7 +258,7 @@ func TestFilter_ServeProxy_Blocked_POST(t *testing.T) { } func TestFilter_ServeProxy_Blocked_DELETE(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) reqStr := "DELETE /v1.41/containers/abc HTTP/1.1\r\nHost: localhost\r\n\r\n" var buf bytes.Buffer @@ -269,7 +269,7 @@ func TestFilter_ServeProxy_Blocked_DELETE(t *testing.T) { } func TestFilter_ServeProxy_Blocked_PUT(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) reqStr := "PUT /v1.41/networks/abc HTTP/1.1\r\nHost: localhost\r\nContent-Length: 0\r\n\r\n" var buf bytes.Buffer @@ -280,7 +280,7 @@ func TestFilter_ServeProxy_Blocked_PUT(t *testing.T) { } func TestFilter_ServeProxy_Blocked_UnversionedPost(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) reqStr := "POST /containers/create HTTP/1.1\r\nHost: localhost\r\nContent-Length: 0\r\n\r\n" var buf bytes.Buffer @@ -291,7 +291,7 @@ func TestFilter_ServeProxy_Blocked_UnversionedPost(t *testing.T) { } func TestServeProxy_ConnectionCloseSetOnRequest(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) reqSeen := make(chan *http.Request, 1) serverErr := make(chan error, 1) @@ -331,7 +331,7 @@ func TestServeProxy_ConnectionCloseSetOnRequest(t *testing.T) { } func TestServeProxy_CompletesAfterDockerResponse(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) serverErr := make(chan error, 1) body := `{"status":"ok"}` @@ -375,7 +375,7 @@ func TestServeProxy_CompletesAfterDockerResponse(t *testing.T) { } func TestServeProxy_StreamingResponseTerminatesOnWriterClose(t *testing.T) { - f := muzzle.New() + f := muzzle.New(false) serverWriteErr := make(chan error, 1) serverErr := make(chan error, 1) diff --git a/backend/internal/models/orthrus_agent.go b/backend/internal/models/orthrus_agent.go index 5154c5456..434511292 100644 --- a/backend/internal/models/orthrus_agent.go +++ b/backend/internal/models/orthrus_agent.go @@ -47,6 +47,13 @@ type OrthrusAgent struct { // 0 = disabled. Valid values: 1024–65535. ExternalProxyPort int `json:"external_proxy_port" gorm:"default:0"` + // WriteEnabled opts this agent into a narrow, fixed set of Docker write endpoints + // (image pull, container start/stop/restart/create/remove) in addition to the + // unconditional read-only allowlist. false = read-only (default). Enforced + // independently by both backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go. + // Takes effect on the agent's next reconnect (see AgentSession handshake). + WriteEnabled bool `json:"write_enabled" gorm:"default:false"` + LastHeartbeat *time.Time `json:"last_heartbeat,omitempty"` LastSeen *time.Time `json:"last_seen,omitempty"` CreatedAt time.Time `json:"created_at"` diff --git a/backend/internal/orthrus/muzzle.go b/backend/internal/orthrus/muzzle.go index 4d941f4ae..ec849be1c 100644 --- a/backend/internal/orthrus/muzzle.go +++ b/backend/internal/orthrus/muzzle.go @@ -7,9 +7,21 @@ import ( "strings" "github.com/Wikid82/charon/backend/internal/logger" + "github.com/Wikid82/charon/backend/internal/models" "github.com/Wikid82/charon/backend/internal/util" + "golang.org/x/time/rate" ) +// AuditLogger is the narrow interface Muzzle depends on for write-path audit +// logging. Satisfied by *services.SecurityService at the call site in +// session.go, where the concrete type is available. Muzzle cannot import +// services directly: services/orthrus_service.go already imports +// "github.com/Wikid82/charon/backend/internal/orthrus", so an orthrus → +// services import would create a cycle. +type AuditLogger interface { + LogAudit(a *models.SecurityAudit) error +} + // sanitizePath strips newlines and carriage returns from a path string to // prevent log injection (CWE-117). func sanitizePath(p string) string { @@ -92,14 +104,42 @@ var allowedDockerPrefixSuffixPatterns = []struct { } // Muzzle is an http.Handler wrapper that restricts Docker socket access -// to a curated allowlist of read-only, non-destructive endpoints. +// to a curated allowlist of read-only, non-destructive endpoints, plus an +// optional narrow set of write endpoints when writeEnabled is true. type Muzzle struct { next http.Handler + // writeEnabled is fixed at construction time (one Muzzle per AgentSession, + // per external-proxy start) — never re-read from the DB per-request. See + // NewMuzzle doc comment for why. + writeEnabled bool + // writeLimiter bounds write-request throughput; nil unless writeEnabled. + writeLimiter *rate.Limiter + // auditLogger records every write attempt (allowed or blocked); nil is + // tolerated (no-op) so tests and read-only sessions don't need one. + auditLogger AuditLogger + // agentUUID identifies the session this Muzzle guards, for audit entries. + agentUUID string } // NewMuzzle wraps handler with the Docker socket allowlist filter. -func NewMuzzle(next http.Handler) *Muzzle { - return &Muzzle{next: next} +// +// writeEnabled, writeLimiter, auditLogger, and agentUUID govern the optional +// write-endpoint allowlist (see Section 3.3.3 of the Orthrus write-mode +// spec). writeEnabled is captured once, at construction time, and never +// re-checked against the database on a per-request basis — StartExternalProxy +// constructs a new Muzzle per AgentSession using the value negotiated at +// connect time, so toggling the DB flag only takes effect on the agent's next +// reconnect. Re-reading it per-request would both reintroduce a TOCTOU-like +// inconsistency with that reconnect-to-apply guarantee and add a DB +// round-trip to the hot proxy path. +func NewMuzzle(next http.Handler, writeEnabled bool, writeLimiter *rate.Limiter, auditLogger AuditLogger, agentUUID string) *Muzzle { + return &Muzzle{ + next: next, + writeEnabled: writeEnabled, + writeLimiter: writeLimiter, + auditLogger: auditLogger, + agentUUID: agentUUID, + } } // ServeHTTP implements http.Handler. Only GET requests to allowlisted paths diff --git a/backend/internal/orthrus/muzzle_test.go b/backend/internal/orthrus/muzzle_test.go index e5c682e4c..dc032866c 100644 --- a/backend/internal/orthrus/muzzle_test.go +++ b/backend/internal/orthrus/muzzle_test.go @@ -27,7 +27,7 @@ func TestMuzzle_AllowlistedGET_Passthrough(t *testing.T) { "/system/df", } - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, path := range allowed { t.Run(path, func(t *testing.T) { @@ -52,7 +52,7 @@ func TestMuzzle_VersionPrefixStripped_Passthrough(t *testing.T) { "/v1.47/system/df", } - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, path := range paths { t.Run(path, func(t *testing.T) { @@ -65,7 +65,7 @@ func TestMuzzle_VersionPrefixStripped_Passthrough(t *testing.T) { } func TestMuzzle_POST_Blocked(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") paths := []string{ "/containers/create", @@ -84,7 +84,7 @@ func TestMuzzle_POST_Blocked(t *testing.T) { } func TestMuzzle_DELETE_Blocked(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") req := httptest.NewRequest(http.MethodDelete, "/containers/abc123", http.NoBody) rr := httptest.NewRecorder() m.ServeHTTP(rr, req) @@ -92,7 +92,7 @@ func TestMuzzle_DELETE_Blocked(t *testing.T) { } func TestMuzzle_HEAD_Ping_Passthrough(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, path := range []string{"/_ping", "/v1.44/_ping"} { t.Run(path, func(t *testing.T) { @@ -105,7 +105,7 @@ func TestMuzzle_HEAD_Ping_Passthrough(t *testing.T) { } func TestMuzzle_HEAD_NonPing_Blocked(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") req := httptest.NewRequest(http.MethodHead, "/containers/json", http.NoBody) rr := httptest.NewRecorder() m.ServeHTTP(rr, req) @@ -133,7 +133,7 @@ func TestMuzzle_DynamicPaths_Passthrough(t *testing.T) { "/v1.44/distribution/alpine/json", } - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, p := range paths { t.Run(p, func(t *testing.T) { @@ -159,7 +159,7 @@ func TestMuzzle_UnknownPath_Blocked(t *testing.T) { "/distribution/create", } - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, path := range paths { t.Run(path, func(t *testing.T) { @@ -185,7 +185,7 @@ func TestMuzzle_NamespacedImagePaths_Passthrough(t *testing.T) { "registry.example.com/team/project/image", } - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") for _, prefix := range []string{"/images/", "/distribution/"} { for _, ref := range refs { @@ -213,7 +213,7 @@ func TestMuzzle_NamespacedImagePaths_Passthrough(t *testing.T) { // ServeHTTP) still rejects writes against namespaced image paths now that // they pass the allowlist's path check. func TestMuzzle_NamespacedImagePaths_NonGET_Blocked(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") paths := []string{ "/images/ghcr.io/org/repo/json", @@ -235,7 +235,7 @@ func TestMuzzle_NamespacedImagePaths_NonGET_Blocked(t *testing.T) { // write path: POST to either is still rejected, even though method-checking // already happens unconditionally before any path match in ServeHTTP. func TestMuzzle_ImageAndDistributionEndpoints_POSTBlocked(t *testing.T) { - m := NewMuzzle(passthroughHandler()) + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") paths := []string{ "/images/alpine/json", diff --git a/backend/internal/orthrus/server.go b/backend/internal/orthrus/server.go index 0ac63599d..96bfd4c5d 100644 --- a/backend/internal/orthrus/server.go +++ b/backend/internal/orthrus/server.go @@ -84,7 +84,7 @@ func (s *OrthrusServer) HandleWebSocket(c *gin.Context) { return } - session, err := NewAgentSession(agent.UUID, agent.Name, conn) + session, err := NewAgentSession(agent.UUID, agent.Name, agent.WriteEnabled, conn) if err != nil { logger.Log().WithError(err).Error("orthrus: create agent session failed") _ = conn.Close() diff --git a/backend/internal/orthrus/server_coverage_test.go b/backend/internal/orthrus/server_coverage_test.go index 89f4697f2..2f5175343 100644 --- a/backend/internal/orthrus/server_coverage_test.go +++ b/backend/internal/orthrus/server_coverage_test.go @@ -25,7 +25,7 @@ func TestOrthrusServer_GetSession_KnownUUID(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("sess-uuid", "sess-agent", serverConn) + sess, err := NewAgentSession("sess-uuid", "sess-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -44,7 +44,7 @@ func TestOrthrusServer_GetProxyAddr_SessionExists_NoProxy(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("no-proxy-uuid", "agent", serverConn) + sess, err := NewAgentSession("no-proxy-uuid", "agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -63,7 +63,7 @@ func TestOrthrusServer_GetProxyAddr_SessionExists_WithProxy(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("with-proxy-uuid", "agent", serverConn) + sess, err := NewAgentSession("with-proxy-uuid", "agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -86,7 +86,7 @@ func TestOrthrusServer_DisconnectAgent_WithSession(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("disc-uuid", "disc-agent", serverConn) + sess, err := NewAgentSession("disc-uuid", "disc-agent", false, serverConn) require.NoError(t, err) srv.sessions.Store("disc-uuid", sess) @@ -194,7 +194,7 @@ func TestOrthrusServer_WatchHeartbeat_ClosedSession_ExitsAndMarksOffline(t *test serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("wh-uuid", "wh-agent", serverConn) + sess, err := NewAgentSession("wh-uuid", "wh-agent", false, serverConn) require.NoError(t, err) // Close session immediately so IsAlive() returns false on first tick. @@ -394,7 +394,7 @@ func TestHandleWebSocket_DisplacesExistingSession(t *testing.T) { // prior connection that has not yet been cleaned up. oldConn, oldCleanup := testWSPair(t) defer oldCleanup() - oldSess, err := NewAgentSession("displace-uuid", "displace-agent", oldConn) + oldSess, err := NewAgentSession("displace-uuid", "displace-agent", false, oldConn) require.NoError(t, err) srv.sessions.Store("displace-uuid", oldSess) diff --git a/backend/internal/orthrus/server_external_proxy_test.go b/backend/internal/orthrus/server_external_proxy_test.go index 401581352..fb2013f2d 100644 --- a/backend/internal/orthrus/server_external_proxy_test.go +++ b/backend/internal/orthrus/server_external_proxy_test.go @@ -81,7 +81,7 @@ func TestOrthrusServer_DisconnectAgent_ClosesExternalProxy(t *testing.T) { serverConn, wsCleanup := testWSPair(t) defer wsCleanup() - sess, err := NewAgentSession("ext-srv02-uuid", "ext-srv02-agent", serverConn) + sess, err := NewAgentSession("ext-srv02-uuid", "ext-srv02-agent", false, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) diff --git a/backend/internal/orthrus/server_proxy_test.go b/backend/internal/orthrus/server_proxy_test.go index 98e47a8b3..92a2e1f9a 100644 --- a/backend/internal/orthrus/server_proxy_test.go +++ b/backend/internal/orthrus/server_proxy_test.go @@ -71,7 +71,7 @@ func TestOrthrusServer_WatchHeartbeat_ClosesProxyListener(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("proxy-s2-uuid", "proxy-s2-agent", serverConn) + sess, err := NewAgentSession("proxy-s2-uuid", "proxy-s2-agent", false, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) diff --git a/backend/internal/orthrus/server_test.go b/backend/internal/orthrus/server_test.go index d6de28b5a..dd6355941 100644 --- a/backend/internal/orthrus/server_test.go +++ b/backend/internal/orthrus/server_test.go @@ -217,7 +217,7 @@ func TestWatchHeartbeat_StaleGoroutine_DoesNotEvictNewSession(t *testing.T) { // goroutine is still running after a newer session has replaced it. conn1, done1 := testWSPair(t) defer done1() - sess1, err := NewAgentSession(agentUUID, "race-agent", conn1) + sess1, err := NewAgentSession(agentUUID, "race-agent", false, conn1) require.NoError(t, err) require.NoError(t, sess1.Close()) require.False(t, sess1.IsAlive()) @@ -225,7 +225,7 @@ func TestWatchHeartbeat_StaleGoroutine_DoesNotEvictNewSession(t *testing.T) { // sess2: alive — represents the current (newer) reconnect stored in the map. conn2, done2 := testWSPair(t) defer done2() - sess2, err := NewAgentSession(agentUUID, "race-agent", conn2) + sess2, err := NewAgentSession(agentUUID, "race-agent", false, conn2) require.NoError(t, err) t.Cleanup(func() { _ = sess2.Close() }) srv.sessions.Store(agentUUID, sess2) @@ -277,7 +277,7 @@ func TestWatchHeartbeat_CurrentSession_MarksOfflineAndEvictsFromMap(t *testing.T conn, wsCleanup := testWSPair(t) defer wsCleanup() - sess, err := NewAgentSession(agentUUID, "current-agent", conn) + sess, err := NewAgentSession(agentUUID, "current-agent", false, conn) require.NoError(t, err) require.NoError(t, sess.Close()) require.False(t, sess.IsAlive()) diff --git a/backend/internal/orthrus/session.go b/backend/internal/orthrus/session.go index 183d1911a..da91faa85 100644 --- a/backend/internal/orthrus/session.go +++ b/backend/internal/orthrus/session.go @@ -101,6 +101,7 @@ type ExternalProxyStatus struct { ActivePort int `json:"active_port"` // actual bound port (0 if not active) BoundAddress string `json:"bind_address"` // e.g. "0.0.0.0:9999" Active bool `json:"active"` + WriteEnabled bool `json:"write_enabled"` // negotiated value for this live session Error string `json:"error,omitempty"` // last start error, if any } @@ -118,11 +119,16 @@ type AgentSession struct { extListener net.Listener // nil until StartExternalProxy succeeds extProxyPort int // port passed to StartExternalProxy; 0 if never started extErr error // last error from StartExternalProxy + // writeEnabled is the write-mode value negotiated at connect time (from + // OrthrusAgent.WriteEnabled at the moment HandleWebSocket ran). Fixed for + // the life of this session, exactly like extProxyPort — a DB toggle only + // takes effect on the agent's next reconnect. + writeEnabled bool mu sync.Mutex } // NewAgentSession wraps the WebSocket connection in a Yamux server session. -func NewAgentSession(agentUUID, agentName string, conn *websocket.Conn) (*AgentSession, error) { +func NewAgentSession(agentUUID, agentName string, writeEnabled bool, conn *websocket.Conn) (*AgentSession, error) { cfg := yamux.DefaultConfig() cfg.LogOutput = io.Discard @@ -134,11 +140,12 @@ func NewAgentSession(agentUUID, agentName string, conn *websocket.Conn) (*AgentS _, cancel := context.WithCancel(context.Background()) return &AgentSession{ - agentUUID: agentUUID, - agentName: agentName, - conn: conn, - session: session, - cancel: cancel, + agentUUID: agentUUID, + agentName: agentName, + conn: conn, + session: session, + cancel: cancel, + writeEnabled: writeEnabled, }, nil } @@ -302,8 +309,11 @@ func (s *AgentSession) StartExternalProxy(port int) error { Transport: baseTransport, } + // writeLimiter and auditLogger are wired in Commit 3 (rate limiting + + // audit logging); nil is a safe no-op here since the write-endpoint + // allowlist branch that would consult them does not exist yet. srv := &http.Server{ - Handler: NewMuzzle(rp), + Handler: NewMuzzle(rp, s.writeEnabled, nil, nil, s.agentUUID), ReadHeaderTimeout: 10 * time.Second, WriteTimeout: 0, } @@ -353,6 +363,7 @@ func (s *AgentSession) GetExternalProxyStatus() ExternalProxyStatus { ActivePort: activePort, BoundAddress: boundAddr, Active: active, + WriteEnabled: s.writeEnabled, Error: errStr, } } diff --git a/backend/internal/orthrus/session_coverage_test.go b/backend/internal/orthrus/session_coverage_test.go index 776aaad3c..86d69feaf 100644 --- a/backend/internal/orthrus/session_coverage_test.go +++ b/backend/internal/orthrus/session_coverage_test.go @@ -72,7 +72,7 @@ func TestAgentSession_GetProxyAddr_WithPort(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("port-uuid", "port-agent", serverConn) + sess, err := NewAgentSession("port-uuid", "port-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -105,7 +105,7 @@ func TestGetExternalProxyStatus_ErrorFieldPopulated(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext-err-uuid", "ext-err-agent", serverConn) + sess, err := NewAgentSession("ext-err-uuid", "ext-err-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() diff --git a/backend/internal/orthrus/session_external_proxy_test.go b/backend/internal/orthrus/session_external_proxy_test.go index 3f330fe6a..fba0b4dd2 100644 --- a/backend/internal/orthrus/session_external_proxy_test.go +++ b/backend/internal/orthrus/session_external_proxy_test.go @@ -38,7 +38,7 @@ func sessionWithLoopback(t *testing.T) (sess *AgentSession, clientYamux *yamux.S cy, err := yamux.Client(newWSNetConn(clientConn), cfg) require.NoError(t, err) - s, err := NewAgentSession("ext-test-uuid", "ext-test-agent", serverConn) + s, err := NewAgentSession("ext-test-uuid", "ext-test-agent", false, serverConn) require.NoError(t, err) require.NoError(t, s.StartDockerProxy()) @@ -89,7 +89,7 @@ func TestStartExternalProxy_ZeroPortIsNoOp(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext01-uuid", "ext01-agent", serverConn) + sess, err := NewAgentSession("ext01-uuid", "ext01-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -103,7 +103,7 @@ func TestStartExternalProxy_LoopbackNotStarted(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext02-uuid", "ext02-agent", serverConn) + sess, err := NewAgentSession("ext02-uuid", "ext02-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() diff --git a/backend/internal/orthrus/session_proxy_test.go b/backend/internal/orthrus/session_proxy_test.go index 1544746d1..0540007d0 100644 --- a/backend/internal/orthrus/session_proxy_test.go +++ b/backend/internal/orthrus/session_proxy_test.go @@ -49,7 +49,7 @@ func TestStartDockerProxy_SetsProxyAddr(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u1-uuid", "u1-agent", serverConn) + sess, err := NewAgentSession("u1-uuid", "u1-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -64,7 +64,7 @@ func TestStartDockerProxy_Idempotent(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u2-uuid", "u2-agent", serverConn) + sess, err := NewAgentSession("u2-uuid", "u2-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -84,7 +84,7 @@ func TestStartDockerProxy_AcceptsAndForwards(t *testing.T) { serverConn, clientConn, done := testWSPairBoth(t) defer done() - sess, err := NewAgentSession("u3-uuid", "u3-agent", serverConn) + sess, err := NewAgentSession("u3-uuid", "u3-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -139,7 +139,7 @@ func TestClose_StopsProxyListener(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u4-uuid", "u4-agent", serverConn) + sess, err := NewAgentSession("u4-uuid", "u4-agent", false, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) @@ -161,7 +161,7 @@ func TestStartDockerProxy_AfterClose(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u5-uuid", "u5-agent", serverConn) + sess, err := NewAgentSession("u5-uuid", "u5-agent", false, serverConn) require.NoError(t, err) require.NoError(t, sess.Close()) @@ -185,7 +185,7 @@ func TestAgentSession_runProxyListener_NonErrClosedError_Returns(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("rpl-uuid", "rpl-agent", serverConn) + sess, err := NewAgentSession("rpl-uuid", "rpl-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -208,7 +208,7 @@ func TestAgentSession_proxyConn_ClosedSession_ReturnsQuietly(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("pc-closed-uuid", "pc-agent", serverConn) + sess, err := NewAgentSession("pc-closed-uuid", "pc-agent", false, serverConn) require.NoError(t, err) // Close the yamux session so session.Open() returns an error. diff --git a/backend/internal/orthrus/session_test.go b/backend/internal/orthrus/session_test.go index 1e87bb458..5cb1065eb 100644 --- a/backend/internal/orthrus/session_test.go +++ b/backend/internal/orthrus/session_test.go @@ -50,7 +50,7 @@ func TestNewAgentSession_IsAlive(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -61,7 +61,7 @@ func TestAgentSession_GetProxyAddr_NoPort(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -72,7 +72,7 @@ func TestAgentSession_Close_SetsNotAlive(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) require.NoError(t, err) require.NoError(t, sess.Close()) @@ -83,7 +83,7 @@ func TestStartExternalProxy_TransportDisablesKeepAlives(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("keepalive-uuid", "keepalive-agent", serverConn) + sess, err := NewAgentSession("keepalive-uuid", "keepalive-agent", false, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() From 8311999e51af880b19cd0a5f9fd7b893ca6570f4 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 11:27:03 +0000 Subject: [PATCH 04/22] feat(orthrus): implement opt-in write-endpoint allowlist, body validation, rate limiting, and audit logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core write-path implementation for the Orthrus write-mode feature (docs/plans/current_spec.md), in both independently-enforcing muzzle filters: - backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go each gain a fixed write-endpoint allowlist (image pull, container start/stop/restart/create/remove), consulted only when a session negotiated write mode. Every other endpoint (exec, volume/network mutation, image delete, build, prune, auth, commit, Swarm/service) stays permanently blocked regardless of the flag. - POST /containers/create gets hybrid allowlist/value-level body validation: an exhaustive HostConfig key allowlist (fail-closed against unknown future Docker API fields), a value check on NetworkMode (rejects "host" and "container:*"), and a Mounts check that rejects bind-type mounts AND any Mounts entry carrying VolumeOptions.DriverConfig at all — closing the documented bypass where Docker's "local" volume driver can make a Type:"volume" mount behave as an arbitrary host bind mount. Identical logic in both files, duplicated by design (agent/ is a separate minimal-binary module), verified identical via a new shared JSON test corpus (backend/internal/orthrus/testdata/muzzle_corpus.json) loaded by both packages' test suites — the concrete first implementation of the anti-drift mitigation GH #1161 already recommended. - Write traffic gets its own session-scoped rate limiter (0.5 req/s, burst 5 — one full pull/stop/remove/create/start cycle), since the External Proxy's raw http.Server has never been covered by Charon's existing Gin-based rate limiting. - Every write attempt (allowed, blocked, or rate-limited) produces one SecurityAudit entry via a new narrow AuditLogger interface (avoids an orthrus->services import cycle). - X-Orthrus-Write-Enabled handshake header negotiates write mode atomically at WebSocket upgrade time; the agent's Filter becomes connection-scoped so a DB toggle only takes effect on next reconnect. - CI: orthrus-build.yml and nightly-build.yml now run the agent module's own test suite (previously zero test execution in CI for agent/, confirmed via direct workflow grep) before the Docker build step, gating both the PR-facing and the nightly force-push-based publish paths. Full backend and agent test suites green, including the dangerous-body corpus cases (Privileged, CapAdd, Binds, NetworkMode:host, bind-type Mounts, the VolumeOptions.DriverConfig bypass, Devices, Sysctls, unrecognized future fields) — all independently rejected by both filters. --- .github/workflows/nightly-build.yml | 19 + .github/workflows/orthrus-build.yml | 20 ++ agent/leash/leash.go | 26 +- agent/muzzle/muzzle.go | 234 +++++++++++- agent/muzzle/muzzle_test.go | 182 +++++++++- backend/internal/orthrus/muzzle.go | 333 ++++++++++++++++++ backend/internal/orthrus/muzzle_test.go | 298 ++++++++++++++++ backend/internal/orthrus/server.go | 32 +- .../internal/orthrus/server_coverage_test.go | 12 +- .../orthrus/server_external_proxy_test.go | 2 +- backend/internal/orthrus/server_proxy_test.go | 2 +- backend/internal/orthrus/server_test.go | 6 +- backend/internal/orthrus/session.go | 37 +- .../internal/orthrus/session_coverage_test.go | 4 +- .../orthrus/session_external_proxy_test.go | 6 +- .../internal/orthrus/session_proxy_test.go | 14 +- backend/internal/orthrus/session_test.go | 8 +- .../orthrus/testdata/muzzle_corpus.json | 263 ++++++++++++++ 18 files changed, 1441 insertions(+), 57 deletions(-) create mode 100644 backend/internal/orthrus/testdata/muzzle_corpus.json diff --git a/.github/workflows/nightly-build.yml b/.github/workflows/nightly-build.yml index 0cd0382d8..47c2848b5 100644 --- a/.github/workflows/nightly-build.yml +++ b/.github/workflows/nightly-build.yml @@ -428,6 +428,25 @@ jobs: - name: Set lowercase image name run: echo "ORTHRUS_IMAGE_NAME_LC=${ORTHRUS_IMAGE_NAME,,}" >> "$GITHUB_ENV" + # Gates this nightly image publish on the agent module's own test + # suite, mirroring orthrus-build.yml's PR-facing gate — needed here + # too because sync-development-to-nightly performs a git reset --hard + # / force-push path that does not itself go through a reviewed PR + # against nightly, so gating only the PR-facing workflow would leave + # this production-image-publishing path untested. See Section 3.3.5 of + # docs/plans/current_spec.md and GH #1161. + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: agent/go.mod + cache-dependency-path: agent/go.sum + + - name: Run Orthrus agent tests + run: | + set -euo pipefail + cd agent + go test ./... + - name: Set up QEMU uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0 diff --git a/.github/workflows/orthrus-build.yml b/.github/workflows/orthrus-build.yml index 4e45c8574..cc3a38249 100644 --- a/.github/workflows/orthrus-build.yml +++ b/.github/workflows/orthrus-build.yml @@ -98,6 +98,26 @@ jobs: fi fi + # Gates the Docker build on the agent module's own test suite, + # including the shared anti-drift corpus with backend/internal/orthrus + # (testdata/muzzle_corpus.json) — see Section 3.3.5 of + # docs/plans/current_spec.md and GH #1161. Prior to this step, agent/ + # had zero CI-enforced test execution: every workflow touching it went + # straight from checkout to docker/build-push-action, only building the + # module's source as an opaque COPY . . step inside agent/Dockerfile's + # multi-stage build. + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: agent/go.mod + cache-dependency-path: agent/go.sum + + - name: Run Orthrus agent tests + run: | + set -euo pipefail + cd agent + go test ./... + - name: Set up QEMU uses: docker/setup-qemu-action@96fe6ef7f33517b61c61be40b68a1882f3264fb8 # v4.2.0 diff --git a/agent/leash/leash.go b/agent/leash/leash.go index 9ad342e96..e767a1452 100644 --- a/agent/leash/leash.go +++ b/agent/leash/leash.go @@ -123,7 +123,7 @@ func (l *Leash) connect(ctx context.Context) error { l.log.WithField("server_url", l.serverURL).Info("leash: connecting to server") - wsConn, _, err := dialer.DialContext(ctx, l.serverURL, http.Header{ + wsConn, resp, err := dialer.DialContext(ctx, l.serverURL, http.Header{ "Authorization": {"Bearer " + l.authKey}, "X-Orthrus-Version": {"1.0.0"}, "X-Orthrus-Name": {l.agentID}, @@ -132,6 +132,14 @@ func (l *Leash) connect(ctx context.Context) error { return fmt.Errorf("leash: websocket dial: %w", err) } + // X-Orthrus-Write-Enabled is delivered atomically in the 101 Switching + // Protocols response that completes the handshake (server.go sets it via + // wsUpgrader.Upgrade's responseHeader parameter). A missing header or + // any value other than the literal string "true" is treated as false — + // fail closed, matching this package's existing "unknown stream type, + // closing" posture elsewhere in this file. + writeEnabled := resp != nil && resp.Header.Get("X-Orthrus-Write-Enabled") == "true" + cfg := yamux.DefaultConfig() cfg.LogOutput = io.Discard @@ -145,16 +153,12 @@ func (l *Leash) connect(ctx context.Context) error { l.log.Info("leash: connected, accepting proxy streams") // filter is scoped to this one connection: constructed fresh per - // successful dial, closed over by every stream this connection accepts, - // and discarded on reconnect. This is what makes a write-mode toggle - // take effect only on the agent's next reconnect, matching the backend's - // per-AgentSession Muzzle scoping. - // - // TODO(Commit 3): negotiate writeEnabled from the X-Orthrus-Write-Enabled - // handshake response header (see leash_test.go for the discarded *http.Response - // this will read). Hardcoded false here preserves today's unconditional - // read-only behavior until that wiring lands. - filter := muzzle.New(false) + // successful dial with the writeEnabled value negotiated above, closed + // over by every stream this connection accepts, and discarded on + // reconnect. This is what makes a write-mode toggle take effect only on + // the agent's next reconnect, matching the backend's per-AgentSession + // Muzzle scoping. + filter := muzzle.New(writeEnabled) hbCtx, hbCancel := context.WithCancel(ctx) defer hbCancel() diff --git a/agent/muzzle/muzzle.go b/agent/muzzle/muzzle.go index 6bf62e06e..fef9077a9 100644 --- a/agent/muzzle/muzzle.go +++ b/agent/muzzle/muzzle.go @@ -7,6 +7,8 @@ package muzzle import ( "bufio" + "bytes" + "encoding/json" "fmt" "io" "net" @@ -108,6 +110,156 @@ var imageDistributionPatterns = []struct { {prefix: "/distribution/", suffix: "/json"}, // registry digest check — read-only } +// allowedWriteExactPaths lists the fixed-path write endpoints permitted +// when a connection has negotiated write mode (Filter.writeEnabled), keyed +// on the version-stripped path. /containers/create is handled as its own +// special case in allowWrite (body-validated); /images/create takes its +// parameters via query string and has no body to validate. This MUST stay +// in exact policy agreement with allowedWriteExactPaths/allowedWritePatterns +// in backend/internal/orthrus/muzzle.go — the shared test corpus +// (backend/internal/orthrus/testdata/muzzle_corpus.json) exists specifically +// to catch drift between the two independently-maintained copies. +var allowedWriteExactPaths = map[string]struct{}{ + "/containers/create": {}, + "/images/create": {}, +} + +// allowedWritePatterns lists the dynamic-segment write endpoints permitted +// when write mode is on, both unversioned and versioned forms, matching +// this file's existing convention for the read-only allowlist above. +var allowedWritePatterns = []struct { + method string + pattern string +}{ + {http.MethodPost, "/containers/*/start"}, + {http.MethodPost, "/v*/containers/*/start"}, + {http.MethodPost, "/containers/*/stop"}, + {http.MethodPost, "/v*/containers/*/stop"}, + {http.MethodPost, "/containers/*/restart"}, + {http.MethodPost, "/v*/containers/*/restart"}, + {http.MethodDelete, "/containers/*"}, + {http.MethodDelete, "/v*/containers/*"}, +} + +// maxContainerCreateBodyBytes bounds the size of a /containers/create body +// accepted for validation. Must stay numerically identical to the backend's +// copy (backend/internal/orthrus/muzzle.go) — the shared test corpus +// includes a boundary-size case to catch drift. +const maxContainerCreateBodyBytes = 64 * 1024 + +// hostConfigAllowedKeys, mountEntry, and the two value-level validators +// below are an exact policy mirror of backend/internal/orthrus/muzzle.go's +// copies. See that file's doc comments for the full rationale (allowlist +// over denylist for fail-closed behavior against unknown future fields; the +// VolumeOptions.DriverConfig check closing the local-driver +// bind-mount-via-volume bypass). Duplicated here, not shared via import, +// because agent/ is a separate Go module built as a minimal standalone +// binary and does not import backend/ packages. +var hostConfigAllowedKeys = map[string]struct{}{ + "PortBindings": {}, + "RestartPolicy": {}, + "Memory": {}, + "MemorySwap": {}, + "NanoCpus": {}, + "CpuShares": {}, + "Mounts": {}, + "Dns": {}, + "DnsSearch": {}, + "ExtraHosts": {}, + "LogConfig": {}, + "AutoRemove": {}, + "ReadonlyRootfs": {}, + "Init": {}, + "NetworkMode": {}, +} + +type mountEntry struct { + Type string `json:"Type"` + VolumeOptions json.RawMessage `json:"VolumeOptions"` +} + +func validateNetworkModeValue(raw json.RawMessage) bool { + var mode string + if err := json.Unmarshal(raw, &mode); err != nil { + return false + } + if mode == "host" { + return false + } + if strings.HasPrefix(mode, "container:") { + return false + } + return true +} + +func validateMountsValue(raw json.RawMessage) bool { + var mounts []mountEntry + if err := json.Unmarshal(raw, &mounts); err != nil { + return false + } + for _, mnt := range mounts { + if mnt.Type != "volume" && mnt.Type != "tmpfs" { + return false + } + if len(mnt.VolumeOptions) == 0 { + continue + } + var volumeOptions map[string]json.RawMessage + if err := json.Unmarshal(mnt.VolumeOptions, &volumeOptions); err != nil { + return false + } + if _, hasDriverConfig := volumeOptions["DriverConfig"]; hasDriverConfig { + return false + } + } + return true +} + +// validateContainerCreateBody validates a /containers/create request body +// against hostConfigAllowedKeys plus the NetworkMode/Mounts value-level +// checks. Unlike the backend's copy, this function does not re-buffer +// req.Body itself — ServeProxy already reads the full body into bodyBytes +// once, up front, for every request (see the doc comment there), so there +// is nothing left to re-buffer by the time this function runs. +func validateContainerCreateBody(bodyBytes []byte) (ok bool, reason string) { + if len(bodyBytes) == 0 { + return true, "" + } + + var top map[string]json.RawMessage + if err := json.Unmarshal(bodyBytes, &top); err != nil { + return false, "malformed request body" + } + + hostConfigRaw, hasHostConfig := top["HostConfig"] + if !hasHostConfig { + return true, "" + } + + var hostConfig map[string]json.RawMessage + if err := json.Unmarshal(hostConfigRaw, &hostConfig); err != nil { + return false, "malformed request body" + } + + for key, rawValue := range hostConfig { + if _, allowed := hostConfigAllowedKeys[key]; !allowed { + return false, "disallowed HostConfig field: " + key + } + switch key { + case "NetworkMode": + if !validateNetworkModeValue(rawValue) { + return false, "disallowed HostConfig field: NetworkMode" + } + case "Mounts": + if !validateMountsValue(rawValue) { + return false, "disallowed HostConfig field: Mounts" + } + } + } + + return true, "" +} + // Filter is an HTTP allowlist filter for Docker socket proxy streams. // // Filter is connection-scoped, not process-scoped: a fresh Filter is @@ -130,10 +282,16 @@ func New(writeEnabled bool) *Filter { return &Filter{writeEnabled: writeEnabled} } -// Allow returns true if method+reqPath is on the allowlist. -// Only GET is permitted, except HEAD which is allowed on /_ping and /v*/_ping -// (Docker SDK connectivity check). -func (f *Filter) Allow(method, reqPath string) bool { +// Allow returns true if method+reqPath (+body, for the one body-validated +// write endpoint) is on the allowlist. Only GET is permitted for read +// traffic, except HEAD which is allowed on /_ping and /v*/_ping (Docker SDK +// connectivity check); POST/DELETE are additionally permitted for the fixed +// write-endpoint set when f.writeEnabled is true. +// +// body is only consulted for the one body-validated case (POST to +// /containers/create or /v*/containers/create); every other allowlist +// branch ignores it entirely, so passing nil for read-only calls is safe. +func (f *Filter) Allow(method, reqPath string, body []byte) bool { // HEAD is permitted only for /_ping (Docker SDK connectivity check). if strings.EqualFold(method, http.MethodHead) { cleanPath := path.Clean(reqPath) @@ -145,13 +303,21 @@ func (f *Filter) Allow(method, reqPath string) bool { return false } + cleanPath := path.Clean(reqPath) + + if f.writeEnabled && (strings.EqualFold(method, http.MethodPost) || strings.EqualFold(method, http.MethodDelete)) { + if f.allowWrite(method, cleanPath, body) { + return true + } + // Not on the write allowlist even with write mode on — fall through + // to the same false every other disallowed request gets below. + } + if !strings.EqualFold(method, http.MethodGet) { return false } // path.Clean normalises redundant separators and removes trailing slashes. - cleanPath := path.Clean(reqPath) - for _, pattern := range allowedPatterns { matched, err := path.Match(pattern, cleanPath) if err == nil && matched { @@ -172,9 +338,46 @@ func (f *Filter) Allow(method, reqPath string) bool { return false } +// allowWrite checks method+cleanPath against allowedWriteExactPaths and +// allowedWritePatterns, applying validateContainerCreateBody specifically +// for /containers/create. Only called when f.writeEnabled is true. +func (f *Filter) allowWrite(method, cleanPath string, body []byte) bool { + if strings.EqualFold(method, http.MethodPost) { + unversioned := versionPrefixRe.ReplaceAllString(cleanPath, "") + if _, ok := allowedWriteExactPaths[unversioned]; ok { + if unversioned == "/containers/create" { + valid, _ := validateContainerCreateBody(body) + return valid + } + return true + } + } + + for _, p := range allowedWritePatterns { + if !strings.EqualFold(method, p.method) { + continue + } + if matched, err := path.Match(p.pattern, cleanPath); err == nil && matched { + return true + } + } + return false +} + // ServeProxy reads the HTTP request from r, checks Allow(), then dials the Docker // unix socket at dst and proxies the full HTTP transaction. If the request is // blocked, a 403 response is written to w and an error is returned. +// +// Every request's body is read and re-buffered unconditionally, not only +// for /containers/create: reading req.Body to obtain bytes for Allow +// consumes the underlying io.ReadCloser exactly once, and req.Write(conn) +// below would forward an empty/already-drained body unless the bytes are +// wrapped back into a fresh reader first. GET requests present req.Body as +// nil or an already-empty, immediately-io.EOF reader (standard net/http +// behavior for bodyless requests), so this is a cheap no-op for the +// overwhelming majority of traffic — keeping this step unconditional avoids +// duplicating the "is this /containers/create" check that already has to +// live inside Allow/allowWrite. func (f *Filter) ServeProxy(dst string, r io.Reader, w io.Writer) error { bufr := bufio.NewReader(r) @@ -183,7 +386,24 @@ func (f *Filter) ServeProxy(dst string, r io.Reader, w io.Writer) error { return fmt.Errorf("muzzle: read request: %w", err) } - if !f.Allow(req.Method, req.URL.Path) { + var bodyBytes []byte + if req.Body != nil { + limited := io.LimitReader(req.Body, maxContainerCreateBodyBytes+1) + bodyBytes, err = io.ReadAll(limited) + _ = req.Body.Close() + if err != nil { + return fmt.Errorf("muzzle: read body: %w", err) + } + if len(bodyBytes) > maxContainerCreateBodyBytes { + _, _ = io.WriteString(w, forbiddenResponse) + return fmt.Errorf("muzzle: blocked %s %s: body too large", req.Method, req.URL.Path) + } + // Re-buffer: req.Write below must still forward the original body intact. + req.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + req.ContentLength = int64(len(bodyBytes)) + } + + if !f.Allow(req.Method, req.URL.Path, bodyBytes) { // Fail closed: write 403 and abort the stream. _, _ = io.WriteString(w, forbiddenResponse) return fmt.Errorf("muzzle: blocked %s %s", req.Method, req.URL.Path) diff --git a/agent/muzzle/muzzle_test.go b/agent/muzzle/muzzle_test.go index dbf85f818..72ad8b88a 100644 --- a/agent/muzzle/muzzle_test.go +++ b/agent/muzzle/muzzle_test.go @@ -3,11 +3,13 @@ package muzzle_test import ( "bufio" "bytes" + "encoding/json" "errors" "fmt" "io" "net" "net/http" + "os" "path/filepath" "strings" "sync" @@ -184,7 +186,7 @@ func TestFilter_Allow(t *testing.T) { for _, tt := range tests { t.Run(tt.method+" "+tt.reqPath, func(t *testing.T) { - got := f.Allow(tt.method, tt.reqPath) + got := f.Allow(tt.method, tt.reqPath, nil) assert.Equal(t, tt.allowed, got) }) } @@ -212,12 +214,12 @@ func TestFilter_Allow_NamespacedImagePaths(t *testing.T) { for _, ref := range refs { p := prefix + ref + "/json" t.Run(p, func(t *testing.T) { - assert.True(t, f.Allow("GET", p)) + assert.True(t, f.Allow("GET", p, nil)) }) vp := "/v1.44" + prefix + ref + "/json" t.Run(vp, func(t *testing.T) { - assert.True(t, f.Allow("GET", vp)) + assert.True(t, f.Allow("GET", vp, nil)) }) } } @@ -240,7 +242,7 @@ func TestFilter_Allow_NamespacedImagePaths_NonGETBlocked(t *testing.T) { for _, p := range paths { for _, method := range []string{"POST", "PUT", "DELETE", "PATCH"} { t.Run(method+" "+p, func(t *testing.T) { - assert.False(t, f.Allow(method, p)) + assert.False(t, f.Allow(method, p, nil)) }) } } @@ -441,3 +443,175 @@ func TestServeProxy_StreamingResponseTerminatesOnWriterClose(t *testing.T) { default: } } + +// --- Write-mode tests (Section 3.3.4/3.4.1 of the Orthrus write-mode spec) --- +// +// These mirror backend/internal/orthrus/muzzle_test.go's write-mode tests +// exactly in intent (though this package's Allow(method, path, body) is +// called directly rather than through the httptest-based ServeHTTP the +// backend uses, since Filter has no http.Handler interface of its own). + +func TestFilter_Allow_WriteEndpoints_BlockedWhenWriteDisabled(t *testing.T) { + f := muzzle.New(false) + + cases := []struct { + method string + path string + }{ + {"POST", "/containers/create"}, + {"POST", "/images/create"}, + {"POST", "/containers/abc123/start"}, + {"POST", "/containers/abc123/stop"}, + {"POST", "/containers/abc123/restart"}, + {"DELETE", "/containers/abc123"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + assert.False(t, f.Allow(tc.method, tc.path, nil)) + }) + } +} + +func TestFilter_Allow_WriteEndpoints_AllowedWhenWriteEnabled(t *testing.T) { + f := muzzle.New(true) + + cases := []struct { + method string + path string + }{ + {"POST", "/images/create"}, + {"POST", "/containers/abc123/start"}, + {"POST", "/containers/abc123/stop"}, + {"POST", "/containers/abc123/restart"}, + {"DELETE", "/containers/abc123"}, + {"POST", "/v1.44/containers/abc123/start"}, + {"DELETE", "/v1.44/containers/abc123"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + assert.True(t, f.Allow(tc.method, tc.path, nil)) + }) + } +} + +func TestFilter_Allow_WriteMode_NonWriteEndpointsStillBlocked(t *testing.T) { + f := muzzle.New(true) + + cases := []struct { + method string + path string + }{ + {"POST", "/containers/abc123/exec"}, + {"POST", "/exec/xyz/start"}, + {"DELETE", "/images/nginx"}, + {"POST", "/build"}, + {"POST", "/system/prune"}, + {"POST", "/auth"}, + {"POST", "/commit"}, + {"POST", "/networks/create"}, + {"DELETE", "/networks/mynet"}, + {"POST", "/volumes/create"}, + {"DELETE", "/volumes/myvol"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + assert.False(t, f.Allow(tc.method, tc.path, nil)) + }) + } +} + +func TestFilter_Allow_ContainersCreate_SafeBodyAllowed(t *testing.T) { + f := muzzle.New(true) + + body := []byte(`{"Image":"nginx:latest","HostConfig":{"PortBindings":{"80/tcp":[{"HostPort":"8080"}]},"RestartPolicy":{"Name":"unless-stopped"},"NetworkMode":"my-bridge"}}`) + assert.True(t, f.Allow("POST", "/containers/create", body)) + assert.True(t, f.Allow("POST", "/v1.44/containers/create", body)) +} + +// TestFilter_Allow_ContainersCreate_DangerousBodiesRejected mirrors +// backend/internal/orthrus/muzzle_test.go's TestMuzzle_ContainersCreate_DangerousBodiesRejected +// case-for-case, including the local-driver bind-mount-via-volume bypass +// (VolumeOptions.DriverConfig) that both filters must independently reject. +func TestFilter_Allow_ContainersCreate_DangerousBodiesRejected(t *testing.T) { + f := muzzle.New(true) + + cases := []struct { + name string + body string + }{ + {"Privileged", `{"Image":"nginx","HostConfig":{"Privileged":true}}`}, + {"CapAdd", `{"Image":"nginx","HostConfig":{"CapAdd":["SYS_ADMIN"]}}`}, + {"legacy Binds", `{"Image":"nginx","HostConfig":{"Binds":["/:/host"]}}`}, + {"NetworkMode host", `{"Image":"nginx","HostConfig":{"NetworkMode":"host"}}`}, + {"NetworkMode container:*", `{"Image":"nginx","HostConfig":{"NetworkMode":"container:abc"}}`}, + {"bind-type Mounts", `{"Image":"nginx","HostConfig":{"Mounts":[{"Type":"bind","Source":"/etc","Target":"/x"}]}}`}, + { + "local-driver bind-mount-via-volume bypass (VolumeOptions.DriverConfig)", + `{"Image":"nginx","HostConfig":{"Mounts":[{"Type":"volume","Source":"fake","Target":"/etc","VolumeOptions":{"DriverConfig":{"Name":"local","Options":{"type":"none","device":"/etc","o":"bind"}}}}]}}`, + }, + {"non-empty Devices", `{"Image":"nginx","HostConfig":{"Devices":[{"PathOnHost":"/dev/sda","PathInContainer":"/dev/sda"}]}}`}, + {"non-empty Sysctls", `{"Image":"nginx","HostConfig":{"Sysctls":{"net.ipv4.ip_forward":"1"}}}`}, + {"unrecognized future HostConfig key", `{"Image":"nginx","HostConfig":{"SomeFutureField":true}}`}, + {"malformed JSON", `{"Image": not valid json`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.False(t, f.Allow("POST", "/containers/create", []byte(tc.body))) + }) + } +} + +// --- Shared anti-drift corpus (Section 3.3.5 of the Orthrus write-mode spec) --- +// +// Loads the exact same testdata/muzzle_corpus.json file that +// backend/internal/orthrus/muzzle_test.go's TestMuzzle_SharedCorpus loads, +// via a relative path across the module boundary — agent/ is a separate Go +// module but plain file I/O in a test isn't constrained by Go import +// resolution, so a single JSON fixture can be the source of truth for both +// independently-maintained allowlist implementations without duplicating +// the data itself (only the enforcement logic is duplicated, by design — +// see the doc comments on hostConfigAllowedKeys etc. in muzzle.go). + +type muzzleCorpusCase struct { + Description string `json:"description"` + Method string `json:"method"` + Path string `json:"path"` + Body json.RawMessage `json:"body,omitempty"` + AgentWriteEnabled bool `json:"agent_write_enabled"` + WantAllowed bool `json:"want_allowed"` +} + +func loadMuzzleCorpus(t *testing.T) []muzzleCorpusCase { + t.Helper() + data, err := os.ReadFile("../../backend/internal/orthrus/testdata/muzzle_corpus.json") + require.NoError(t, err) + var cases []muzzleCorpusCase + require.NoError(t, json.Unmarshal(data, &cases)) + require.NotEmpty(t, cases) + return cases +} + +func TestFilter_SharedCorpus(t *testing.T) { + cases := loadMuzzleCorpus(t) + for _, tc := range cases { + t.Run(tc.Description, func(t *testing.T) { + f := muzzle.New(tc.AgentWriteEnabled) + + // The corpus's Path field may include a query string (e.g. + // "/images/create?fromImage=nginx&tag=latest"), which Allow + // expects to have already been split off by the caller (mirrors + // how ServeProxy passes req.URL.Path, not req.URL.RequestURI()). + p := tc.Path + if idx := strings.IndexByte(p, '?'); idx >= 0 { + p = p[:idx] + } + + got := f.Allow(tc.Method, p, tc.Body) + assert.Equal(t, tc.WantAllowed, got) + }) + } +} diff --git a/backend/internal/orthrus/muzzle.go b/backend/internal/orthrus/muzzle.go index ec849be1c..92c7d8c49 100644 --- a/backend/internal/orthrus/muzzle.go +++ b/backend/internal/orthrus/muzzle.go @@ -1,6 +1,9 @@ package orthrus import ( + "bytes" + "encoding/json" + "io" "net/http" "path" "regexp" @@ -103,6 +106,275 @@ var allowedDockerPrefixSuffixPatterns = []struct { {prefix: "/distribution/", suffix: "/json"}, // registry digest check — read-only, used by update-checker tools } +// allowedWriteExactPaths lists the fixed-path write endpoints permitted when +// a session has negotiated write mode. Only consulted for POST requests, and +// only when Muzzle.writeEnabled is true — see the write-mode gate at the top +// of ServeHTTP. /containers/create additionally requires its body to pass +// validateContainerCreateBody; /images/create takes its parameters via query +// string (fromImage=, tag=) and has no body to validate. +var allowedWriteExactPaths = map[string]struct{}{ + "/containers/create": {}, + "/images/create": {}, +} + +// allowedWritePatterns lists the dynamic-segment write endpoints permitted +// when write mode is on: start/stop/restart an existing container, or +// remove one outright. Each entry pairs the exact HTTP method required with +// a path.Match pattern — unlike allowedDockerPrefixSuffixPatterns, these +// patterns operate on Docker-generated container IDs (a single path +// segment, never namespaced), so path.Match's "no cross-slash" behavior is +// the correct, sufficient tool here. +var allowedWritePatterns = []struct { + method string + pattern string +}{ + {method: http.MethodPost, pattern: "/containers/*/start"}, + {method: http.MethodPost, pattern: "/containers/*/stop"}, + {method: http.MethodPost, pattern: "/containers/*/restart"}, + {method: http.MethodDelete, pattern: "/containers/*"}, +} + +// maxContainerCreateBodyBytes bounds the size of a /containers/create +// request body accepted for validation. Generously larger than any +// realistic container-create body (typically a few KB even with a long +// env/label list) — bounds worst-case JSON-parse cost. This constant must +// stay numerically identical to agent/muzzle's copy; the shared test corpus +// (Section 3.3.5 of the write-mode spec) includes a boundary-size case to +// catch drift between the two. +const maxContainerCreateBodyBytes = 64 * 1024 + +// hostConfigAllowedKeys is the exhaustive allowlist of HostConfig top-level +// keys considered safe for a same-host container recreate. A key NOT in +// this set causes the whole /containers/create request to be rejected — +// this is what gives fail-closed behavior against Docker Engine API fields +// this list's authors don't know about yet, rather than trying to enumerate +// every dangerous key by name (Privileged, CapAdd, CapDrop, Binds, PidMode, +// IpcMode, UTSMode, CgroupnsMode, Devices, DeviceCgroupRules, SecurityOpt, +// Sysctls, Ulimits, GroupAdd, and any future field are all rejected by +// simply not appearing here — no separate denylist is maintained). +// +// NetworkMode and Mounts additionally receive a value-level check (see +// validateNetworkModeValue, validateMountsValue) beyond simple key presence, +// since both are operationally necessary fields that cannot be blanket-excluded +// the way the fields above are, but can still express a host-escape primitive +// through their value rather than their mere presence. +var hostConfigAllowedKeys = map[string]struct{}{ + "PortBindings": {}, + "RestartPolicy": {}, + "Memory": {}, + "MemorySwap": {}, + "NanoCpus": {}, + "CpuShares": {}, + "Mounts": {}, + "Dns": {}, + "DnsSearch": {}, + "ExtraHosts": {}, + "LogConfig": {}, + "AutoRemove": {}, + "ReadonlyRootfs": {}, + "Init": {}, + "NetworkMode": {}, +} + +// mountEntry is the subset of Docker's Mount struct this validator inspects. +type mountEntry struct { + Type string `json:"Type"` + VolumeOptions json.RawMessage `json:"VolumeOptions"` +} + +// validateNetworkModeValue rejects "host" networking and container-mode +// networking ("container:", which grants access to another container's +// network namespace) — the two NetworkMode values that carry the same class +// of host-escape risk as the blanket-excluded HostConfig keys. Every other +// value (a bridge network name, "bridge", "none", "default") is accepted, +// since NetworkMode is a normal, operationally required field for any +// container recreate that isn't on the default bridge network. +func validateNetworkModeValue(raw json.RawMessage) bool { + var mode string + if err := json.Unmarshal(raw, &mode); err != nil { + return false + } + if mode == "host" { + return false + } + if strings.HasPrefix(mode, "container:") { + return false + } + return true +} + +// validateMountsValue rejects any bind-type mount and any mount entry whose +// VolumeOptions carries a DriverConfig key at all, regardless of Type. +// +// The DriverConfig check closes a documented bypass of the Type check +// alone: Docker's default "local" volume driver accepts +// {"type":"none","device":"/host/path","o":"bind"} inside +// VolumeOptions.DriverConfig.Options, which makes a Type:"volume" mount +// behave exactly like an arbitrary host bind mount — fully equivalent in +// effect to the Type:"bind" mount rejected below, but reached through a +// field one level deeper. No attempt is made to selectively allow safe +// DriverConfig.Options key/value pairs: any DriverConfig presence at all is +// rejected, matching this validator's hard-reject-over-silent-strip +// philosophy. +func validateMountsValue(raw json.RawMessage) bool { + var mounts []mountEntry + if err := json.Unmarshal(raw, &mounts); err != nil { + return false + } + for _, mnt := range mounts { + if mnt.Type != "volume" && mnt.Type != "tmpfs" { + // Rejects "bind" and any unrecognized/future mount type. The + // legacy HostConfig.Binds field is rejected separately, simply + // by not appearing in hostConfigAllowedKeys. + return false + } + if len(mnt.VolumeOptions) == 0 { + continue + } + var volumeOptions map[string]json.RawMessage + if err := json.Unmarshal(mnt.VolumeOptions, &volumeOptions); err != nil { + return false + } + if _, hasDriverConfig := volumeOptions["DriverConfig"]; hasDriverConfig { + return false + } + } + return true +} + +// validateContainerCreateBody reads, size-caps, and validates a +// /containers/create request body against hostConfigAllowedKeys plus the +// NetworkMode/Mounts value-level checks, then re-buffers r.Body so the +// downstream reverse proxy can still forward the original bytes intact — +// reading r.Body consumes it exactly once, so it must be replaced with a +// fresh reader over the same bytes before this function returns, regardless +// of outcome. +// +// Returns (true, "") if the body is safe to forward. Returns (false, reason) +// if the request should be rejected, with reason suitable for both the audit +// log and the HTTP error response. +func validateContainerCreateBody(r *http.Request) (ok bool, reason string) { + if r.Body == nil { + return true, "" + } + + limited := io.LimitReader(r.Body, maxContainerCreateBodyBytes+1) + bodyBytes, err := io.ReadAll(limited) + _ = r.Body.Close() + if err != nil { + r.Body = http.NoBody + return false, "malformed request body" + } + if len(bodyBytes) > maxContainerCreateBodyBytes { + // Re-buffer the (oversized, rejected) bytes anyway: forwarding never + // happens on this path, but leaving r.Body in a consumed state would + // be surprising for any future caller of this function. + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + r.ContentLength = int64(len(bodyBytes)) + return false, "request body too large" + } + + // Re-buffer before any further validation so every return path below — + // success or rejection — leaves r.Body forwarding-ready. + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + r.ContentLength = int64(len(bodyBytes)) + + if len(bodyBytes) == 0 { + return true, "" + } + + var top map[string]json.RawMessage + if err := json.Unmarshal(bodyBytes, &top); err != nil { + return false, "malformed request body" + } + + hostConfigRaw, hasHostConfig := top["HostConfig"] + if !hasHostConfig { + return true, "" + } + + var hostConfig map[string]json.RawMessage + if err := json.Unmarshal(hostConfigRaw, &hostConfig); err != nil { + return false, "malformed request body" + } + + for key, rawValue := range hostConfig { + if _, allowed := hostConfigAllowedKeys[key]; !allowed { + return false, "disallowed HostConfig field: " + key + } + switch key { + case "NetworkMode": + if !validateNetworkModeValue(rawValue) { + return false, "disallowed HostConfig field: NetworkMode" + } + case "Mounts": + if !validateMountsValue(rawValue) { + return false, "disallowed HostConfig field: Mounts" + } + } + } + + return true, "" +} + +// writeAuditDetails marshals a small flat field set into the JSON string +// stored in a SecurityAudit's Details column. Marshal failure (unreachable +// for the string-only maps this is called with) degrades to an empty JSON +// object rather than losing the audit entry entirely. +func writeAuditDetails(fields map[string]string) string { + b, err := json.Marshal(fields) + if err != nil { + return "{}" + } + return string(b) +} + +// logAudit is a nil-safe wrapper around m.auditLogger.LogAudit. auditLogger +// is nil for read-only sessions and in most tests — a no-op in that case, +// not an error, since read-only traffic was never audited before this +// feature and isn't in scope for it now (see Section 3.3.7 of the +// write-mode spec). +func (m *Muzzle) logAudit(action, details string) { + if m.auditLogger == nil { + return + } + // Actor identifies the agent, not a Charon user — there is no + // authenticated operator in this request path, only a third-party tool + // on the other end of the tunnel. agentUUID (not agent name) is used + // here because it's what NewMuzzle actually receives; ResourceUUID + // carries the same value for querying, and a UUID is a more stable + // identifier for an audit trail than a renamable display name anyway. + _ = m.auditLogger.LogAudit(&models.SecurityAudit{ + Actor: "orthrus-agent:" + m.agentUUID, + Action: action, + EventCategory: "orthrus_write", + ResourceUUID: m.agentUUID, + Details: details, + }) +} + +func (m *Muzzle) auditAllowed(r *http.Request) { + m.logAudit("orthrus_write_allowed", writeAuditDetails(map[string]string{ + "method": r.Method, + "path": sanitizePath(r.URL.Path), + })) +} + +func (m *Muzzle) auditBlocked(r *http.Request, reason string) { + m.logAudit("orthrus_write_blocked", writeAuditDetails(map[string]string{ + "method": r.Method, + "path": sanitizePath(r.URL.Path), + "reason": reason, + })) +} + +func (m *Muzzle) auditRateLimited(r *http.Request) { + m.logAudit("orthrus_write_rate_limited", writeAuditDetails(map[string]string{ + "method": r.Method, + "path": sanitizePath(r.URL.Path), + })) +} + // Muzzle is an http.Handler wrapper that restricts Docker socket access // to a curated allowlist of read-only, non-destructive endpoints, plus an // optional narrow set of write endpoints when writeEnabled is true. @@ -159,6 +431,20 @@ func (m *Muzzle) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Write-endpoint handling must run before the unconditional GET-only + // check below, since POST/DELETE requests would otherwise be rejected + // before ever reaching the write allowlist. Only consulted when this + // session negotiated write mode (m.writeEnabled, fixed at construction + // time — see NewMuzzle) — every other session's POST/DELETE traffic + // falls straight through, unaffected, to the same 403 it always got. + if m.writeEnabled && (r.Method == http.MethodPost || r.Method == http.MethodDelete) { + if m.tryServeWrite(w, r, stripped) { + return + } + // Not on the write allowlist even with write mode on — fall through + // to the same "Forbidden" response every other disallowed request gets. + } + if r.Method != http.MethodGet { logger.Log().WithField("method", util.SanitizeForLog(r.Method)).WithField("path", sanitizePath(r.URL.Path)). Warn("orthrus: muzzle blocked non-GET Docker request") @@ -194,3 +480,50 @@ func (m *Muzzle) ServeHTTP(w http.ResponseWriter, r *http.Request) { Warn("orthrus: muzzle blocked disallowed Docker path") http.Error(w, "Forbidden", http.StatusForbidden) } + +// tryServeWrite handles a POST/DELETE request when this session has write +// mode enabled. Returns true if it fully handled the request — forwarded, +// rate-limited, or rejected with a write-specific reason and audit entry — +// false if the method/path combination isn't on the write allowlist at all, +// in which case the caller (ServeHTTP) falls through to the same generic +// 403 every other disallowed request receives. +func (m *Muzzle) tryServeWrite(w http.ResponseWriter, r *http.Request, stripped string) bool { + if r.Method == http.MethodPost { + if _, ok := allowedWriteExactPaths[stripped]; ok { + if stripped == "/containers/create" { + if valid, reason := validateContainerCreateBody(r); !valid { + m.auditBlocked(r, reason) + http.Error(w, "Forbidden: "+reason, http.StatusForbidden) + return true + } + } + return m.forwardWrite(w, r) + } + } + + for _, p := range allowedWritePatterns { + if r.Method != p.method { + continue + } + if matched, err := path.Match(p.pattern, stripped); err == nil && matched { + return m.forwardWrite(w, r) + } + } + + return false +} + +// forwardWrite applies the write-path rate limiter, audits the outcome, and +// forwards the request on success. Always returns true (the caller uses the +// return value only to distinguish "on the write allowlist" from "not"; a +// rate-limited request is still a request tryServeWrite fully handled). +func (m *Muzzle) forwardWrite(w http.ResponseWriter, r *http.Request) bool { + if m.writeLimiter != nil && !m.writeLimiter.Allow() { + m.auditRateLimited(r) + http.Error(w, "Too Many Requests", http.StatusTooManyRequests) + return true + } + m.auditAllowed(r) + m.next.ServeHTTP(w, r) + return true +} diff --git a/backend/internal/orthrus/muzzle_test.go b/backend/internal/orthrus/muzzle_test.go index dc032866c..7c1109916 100644 --- a/backend/internal/orthrus/muzzle_test.go +++ b/backend/internal/orthrus/muzzle_test.go @@ -1,11 +1,20 @@ package orthrus import ( + "bytes" + "encoding/json" "net/http" "net/http/httptest" + "os" + "sync" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/time/rate" + + "github.com/Wikid82/charon/backend/internal/models" ) func passthroughHandler() http.Handler { @@ -251,3 +260,292 @@ func TestMuzzle_ImageAndDistributionEndpoints_POSTBlocked(t *testing.T) { }) } } + +// --- Write-mode tests (Section 3.3.3/3.3.4 of the Orthrus write-mode spec) --- + +func TestMuzzle_WriteEndpoints_BlockedWhenWriteDisabled(t *testing.T) { + m := NewMuzzle(passthroughHandler(), false, nil, nil, "") + + cases := []struct { + method string + path string + }{ + {http.MethodPost, "/containers/create"}, + {http.MethodPost, "/images/create"}, + {http.MethodPost, "/containers/abc123/start"}, + {http.MethodPost, "/containers/abc123/stop"}, + {http.MethodPost, "/containers/abc123/restart"}, + {http.MethodDelete, "/containers/abc123"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + req := httptest.NewRequest(tc.method, tc.path, http.NoBody) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusForbidden, rr.Code) + }) + } +} + +func TestMuzzle_WriteEndpoints_AllowedWhenWriteEnabled(t *testing.T) { + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + cases := []struct { + method string + path string + }{ + {http.MethodPost, "/images/create"}, + {http.MethodPost, "/containers/abc123/start"}, + {http.MethodPost, "/containers/abc123/stop"}, + {http.MethodPost, "/containers/abc123/restart"}, + {http.MethodDelete, "/containers/abc123"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + req := httptest.NewRequest(tc.method, tc.path, http.NoBody) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusOK, rr.Code) + }) + } +} + +func TestMuzzle_WriteMode_NonWriteEndpointsStillBlocked(t *testing.T) { + // Even with write mode on, endpoints outside the fixed six-operation + // list (exec, image delete, build, prune, auth, commit, Swarm/service) + // must remain permanently blocked. Section 7, Explicit Out-of-Scope. + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + cases := []struct { + method string + path string + }{ + {http.MethodPost, "/containers/abc123/exec"}, + {http.MethodPost, "/exec/xyz/start"}, + {http.MethodDelete, "/images/nginx"}, + {http.MethodPost, "/build"}, + {http.MethodPost, "/system/prune"}, + {http.MethodPost, "/auth"}, + {http.MethodPost, "/commit"}, + {http.MethodPost, "/networks/create"}, + {http.MethodDelete, "/networks/mynet"}, + {http.MethodPost, "/volumes/create"}, + {http.MethodDelete, "/volumes/myvol"}, + } + + for _, tc := range cases { + t.Run(tc.method+" "+tc.path, func(t *testing.T) { + req := httptest.NewRequest(tc.method, tc.path, http.NoBody) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusForbidden, rr.Code) + }) + } +} + +func TestMuzzle_ContainersCreate_SafeBodyAllowed(t *testing.T) { + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + body := `{"Image":"nginx:latest","HostConfig":{"PortBindings":{"80/tcp":[{"HostPort":"8080"}]},"RestartPolicy":{"Name":"unless-stopped"},"NetworkMode":"my-bridge"}}` + req := httptest.NewRequest(http.MethodPost, "/containers/create", bytes.NewReader([]byte(body))) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusOK, rr.Code) +} + +func TestMuzzle_ContainersCreate_DangerousBodiesRejected(t *testing.T) { + writeLimiter := rate.NewLimiter(rate.Inf, 100) + + cases := []struct { + name string + body string + }{ + {"Privileged", `{"Image":"nginx","HostConfig":{"Privileged":true}}`}, + {"CapAdd", `{"Image":"nginx","HostConfig":{"CapAdd":["SYS_ADMIN"]}}`}, + {"legacy Binds", `{"Image":"nginx","HostConfig":{"Binds":["/:/host"]}}`}, + {"NetworkMode host", `{"Image":"nginx","HostConfig":{"NetworkMode":"host"}}`}, + {"NetworkMode container:*", `{"Image":"nginx","HostConfig":{"NetworkMode":"container:abc"}}`}, + {"bind-type Mounts", `{"Image":"nginx","HostConfig":{"Mounts":[{"Type":"bind","Source":"/etc","Target":"/x"}]}}`}, + { + "local-driver bind-mount-via-volume bypass (VolumeOptions.DriverConfig)", + `{"Image":"nginx","HostConfig":{"Mounts":[{"Type":"volume","Source":"fake","Target":"/etc","VolumeOptions":{"DriverConfig":{"Name":"local","Options":{"type":"none","device":"/etc","o":"bind"}}}}]}}`, + }, + {"non-empty Devices", `{"Image":"nginx","HostConfig":{"Devices":[{"PathOnHost":"/dev/sda","PathInContainer":"/dev/sda"}]}}`}, + {"non-empty Sysctls", `{"Image":"nginx","HostConfig":{"Sysctls":{"net.ipv4.ip_forward":"1"}}}`}, + {"unrecognized future HostConfig key", `{"Image":"nginx","HostConfig":{"SomeFutureField":true}}`}, + {"malformed JSON", `{"Image": not valid json`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + req := httptest.NewRequest(http.MethodPost, "/containers/create", bytes.NewReader([]byte(tc.body))) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusForbidden, rr.Code) + }) + } +} + +func TestMuzzle_ContainersCreate_BodyTooLarge(t *testing.T) { + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + oversized := bytes.Repeat([]byte("a"), maxContainerCreateBodyBytes+1) + body := `{"Image":"nginx","Labels":{"padding":"` + string(oversized) + `"}}` + req := httptest.NewRequest(http.MethodPost, "/containers/create", bytes.NewReader([]byte(body))) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + assert.Equal(t, http.StatusForbidden, rr.Code) +} + +func TestMuzzle_WriteEndpoints_RateLimited(t *testing.T) { + // Burst of 1, refill effectively never within the test's lifetime — the + // first write request consumes the only token; the second must 429. + writeLimiter := rate.NewLimiter(rate.Every(time.Hour), 1) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + req1 := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + rr1 := httptest.NewRecorder() + m.ServeHTTP(rr1, req1) + assert.Equal(t, http.StatusOK, rr1.Code) + + req2 := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + rr2 := httptest.NewRecorder() + m.ServeHTTP(rr2, req2) + assert.Equal(t, http.StatusTooManyRequests, rr2.Code) +} + +// mockAuditLogger records every SecurityAudit entry passed to LogAudit, for +// assertion in tests. Safe for concurrent use. +type mockAuditLogger struct { + mu sync.Mutex + entries []*models.SecurityAudit +} + +func (m *mockAuditLogger) LogAudit(a *models.SecurityAudit) error { + m.mu.Lock() + defer m.mu.Unlock() + m.entries = append(m.entries, a) + return nil +} + +func (m *mockAuditLogger) all() []*models.SecurityAudit { + m.mu.Lock() + defer m.mu.Unlock() + out := make([]*models.SecurityAudit, len(m.entries)) + copy(out, m.entries) + return out +} + +func TestMuzzle_AuditLog_AllowedWriteRecorded(t *testing.T) { + logger := &mockAuditLogger{} + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, logger, "agent-uuid-1") + + req := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + + entries := logger.all() + require.Len(t, entries, 1) + assert.Equal(t, "orthrus_write_allowed", entries[0].Action) + assert.Equal(t, "orthrus_write", entries[0].EventCategory) + assert.Equal(t, "agent-uuid-1", entries[0].ResourceUUID) + assert.Contains(t, entries[0].Actor, "agent-uuid-1") +} + +func TestMuzzle_AuditLog_BlockedWriteRecorded(t *testing.T) { + logger := &mockAuditLogger{} + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, logger, "agent-uuid-2") + + body := `{"Image":"nginx","HostConfig":{"Privileged":true}}` + req := httptest.NewRequest(http.MethodPost, "/containers/create", bytes.NewReader([]byte(body))) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + + entries := logger.all() + require.Len(t, entries, 1) + assert.Equal(t, "orthrus_write_blocked", entries[0].Action) + assert.Contains(t, entries[0].Details, "Privileged") +} + +func TestMuzzle_AuditLog_RateLimitedRecorded(t *testing.T) { + logger := &mockAuditLogger{} + writeLimiter := rate.NewLimiter(rate.Every(time.Hour), 1) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, logger, "agent-uuid-3") + + req1 := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + m.ServeHTTP(httptest.NewRecorder(), req1) + req2 := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + m.ServeHTTP(httptest.NewRecorder(), req2) + + entries := logger.all() + require.Len(t, entries, 2) + assert.Equal(t, "orthrus_write_allowed", entries[0].Action) + assert.Equal(t, "orthrus_write_rate_limited", entries[1].Action) +} + +func TestMuzzle_AuditLog_NilLoggerIsNoop(t *testing.T) { + writeLimiter := rate.NewLimiter(rate.Inf, 100) + m := NewMuzzle(passthroughHandler(), true, writeLimiter, nil, "agent-uuid") + + req := httptest.NewRequest(http.MethodPost, "/containers/abc/start", http.NoBody) + rr := httptest.NewRecorder() + assert.NotPanics(t, func() { m.ServeHTTP(rr, req) }) + assert.Equal(t, http.StatusOK, rr.Code) +} + +// --- Shared anti-drift corpus (Section 3.3.5 of the Orthrus write-mode spec) --- +// +// muzzle_corpus.json is the single source of truth this test and +// agent/muzzle/muzzle_test.go's TestFilter_SharedCorpus both load, so the +// backend and agent allowlist implementations are proven to agree on every +// case in it — the concrete mitigation for the drift class that let the +// image/distribution read-only fix land in this package without the +// agent-side copy (fixed later, commits 98a68b67 / eabf358d) once already. + +type muzzleCorpusCase struct { + Description string `json:"description"` + Method string `json:"method"` + Path string `json:"path"` + Body json.RawMessage `json:"body,omitempty"` + AgentWriteEnabled bool `json:"agent_write_enabled"` + WantAllowed bool `json:"want_allowed"` +} + +func loadMuzzleCorpus(t *testing.T) []muzzleCorpusCase { + t.Helper() + data, err := os.ReadFile("testdata/muzzle_corpus.json") + require.NoError(t, err) + var cases []muzzleCorpusCase + require.NoError(t, json.Unmarshal(data, &cases)) + require.NotEmpty(t, cases) + return cases +} + +func TestMuzzle_SharedCorpus(t *testing.T) { + cases := loadMuzzleCorpus(t) + for _, tc := range cases { + t.Run(tc.Description, func(t *testing.T) { + var writeLimiter *rate.Limiter + if tc.AgentWriteEnabled { + writeLimiter = rate.NewLimiter(rate.Inf, 1000) + } + m := NewMuzzle(passthroughHandler(), tc.AgentWriteEnabled, writeLimiter, nil, "corpus-test-agent") + + req := httptest.NewRequest(tc.Method, tc.Path, bytes.NewReader(tc.Body)) + rr := httptest.NewRecorder() + m.ServeHTTP(rr, req) + + allowed := rr.Code == http.StatusOK + assert.Equal(t, tc.WantAllowed, allowed, "got status %d", rr.Code) + }) + } +} diff --git a/backend/internal/orthrus/server.go b/backend/internal/orthrus/server.go index 96bfd4c5d..6e6f46544 100644 --- a/backend/internal/orthrus/server.go +++ b/backend/internal/orthrus/server.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "strconv" "strings" "sync" "time" @@ -33,6 +34,11 @@ type OrthrusServer struct { ctx context.Context cancel context.CancelFunc wg sync.WaitGroup + // auditLogger records write-path audit entries for every AgentSession + // this server creates. nil until SetAuditLogger is called (e.g. at + // startup in routes.go, once the concrete *services.SecurityService is + // available) — a nil auditLogger is a safe no-op throughout Muzzle. + auditLogger AuditLogger } // NewOrthrusServer creates an OrthrusServer with the given database and internal CA. @@ -47,6 +53,15 @@ func NewOrthrusServer(db *gorm.DB, ca *InternalCA) (*OrthrusServer, error) { }, nil } +// SetAuditLogger wires the write-path audit logger used by every +// AgentSession this server creates from this point forward. Called once at +// startup (routes.go) with the concrete *services.SecurityService, which +// satisfies the AuditLogger interface structurally — orthrus never imports +// services directly (see the AuditLogger doc comment in muzzle.go for why). +func (s *OrthrusServer) SetAuditLogger(al AuditLogger) { + s.auditLogger = al +} + // Stop cancels the server context, closes all active agent sessions, and waits // for all background goroutines to exit. Closing sessions explicitly ensures // yamux's internal goroutines terminate promptly regardless of the underlying @@ -78,13 +93,26 @@ func (s *OrthrusServer) HandleWebSocket(c *gin.Context) { return } - conn, err := wsUpgrader.Upgrade(c.Writer, c.Request, nil) + // X-Orthrus-Write-Enabled is delivered atomically as part of the 101 + // Switching Protocols response that completes the handshake — the agent + // reads it off the *http.Response returned by its dial call (see + // leash.go:connect) and uses it to construct a connection-scoped + // muzzle.Filter for the life of this one connection. This is Option A + // from Section 3.3.1 of the write-mode spec: no new yamux control-stream + // type is introduced, avoiding a third hand-maintained stream-type + // constant on top of the two (streamTypeDocker, streamTypePortForward) + // that already have to be kept in sync between this package and + // agent/leash/leash.go. + respHeader := http.Header{} + respHeader.Set("X-Orthrus-Write-Enabled", strconv.FormatBool(agent.WriteEnabled)) + + conn, err := wsUpgrader.Upgrade(c.Writer, c.Request, respHeader) if err != nil { logger.Log().WithError(err).Error("orthrus: WebSocket upgrade failed") return } - session, err := NewAgentSession(agent.UUID, agent.Name, agent.WriteEnabled, conn) + session, err := NewAgentSession(agent.UUID, agent.Name, agent.WriteEnabled, s.auditLogger, conn) if err != nil { logger.Log().WithError(err).Error("orthrus: create agent session failed") _ = conn.Close() diff --git a/backend/internal/orthrus/server_coverage_test.go b/backend/internal/orthrus/server_coverage_test.go index 2f5175343..386d4bd24 100644 --- a/backend/internal/orthrus/server_coverage_test.go +++ b/backend/internal/orthrus/server_coverage_test.go @@ -25,7 +25,7 @@ func TestOrthrusServer_GetSession_KnownUUID(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("sess-uuid", "sess-agent", false, serverConn) + sess, err := NewAgentSession("sess-uuid", "sess-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -44,7 +44,7 @@ func TestOrthrusServer_GetProxyAddr_SessionExists_NoProxy(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("no-proxy-uuid", "agent", false, serverConn) + sess, err := NewAgentSession("no-proxy-uuid", "agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -63,7 +63,7 @@ func TestOrthrusServer_GetProxyAddr_SessionExists_WithProxy(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("with-proxy-uuid", "agent", false, serverConn) + sess, err := NewAgentSession("with-proxy-uuid", "agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -86,7 +86,7 @@ func TestOrthrusServer_DisconnectAgent_WithSession(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("disc-uuid", "disc-agent", false, serverConn) + sess, err := NewAgentSession("disc-uuid", "disc-agent", false, nil, serverConn) require.NoError(t, err) srv.sessions.Store("disc-uuid", sess) @@ -194,7 +194,7 @@ func TestOrthrusServer_WatchHeartbeat_ClosedSession_ExitsAndMarksOffline(t *test serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("wh-uuid", "wh-agent", false, serverConn) + sess, err := NewAgentSession("wh-uuid", "wh-agent", false, nil, serverConn) require.NoError(t, err) // Close session immediately so IsAlive() returns false on first tick. @@ -394,7 +394,7 @@ func TestHandleWebSocket_DisplacesExistingSession(t *testing.T) { // prior connection that has not yet been cleaned up. oldConn, oldCleanup := testWSPair(t) defer oldCleanup() - oldSess, err := NewAgentSession("displace-uuid", "displace-agent", false, oldConn) + oldSess, err := NewAgentSession("displace-uuid", "displace-agent", false, nil, oldConn) require.NoError(t, err) srv.sessions.Store("displace-uuid", oldSess) diff --git a/backend/internal/orthrus/server_external_proxy_test.go b/backend/internal/orthrus/server_external_proxy_test.go index fb2013f2d..c10dc2854 100644 --- a/backend/internal/orthrus/server_external_proxy_test.go +++ b/backend/internal/orthrus/server_external_proxy_test.go @@ -81,7 +81,7 @@ func TestOrthrusServer_DisconnectAgent_ClosesExternalProxy(t *testing.T) { serverConn, wsCleanup := testWSPair(t) defer wsCleanup() - sess, err := NewAgentSession("ext-srv02-uuid", "ext-srv02-agent", false, serverConn) + sess, err := NewAgentSession("ext-srv02-uuid", "ext-srv02-agent", false, nil, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) diff --git a/backend/internal/orthrus/server_proxy_test.go b/backend/internal/orthrus/server_proxy_test.go index 92a2e1f9a..bfca9a5ad 100644 --- a/backend/internal/orthrus/server_proxy_test.go +++ b/backend/internal/orthrus/server_proxy_test.go @@ -71,7 +71,7 @@ func TestOrthrusServer_WatchHeartbeat_ClosesProxyListener(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("proxy-s2-uuid", "proxy-s2-agent", false, serverConn) + sess, err := NewAgentSession("proxy-s2-uuid", "proxy-s2-agent", false, nil, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) diff --git a/backend/internal/orthrus/server_test.go b/backend/internal/orthrus/server_test.go index dd6355941..6ed2184c8 100644 --- a/backend/internal/orthrus/server_test.go +++ b/backend/internal/orthrus/server_test.go @@ -217,7 +217,7 @@ func TestWatchHeartbeat_StaleGoroutine_DoesNotEvictNewSession(t *testing.T) { // goroutine is still running after a newer session has replaced it. conn1, done1 := testWSPair(t) defer done1() - sess1, err := NewAgentSession(agentUUID, "race-agent", false, conn1) + sess1, err := NewAgentSession(agentUUID, "race-agent", false, nil, conn1) require.NoError(t, err) require.NoError(t, sess1.Close()) require.False(t, sess1.IsAlive()) @@ -225,7 +225,7 @@ func TestWatchHeartbeat_StaleGoroutine_DoesNotEvictNewSession(t *testing.T) { // sess2: alive — represents the current (newer) reconnect stored in the map. conn2, done2 := testWSPair(t) defer done2() - sess2, err := NewAgentSession(agentUUID, "race-agent", false, conn2) + sess2, err := NewAgentSession(agentUUID, "race-agent", false, nil, conn2) require.NoError(t, err) t.Cleanup(func() { _ = sess2.Close() }) srv.sessions.Store(agentUUID, sess2) @@ -277,7 +277,7 @@ func TestWatchHeartbeat_CurrentSession_MarksOfflineAndEvictsFromMap(t *testing.T conn, wsCleanup := testWSPair(t) defer wsCleanup() - sess, err := NewAgentSession(agentUUID, "current-agent", false, conn) + sess, err := NewAgentSession(agentUUID, "current-agent", false, nil, conn) require.NoError(t, err) require.NoError(t, sess.Close()) require.False(t, sess.IsAlive()) diff --git a/backend/internal/orthrus/session.go b/backend/internal/orthrus/session.go index da91faa85..f4bb1fd55 100644 --- a/backend/internal/orthrus/session.go +++ b/backend/internal/orthrus/session.go @@ -15,6 +15,21 @@ import ( "github.com/Wikid82/charon/backend/internal/logger" "github.com/gorilla/websocket" "github.com/hashicorp/yamux" + "golang.org/x/time/rate" +) + +// writeLimiterEvery and writeLimiterBurst configure the token-bucket rate +// limiter applied to write-path traffic on any write-enabled session +// (Section 3.3.6 of the write-mode spec). Steady-state 0.5 req/s (30/min), +// burst of 5: a single compliant update cycle (pull → stop → remove → +// create → start) is exactly 5 write calls, so the burst allows one full +// update to proceed immediately, while the steady-state rate bounds +// sustained container churn well below anything a legitimate periodic +// update-checker would ever need. Fixed constants for v1 — no DB-backed +// per-agent tuning (see Section 7, Explicit Out-of-Scope). +const ( + writeLimiterEvery = 2 * time.Second + writeLimiterBurst = 5 ) // streamTypeDocker identifies Docker socket proxy streams opened toward the agent. @@ -124,11 +139,17 @@ type AgentSession struct { // the life of this session, exactly like extProxyPort — a DB toggle only // takes effect on the agent's next reconnect. writeEnabled bool - mu sync.Mutex + // writeLimiter bounds write-request throughput for this session; nil + // unless writeEnabled (lazily constructed in NewAgentSession). + writeLimiter *rate.Limiter + // auditLogger records write-path audit entries; nil is a safe no-op + // (see Muzzle.logAudit), used by tests and read-only sessions. + auditLogger AuditLogger + mu sync.Mutex } // NewAgentSession wraps the WebSocket connection in a Yamux server session. -func NewAgentSession(agentUUID, agentName string, writeEnabled bool, conn *websocket.Conn) (*AgentSession, error) { +func NewAgentSession(agentUUID, agentName string, writeEnabled bool, auditLogger AuditLogger, conn *websocket.Conn) (*AgentSession, error) { cfg := yamux.DefaultConfig() cfg.LogOutput = io.Discard @@ -139,11 +160,18 @@ func NewAgentSession(agentUUID, agentName string, writeEnabled bool, conn *webso _, cancel := context.WithCancel(context.Background()) + var writeLimiter *rate.Limiter + if writeEnabled { + writeLimiter = rate.NewLimiter(rate.Every(writeLimiterEvery), writeLimiterBurst) + } + return &AgentSession{ agentUUID: agentUUID, agentName: agentName, conn: conn, session: session, + writeLimiter: writeLimiter, + auditLogger: auditLogger, cancel: cancel, writeEnabled: writeEnabled, }, nil @@ -309,11 +337,8 @@ func (s *AgentSession) StartExternalProxy(port int) error { Transport: baseTransport, } - // writeLimiter and auditLogger are wired in Commit 3 (rate limiting + - // audit logging); nil is a safe no-op here since the write-endpoint - // allowlist branch that would consult them does not exist yet. srv := &http.Server{ - Handler: NewMuzzle(rp, s.writeEnabled, nil, nil, s.agentUUID), + Handler: NewMuzzle(rp, s.writeEnabled, s.writeLimiter, s.auditLogger, s.agentUUID), ReadHeaderTimeout: 10 * time.Second, WriteTimeout: 0, } diff --git a/backend/internal/orthrus/session_coverage_test.go b/backend/internal/orthrus/session_coverage_test.go index 86d69feaf..d267d1b7a 100644 --- a/backend/internal/orthrus/session_coverage_test.go +++ b/backend/internal/orthrus/session_coverage_test.go @@ -72,7 +72,7 @@ func TestAgentSession_GetProxyAddr_WithPort(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("port-uuid", "port-agent", false, serverConn) + sess, err := NewAgentSession("port-uuid", "port-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -105,7 +105,7 @@ func TestGetExternalProxyStatus_ErrorFieldPopulated(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext-err-uuid", "ext-err-agent", false, serverConn) + sess, err := NewAgentSession("ext-err-uuid", "ext-err-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() diff --git a/backend/internal/orthrus/session_external_proxy_test.go b/backend/internal/orthrus/session_external_proxy_test.go index fba0b4dd2..febb02fba 100644 --- a/backend/internal/orthrus/session_external_proxy_test.go +++ b/backend/internal/orthrus/session_external_proxy_test.go @@ -38,7 +38,7 @@ func sessionWithLoopback(t *testing.T) (sess *AgentSession, clientYamux *yamux.S cy, err := yamux.Client(newWSNetConn(clientConn), cfg) require.NoError(t, err) - s, err := NewAgentSession("ext-test-uuid", "ext-test-agent", false, serverConn) + s, err := NewAgentSession("ext-test-uuid", "ext-test-agent", false, nil, serverConn) require.NoError(t, err) require.NoError(t, s.StartDockerProxy()) @@ -89,7 +89,7 @@ func TestStartExternalProxy_ZeroPortIsNoOp(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext01-uuid", "ext01-agent", false, serverConn) + sess, err := NewAgentSession("ext01-uuid", "ext01-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -103,7 +103,7 @@ func TestStartExternalProxy_LoopbackNotStarted(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("ext02-uuid", "ext02-agent", false, serverConn) + sess, err := NewAgentSession("ext02-uuid", "ext02-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() diff --git a/backend/internal/orthrus/session_proxy_test.go b/backend/internal/orthrus/session_proxy_test.go index 0540007d0..ce30972bf 100644 --- a/backend/internal/orthrus/session_proxy_test.go +++ b/backend/internal/orthrus/session_proxy_test.go @@ -49,7 +49,7 @@ func TestStartDockerProxy_SetsProxyAddr(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u1-uuid", "u1-agent", false, serverConn) + sess, err := NewAgentSession("u1-uuid", "u1-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -64,7 +64,7 @@ func TestStartDockerProxy_Idempotent(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u2-uuid", "u2-agent", false, serverConn) + sess, err := NewAgentSession("u2-uuid", "u2-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -84,7 +84,7 @@ func TestStartDockerProxy_AcceptsAndForwards(t *testing.T) { serverConn, clientConn, done := testWSPairBoth(t) defer done() - sess, err := NewAgentSession("u3-uuid", "u3-agent", false, serverConn) + sess, err := NewAgentSession("u3-uuid", "u3-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -139,7 +139,7 @@ func TestClose_StopsProxyListener(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u4-uuid", "u4-agent", false, serverConn) + sess, err := NewAgentSession("u4-uuid", "u4-agent", false, nil, serverConn) require.NoError(t, err) require.NoError(t, sess.StartDockerProxy()) @@ -161,7 +161,7 @@ func TestStartDockerProxy_AfterClose(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("u5-uuid", "u5-agent", false, serverConn) + sess, err := NewAgentSession("u5-uuid", "u5-agent", false, nil, serverConn) require.NoError(t, err) require.NoError(t, sess.Close()) @@ -185,7 +185,7 @@ func TestAgentSession_runProxyListener_NonErrClosedError_Returns(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("rpl-uuid", "rpl-agent", false, serverConn) + sess, err := NewAgentSession("rpl-uuid", "rpl-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -208,7 +208,7 @@ func TestAgentSession_proxyConn_ClosedSession_ReturnsQuietly(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("pc-closed-uuid", "pc-agent", false, serverConn) + sess, err := NewAgentSession("pc-closed-uuid", "pc-agent", false, nil, serverConn) require.NoError(t, err) // Close the yamux session so session.Open() returns an error. diff --git a/backend/internal/orthrus/session_test.go b/backend/internal/orthrus/session_test.go index 5cb1065eb..2829f37f5 100644 --- a/backend/internal/orthrus/session_test.go +++ b/backend/internal/orthrus/session_test.go @@ -50,7 +50,7 @@ func TestNewAgentSession_IsAlive(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -61,7 +61,7 @@ func TestAgentSession_GetProxyAddr_NoPort(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() @@ -72,7 +72,7 @@ func TestAgentSession_Close_SetsNotAlive(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("uuid-1", "agent-1", false, serverConn) + sess, err := NewAgentSession("uuid-1", "agent-1", false, nil, serverConn) require.NoError(t, err) require.NoError(t, sess.Close()) @@ -83,7 +83,7 @@ func TestStartExternalProxy_TransportDisablesKeepAlives(t *testing.T) { serverConn, done := testWSPair(t) defer done() - sess, err := NewAgentSession("keepalive-uuid", "keepalive-agent", false, serverConn) + sess, err := NewAgentSession("keepalive-uuid", "keepalive-agent", false, nil, serverConn) require.NoError(t, err) defer func() { _ = sess.Close() }() diff --git a/backend/internal/orthrus/testdata/muzzle_corpus.json b/backend/internal/orthrus/testdata/muzzle_corpus.json new file mode 100644 index 000000000..2d9421b7e --- /dev/null +++ b/backend/internal/orthrus/testdata/muzzle_corpus.json @@ -0,0 +1,263 @@ +[ + { + "description": "read-only allowlist regression: GET /_ping always allowed", + "method": "GET", + "path": "/_ping", + "agent_write_enabled": false, + "want_allowed": true + }, + { + "description": "read-only allowlist regression: GET /containers/json always allowed", + "method": "GET", + "path": "/containers/json", + "agent_write_enabled": false, + "want_allowed": true + }, + { + "description": "read-only allowlist regression: GET /containers/json still allowed with write mode on", + "method": "GET", + "path": "/containers/json", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "read-only allowlist regression: namespaced image inspect allowed", + "method": "GET", + "path": "/images/ghcr.io/org/repo/json", + "agent_write_enabled": false, + "want_allowed": true + }, + { + "description": "read-only allowlist regression: /exec never on any allowlist", + "method": "GET", + "path": "/exec/abc123/start", + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "write endpoint blocked when write mode off: POST /containers/create", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx:latest" }, + "agent_write_enabled": false, + "want_allowed": false + }, + { + "description": "write endpoint allowed when write mode on: POST /containers/create, safe body", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx:latest", "Env": ["FOO=bar"] }, + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint allowed: POST /containers/create, safe HostConfig", + "method": "POST", + "path": "/containers/create", + "body": { + "Image": "nginx:latest", + "HostConfig": { + "PortBindings": { "80/tcp": [{ "HostPort": "8080" }] }, + "RestartPolicy": { "Name": "unless-stopped" }, + "NetworkMode": "my-bridge-network" + } + }, + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint allowed: POST /containers/create, safe volume mount", + "method": "POST", + "path": "/containers/create", + "body": { + "Image": "nginx:latest", + "HostConfig": { + "Mounts": [{ "Type": "volume", "Source": "data", "Target": "/data" }] + } + }, + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint blocked when write mode off: POST /images/create", + "method": "POST", + "path": "/images/create?fromImage=nginx&tag=latest", + "agent_write_enabled": false, + "want_allowed": false + }, + { + "description": "write endpoint allowed when write mode on: POST /images/create", + "method": "POST", + "path": "/images/create?fromImage=nginx&tag=latest", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint allowed when write mode on: POST /containers/{id}/start", + "method": "POST", + "path": "/containers/abc123/start", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint blocked when write mode off: POST /containers/{id}/start", + "method": "POST", + "path": "/containers/abc123/start", + "agent_write_enabled": false, + "want_allowed": false + }, + { + "description": "write endpoint allowed when write mode on: POST /containers/{id}/stop", + "method": "POST", + "path": "/containers/abc123/stop", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint allowed when write mode on: POST /containers/{id}/restart", + "method": "POST", + "path": "/containers/abc123/restart", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint allowed when write mode on: DELETE /containers/{id}", + "method": "DELETE", + "path": "/containers/abc123", + "agent_write_enabled": true, + "want_allowed": true + }, + { + "description": "write endpoint blocked when write mode off: DELETE /containers/{id}", + "method": "DELETE", + "path": "/containers/abc123", + "agent_write_enabled": false, + "want_allowed": false + }, + { + "description": "never allowed under any configuration: /containers/{id}/exec", + "method": "POST", + "path": "/containers/abc123/exec", + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "never allowed under any configuration: image delete", + "method": "DELETE", + "path": "/images/nginx", + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "never allowed under any configuration: system prune", + "method": "POST", + "path": "/system/prune", + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: Privileged", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "Privileged": true } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: CapAdd", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "CapAdd": ["SYS_ADMIN"] } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: legacy Binds", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "Binds": ["/:/host"] } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: NetworkMode host", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "NetworkMode": "host" } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: NetworkMode container:", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "NetworkMode": "container:abc123" } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: bind-type Mounts entry", + "method": "POST", + "path": "/containers/create", + "body": { + "Image": "nginx", + "HostConfig": { + "Mounts": [{ "Type": "bind", "Source": "/etc", "Target": "/host-etc" }] + } + }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: local-driver bind-mount-via-volume bypass (VolumeOptions.DriverConfig) — the specific bypass found and closed during Supervisor review", + "method": "POST", + "path": "/containers/create", + "body": { + "Image": "nginx", + "HostConfig": { + "Mounts": [ + { + "Type": "volume", + "Source": "fake-volume", + "Target": "/host-etc", + "VolumeOptions": { + "DriverConfig": { + "Name": "local", + "Options": { "type": "none", "device": "/etc", "o": "bind" } + } + } + } + ] + } + }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: non-empty Devices", + "method": "POST", + "path": "/containers/create", + "body": { + "Image": "nginx", + "HostConfig": { "Devices": [{ "PathOnHost": "/dev/sda", "PathInContainer": "/dev/sda" }] } + }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: non-empty Sysctls", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "Sysctls": { "net.ipv4.ip_forward": "1" } } }, + "agent_write_enabled": true, + "want_allowed": false + }, + { + "description": "dangerous body rejected even with write mode on: unrecognized future HostConfig key (fail-closed against unknown fields)", + "method": "POST", + "path": "/containers/create", + "body": { "Image": "nginx", "HostConfig": { "SomeFutureDockerAPIField": true } }, + "agent_write_enabled": true, + "want_allowed": false + } +] From 932621510aed5d747ec6824252756fea82ad39a6 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 11:40:36 +0000 Subject: [PATCH 05/22] feat(orthrus): extend PATCH/proxy-status API for write mode, wire audit logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section 3.2 of docs/plans/current_spec.md: - OrthrusService.Patch gains a writeEnabled *bool parameter, following the existing "*T present = intend to change" pattern used by every other field on this endpoint. - OrthrusHandler.Patch emits a SecurityAudit entry (orthrus_write_enabled/orthrus_write_disabled) whenever an operator toggles write_enabled, using this codebase's existing actorFromContext(c) convention for admin-initiated audit events (matches security_handler.go/crowdsec_handler.go) rather than growing OrthrusService's dependencies with a *gin.Context-derived actor — this is a handler-layer concern, separate from the per-write-request audit entries Muzzle already emits directly. - GetProxyStatus response gains configured_write_enabled (DB value) and active_write_enabled (live session value, from the extended ExternalProxyStatus), so the frontend can detect and surface the same "configured differs from active, reconnect needed" situation already modeled for external_proxy_port. - routes.go wires orthrusServer.SetAuditLogger(securityService) so Muzzle's per-write-request audit entries (added in the previous commit) actually reach the database in the running app, not just in tests. New test coverage: write_enabled toggling on/off at the service layer, audit-entry emission (and non-emission when write_enabled isn't part of the request) at the handler layer via SecurityService.Flush() + ListAuditLogs, and the two new proxy-status response fields. Full backend suite green; GORM security scan clean (0 critical/high). --- .../internal/api/handlers/orthrus_handler.go | 60 +++++-- .../api/handlers/orthrus_handler_test.go | 163 +++++++++++++++++- backend/internal/api/routes/routes.go | 8 +- backend/internal/services/orthrus_service.go | 15 +- .../internal/services/orthrus_service_test.go | 80 +++++++-- 5 files changed, 297 insertions(+), 29 deletions(-) diff --git a/backend/internal/api/handlers/orthrus_handler.go b/backend/internal/api/handlers/orthrus_handler.go index 0f84b53c3..1514c5c95 100644 --- a/backend/internal/api/handlers/orthrus_handler.go +++ b/backend/internal/api/handlers/orthrus_handler.go @@ -11,6 +11,7 @@ import ( "github.com/gin-gonic/gin" "gorm.io/gorm" + "github.com/Wikid82/charon/backend/internal/models" "github.com/Wikid82/charon/backend/internal/orthrus" "github.com/Wikid82/charon/backend/internal/services" ) @@ -22,13 +23,21 @@ type orthrusProxyStatusResolver interface { // OrthrusHandler handles REST requests for Orthrus agent management. type OrthrusHandler struct { - svc *services.OrthrusService - proxyResolver orthrusProxyStatusResolver + svc *services.OrthrusService + proxyResolver orthrusProxyStatusResolver + securityService *services.SecurityService } // NewOrthrusHandler creates an OrthrusHandler backed by the given service. -func NewOrthrusHandler(orthrsuSvc *services.OrthrusService) *OrthrusHandler { - return &OrthrusHandler{svc: orthrsuSvc} +// securityService is used to emit an audit entry whenever an operator +// toggles an agent's write-mode flag (see Patch) — a separate, handler-level +// concern from the per-request write-path audit entries Muzzle emits +// directly via the AuditLogger interface (see muzzle.go), following this +// codebase's existing convention of logging admin-initiated audit events at +// the handler layer rather than inside the service (compare +// security_handler.go, crowdsec_handler.go). +func NewOrthrusHandler(orthrsuSvc *services.OrthrusService, securityService *services.SecurityService) *OrthrusHandler { + return &OrthrusHandler{svc: orthrsuSvc, securityService: securityService} } // SetProxyResolver wires a live OrthrusServer so that GetProxyStatus can @@ -107,9 +116,14 @@ type patchAgentRequest struct { DeviceID *string `json:"device_id"` ResolvedAddress *string `json:"resolved_address"` ExternalProxyPort *int `json:"external_proxy_port"` + WriteEnabled *bool `json:"write_enabled"` } -// Patch applies a partial update to an Orthrus agent. +// Patch applies a partial update to an Orthrus agent. If WriteEnabled is +// present in the request, this is the one field on this endpoint whose +// change is itself security-relevant enough to warrant its own audit entry +// (distinct from, and in addition to, the per-write-request audit entries +// Muzzle emits directly for actual proxied traffic — see muzzle.go). func (h *OrthrusHandler) Patch(c *gin.Context) { uuid := c.Param("uuid") var req patchAgentRequest @@ -117,7 +131,7 @@ func (h *OrthrusHandler) Patch(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } - agent, err := h.svc.Patch(uuid, req.Name, req.HecateTunnelUUID, req.DeviceID, req.ResolvedAddress, req.ExternalProxyPort) + agent, err := h.svc.Patch(uuid, req.Name, req.HecateTunnelUUID, req.DeviceID, req.ResolvedAddress, req.ExternalProxyPort, req.WriteEnabled) if err != nil { if errors.Is(err, gorm.ErrRecordNotFound) { c.JSON(http.StatusNotFound, gin.H{"error": "agent not found"}) @@ -126,6 +140,21 @@ func (h *OrthrusHandler) Patch(c *gin.Context) { } return } + + if req.WriteEnabled != nil && h.securityService != nil { + action := "orthrus_write_disabled" + if *req.WriteEnabled { + action = "orthrus_write_enabled" + } + _ = h.securityService.LogAudit(&models.SecurityAudit{ + Actor: actorFromContext(c), + Action: action, + EventCategory: "orthrus_write", + ResourceUUID: uuid, + Details: fmt.Sprintf(`{"agent_name":%q}`, agent.Name), + }) + } + c.JSON(http.StatusOK, agent) } @@ -206,14 +235,16 @@ func (h *OrthrusHandler) GetProxyStatus(c *gin.Context) { return } resp := gin.H{ - "agent_uuid": agent.UUID, - "agent_online": false, - "configured_port": agent.ExternalProxyPort, - "active": false, - "active_port": 0, - "bind_address": "", - "connection_string": "", - "error": "", + "agent_uuid": agent.UUID, + "agent_online": false, + "configured_port": agent.ExternalProxyPort, + "configured_write_enabled": agent.WriteEnabled, + "active_write_enabled": false, + "active": false, + "active_port": 0, + "bind_address": "", + "connection_string": "", + "error": "", } if h.proxyResolver != nil { if status, ok := h.proxyResolver.GetExternalProxyStatus(uuid); ok { @@ -221,6 +252,7 @@ func (h *OrthrusHandler) GetProxyStatus(c *gin.Context) { resp["active"] = status.Active resp["active_port"] = status.ActivePort resp["bind_address"] = status.BoundAddress + resp["active_write_enabled"] = status.WriteEnabled if status.Active && status.ActivePort > 0 { resp["connection_string"] = fmt.Sprintf("tcp://%s:%d", resolveExternalProxyHost(c), status.ActivePort) } diff --git a/backend/internal/api/handlers/orthrus_handler_test.go b/backend/internal/api/handlers/orthrus_handler_test.go index 9b033e8c8..15e5bf698 100644 --- a/backend/internal/api/handlers/orthrus_handler_test.go +++ b/backend/internal/api/handlers/orthrus_handler_test.go @@ -27,7 +27,7 @@ func openOrthrusTestDB(t *testing.T) *gorm.DB { Logger: glogger.Default.LogMode(glogger.Silent), }) require.NoError(t, err) - require.NoError(t, db.AutoMigrate(&models.OrthrusAgent{})) + require.NoError(t, db.AutoMigrate(&models.OrthrusAgent{}, &models.SecurityAudit{})) t.Cleanup(func() { sqlDB, _ := db.DB() if sqlDB != nil { @@ -51,7 +51,31 @@ func newOrthrusTestSetup(t *testing.T) (*OrthrusHandler, *gorm.DB) { t.Cleanup(srv.Stop) svc := services.NewOrthrusService(db, srv) - return NewOrthrusHandler(svc), db + securityService := services.NewSecurityService(db) + t.Cleanup(securityService.Close) + return NewOrthrusHandler(svc, securityService), db +} + +// newOrthrusTestSetupWithSecurity is a variant of newOrthrusTestSetup that +// also returns the *services.SecurityService, for tests that need to +// Flush() and query the audit trail produced by write_enabled toggles. +func newOrthrusTestSetupWithSecurity(t *testing.T) (*OrthrusHandler, *services.SecurityService) { + t.Helper() + dir := t.TempDir() + caPath := filepath.Join(dir, "ca") + require.NoError(t, os.MkdirAll(caPath, 0o700)) + + db := openOrthrusTestDB(t) + ca, err := orthrus.NewInternalCA(caPath) + require.NoError(t, err) + srv, err := orthrus.NewOrthrusServer(db, ca) + require.NoError(t, err) + t.Cleanup(srv.Stop) + + svc := services.NewOrthrusService(db, srv) + securityService := services.NewSecurityService(db) + t.Cleanup(securityService.Close) + return NewOrthrusHandler(svc, securityService), securityService } func TestOrthrusHandler_List_Empty(t *testing.T) { @@ -619,6 +643,141 @@ func TestOrthrusHandler_PatchAgent_ExternalProxyPort_Invalid(t *testing.T) { assert.Equal(t, http.StatusBadRequest, w.Code) } +func TestOrthrusHandler_PatchAgent_WriteEnabled_EmitsAuditEntry(t *testing.T) { + h, securityService := newOrthrusTestSetupWithSecurity(t) + + wProv := httptest.NewRecorder() + cProv, _ := gin.CreateTestContext(wProv) + cProv.Request = httptest.NewRequest(http.MethodPost, "/management/orthrus/agents", + bytes.NewBufferString(`{"name":"write-mode-agent"}`)) + cProv.Request.Header.Set("Content-Type", "application/json") + h.Provision(cProv) + require.Equal(t, http.StatusCreated, wProv.Code) + var provisioned map[string]any + require.NoError(t, json.Unmarshal(wProv.Body.Bytes(), &provisioned)) + agentUUID := provisioned["agent"].(map[string]any)["uuid"].(string) + + body, _ := json.Marshal(map[string]bool{"write_enabled": true}) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPatch, "/management/orthrus/agents/"+agentUUID, + bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "uuid", Value: agentUUID}} + h.Patch(c) + + assert.Equal(t, http.StatusOK, w.Code) + var resp models.OrthrusAgent + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.True(t, resp.WriteEnabled) + + securityService.Flush() + entries, total, err := securityService.ListAuditLogs(services.AuditLogFilter{ + EventCategory: "orthrus_write", + ResourceUUID: agentUUID, + }, 1, 10) + require.NoError(t, err) + require.EqualValues(t, 1, total) + assert.Equal(t, "orthrus_write_enabled", entries[0].Action) + assert.Contains(t, entries[0].Details, "write-mode-agent") +} + +func TestOrthrusHandler_PatchAgent_WriteDisabled_EmitsAuditEntry(t *testing.T) { + h, securityService := newOrthrusTestSetupWithSecurity(t) + + wProv := httptest.NewRecorder() + cProv, _ := gin.CreateTestContext(wProv) + cProv.Request = httptest.NewRequest(http.MethodPost, "/management/orthrus/agents", + bytes.NewBufferString(`{"name":"write-mode-agent-2"}`)) + cProv.Request.Header.Set("Content-Type", "application/json") + h.Provision(cProv) + require.Equal(t, http.StatusCreated, wProv.Code) + var provisioned map[string]any + require.NoError(t, json.Unmarshal(wProv.Body.Bytes(), &provisioned)) + agentUUID := provisioned["agent"].(map[string]any)["uuid"].(string) + + body, _ := json.Marshal(map[string]bool{"write_enabled": false}) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPatch, "/management/orthrus/agents/"+agentUUID, + bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "uuid", Value: agentUUID}} + h.Patch(c) + + assert.Equal(t, http.StatusOK, w.Code) + + securityService.Flush() + entries, total, err := securityService.ListAuditLogs(services.AuditLogFilter{ + EventCategory: "orthrus_write", + ResourceUUID: agentUUID, + }, 1, 10) + require.NoError(t, err) + require.EqualValues(t, 1, total) + assert.Equal(t, "orthrus_write_disabled", entries[0].Action) +} + +func TestOrthrusHandler_PatchAgent_NoWriteEnabledField_NoAuditEntry(t *testing.T) { + h, securityService := newOrthrusTestSetupWithSecurity(t) + + wProv := httptest.NewRecorder() + cProv, _ := gin.CreateTestContext(wProv) + cProv.Request = httptest.NewRequest(http.MethodPost, "/management/orthrus/agents", + bytes.NewBufferString(`{"name":"no-audit-agent"}`)) + cProv.Request.Header.Set("Content-Type", "application/json") + h.Provision(cProv) + require.Equal(t, http.StatusCreated, wProv.Code) + var provisioned map[string]any + require.NoError(t, json.Unmarshal(wProv.Body.Bytes(), &provisioned)) + agentUUID := provisioned["agent"].(map[string]any)["uuid"].(string) + + body, _ := json.Marshal(map[string]string{"name": "renamed"}) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPatch, "/management/orthrus/agents/"+agentUUID, + bytes.NewReader(body)) + c.Request.Header.Set("Content-Type", "application/json") + c.Params = gin.Params{{Key: "uuid", Value: agentUUID}} + h.Patch(c) + + assert.Equal(t, http.StatusOK, w.Code) + + securityService.Flush() + _, total, err := securityService.ListAuditLogs(services.AuditLogFilter{ + EventCategory: "orthrus_write", + ResourceUUID: agentUUID, + }, 1, 10) + require.NoError(t, err) + assert.EqualValues(t, 0, total, "a Patch call that doesn't touch write_enabled must not emit an orthrus_write audit entry") +} + +func TestOrthrusHandler_GetProxyStatus_IncludesWriteEnabledFields(t *testing.T) { + h, _ := newOrthrusTestSetup(t) + + wProv := httptest.NewRecorder() + cProv, _ := gin.CreateTestContext(wProv) + cProv.Request = httptest.NewRequest(http.MethodPost, "/management/orthrus/agents", + bytes.NewBufferString(`{"name":"proxy-status-write-agent"}`)) + cProv.Request.Header.Set("Content-Type", "application/json") + h.Provision(cProv) + require.Equal(t, http.StatusCreated, wProv.Code) + var provisioned map[string]any + require.NoError(t, json.Unmarshal(wProv.Body.Bytes(), &provisioned)) + agentUUID := provisioned["agent"].(map[string]any)["uuid"].(string) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodGet, "/management/orthrus/agents/"+agentUUID+"/proxy-status", http.NoBody) + c.Params = gin.Params{{Key: "uuid", Value: agentUUID}} + h.GetProxyStatus(c) + + assert.Equal(t, http.StatusOK, w.Code) + var resp map[string]any + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &resp)) + assert.Equal(t, false, resp["configured_write_enabled"]) + assert.Equal(t, false, resp["active_write_enabled"]) +} + func TestOrthrusHandler_GetProxyStatus_NilResolver(t *testing.T) { h, _ := newOrthrusTestSetup(t) diff --git a/backend/internal/api/routes/routes.go b/backend/internal/api/routes/routes.go index 44fdaf132..2c46518eb 100644 --- a/backend/internal/api/routes/routes.go +++ b/backend/internal/api/routes/routes.go @@ -504,11 +504,17 @@ func RegisterWithDeps(ctx context.Context, router *gin.Engine, db *gorm.DB, cfg hecateHandler := handlers.NewHecateHandler(hecateSvc) hecateHandler.RegisterRoutes(management) - orthrusHandler := handlers.NewOrthrusHandler(orthrsuSvc) + orthrusHandler := handlers.NewOrthrusHandler(orthrsuSvc, securityService) orthrusHandler.RegisterRoutes(management) if orthrusServer != nil { orthrusHandler.SetProxyResolver(orthrusServer) + // Wires write-path audit logging (Section 3.3.7 of the + // Orthrus write-mode spec) — securityService satisfies + // orthrus.AuditLogger structurally; orthrus never imports + // services directly (see AuditLogger's doc comment in + // muzzle.go for why that import direction is a cycle). + orthrusServer.SetAuditLogger(securityService) } hecateWSHandler := handlers.NewHecateWSHandler(hecateSvc, wsTracker) diff --git a/backend/internal/services/orthrus_service.go b/backend/internal/services/orthrus_service.go index 118a3903d..a00bdc479 100644 --- a/backend/internal/services/orthrus_service.go +++ b/backend/internal/services/orthrus_service.go @@ -81,7 +81,15 @@ func (s *OrthrusService) Get(uuid string) (*models.OrthrusAgent, error) { } // Patch applies a partial update to an agent. Only non-nil fields are written. -func (s *OrthrusService) Patch(uuid string, name, hecateTunnelUUID, deviceID, resolvedAddress *string, externalProxyPort *int) (*models.OrthrusAgent, error) { +// +// writeEnabled has no range validation (unlike externalProxyPort) since a +// bool has no invalid values; the audit entry for this specific field is +// emitted by the caller (OrthrusHandler.Patch), not here, matching this +// codebase's existing convention of logging admin-initiated audit events at +// the handler layer (see e.g. security_handler.go, crowdsec_handler.go) +// rather than growing OrthrusService's dependencies to include +// *services.SecurityService and a *gin.Context-derived actor. +func (s *OrthrusService) Patch(uuid string, name, hecateTunnelUUID, deviceID, resolvedAddress *string, externalProxyPort *int, writeEnabled *bool) (*models.OrthrusAgent, error) { updates := map[string]interface{}{} if name != nil { trimmed := strings.TrimSpace(*name) @@ -106,6 +114,9 @@ func (s *OrthrusService) Patch(uuid string, name, hecateTunnelUUID, deviceID, re } updates["external_proxy_port"] = port } + if writeEnabled != nil { + updates["write_enabled"] = *writeEnabled + } if len(updates) == 0 { return s.Get(uuid) } @@ -117,7 +128,7 @@ func (s *OrthrusService) Patch(uuid string, name, hecateTunnelUUID, deviceID, re // Rename updates the display name of an agent (backward-compat wrapper around Patch). func (s *OrthrusService) Rename(uuid, newName string) (*models.OrthrusAgent, error) { - return s.Patch(uuid, &newName, nil, nil, nil, nil) + return s.Patch(uuid, &newName, nil, nil, nil, nil, nil) } // Delete removes an agent from the database (does not revoke/disconnect first). diff --git a/backend/internal/services/orthrus_service_test.go b/backend/internal/services/orthrus_service_test.go index f7879eaa5..4c8efa116 100644 --- a/backend/internal/services/orthrus_service_test.go +++ b/backend/internal/services/orthrus_service_test.go @@ -289,7 +289,7 @@ func TestOrthrusService_Patch_NameOnly(t *testing.T) { require.NoError(t, err) newName := "updated-name" - got, err := svc.Patch(agent.UUID, &newName, nil, nil, nil, nil) + got, err := svc.Patch(agent.UUID, &newName, nil, nil, nil, nil, nil) require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, "updated-name", got.Name) @@ -307,7 +307,7 @@ func TestOrthrusService_Patch_ProviderFields(t *testing.T) { deviceID := "device-id-456" resolved := "10.0.0.1" - got, err := svc.Patch(agent.UUID, nil, &tunnelUUID, &deviceID, &resolved, nil) + got, err := svc.Patch(agent.UUID, nil, &tunnelUUID, &deviceID, &resolved, nil, nil) require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, "provider-agent", got.Name) @@ -327,7 +327,7 @@ func TestOrthrusService_Patch_BlankName(t *testing.T) { require.NoError(t, err) blank := " " - _, err = svc.Patch(agent.UUID, &blank, nil, nil, nil, nil) + _, err = svc.Patch(agent.UUID, &blank, nil, nil, nil, nil, nil) require.Error(t, err) assert.Contains(t, err.Error(), "cannot be blank") } @@ -339,7 +339,7 @@ func TestOrthrusService_Patch_EmptyUpdate(t *testing.T) { agent, _, err := svc.Provision("no-change-agent") require.NoError(t, err) - got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, nil) + got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, nil, nil) require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, "no-change-agent", got.Name) @@ -351,7 +351,7 @@ func TestOrthrusService_Patch_UnknownUUID(t *testing.T) { svc := services.NewOrthrusService(db, setupOrthrusServer(t, db)) newName := "irrelevant" - _, err := svc.Patch("00000000-0000-0000-0000-000000000000", &newName, nil, nil, nil, nil) + _, err := svc.Patch("00000000-0000-0000-0000-000000000000", &newName, nil, nil, nil, nil, nil) require.Error(t, err) assert.ErrorIs(t, err, gorm.ErrRecordNotFound) } @@ -378,7 +378,7 @@ func TestOrthrusService_Patch_ExternalProxyPort_Zero(t *testing.T) { require.NoError(t, err) port := 0 - got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, &port) + got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, &port, nil) require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, 0, got.ExternalProxyPort) @@ -392,7 +392,7 @@ func TestOrthrusService_Patch_ExternalProxyPort_Valid(t *testing.T) { require.NoError(t, err) port := 2375 - got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, &port) + got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, &port, nil) require.NoError(t, err) require.NotNil(t, got) assert.Equal(t, 2375, got.ExternalProxyPort) @@ -406,7 +406,7 @@ func TestOrthrusService_Patch_ExternalProxyPort_TooLow(t *testing.T) { require.NoError(t, err) port := 1023 - _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port) + _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port, nil) require.Error(t, err) assert.Contains(t, err.Error(), "external_proxy_port") } @@ -419,7 +419,7 @@ func TestOrthrusService_Patch_ExternalProxyPort_TooHigh(t *testing.T) { require.NoError(t, err) port := 70000 - _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port) + _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port, nil) require.Error(t, err) assert.Contains(t, err.Error(), "external_proxy_port") } @@ -432,7 +432,67 @@ func TestOrthrusService_Patch_ExternalProxyPort_Negative(t *testing.T) { require.NoError(t, err) port := -1 - _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port) + _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, &port, nil) require.Error(t, err) assert.Contains(t, err.Error(), "external_proxy_port") } + +func TestOrthrusService_Patch_WriteEnabled_DefaultsFalse(t *testing.T) { + db := setupOrthrusTestDB(t) + svc := services.NewOrthrusService(db, setupOrthrusServer(t, db)) + + agent, _, err := svc.Provision("write-mode-default") + require.NoError(t, err) + assert.False(t, agent.WriteEnabled) +} + +func TestOrthrusService_Patch_WriteEnabled_True(t *testing.T) { + db := setupOrthrusTestDB(t) + svc := services.NewOrthrusService(db, setupOrthrusServer(t, db)) + + agent, _, err := svc.Provision("write-mode-enable") + require.NoError(t, err) + + enabled := true + got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, nil, &enabled) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.WriteEnabled) +} + +func TestOrthrusService_Patch_WriteEnabled_BackToFalse(t *testing.T) { + db := setupOrthrusTestDB(t) + svc := services.NewOrthrusService(db, setupOrthrusServer(t, db)) + + agent, _, err := svc.Provision("write-mode-disable") + require.NoError(t, err) + + enabled := true + _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, nil, &enabled) + require.NoError(t, err) + + disabled := false + got, err := svc.Patch(agent.UUID, nil, nil, nil, nil, nil, &disabled) + require.NoError(t, err) + require.NotNil(t, got) + assert.False(t, got.WriteEnabled) +} + +func TestOrthrusService_Patch_WriteEnabled_NilLeavesUnchanged(t *testing.T) { + db := setupOrthrusTestDB(t) + svc := services.NewOrthrusService(db, setupOrthrusServer(t, db)) + + agent, _, err := svc.Provision("write-mode-untouched") + require.NoError(t, err) + + enabled := true + _, err = svc.Patch(agent.UUID, nil, nil, nil, nil, nil, &enabled) + require.NoError(t, err) + + newName := "renamed-agent" + got, err := svc.Patch(agent.UUID, &newName, nil, nil, nil, nil, nil) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "renamed-agent", got.Name) + assert.True(t, got.WriteEnabled, "write_enabled must be untouched by an unrelated Patch call") +} From 30cf1c08033b7b7a5db0884633a84f322c9fb550 Mon Sep 17 00:00:00 2001 From: Jeremy Hatfield Date: Mon, 20 Jul 2026 12:09:04 +0000 Subject: [PATCH 06/22] feat(orthrus): add write-mode UI, audit-log deep-linking, i18n MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section 3.5 of docs/plans/current_spec.md: - OrthrusAgent/PatchAgentRequest/ExternalProxyStatus types gain write_enabled / configured_write_enabled / active_write_enabled, matching the backend response shapes from the previous two commits. - New AgentWriteModeDialog, deliberately separate from AgentExternalProxyDialog rather than a section bolted onto it (see in-file rationale: transport-layer vs. permissions-escalation concerns, each with independent "configured vs. active" state). Toggling on requires typing the agent's exact current name before Save enables; toggling off requires no confirmation. Shows the fixed, non-editable list of permitted operations, a security warning distinct from the external-proxy dialog's, a reconnect notice mirroring the existing configuredDiffersFromActive pattern, and a link into a pre-filtered audit-log view. - OrthrusAgentManager gains a write-mode action button (ShieldCheck icon) and a WRITE badge, alongside the existing PROXY badge. - AuditLogs.tsx now initializes its filter state from the URL query string on mount (event_category, resource_uuid, actor, action, start_date, end_date) via react-router-dom's useSearchParams, auto-expanding the filter panel when any are present — closes a real, previously undocumented deep-linking gap found during this feature's research: a link into a filtered audit view landed unfiltered. Generic fix, not Orthrus-specific. - EventCategory/AuditAction TS unions extended with orthrus_write and its five action values. - i18n: new hecate.writeMode.* keys and agentManager.writeMode across all five locales (en fully localized; fr/de/es/zh carry English placeholder text for the new keys, matching this repo's existing handling of other not-yet-translated strings like configureProxy). Full frontend suite green: 2988 tests passed, 0 failures. Type-check and production build both clean. --- frontend/src/api/auditLogs.ts | 13 +- frontend/src/api/orthrus.ts | 10 + .../hecate/AgentWriteModeDialog.tsx | 180 ++++++++++++++++ .../components/hecate/OrthrusAgentManager.tsx | 36 +++- .../AgentExternalProxyDialog.test.tsx | 13 ++ .../AgentProviderAssignDialog.test.tsx | 1 + .../__tests__/AgentWriteModeDialog.test.tsx | 192 ++++++++++++++++++ .../__tests__/OrthrusAgentManager.test.tsx | 63 ++++++ .../src/hooks/__tests__/useOrthrus.test.tsx | 3 + frontend/src/locales/de/translation.json | 24 ++- frontend/src/locales/en/translation.json | 24 ++- frontend/src/locales/es/translation.json | 24 ++- frontend/src/locales/fr/translation.json | 24 ++- frontend/src/locales/zh/translation.json | 24 ++- frontend/src/pages/AuditLogs.tsx | 31 ++- .../src/pages/__tests__/AuditLogs.test.tsx | 56 +++++ 16 files changed, 708 insertions(+), 10 deletions(-) create mode 100644 frontend/src/components/hecate/AgentWriteModeDialog.tsx create mode 100644 frontend/src/components/hecate/__tests__/AgentWriteModeDialog.test.tsx diff --git a/frontend/src/api/auditLogs.ts b/frontend/src/api/auditLogs.ts index 827718a60..b0c150b32 100644 --- a/frontend/src/api/auditLogs.ts +++ b/frontend/src/api/auditLogs.ts @@ -1,7 +1,13 @@ import client from './client' /** Audit log event category */ -export type EventCategory = 'dns_provider' | 'certificate' | 'proxy_host' | 'user' | 'system' +export type EventCategory = + | 'dns_provider' + | 'certificate' + | 'proxy_host' + | 'user' + | 'system' + | 'orthrus_write' /** Audit log action type */ export type AuditAction = @@ -18,6 +24,11 @@ export type AuditAction = | 'user_login' | 'user_logout' | 'settings_update' + | 'orthrus_write_allowed' + | 'orthrus_write_blocked' + | 'orthrus_write_rate_limited' + | 'orthrus_write_enabled' + | 'orthrus_write_disabled' /** Represents a single audit log entry */ export interface AuditLog { diff --git a/frontend/src/api/orthrus.ts b/frontend/src/api/orthrus.ts index 020f72bc9..3fd46c958 100644 --- a/frontend/src/api/orthrus.ts +++ b/frontend/src/api/orthrus.ts @@ -18,6 +18,9 @@ export interface OrthrusAgent { resolved_address?: string; // External Docker proxy port (0 = disabled, 1024–65535 = enabled) external_proxy_port: number; + // Opt-in write access to a fixed set of Docker write endpoints through + // this agent's External Docker Proxy. false = read-only (default). + write_enabled: boolean; } export interface PatchAgentRequest { @@ -26,6 +29,7 @@ export interface PatchAgentRequest { device_id?: string | null; resolved_address?: string | null; external_proxy_port?: number; + write_enabled?: boolean; } export interface ExternalProxyStatus { @@ -37,6 +41,12 @@ export interface ExternalProxyStatus { bind_address: string; connection_string: string; error: string; + // configured_write_enabled reflects the DB value; active_write_enabled + // reflects the value the currently-connected session actually negotiated + // at its last reconnect. The two can differ if an operator toggled + // write_enabled while the agent was already connected — see reconnectNotice. + configured_write_enabled: boolean; + active_write_enabled: boolean; } export interface ProvisionAgentRequest { diff --git a/frontend/src/components/hecate/AgentWriteModeDialog.tsx b/frontend/src/components/hecate/AgentWriteModeDialog.tsx new file mode 100644 index 000000000..b566d02de --- /dev/null +++ b/frontend/src/components/hecate/AgentWriteModeDialog.tsx @@ -0,0 +1,180 @@ +import { AlertTriangle } from 'lucide-react'; +import { useEffect, useState } from 'react'; +import { useTranslation } from 'react-i18next'; +import { Link } from 'react-router-dom'; + +import { type OrthrusAgent } from '../../api/orthrus'; +import { useAgentProxyStatus, usePatchAgent } from '../../hooks/useOrthrus'; +import { Button } from '../ui/Button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '../ui/Dialog'; +import { Input } from '../ui/Input'; +import { Switch } from '../ui/Switch'; + +interface AgentWriteModeDialogProps { + agent: OrthrusAgent; + open: boolean; + onClose: () => void; +} + +const WRITE_MODE_OPERATION_KEYS = ['pull', 'start', 'stop', 'restart', 'remove', 'recreate'] as const; + +/** + * Write-mode dialog is intentionally separate from AgentExternalProxyDialog + * rather than a section bolted onto it — the port dialog governs a + * transport-layer setting (is there a TCP port bound at all), this dialog + * governs a permissions-escalation setting (what that tunnel is allowed to + * do). Keeping them as two components with minimal, independent local state + * is more testable than one component tracking two loosely-related + * "configured vs. active" pairs. See docs/plans/current_spec.md Section 3.5.1. + */ +export function AgentWriteModeDialog({ agent, open, onClose }: AgentWriteModeDialogProps) { + const { t } = useTranslation(); + const { mutate: patch, isPending } = usePatchAgent(); + + const isOnline = agent.status === 'online'; + + const { data: proxyStatus } = useAgentProxyStatus(agent.uuid, open && isOnline); + + const [desiredEnabled, setDesiredEnabled] = useState(agent.write_enabled); + const [confirmText, setConfirmText] = useState(''); + + useEffect(() => { + if (open) { + setDesiredEnabled(agent.write_enabled); + setConfirmText(''); + } + }, [open, agent.write_enabled]); + + // Only turning the toggle ON (from an off starting point) requires the + // typed-name confirmation gate — disabling is strictly safety-increasing + // and needs no extra friction. This is why the check compares against + // agent.write_enabled (the value the dialog opened with), not merely + // "is desiredEnabled true": flipping on then back off within the same + // dialog session must not require typing anything either. + const requiresConfirmation = desiredEnabled && !agent.write_enabled; + const confirmMatches = confirmText.trim() === agent.name; + const canSave = !requiresConfirmation || confirmMatches; + + const handleSave = () => { + if (!canSave) return; + patch( + { uuid: agent.uuid, req: { write_enabled: desiredEnabled } }, + { onSuccess: onClose }, + ); + }; + + const configuredDiffersFromActive = + proxyStatus !== undefined && proxyStatus.active_write_enabled !== agent.write_enabled; + + const auditLogHref = `/audit-logs?resource_uuid=${encodeURIComponent(agent.uuid)}&event_category=orthrus_write`; + + return ( + !isOpen && onClose()}> + + + {t('hecate.writeMode.title', { name: agent.name })} + {t('hecate.writeMode.description')} + + +
+ {/* Toggle */} +
+ + {t('hecate.writeMode.toggleLabel')} + + { + setDesiredEnabled(checked); + if (!checked) setConfirmText(''); + }} + disabled={isPending} + /> +
+ + {/* Typed-name confirmation — only shown when enabling from off */} + {requiresConfirmation && ( +
+ + setConfirmText(e.target.value)} + placeholder={t('hecate.writeMode.confirmPlaceholder')} + disabled={isPending} + aria-describedby="write-mode-confirm-hint" + /> +

+ {t('hecate.writeMode.confirmHint')} +

+
+ )} + + {/* Security warning — distinct from AgentExternalProxyDialog's + network-exposure warning: different DOM node, different copy, + describes a permissions escalation rather than a network + reachability change. */} +
+
+ + {/* Fixed, non-editable list of permitted operations — shown only + while the toggle is (or is about to be) on. */} + {desiredEnabled && ( +
+

+ {t('hecate.writeMode.permittedOperationsHeading')} +

+
    + {WRITE_MODE_OPERATION_KEYS.map((key) => ( +
  • {t(`hecate.writeMode.operations.${key}`)}
    • + ))} +
+
+ )} + + {configuredDiffersFromActive && ( +

+ {t('hecate.writeMode.reconnectNotice')} +

+ )} + + + {t('hecate.writeMode.viewAuditLog')} + +
+ + + + + + + + ); +} diff --git a/frontend/src/components/hecate/OrthrusAgentManager.tsx b/frontend/src/components/hecate/OrthrusAgentManager.tsx index 95ba9e2a1..c6a0eedb4 100644 --- a/frontend/src/components/hecate/OrthrusAgentManager.tsx +++ b/frontend/src/components/hecate/OrthrusAgentManager.tsx @@ -1,9 +1,10 @@ -import { Check, Link2, Pencil, Settings, Trash2, X } from 'lucide-react'; +import { Check, Link2, Pencil, Settings, ShieldCheck, Trash2, X } from 'lucide-react'; import { useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { AgentExternalProxyDialog } from './AgentExternalProxyDialog'; import { AgentProviderAssignDialog } from './AgentProviderAssignDialog'; +import { AgentWriteModeDialog } from './AgentWriteModeDialog'; import { type OrthrusAgent } from '../../api/orthrus'; import { useDeleteAgent, useRenameAgent } from '../../hooks/useOrthrus'; import { Badge } from '../ui/Badge'; @@ -37,9 +38,16 @@ interface AgentRowProps { onDelete: (uuid: string, name: string) => void; onAssignProvider: (agent: OrthrusAgent) => void; onConfigureProxy: (agent: OrthrusAgent) => void; + onConfigureWriteMode: (agent: OrthrusAgent) => void; } -const AgentRow = ({ agent, onDelete, onAssignProvider, onConfigureProxy }: AgentRowProps) => { +const AgentRow = ({ + agent, + onDelete, + onAssignProvider, + onConfigureProxy, + onConfigureWriteMode, +}: AgentRowProps) => { const { t } = useTranslation(); const { mutate: rename, isPending: isRenaming } = useRenameAgent(); const [editing, setEditing] = useState(false); @@ -143,6 +151,11 @@ const AgentRow = ({ agent, onDelete, onAssignProvider, onConfigureProxy }: Agent PROXY )} + {agent.write_enabled && ( + + WRITE + + )} @@ -178,6 +191,15 @@ const AgentRow = ({ agent, onDelete, onAssignProvider, onConfigureProxy }: Agent >