Security report fixes - #324
Merged
Merged
Conversation
The peer binding looked up node(<peerID>) with GetBlockID and checked only the returned error. GetBlockID searches appended attenuation blocks as well as the authority block, and appending needs no root key, so any holder of a valid token could append node(<their own peer id>) offline and connect under their own libp2p key. The signature chain still verified and the authority facts were untouched, so the forged binding passed. Nodes and routers hand their identity biscuit to any peer that completes a handshake, which made a router token easy to obtain and therefore router impersonation, attested-label spoofing and control-plane lease spoofing reachable by any authenticated mesh member. RequireAuthorityBinding now demands the fact come from block 0. The three call sites route through it, which also replaces the raw "node" literals with api.FactNode so a rename cannot desync them from the mint side. The dataplane was already protected by BaselineReplayCheck, which reads client_peer_id through the Datalog authorizer; appended facts never reach it. That check needs no change, and the existing attenuation test pins it.
Settings lived under a top-level "linters-settings" key, which is v1 syntax that v2 ignores. goheader was therefore enabled with no template and passed every file, and "config verify" failed outright because the version has to be a string. 26 files had drifted in the meantime: some carried no license header, some a truncated one, some a tab where the template wants spaces. Moving the settings to linters.settings turns goheader back on, so those are repaired here. Note the header needs a blank line before the package clause: without one gofmt reads it as a doc comment and re-indents the URL with a tab, which goheader then rejects. forbidigo replaces what would otherwise be a grep: with analyze-types it matches the resolved symbol, so it still fires through an import rename. Biscuit.GetBlockID searches appended attenuation blocks, which any token holder can forge, so identity.RequireAuthorityBinding is the only place allowed to call it. Tests are exempt because they forge such blocks to prove the check rejects them.
GET /policies accepted any node record that was not explicitly banned, so a node whose OIDC session had lapsed kept reading mesh policy — roles, bindings and allowed targets — until an operator banned it by hand. The refresh handler on the same server already checked ExpiresAt; that divergence is the whole bug, since passive expiry is meant to be the backstop that works without anyone intervening. Enrollment is bounded by two independent conditions, a ban and the end of the session, and both are now expressed once in EnrolledNode.CheckAdmission rather than spelled out per call site. The refresh path keeps its distinct status codes by matching on the returned sentinel. The handler test drives one unchanging biscuit against a node record that moves between states, so what it measures is the admission decision and not the token.
The mesh credential is stored in an HttpOnly cookie with the stated reason that an XSS in the console must not be able to exfiltrate mesh admin rights. /auth/session then read that cookie and returned its raw value to any same-origin caller, so injected JS could fetch it — the precise attack the flag was chosen to stop. Nothing needed it. The reverse proxy already injects the cookie as an Authorization header for /api/ calls, and no caller of this endpoint exists in the console SPA, so reporting session state alone is sufficient.
custom_datalog holds two kinds of entry. Facts are embedded in the token; rules are distributed to nodes and applied by their authorizer. The control plane's validator accepts both and nodes consume both, but minting parsed every entry as a fact and treated the failure as fatal, so one rule-form entry made enroll and refresh return 500 for every node resolving to that role. Refresh is the worse half: a rule added to a role long after enrollment breaks nodes that were working, at renewal, with nothing having changed on their side. Minting now skips an entry that parses as a rule. An entry that is neither a fact nor a rule is still an error, so a typo is not quietly dropped along with the permission it was meant to grant, and facts listed beside a rule still reach the token.
…n TCP Two small gaps on the same local API surface, which the code itself documents as reachable by any local process. The shared-secret gate compared with plain !=. The control plane already does this the other way, hashing and using crypto/subtle, so this was a divergence rather than an oversight in design. Go's string != checks length first and then compares word-wise, which is not a clean byte-by-byte oracle, but there is no reason for the two components to differ. /metrics sat outside the gate. Its labels carry peer IDs, models and per-route request counts, so an unauthenticated local caller learned who a node talks to and how much. It is now gated like everything else, which leaves socket callers unaffected: reaching the socket already proves the caller owns it, and every scrape in this repo and its docs goes that way. Deployments that scrape a node over TCP need to send the token. The token test asserts correctness, not timing; a timing assertion in a unit test is flaky enough to be worse than nothing.
The ingress socket is the one hole punched through the isolation the sandbox exists to enforce: it reads "CONNECT <port>" and relays the caller to that port inside the namespace, with no token and no capability of its own. Its permissions are therefore the whole credential, and it was created with whatever the umask happened to allow. Both sibling sockets in this repo are chmod 0600 immediately after Listen, with comments saying why. This one now matches, and fails closed rather than serving a socket it could not restrict. Under the usual umask 022 the socket lands at 0755 and connect() from another uid is refused, so exploitability was environment-dependent; the test clears the umask so a missing chmod cannot be masked into passing.
A node names the agent it is acting for beside its token, and the receiving node injected that name as a trusted agent() fact without checking anything. Since agent: is a valid policy member prefix, any authenticated peer could name any agent and pick up whatever role a binding gave it. The caller chose its own principal. A role now carries allowed_agents, the namespaces a node holding it can act for, minted into its token as granted_agent_* facts. The receiver derives agent_authorized from the caller's own token and refuses a claim that falls outside it. A node with no grant cannot name any agent. The architecture doc has described this bound as mandatory since the agent work landed (section 8.6), and said the target-fact machinery already expressed it. It did not: allowed_targets says which destinations a node may reach, not which agents it may act for, so this is a separate fact family rather than a reuse of that one. The grant is keyed on the node's role, not on its labels. Labels are declared by the node itself at OIDC enrollment and only syntax-checked, so a namespace derived from one would be a namespace the node picked. Roles resolve from verified OIDC claims against admin-written bindings. The check runs only when a claim is present. A node's own housekeeping acts for no agent, and requiring a grant unconditionally would refuse its model-catalog probes and quietly drop its models from peers' listings.
allowed_targets accepted any fact:value pair, but a target only matches if
something derives a target_fact for that name, and only five do: node, and
the four OIDC claim facts. Anything else minted a granted_target_* fact that
nothing could ever satisfy, so the grant denied silently and looked
configured.
This was not hypothetical. An integration fixture in this repo carried
"tcp://db:5432" as an allowed_target, which parses, validates, mints a fact
and matches nothing; it is corrected here.
"agent:" gets its own message. It reads like it should work, and section 8.3
of the architecture doc intended it to, but a node's identity does not say
which agents it hosts, so no target_fact("agent", ...) exists to match. The
error points at allowed_agents, which is the axis that does exist: being
allowed to call an agent was never the same as being allowed to act for one.
The accepted set is derived from the same place TargetFactRules is built, so
the two cannot drift apart again. "*" stays valid as a fact, since
granted_target_all_facts matches every target_fact.
isExactService and isExactTarget re-tested the same three wildcard shapes that BuildServiceDatalogFact and BuildTargetDatalogFact branch on, and BuildServiceDatalogFacts routes each grant between a grouped set fact and a single fact based on the two agreeing. They agreed, but nothing made them. Add a wildcard shape to the builder and not to the classifier and a wildcard grant is frozen into a set, where only an exact contains() can match it, so the grant silently allows nothing. The reverse mistake loses the grouping and inflates the fact count. Both now ask the builder what it produced, so there is one classifier and the shapes cannot drift apart. Also replaces the last two "node" literals in this file with api.FactNode.
The OIDC claim to fact relationship existed in three places: the oidcClaimToFact map, a set of hardcoded claim reads and a prefix switch in resolveRoles, and a switch over which facts are multi-valued in translateClaimsToFacts. Only convention kept them together. Adding a claim to the map alone did nothing. The binding side never read it, so a binding naming that fact resolved no role, and the mint side dropped it because its switch had no case. The entry looked configured and granted nothing, in both directions. resolveRoles now builds its claim values from the map, keeping node as the one explicit case because it is matched against the connecting peer rather than the token. translateClaimsToFacts treats every claim as a list, since a scalar is a list of one, which removes the need to know which facts are multi-valued at all. Both tests walk the map rather than a fixed list, so a claim added later is covered without anyone remembering to extend them.
The OIDC redirect_uri and the session cookie's Secure flag were built from the request's Host header and X-Forwarded-Proto, in four places that each worked it out again. Both are client-controlled, and a proxy that terminates TLS without setting the header leaves the mesh admin cookie without Secure. --external-url names the origin browsers actually reach the console on and takes precedence over both headers. Leaving it unset keeps the old behaviour, which is what a local run needs. The four derivations now go through one helper, so the redirect_uri sent in the authorization request and the one sent in the token exchange cannot drift apart; they have to match byte for byte. A malformed value is rejected at startup rather than failing every login. PKCE with the verifier in an HttpOnly cookie already made a captured code unusable, and providers match redirect_uri exactly, so this closes the residual cookie-downgrade case rather than an open redirect.
role_permissions rows are discriminated by a resource_type string, and only target, service and custom_datalog had cases. allowed_agents went in with none, so SaveMeshPolicy wrote it nowhere and GetMeshPolicy read it back empty. Nothing failed: the policy saved, the API returned it, and the grant did not exist. That made the namespace bound added a few commits ago inert whenever the policy came from the database, which is every real deployment; its tests passed because the mock control plane in tests/integration mints tokens directly. Both grants are stored now, and the round-trip test reflects over the message's repeated fields rather than listing them, so the next field added to PolicyRole is caught here instead of silently granting nothing.
A node sends its own labels in its enrollment request. The control plane checked their syntax and then minted each one as a signed label() fact, which peers treat as attested: call_remote_tool's required_labels rejects a provider unless it holds a matching fact, and that gate is what "discovery is not permission" rests on. Any identity able to enrol could declare region="us-east-1" and satisfy every consumer requiring it. A role now carries allowed_labels, and enrollment refuses a label no resolved role permits. Fail-closed: a role granting none means the node declares none. The two paths that mint without review are covered, OIDC /register and bootstrap auto-approve; manual approval already attests them, since the administrator sees the declared labels before approving. The documentation said labels were attested by the control plane and described only the approval path, which was the one case where it was true. This is also why the agent namespace grant added earlier is keyed on a node's role rather than its labels: a namespace derived from a self-declared label would have been a namespace the node chose for itself.
The integration suite mints tokens with the mock control plane in minimal_helpers_test.go, which builds a biscuit directly and never touches the policy store. That is why allowed_agents shipped while SaveMeshPolicy dropped it: the grant was configured, the API returned it, and the token was minted without it, with every test passing. This drives the sam-control-plane binary over its own REST API, so the path under test is the deployed one: POST /policies, the database, enrollment, and the facts actually inside the signed token. It asserts every repeated field of PolicyRole survives the round trip, that the agent namespace grant reaches the token, and that a label outside the role's grant is refused. Reverting the storage fix fails it twice, once on the API round trip and once on the minted token, which is the coverage that was missing.
Three paths mint a token: OIDC /register, bootstrap auto-approve, and bootstrap with an administrator approving a pending request. The first two refuse a label the node's role does not grant; the third did not, on the grounds that approval was itself the attestation. It is not. Approving says the identity may join. The labels came from the node, they are shown on the request, and nothing makes anyone read them before clicking approve, so "attested" rested on a habit rather than a check. The gap also made this path the way around the other two: the same label refused at /register was accepted by asking an admin. All three now apply the role's allowed_labels, which is the point. A rule enforced on two paths out of three is one a future reader fixes by copying the wrong one. An operator who needs a one-off label adds it to the role, which leaves a record; approving it silently did not.
Each record is taken apart into named columns on the way in and reassembled on the way out, and the two column lists are written by hand. A field added to the struct but not to both is stored nowhere and read back zero, and nothing fails: the caller saves, the API returns what it was handed, and the value quietly does not exist. A role's allowed_agents shipped that way, and only PolicyRole was checked afterwards. This covers the rest, EnrolledNode, BootstrapToken, EnrollmentRequest, RouterLease, User and PolicyBinding, against both the Get and the List query, which have separate column lists and can drift independently. The tests are arranged so that adding a field breaks them: the value being saved is walked first and a field still at its zero value fails before any database work, naming the field. That is the moment to decide whether the store should write it, rather than finding out from a grant that turned out to be inert. Also pins that re-enrolling does not clear a ban. Enrollment writes every other column on conflict and deliberately leaves banned alone, so a banned node cannot restart its way back into the mesh.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a robust policy framework for managing agent namespaces and node labels, securing node enrollment, and hardening sidecar and console endpoints. Key enhancements include restricting agent claims to authorized namespaces, validating self-declared node labels against role permissions, enforcing token bindings strictly within the root authority block, and securing console session cookies and metrics. The code review feedback highlights opportunities to optimize target validation by caching TargetFactNames(), deduplicate wildcard patterns during agent fact generation to minimize token size, and improve debugging by detailing the offending pattern in label validation errors.
Comment on lines
+138
to
+143
| if fact != "*" && !slices.Contains(TargetFactNames(), fact) { | ||
| if fact == FactAgent { | ||
| return fmt.Errorf("invalid target %q: an agent cannot be a target, because a node's identity does not say which agents it hosts. Use allowed_agents to grant the agent namespaces a node may act for", target) | ||
| } | ||
| return fmt.Errorf("invalid target %q: %q is not a target fact, so nothing would ever match it (want %s or \"*\")", target, fact, strings.Join(TargetFactNames(), ", ")) | ||
| } |
Contributor
There was a problem hiding this comment.
Calling TargetFactNames() twice in the validation path is inefficient as it performs map cloning, iteration, and sorting on each call. Storing the result in a local variable avoids the redundant second call in the error path.
Suggested change
| if fact != "*" && !slices.Contains(TargetFactNames(), fact) { | |
| if fact == FactAgent { | |
| return fmt.Errorf("invalid target %q: an agent cannot be a target, because a node's identity does not say which agents it hosts. Use allowed_agents to grant the agent namespaces a node may act for", target) | |
| } | |
| return fmt.Errorf("invalid target %q: %q is not a target fact, so nothing would ever match it (want %s or \"*\")", target, fact, strings.Join(TargetFactNames(), ", ")) | |
| } | |
| validFacts := TargetFactNames() | |
| if fact != "*" && !slices.Contains(validFacts, fact) { | |
| if fact == FactAgent { | |
| return fmt.Errorf("invalid target %q: an agent cannot be a target, because a node's identity does not say which agents it hosts. Use allowed_agents to grant the agent namespaces a node may act for", target) | |
| } | |
| return fmt.Errorf("invalid target %q: %q is not a target fact, so nothing would ever match it (want %s or \"*\")", target, fact, strings.Join(validFacts, ", ")) | |
| } |
Comment on lines
+627
to
639
| facts := make([]biscuit.Fact, 0, len(patterns)) | ||
| exact := make(map[string]bool) | ||
| for _, p := range patterns { | ||
| trimmed := strings.TrimPrefix(p, FactAgent+":") | ||
| if trimmed == "" { | ||
| continue | ||
| } | ||
| if trimmed == "*" || strings.HasPrefix(trimmed, "*.") || strings.HasSuffix(trimmed, ".*") { | ||
| facts = append(facts, BuildAgentDatalogFact(trimmed)) | ||
| continue | ||
| } | ||
| exact[trimmed] = true | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate wildcard patterns in patterns will result in duplicate facts being appended to the Biscuit token. Deduplicating wildcard patterns using a map prevents redundant facts, keeping the token size smaller and reducing authorization overhead.
facts := make([]biscuit.Fact, 0, len(patterns))
exact := make(map[string]bool)
seenWildcards := make(map[string]bool)
for _, p := range patterns {
trimmed := strings.TrimPrefix(p, FactAgent+":")
if trimmed == "" {
continue
}
if trimmed == "*" || strings.HasPrefix(trimmed, "*.") || strings.HasSuffix(trimmed, ".*") {
if !seenWildcards[trimmed] {
seenWildcards[trimmed] = true
facts = append(facts, BuildAgentDatalogFact(trimmed))
}
continue
}
exact[trimmed] = true
}| } | ||
| for _, label := range r.AllowedLabels { | ||
| if err := api.ValidateLabelPattern(label); err != nil { | ||
| return fmt.Errorf("in role %s: %w", r.Name, err) |
Contributor
There was a problem hiding this comment.
The error message for invalid label patterns is generic and does not specify which label pattern failed validation. Enhancing the error message to include the offending label pattern makes configuration errors much easier to debug.
Suggested change
| return fmt.Errorf("in role %s: %w", r.Name, err) | |
| return fmt.Errorf("invalid allowed_label %q in role %s: %w", label, r.Name, err) |
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
1 participant
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.
Fixes for audit reported by @kaisoz