refactor(network): consolidate proxy egress pipeline - #2373
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-2373.docs.buildwithfern.com/openshell |
96addbc to
0c67990
Compare
|
/ok to test 0c67990 |
|
Label |
|
/ok to test 79d2123 |
|
/ok to test 1d568e3 |
|
/ok to test 92efd90 |
|
/ok to test 344a2eb |
|
/ok to test 0f9a6bf |
| enum GlobToken { | ||
| Literal(char), | ||
| Star { crosses_delimiter: bool }, | ||
| } |
There was a problem hiding this comment.
The glob we use for matching endpoint to the policy supports other placeholders, like ? and brackets:
OpenShell/crates/openshell-supervisor-network/src/l7/mod.rs
Lines 890 to 893 in 1d568e3
For example if the policy has foo.com/v? and foo.com/v1 these won't be considered ambiguous right now.
| entrypoint_pid: u32, | ||
| require_binary_identity: bool, | ||
| ) -> Result<Self> { | ||
| let ambiguities = openshell_policy::find_endpoint_ambiguities(proto); |
There was a problem hiding this comment.
Could we also run this validation in the gateway before persisting changes whose affected scope is known, while keeping this runtime check as defense in depth?
The current behavior differs depending on how the effective policy changes:
- A direct policy set/update persists a new pending base-policy revision first. Ambiguity is discovered only when the supervisor reloads it, so
--waiteventually fails, but without--waitthe mutation reports success and the invalid revision remains in history. - Approving a policy chunk merges it into and persists a new base-policy revision, then marks the chunk approved before activation. That revision can still fail here.
- Attaching a provider persists the attachment via CAS but creates no new policy revision and does not wait for activation. The composed effective policy changes asynchronously, and a conflict is reported against the existing base-policy version.
- Updating a provider profile has fan-out: with provider policy composition enabled, the profile is resolved dynamically and changes the effective policy/hash of every sandbox in scope that has a provider of that profile attached, without rewriting any base-policy revision.
For the first three single-sandbox paths, could the gateway compose the candidate effective policy and call find_endpoint_ambiguities before the write? That would return an immediate FAILED_PRECONDITION and avoid persisting an invalid revision, approved chunk, or attachment. Profile update is broader, but profile_attached_sandbox_diagnostics already enumerates affected sandboxes for token-grant conflicts; ambiguity validation could potentially join that preflight and reject the whole profile update if any affected sandbox would become invalid.
I think the runtime validation should remain regardless, to cover races, external profile sources, global changes, and defense in depth. The open semantic question for fan-out changes is whether one incompatible sandbox should reject the entire profile update or allow the update and quarantine only that sandbox. If the latter is intentional, it likely needs an activation status keyed to the effective config revision rather than only the existing base-policy version.
There was a problem hiding this comment.
[agent-review]
Agent infrastructure also needs a companion update: .agents/skills/generate-sandbox-policy/SKILL.md:442 still says all overlapping endpoint coverage causes OPA evaluation errors. This PR permits compatible overlaps and adds rejected-generation quarantine behavior, so the policy-generation, CLI, and cluster-debugging skills should reflect the new semantics.
Focused verification passed: 11 ambiguity tests, 4 relay-generation tests, and 4 validation-failure posture tests.
| opa_engine: &OpaEngine, | ||
| decision: &EgressDecision, | ||
| ) -> Option<PolicyGenerationGuard> { | ||
| let expected_generation = route.map_or(decision.l4_policy_generation, |route| { |
There was a problem hiding this comment.
[P1] Preserve the L4 authorization generation here
For CONNECT, policy can reload after authorize_egress_intent returns but before hydrate_l7_route. This code then replaces the old allow decision's generation with the newer route-lookup generation. If the newer generation denies the destination or represents fail-closed quarantine, the endpoint query can still produce a current-generation snapshot, so the stale L4 allow receives a current guard and remains active. The forward-proxy path explicitly pins L4 and rejects an L7 generation mismatch. CONNECT needs the same invariant: pin decision.l4_policy_generation before hydration and reject every later snapshot from another generation.
| !endpoint.protocol.is_empty() || !endpoint.allowed_ips.is_empty() || !endpoint.tls.is_empty() | ||
| } | ||
|
|
||
| fn request_pipeline_conflicts(left: &NetworkEndpoint, right: &NetworkEndpoint) -> Vec<String> { |
There was a problem hiding this comment.
[P1] Include WebSocket GraphQL classification in ambiguity checks
websocket_graphql_policy is derived from operation rules and persisted-query configuration, but this comparison omits that derived value while allowing rule differences. Two equal-rank overlapping WebSocket endpoints can therefore pass validation while one config selects GraphQL message inspection and the other selects generic transport inspection. Route selection then chooses one config; if transport inspection wins, operation-specific GraphQL deny rules receive no GraphQL metadata and a generic WEBSOCKET_TEXT allow can pass. Please compare the derived classification or aggregate it deterministically.
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
0f9a6bf to
b913aa0
Compare
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>
| &normalized_body_limit(right.graphql_max_body_bytes), | ||
| ); | ||
| } | ||
| if is_json_rpc_family(&left.protocol) && is_json_rpc_family(&right.protocol) { |
There was a problem hiding this comment.
is_json_rpc_family() covers both json-rpc and mcp; it's possible for this condition to be true when left is one and right is the other. It seems like it should be an exact match like graphql above and mcp below, or this block should be moved below the mcp block.
Summary
Refactor sandbox proxy egress so CONNECT and forward-proxy traffic share explicit destination authorization and relay primitives while preserving the existing user-facing network behavior. This establishes the RFC #2155 foundation without introducing the later DNS or arbitrary TCP features, and keeps the shared path compatible with the middleware framework from #2027.
User-facing behavior
policy_validation_failure_mode = "retain_last_valid"under[openshell.gateway]ingateway.toml.Related Issue
Related design: #2155
Related middleware implementation: #2027
Changes
[openshell.gateway] policy_validation_failure_modeingateway.toml; default tofail_closedand allow the explicitretain_last_validavailability mode.Testing
mise run pre-commitpasses after rebasing onto currentmainmise run helm:lintmise run helm:test(64 passed)Checklist