feat(orthrus): write-mode support + muzzle allowlist parity (#1160, #1161)#1166
Open
Wikid82 wants to merge 22 commits into
Open
feat(orthrus): write-mode support + muzzle allowlist parity (#1160, #1161)#1166Wikid82 wants to merge 22 commits into
Wikid82 wants to merge 22 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.
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 enabledDesign 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 that diffs all 8 paired allowlist declarations (including the normalization regex itself) between the two files, 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.Test plan
check_muzzle_allowlist_parity.gopasses; independently verified its negative-path detection (both a data-mismatch and a regex-mismatch injection were correctly caught)docs/reports/qa_report.md— READY, zero CRITICAL/HIGH findings