HOMELAB-1683: feat(cockpit): LD3.3 sandbox attach bridge, built disabled #579
No reviewers
Labels
No labels
operator-review
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Homelab/infra-core!579
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-1683-attach-bridge"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
LD3.3 slice
LD3.3-code: the sandbox attach bridge's cockpit backend — the PTY pipe, the server wiring, the refusal matrix and theld.audit.attach/v1ledger — built so it lands disabled.Scope is exactly
lab-director/cockpit/**(Go). Noweb/**, noci.yaml, noprod/**, nolab-director/dispatcher/**, no manifests, no RBAC, no CNP.New package
attach/:Registryseam: resolve a sandbox id →{namespace, pod, container, class, ticket, hasTTY, identity}through a narrow read interface, with a recording fixture and the refusing unwired registry that names the unratified RBAC. The live CRD reads are deliberately not built here (they need the ratified RBAC).Executor/Sessionseam:Dial(namespace, pod, container, term)→ bidirectional stdin/stdout +Resize. The client-go impl isremotecommand.NewSPDYExecutorover thepods/execsubresource withTty: trueand aTerminalSizeQueue. Construction reads a kubeconfig (LD_ATTACH_KUBECONFIG, empty = in-cluster) and contacts nothing; the dial is the only network call.P-ATT-00Npolicy line (thegates/sentineldiscipline).ld.audit.attach/v1writer.audit.Entrygains attach kinds/outcomes/fields;Validatenow enforces the schema↔kind↔outcome pairing in both directions and keeps sentinel entries free of attach fields (a claimed schema the entry cannot satisfy is worse than a schema of its own — the existing "attach placeholder" assertion inaudit_test.gobecame a real attach entry plus explicit pairing cases; that is a strengthening, not a weakening).Wire()is the one place a kube client can be constructed. Disabled builds none (TestWireDisabledConstructsNoKubeClientcounts constructions); enabled builds exactly one and refuses per attempt while the registry is unwired.Server (
server/attach.go, wired as root routes, i.e. outsideRequireOperatorso a refused handshake is still a ledger entry):GET /api/v1/attach/{sandbox}/info—{exists, has_tty, attachable, reason, policy, wiring, terminal, policy_lines, audit_recorded}. 200 (attachable:false) when the sandbox exists but cannot attach, 404 unknown, 401/403 for identity, 503 disabled.WebSocket /ws/attach/{sandbox}— text frames are JSON control ({"type":"resize","cols":N,"rows":N},{"type":"close"}), binary frames are terminal bytes both ways;openframe first,closeframe last,errorframe instead of an open when the dial fails after the open was recorded.statusRecorder(the access-log middleware) gained theHijackpassthrough a websocket upgrade needs through the chain — without it every attach handshake would have been a 500.Config:
LD_ATTACH_ENABLED(default false),LD_ATTACH_KUBECONFIG. A kubeconfig set while the flag is nottrueis refused as an ambiguous configuration (same shape as the half-set OIDC block).mainrenders attach in--check's OK lines and in the boot log. It is deliberately not a read-model census row (e79ed6f): the census has a frontend parity guard (web/e2e/shell.spec.tsasserts 4 source rows) and the attach row belongs with the xterm.js pane slice that will render it — see the NOTE atcmd/cockpit/main.go.Why
The LD3.3 ticket's AC is "attach from the app into a live sandbox works and is audit-logged". The ratification it needs (start-up RBAC, CNP delta, mTLS, sentinel arming) is a separate, class-E operator decision (
.rpiv/artifacts/designs/ld3/ratification-package-3.3.md). Splitting code from grant is what lets this land class B/C: the mechanism is complete and tested, the feature is inert.Disabled-by-default posture (the review's main question)
attach_closeperattach_open(a failed dial writes the aborted close), and an attach never opens when its open entry could not be written.Test plan
cd lab-director/cockpit && go vet ./... && go test -race -count=1 ./...→ 192 tests pass, 0 failures (attach32,server47,audit9,config13 new/extended).go build ./...clean.Fixture-driven, no network, no cluster. Covered: the full refusal matrix (disabled · unauthenticated · non-operator · unwired registry · unknown sandbox · no-TTY class · unknown class · ticketless sandbox · malformed resolution · invalid terminal · dial failure · ledger unavailable), the WebSocket protocol over a real socket (upgrade, open frame, byte round-trip, resize reaching the executor, sandbox-side EOF, abrupt client disconnect, protocol error, slow consumer torn down by the write deadline), open/close ledger pairing and the close entry's JSON shape, and the no-kube-client-at-boot proof.
Manual dev-mode evidence (binary,
OIDC_ISSUERunset):GET /api/v1/attach/ld-light-1/info→503{"disabled":true,"attachable":false,"policy":"P-ATT-001","audit_recorded":true,…}+ ald.audit.attach/v1attach_refusedledger line.GET /ws/attach/ld-light-1with websocket upgrade headers →HTTP/1.1 503, no101, noUpgradeheader, refusal recorded.--check→OK attach enabled=false kube_client_constructed=false registry=not-applicable (disabled) kubeconfig=in-cluster.Rollout
Nothing to roll out: no manifests in this PR. The live attach deployment (RBAC
get/listsandboxclaims+pods andcreate pods/execinagent-sandbox-system, the CNP egress delta, the mTLS ruling) is the ratified 3.3 deploy slice; the xterm.js pane is a separate frontend slice after the three-panes PR lands.Adds the attach bridge behind LD_ATTACH_ENABLED (default false): the cockpit/** mechanism, the server wiring and the refusal matrix, with no manifests, no RBAC, no CNP, no dispatcher change and no web/**. attach/ (new): - Registry seam (read-only interface, recording fixture, and the refusing unwired registry that names the unratified RBAC) — the live CRD reads wait for the operator's ratification package §4/§5. - Executor seam + client-go impl: remotecommand.NewSPDYExecutor over pods/exec with Tty:true and a TerminalSizeQueue; construction reads a kubeconfig (LD_ATTACH_KUBECONFIG, empty = in-cluster) and contacts nothing. - Refusal matrix with a P-ATT-001..008 policy book, the class harness allowlist (spawn's own class constants; unknown class refuses), and the ld.audit.attach/v1 writer (audit.Entry gains attach kinds, outcomes and fields; Validate enforces the schema/kind/outcome pairing both ways and keeps sentinel entries free of attach fields). - Wire() is the one place a kube client can be constructed: disabled builds none (asserted by a construction counter), enabled builds one and refuses per attempt while the registry is unwired. server/: - GET /api/v1/attach/{sandbox}/info and WebSocket /ws/attach/{sandbox}, registered outside RequireOperator so that a refused handshake is still a ledger entry; identity resolution, refusals and the WS protocol all audit (open/close/refused), statusLogger gains the Hijack passthrough a websocket upgrade needs through the chain. - One attach_open is answered by exactly one attach_close (a failed dial writes the aborted close), and an attach never opens when its open entry cannot be written. config: LD_ATTACH_ENABLED (default false) + LD_ATTACH_KUBECONFIG; a kubeconfig configured while the flag is off is refused as ambiguous. main: attach declared in the read-model census, in --check's OK lines and in the boot log ("no kube client constructed"). Tests: 192 pass under -race (attach 32, audit 9, server 47, config 13 new or extended); full refusal matrix, WS protocol over a real socket (upgrade/resize/EOF both ways/abrupt disconnect/protocol error/slow consumer), ledger pairing and entry JSON shape.Review 2/2 — protocol & correctness (lens: WS framing, lifecycle, audit pairing, tests)
VERDICT: APPROVE-WITH-CONCERNS
Reviewed at head
ca9022f(verified:git rev-parse origin/plane/HOMELAB-1683-attach-bridge=ca9022f3384444dc951339eaf9e2b88e9059e6f4), againstorigin/epic/LD3...``origin/plane/HOMELAB-1683-attach-bridge(27 files, +5659/-21, all underlab-director/cockpit/). I did not write this code. Line numbers below are the post-change file lines (as reported by the coverage profile andgit show <branch>:<path>).No BLOCKER found: the byte path is binary-clean, frames are disambiguated by frame type, the write budget is real and tested, 50 sessions leak 0 goroutines (measured), and no normal path produces an orphan
attach_open. The concerns below are ledger-fidelity, comment-vs-code drift and test-honesty items.Findings
lab-director/cockpit/server/attach.go:416— the pump's own frame-length check is unreachable, so the misclassification it exists to prevent is what actually happens.conn.SetReadLimit(maxAttachInputFrame)(attach.go:220, 1 MiB) and the inner bound are the same number, and gorilla enforces the limit off the frame header before the payload is materialised (websocket@v1.5.4-…/conn.go:933), returningErrReadLimit— never a too-large payload.ErrReadLimitis not a*CloseError, soisCloseFrame(attach.go:552) is false and the session is recorded asclient_eof/closedinstead ofprotocol_error/aborted. Repro (probe, real socket through the full chain): client sends one binary frame ofmaxAttachInputFrame+1bytes → ledger close entryreason="client_eof" outcome="closed" detail="websocket: read limit exceeded". Evidence:go tool cover -funcshows the line at 0.0% and no test can reach it. Why it matters: the ledger's own vocabulary says a protocol violation is an aborted session; here a violation reads as a clean client hang-up, and theDetailis the only field that tells the truth. The allocation bound itself is fine.lab-director/cockpit/server/attach.go:286-300(writeAttachError, 0.0% coverage) vs the contract in two other places. The handler doc says the post-upgrade refusal is "a{"type":"error"}frame followed by the close frame" (server/attach.go:43), andprotocol.go:23promisesclose"exactly once, last" on every attach — butwriteAttachErrorwrites the error frame and thenconn.Close(); no close frame is ever sent on this path. Any client that implements the documented grammar and waits for the final close frame sees an abnormal socket close instead. Also: this is the only rendering of thedial_failedrow on the wire, and nothing tests it (see the matrix below). Why it matters: the protocol doc is the contract the xterm.js pane slice will implement against, and it describes a frame sequence the server does not produce.lab-director/cockpit/server/attach.go:513-518— the open/close pairing is fail-closed at open and fail-open at close.Bridge.Openaborts the whole attach when the open entry cannot be written (bridge.go:330-345), butfinishAttachlogss.log.Error("the attach close could not be recorded")and carries on: it writes the close frame, closes the socket and tells the client the session ended (reason=… outcome=…) while the ledger still holds anattach_openwith no answer — precisely the stateaudit/audit.go:78-82calls "the one lie an attach ledger must never tell". Not a BLOCKER: the production sink isaudit.LogSink, whose only failure mode isEntry.Validate, and the close entry built byAuditor.Closed(attach/auditor.go:106-146) always satisfies it (ticket scope, non-empty reason defaulted atauditor.go:121, session id always set) — so this needs a future durable sink to become reachable. It still makes the PR's blanket claim "exactly oneattach_closeperattach_open" untrue as stated, and the asymmetry deserves a comment at minimum.cmd/cockpit/main.go:243-275,server/attach.go:394-491. The pumps never observe their context:pumpCtxis only read once, in<-pumpCtx.Done()(attach.go:488), and the request context is not the signal context (main.go:215usessignal.NotifyContext, noServer.BaseContext, noRegisterOnShutdown). Go'shttp.Server.Shutdownexplicitly does not close or wait for hijacked connections. So SIGTERM during a redeploy with a live attach ends the process with the exec stream torn down by the kernel, no close frame, and anattach_openwith noattach_close— a routine operational event, not an exotic one. Fix is a process-level one (aBaseContextcancelled on SIGTERM, plus a<-ctx.Done()leg in the teardownselect); flagging rather than blocking because it is outside the slice's stated scope and the feature ships disabled.server/attach.go:404-413. The input pump loops onconn.ReadMessage()with no deadline; the 10 s budget (attach.go:464) only applies before writes. A client whose TCP is blackholed (lid closed, no FIN) plus a quiet sandbox (a shell at a prompt) therefore holds a sandbox shell open forever, which is exactly whatprotocol.go:63-66says must not happen ("a stalled viewer must not hold a sandbox shell open forever") and whatattach.go:47-49claims is handled. The tested case (TestAttachSlowConsumerIsTornDownAndRecorded) covers the stalled-viewer-with-output case only. Why it matters: the shell is the expensive resource, and this is the unbounded one.lab-director/cockpit/attach/kube.go:99-141,164-227— the real backend is entirely unexecuted.go tool cover -funcon the branch:Dial0.0%,kubeSession.Read/Write/Resize/Close/Wait/finish0.0%,errOrEOF0.0%; only the pure functions (execURL,sizeQueue, construction) are covered. So nothing in the suite would catchTty: false, aStderrset alongsideTty: true(whichremotecommandrefuses), a missingTerminalSizeQueue, or the exec URL losingtty=true. The three claims in the PR description about this file are asserted by reading the constants (TestExecURLIsThePodsExecSubresourcecovers the URL;Tty/queue are only compile-time interface assertions atkube.go:356-359). Per the review brief this is a CONCERN, not a BLOCKER: the flag is off, the dial is the only network call, andexecURL(the RBAC-relevant half) is well tested. Aremotecommand.StreamOptions-shaped seam would have madeTty: trueassertable without a cluster.lab-director/cockpit/audit/audit.go:260— "sentinel entries are kept free of attach fields" is enforced for 4 of 7 fields.Validate's sentinel branch checksSandbox,SessionID,Term,Class, but notBytesIn,BytesOutorDurationMS. Probe (added to the package, run, then removed): aschema=ld.audit.sentinel/v1entry withBytesIn=4096,BytesOut=4096orDurationMS=1234validates with no error, while the same entry carryingSandbox/Class/SessionID/Termis refused. The test that appears to prove purity (server/attach_test.go:879-883) marshals a zero-valued sentinel entry and asserts the JSON has nobytes_in/… key — which passes because ofomitempty, and can only fail if someone dropsomitempty, not if someone sets the field. No writer sets these today, so it is latent; one added||closes it.lab-director/cockpit/server/attach_test.go:474—TestAttachInfoNamesTheUnratifiedRegistrydoes not test the unwired registry. The harness wires a readyRecordingRegistry(sandboxes/registrydefaults atattach_test.go:81-91), so the test assertsattachable:true, 200 and the policy-book length; the in-test comment concedes the point ("the unwired case is asserted in the attach package and in the disabled default above"). Result: theregistry_unwiredrow has no transport-level assertion, and the test name reports coverage that does not exist — worse than no test, because a green here is read as "the enabled-but-unratified posture was checked over HTTP".lab-director/cockpit/attach/bridge_test.go:325—TestMalformedResolutionIsRefusedasserts onlyref.Kind, with no ledger assertion and noDialCount() != 0check. Every sibling row in the same file asserts the entry and the no-dial proof; this one can pass while the refusal is unaudited.attach/protocol.go:58definesCloseReasonStreamErrorandserver/attach.go:474-481maps a non-EOF read error to it with outcomeaborted, but no test in the tree referencesCloseReasonStreamError(grep: 0 hits in*_test.go) — the fixture has no knob to makeSession.Readfail with a real error (FixtureExecutoroffersDialErr/ResizeErronly;EndAfterBanneryields a cleanio.EOF). The line is executed incidentally (line 476 is covered) because teardown closes the pipe under the output pump, but the observable outcome is always already recorded by the other pump. So one of the three disconnect directions is untested at the level where it matters (what the ledger says).lab-director/cockpit/server/middleware.go:210-218— theHijackpassthrough is correct and does not break the access log, but it is redundant, and both justifications in its comment are wrong. Gorilla in use callshttp.NewResponseController(w).Hijack()(server.go:175), which already walksUnwrap()— andstatusRecorder.Unwraphas existed since the SSE fix (middleware.go:204). Proof: renaming the new method toHijackDISABLEDleavesTestAttachSession*/TestAttachWebSocket*/TestAttachSlow*green (vet clean) — the upgrades still work. And the comment's "(the access log line … is whatever status was written before the upgrade (101))" is false: the recorder defaults tohttp.StatusOK(middleware.go:86) and gorilla writesHTTP/1.1 101directly to the hijacked socket without callingWriteHeader(websocket@v1.5.4-…/server.go:228), so an upgraded attach logsstatus=200. Probe against the real chain:level=DEBUG msg=request method=GET status=200 bytes=0for the upgrade,status=503for the refused handshake. Keep the method if you like the fast path; fix the comment.mainrenders attach in the read-model census", butcmd/cockpit/main.go:404-409says the opposite ("deliberately NOT an ExtraSource in the read model's census … the census row lands with the xterm.js pane slice"). The README agrees with the code. The PR text should be corrected — a reviewer looking for the census row will not find it.lab-director/cockpit/cmd/cockpit/main.go:324-325printscheckBridge.Enabled()for bothenabledandkube_client_constructed. The two are equal only becauseWireconstructs an executor iffEnabled; the column is asserted by construction, not measured. Theattach.Wirecall inrunCheckalso constructs the real executor (and reads a cluster credential) when the flag is on, which is defensible for--checkbut is not what the "the constructor is NOT called on the disabled path" comment implies for the enabled one.Claim verification (author's claims vs what I found)
payload([]bytefromReadMessage) goes straight intolive.Exec.Write(payload)(server/attach.go:422); sandbox→client:buf[:n]straight intoconn.WriteMessage(websocket.BinaryMessage, …)(attach.go:465). Nostring()/[]rune()/UTF-8 validation anywhere on the data path (the only conversion isstrings.NewReader(string(payload))insideDecodeClientFrame, control frames only,protocol.go:132). Disambiguation is by frame type only (switch typatattach.go:414); no payload sniffing. No text frame can reachExec.Write: theTextMessagearm only ever callsDecodeClientFrame. Round-trip proven over a real socket:TestAttachSessionRoundTripwrites binary input and reads it back byte-identical as a binary frame (server/attach_test.go:558-564), andTestAttachSessionClosesOnAProtocolErrorproves a hostile text frame ({"type":"exec","command":"rm -rf /"}) is refused, not typed (attach_test.go:723).attach.go:383,464,533), no unbounded queue (writes go straight to the socket, so a stalled reader blocks the writer until the deadline fires),TestAttachSlowConsumerIsTornDownAndRecordeddrives 32 MiB of flood with a 50 ms budget and assertsclient_write_timeout+aborted(attach_test.go:762-793). Refuted for the no-output case: no read deadline/ping after the upgrade, so a blackholed client with a quiet sandbox is not torn down (see finding 5).TestAttachSessionEndsWhenTheClientVanishes(client_eof/closed),TestAttachSessionEndsWhenTheSandboxHangsUp(stream_eof/closed), protocol error (protocol_error/aborted). Termination is structural, not hopeful: whichever pump finishes first cancelspumpCtx, and the teardown is ordered<-pumpCtx.Done()→live.Exec.Close()→conn.SetReadDeadline(time.Now())→wg.Wait()(attach.go:488-491) — both unblocking actions precede the wait, so neither direction can keep copying. The third direction (executor error mid-stream) has no test that can observe it (finding 10). Theerror-frame semantics are implemented (Bridge.Openrefuses after the ledger has the open,bridge.go:347-362; the frame is sent byserver/attach.go:228) but untested on the wire.TestDialFailureClosesTheOpenItWroteasserts open +abortedclose + sharedsession_id+ the dial error inDetail(bridge_test.go:403-428), andLiveSession.closeis idempotent under a mutex (bridge.go:120-148, re-asserted atbridge_test.go:392-398). EveryserveAttachexit path reachesfinishAttach: the open-frame-write failure (attach.go:386-390), and the normal path (attach.go:493). Refuted as an absolute: a close can be lost — on a sink failure at close (logged, not retried:attach.go:513-518) and on process shutdown with a live session (findings 3 and 4). Neither is reachable on the normal path with today'sLogSink; I judged both CONCERN rather than BLOCKER for that reason, and state it here so the call can be overridden deliberately.attach/attach.go:284-315; rows (12) mapping: disabled (bridge.go:307, testedbridge_test.go:115,server/attach_test.go:234,272); unauthenticated (server/attach.go:109, testedattach_test.go:297); non-operator (server/attach.go:112, testedattach_test.go:334); unwired registry (bridge.go:385, testedbridge_test.go:202,wire_test.go:92— not at the transport, despiteattach_test.go:474's name); unknown sandbox (bridge.go:389, tested + matrix); no-TTY class (bridge.go:412, tested + matrix + WS 409 with no dial atattach_test.go:499); unknown class (bridge.go:407, testedbridge_test.go:231quantum-harness+ matrix); ticketless (bridge.go:421, tested + matrix); malformed resolution (bridge.go:398, superficial — kind only); invalid terminal (bridge.go:313/server/attach.go:120, tested both layers); dial failure (bridge.go:347, tested in-package, wire rendering 0%); ledger unavailable (bridge.go:330, testedbridge_test.go:446, wire rendering 0%). The README table lists 10 rows (it folds "unknown class" into the no-TTY row and omits malformed target).attach.FixtureExecutor,attach.RecordingRegistry) as the PR states, and those fixtures are genuinely behavioural (realio.Pipes, real flood/EOF), so the transport claims are real. Butkube.go— the only file where aTty: true/TerminalSizeQueue/Stderrmistake could live — is 0.0% covered (finding 6). Disclosed honestly here: CONCERN, not BLOCKER.192 PASS,1 SKIP(--- SKIP: TestProductionCSPCoversTheRealBuild) = 193 test functions, 192 executed — matching "192 tests pass". Per package (top-level): attach 32, server 48 (47 pass + the 1 skip → the claimed 47), audit 9, config 13 (of which the new attach config test contributes 1 function with 5 subtests). Note these are package totals, not "new" counts.Verified by (commands you actually ran)
git rev-parse origin/plane/HOMELAB-1683-attach-bridge origin/epic/LD3→ca9022f…/6f98509…(head matches the review target).git diff --stat origin/epic/LD3...origin/plane/HOMELAB-1683-attach-bridge→ 27 files, +5659/-21, all underlab-director/cockpit/.cd lab-director/cockpit && go vet ./... && go test -race -count=1 ./...→VET_EXIT=0,TEST_EXIT=0;okfor all 10 packages (attach 1.043s, audit 1.023s, auth 1.026s, config 1.024s, gates 1.036s, plane 1.046s, readmodel 1.023s, server 1.324s, sse 1.279s;cmd/cockpit [no test files]). No race reports.go test -count=1 -v ./... | grep -E '^--- (PASS|FAIL|SKIP)' | awk '{print $2}' | sort | uniq -c→192 PASS:,1 SKIP:.go test -count=1 -v ./attach ./server ./audit ./config | grep -cE '^=== RUN Test[^/]*$'→ 32 / 48 / 9 / 13.go tool cover -func→attach67.2%,server90.0%,audit96.0%,config92.9%;attach/kube.goDial/Read/Write/Resize/Close/Wait/finish/errOrEOFall 0.0%;server/attach.gowriteAttachError0.0%,serveAttach83.3%,handleAttachWS83.3%.ca9022f, since removed) — oversize input frame over a real socket through the full chain → close entryreason="client_eof" outcome="closed" detail="websocket: read limit exceeded"(inner bound unreachable).statusRecorder.Hijackrenamed away,go vet ./server/clean,go test -run 'TestAttachSession|TestAttachWebSocket|TestAttachSlow|TestAttachAuditEntryJSONShape' ./server/→ok(the passthrough is redundant;Unwrapsuffices).Chain(inner, RequestID, Logger(log))→ upgraded wsstatus=200 bytes=0(not 101); refused handshakestatus=503.-race: goroutinesbaseline=3 after 50 sessions=3 (delta=0). No goroutine leak.audit.Entry{Schema: SchemaSentinel, …}withBytesIn/BytesOut/DurationMSset →Validate()returns nil (accepted), whileSandbox/Class/SessionID/Termare refused.grep -rn CloseReasonStreamError --include=*_test.go attach/ server/→ 0 hits.forgejo_get_pr 579→ PR body used for the claim-verification section.No byte corruption, no orphan open on a reachable path, no persistent goroutine leak, and no test that cannot fail was found. The two items I would fix before this is used (rather than merged) are the
client_eof/closedmisclassification (one-line: classifywebsocket.ErrReadLimitasprotocol_error/aborted) and the untested wire rendering of the post-upgradeerrorframe, since the xterm.js slice will code against the documented grammar.Review 1/2 — safety & inertness (lens: provable inertness, flag-off)
VERDICT: APPROVE-WITH-CONCERNS
Inertness holds. I could not break it, and the central claim (disabled ⇒ no kube client, asserted as a call count) is real, not a hoped-for branch. Two enabled-mode findings (one unaudited refusal path, one demonstrably false code comment) keep this from a clean APPROVE; neither violates the flag-off property, scope, or identity rules, so neither is a blocker.
Findings
lab-director/cockpit/server/attach.go:210-214— a failedUpgradeis logged, never audited, so an authenticated attach attempt can leave zero ledger entries — repro: withLD_ATTACH_ENABLED=trueand a resolvable sandbox, sendGET /ws/attach/ld-light-1with a valid operator session but noConnection: Upgradeheader.attachRequestpasses (no refusal written),Inforeturnsattachable:true(a success is deliberately not audited), gorilla replies400, the handler logs"attach upgrade refused"and returns →0ledger entries for an operator who asked for a shell. This is the one early return that skips the audit write; it contradicts the README's "Every refusal is a ledger entry" and P-ATT-005 ("an attempt that leaves no record is indistinguishable from an attempt that never happened"). Flag-off is unaffected (Info returns 503 first, audited), so inertness is intact — this is an enabled-mode completeness gap, not a safety regression.lab-director/cockpit/server/middleware.go:206-211— theHijackdoc-comment's parenthetical "(the access log line this recorder produces is whatever status was written before the upgrade (101))" is false; the access log records 200 with 0 bytes for a successful attach handshake — evidence: gorilla v1.5.4'sUpgradetakeshttp.NewResponseController(w).Hijack()and writes the101handshake straight to the hijackedbufiowriter; it never callsw.WriteHeader, sostatusRecorder(initialisedstatus: http.StatusOKatmiddleware.go:83) keeps 200, and hijacked bytes never reachstatusRecorder.Write— sobytesstays 0. End-to-end probe through the realServer.Handler(): client sawhandshake status = 101, the cockpit loggedmsg=request ... path=/ws/attach/ld-light-1 status=200 bytes=0 duration_ms=0. Why it matters: the comment claims the log is trustworthy for the upgrade, when in fact the only access-log line an attach session produces is indistinguishable from an ordinary 200 and reports no byte count — a monitor/operator can't audit attach sessions from the access log. (I re-derived this twice: a standalone clone ofstatusRecorder+ gorilla, and a temp probe against this diff'sserverpackage; both failed the101claim.)lab-director/cockpit/cmd/cockpit/main.go:324—kube_client_constructed=%tis printed fromcheckBridge.Enabled(), i.e. it restates the flag, it does not measure construction. The real proof is the unit call-count; the operator-visible--checkcolumn is therefore weaker evidence than the README implies ("asserted byTestWireDisabledConstructsNoKubeClient" is true of the test, not of the printed column). A stronger column would print something observed (e.g. anExecutable()/hasExecutoraccessor onBridgethat is structurally nil on the disabled path).lab-director/cockpit/server/attach.go:65—attachUpgraderleavesCheckOriginnil, so same-origin enforcement is implicit (gorilla'scheckSameOrigindefault, confirmed in the vendored source atserver.go:148-153). The comment is accurate today (and correctly discloses that a missing Origin is accepted, which is right for non-browser clients), but a future gorilla default change would silently remove the cross-origin guard with no test to catch it. Consider settingCheckOriginexplicitly to the documented policy.main.go:404NOTE plus the README paragraph, both of which depend on a future slice actually landing the row.Claim verification (author's claims vs what I found)
attach/wire.go:56returns before touchingopts.NewExecutor; the only production reach toNewKubeExecutoris the default closure atwire.go:53(grep -rn NewKubeExecutor→ no other production site);rest.InClusterConfig()/clientcmd.BuildConfigFromFlagsexist only insideNewKubeExecutor(attach/kube.go:66,71). Counted, not hoped:TestWireDisabledConstructsNoKubeClientuses anatomic.Int64and asserts0, plusTestWireDisabledIgnoresAConfiguredKubeconfig(path configured, still 0 constructions). Both PASS. The disabledBridgestores the kubeconfig string only (KubeConfigPath()), never opens the file. No path readsLD_ATTACH_KUBECONFIG's file or an in-cluster token while disabled; also the config layer refusesLD_ATTACH_KUBECONFIG+ flag-off outright (config/load.go:134-141,FaultIncompleteAttach), so the ambiguous state can't even boot.LD_ATTACHappears nowhere outsidelab-director/cockpit/in the tree.503,disabled:true,policy:P-ATT-001,Cache-Control: no-store, and exactly oneattach_refusedledger entry (TestAttachInfoIsDisabledByDefaultAndAuditedPASS; I read the handler:handleAttachInfo→Info→!b.opts.Enabledguard atbridge.go:277→refuse). For an unauthenticated caller it is401(audited) and for a non-operator403(audited) — auth precedes the flag, which is the safer ordering and matches the README matrix rows. Both are still refusals before the flag is disclosed.Config.Attach.Enableddefaults false and= "1"is refused as a non-bool.attachRequest(identity + term;attach.go:182) →Info(:192) →Upgrade(:210); with the flag offInfoyieldsStatus=503, sowriteJSON+returnat:196-201executes andUpgradeis never reached.TestAttachWebSocketRefusesToUpgradeWhileDisabledPASS assertsconn == nil,resp.StatusCode == 503, andresp.Header.Get("Upgrade") == ""(no 101, no Upgrade header), plus one ledger entry. Theattachable:false200 case also returns409without upgrading (TestAttachWebSocketDoesNotUpgradeForANoTTYClassPASS). Auth is before the upgrade in the only WS ordering that exists.refuseAbout→Auditor.Refused→Sink.Append(bridge.go:470-486), which setsAuditRecordedhonestly and logs if the sink refuses; the transport reuses that same path viaBridge.Refuse(attach.go:147-161) so 401/403/400 are written, not swallowed. I enumerated every return in both handlers: the only early return that writes no ledger entry is the failed-Upgradepath (attach.go:213) — see finding 1. Also verifiedInfoaudits but never double-audits when the handler also logs a warn line (attach.go:194-208).attach.Requestcarries only{Sandbox, Caller, RequestID, Term}; the caller is built solely from the auth-resolved*auth.Identity(attach.go:88-101,attachCaller), andSandbox.Identityis a field only aRegistrycan set — and the live registry is deliberately absent (onlyUnwiredRegistry+ the tests'RecordingRegistryexist). Theld-<role>label is never read from a pod in this build, andHasTTYis a registry claim, not a pod label. A verified-but-unauthorized identity is recorded asPrincipalUnknown(attach.go:465-473), never as operator. Note this means the ratified "registry resolvesld-<role>" mechanism is a seam plus fixtures here, which the README states plainly — so the claim is honest, but it is not exercised against a real cluster.git diff --name-status origin/epic/LD3...HEAD→ 27 paths, every one underlab-director/cockpit/(25 code/docs +go.mod/go.sum). Noweb/**, no RBAC/CNP/manifest/prod/ci.yaml/dispatcher/herdr changes. The added config surface is exactlyLD_ATTACH_ENABLED+LD_ATTACH_KUBECONFIG.go.modadds onlygorilla/websocket+k8s.io/client-goinside the cockpit module (the mechanism), not the grant. Any deployed env settingLD_ATTACH_*would have been a boot-regression risk from the new config refusal — I grepped the whole tree: nothing sets them.e79ed6fcensus withdrawal: correct — the guard is real and would really fail.web/e2e/shell.spec.ts:77declaressources = ['plane','gates','dispatcher','bus']and:83assertsgetByTestId('source-row')toHaveCount(4);readmodel/readmodel.go:193appendsExtraSourcesintosnap.Sources, andweb/src/routes/+page.svelte:83-84renders oneSourceRowper source — so the removed 5th row would have made the count 5 and failed the spec. That spec runs in CI (ci.yamljobcockpit-e2e→xvfb-run -a npx playwright test). The only alternative to withdrawing was editingweb/e2e/shell.spec.ts, which is exactly theweb/**scope violation this slice must not commit. The README's counter-claim ("the boot log, the--checkOK line, the info endpoint, and the README") is accurate and I verified all three:wire.go:57-62always logs"attach disabled: … NO kube client constructed"at boot,main.go:324prints theOK attach enabled=false …line, andInforeturnsdisabled:true+P-ATT-001. It is partially true that this hides the state from an operator who only reads the cockpit UI — but the README says so explicitly and the row has no UI consumer until the xterm.js pane slice lands, so carrying it would be a placeholder behind a failing-by-design guard. Withdrawing was the right scope-safe choice.Verified by (commands you actually ran)
git diff --stat/--name-status origin/epic/LD3...origin/plane/HOMELAB-1683-attach-bridge→ 27 files, +5659/-21, all underlab-director/cockpit/; no non-cockpit path.git show origin/plane/…:{attach/{attach,bridge,wire,kube,registry,executor,auditor,protocol}.go,server/attach.go,server/middleware.go,server/server.go,audit/audit.go,config/{config,load,faults}.go,auth/middleware.go,cmd/cockpit/main.go}(read in full) → the guards/orderings quoted above.git show e79ed6f+git show origin/epic/LD3:…/web/e2e/shell.spec.ts→ the census row removal and thetoHaveCount(sources.length)parity guard it cites.grep -rn "source-row|data-source" web/src/andgrep -n source web/src/routes/+page.svelte→:83 {#each sources as source}→ one row per source;grep -n ExtraSources readmodel/readmodel.go→:193appended intosnap.Sources.grep -n "playwright|xvfb-run|cockpit-e2e" .forgejo/workflows/ci.yaml→ the e2e job that runsshell.spec.ts.git worktree add --detach … origin/plane/HOMELAB-1683-attach-bridge;GOFLAGS=-mod=mod GOPROXY=file:///home/aaron/go/pkg/mod/cache/download go build ./...→ exit 0;go vet ./...→ exit 0;go test ./attach/... ./server/... ./config/... ./audit/...→ ok for all four packages.go test ./attach/ -run TestWireDisabled -v→PASS×2 (TestWireDisabledConstructsNoKubeClient,TestWireDisabledIgnoresAConfiguredKubeconfig).go test ./server/ -run 'TestAttachInfoIsDisabledByDefaultAndAudited|TestAttachWebSocketRefusesToUpgradeWhileDisabled|TestAttachRequiresASessionAndRecordsTheAttempt|TestAttachRefusesANonOperatorAndRecordsWho' -v→PASS×4./tmp/wslog, clone ofstatusRecorder+ gorilla, then deleted) →--- FAIL: RECORDED STATUS = 200 (comment claims 101); plusgrep -n "Hijack\|checkSameOrigin" gorilla@…/server.go→Upgradeuseshttp.NewResponseController(w).Hijack()andcheckOrigin = checkSameOriginwhen nil.server.New(...).Handler()in a scratch worktree (probe file removed afterwards) →handshake status = 101while the cockpit loggedmsg=request method=GET path=/ws/attach/ld-light-1 status=200 bytes=0.grep -rn "LD_ATTACH"(whole branch tree) → onlylab-director/cockpit/{attach,config,cmd,README}.grep -rn NewKubeExecutor|attach.Wire(|Enabled: *true --include=*.go→ production construction sites:wire.go:53only;attach.Wirecalled only atcmd/cockpit/main.go:159and:314.grep -n "Registry.Resolve|opts.Enabled" attach/bridge.go→Resolveonly at:382, reachable only past the!b.opts.Enabledguards at:277(Info) and:307(Open).VERDICT: APPROVE-WITH-CONCERNS
Landing adjudication — PR #579 (LD3.3 attach bridge, code slice)
Head reviewed:
ca9022f(verified unmerged). Combined CI onca9022f: success, 8/8 contexts(
CI Review×6 +Build Lab Director Cockpit Image×2, runs #1719/#1720). Builder's "all 7 green"is confirmed and one better: 8 contexts.
Two fresh-context reviews, both at head
ca9022f, both APPROVE-WITH-CONCERNS, 0 blockers:Verified myself (spot-check against primary sources, not the reviewers' summaries):
attach/wire.goreturnsNew(Options{Enabled:false})on the disabledpath without ever calling
opts.NewExecutor;NewKubeExecutorhas exactly one production callsite (the default closure). Config defaults
LD_ATTACH_ENABLED=false, andLD_ATTACH_KUBECONFIGwith the flag off is a boot fault (
FaultIncompleteAttach), so the ambiguous state cannot start.attachRequest→Info→Upgrade; flag-offInfois503+ one auditedattach_refused, no101, noUpgradeheader.from
*auth.Identity; the live registry is absent from this build.lab-director/cockpit/**(27 files). No RBAC/CNP/manifest/prod/web/ci.yaml/dispatcher/herdr — i.e. M1/S1/§4/§5 remain unexercised, as the ratification package requires.
e79ed6fcensus withdrawal: correct.web/e2e/shell.spec.tsasserts exactly 4 source rows andthe spec runs in CI; the alternative was touching
web/**, out of scope. Withdrawal is documentedin the README and
main.go's NOTE. I additionally corrected the stale PR-description sentencethat still claimed a census row.
Classifications (non-blocking, follow-up): the 5 concerns are all enabled-mode or doc/test-fidelity
items — the failed-
Upgradepath is unaudited (enabled + wired-registry only, unreachable today);the
statusRecorder.Hijackcomment's "(101)" is false (probe:status=200 bytes=0); the unreachableread-limit branch mislabels an oversize frame as
client_eof/closed;writeAttachErrorsends noclose frame despite the grammar; the close-entry failure is log-only (needs a durable sink to be
reachable). None break flag-off inertness, scope, identity, or the audit-open path. They belong to the
xterm.js pane / durable-sink slices.
Decision: merge. Landing-lock legs: head-sha CI green ✅ · verdicts posted after
ca9022f✅ · noopen blocker ✅. GitOps: no image, no manifests, nothing to roll out (flag off).