Repository navigation
node: check its own ingress grant and cap the body on /mesh/ingress - #851
Merged
Merged
Conversation
The node served an external ingress request forwarded by an authenticated router after checking the service's own ingress opt-in and the router's role, but not whether its own role grants exposing that service in that auth mode. The router checks that grant at announcement and on every request (internal/router/directory.go, internal/router/ingress.go), so an honest router never forwarded such a request, and in edge mode the node verifies the external caller's Biscuit itself. A router that skipped the check could still reach a passthrough service that the admin's allowed_ingress ceiling had not granted, with no caller credential at all. Zero trust between routers and nodes means the node does not take the router's word for it. The node now runs identity.RequireIngress against its own credential, under the mesh policy rules it holds, before it dispatches. A role without the grant answers 403 and counts a deny in agentmesh_node_external_ingress_requests_total; an authorizer that stops at a run limit before a verdict answers 504 (time) or 500 (facts or iterations), counted as undecided, as the label gate does. RequireIngress wraps the authorizer error instead of dropping it, so the two can be told apart. The router caps a request body at 32 MiB on its public listener; the node applied no cap on /mesh/ingress/1.0.0. The cap moves to api.MaxIngressBodyBytes and the node applies it too. The meshnet fake control planes grant mesh:role:node edge ingress on every service, as agentmesh-one's development policy does, and an explicit passthrough grant for the one service a test exposes that way.
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements defense-in-depth for external ingress by having the node verify its own ingress grant against its credential and mesh policy, rather than relying solely on the router's checks. It also standardizes and enforces request body limits (32 MiB) at both the router and node levels, accompanied by comprehensive unit tests and documentation updates. The review feedback recommends caching the verified node identity and Biscuit key on the node to prevent CPU-intensive cryptographic verification on every ingress request.
| if err != nil { | ||
| return err | ||
| } | ||
| b, key, err := identity.VerifyBiscuitAndGetKey(token, pid, keys, n.BiscuitTimeout) |
Contributor
There was a problem hiding this comment.
Performance Optimization: Cache Verified Node Identity\n\nCryptographically verifying the node's own Biscuit token (identity.VerifyBiscuitAndGetKey) on every single external ingress request is CPU-intensive and can become a performance bottleneck under high request volumes. Since the node's own identity token and the trusted control plane keys change very infrequently (only during credential refresh), the verified Biscuit b and verifying key should be cached on the AgentMeshNode struct (e.g., protected by a mutex or stored in an atomic.Pointer / sync.Once pattern) and invalidated/re-verified only when the identity token is updated.\n\nThis avoids redundant cryptographic signature verification and Datalog parsing on the hot path of every ingress request.
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.
Hardening of the external ingress path ahead of v0.2.0, from a review of #838 and its follow-ups.
The node did not check its own role ceiling
The node's
/mesh/ingress/1.0.0handler checked the service's owningress:opt-in and the forwarding router'smesh:role:routercredential, but not whether its own role grants exposing that service in that auth mode. The router checks the grant at announcement and on every request, so an honest router never forwarded such a request, and inedgemode the node verifies the external caller's Biscuit itself. What was open: a router that skipped the check could reach apassthroughservice that the admin'sallowed_ingressceiling had not granted, with no caller credential at all.The node now runs
identity.RequireIngressagainst its own credential, under the mesh policy rules it holds, before it dispatches:403,agentmesh_node_external_ingress_requests_total{outcome="deny"}, a warning naming the service, mode and router;504(time) or500(facts or iterations),outcome="undecided", as the label gate already does for providers (node: tell an authorizer run limit apart from a provider denial #842).RequireIngresswraps the authorizer error with%winstead of dropping it so the two cases can be told apart.Body cap on the router-to-node hop
The router caps a request body at 32 MiB on its public listener; the node applied no cap on
/mesh/ingress/1.0.0. The constant moves toapi.MaxIngressBodyBytesand the node applies it too.Not done here, on purpose
ReadTimeouton either ingress server: innet/httpthe read deadline also fires the connection's background read, which cancels the request context of a long streaming response (MCP and A2A SSE through ingress). A bounded body read deadline needs a different mechanism and a test with a streaming backend; separate change.Tests
TestExternalIngressHandler: the serving role now carries anallowed_ingressgrant (it had none and passed because nothing checked); new cases for an opted-in service the role does not grant (403, deny counted, backend not called) and for a body over the cap (*http.MaxBytesErrorat the backend).TestRequireOwnIngressGrant: granted, mode not granted, role without grant, no credential, and the facts-limit case reported as a run limit rather than a refusal.sdk/meshnetfake control planes grantmesh:role:nodeedge ingress on*, asagentmesh-one's development policy does, plus an explicit passthrough grant for the one service a test exposes that way.go test -raceonapi,internal/identity,internal/node,internal/router,internal/standalone,sdk/meshnet;TestStandaloneExternalIngressCUJandTestFederatedMultiRouterIngressRoleCeilingAndBan;make lint.Docs: the external ingress guide's request flow states where the ceiling and the body cap are enforced.