fix(security): never send a remoteControl.tls bearer token to a publicly-trusted host - #313
Merged
Merged
Conversation
thc1006
force-pushed
the
fix/require-pinned-ca-for-bearer
branch
from
July 31, 2026 01:00
fdf209c to
f2bb245
Compare
…cly-trusted host resolveRemoteControlTLS pinned RootCAs to the Secret's ca.crt when present and otherwise fell back to the system trust store — the code comment said so plainly: "Trust a private CA when provided; otherwise fall back to the system roots." remoteControl.endpoint is CR-author controlled and ServerName is derived from it. So a principal holding the shipped ntncellconfig-editor-role — NTNCellConfig write, NO secrets get — could: 1. reference any opted-in Secret carrying a token but no ca.crt, 2. set endpoint to a host they own, 3. present an ordinary publicly-trusted certificate for that host, 4. and be handed the token verbatim. That is the #251 confused deputy completing, and it needs none of the label gate to be bypassed: the label only governs WHICH Secret may be named, never where its contents may be sent. Both controls that do cover this path ship OFF by default — the endpoint allow-list (#300) and the credential-reference admission policy — so with stock settings nothing stopped it. A Secret carrying a token must now also pin ca.crt. mode=mtls without ca.crt stays permitted: it PROVES a key rather than sending one, so a misdirected mTLS dial is identity misuse rather than credential theft — the line ADR-0009 already draws. A deployment that genuinely fronts its gNB with a publicly-trusted certificate opts in explicitly by putting that CA in ca.crt. Upstream precedent: kubeadm pins a CA public key for token-based discovery rather than trusting the system store before sending its bootstrap token. No new oracle: the refusal wraps errRemoteControlCredentialInvalid, so the CR author still sees only the uniform errRemoteControlCredentialUnavailable message and the specific cause goes to the operator log — the existing discipline. It is also a deterministic credential error, so it self-heals on the 5m poll (#282) once ca.crt is added, with no spec edit. No test covered token-without-ca.crt at all — every existing case supplied ca.crt, which is why the fallback went unnoticed. Besides the direct case, the new test states the INVARIANT once: any successful resolution that returns a token must have pinned roots, so a future key or mode cannot reopen this case-by-case. Mutation-verified (removing the gate reddens both the case and the invariant). Generated artifacts: the API field doc change regenerates the 4 CRD copies. Note that `make docs` also picked up PRE-EXISTING drift in docs/api-reference.md (lastPushedSatSwitchDigest from #285, EphemerisSelectionAmbiguous, the minTerrestrialDwell CEL bounds) — that file is generated but no CI step checks it is current. Included here because regenerating is unavoidable; the missing CI gate is called out as a follow-up rather than bundled into a security fix.
thc1006
force-pushed
the
fix/require-pinned-ca-for-bearer
branch
from
July 31, 2026 02:54
f2bb245 to
fe096c4
Compare
thc1006
added a commit
that referenced
this pull request
Jul 31, 2026
…tials Three gaps remain on main after #219, #251/#300, #296, #297, #309 and #313. They look separate but share one shape: the Secret's owner cannot express, revocably, what their credential may be used for. 1. #309's ValidatingAdmissionPolicy is evaluated at ADMISSION only — ADR-0009's own amendment says stored objects are not re-validated, so withdrawing an author's RBAC afterwards does not re-evaluate the CRs they already created. 2. --remote-control-allowed-endpoint-hosts is ADMIN-GLOBAL. Verified against main: there is no per-Secret endpoint binding anywhere in the tree, so any labelled Secret can be paired with any allow-listed endpoint. 3. Secrets are get-only by design (an informer would need cluster-wide list/watch, rejected in #298), so withdrawing consent is invisible until the next real push — measured at 166s on a live 1.36.3 cluster, driven by the referenced SatelliteEphemeris's heartbeat rather than any per-cell retry. A grant is an OBJECT rather than a check, which is what buys all three: it can be deleted (revocable), it can be watched and indexed so revocation enqueues the affected NTNCellConfigs, and it has somewhere to put allowedEndpoints and allowedModes. Partly supersedes ADR-0009. Its recommendation to prefer a SubjectAccessReview over a ReferenceGrant-style CRD was sound for the requirement as stated there — deciding who may create the reference — but does not survive two requirements that were not on the table: revocability (a SAR is point-in-time on a write) and endpoint binding (a SAR cannot express a destination). So a grant is not a heavier way to do the same job; it does three jobs. ADR-0009 now carries a forward pointer so a reader cannot act on the superseded bullet. #309 is explicitly NOT replaced — it stays the cheap default tier (no CRD, no controller, one declarative policy). Enforcement is admin-gated and default-off behind --require-credential-grant; the "enforce whenever a grant exists" variant is rejected in the ADR because an attacker simply would not create one. Deliberately candid about the limit: rotating a Secret's CONTENTS touches no grant and fires no watch, so that path keeps the ~3-minute bound. The optional credentialRevision field is an opt-in escape hatch, not a Secret watch, and this ADR does not close #298. Proposed only — no CRD, controller or flag ships here.
thc1006
added a commit
that referenced
this pull request
Jul 31, 2026
test/e2e/ contained ZERO tests touching remoteControl, wss or mTLS. Everything under #206, #295, #297, #313, #318 and #322 was unit-tested against httptest, which cannot show the deployment shape works: OCUDU's remote_control server is plaintext, so the feature only exists behind a TLS-terminating proxy — and that proxy shipped as a sample nobody executed. Design choices, each load-bearing: - NO IMAGE BUILD. The gNB stand-in is stdlib-only Go (its own RFC 6455 handshake and framing), mounted from a ConfigMap and run with the stock golang image; the proxy is stock nginx. So the suite runs identically on Kind in CI and on a plain kubeadm cluster — no Docker, no registry, no `kind load`. Verified: the stub interops with the operator's real coder/websocket client, and starts in-cluster from the ConfigMap with no network at container start. - The nginx config is READ FROM the shipped sample, so the sample itself is what is under test. A copy inlined here would drift and let the sample rot green. - The assertion is WHAT CROSSED THE WIRE, read back out of the plaintext backend. A condition flipping to True proves the operator's opinion, not the chain. - The suite is SELF-CONTAINED: it patches the manager's --ephemeris-allowed-private-hosts if absent, because the GP mock sits on a private ClusterIP and the SSRF-safe fetcher would otherwise block it — a failure that reads as "the push is broken" but is not. Proven by stripping the flag and re-running: the suite re-adds it and still passes. - The GP fixture carries EPOCH=now. The checked-in testdata is months old and sourceEpochFresh (maxEpochAge 7d) would refuse it, which is why the runtime push has never actually been reachable from e2e. Four arms, the last three each also asserting that NOTHING reached the backend: happy path (frame validated for cmd/plmn/nci/future-epoch), wrong bearer -> RemoteEndpointRejected, token without ca.crt -> refused before any dial (#313), dropped client certificate -> refused by the proxy. Adversarial pass on the test itself: the arms first waited only for a False prefix, so an arm could assert while the PREVIOUS arm's reason was still on the object — intermittent failure, the worst kind. Each wait is now reason-specific, and every arm restores and waits for True/Pushed so the next starts from a known baseline. Mutation-checked: an arm that skips its own mutation fails. CI guards the same false-green classes the HA step does — a build-tag typo or a rename matching zero tests, a silently deleted arm, and (specific to this suite) a run where no ntn_config_update frame ever crossed the wire.
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
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.
Addresses part of #251. Default-on, unlike the two existing controls for this path.
The review is right, but it is the same ground as ADR-0009 — except for one clause
Everything the review lists (label-only opt-in, SA/bootstrap-token refusal, same-namespace, uniform error, and the three "known limits" the code comment itself admits) is already written down in ADR-0009 and #219, in the same terms. Its severity conditioning matches the ADR's. So on its own it does not move the position.
One clause does: "在使用 system roots 或攻擊者能提供受信任憑證的情況下". That sent me to the trust-anchor code, and it is a real, concrete, default-on hole that ADR-0009 does not mention:
ca.crtabsent →RootCAsnil → Go uses the system trust store. AndremoteControl.endpointis CR-author controlled, withServerNamederived from it.Why that completes the attack with stock settings
A principal holding the shipped
ntncellconfig-editor-role(NTNCellConfig write, nosecrets get):tokenbut noca.crt,endpointto a host they own,None of the label gate needs to be bypassed — the label governs which Secret may be named, never where its contents may be sent. And both controls that do cover this path ship off by default: the endpoint allow-list (#300, empty = permit-all) and the credential-reference admission policy. The private-IP SSRF guard does not apply here either — it inverts on this path, since a legitimate gNB target is private.
So the review's "release blocker" framing is defensible for the default configuration, and this PR is the part that can be fixed without asking the operator to turn anything on.
Change
A Secret carrying
tokenmust now also pinca.crt, or the push is refused as a permanent credential error.mode: mtlswithoutca.crtstays permitted — it proves a key rather than sending one, so a misdirected mTLS dial is identity misuse (and leaks the pushed config), not credential theft. That is the line ADR-0009 already draws between the two modes, so this PR does not redraw it. A deployment that genuinely fronts its gNB with a publicly-trusted certificate opts in explicitly by putting that CA bundle inca.crt.Upstream precedent: kubeadm pins a CA public key (RFC 7469 style) for token-based discovery rather than trusting the system store before sending its bootstrap token — the same "we are about to transmit a token, so the destination must be unforgeable" reasoning.
What this does NOT claim
It is not the authorization boundary #251 asks for, and the
resolveRemoteControlTLScomment still says "partial mitigation", because it still is. This bounds where a named credential may be sent; it does not change who may name it. The three known limits from #219 are untouched.Adversarial pass on my own change
errRemoteControlCredentialInvalid, and the controller surfaces only the uniformerrRemoteControlCredentialUnavailableon the CR while logging the specific cause for the operator. Verified at the call site, not assumed.ca.crtrecovers without a spec edit.ca.crtpresent but invalid PEM still fails in the existing branch (it does not fall through to the new one); no token and noca.crtis unchanged.gocycloon an already-large table — which is also just better placement.Verification
go build,go vet,golangci-lint v2.12.2: 0 issues;gofmtclean.go test ./internal/controller/... ./pkg/...green (envtest 1.36.2).tokenwithoutca.crtat all — every existing case suppliedca.crt, which is exactly why the fallback went unnoticed. Beyond the direct case, the new test states the invariant once: any successful resolution that returns a token must have pinned roots, so a future key or mode cannot reopen this case-by-case.must be refused: the endpoint is caller-controlled…) and the invariant (a token was returned for "token only" with RootCAs=nil).Two things a reviewer should look at deliberately
tokenand noca.crtstarts failing. Worth confirming that matches your deployments.docs/api-reference.mdcarries pre-existing drift. Regenerating it (unavoidable, since I changed an API field doc) also pulled inlastPushedSatSwitchDigestfrom fix(ntncellconfig): send sat_switch_with_resync only on intent change, not every heartbeat #285,EphemerisSelectionAmbiguous, and theminTerrestrialDwellCEL bounds. That file is generated but no CI step checks it is current. I deliberately did not add that gate here — bundling a CI-policy change into a security fix makes the fix harder to review — but it is worth a follow-up.Merge skew with #296
#296 deletes
errRemoteControlCredentialInvalidand routes every credential failure to a singleRemoteControlCredentialUnavailablereason. The newreturnhere wraps that sentinel and sits in a block #296 rewrites, so whichever lands second drops the wrap — after #296 the uniform reason already applies. One-identifier resolution, in code #296 is touching anyway.Skew note refreshed (main @
3ca522c). This PR's own merged HEAD against currentmainis green — build,go vet, and./internal/controller/... ./pkg/...(8 packages, no failures).The #296 resolution written above was derived against
d83de6eand is stale: #296 no longer compiles againstmainon its own, because #300 added a new use of the constant #296 deletes (see #296 for the one-line fix). The resolution here will be re-derived once #296 is rebased. Nothing to do on this branch.