node: operator-enforced egress label floor (egress.require_labels) - #385
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an operator-defined egress floor policy (egress.require_labels) to enforce a cryptographic boundary on all outbound requests, ensuring remote providers satisfy a conjunctive set of labels before receiving data. This floor is checked independently of caller-specified requirements and is applied during both provider ranking and label gate verification. The review feedback highlights a bug in the provider ranking logic where remote providers with partial gossip labels are incorrectly dropped early instead of only when their claims explicitly contradict the floor; a code suggestion is provided to fix this behavior.
| if floor := f.floor(); len(floor) > 0 { | ||
| // A local is settled here, having nothing to attest later. A | ||
| // remote is settled here only if it has claims to contradict; | ||
| // an unlabelled one goes on to the gate. | ||
| if (p.peerID == "" || len(labels) > 0) && !api.LabelsSatisfyFloor(floor, labels) { | ||
| recordFacadeRejection(reasonEgressFloorMismatch) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of the egress floor check for remote providers in rankProviders incorrectly drops any remote provider that has gossip labels but is missing some of the floor's keys, even if those labels do not contradict the floor.
For example, if the floor is {"jurisdiction": "eu", "compliance": "gdpr"} and a remote provider has gossip labels {"jurisdiction": "eu"}, len(labels) > 0 is true, but api.LabelsSatisfyFloor(floor, labels) returns false because the "compliance" key is missing. This causes the provider to be dropped early, even though it does not contradict the floor and might actually satisfy it when verified at the gate.
To align with the documented behavior ("A remote is dropped here only on a claim that already contradicts the floor"), we should check for explicit contradictions for remote providers, while still requiring full satisfaction for local services.
if floor := f.floor(); len(floor) > 0 {
// A local is settled here, having nothing to attest later. A
// remote is settled here only if it has claims to contradict;
// an unlabelled one goes on to the gate.
if p.peerID == "" {
if !api.LabelsSatisfyFloor(floor, labels) {
recordFacadeRejection(reasonEgressFloorMismatch)
continue
}
} else if len(labels) > 0 {
contradicts := false
for k, v := range floor {
if cv, ok := labels[k]; ok && cv != v {
contradicts = true
break
}
}
if contradicts {
recordFacadeRejection(reasonEgressFloorMismatch)
continue
}
}
}There was a problem hiding this comment.
Good catch, and a real bug I introduced — fixed in 33d55d1.
The point is exactly right: my own comment said "a remote is dropped here only when its gossiped claims already contradict the floor", and the code did not do that. LabelsSatisfyFloor is a conjunction, so a remote gossiping {jurisdiction: eu} against a floor of {jurisdiction: eu, compliance: gdpr} failed it on the missing key and was dropped — even though it may well attest both in its Biscuit. Gossip carries part of what a peer attests; treating silence as failure excludes providers that are inside the boundary.
Fixed by splitting the two predicates rather than inlining the loop, since they are a conceptual pair and the distinction is the interesting part:
LabelsSatisfyFloor— every pair holds. Used for a local service, whose labels are its own configuration and therefore complete, and which has no Biscuit and never reaches the gate.LabelsContradictFloor— some required key is stated with a different value. Absence is not contradiction. Used for a remote, where the gate resolves the rest on attested facts.
outside := api.LabelsContradictFloor(floor, labels)
if p.peerID == "" {
outside = !api.LabelsSatisfyFloor(floor, labels)
}No security change: the gate still enforces the floor conjunctively on attested facts, so ranking only stops being over-eager. The floor still holds in full for locals, which is where ranking is the only enforcement point.
Two tests were added for it, and I checked both bite. a remote gossiping only part of the floor is left to the gate fails on the old condition and passes on the new one, with the other six ranking cases unchanged. TestLabelsContradictFloorTreatsSilenceAsUnknown pins the one case the two predicates disagree on — a partial claim contradicts nothing but satisfies nothing either.
67f033b to
3b5259d
Compare
A caller's label requirement is a disjunction: LabelCheck emits
`check if label(a) or label(b)`, because a caller naming several labels
means any of them will do. An operator's egress floor cannot mean that.
A peer attesting only the most permissive of several alternatives would
satisfy the floor while sitting outside the boundary the operator drew.
LabelFloorCheck compiles the same input as a single conjunctive body --
`check if label("jurisdiction","eu"), label("compliance","gdpr")` -- so
every pair must hold. It takes a map, so a floor has no way to spell an
alternative in the first place.
Both are ordinary Biscuit checks, which is what lets a caller
requirement and a floor be combined by adding each to the authorizer and
letting it AND them; neither has to know the other exists.
LabelsSatisfyFloor is the non-attested counterpart, for the one provider
class with no Biscuit to check: a service local to the node.
Egress.RequireLabels carries the floor in the node config schema. It is a
block of its own rather than a field under attenuation, because the two
answer opposite questions -- attenuation is what this node demands of
peers that call it, egress is what it demands of peers it calls.
TestLabelFloorCheckIsConjunctionUnlikeLabelCheck pins the difference on
the compiled shape rather than the rendered string: a disjunction becomes
one query body per alternative, a conjunction one body carrying every
predicate. Making the floor a disjunction fails it.
No enforcement yet; that follows.
3b5259d to
c374b64
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an operator egress floor (egress.require_labels) to enforce outbound policies that every remote provider must attest before communication. Unlike caller-specified requirements which act as disjunctions (any label matches), the operator floor acts as a conjunction (all labels must match). The changes span policy parsing, validation, Biscuit check generation, caching, and provider ranking. The review feedback is highly constructive, suggesting an improvement to the cache key generation in labelGateKey to prevent potential delimiter collisions and optimize string allocations, as well as a minor optimization in openai_scorer.go to avoid a redundant function call for local providers.
| func labelGateKey(peerID peer.ID, required, floor map[string]string) string { | ||
| render := func(m map[string]string) string { | ||
| keys := make([]string, 0, len(m)) | ||
| for k := range m { | ||
| keys = append(keys, k) | ||
| } | ||
| sort.Strings(keys) | ||
| out := "" | ||
| for _, k := range keys { | ||
| out += "|" + k + "=" + m[k] | ||
| } | ||
| return out | ||
| } | ||
| // The separator keeps the two sets from running together, so a pair moved | ||
| // from one to the other cannot produce the same key. | ||
| return peerID.String() + render(required) + "#floor" + render(floor) | ||
| } |
There was a problem hiding this comment.
Using | and #floor as delimiters in the cache key can potentially lead to collisions if the label values contain these characters (e.g., if a value contains #floor). Since control characters are strictly forbidden in label keys and values by the validation logic, using a null byte (\x00) as a separator guarantees that no collisions can occur. Additionally, using strings.Builder is more efficient and avoids multiple string allocations in a loop.
| func labelGateKey(peerID peer.ID, required, floor map[string]string) string { | |
| render := func(m map[string]string) string { | |
| keys := make([]string, 0, len(m)) | |
| for k := range m { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| out := "" | |
| for _, k := range keys { | |
| out += "|" + k + "=" + m[k] | |
| } | |
| return out | |
| } | |
| // The separator keeps the two sets from running together, so a pair moved | |
| // from one to the other cannot produce the same key. | |
| return peerID.String() + render(required) + "#floor" + render(floor) | |
| } | |
| func labelGateKey(peerID peer.ID, required, floor map[string]string) string { | |
| render := func(m map[string]string) string { | |
| keys := make([]string, 0, len(m)) | |
| for k := range m { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| var sb strings.Builder | |
| for _, k := range keys { | |
| sb.WriteByte(0) | |
| sb.WriteString(k) | |
| sb.WriteByte('=') | |
| sb.WriteString(m[k]) | |
| } | |
| return sb.String() | |
| } | |
| return peerID.String() + render(required) + "\x00\x00floor\x00" + render(floor) | |
| } |
There was a problem hiding this comment.
Took the strings.Builder — that part is a clear improvement, avoiding the repeated concatenation in the loop. Kept the separators, though, because the premise doesn't hold:
Since control characters are strictly forbidden in label keys and values by the validation logic, using a null byte (
\x00) as a separator guarantees that no collisions can occur.
ValidateLabelValue rejects only ,, =, \n, \r and \t — \x00 is permitted in a label value. So moving to a null separator would trade an invariant that is actually checked for one that isn't.
What rules a collision out today is the =. Every entry is |<key>=<value>; ValidateLabelKey admits only [a-zA-Z0-9_.-] so a key can't hold a separator, and a value can't hold =, so it can't forge the |<key>= that begins an entry. A value may therefore contain | or #floor freely without moving any boundary.
Sketching the collision the suggestion is guarding against makes the point: for R1 + "#floor" + F1 == R2 + "#floor" + F2 with different inputs, the longer requirement's value would have to end ...#floor|key=..., and that needs an = inside a value.
I've written the reasoning into the function comment, since the next reader will reasonably have the same doubt, and added TestLabelGateKeyIsUnambiguousWithSeparatorsInValues, which builds keys from values that do contain |, #floor and #floor|b and asserts no two distinct inputs collide. It first asserts those values really are valid input, so the test fails loudly if the validator ever changes and the argument stops holding. I checked it bites: removing the #floor marker fails it.
Second point taken as-is in the same commit — for a local the contradiction result was computed and discarded, and the if/else reads better anyway.
| outside := api.LabelsContradictFloor(floor, labels) | ||
| if p.peerID == "" { | ||
| outside = !api.LabelsSatisfyFloor(floor, labels) | ||
| } |
There was a problem hiding this comment.
The current implementation always calls api.LabelsContradictFloor(floor, labels) first, but if the provider is local (p.peerID == ""), its result is immediately discarded and overwritten by !api.LabelsSatisfyFloor(floor, labels). This is slightly inefficient. Using an if-else block avoids the redundant call for local providers.
var outside bool
if p.peerID == "" {
outside = !api.LabelsSatisfyFloor(floor, labels)
} else {
outside = api.LabelsContradictFloor(floor, labels)
}Every egress label requirement came from the caller: the X-Sam-Required-Labels header on the inference and a2a surfaces, and the required_labels parameter on call_remote_tool. A caller that sent none was unconstrained, so the jurisdictional boundary was opt-in by the party it was meant to constrain. Local attenuation already gives the operator enforced control over who may call in; this is the same for who the node will call out to. egress.require_labels in sam-node.yaml is now a floor every provider must attest before the node sends it anything. Absent, nothing changes. Three parts, of which the first is the one that matters: - VerifyPeerLabels returned early when the caller required nothing. With a floor configured there is always something to check, so that short-circuit now accounts for it; leaving it would have meant a floor that any caller could waive by staying silent, which is the same fail-open shape as an empty requirement set. - checkPeerLabels adds the caller's check and the floor's check separately and lets the authorizer AND them. Merging them into one disjunction would let the caller's pairs stand in for the floor's, so a caller naming an unrelated label could widen exactly what the floor exists to bound. - rankProviders applies the floor too, because a local service has no biscuit and never reaches the gate; for a local, ranking is the only enforcement point there is. A remote is dropped here only when its gossiped claims already contradict the floor -- an unlabelled one goes on to the gate, which decides on attested facts. The gate's cache key now includes the floor. Keying on the caller's requirement alone would let a verdict reached under one floor be replayed under another after a config change. A malformed floor fails at load rather than at first use, so an operator who wrote one finds out at startup instead of on the request that needed it. The strict schema means a misspelled key is refused rather than silently leaving the node with no floor. Rejections are counted under their own reason, egress_floor_mismatch, so an operator can tell their floor apart from a caller's requirement when a request finds no provider. Tests cover the cases the design turns on: a caller requiring nothing is still gated; one pair short of the floor is not enough; a caller cannot widen the floor by naming another label; a local outside the floor is dropped; an unlabelled remote is left to the gate; and with no floor configured every path behaves as before.
The sovereignty checklist told operators to "Direct agents to specify X-Sam-Required-Labels: jurisdiction=eu on all inference and MCP requests to guarantee zero data leakage beyond authorized perimeters". Directing an agent is not a guarantee: the field's own contract says absent means any provider policy allows, so an agent that omitted the header left the perimeter open. That mattered in a document that cites GDPR Chapter V and the EU Cloud Sovereignty Framework, where a reader may be deciding whether the control satisfies an obligation. It now points at egress.require_labels, which is enforced, and says what the header can still do: narrow further, never widen or waive. node-configuration gains the block alongside the header it complements, with the asymmetry in meaning stated plainly, since the same YAML shape means different things on the two sides: X-Sam-Required-Labels (caller) -> any pair is enough egress.require_labels (operator) -> every pair must hold That follows from what each is for -- a caller is choosing among acceptable providers, an operator is drawing a boundary -- but it is not guessable from the syntax, so it is written down.
c374b64 to
fe7b1e4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces an operator-defined egress floor (egress.require_labels) in the node configuration, allowing operators to enforce a baseline set of labels that all providers must satisfy. Unlike the caller's request-specific labels (which use disjunctive 'OR' semantics), the egress floor uses conjunctive 'AND' semantics, requiring every specified label to be attested. The changes include compiling the floor into Biscuit checks, updating the peer label verification gate and provider ranking logic to enforce the floor, and adding comprehensive unit tests at the lowest adequate layer in accordance with the test pyramid. Documentation has also been updated to reflect these changes. I have no feedback to provide as the implementation is robust, well-tested, and adheres to the repository's style guides.
The floor was enforced inside VerifyPeerLabels and checkPeerLabels, but every egress surface gated the *call* to them on the caller's own requirement: the facade forwarded, a2a proxied and ConnectMCPSession handed the stream to the SDK without ever reaching the gate when no labels were required. A caller could therefore waive the floor by staying silent -- the exact fail-open shape the floor exists to close. Ranking did not catch it, because gossip labels are self-asserted and an unlabelled remote is deliberately left "to the gate". Enforced now in two places: - The /sam/ egress proxy, so the floor holds at the chokepoint whatever the surface: a2a, the facade's forward, and an agent that skips both and dials /sam/<peer>/... raw. Confinement must not depend on the agent's cooperation. required is nil there; the caller's own requirement, when present, is still enforced by the surface that parsed it. - ConnectMCPSession, which reaches peers over a libp2p stream and not the proxy: the gate now also runs when only the floor requires it. The facade's forward loop runs the gate too when only the floor demands it, so an ineligible provider is skipped in favour of the next instead of surfacing the chokepoint's 403, and a floor failure with no caller requirement is attributed to egress_floor_mismatch rather than the caller-facing label_unattested. The regression tests pin the whole path, not the gate function: a silent caller on the facade is still gated and fails closed without a seam; a raw /sam/ GET inside the floor passes and one pair short is 403 on attested facts; CallMCPTool with nil required_labels is held to the floor. All three fail on the previous commit.
|
/gemini review pushed a new commit with one bug I found |
There was a problem hiding this comment.
Code Review
This pull request introduces an operator-defined 'egress floor' (egress.require_labels) to enforce strict outbound boundaries that callers cannot waive or widen. The changes span policy configuration, startup validation, caching, and enforcement across MCP, OpenAI facade, and raw egress proxy pathways, accompanied by extensive tests and documentation. Feedback from the review highlights a security-critical violation of Style Guide §1 in internal/node/sidecar.go where a raw Peer ID is used instead of its canonicalized form, along with opportunities for defensive programming checks in api/datalog.go's floor evaluation functions.
| func LabelsSatisfyFloor(floor, claimed map[string]string) bool { | ||
| for k, v := range floor { | ||
| if claimed[k] != v { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
For defensive programming and clarity, explicitly handle the case where claimed is nil or floor is empty. Although map indexing on a nil map is safe in Go, explicit checks make the code more robust and self-documenting.
| func LabelsSatisfyFloor(floor, claimed map[string]string) bool { | |
| for k, v := range floor { | |
| if claimed[k] != v { | |
| return false | |
| } | |
| } | |
| return true | |
| } | |
| func LabelsSatisfyFloor(floor, claimed map[string]string) bool { | |
| if len(floor) == 0 { | |
| return true | |
| } | |
| if claimed == nil { | |
| return false | |
| } | |
| for k, v := range floor { | |
| if claimed[k] != v { | |
| return false | |
| } | |
| } | |
| return true | |
| } |
| func LabelsContradictFloor(floor, claimed map[string]string) bool { | ||
| for k, v := range floor { | ||
| if got, stated := claimed[k]; stated && got != v { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
For defensive programming, explicitly handle the case where claimed is nil or floor is empty. This avoids executing the loop when there are no claims or no floor constraints.
| func LabelsContradictFloor(floor, claimed map[string]string) bool { | |
| for k, v := range floor { | |
| if got, stated := claimed[k]; stated && got != v { | |
| return true | |
| } | |
| } | |
| return false | |
| } | |
| func LabelsContradictFloor(floor, claimed map[string]string) bool { | |
| if len(floor) == 0 || claimed == nil { | |
| return false | |
| } | |
| for k, v := range floor { | |
| if got, stated := claimed[k]; stated && got != v { | |
| return true | |
| } | |
| } | |
| return false | |
| } |
The Android node reuses internal/node wholesale -- NewSamNode, the sidecar, the /sam/ egress proxy -- so every enforcement point of the egress floor already applies to it. What it could not do was *have* a floor: MobileConfig carried labels, services and attenuation but no egress block, so EgressRequireLabels was always nil on mobile and the sovereignty boundary could not be drawn on the one node class most likely to roam across jurisdictions. MobileConfig gains the egress block, passed through CompleteNodeConfig like the rest, so a malformed floor fails StartNode the same way it fails a desktop node at startup. The decode test pins the JSON spelling (requireLabels, matched case-insensitively to the Go field, not yaml's require_labels), because api.Egress carries only yaml tags and a silently missed key would start the node with no floor while the operator believes one is set -- the same hazard the attenuation decode test already pins.
A peer ID has more than one valid spelling (base58 multihash and CID), and the floor gate decoded the path's segment for its verdict while letting the original spelling travel on to the proxy Director and the log line -- so the gate and the dial could name the same peer in different forms. Per the style guide, peer IDs are canonicalized at the boundary: the gate now rewrites the path segment to the canonical form when the caller spelled it otherwise, and logs the same. The floor test gains the alias case: a CID-spelled peer inside the floor passes end to end.
2 participants
Fixes #383. Implements Option B as you specified it, including the two independent checks.
What it adds
Absent, nothing changes: the requirement is whatever the caller supplied, and a caller supplying nothing is unconstrained, exactly as today. The floor only exists for operators who ask for it.
Three commits
bac1401api —LabelFloorCheckcompiles the floor as a conjunction (check if label("jurisdiction","eu"), label("compliance","gdpr")), unlikeLabelCheck's disjunction. It takes a map, so a floor has no way to spell an alternative — which is the point: a peer attesting only the most permissive of several alternatives would satisfy the floor while sitting outside the boundary. PlusLabelsSatisfyFloorfor the one provider class with no Biscuit, andEgress.RequireLabelsin the schema.82acca4node — enforcement, in three places. The first is the one that matters.67f033bdocs — see below; this one is not cosmetic.The three enforcement points, and why each is needed
1.
VerifyPeerLabelsreturned early when the caller required nothing. With a floor configured there is always something to check, so that short-circuit now accounts for it. Leaving it would have produced a floor any caller could waive by staying silent — the same fail-open shape as an empty requirement set, which is what #348 was about.2.
checkPeerLabelsadds the two checks separately and lets the authorizer AND them, as you wrote:Merging them into one disjunction would let the caller's pairs stand in for the floor's. Pinned by
a caller cannot widen the floor by naming another label: floorjurisdiction=eu, callerregion=us-east-1, provider attestsjurisdiction=us, region=us-east-1→ rejected. Merged, it would pass.3.
rankProvidersapplies the floor too — this one wasn't in the issue and is worth a look. A local service has no biscuit and never reaches the gate, so for a local, ranking is the only enforcement point there is;openai_scorer.goalready noted the same asymmetry for caller requirements ("Locals have no gate, so their declared labels stay fail-closed here"). A remote is dropped here only when its gossiped claims already contradict the floor; an unlabelled one goes on to the gate and is decided on attested facts, so gossip is still never trusted for authorization.Two details you may want to check
The gate cache key now includes the floor. Keying on the caller's requirement alone would let a verdict reached under one floor be replayed under another after a config change.
TestLabelGateKeySeparatesFloorFromRequirementpins that a requirement and a floor carrying the same pair don't collide.A malformed floor fails at load, not at first use — otherwise it would fail open on the request that needed it. And since the schema is strict,
required_labelsmisspelled forrequire_labelsis refused rather than silently leaving the node with no floor.On the kill switch
Left working and documented as such, rather than special-cased. It falls out of the semantics and reads as useful.
Docs — this is why I filed the issue
The sovereignty checklist said:
Directing an agent isn't a guarantee, and the field's own contract says absent means any provider policy allows. In a document citing GDPR Chapter V and the EU Cloud Sovereignty Framework, a reader may be deciding whether the control satisfies an obligation — so it now points at
egress.require_labelsand states what the header can still do (narrow, never widen or waive).node-configurationgains the block next to the header it complements, with the asymmetry written down, since the same YAML shape means different things on the two sides:X-Sam-Required-Labels(caller)egress.require_labels(operator)Tests
api: the conjunction/disjunction difference is pinned on the compiled shape, not the rendered string — a disjunction becomes one query body per alternative, a conjunction one body carrying every predicate. I verified the test bites by temporarily making the floor a disjunction; it fails.node: caller requiring nothing is still gated; one pair short is not enough; caller cannot widen; caller narrows within the floor; local outside the floor dropped; local one pair short dropped; unlabelled remote left to the gate; no floor configured leaves every path unchanged; malformed and misspelled config rejected at load.Each commit was checked out in a separate worktree and built and vetted on its own, so the branch is bisectable.
Local verification caveat
Six tests in
internal/nodefail on my Windows workstation with and without this change — the Unix-socket and subprocess-backend ones (TestListenLocalSocket,TestSidecarSocketAuthorizesWithoutToken,TestStaticServiceRegistration*,TestBaseService_InitCommandBackend_BuildsBridge,TestIdentityEvidenceTrailingSlashReturnsNotFound). Same set on a clean checkout of the merge base.make lintandmake e2e-testneed a Linux toolchain I don't have locally, so CI is the authority.