fix(fleet): waive the visualiser's plain-HTTP listener, which is loopback-only - #433
Merged
Merged
Conversation
…back-only `SAST — semgrep` blocks on one finding: `use-tls` against the g7 visualiser's `http.ListenAndServe`. The rule cannot see the deployment, and the deployment is what makes it safe — the compose service publishes the port as `127.0.0.1:8099:8099`, so the listener is reachable from the host loopback alone, never from the fleet network and never off-box. It serves a read-only visualiser over data the operator already has locally, and terminating TLS on a loopback port means shipping a cert nobody can validate. The waiver states that reason at the line, and says what would invalidate it: the published port losing its 127.0.0.1 prefix. Per-line, not a rule disable — a second plain listener added later fires fresh. Verified: the finding goes 1 -> 0 with the waiver and returns when it is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The waiver called this a read-only visualiser. It is not: the handler POSTs to the gateway over mTLS with a client cert from /pki, so the plain-HTTP port is an unauthenticated front to an authenticated channel. That does not make the waiver false — loopback is still loopback, and reaching the port already means host access — but the reason a reader checks has to be the true one. The comment now states it, and names the second condition that would invalidate it: a route here that is not safe for whoever holds a shell on the host. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 11, 2026
Yambr
added a commit
that referenced
this pull request
Aug 11, 2026
…tend bridge (#434) * fix(fleet): take the unauthenticated exec surface off the shared frontend bridge The waiver merged in #433 rested on a false premise. It said the visualiser was "reachable from the host loopback alone — never from the fleet network". The `ports:` publish is loopback, but the service sat on `ocu-frontend`, a plain bridge it shared with open-webui, webui, embed-portal, admin and mcp-gateway. A published port says nothing about in-network callers, so every one of those could reach it. What they could reach matters: `/api/create`, `/api/exec`, `/api/tool` and `/api/destroy` take NO inbound credential, and the process holds the gateway mTLS client cert. Any co-tenant could POST an arbitrary argv in cleartext and have it executed in a live guest under that cert — with open-webui, which processes untrusted input, among the co-tenants. g7 now shares a dedicated bridge with control alone, which is the only hop it needs. The waiver stays, because the rule still cannot see a deployment, but it now states the containment that is real instead of the one that was not, and says that the exposure here is authorisation rather than eavesdropping. `deploy/tests/test_fleet_g7_isolation.py` turns all of it into properties: the loopback prefix, no shared bridge with the web tier, and — so the isolation cannot pass by breaking the service — that g7 still shares a network with control. Mutation-checked: each of the three reds on its own violation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(fleet): allowlist g7's co-tenants instead of denylisting the web tier A hard-coded list of web-tier names passes silently the moment a service is renamed or a new one is added — the assertion would still be green while the containment was gone. The test now derives the actual co-tenant set from the compose file and requires it to be exactly `{control}`, so any unlisted service joining that bridge reds and is named in the failure. Mutation-checked: adding an arbitrary service to `ocu-g7` reds and prints which one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Nick <developer@widemoat.ai> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Clears the last red gate on #346.
The finding
SAST — semgrepblocks on exactly one:go.lang.security.audit.net.use-tlsagainsthttp.ListenAndServeindeploy/fleet/g7-visualizer/proxy.go:236.Why a waiver and not a fix
The rule cannot see the deployment, and the deployment is what makes it safe. The compose service publishes the port as:
Loopback only — never the fleet network, never off-box. That is the sole exposure: no second compose file or override, no nginx or ingress forwarding to it, no k8s manifest, no Helm value, no
docker runin a stand script. Terminating TLS on a loopback port means shipping a certificate nobody can validate.What sits behind the port
Worth stating plainly, because the first version of this waiver got it wrong. The service is not a read-only visualiser: the handler POSTs to the gateway over mTLS (TLS 1.3, client cert from
/pki), so this plain-HTTP port is an unauthenticated front to an authenticated channel.That does not make the waiver false — reaching a loopback port already means host access — but it is the fact a reviewer needs, so the comment states it rather than the softer claim. Two conditions invalidate the waiver, and both are written at the line: the published port losing its
127.0.0.1prefix, and any route here that is not safe for whoever holds a shell on the host.Per-line, not a rule disable
A second plain-HTTP listener added later fires fresh instead of being silently pre-exempted. Verified: the finding goes 1 → 0 with the waiver and returns to 1 when it is removed.
One note:
gofmtreports this file as unformatted, from a stray blank line at 294 that predates this change. Left alone — it is not this diff's line.