feat: add private ingress request boundary - #5922
Conversation
|
|
Running ultrareview automatically — This adds a new security-critical network ingress boundary with TokenReview verification, credential caching, rate limiting, and fail-closed middleware—any subtle flaw could bypass authentication, so an ultrareview is warranted.. I'll post findings when complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 27m 10s
All reported issues were addressed across 22 files
Linked issue analysis
Linked issue: AIS-667: Attestor, TokenReview verifier, and isolated private listener
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | TokenReview validates audience, service-account subject, and token expiry; inactive or mismatched ingress combinations fail closed. | The verifier sends the required audience, accepts only service-account subjects, rejects missing or expired JWTs, looks up active ingress bindings, and rechecks serving state for disabled, deleted, or changed ingresses. |
| ✅ | Host mismatches are rejected and private ingress authority is bound to the verified ingress. | Both the attestor and private middleware canonicalize Host and require it to match the expected or verified ingress DNS name. |
| ✅ | The private listener exposes only the intended MCP/OAuth/install routes and no RPC, admin, hooks, marketplace, callback, or other global routes. | A single private route catalog drives route guarding and private handler registration, with explicit reserved-path rejection and route-census coverage. |
| ✅ | Attestation does not overwrite MCP Authorization, unsupported identity headers are stripped, and attestation tokens are absent from logs and errors. | The attestor replaces only the dedicated attestation header, preserves Authorization, strips unsupported Tailscale and client-IP headers, and error paths use generic messages without including the token. |
| Streaming POST/SSE/DELETE requests and cancellation pass through the attestor and private listener. | The route catalog registers POST, GET/SSE, and DELETE runtime methods, and an in-process SSE cancellation test verifies propagation, but the supplied evidence does not independently exercise streaming POST and DELETE cancellation. |
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
d56988e to
ad1bd9c
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
All reported issues were addressed across 16 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Addressed the Ultrareview summary from #5922 (review) across ad1bd9c and 764ac80. The code now re-resolves private endpoints from live ingress authority, requires TLS and explicit CA trust, disables ambient proxying, scopes serving-state rechecks by organization, reserves xMCP callbacks, bounds/coalesces verification work, unifies the reserved header, handles IPv6 authority safely, and tightens identity parsing. The wildcard attestor listener remains intentionally pod-reachable; AIS-608 per-ingress NetworkPolicy is the required source boundary before deployment. All 13 inline threads have detailed replies and are resolved. |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
0b72fe5 to
7570eed
Compare
|
Addressed the remaining actionable Cubic identity-parsing feedback in commit |
7570eed to
8677957
Compare
8677957 to
9906711
Compare
1 participant
Why
Private MCP traffic needs a serving boundary that can prove which provisioned ingress forwarded a request before trusting its Host or advisory network identity. The public application mux cannot provide that isolation because it also exposes management, callback, hook, and marketplace surfaces.
What changed
/app/gram netingress-attestorwith a separate pod-local health listener. It allowlists private MCP/OAuth/install routes, overwrites workload attestation from a rotating projected token, preserves MCPAuthorization, strips unsupported identity/client-IP headers, and supports streaming and cancellation.GRAM_NETINGRESS_ADDRESSprivate application listener with its own mux, observability/recovery middleware, serving-policy version, TokenReview boundary, and concurrent graceful drain./mcpand/x/mcphandler registration, preventing allowlist/mount drift and excluding global callback paths.Review notes
agentlink/ais-666-private-oauth-authority). Review only the AIS-667 commits on this branch.server/internal/netingress/attestation.go,middleware.go,attestor.go, androutes.go; composition and shutdown wiring are inserver/cmd/gram/start.go.GRAM_NETINGRESS_ADDRESSis empty. This PR does not deploy the listener, grant product entitlement, enable the PostHog rollout, or add Kubernetes/NetworkPolicy resources; those remain separate follow-up tickets.Validation
mise run test:server ./internal/netingress ./internal/middleware ./cmd/gram— 302 tests passedmise run test:server -race ./internal/netingress— 63 tests passedmise run test:server ./internal/xmcp— 54 tests passedmise run test:server ./internal/mcp— 667 tests passedmise build:servermise lint:servergit diff --check0b72fe5d32remains pendingLinear: AIS-667
Rollout / deployment notes
GRAM_NETINGRESS_ADDRESSdefaults empty, so existing deployments keep only the public listener.GRAM_NETINGRESS_TLS_CERT_FILEandGRAM_NETINGRESS_TLS_KEY_FILE.GRAM_NETINGRESS_UPSTREAM_CA_FILE, expected private Host, and projected ServiceAccount token path. Its dedicated transport ignores ambient proxy variables.