Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions architecture/security-policy.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down
15 changes: 9 additions & 6 deletions crates/openshell-policy/src/ambiguity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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);
Expand Down
44 changes: 39 additions & 5 deletions crates/openshell-policy/src/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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(())
}
Expand Down Expand Up @@ -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;
Expand All @@ -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]
Expand Down
72 changes: 64 additions & 8 deletions crates/openshell-server/src/grpc/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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()
}],
Expand Down Expand Up @@ -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(),
Expand All @@ -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 {
Expand All @@ -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(),
Expand All @@ -14433,20 +14437,30 @@ 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(),
})),
)
.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: "),
Expand All @@ -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")
);
}

Expand Down
1 change: 1 addition & 0 deletions crates/openshell-supervisor-network/src/policy_local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions docs/sandboxes/policies.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 20 additions & 1 deletion docs/sandboxes/policy-advisor.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.

Expand Down
Loading