From b4f81f7e5cfaf3272a0a7c84bda5e4b513e1118a Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 2 Sep 2026 15:17:51 +0200 Subject: [PATCH 1/3] fix(server): gate only newly authored uninspected credentialed endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attaching a credentialed provider made every later `policy set` and `policy update` fail, even for unrelated edits: credentialed endpoint 'github.com:443' in rule 'pypi' uses L4-only; configure L7 inspection or explicitly set allow_uninspected_credentials: true The rule named in the error is one the operator never wrote. A sandbox image's baked policy reaches the store through the supervisor sync path, which skips the credential gate by design — rejecting that delivery would crash-loop the sandbox. Sandbox create passes too, because it only sees the request's policy plus the provider layers. Endpoint stamping then marks any endpoint overlapping a credentialed provider's host and port, so the image's L4-only `github.com:443` becomes credentialed, and every operator edit was re-validated against the whole effective policy and rejected. The operator could not even edit the offending endpoint, because that edit was rejected as well. Gate the difference on edit paths instead. An edit is rejected for the uninspected credentialed endpoints it introduces; endpoints the current policy already carries are logged and admitted. Endpoint identity is the rule name, host, port, and uninspected mode, so moving an inherited endpoint from L4-only to `tls: skip` is a fresh authoring act. Stamping is untouched and stays a full recomputation. Sandbox create and provider attach keep the strict whole-policy gate, since attaching a credentialed provider is a new credential exposure. Nothing is granted by admitting an inherited endpoint: the network supervisor still denies that traffic. Refs: #2998 Signed-off-by: Philippe Martin --- architecture/security-policy.md | 17 + crates/openshell-server/src/grpc/policy.rs | 493 ++++++++++++++++++++- docs/sandboxes/policies.mdx | 2 +- 3 files changed, 490 insertions(+), 22 deletions(-) diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 9203ba3178..500af8293e 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -135,6 +135,23 @@ flag defaults to `false` and is security-flagged in policy approval flows. Incremental merges only ever add the flag to a matching endpoint; clearing it requires removing the endpoint or replacing the policy. +Sandbox create and provider attach gate the whole effective policy. Edits to an +existing sandbox policy — `policy set` and `policy update`, and the proposal +evaluation behind them — gate the difference instead: an edit is rejected for +the uninspected credentialed endpoints it introduces, while endpoints the +sandbox's current policy already carries are logged and admitted. An endpoint's +identity for this comparison is its rule name, host, port, and uninspected mode, +so moving an inherited endpoint from L4-only to `tls: skip` is a fresh authoring +act rather than the same finding carried forward. Stamping is unaffected and +stays a full recomputation. + +The gate has to work this way because a supervisor syncing the policy baked into +a sandbox image reaches the store without passing it — rejecting that delivery +would crash-loop the sandbox. Gating the whole policy on every later edit would +make a policy the gateway itself admitted permanently uneditable, including the +edit that would fix the offending endpoint. Nothing is granted by admitting an +inherited endpoint: the network supervisor still denies that traffic. + The network supervisor independently enforces the same boundary. Credentialed WebSocket upgrades use the parsed relay, binary frames fail closed, and text placeholders require rewrite. REST bodies can continue streaming when body diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 0e4c74af95..8dc732cdd6 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -693,7 +693,14 @@ fn evaluate_proposal_candidate( }; let bindings = policy_static_credential_endpoint_bindings(Some(&effective))?; if let Some(context) = validation_context.credential_binding { - validate_operator_merged_credential_policy(&mut effective, &bindings, context)?; + let inherited = + inherited_uninspected_credentialed_endpoints(base_policy, validation_context); + validate_operator_merged_credential_policy( + &mut effective, + &bindings, + context, + &inherited, + )?; } Ok(effective) })(); @@ -2130,6 +2137,35 @@ pub(super) async fn validate_candidate_sandbox_credential_policy( validate_uninspected_credentialed_endpoints(&effective) } +/// Credential gate for a full policy replacement on an existing sandbox. Only +/// the uninspected credentialed endpoints this replacement introduces are +/// rejected; those the sandbox's current policy already carries are inherited, +/// so an operator can round-trip `policy get --base` through `policy set` — and +/// can edit the offending endpoint — instead of being locked out by a rule the +/// sandbox image supplied. +pub(super) async fn validate_updated_sandbox_credential_policy( + state: &ServerState, + workspace: &str, + sandbox: &Sandbox, + provider_names: &[String], + policy: &ProtoSandboxPolicy, +) -> Result<(), Status> { + let catalog = state + .provider_profile_sources + .snapshot_catalog(state.store.as_ref(), workspace) + .await?; + let current_base = current_base_policy_for_sandbox(state.store.as_ref(), sandbox).await?; + let current_effective = + effective_policy_for_source(state, &catalog, workspace, provider_names, current_base) + .await?; + let inherited = uninspected_credentialed_endpoint_keys(¤t_effective); + + let effective = + effective_policy_for_source(state, &catalog, workspace, provider_names, policy.clone()) + .await?; + validate_uninspected_credentialed_endpoints_excluding(&effective, &inherited) +} + fn truncate_for_log(input: &str, max_chars: usize) -> String { let mut chars = input.chars(); let truncated: String = chars.by_ref().take(max_chars).collect(); @@ -2918,13 +2954,36 @@ struct UninspectedCredentialedEndpoint { mode: &'static str, } +impl UninspectedCredentialedEndpoint { + fn key(&self) -> UninspectedCredentialedEndpointKey { + UninspectedCredentialedEndpointKey { + rule_name: self.rule_name.clone(), + host: self.host.to_ascii_lowercase(), + port: self.port, + mode: self.mode, + } + } +} + +/// Identity of an uninspected credentialed endpoint. `mode` participates: moving +/// an inherited L4-only endpoint to `tls: skip` is a fresh authoring act, not the +/// same finding carried forward. +#[derive(Clone, PartialEq, Eq, Hash)] +struct UninspectedCredentialedEndpointKey { + rule_name: String, + host: String, + port: u32, + mode: &'static str, +} + /// Scan the effective policy for credentialed endpoints on uninspected modes. /// /// Explicit opt-ins are logged and skipped. Never logs credential names, /// placeholders, or secret values. -fn find_uninspected_credentialed_endpoint( +fn collect_uninspected_credentialed_endpoints( policy: &ProtoSandboxPolicy, -) -> Option { +) -> Vec { + let mut violations = Vec::new(); for (rule_name, rule) in &policy.network_policies { for endpoint in &rule.endpoints { if !endpoint.provider_credentialed { @@ -2950,7 +3009,7 @@ fn find_uninspected_credentialed_endpoint( continue; } - return Some(UninspectedCredentialedEndpoint { + violations.push(UninspectedCredentialedEndpoint { rule_name: rule_name.clone(), host: endpoint.host.clone(), port: endpoint_ports(endpoint) @@ -2961,13 +3020,76 @@ fn find_uninspected_credentialed_endpoint( }); } } - None + violations } -/// Admission gate for policy-authoring paths (create, attach, operator config -/// update). Rejects credentialed endpoints that would lose L7 inspection. +fn find_uninspected_credentialed_endpoint( + policy: &ProtoSandboxPolicy, +) -> Option { + collect_uninspected_credentialed_endpoints(policy) + .into_iter() + .next() +} + +/// The uninspected credentialed endpoints an already-stamped policy carries. +/// +/// A sandbox syncing the policy baked into its image reaches the store without +/// passing the admission gate, by design: rejecting that delivery would +/// crash-loop the supervisor. Operator edits inherit those endpoints instead of +/// being blocked by them, so an unrelated change is not held hostage by a rule +/// the operator never authored — and so the operator can edit the offending +/// endpoint itself. +fn uninspected_credentialed_endpoint_keys( + policy: &ProtoSandboxPolicy, +) -> HashSet { + collect_uninspected_credentialed_endpoints(policy) + .iter() + .map(UninspectedCredentialedEndpoint::key) + .collect() +} + +/// Admission gate for policy-authoring paths (sandbox create, provider attach). +/// Rejects credentialed endpoints that would lose L7 inspection. fn validate_uninspected_credentialed_endpoints(policy: &ProtoSandboxPolicy) -> Result<(), Status> { - let Some(violation) = find_uninspected_credentialed_endpoint(policy) else { + validate_uninspected_credentialed_endpoints_excluding(policy, &HashSet::new()) +} + +/// Admission gate for edits to an existing sandbox policy. Rejects only the +/// credentialed endpoints the edit introduces; endpoints already present in +/// `inherited` are logged and admitted. The network supervisor still denies that +/// traffic, so the boundary holds without making the policy uneditable. +fn validate_uninspected_credentialed_endpoints_excluding( + policy: &ProtoSandboxPolicy, + inherited: &HashSet, +) -> Result<(), Status> { + let violations = collect_uninspected_credentialed_endpoints(policy); + let mut introduced: Option<&UninspectedCredentialedEndpoint> = None; + for violation in &violations { + if inherited.contains(&violation.key()) { + warn!( + rule_name = %violation.rule_name, + host = %violation.host, + port = violation.port, + mode = violation.mode, + "policy edit inherits an uninspected credentialed endpoint; the sandbox proxy denies this traffic unless allow_uninspected_credentials is set" + ); + continue; + } + // Rules iterate in map order, so pick the reported endpoint by name to + // keep the message stable when an edit introduces more than one. + let sort_key = |candidate: &UninspectedCredentialedEndpoint| { + ( + candidate.rule_name.clone(), + candidate.host.clone(), + candidate.port, + ) + }; + if introduced.is_none_or(|current| sort_key(violation) < sort_key(current)) { + introduced = Some(violation); + } + } + + let Some(violation) = introduced else { return Ok(()); }; @@ -3660,11 +3782,12 @@ async fn handle_update_config_inner( // on disk. Rejecting it here would crash-loop the sandbox instead of // surfacing an operator decision, so only operator-authored updates gate. if !sandbox_caller { - validate_candidate_sandbox_credential_policy( + validate_updated_sandbox_credential_policy( state, &workspace, + &sandbox, &spec.providers, - Some(&new_policy), + &new_policy, ) .await?; } @@ -6310,17 +6433,15 @@ struct PolicyMergeValidationContext<'a> { credential_binding: Option<&'a PolicyCredentialBindingValidationContext<'a>>, } -fn validate_operator_merged_credential_policy( +/// Recompute the credentialed markers on an effective policy from the provider +/// scopes plus any policy-declared bindings. The stamp is always a full +/// recomputation, never a delta: a partial derivation would let a series of +/// individually valid edits reach a state no single edit would have admitted. +fn stamp_operator_merged_credential_policy( effective_policy: &mut ProtoSandboxPolicy, bindings: &HashMap>, context: &PolicyCredentialBindingValidationContext<'_>, -) -> Result<(), Status> { - validate_policy_credential_binding_context( - context.catalog, - context.records, - effective_policy, - bindings, - )?; +) { let mut credentialed_scopes = context.credentialed_scopes.to_vec(); extend_credentialed_scopes_from_policy_bindings( &mut credentialed_scopes, @@ -6329,7 +6450,44 @@ fn validate_operator_merged_credential_policy( ); clear_provider_credentialed_markers(effective_policy); stamp_provider_credentialed_endpoints(effective_policy, &credentialed_scopes); - validate_uninspected_credentialed_endpoints(effective_policy) +} + +fn validate_operator_merged_credential_policy( + effective_policy: &mut ProtoSandboxPolicy, + bindings: &HashMap>, + context: &PolicyCredentialBindingValidationContext<'_>, + inherited: &HashSet, +) -> Result<(), Status> { + validate_policy_credential_binding_context( + context.catalog, + context.records, + effective_policy, + bindings, + )?; + stamp_operator_merged_credential_policy(effective_policy, bindings, context); + validate_uninspected_credentialed_endpoints_excluding(effective_policy, inherited) +} + +/// The uninspected credentialed endpoints the sandbox's current policy already +/// carries, composed and stamped exactly the way a candidate is. A baseline that +/// cannot be evaluated yields an empty set, which keeps the gate strict. +fn inherited_uninspected_credentialed_endpoints( + current_policy: &ProtoSandboxPolicy, + validation_context: PolicyMergeValidationContext<'_>, +) -> HashSet { + let Some(context) = validation_context.credential_binding else { + return HashSet::new(); + }; + let mut effective = if validation_context.provider_layers.is_empty() { + current_policy.clone() + } else { + compose_effective_policy(current_policy, validation_context.provider_layers) + }; + let Ok(bindings) = policy_static_credential_endpoint_bindings(Some(&effective)) else { + return HashSet::new(); + }; + stamp_operator_merged_credential_policy(&mut effective, &bindings, context); + uninspected_credentialed_endpoint_keys(&effective) } fn stage_validated_merge_operation( @@ -6350,7 +6508,9 @@ fn stage_validated_merge_operation( }; let bindings = policy_static_credential_endpoint_bindings(Some(&effective))?; if let Some(context) = validation_context.credential_binding { - validate_operator_merged_credential_policy(&mut effective, &bindings, context)?; + let inherited = + inherited_uninspected_credentialed_endpoints(current_policy, validation_context); + validate_operator_merged_credential_policy(&mut effective, &bindings, context, &inherited)?; } Ok(candidate) } @@ -6380,6 +6540,9 @@ async fn apply_merge_operations_with_retry( baseline_policy.cloned().unwrap_or_default() }; + let inherited = + inherited_uninspected_credentialed_endpoints(¤t_policy, validation_context); + if let Some(expected_hash) = expected_current_effective_hash { let mut current_effective = if provider_layers.is_empty() { current_policy.clone() @@ -6392,6 +6555,7 @@ async fn apply_merge_operations_with_retry( &mut current_effective, &bindings, context, + &inherited, )?; } if deterministic_policy_hash(¤t_effective) != expected_hash { @@ -6417,7 +6581,12 @@ async fn apply_merge_operations_with_retry( }; let bindings = policy_static_credential_endpoint_bindings(Some(&effective_policy))?; if let Some(context) = validation_context.credential_binding { - validate_operator_merged_credential_policy(&mut effective_policy, &bindings, context)?; + validate_operator_merged_credential_policy( + &mut effective_policy, + &bindings, + context, + &inherited, + )?; } if let Some(ref current) = latest @@ -7047,6 +7216,68 @@ mod tests { ); } + #[test] + fn inherited_uninspected_endpoints_are_admitted_but_new_ones_are_not() { + let endpoint = |host: &str, protocol: &str, tls: &str| NetworkEndpoint { + host: host.to_string(), + port: 443, + protocol: protocol.to_string(), + tls: tls.to_string(), + provider_credentialed: true, + ..Default::default() + }; + let policy = |rules: Vec<(&str, NetworkEndpoint)>| ProtoSandboxPolicy { + network_policies: rules + .into_iter() + .map(|(name, endpoint)| { + ( + name.to_string(), + NetworkPolicyRule { + endpoints: vec![endpoint], + ..Default::default() + }, + ) + }) + .collect(), + ..Default::default() + }; + + let baseline = policy(vec![("pypi", endpoint("github.com", "", ""))]); + let inherited = uninspected_credentialed_endpoint_keys(&baseline); + assert_eq!(inherited.len(), 1); + + // The same finding, carried forward next to an unrelated addition. + let unchanged = policy(vec![ + ("pypi", endpoint("github.com", "", "")), + ("npm", endpoint("registry.npmjs.org", "rest", "")), + ]); + assert!( + validate_uninspected_credentialed_endpoints_excluding(&unchanged, &inherited).is_ok() + ); + + // A second rule reaching the same credentialed host is newly authored. + let widened = policy(vec![ + ("pypi", endpoint("github.com", "", "")), + ("mirror", endpoint("github.com", "", "")), + ]); + let error = validate_uninspected_credentialed_endpoints_excluding(&widened, &inherited) + .expect_err("a violation in a new rule must be rejected"); + assert!(error.message().contains("mirror"), "{}", error.message()); + + // Switching the inherited endpoint to `tls: skip` is a fresh authoring + // act, not the same finding: `mode` is part of the identity. + let retuned = policy(vec![("pypi", endpoint("github.com", "rest", "skip"))]); + let error = validate_uninspected_credentialed_endpoints_excluding(&retuned, &inherited) + .expect_err("changing an inherited endpoint's uninspected mode must be rejected"); + assert!(error.message().contains("tls: skip"), "{}", error.message()); + + // With no baseline the gate stays strict. + assert!( + validate_uninspected_credentialed_endpoints_excluding(&baseline, &HashSet::new()) + .is_err() + ); + } + #[test] fn credentialed_l4_and_tls_skip_require_explicit_opt_in() { let endpoint = |protocol: &str, tls: &str, allow: bool| NetworkEndpoint { @@ -8981,6 +9212,226 @@ mod tests { ); } + /// The sandbox image's baked policy reaches the store through the ungated + /// sandbox-sync path. Once a credentialed provider stamps one of its + /// L4-only endpoints, every later operator edit used to be rejected for a + /// rule the operator never authored (NVIDIA/OpenShell#2998). + async fn inherited_uninspected_gating_state() + -> (Arc, openshell_core::proto::SandboxPolicy) { + use openshell_core::proto::{ + ProviderProfile, ProviderProfileCategory, ProviderProfileCredential, + StoredProviderProfile, + }; + + let state = test_server_state().await; + state + .store + .put_message(&StoredProviderProfile { + metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { + id: "profile-inherited-gating".to_string(), + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + ..Default::default() + }), + profile: Some(ProviderProfile { + id: "inherited-gating".to_string(), + display_name: "Inherited Gating".to_string(), + category: ProviderProfileCategory::SourceControl as i32, + credentials: vec![ProviderProfileCredential { + name: "api_token".to_string(), + env_vars: vec!["GITHUB_TOKEN".to_string()], + required: true, + auth_style: "bearer".to_string(), + header_name: "authorization".to_string(), + ..Default::default() + }], + endpoints: vec![ + NetworkEndpoint { + host: "github.com".to_string(), + port: 443, + protocol: "rest".to_string(), + access: "read-only".to_string(), + enforcement: "enforce".to_string(), + ..Default::default() + }, + NetworkEndpoint { + host: "api.github.com".to_string(), + port: 443, + protocol: "rest".to_string(), + access: "read-only".to_string(), + enforcement: "enforce".to_string(), + ..Default::default() + }, + ], + ..Default::default() + }), + }) + .await + .unwrap(); + state + .store + .put_message(&test_provider("my-github", "inherited-gating")) + .await + .unwrap(); + + // The policy the sandbox image ships: an L4-only `github.com:443` in a + // rule the operator did not write. + let mut image_policy = test_policy_with_rule("pypi", "github.com"); + image_policy + .network_policies + .extend(test_policy_with_rule("docs", "docs.example.com").network_policies); + + let mut sandbox = test_sandbox( + "sb-inherited-gating", + "inherited-gating", + ProtoSandboxPolicy::default(), + vec!["my-github".to_string()], + ); + sandbox.spec.as_mut().unwrap().policy = None; + state.store.put_message(&sandbox).await.unwrap(); + + // Sandbox-authored sync: ungated by design, so this is how the + // offending endpoint legitimately reaches the store. + handle_update_config( + &state, + with_sandbox( + Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(image_policy.clone()), + ..Default::default() + }), + "sb-inherited-gating", + ), + ) + .await + .expect("sandbox-authored policy sync must not be gated"); + + (state, image_policy) + } + + #[tokio::test] + async fn update_config_admits_edits_that_inherit_uninspected_credentialed_endpoints() { + let (state, image_policy) = inherited_uninspected_gating_state().await; + + // An unrelated merge: the exact shape that failed in the field. + let mut added = image_policy.clone(); + added + .network_policies + .extend(test_policy_with_rule("npm", "registry.npmjs.org").network_policies); + let npm_rule = added.network_policies["npm"].clone(); + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + merge_operations: vec![PolicyMergeOperation { + operation: Some(policy_merge_operation::Operation::AddRule( + openshell_core::proto::AddNetworkRule { + rule_name: "npm".to_string(), + rule: Some(npm_rule), + }, + )), + }], + ..Default::default() + })), + ) + .await + .expect("an unrelated merge must not be blocked by an inherited endpoint"); + + let stored = ProtoSandboxPolicy::decode( + state + .store + .get_latest_policy("sb-inherited-gating") + .await + .unwrap() + .expect("merge must persist a revision") + .policy_payload + .as_slice(), + ) + .unwrap(); + assert!(stored.network_policies.contains_key("npm")); + + // A full replacement that replays the same base policy: the + // `policy get --base` -> `policy set` round trip from the issue. + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(stored.clone()), + ..Default::default() + })), + ) + .await + .expect("replaying the current base policy must round-trip"); + } + + #[tokio::test] + async fn update_config_still_rejects_newly_authored_uninspected_credentialed_endpoints() { + let (state, _image_policy) = inherited_uninspected_gating_state().await; + + // A *new* rule reaching a credentialed host over L4. It has to be a host + // no existing rule covers: `merge_policy` folds a rule into whichever + // rule already owns that host and port. + let fresh_rule = + test_policy_with_rule("ghapi", "api.github.com").network_policies["ghapi"].clone(); + let error = handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + merge_operations: vec![PolicyMergeOperation { + operation: Some(policy_merge_operation::Operation::AddRule( + openshell_core::proto::AddNetworkRule { + rule_name: "ghapi".to_string(), + rule: Some(fresh_rule), + }, + )), + }], + ..Default::default() + })), + ) + .await + .expect_err("a newly authored L4-only credentialed endpoint must still be rejected"); + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("L4-only")); + assert!(error.message().contains("ghapi")); + assert!(error.message().contains("api.github.com")); + } + + #[tokio::test] + async fn update_config_lets_operators_remove_an_inherited_uninspected_endpoint() { + let (state, image_policy) = inherited_uninspected_gating_state().await; + + let mut fixed = image_policy; + fixed.network_policies.remove("pypi"); + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(fixed), + ..Default::default() + })), + ) + .await + .expect("removing the offending rule must be possible"); + + let stored = ProtoSandboxPolicy::decode( + state + .store + .get_latest_policy("sb-inherited-gating") + .await + .unwrap() + .expect("replacement must persist a revision") + .policy_payload + .as_slice(), + ) + .unwrap(); + assert!(!stored.network_policies.contains_key("pypi")); + } + #[tokio::test] async fn update_config_rejects_sigv4_without_credential_source_before_persisting_revision() { let state = test_server_state().await; diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index c5a34db14f..7114757f3d 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -645,7 +645,7 @@ Allow `pip install` and `uv pip install` to reach PyPI: - { path: /usr/local/bin/uv } ``` -Endpoints without `protocol` use explicit-proxy TCP passthrough, where OpenShell allows the stream without inspecting payloads. Use `protocol: tcp` when the application needs ordinary DNS resolution and native TCP connections through transparent capture. Provider-credentialed endpoints cannot use either L4 shape unless `allow_uninspected_credentials: true` records the exception. If an explicit-proxy stream is HTTP and TLS is auto-terminated, the proxy can still rewrite configured credential placeholders and closes keep-alive passthrough tunnels on policy reload before forwarding another request. WebSocket text-frame policy requires an explicit `protocol: websocket` endpoint. WebSocket payload credential rewrite can also be enabled on a `protocol: rest` compatibility endpoint with `websocket_credential_rewrite: true`. REST request body credential rewrite requires an inspected `protocol: rest` endpoint with `request_body_credential_rewrite: true`. +Endpoints without `protocol` use explicit-proxy TCP passthrough, where OpenShell allows the stream without inspecting payloads. Use `protocol: tcp` when the application needs ordinary DNS resolution and native TCP connections through transparent capture. Provider-credentialed endpoints cannot use either L4 shape unless `allow_uninspected_credentials: true` records the exception. When a sandbox image ships a policy that already reaches a credentialed host over L4, the gateway keeps that endpoint editable: it rejects only the uninspected credentialed endpoints an edit introduces, and the proxy denies the inherited one's traffic until you give it an inspected protocol or the explicit exception. If an explicit-proxy stream is HTTP and TLS is auto-terminated, the proxy can still rewrite configured credential placeholders and closes keep-alive passthrough tunnels on policy reload before forwarding another request. WebSocket text-frame policy requires an explicit `protocol: websocket` endpoint. WebSocket payload credential rewrite can also be enabled on a `protocol: rest` compatibility endpoint with `websocket_credential_rewrite: true`. REST request body credential rewrite requires an inspected `protocol: rest` endpoint with `request_body_credential_rewrite: true`. From 198906ab9c0cdd76ca3b4b2e97f2fac952c56066 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 2 Sep 2026 21:06:32 +0200 Subject: [PATCH 2/3] fix(server): key inherited credential findings per port and revalidate on commit Review of the differential credential gate found two reachable gaps. An uninspected credentialed endpoint's identity collapsed a multi-port endpoint onto its first port. Keeping an inherited port and adding a second one to the same endpoint therefore matched the baseline and was admitted, even though the added port had never been classified. Identity is now one key per effective port, so widening `[443]` to `[443, 8443]` introduces `8443`, while reordering or narrowing the port set carries the endpoint forward. Full policy replacement classified the edit before its persistence loop. Between that check and the write, another writer could remove the violation the edit was inheriting; the replacement then restored it without ever being classified. The gate now runs inside each persistence attempt, against the revision that attempt commits against. The write claims `latest.version + 1`, so any revision landing after that read loses the unique-version race and the retry reclassifies against the newer policy. Refs: #2998 Signed-off-by: Philippe Martin --- architecture/security-policy.md | 9 +- crates/openshell-server/src/grpc/policy.rs | 267 ++++++++++++++++++--- 2 files changed, 245 insertions(+), 31 deletions(-) diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 500af8293e..40fe62086e 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -142,8 +142,13 @@ the uninspected credentialed endpoints it introduces, while endpoints the sandbox's current policy already carries are logged and admitted. An endpoint's identity for this comparison is its rule name, host, port, and uninspected mode, so moving an inherited endpoint from L4-only to `tls: skip` is a fresh authoring -act rather than the same finding carried forward. Stamping is unaffected and -stays a full recomputation. +act rather than the same finding carried forward. A multi-port endpoint has one +identity per port, so keeping an inherited port and adding another one is +likewise newly authored. The baseline is the policy revision the edit is about +to be committed against, evaluated inside the persistence attempt: a baseline +read earlier could name a violation a concurrent writer has since removed, and +the edit would restore it without ever being classified. Stamping is unaffected +and stays a full recomputation. The gate has to work this way because a supervisor syncing the policy baked into a sandbox image reaches the store without passing it — rejecting that delivery diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 8dc732cdd6..da7716d9de 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -2143,10 +2143,15 @@ pub(super) async fn validate_candidate_sandbox_credential_policy( /// so an operator can round-trip `policy get --base` through `policy set` — and /// can edit the offending endpoint — instead of being locked out by a rule the /// sandbox image supplied. +/// +/// `current_base` must be the policy revision the replacement is about to be +/// committed against, so callers evaluate it inside their persistence attempt: +/// a baseline read before the attempt could name a violation another writer has +/// since removed, and the replacement would restore it unclassified. pub(super) async fn validate_updated_sandbox_credential_policy( state: &ServerState, workspace: &str, - sandbox: &Sandbox, + current_base: &ProtoSandboxPolicy, provider_names: &[String], policy: &ProtoSandboxPolicy, ) -> Result<(), Status> { @@ -2154,10 +2159,14 @@ pub(super) async fn validate_updated_sandbox_credential_policy( .provider_profile_sources .snapshot_catalog(state.store.as_ref(), workspace) .await?; - let current_base = current_base_policy_for_sandbox(state.store.as_ref(), sandbox).await?; - let current_effective = - effective_policy_for_source(state, &catalog, workspace, provider_names, current_base) - .await?; + let current_effective = effective_policy_for_source( + state, + &catalog, + workspace, + provider_names, + current_base.clone(), + ) + .await?; let inherited = uninspected_credentialed_endpoint_keys(¤t_effective); let effective = @@ -2967,7 +2976,9 @@ impl UninspectedCredentialedEndpoint { /// Identity of an uninspected credentialed endpoint. `mode` participates: moving /// an inherited L4-only endpoint to `tls: skip` is a fresh authoring act, not the -/// same finding carried forward. +/// same finding carried forward. A multi-port endpoint contributes one key per +/// port, so widening `[443]` to `[443, 8443]` introduces `8443` while reordering +/// the same ports carries the whole endpoint forward. #[derive(Clone, PartialEq, Eq, Hash)] struct UninspectedCredentialedEndpointKey { rule_name: String, @@ -3009,15 +3020,25 @@ fn collect_uninspected_credentialed_endpoints( continue; } - violations.push(UninspectedCredentialedEndpoint { - rule_name: rule_name.clone(), - host: endpoint.host.clone(), - port: endpoint_ports(endpoint) - .first() - .copied() - .unwrap_or(endpoint.port), - mode, - }); + // One violation per effective port. Collapsing a multi-port + // endpoint onto its first port would let an edit keep an inherited + // port and add another one under the same identity. + let mut ports = endpoint_ports(endpoint); + if ports.is_empty() { + // Unconstrained endpoint: it stands for every port, and `0` + // is the identity the message reports. + ports.push(endpoint.port); + } + ports.sort_unstable(); + ports.dedup(); + for port in ports { + violations.push(UninspectedCredentialedEndpoint { + rule_name: rule_name.clone(), + host: endpoint.host.clone(), + port, + mode, + }); + } } } violations @@ -3778,20 +3799,6 @@ async fn handle_update_config_inner( &effective_policy, ) .await?; - // Sandbox-authored syncs replay a policy the supervisor already discovered - // on disk. Rejecting it here would crash-loop the sandbox instead of - // surfacing an operator decision, so only operator-authored updates gate. - if !sandbox_caller { - validate_updated_sandbox_credential_policy( - state, - &workspace, - &sandbox, - &spec.providers, - &new_policy, - ) - .await?; - } - let _sandbox_sync_guard = if backfill_policy.is_some() { Some(state.compute.sandbox_sync_guard().await) } else { @@ -3809,6 +3816,31 @@ async fn handle_update_config_inner( .await .map_err(|e| Status::internal(format!("fetch latest policy failed: {e}")))?; + // Sandbox-authored syncs replay a policy the supervisor already + // discovered on disk. Rejecting it here would crash-loop the sandbox + // instead of surfacing an operator decision, so only + // operator-authored updates gate. The baseline is this attempt's + // revision: the write below claims `latest.version + 1` and loses + // the unique-version race to any revision that landed after this + // read, so a retry reclassifies against the newer policy. + if !sandbox_caller { + let current_base = match latest { + Some(ref record) => ProtoSandboxPolicy::decode( + record.policy_payload.as_slice(), + ) + .map_err(|e| Status::internal(format!("decode current policy failed: {e}")))?, + None => spec.policy.clone().unwrap_or_default(), + }; + validate_updated_sandbox_credential_policy( + state, + &workspace, + ¤t_base, + &spec.providers, + &new_policy, + ) + .await?; + } + if let Some(ref current) = latest && current.policy_hash == hash && current.provenance == req.annotations @@ -7278,6 +7310,62 @@ mod tests { ); } + #[test] + fn a_port_added_to_an_inherited_endpoint_is_newly_authored() { + let endpoint = |ports: Vec| NetworkEndpoint { + host: "github.com".to_string(), + ports, + provider_credentialed: true, + ..Default::default() + }; + let policy = |endpoint: NetworkEndpoint| ProtoSandboxPolicy { + network_policies: std::iter::once(( + "pypi".to_string(), + NetworkPolicyRule { + endpoints: vec![endpoint], + ..Default::default() + }, + )) + .collect(), + ..Default::default() + }; + + // Keeping the inherited port and adding another one exposes the added + // port for the first time, so the endpoint cannot be carried forward + // wholesale on the strength of its first port. + let single_port = uninspected_credentialed_endpoint_keys(&policy(endpoint(vec![443]))); + let error = validate_uninspected_credentialed_endpoints_excluding( + &policy(endpoint(vec![443, 8443])), + &single_port, + ) + .expect_err("a port added to an inherited endpoint must be rejected"); + assert!( + error.message().contains("github.com:8443"), + "{}", + error.message() + ); + + let both_ports = uninspected_credentialed_endpoint_keys(&policy(endpoint(vec![443, 8443]))); + assert_eq!(both_ports.len(), 2); + // Reordering exposes nothing new. + assert!( + validate_uninspected_credentialed_endpoints_excluding( + &policy(endpoint(vec![8443, 443])), + &both_ports + ) + .is_ok() + ); + // Neither does dropping a port: narrowing an inherited endpoint is the + // repair the operator has to be able to make. + assert!( + validate_uninspected_credentialed_endpoints_excluding( + &policy(endpoint(vec![443])), + &both_ports + ) + .is_ok() + ); + } + #[test] fn credentialed_l4_and_tls_skip_require_explicit_opt_in() { let endpoint = |protocol: &str, tls: &str, allow: bool| NetworkEndpoint { @@ -9432,6 +9520,127 @@ mod tests { assert!(!stored.network_policies.contains_key("pypi")); } + #[tokio::test] + async fn update_config_rejects_a_port_added_to_an_inherited_uninspected_endpoint() { + let (state, image_policy) = inherited_uninspected_gating_state().await; + + // The same endpoint, restated through `ports`: nothing new is exposed. + let mut restated = image_policy.clone(); + restated.network_policies.get_mut("pypi").unwrap().endpoints[0].ports = vec![443]; + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(restated), + ..Default::default() + })), + ) + .await + .expect("restating the inherited port must be admitted"); + + // Keeping the inherited port and adding a second one to the same + // endpoint authors a new uninspected credentialed port. + let mut widened = image_policy; + widened.network_policies.get_mut("pypi").unwrap().endpoints[0].ports = vec![443, 8443]; + let error = handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(widened), + ..Default::default() + })), + ) + .await + .expect_err("a port added to an inherited endpoint must be rejected"); + assert_eq!(error.code(), Code::FailedPrecondition); + assert!( + error.message().contains("github.com:8443"), + "{}", + error.message() + ); + } + + #[tokio::test] + async fn update_config_rejects_a_replacement_that_restores_a_removed_uninspected_endpoint() { + let (state, image_policy) = inherited_uninspected_gating_state().await; + + let mut fixed = image_policy.clone(); + fixed.network_policies.remove("pypi"); + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(fixed), + ..Default::default() + })), + ) + .await + .expect("removing the offending rule must be possible"); + + // A replacement composed against the older revision. The endpoint is no + // longer inherited from the policy this write commits against, so it is + // newly authored and must not slip back in unclassified. + let error = handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(image_policy), + ..Default::default() + })), + ) + .await + .expect_err("restoring a removed uninspected endpoint must be rejected"); + assert_eq!(error.code(), Code::FailedPrecondition); + assert!(error.message().contains("pypi"), "{}", error.message()); + } + + #[tokio::test] + async fn concurrent_full_replacements_do_not_restore_a_removed_uninspected_endpoint() { + let (state, image_policy) = inherited_uninspected_gating_state().await; + + let mut fixed = image_policy.clone(); + fixed.network_policies.remove("pypi"); + + // Whichever order these interleave in, the inherited endpoint must not + // survive a replacement classified against a revision that no longer + // carries it. + let (restore, remove) = tokio::join!( + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(image_policy), + ..Default::default() + })), + ), + handle_update_config( + &state, + with_user(Request::new(UpdateConfigRequest { + name: "inherited-gating".to_string(), + workspace: "default".to_string(), + policy: Some(fixed), + ..Default::default() + })), + ) + ); + remove.expect("removing the offending rule must be possible"); + + if restore.is_ok() { + let stored = get_sandbox_policy(&state, "sb-inherited-gating").await; + assert!( + !stored.network_policies.contains_key("pypi"), + "a replacement validated against a stale revision restored the endpoint" + ); + } else { + assert_eq!(restore.unwrap_err().code(), Code::FailedPrecondition); + } + } + #[tokio::test] async fn update_config_rejects_sigv4_without_credential_source_before_persisting_revision() { let state = test_server_state().await; From 4d3c37c476d3223af65864b3e9584387f1558b21 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 2 Sep 2026 21:43:52 +0200 Subject: [PATCH 3/3] fix(server): satisfy clippy in inherited-gating restore assertion Clippy's unnecessary_unwrap fires on unwrap_err() after an is_ok() check, failing the workspace lint under -D warnings. Bind the status directly with if let Err instead. Signed-off-by: Philippe Martin --- crates/openshell-server/src/grpc/policy.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index da7716d9de..f7cc6614c7 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -9630,14 +9630,14 @@ mod tests { ); remove.expect("removing the offending rule must be possible"); - if restore.is_ok() { + if let Err(status) = restore { + assert_eq!(status.code(), Code::FailedPrecondition); + } else { let stored = get_sandbox_policy(&state, "sb-inherited-gating").await; assert!( !stored.network_policies.contains_key("pypi"), "a replacement validated against a stale revision restored the endpoint" ); - } else { - assert_eq!(restore.unwrap_err().code(), Code::FailedPrecondition); } }