Skip to content

Commit d50f18a

Browse files
fix(policy): preserve advisor provenance during merge
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
1 parent 758f643 commit d50f18a

2 files changed

Lines changed: 91 additions & 6 deletions

File tree

‎crates/openshell-policy/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4884,7 +4884,7 @@ network_policies:
48844884
let error = parse_sandbox_policy(yaml).expect_err("removed harness field must be rejected");
48854885
let error_debug = format!("{error:?}");
48864886
assert!(
4887-
error_debug.contains("unknown field `harness`"),
4887+
error_debug.contains("unknown field") && error_debug.contains("harness"),
48884888
"unexpected error: {error_debug}"
48894889
);
48904890
}

‎crates/openshell-policy/src/merge.rs‎

Lines changed: 90 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1531,10 +1531,11 @@ fn merge_endpoint(
15311531
existing.websocket_credential_rewrite |= incoming.websocket_credential_rewrite;
15321532
existing.request_body_credential_rewrite |= incoming.request_body_credential_rewrite;
15331533
existing.allow_uninspected_credentials |= incoming.allow_uninspected_credentials;
1534-
// Provenance is not an authorization bit. If either declaration came
1535-
// directly from a user or provider, keep the endpoint explicit. This
1536-
// mirrors binary provenance and prevents an advisor overlay from tainting
1537-
// an already explicit endpoint for exact-host SSRF evaluation.
1534+
// If either declaration came directly from a user or provider, keep the
1535+
// endpoint explicit. Clearing advisor provenance changes exact-host SSRF
1536+
// trust, so `ensure_authorization_inheritance_is_declared` only permits
1537+
// this transition when the incoming rule declares the existing binaries
1538+
// that will receive that trust.
15381539
existing.advisor_proposed &= incoming.advisor_proposed;
15391540
normalize_endpoint(existing);
15401541
Ok(())
@@ -1810,7 +1811,13 @@ fn authorization_unit_unchanged(
18101811
merged: &NetworkEndpoint,
18111812
port: Option<u32>,
18121813
) -> bool {
1813-
endpoint_authorization_covers_port(existing, merged, port)
1814+
// Advisor provenance affects whether an exact hostname may resolve to a
1815+
// private address. Treat clearing it as an authorization change even
1816+
// though proposal coverage deliberately ignores provenance. Otherwise an
1817+
// explicit rule for one binary could make advisor-observed binaries on the
1818+
// same rule eligible for private-address access.
1819+
existing.advisor_proposed == merged.advisor_proposed
1820+
&& endpoint_authorization_covers_port(existing, merged, port)
18141821
&& endpoint_authorization_covers_port(merged, existing, port)
18151822
}
18161823

@@ -4766,6 +4773,84 @@ mod tests {
47664773
assert_eq!(advisor.binaries, vec![binary("/usr/bin/curl")]);
47674774
}
47684775

4776+
#[test]
4777+
fn add_rule_keeps_explicit_binary_separate_from_advisor_endpoint() {
4778+
let mut advisor_endpoint = endpoint("index.crates.io", 443);
4779+
advisor_endpoint.advisor_proposed = true;
4780+
let mut policy = restrictive_default_policy();
4781+
policy.network_policies.insert(
4782+
"advisor_index".to_string(),
4783+
NetworkPolicyRule {
4784+
name: "advisor-index".to_string(),
4785+
endpoints: vec![advisor_endpoint],
4786+
binaries: vec![binary("/usr/bin/curl")],
4787+
},
4788+
);
4789+
4790+
let result = merge_policy(
4791+
policy,
4792+
&[PolicyMergeOp::AddRule {
4793+
rule_name: "explicit_index".to_string(),
4794+
rule: NetworkPolicyRule {
4795+
name: "explicit-index".to_string(),
4796+
endpoints: vec![endpoint("index.crates.io", 443)],
4797+
binaries: vec![binary("/usr/bin/wget")],
4798+
},
4799+
}],
4800+
)
4801+
.expect("explicit authorization should remain separate");
4802+
4803+
let advisor = &result.policy.network_policies["advisor_index"];
4804+
assert!(advisor.endpoints[0].advisor_proposed);
4805+
assert_eq!(advisor.binaries, vec![binary("/usr/bin/curl")]);
4806+
4807+
let explicit = &result.policy.network_policies["explicit_index"];
4808+
assert!(!explicit.endpoints[0].advisor_proposed);
4809+
assert_eq!(explicit.binaries, vec![binary("/usr/bin/wget")]);
4810+
assert!(result.warnings.iter().any(|warning| matches!(
4811+
warning,
4812+
PolicyMergeWarning::KeptRequestedRuleNameToAvoidWidening {
4813+
rule_name,
4814+
overlapping_rule_name,
4815+
..
4816+
} if rule_name == "explicit_index" && overlapping_rule_name == "advisor_index"
4817+
)));
4818+
}
4819+
4820+
#[test]
4821+
fn add_rule_rejects_clearing_advisor_provenance_for_undeclared_binary() {
4822+
let mut advisor_endpoint = endpoint("index.crates.io", 443);
4823+
advisor_endpoint.advisor_proposed = true;
4824+
let policy = policy_with_rule(
4825+
"advisor_index",
4826+
NetworkPolicyRule {
4827+
name: "advisor-index".to_string(),
4828+
endpoints: vec![advisor_endpoint],
4829+
binaries: vec![binary("/usr/bin/curl")],
4830+
},
4831+
);
4832+
4833+
let result = merge_policy(
4834+
policy,
4835+
&[PolicyMergeOp::AddRule {
4836+
rule_name: "advisor_index".to_string(),
4837+
rule: NetworkPolicyRule {
4838+
name: "advisor-index".to_string(),
4839+
endpoints: vec![endpoint("index.crates.io", 443)],
4840+
binaries: vec![binary("/usr/bin/wget")],
4841+
},
4842+
}],
4843+
);
4844+
4845+
assert!(matches!(
4846+
result,
4847+
Err(PolicyMergeError::ExistingBinariesWouldInheritAuthorization {
4848+
undeclared_binaries,
4849+
..
4850+
}) if undeclared_binaries == ["/usr/bin/curl"]
4851+
));
4852+
}
4853+
47694854
fn endpoint_with_ports(host: &str, ports: &[u32]) -> NetworkEndpoint {
47704855
NetworkEndpoint {
47714856
host: host.to_string(),

0 commit comments

Comments
 (0)