Skip to content

Commit abe84ef

Browse files
committed
fix(policy): address approval loop review feedback
Signed-off-by: Alexander Watson <zredlined@gmail.com>
1 parent d851129 commit abe84ef

18 files changed

Lines changed: 672 additions & 102 deletions

File tree

architecture/security-policy.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ because it changes the effective access model for every sandbox on the gateway.
9191
The policy advisor pipeline turns observed denials into draft policy
9292
recommendations. There are two proposers (sandbox-side mechanistic mapper,
9393
agent-authored via `policy.local`); the gateway is the single referee.
94+
When enabled, L7 `policy_denied` responses include both structured
95+
`next_steps` and a short `agent_guidance` string so generic agents can continue
96+
through the proposal loop instead of treating the denial as terminal.
9497

9598
1. **Submit.** Both proposers POST through the same `SubmitPolicyAnalysis`
9699
path. Each chunk is persisted with its `analysis_mode` for audit provenance.
@@ -130,15 +133,17 @@ than one reach + N method findings.
130133

131134
| Category | The prover detects… |
132135
|---|---|
133-
| `link_local_reach` | The proposal grants reach to a host in `169.254.0.0/16` or `fe80::/10`. Unconditional — cloud-metadata endpoints serve credentials regardless of sandbox state. |
136+
| `link_local_reach` | The proposal grants reach to a host in `169.254.0.0/16`, `fe80::/10`, or a known metadata hostname such as `metadata.google.internal`. Unconditional — cloud-metadata endpoints serve credentials regardless of sandbox state. |
134137
| `l7_bypass_credentialed` | The proposal lets a binary using a non-HTTP wire protocol (`git-remote-https`, `ssh`, `nc`) reach a host where a sandbox credential is in scope. The L7 proxy cannot inspect the wire protocol; the reviewer decides whether to trust the binary with the credential. |
135138
| `credential_reach_expansion` | A binary gained credentialed reach to a (host, port) it could not reach before. New authenticated reach is a stated intent change; the reviewer confirms the binary should authenticate to the host at all. |
136139
| `capability_expansion` | On a (binary, host, port) that already had credentialed reach, the policy adds a new HTTP method. The reviewer sees exactly which method was added (e.g., PUT) and decides if it's part of the agent's task. |
137140

138141
"Credential in scope" is sandbox-coarse, not binary-fine: a credential is
139142
considered in scope if the sandbox has a provider attached whose
140-
`target_hosts` include the proposed endpoint's host. v1 does not model
141-
credential scopes (read-only vs write); presence is enough.
143+
`target_hosts` include the proposed endpoint's host, including runtime-like
144+
first-label wildcard coverage such as `*.github.com` covering
145+
`api.github.com`. v1 does not model credential scopes (read-only vs write);
146+
presence is enough.
142147

143148
Proposals intentionally omit `allowed_ips`. If a proposed rule targets a host
144149
that resolves to a private IP, the proxy's runtime SSRF classification blocks

crates/openshell-cli/src/run.rs

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5557,14 +5557,16 @@ fn parse_cli_setting_value(key: &str, raw_value: &str) -> Result<SettingValue> {
55575557
// proposal_approval_mode autom` errors immediately instead of
55585558
// round-tripping through the server. The server enforces the
55595559
// same check independently for non-CLI callers.
5560-
setting.validate_string_value(raw_value).map_err(|allowed| {
5561-
miette::miette!(
5562-
"invalid value '{}' for key '{}'; expected one of: {}",
5563-
raw_value,
5564-
key,
5565-
allowed.join(", ")
5566-
)
5567-
})?;
5560+
setting
5561+
.validate_string_value(raw_value)
5562+
.map_err(|allowed| {
5563+
miette::miette!(
5564+
"invalid value '{}' for key '{}'; expected one of: {}",
5565+
raw_value,
5566+
key,
5567+
allowed.join(", ")
5568+
)
5569+
})?;
55685570
setting_value::Value::StringValue(raw_value.to_string())
55695571
}
55705572
SettingValueKind::Int => {

crates/openshell-core/src/net.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,16 @@
1212
1313
use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
1414

15+
/// Check if a hostname is a known cloud metadata hostname that resolves to an
16+
/// always-blocked metadata service.
17+
///
18+
/// This is intentionally a static name check. Do not perform DNS resolution in
19+
/// policy validation or proposal generation paths.
20+
pub fn is_known_metadata_hostname(host: &str) -> bool {
21+
let normalized = host.trim().trim_end_matches('.').to_ascii_lowercase();
22+
matches!(normalized.as_str(), "metadata.google.internal")
23+
}
24+
1525
/// Check if an IP address is link-local.
1626
///
1727
/// Covers IPv4 `169.254.0.0/16`, IPv6 `fe80::/10`, and IPv4-mapped IPv6
@@ -213,6 +223,21 @@ fn is_internal_v4(v4: Ipv4Addr) -> bool {
213223
mod tests {
214224
use super::*;
215225

226+
// -- is_known_metadata_hostname --
227+
228+
#[test]
229+
fn test_known_metadata_hostname_accepts_gcp_variants() {
230+
assert!(is_known_metadata_hostname("metadata.google.internal"));
231+
assert!(is_known_metadata_hostname("METADATA.GOOGLE.INTERNAL"));
232+
assert!(is_known_metadata_hostname("metadata.google.internal."));
233+
}
234+
235+
#[test]
236+
fn test_known_metadata_hostname_rejects_public_hosts() {
237+
assert!(!is_known_metadata_hostname("api.github.com"));
238+
assert!(!is_known_metadata_hostname(""));
239+
}
240+
216241
// -- is_link_local_ip --
217242

218243
#[test]

crates/openshell-core/src/settings.rs

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -100,10 +100,11 @@ pub const AGENT_POLICY_PROPOSALS_ENABLED_KEY: &str = "agent_policy_proposals_ena
100100
/// global is set.
101101
pub const PROPOSAL_APPROVAL_MODE_KEY: &str = "proposal_approval_mode";
102102

103-
/// Allowed values for [`PROPOSAL_APPROVAL_MODE_KEY`]. Any other string is
104-
/// rejected at configure time (so operators get immediate feedback on typos
105-
/// like `"autom"`) while the runtime resolver still fail-closes on unknown
106-
/// persisted values for defense in depth.
103+
/// Allowed values for [`PROPOSAL_APPROVAL_MODE_KEY`].
104+
///
105+
/// Any other string is rejected at configure time (so operators get immediate
106+
/// feedback on typos like `"autom"`) while the runtime resolver still
107+
/// fail-closes on unknown persisted values for defense in depth.
107108
pub const PROPOSAL_APPROVAL_MODE_VALUES: &[&str] = &["manual", "auto"];
108109

109110
pub const REGISTERED_SETTINGS: &[RegisteredSetting] = &[
@@ -242,7 +243,15 @@ mod tests {
242243
fn proposal_approval_mode_rejects_typos_and_future_modes() {
243244
let setting = setting_for_key(PROPOSAL_APPROVAL_MODE_KEY)
244245
.expect("proposal_approval_mode should be registered");
245-
for bad in ["autom", "AUTO", "Manual", "", " auto", "auto_on_low_risk", "yes"] {
246+
for bad in [
247+
"autom",
248+
"AUTO",
249+
"Manual",
250+
"",
251+
" auto",
252+
"auto_on_low_risk",
253+
"yes",
254+
] {
246255
let err = setting
247256
.validate_string_value(bad)
248257
.expect_err(&format!("expected '{bad}' to be rejected"));

crates/openshell-prover/src/credentials.rs

Lines changed: 189 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -135,27 +135,115 @@ pub struct CredentialSet {
135135
}
136136

137137
impl CredentialSet {
138-
/// Credentials that target a given host. Comparison is case-insensitive
139-
/// so a policy author writing `API.github.com` matches credentials
140-
/// registered for `api.github.com`.
138+
/// Credentials that target a given host. Matching mirrors runtime host
139+
/// policy semantics for exact names and first-label wildcards, so a
140+
/// proposal for `*.github.com` is treated as credentialed when the
141+
/// attached credential targets `api.github.com`.
141142
pub fn credentials_for_host(&self, host: &str) -> Vec<&Credential> {
142-
let needle = host.to_ascii_lowercase();
143143
self.credentials
144144
.iter()
145145
.filter(|c| {
146146
c.target_hosts
147147
.iter()
148-
.any(|h| h.eq_ignore_ascii_case(&needle))
148+
.any(|target| host_patterns_overlap(host, target))
149149
})
150150
.collect()
151151
}
152152

153-
/// API capability registry for a given host. Case-insensitive match.
153+
/// API capability registry for a given host. Exact matches win, then
154+
/// wildcard host overlap is used so credentialed wildcard proposals can be
155+
/// evaluated against concrete API capability registries.
154156
pub fn api_for_host(&self, host: &str) -> Option<&ApiCapability> {
157+
let needle = normalize_host(host);
155158
self.api_registries
156159
.values()
157-
.find(|api| api.host.eq_ignore_ascii_case(host))
160+
.find(|api| normalize_host(&api.host) == needle)
161+
.or_else(|| {
162+
self.api_registries
163+
.values()
164+
.find(|api| host_patterns_overlap(host, &api.host))
165+
})
166+
}
167+
}
168+
169+
fn normalize_host(host: &str) -> String {
170+
host.trim().trim_end_matches('.').to_ascii_lowercase()
171+
}
172+
173+
fn host_patterns_overlap(left: &str, right: &str) -> bool {
174+
let left = normalize_host(left);
175+
let right = normalize_host(right);
176+
if left.is_empty() || right.is_empty() {
177+
return false;
178+
}
179+
left == right || host_pattern_covers(&left, &right) || host_pattern_covers(&right, &left)
180+
}
181+
182+
fn host_pattern_covers(pattern: &str, host: &str) -> bool {
183+
let pattern_labels: Vec<&str> = pattern.split('.').collect();
184+
let host_labels: Vec<&str> = host.split('.').collect();
185+
let Some(first_pattern_label) = pattern_labels.first().copied() else {
186+
return false;
187+
};
188+
189+
if first_pattern_label == "**" {
190+
let suffix = &pattern_labels[1..];
191+
let host_suffix = host_labels
192+
.len()
193+
.checked_sub(suffix.len())
194+
.map(|start| &host_labels[start..]);
195+
return !suffix.is_empty()
196+
&& host_labels.len() > suffix.len()
197+
&& matches!(host_suffix, Some(host_suffix) if host_suffix == suffix);
198+
}
199+
200+
if !first_pattern_label.contains('*') {
201+
return false;
158202
}
203+
204+
// Runtime host wildcards only apply in the first DNS label. Wildcards in
205+
// later labels are not treated as policy globs here.
206+
pattern_labels.len() == host_labels.len()
207+
&& pattern_labels[1..] == host_labels[1..]
208+
&& wildcard_label_matches(first_pattern_label, host_labels[0])
209+
}
210+
211+
fn wildcard_label_matches(pattern: &str, label: &str) -> bool {
212+
if pattern == "*" {
213+
return !label.is_empty();
214+
}
215+
if label.is_empty() || !pattern.contains('*') {
216+
return false;
217+
}
218+
219+
let parts: Vec<&str> = pattern.split('*').collect();
220+
let mut remaining = label;
221+
222+
if let Some(prefix) = parts.first().copied().filter(|part| !part.is_empty()) {
223+
let Some(stripped) = remaining.strip_prefix(prefix) else {
224+
return false;
225+
};
226+
remaining = stripped;
227+
}
228+
229+
if parts.len() > 2 {
230+
for part in parts[1..parts.len() - 1]
231+
.iter()
232+
.copied()
233+
.filter(|part| !part.is_empty())
234+
{
235+
let Some(offset) = remaining.find(part) else {
236+
return false;
237+
};
238+
remaining = &remaining[offset + part.len()..];
239+
}
240+
}
241+
242+
parts
243+
.last()
244+
.copied()
245+
.filter(|suffix| !suffix.is_empty())
246+
.is_none_or(|suffix| remaining.ends_with(suffix))
159247
}
160248

161249
// ---------------------------------------------------------------------------
@@ -276,3 +364,97 @@ pub fn load_credential_set_from_dir(
276364
api_registries,
277365
})
278366
}
367+
368+
#[cfg(test)]
369+
mod tests {
370+
use super::*;
371+
372+
fn github_credential() -> Credential {
373+
Credential {
374+
name: "github-pat".to_string(),
375+
cred_type: "github-pat".to_string(),
376+
scopes: vec!["repo".to_string()],
377+
injected_via: "GITHUB_TOKEN".to_string(),
378+
target_hosts: vec!["api.github.com".to_string()],
379+
}
380+
}
381+
382+
fn github_api() -> ApiCapability {
383+
ApiCapability {
384+
api: "github".to_string(),
385+
host: "api.github.com".to_string(),
386+
port: 443,
387+
credential_type: "github-pat".to_string(),
388+
scope_capabilities: HashMap::new(),
389+
action_risk: HashMap::new(),
390+
}
391+
}
392+
393+
#[test]
394+
fn host_patterns_overlap_matches_exact_case_and_trailing_dot() {
395+
assert!(host_patterns_overlap("API.GITHUB.COM.", "api.github.com"));
396+
assert!(!host_patterns_overlap(
397+
"api.github.com",
398+
"uploads.github.com"
399+
));
400+
}
401+
402+
#[test]
403+
fn host_patterns_overlap_matches_first_label_wildcard_only() {
404+
assert!(host_patterns_overlap("*.github.com", "api.github.com"));
405+
assert!(!host_patterns_overlap("*.github.com", "github.com"));
406+
assert!(!host_patterns_overlap(
407+
"*.github.com",
408+
"deep.api.github.com"
409+
));
410+
}
411+
412+
#[test]
413+
fn host_patterns_overlap_matches_intra_label_first_label_wildcard() {
414+
assert!(host_patterns_overlap(
415+
"api-*.github.com",
416+
"api-v3.github.com"
417+
));
418+
assert!(!host_patterns_overlap(
419+
"api-*.github.com",
420+
"uploads.github.com"
421+
));
422+
assert!(!host_patterns_overlap(
423+
"api.*.github.com",
424+
"api.v3.github.com"
425+
));
426+
}
427+
428+
#[test]
429+
fn host_patterns_overlap_matches_recursive_first_label_wildcard() {
430+
assert!(host_patterns_overlap("**.github.com", "api.github.com"));
431+
assert!(host_patterns_overlap(
432+
"**.github.com",
433+
"deep.api.github.com"
434+
));
435+
assert!(!host_patterns_overlap("**.github.com", "github.com"));
436+
}
437+
438+
#[test]
439+
fn wildcard_policy_host_finds_credentialed_concrete_target() {
440+
let set = CredentialSet {
441+
credentials: vec![github_credential()],
442+
api_registries: HashMap::new(),
443+
};
444+
445+
let creds = set.credentials_for_host("*.github.com");
446+
assert_eq!(creds.len(), 1);
447+
assert_eq!(creds[0].name, "github-pat");
448+
}
449+
450+
#[test]
451+
fn wildcard_policy_host_finds_concrete_api_registry() {
452+
let set = CredentialSet {
453+
credentials: Vec::new(),
454+
api_registries: HashMap::from([("github".to_string(), github_api())]),
455+
};
456+
457+
let api = set.api_for_host("*.github.com").expect("github API");
458+
assert_eq!(api.host, "api.github.com");
459+
}
460+
}

0 commit comments

Comments
 (0)