diff --git a/architecture/security-policy.md b/architecture/security-policy.md index 3f582eb25d..9203ba3178 100644 --- a/architecture/security-policy.md +++ b/architecture/security-policy.md @@ -268,6 +268,22 @@ After any successful policy write, pending chunks already covered by the new live effective policy are rejected as redundant. This keeps the review inbox aligned with what the sandbox currently enforces. +Endpoint and binary advisor markers are provenance, not authorization or +connection metadata. Provider- or user-authored declarations carry explicit +provenance; `policy.local` declarations carry advisor provenance. A difference +in endpoint provenance alone is compatible during effective-policy ambiguity +validation. When identical endpoint or binary identities merge, an explicit +declaration dominates an advisor declaration. Proposal coverage likewise +ignores provenance so an approved overlay converges when an existing explicit +declaration already supplies the same identity. + +This compatibility does not weaken SSRF classification. Exact-host trust +requires one matching rule to contain both an exact explicit endpoint and an +explicit binary identity. An advisor-only endpoint or binary cannot assemble +that trust from unrelated rules. A provider rule may independently establish +trust for its own explicit endpoint and binary pair, but an advisor overlay +does not broaden that pair to a different binary. + ### Security-notes gate Separately from the prover, each chunk carries advisory `security_notes`. diff --git a/crates/openshell-policy/src/ambiguity.rs b/crates/openshell-policy/src/ambiguity.rs index 5c2b9d9177..2c0b1b1944 100644 --- a/crates/openshell-policy/src/ambiguity.rs +++ b/crates/openshell-policy/src/ambiguity.rs @@ -181,12 +181,6 @@ fn connection_conflicts(left: &NetworkEndpoint, right: &NetworkEndpoint) -> Vec< &normalized_strings(&left.allowed_ips), &normalized_strings(&right.allowed_ips), ); - push_conflict( - &mut conflicts, - "advisor_proposed", - &left.advisor_proposed, - &right.advisor_proposed, - ); conflicts } @@ -787,6 +781,15 @@ mod tests { assert!(find_endpoint_ambiguities(&policy_with(left, right)).is_empty()); } + #[test] + fn advisor_provenance_does_not_make_endpoints_ambiguous() { + let explicit = endpoint("api.example.com", 443); + let mut proposed = explicit.clone(); + proposed.advisor_proposed = true; + + assert!(find_endpoint_ambiguities(&policy_with(explicit, proposed)).is_empty()); + } + #[test] fn plain_l4_endpoint_does_not_compete_with_l7_endpoint_metadata() { let left = endpoint("api.example.com", 443); diff --git a/crates/openshell-policy/src/merge.rs b/crates/openshell-policy/src/merge.rs index 12026bbea3..a87defa340 100644 --- a/crates/openshell-policy/src/merge.rs +++ b/crates/openshell-policy/src/merge.rs @@ -632,7 +632,8 @@ fn endpoint_attributes_cover(loaded: &NetworkEndpoint, proposed: &NetworkEndpoin return false; } - // Widened fields (list appends and `|=` flags) use containment: merging + // Widened fields (list appends and authorization flags) use containment: + // merging // into an endpoint that already carries them leaves the loaded copy a // superset of the proposal, so equality would report "not covered" for a // proposal that did land. @@ -648,7 +649,6 @@ fn endpoint_attributes_cover(loaded: &NetworkEndpoint, proposed: &NetworkEndpoin loaded.request_body_credential_rewrite, proposed.request_body_credential_rewrite, ) - && flag_covers(loaded.advisor_proposed, proposed.advisor_proposed) // Fields the merge neither widens nor retains: it drops them entirely. // An unset proposal value asks for nothing and is satisfied by whatever // is loaded; a set value that differs was dropped, so the proposal is @@ -1374,7 +1374,11 @@ fn merge_endpoint( existing.websocket_credential_rewrite |= incoming.websocket_credential_rewrite; existing.request_body_credential_rewrite |= incoming.request_body_credential_rewrite; existing.allow_uninspected_credentials |= incoming.allow_uninspected_credentials; - existing.advisor_proposed |= incoming.advisor_proposed; + // Provenance is not an authorization bit. If either declaration came + // directly from a user or provider, keep the endpoint explicit. This + // mirrors binary provenance and prevents an advisor overlay from tainting + // an already explicit endpoint for exact-host SSRF evaluation. + existing.advisor_proposed &= incoming.advisor_proposed; normalize_endpoint(existing); Ok(()) } @@ -3025,7 +3029,7 @@ mod tests { } #[test] - fn policy_coverage_requires_endpoint_advisor_provenance_to_be_loaded() { + fn policy_coverage_ignores_endpoint_advisor_provenance() { let loaded_endpoint = endpoint("api.example.com", 443); let mut proposed_endpoint = loaded_endpoint.clone(); proposed_endpoint.advisor_proposed = true; @@ -3036,7 +3040,37 @@ mod tests { let proposed = rule_with_authorizations("proposed", vec![proposed_endpoint], &["/usr/bin/client"]); - assert!(!policy_covers_rule(&loaded, &proposed)); + assert!(policy_covers_rule(&loaded, &proposed)); + } + + #[test] + fn explicit_endpoint_provenance_wins_in_either_merge_order() { + let explicit_endpoint = endpoint("api.example.com", 443); + let mut proposed_endpoint = explicit_endpoint.clone(); + proposed_endpoint.advisor_proposed = true; + + for (existing_endpoint, incoming_endpoint) in [ + (explicit_endpoint.clone(), proposed_endpoint.clone()), + (proposed_endpoint, explicit_endpoint), + ] { + let incoming = + rule_with_authorizations("api", vec![incoming_endpoint], &["/usr/bin/client"]); + let merged = merge_policy( + policy_with_rule( + "api", + rule_with_authorizations("api", vec![existing_endpoint], &["/usr/bin/client"]), + ), + &[PolicyMergeOp::AddRule { + rule_name: "api".to_string(), + rule: incoming.clone(), + }], + ) + .expect("compatible explicit and advisor rules should merge"); + + let endpoint = &merged.policy.network_policies["api"].endpoints[0]; + assert!(!endpoint.advisor_proposed); + assert!(policy_covers_rule(&merged.policy, &incoming)); + } } #[test] diff --git a/crates/openshell-server/src/grpc/policy.rs b/crates/openshell-server/src/grpc/policy.rs index 114b43aba0..177db5b307 100644 --- a/crates/openshell-server/src/grpc/policy.rs +++ b/crates/openshell-server/src/grpc/policy.rs @@ -11342,7 +11342,7 @@ mod tests { } #[tokio::test] - async fn approve_all_skips_later_batch_conflict_and_applies_compatible_prefix() { + async fn approve_all_skips_later_tls_conflict_and_applies_compatible_prefix() { let state = test_server_state().await; let sandbox_id = "sb-approve-all-conflict"; let sandbox_name = "approve-all-conflict"; @@ -11389,6 +11389,7 @@ mod tests { endpoints: vec![NetworkEndpoint { host: "shared.example.com".to_string(), port: 443, + tls: "skip".to_string(), advisor_proposed: true, ..Default::default() }], @@ -14364,10 +14365,11 @@ mod tests { .await .unwrap(); + let sandbox_id = "sb-agent-provider-effective-policy"; let sandbox_name = "agent-provider-effective-policy".to_string(); let mut sandbox = Sandbox { metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta { - id: "sb-agent-provider-effective-policy".to_string(), + id: sandbox_id.to_string(), name: sandbox_name.clone(), created_at_ms: 1_000_000, labels: HashMap::new(), @@ -14393,6 +14395,7 @@ mod tests { sandbox.set_phase(SandboxPhase::Ready as i32); state.store.put_message(&sandbox).await.unwrap(); + #[allow(deprecated)] let proposed_rule = NetworkPolicyRule { name: "github_contents_write".to_string(), endpoints: vec![NetworkEndpoint { @@ -14410,15 +14413,16 @@ mod tests { ..Default::default() }), }], + advisor_proposed: true, ..Default::default() }], binaries: vec![NetworkBinary { path: "/usr/bin/curl".to_string(), - ..Default::default() + harness: true, }], }; - handle_submit_policy_analysis( + let submit = handle_submit_policy_analysis( &state, with_user(Request::new(SubmitPolicyAnalysisRequest { name: sandbox_name.clone(), @@ -14433,12 +14437,16 @@ mod tests { })), ) .await - .unwrap(); + .unwrap() + .into_inner(); + assert_eq!(submit.accepted_chunks, 1); + assert_eq!(submit.rejected_chunks, 0); + let chunk_id = submit.accepted_chunk_ids[0].clone(); let draft = handle_get_draft_policy( &state, with_user(Request::new(GetDraftPolicyRequest { - name: sandbox_name, + name: sandbox_name.clone(), status_filter: String::new(), workspace: "default".to_string(), })), @@ -14446,7 +14454,13 @@ mod tests { .await .unwrap() .into_inner(); - let verdict = &draft.chunks[0].validation_result; + let chunk = draft + .chunks + .iter() + .find(|chunk| chunk.id == chunk_id) + .expect("provider-overlap proposal should reach the draft inbox"); + assert_eq!(chunk.status, "pending"); + let verdict = &chunk.validation_result; let first_line = verdict.lines().next().unwrap_or(""); assert!( first_line.starts_with("prover: "), @@ -14456,7 +14470,49 @@ mod tests { assert!( !verdict.contains("validation unavailable"), "providers-v2 composition must not break the prover pipeline; \ - got: {verdict}" + got: {verdict}" + ); + + handle_approve_draft_chunk( + &state, + authed_request(ApproveDraftChunkRequest { + name: sandbox_name, + chunk_id, + workspace: "default".to_string(), + review_token: chunk.review_token.clone(), + }), + ) + .await + .expect("provider-overlap proposal should approve"); + + let stored = state + .store + .get_latest_policy(sandbox_id) + .await + .unwrap() + .expect("approval should persist a base-policy revision"); + let base_policy = ProtoSandboxPolicy::decode(stored.policy_payload.as_slice()).unwrap(); + assert!( + base_policy + .network_policies + .contains_key("github_contents_write") + ); + assert!( + !base_policy + .network_policies + .contains_key("_provider_work_custom"), + "provider-composed rules must not be copied into the mutable base policy" + ); + + let effective_policy = get_sandbox_policy(&state, sandbox_id).await; + let provider_rule = &effective_policy.network_policies["_provider_work_custom"]; + assert_eq!(provider_rule.endpoints[0].access, "full"); + assert_eq!(provider_rule.endpoints[0].deny_rules.len(), 1); + assert!(!provider_rule.endpoints[0].advisor_proposed); + assert!( + effective_policy + .network_policies + .contains_key("github_contents_write") ); } diff --git a/crates/openshell-supervisor-network/src/policy_local.rs b/crates/openshell-supervisor-network/src/policy_local.rs index 908587ed31..ef899b666e 100644 --- a/crates/openshell-supervisor-network/src/policy_local.rs +++ b/crates/openshell-supervisor-network/src/policy_local.rs @@ -1440,6 +1440,7 @@ mod tests { assert_eq!(rule.endpoints[0].port, 443); assert_eq!(rule.endpoints[0].ports, vec![443]); assert_eq!(rule.endpoints[0].protocol, "rest"); + assert!(rule.endpoints[0].advisor_proposed); #[allow(deprecated)] { assert!(rule.binaries[0].harness); diff --git a/docs/sandboxes/policies.mdx b/docs/sandboxes/policies.mdx index a85bbd9b9d..16202751f8 100644 --- a/docs/sandboxes/policies.mdx +++ b/docs/sandboxes/policies.mdx @@ -236,6 +236,8 @@ The following steps outline the hot-reload policy update workflow. OpenShell validates a complete candidate policy before activating any part of it. Endpoints may overlap when their connection and request-processing metadata agree. For example, two `api.example.com:443` REST entries can contribute different allow and deny rules when they use the same TLS, destination, credential, parser, and enforcement settings. A plain L4 endpoint may overlap an L7 endpoint because it authorizes the destination without contributing request-processing metadata. A more-specific path endpoint may override request-processing metadata from a broader endpoint, such as a `/graphql` GraphQL endpoint alongside a general REST endpoint for the same host. OpenShell rejects the candidate when overlapping exact or wildcard host selectors can both contribute equally specific endpoint configuration and disagree on those fields. +Internal policy-advisor provenance does not make otherwise compatible endpoints ambiguous. This lets an advisor proposal extend a provider-covered host without modifying the provider rule. TLS, destination IP constraints, credential handling, protocol, parser, and equally specific enforcement settings must still agree. + When the gateway knows the affected sandbox scope, it validates the complete effective candidate before persistence. This covers direct policy replacement, incremental merges and proposal approvals, provider attachment, and diff --git a/docs/sandboxes/policy-advisor.mdx b/docs/sandboxes/policy-advisor.mdx index bc09468cdb..82e7ec12cd 100644 --- a/docs/sandboxes/policy-advisor.mdx +++ b/docs/sandboxes/policy-advisor.mdx @@ -131,6 +131,25 @@ OpenShell has two proposal paths: | Mechanistic mapper | Aggregated denial summaries from the sandbox. | Groups by host, port, and binary. If L7 request samples are available, it can draft REST method and path rules. Otherwise it drafts an L4 endpoint. | | Agent-authored proposal | The in-sandbox agent, using `policy.local`. | Usually a REST `addRule` with exact host, port, binary, method, and path from the structured denial. It can also propose L4 rules for opaque protocols. | +### How proposal provenance works + +OpenShell tracks whether an endpoint and binary came from policy advisor. This is internal provenance; it is not a policy YAML field that authors set. Think of each marker as answering “who introduced this identity?” rather than “what traffic does this allow?” + +| Marker | `false` | `true` | When both declarations meet | +|---|---|---|---| +| Endpoint provenance | A user or provider explicitly declared the endpoint. | Policy advisor proposed the endpoint. | The values do not conflict by themselves. The explicit declaration wins if the endpoints merge. | +| Binary provenance | A user or provider explicitly declared the binary path. | Policy advisor proposed the binary path. | The explicit declaration wins if the same path merges. | + +For example, a GitHub provider can explicitly declare `api.github.com:443` for read operations. An agent can then propose `PUT /repos/NVIDIA/OpenShell/contents/docs/**` for the same endpoint. The provider endpoint has provenance `false`; the proposal endpoint has provenance `true`. OpenShell allows that overlap, keeps the provider rule immutable, and stores an approved write rule in the sandbox policy layer. + +Provenance does not hide a real endpoint conflict. The same two declarations still fail validation if they disagree on connection or request-processing behavior that must have one value, such as TLS mode, `allowed_ips`, an equally specific L7 protocol or parser contract, credential binding, or enforcement mode. Authorization fields such as compatible allow and deny rules can combine. + +Exact-host SSRF trust requires an exact endpoint and the matching binary identity to be explicit in the same rule. An advisor-only endpoint or binary does not create that stronger trust. For example: + +- A provider rule that explicitly declares both `/usr/bin/gh` and `api.github.com:443` already establishes exact-host trust for that pair. A later advisor proposal does not create or broaden that trust. +- If the provider declares `api.github.com:443` for `/usr/bin/gh`, but the advisor proposes the endpoint for `/usr/bin/curl`, `curl` does not inherit the provider's binary identity. Its proposed rule remains subject to the normal SSRF checks. +- If an advisor proposes `internal-api.example:443` and it resolves to a private address, approval alone is not enough. A developer must explicitly authorize the intended address range with `allowed_ips`. + For REST APIs, prefer L7 rules over broad L4 access. A good proposal allows one method and the smallest safe path: ```json @@ -172,7 +191,7 @@ For REST APIs, prefer L7 rules over broad L4 access. A good proposal allows one The current `policy.local` JSON shape covers L4 endpoints and REST method or path rules. Use [Customize Sandbox Policies](/sandboxes/policies) or [Policy Schema Reference](/reference/policy-schema) for policy fields that are not part of the agent-authored proposal surface, such as WebSocket credential rewrite, GraphQL operation matching, endpoint path scoping, and provider-owned policy bundles. -Policy advisor proposals do not add `allowed_ips` automatically. If an advisor-proposed hostname resolves to an internal or private address, OpenShell's SSRF protections still block the connection until a developer explicitly adds the required `allowed_ips` entry. Exact hostname trust for user-declared policy endpoints does not apply to advisor-generated proposal binaries. +Policy advisor proposals do not add `allowed_ips` automatically. If an advisor-proposed hostname resolves to an internal or private address, OpenShell's SSRF protections still block the connection until a developer explicitly adds the required `allowed_ips` entry. Private RFC 1918, CGNAT, IPv6 ULA, and other special-use destinations classified as internal produce advisory security notes when they appear as literal endpoint IPs or in `allowed_ips`. CIDR intersections are included, and hostless `allowed_ips` rules receive an additional warning because they can match any hostname resolving into the configured range.