feat(orthrus): write-mode support + muzzle allowlist parity (#1160, #1161)#1166
Open
Wikid82 wants to merge 31 commits into
Open
feat(orthrus): write-mode support + muzzle allowlist parity (#1160, #1161)#1166Wikid82 wants to merge 31 commits into
Wikid82 wants to merge 31 commits into
Conversation
…view 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.
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.
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.
…tion, rate limiting, and audit logging 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.
…it logging 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).
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.
…y claims Final commit for the Orthrus write-mode feature (docs/plans/current_spec.md): - tests/orthrus-write-mode.spec.ts: un-skip all Phase 1 specs now that the UI exists. Fixed two issues found only by running against the real app (not visible from code review): the Switch component's underlying <input role="switch"> is sr-only with near-zero hit area, so clicking it directly (even with force:true) can land on a clipped coordinate — clicking the wrapping <label> instead is both more robust and closer to real user interaction. Also split the "enable succeeds" test in two: a real save always closes the dialog (matching AgentExternalProxyDialog's existing pattern), so the reconnect notice can only be observed on a later re-open, not within the same session — the original single test's premise didn't match the dialog's actual (correct) behavior. - docs/features/orthrus.md: rewrites the three absolute "cannot be changed" / "no way to turn it off" claims (Section 2.8 of the spec) to accurately describe the read-only default (still unconditional, can't be loosened by accident) alongside the new opt-in, per-agent, audited write capability — including the six-operation fixed list, the typed-confirmation requirement, and the multi-network limitation from Section 4.2. Validation: full backend/agent test suites, GORM scan, and frontend suite already green from prior commits. This commit's own testable surface — tests/orthrus-write-mode.spec.ts — run 3x consecutively against a freshly rebuilt E2E container (9/9 passing each time, no flakiness). Full `npx playwright test --project=firefox` run: 848 passed, 37 skipped, 14 failed — all 14 failures verified via `git diff 0d81fc9..HEAD` to be pre-existing and untouched by any commit in this branch (zero diff on every failing test's file).
…zation divergence (#1160) Add 7 shared corpus entries (backend/internal/orthrus/testdata/muzzle_corpus.json) covering the version-prefix normalization order divergence between backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go: a traversal-disguised version prefix, a non-numeric fake version prefix on a read path/HEAD path/write path, plus three already-agreed traversal/double- slash regression cases migrated from agent-only tests. No production code changes. TestMuzzle_SharedCorpus (backend) passes unchanged. TestFilter_SharedCorpus (agent) fails on exactly the 4 divergence rows -- this is the intentional red state proving the bug before the fix lands in a following commit.
…le (#1160) Extract ServeHTTP's existing strip-then-clean sequence into a named normalizeDockerPath(rawPath string) string helper, purely for naming symmetry with the forthcoming agent-side helper (agent/muzzle/muzzle.go) and to give the upcoming structural parity checker (#1161) a stable, identically-named anchor in both files. No behavior change -- full pre-existing test suite passes unmodified. Adds direct unit tests for normalizeDockerPath covering the two GH #1160 divergence rows (traversal-disguised version prefix, non-numeric fake version prefix) plus existing traversal/double-slash regression cases.
) Introduce normalizeDockerPath in agent/muzzle/muzzle.go, applying versionPrefixRe strip-then-path.Clean in the same order as backend/internal/orthrus/muzzle.go's helper of the same name, and route every allowlist check (read, HEAD /_ping, and write) through it. Collapses the previously-duplicated allowedPatterns list into allowedDockerPaths (exact) + allowedDockerPatterns (dynamic), renames imageDistributionPatterns to allowedDockerPrefixSuffixPatterns, and drops all now-redundant "/v*/..." pattern entries -- version handling is unified through normalizeDockerPath instead of a loose path.Match "v*" wildcard, which required no digits and was strictly more permissive than backend's numeric-anchored ^/v\d+\.\d+ regex. allowWrite's signature changes to accept a pre-normalized path computed once by its caller (Allow), instead of separately re-deriving an "unversioned" value for its exact-path branch only while its pattern branch matched the raw path directly -- closing a second, independent instance of the same divergence class reaching a write endpoint. Turns green the 4 corpus cases added red in the prior commit. Full agent and backend/internal/orthrus test suites pass.
Add scripts/ci/check_muzzle_allowlist_parity.go, a stdlib-only (go/ast, go/parser, go/constant) drift guard that structurally compares all eight paired declarations between backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go: the seven allowlist/body-size-constant data declarations plus versionPrefixRe's regex source string (extracted from its regexp.MustCompile(...) call expression). Fails loudly, not silently, on any declaration it cannot find or extract, and reports every mismatch with the specific missing/extra entries. The existing shared behavioral corpus proves outcome parity for the inputs it contains but cannot catch "a new pattern was added to one file's declaration and nobody added a corresponding corpus case" -- the exact mechanism behind the production incident that motivated this plan. This is a second, independent guard layered on top of the corpus. Wired into lefthook.yml's pre-commit stage (glob-scoped to the two muzzle files) and a new always-run quality-checks.yml job (not gated on agent/** paths, so a lefthook bypass via --no-verify is still caught in CI). Manually verified (throwaway edit + revert, not committed) that the checker independently catches both AST-extraction shapes: an added allowedWritePatterns entry, and a loosened versionPrefixRe regex.
…for agent module (#1161) Extends agent/'s CI parity with backend/ beyond "tests run" (already landed in the write-mode commits) to "tests run, lint enforced, coverage gated": - Move backend/.golangci-fast.yml to repo-root .golangci-fast.yml (single shared source of truth); golangci-lint-fast.sh/-full.sh now loop over both backend and agent. - Split lefthook.yml's go-vet into module-scoped go-vet (backend/**/*.go) and go-vet-agent (agent/**/*.go), and add agent-test-coverage to the manual `testing` pipeline. - Add scripts/agent-test-coverage.sh: same line-coverage-from-coverprofile gate arithmetic as scripts/go-test-coverage.sh, minus encryption-key bootstrapping (agent has none). Threshold calibrated to the module's actual post-this-PR aggregate (65%) per R8 -- agent/leash's pre-existing 44% coverage is explicitly out of scope to raise here. - Add agent-quality job to quality-checks.yml: go vet, golangci-lint (fast, root-shared config), coverage gate -- runs unconditionally on every PR, not gated on agent/** paths. Deliberately omits backend-quality's encryption-key resolution, GORM scan, and Perf Asserts steps (agent has none of those concerns). - Implement the previously-dangling scripts/check-module-coverage.sh (Makefile referenced it but it didn't exist on disk -- a pre-existing, unrelated bug found during this PR's research and fixed alongside the new agent script); Makefile's check-module-coverage echo text corrected from the already-inaccurate "backend + frontend" to "backend + agent". - Add agent/cert/cert_test.go (LoadOrGenerate had 0% coverage despite being genuine ECDSA/x509 logic) and agent/protocol/message_test.go (wire-format constant-value regression guard). - Extend scripts/local-patch-report.sh, backend/cmd/localpatchreport, and backend/internal/patchreport to compute and report agent/ patch coverage: patchreport.ParseUnifiedDiffChangedLines gains a third (agent) return value; ParseGoCoverageProfile and normalizeGoCoveragePath gain a moduleDir parameter (was hardcoded to backend's prefix-stripping rules, which would have silently produced 0% patch coverage for every agent file -- the same class of masked-by-a-different-mechanism bug the Root Cause Analysis Protocol warns about). Verified end-to-end: a local run against a diff touching agent/muzzle/muzzle.go produces a populated, non-zero "Agent" row in local-patch-report.md. - Add agent-codecov job to codecov-upload.yml (uploads agent/coverage.txt under a distinct `agent` flag) and exclude agent/main.go from that flag's Codecov accounting in codecov.yml, mirroring backend/cmd/api/**'s existing entrypoint-exclusion treatment. Also fixes 10 pre-existing errcheck/shadow findings in agent/leash and agent/muzzle (unchecked Close() return values, one shadowed err) -- necessary for the new agent-quality lint gate to pass cleanly rather than inheriting a backlog of findings never checked before this commit. No behavior change: every fix is an explicit `_ = x.Close()` or a renamed local variable. Also fixes make lint-staticcheck-only, broken pre-existing (golangci-lint v2 doesn't support the v1 --disable-all flag) -- switched to --enable-only, found while wiring up the same target for agent.
CHANGELOG.md: add Security entry for the GH #1160 muzzle normalization order fix, and a CI/CD entry summarizing the GH #1161 agent CI parity work (lint/vet/coverage gates, structural allowlist parity checker, agent patch coverage, agent-codecov upload). ARCHITECTURE.md: update the Pre-Commit Checks and Continuous Integration sections to describe agent/'s new lint/vet/coverage parity with backend/, the muzzle allowlist parity checker, and the agent Codecov flag. Also corrects a stale reference to ".pre-commit-config.yaml" (the repo migrated to lefthook.yml; the file no longer exists).
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
…d enforce threshold locally scripts/local-patch-report.sh always resolved its diff baseline to origin/main first, never matching PR #1166's actual GitHub base (development), so the local tool's patch-coverage numbers silently diverged from what Codecov reports on the PR. Separately, the tool computed WARN-level coverage scopes but never failed the process on them, so CLAUDE.md's Definition of Done Step 2 ("local patch coverage preflight is MANDATORY") was unenforceable in practice. - scripts/local-patch-report.sh: three-tier baseline resolution - explicit $CHARON_PATCH_BASELINE override, then `gh pr view`'s real baseRefName, then a development-preferring static heuristic (main is only ever the nightly-promotion target, not the default integration branch). Restructures the Go-tool invocation so a strict-mode non-zero exit still lets the script's own artifact checks run before propagating the real exit code. - backend/cmd/localpatchreport/main.go: new `-advisory` flag (default strict - non-zero exit when any coverage scope is below threshold, checked only after both report artifacts are written and the WARN diagnostic lines are printed, so a failing gate still leaves a full report to read); report.Mode now truthfully reflects "strict" or "advisory" instead of the previous hardcoded "warn" literal. - backend/internal/patchreport: new HasWarnStatus helper, unit-tested independently of os.Exit/CLI parsing. - New scripts/tests/local-patch-report_baseline.bats covering all four baseline-resolution branches (gh success, gh absent, gh erroring, explicit override winning). - .gitignore: cover the backend/localpatchreport compiled binary (previously untracked but unignored stray build artifact). No behavior change to agent/muzzle or agent/leash in this commit - this only fixes the measurement/gating tool itself, so its own validation gate can still correctly report warn/exit 1 against the genuinely-uncovered agent code, proving the fix works before the follow-up commit closes that coverage gap for a real reason.
…flagged by Codecov on PR #1166 Codecov's real patch-coverage comment on PR #1166 flagged agent/muzzle/muzzle.go (81.72%, 9 missing/8 partial) and agent/leash/leash.go (36.36%, 7 missing) - existing, already-shipped fail-closed validation branches and the connection-scoped write-mode dispatch wiring that had never been exercised by any test. No production code changes; test-only. agent/muzzle/muzzle_test.go adds: - 4 new malformed-body cases isolating still-untested fail-closed unmarshal branches in validateContainerCreateBody (NetworkMode, Mounts, Mounts VolumeOptions, HostConfig itself). - TestFilter_Allow_ContainersCreate_EmptyBodyAllowed, locking in the one deliberately permissive branch (empty body allowed through). - TestFilter_ServeProxy_BodyReadError_ReturnsError and _BodyTooLarge_Returns403AndError for ServeProxy's body-handling branches. - TestFilter_ServeProxy_DockerWriteError_ReturnsError for the req.Write(conn) forwarding-failure branch. Forcing specifically the write (not the dial, not the response read) to fail via a real kernel socket is inherently racy with no synchronization hook available from outside ServeProxy; an accept-then-SO_LINGER=0-close approach was tried first per Supervisor review and found flaky (write often succeeds locally before the close propagates). Uses a bounded-retry technique instead (closing the listener before ever accepting, retried on a fresh socket up to 200 times, accepting only the specific error this branch produces) - verified flake-free across 100+ in-process and 40+ freshly-exec'd process runs under -race. agent/leash/leash_test.go adds two integration-style tests proving the write-mode connection-scoped filter dispatch wiring actually executes for a real accepted yamux stream, not just in isolation: - TestLeash_Connect_DockerStreamDispatchesThroughFilter: connect()'s AcceptStream loop -> handleStream's streamTypeDocker dispatch -> handleDockerStream -> filter.ServeProxy -> net.Dial(dockerSock). - TestLeash_Connect_PortForwardStreamDialsTarget: handleStream's streamTypePortForward dispatch -> handlePortForward's successful-dial path and its deferred conn.Close(). Both drive the real exported Leash/Config/Run surface via a test WebSocket server running a yamux server session, mirroring TestLeash_Reconnect's existing harness (NewWSNetConn) rather than adding white-box test surface. Each new test was verified to fail for the intended reason by temporarily breaking its target code path and confirming the failure, then reverting.
Dockhand's standard image-update flow renames the old container out of the way (e.g. to <name>_old) before starting the new one under the original name. POST /containers/*/rename was missing from both dual-maintained write-mode allowlists, so a live update pulled the new image successfully but then 403'd on the rename step. Add the pattern to allowedWritePatterns in both backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go, following the existing no-body/query-param handling already used for /start, /stop, and /restart (Docker's rename endpoint takes the new name via ?name=, not a request body). Covered by new corpus and unit test cases in both modules; scripts/ci/check_muzzle_allowlist_parity.go confirms the two allowlists stay byte-identical.
…nt restart Turning write mode on for an Orthrus agent only takes effect after the agent's next reconnect, but the dialog gave no indication of that at save time -- an operator could enable write access, believe it was live immediately, and be confused when writes still failed until a manual restart. AgentWriteModeDialog now checks the PATCH response's fresh agent status: if write mode was just turned on (off->on transition) and the agent is currently online, fire a toast (via the existing react-hot-toast mechanism, no new dependency) naming the agent and instructing the operator to restart it on its remote machine. No toast fires when turning write mode off, on a no-op save, or for an agent that isn't currently connected, since a not-yet-connected agent will simply pick up the new value on its first connection. New i18n key hecate.writeMode.restartRequiredToast added to all 5 locale files, matching this block's existing precedent of leaving some strings English-only in non-en locales.
… design Append an addendum to the PR #1166 spec documenting both fixes: the dual-allowlist rename gap (root cause, fix, and validation) and the write-mode restart toast's design research (toast mechanism precedent, connection-status source, exact file/function targets, and the Vitest test matrix), for traceability alongside the already-shipped GH #1160/#1161 muzzle-parity work documented earlier in the same file.
…ds in muzzle allowlist Docker's HostConfig echo-back on container recreate keeps surfacing previously-unconsidered fields one at a time, since most HostConfig/ Resources struct fields lack omitempty tags. Adds CgroupnsMode with a value-level check (reject only "host", matching NetworkMode/UsernsMode's shape) plus CPUCount, CPUPercent, IOMaximumIOps, and IOMaximumBandwidth as unconditionally-safe resource-limit fields (Windows-only in Docker's schema but always serialized on Linux daemons too). Mirrors the change across backend/internal/orthrus/muzzle.go and agent/muzzle/muzzle.go with matching test coverage and shared corpus cases, so an update-checker tool recreating containers with these fields set no longer gets rejected. Claude-Session: https://claude.ai/code/session_01GrUDjmb2PcRiQbX7StjqXb
VolumesFrom (host container's mounts inheritance) can't be blanket-allowed like the other recently-added HostConfig fields, since the muzzle has no way to inspect what the *referenced* container's mounts actually are — unlike NetworkMode/UsernsMode/CgroupnsMode, there's no single safe/unsafe value shape. Instead, adds OrthrusAgent.AllowedVolumesFromSources: an explicit, per-agent, operator-configured allowlist of container names/IDs a write-enabled agent may reference via VolumesFrom, negotiated at connect time over a new X-Orthrus-Allowed-Volumes-From handshake header (mirroring how X-Orthrus-Write-Enabled already works) and enforced independently by both muzzles. An unconfigured allowlist keeps VolumesFrom fail-closed by default, same as before this change. Also allows Isolation (Windows-only "default"/"process"/"hyperv" container platform selector) unconditionally — it only selects an isolation technology, never grants new access, and can still appear in a Linux daemon's inspect response like the other Windows-only fields already allowed. Adds a UI control on the write-mode dialog for managing the source allowlist, and unit test coverage across both muzzle implementations, the service/handler layer, and the frontend. Claude-Session: https://claude.ai/code/session_01GrUDjmb2PcRiQbX7StjqXb
PidMode fits the same pattern already established for NetworkMode,
UsernsMode, and CgroupnsMode: a closed value space where most values are
safe but one specific value is a genuine host-escape primitive.
PidMode:"host" shares the host's PID namespace (visibility into, and
signal/ptrace access to, every host process), so validatePidModeValue
accepts only the empty string — unlike the other value-checked fields,
PidMode's only other value ("container:<id>") has the same
unverifiable-target problem as VolumesFrom, so there's no safe non-empty
value to allow.
Proactively extends to IpcMode and UTSMode, PidMode's siblings in the same
namespace-placement doc-comment group: UTSMode mirrors
UsernsMode/CgroupnsMode exactly (binary, reject only "host"); IpcMode gets
an explicit allowlist of its three safe values ("", "shareable", "private")
rather than a denylist, rejecting "host" and "container:<id>" plus any
unrecognized future value by default.
Mirrored across both muzzle implementations with matching unit tests and
shared-corpus cases, verified against the allowlist parity checker.
Claude-Session: https://claude.ai/code/session_01GrUDjmb2PcRiQbX7StjqXb
…rite mode The field-by-field HostConfig allowlist kept surfacing new legitimate-but- unconsidered fields one at a time (CpuQuota, CgroupnsMode, VolumesFrom, PidMode, Runtime, ...) as real update-checker tools echoed back full container configs. Rather than continuing to chase Docker's ~60-field HostConfig schema indefinitely, write mode is now a deliberate full-access operator-consent model: once an agent has write_enabled, every Docker Engine API endpoint and request body is forwarded unconditionally, with no allowlist restricting either. Read-only (write_enabled: false, the default) sessions are completely unaffected — the GET-only path allowlist is unchanged. This removes the entire HostConfig field allowlist, all nine value-level validators, the fixed 7-operation write-endpoint allowlist, and the per-agent VolumesFrom source allowlist added the round before (now meaningless under full access) — from both independently-maintained muzzle implementations. The agent-side proxy also no longer buffers request bodies into memory at all, since nothing inspects body content anymore; requests stream straight through, same as GET traffic already did. scripts/ci/check_muzzle_allowlist_parity.go now compares 4 declarations instead of 8 (the write-endpoint/HostConfig-allowlist targets no longer exist in either file) — its remaining scope is the read-only path-matching logic both filters still share. The write-mode dialog's warning copy is rewritten to state plainly that enabling write mode is equivalent to direct Docker socket access, not a narrow six-operation grant, matching the new reality. Claude-Session: https://claude.ai/code/session_01GrUDjmb2PcRiQbX7StjqXb
Since the full-access write-mode redesign, ServeHTTP routed every request through forwardWrite once write_enabled was true — including plain GET polling. The write-path rate limiter is sized for occasional mutation bursts (5 tokens, refilling one every 2s — "one full update cycle is exactly 5 write calls"), not for a Docker client's routine container list/inspect polling, which runs far more often than that. Once the burst was spent, update-checker tools (e.g. Dockhand) polling GET /containers/json started intermittently getting 429'd, making containers appear to vanish from their UI and reappear on refresh — exactly the reported symptom. GET/HEAD now bypass the write-path branch entirely for write-enabled sessions, forwarded the same unaudited, un-rate-limited way a read-only session's GET traffic already is. Only mutating methods (POST/PUT/PATCH/DELETE) go through forwardWrite's rate limiter and audit log, matching the limiter's actual intent. Claude-Session: https://claude.ai/code/session_01GrUDjmb2PcRiQbX7StjqXb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/ci/check_muzzle_allowlist_parity.go), and brings theagent/Go module up to the same CI bar asbackend/(lint, staticcheck, vet, coverage gate).Root cause (#1160)
Backend (
backend/internal/orthrus/muzzle.go) stripped the/vX.YDocker API version prefix beforepath.Clean; the agent (agent/muzzle/muzzle.go) didpath.Cleanfirst and used a looserpath.Match("v*", ...)wildcard instead of a numeric-anchored regex. Three divergences were found and closed, all agent-more-permissive-than-backend:/foo/../v1.44/images/x/json(the original issue example)/vFOO/containers/json(non-numeric version segment)POST /vFOO/containers/abc/start— same class of bug, but on a write endpoint, once write-mode is enabledThis fix is unaffected by the write-mode redesign below — it lives entirely in
normalizeDockerPath, which still governs the read-only allowlist both filters keep.Design decision (#1161)
Evaluated a shared importable Go package and codegen from a single source-of-truth file; rejected both —
agent/is intentionally a minimal, dependency-freeFROM scratchbinary, and codegen was disproportionate to how rarely this data changes. Instead: ago/ast-based structural drift checker, wired into CI, layered on top of a shared behavioral test corpus (backend/internal/orthrus/testdata/muzzle_corpus.json) consumed by both modules' test suites. Originally diffed 8 paired declarations; now diffs 4 (versionPrefixRe,allowedDockerPaths,allowedDockerPatterns,allowedDockerPrefixSuffixPatterns) — the write-endpoint-allowlist andHostConfig-field-allowlist declarations it used to also compare no longer exist in either file, per the redesign below.Write-mode security model: full-access redesign (supersedes the section below)
Write-mode originally shipped with, and this PR then iteratively hardened, an exhaustive allowlist of specific
HostConfigfields considered safe for/containers/create, plus a fixed 7-operation write-endpoint allowlist. In practice, real update-checker tools (Dockhand-style) recreate containers by echoing back their full previously-inspectedHostConfig, and Docker's ~60-fieldHostConfig/Resourcesschema mostly lacksomitempty— so legitimate, harmless fields kept surfacing one at a time as separate bug reports (CpuQuota,CgroupnsMode,VolumesFrom,PidMode,Runtime, ...), each requiring a new value-classification pass mirrored across two independently-maintained files. The section below documents that iterative hardening process; all of it has since been removed in favor of a different security model, at the operator's explicit request: write mode is now a deliberate full-access, informed-consent grant. Once an agent haswrite_enabled, every Docker Engine API endpoint and every request body is forwarded unconditionally — no field allowlist, no endpoint allowlist. Read-only sessions (write_enabled: false, the default) are completely unaffected.What stayed in place: the read-only GET-path allowlist (unchanged), rate limiting, and audit logging on every write-mode request. What changed in the write-mode dialog: the warning copy now states plainly that enabling write mode is equivalent to direct Docker socket access, replacing the previous copy that described a narrow six-operation grant. The per-agent
VolumesFromsource allowlist added during the hardening pass below was removed as part of this redesign — it becomes meaningless onceHostConfigis unrestricted.HostConfig field allowlist hardening (superseded, kept for history)
Once write-mode landed, real update-checker tools (Dockhand-style) recreating containers by echoing back their full inspected
HostConfigbegan surfacing previously-unconsidered fields one at a time — mostHostConfig/Resourcesstruct fields lackomitempty, so Docker always serializes them even at zero/default values. Rather than fixing each reported field in isolation, every round did a proactive pass over the related Docker HostConfig schema and applied the same safe/value-checked/dangerous classification framework to catch adjacent fields before they became separate bug reports. All changes were mirrored across both muzzle implementations with matching unit tests and shared-corpus cases, verified againstscripts/ci/check_muzzle_allowlist_parity.go. This entire allowlist — every field and validator described below — has been removed; see "Write-mode security model" above.CapDrop,UsernsMode:CapDroponly ever reduces privilege (unlikeCapAdd);UsernsModegot a value-level check rejecting only"host".CpuQuota,ContainerIDFile, +27 other resource-limit/cosmetic/networking-convenience fields: a proactive pass across the CPU/memory/IO/pid resource-limit family, restriction-only fields, cosmetic/metadata fields, and networking-convenience fields.ContainerIDFilegot a value-level check (empty string only).CgroupnsMode,IOMaximumBandwidth+ 3 Windows-only-but-Linux-serialized siblings:CgroupnsModewas reclassified from blanket-excluded to value-checked.VolumesFrom: addedOrthrusAgent.AllowedVolumesFromSources, an explicit per-agent operator-configured allowlist of container names/IDs, negotiated over a handshake header — since removed (see above).Isolation: Windows-only platform selector, was allowed unconditionally.PidMode,IpcMode,UTSMode: each got a value-level check closing their"host"/"container:<id>"values.Test plan
exec/build/prune, and previously-rejectedHostConfigbodies, now succeed underwrite_enabled: true; unchanged underwrite_enabled: false)check_muzzle_allowlist_parity.gopasses (4/4 declarations)AgentWriteModeDialogtests updated for the new warning copy and payload shapego test ./...) and agent (go test ./...) suites green