From 1c5a0f7d07675145b466888874ba31f64ffb7ffa Mon Sep 17 00:00:00 2001 From: mkoushni Date: Wed, 26 Aug 2026 18:19:47 +0300 Subject: [PATCH 1/7] fix: close fail-open paths found by the security analysis A panicking parallel branch, omitted JWT audiences, != on a missing attribute, and an unreadable handler result all resolved toward Allow. Close those, enforce the egress table on the bundled transport, and stop leg-2 OAuth errors from echoing the subject token. Dispositions are in docs/security-analysis.md. Signed-off-by: mkoushni --- CHANGELOG.md | 18 +- Cargo.lock | 1 + .../plugins/delegator-oauth/src/delegator.rs | 24 +- .../delegator-oauth/tests/oauth_e2e.rs | 40 ++-- builtins/plugins/identity-jwt/src/config.rs | 40 +++- builtins/plugins/identity-jwt/src/resolver.rs | 143 +++++++++++- .../identity-jwt/src/trusted_issuer.rs | 11 +- crates/ppe-apl-core/src/evaluator.rs | 185 +++++++++++++-- crates/ppe-core/src/executor.rs | 123 +++++++++- crates/ppe/Cargo.toml | 3 + crates/ppe/src/http_hyper.rs | 219 +++++++++++++++++- crates/ppe/tests/http_hyper_e2e.rs | 27 ++- docs/security-analysis.md | 209 +++++++++++++++++ 13 files changed, 948 insertions(+), 95 deletions(-) create mode 100644 docs/security-analysis.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d8c46c..28e38fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,7 +31,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - **Retries are keyed to whether a repeat is safe.** `RetryPolicy` distinguishes an operation that can be repeated from one that cannot, and `HttpTransportError::may_have_reached_peer` answers the question a caller actually needs. A JWKS `GET` retries freely; a token exchange and a CIBA dispatch retry only failures that provably never reached the peer, because a timeout cannot tell "never arrived" from "the reply was lost" and repeating either would mint a second credential or ask a human twice. -- **`delegation.egress_denied` / `elicitation.egress_denied`.** New deny codes for the case where the host refuses a call before it leaves the process — an egress policy, an SSRF guard, an open circuit. Kept distinct from `idp_unreachable` on purpose: "we declined to try" and "we tried and failed" send an operator to different places, and collapsing them turns a blocked destination into a phantom network problem. No behaviour changes until a host transport produces the refusal; the bundled hyper transport never does. +- **`delegation.egress_denied` / `elicitation.egress_denied`.** New deny codes for the case where the host refuses a call before it leaves the process — an egress policy, an SSRF guard, an open circuit. Kept distinct from `idp_unreachable` on purpose: "we declined to try" and "we tried and failed" send an operator to different places, and collapsing them turns a blocked destination into a phantom network problem. The bundled hyper transport produces the refusal for destinations in the shared address table. - **A shared table of addresses an outbound call must not reach.** `praxis_policy_core::http_addr` covers loopback, RFC 1918, link-local (the cloud-metadata range), CGNAT `100.64/10`, the IPv6 equivalents, and the embedded-IPv4 forms including NAT64. The table only; `praxis-policy-core` opens no sockets, so a transport enforces it where it dials. Sharing it stops three transports each writing a range list that drifts, and these are exactly the ranges that look finished while missing an entry. @@ -57,6 +57,22 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - **A workload's trust domain is no longer mappable.** It is the authority of the SPIFFE ID, so it is derived from the identity rather than read from a claim. ([#31](https://github.com/praxis-proxy/policy/pull/31)) +### Security + +- **A panicking `parallel:` branch is a Deny.** Dropping it let a sibling Allow stand in for a gate that never finished, the same fail-open shape as pairing a Deny with Aborted. The Deny reason says fail-closed and names the panic. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **The bundled hyper transport enforces the shared address table.** Loopback, RFC 1918, link-local (including cloud metadata), and CGNAT are refused at the address that would be dialled, including IP literals that never hit DNS. `with_allow_private_destinations` is the hatch for a local `IdP`. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **Leg-2 token-exchange denials no longer forward `error_description` or the raw body.** Leg 1 already dropped those because an `IdP` may echo the submitted credential; leg 2 submits the caller's bearer as `subject_token` and had the same leak. The violation now carries the OAuth `error` code or the HTTP status. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **An omitted JWT `audiences` list is refused at load.** An empty list used to disable `aud` checking, so a token minted for another app was accepted if the signature and issuer matched. The hatch is `skip_audience_validation: true`. **Breaking** for a config that listed no audiences. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **`!=` on a missing attribute is true.** `subject.role != "admin": deny` did not fire when `role` was absent, because every missing comparison returned false, including `NotEq` — so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **Non-finite string amounts do not order-compare.** `"NaN"`, `"inf"`, and `"-Infinity"` parse as `f64` but every IEEE order test against them is false, so `args.amount > 10000: deny` allowed them. They are now non-numeric, the same as `"lots"`. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + +- **A handler result that cannot be read is an execution error.** Downcast failure used to be treated as Allow in both the serial and concurrent executors, so a deny the framework could not decode was dropped. `on_error: fail` now halts. ([#16](https://github.com/praxis-proxy/policy/issues/16)) + ### Fixed - **Subject claims keep their JSON shape.** `SubjectExtension.claims` holds `serde_json::Value` and flattens into the attribute bag through `payload::walk`, so Keycloak's nested `realm_access.roles` is a `StringSet` a policy can test instead of one opaque string. Client claims always worked this way. **Breaking** for Rust callers reading `claims`; `SubjectExtension::claim_str` covers the scalar lookups. Scalar policies such as `claim.tenant == 'acme'` are unaffected, but a structured claim now sets only the flattened children beneath `claim.`, not the key itself, and a claim whose value is `{}` or `null` sets no key at all where it previously landed as stringified text. ([#9](https://github.com/praxis-proxy/policy/pull/9)) diff --git a/Cargo.lock b/Cargo.lock index a05fcf0..be794fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2334,6 +2334,7 @@ dependencies = [ "praxis-policy-plugin-identity-jwt", "praxis-policy-session-valkey", "tokio", + "tower-service", ] [[package]] diff --git a/builtins/plugins/delegator-oauth/src/delegator.rs b/builtins/plugins/delegator-oauth/src/delegator.rs index f760bc6..5a03b43 100644 --- a/builtins/plugins/delegator-oauth/src/delegator.rs +++ b/builtins/plugins/delegator-oauth/src/delegator.rs @@ -519,24 +519,16 @@ impl OAuthDelegator { let status = response.status; if !response.is_success() { - // Try to surface the standard `error` / `error_description` - // fields from the IdP. Fall back to status code. let body = String::from_utf8_lossy(&response.body).into_owned(); - let (code, reason) = match serde_json::from_str::(&body) { - Ok(err) => { - let mut reason = err.error.clone(); - if let Some(desc) = err.error_description { - reason.push_str(": "); - reason.push_str(&desc); - } - ("delegation.idp_rejected", reason) - }, - Err(_) => ( - "delegation.idp_rejected", - format!("IdP returned {status}: {body}"), - ), + // Same sanitization as leg 1: the OAuth `error` CODE only, + // never `error_description` or the raw body. Leg 2 submits + // the caller's bearer as `subject_token`, and an IdP may + // echo that credential back in those fields. + let reason = match serde_json::from_str::(&body) { + Ok(err) => format!("token exchange rejected: {}", err.error), + Err(_) => format!("token exchange rejected (HTTP {status})"), }; - return Err(PluginViolation::new(code, reason)); + return Err(PluginViolation::new("delegation.idp_rejected", reason)); } let parsed = match serde_json::from_slice::(&response.body) { diff --git a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs index 75248e3..9e328b6 100644 --- a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs +++ b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs @@ -877,20 +877,17 @@ async fn a_leg1_success_that_is_not_a_token_response_denies() { // Leg-2 error shapes // ===================================================================== -/// A leg-2 rejection carrying `error_description` surfaces both the code and -/// the description. -/// -/// Note the asymmetry with leg 1, which deliberately drops the description -/// because an `IdP` may echo the submitted credential back in it. Leg 2 submits -/// the caller's bearer token as `subject_token`, so the same echo is possible -/// here. This test records what the code does today rather than endorsing it. +/// A leg-2 rejection carrying `error_description` must not surface the +/// description. Leg 2 submits the caller's bearer as `subject_token`, and +/// an `IdP` may echo that credential back in the description — the same +/// reason leg 1 drops it. #[tokio::test] -async fn a_leg2_rejection_surfaces_the_error_description() { +async fn a_leg2_rejection_does_not_leak_error_description() { let http = idp( 400, &json!({ "error": "invalid_scope", - "error_description": "read:compensation is not granted to this client", + "error_description": "subject_token eyJhbGciOiJnone.echoed.token is not granted", }) .to_string(), ); @@ -911,17 +908,24 @@ async fn a_leg2_rejection_surfaces_the_error_description() { violation.reason ); assert!( - violation.reason.contains("not granted to this client"), - "and today the description is appended to it: {}", + !violation.reason.contains("eyJhbGciOiJnone"), + "an IdP that echoes the subject_token in error_description must \ + not put it on the violation: {}", + violation.reason + ); + assert!( + !violation.reason.contains("not granted"), + "error_description is free text and is not forwarded: {}", violation.reason ); } /// A leg-2 rejection whose body is not OAuth error JSON falls back to the -/// status. Without the fallback the violation would carry an empty reason. +/// status. The raw body is not forwarded: it can echo the `subject_token` +/// the same way `error_description` can. #[tokio::test] async fn a_leg2_rejection_with_an_unparseable_body_falls_back_to_the_status() { - let http = idp(500, "upstream exploded"); + let http = idp(500, "upstream exploded; subject_token=eyJhbGciOiJnone"); let violation = violation_for( build_payload("get_compensation", "https://hr.example.com", &[]), @@ -934,6 +938,16 @@ async fn a_leg2_rejection_with_an_unparseable_body_falls_back_to_the_status() { "the status must appear when nothing else is parseable: {}", violation.reason ); + assert!( + !violation.reason.contains("upstream exploded"), + "the raw body is not forwarded: {}", + violation.reason + ); + assert!( + !violation.reason.contains("eyJhbGciOiJnone"), + "a body that echoes the subject_token must not land on the violation: {}", + violation.reason + ); } /// Leg 2 answering 200 with a body that carries no `access_token`. There is no diff --git a/builtins/plugins/identity-jwt/src/config.rs b/builtins/plugins/identity-jwt/src/config.rs index 0060440..1100360 100644 --- a/builtins/plugins/identity-jwt/src/config.rs +++ b/builtins/plugins/identity-jwt/src/config.rs @@ -149,10 +149,20 @@ pub struct TrustedIssuerConfig { /// Expected `iss` claim value. pub issuer: String, - /// Expected audience(s). Empty list disables `aud` validation. + /// Expected audience(s). Tokens must carry at least one matching + /// `aud` value. An empty list disables `aud` validation only when + /// [`skip_audience_validation`](Self::skip_audience_validation) is + /// set; omitting both is refused at load, the same class as an + /// empty algorithm list. #[serde(default)] pub audiences: Vec, + /// Opt out of `aud` checking. Default is to require + /// [`audiences`](Self::audiences). Setting this together with a + /// non-empty audience list is refused: the two readings conflict. + #[serde(default)] + pub skip_audience_validation: bool, + /// Algorithms accepted for signature verification (e.g., /// `RS256`, `ES256`). At least one required. pub algorithms: Vec, @@ -735,15 +745,18 @@ impl std::fmt::Display for KeySourceError { impl std::error::Error for KeySourceError {} impl TrustedIssuerConfig { - /// Validate shape (non-empty issuer, at least one algorithm) + /// Validate shape (non-empty issuer, at least one algorithm, + /// audiences configured or explicitly skipped) /// without resolving the key. Used at construction time as a /// fast-fail gate so misshapen YAML is rejected before any /// network I/O is attempted. /// # Errors /// - /// Returns a message when `issuer` is empty or `algorithms` is empty. An - /// issuer with no accepted algorithm can verify nothing, so it is rejected - /// here rather than failing every token later. + /// Returns a message when `issuer` is empty, `algorithms` is empty, or + /// audience checking is neither configured nor explicitly skipped. An + /// issuer with no accepted algorithm can verify nothing; an issuer + /// with no audiences and no skip flag accepts a token minted for + /// any app. pub fn validate(&self) -> Result<(), String> { if self.issuer.trim().is_empty() { return Err("trusted_issuer.issuer must be non-empty".into()); @@ -754,6 +767,20 @@ impl TrustedIssuerConfig { self.issuer )); } + if self.skip_audience_validation && !self.audiences.is_empty() { + return Err(format!( + "trusted_issuer '{}' sets skip_audience_validation together \ + with audiences; pick one", + self.issuer + )); + } + if !self.skip_audience_validation && self.audiences.is_empty() { + return Err(format!( + "trusted_issuer '{}' must list at least one audience \ + (or set skip_audience_validation: true)", + self.issuer + )); + } Ok(()) } @@ -781,6 +808,7 @@ impl TrustedIssuerConfig { Ok(TrustedIssuer { issuer: self.issuer, audiences: self.audiences, + skip_audience_validation: self.skip_audience_validation, keys: std::sync::Arc::new(std::sync::RwLock::new(keys)), algorithms: self.algorithms, leeway_seconds: self.leeway_seconds, @@ -829,6 +857,7 @@ impl TrustedIssuerConfig { Ok(TrustedIssuer { issuer: self.issuer, audiences: self.audiences, + skip_audience_validation: self.skip_audience_validation, keys: std::sync::Arc::new(std::sync::RwLock::new(keys)), algorithms: self.algorithms, leeway_seconds: self.leeway_seconds, @@ -1175,6 +1204,7 @@ mod tests { let cfg = TrustedIssuerConfig { issuer: String::new(), audiences: vec!["a".to_owned()], + skip_audience_validation: false, algorithms: vec![Algorithm::HS256], decoding_key: DecodingKeySource::Secret { secret: "s".into() }, leeway_seconds: 0, diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index a3e9daf..8f5d10a 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -455,6 +455,7 @@ impl Plugin for JwtIdentityResolver { TrustedIssuer { issuer: cfg.issuer.clone(), audiences: cfg.audiences.clone(), + skip_audience_validation: cfg.skip_audience_validation, keys: Arc::new(std::sync::RwLock::new(KeyStore::empty())), algorithms: cfg.algorithms.clone(), leeway_seconds: cfg.leeway_seconds, @@ -808,6 +809,17 @@ impl HookHandler for JwtIdentityResolver { ), )); }, + Err(ValidateError::NoAudiences) => { + return PluginResult::deny(PluginViolation::new( + "auth.no_audiences", + format!( + "issuer '{iss}' lists no audiences and did not set \ + skip_audience_validation, so a token minted for any \ + app would be accepted; this is a configuration fault \ + rather than a problem with the token" + ), + )); + }, Err(ValidateError::Jwt(e)) => { let (code, reason) = classify_jwt_error(&e); return PluginResult::deny(PluginViolation::new(code, reason)); @@ -965,6 +977,12 @@ enum ValidateError { /// list as "accept any algorithm" would let an attacker pick the algorithm, /// which is the classic JWT confusion attack. NoAlgorithms, + /// The issuer carries no audiences and did not opt out of `aud` + /// checking. Same class as [`NoAlgorithms`]: an empty list read as + /// "any audience is acceptable" accepts a token minted for another + /// app. Config load rejects this; the variant exists because + /// `audiences` is a public field. + NoAudiences, /// jsonwebtoken's own validation outcome (signature, exp, /// nbf, iss, aud, algorithm). Jwt(jsonwebtoken::errors::Error), @@ -1027,8 +1045,10 @@ fn validate_token( } else { issuer.leeway_seconds }; - if issuer.audiences.is_empty() { + if issuer.skip_audience_validation { validation.validate_aud = false; + } else if issuer.audiences.is_empty() { + return Err(ValidateError::NoAudiences); } else { let aud_refs: Vec<&str> = issuer.audiences.iter().map(String::as_str).collect(); validation.set_audience(&aud_refs); @@ -1080,6 +1100,7 @@ mod tests { ValidateError::UnknownKid(kid) => format!("UnknownKid({kid:?})"), ValidateError::KeysUnavailable => "KeysUnavailable".to_owned(), ValidateError::NoAlgorithms => "NoAlgorithms".to_owned(), + ValidateError::NoAudiences => "NoAudiences".to_owned(), ValidateError::Jwt(inner) => format!("Jwt({inner})"), } } @@ -1104,7 +1125,8 @@ mod tests { fn empty_algorithm_list_rejects_the_token() { let issuer = TrustedIssuer { issuer: "https://idp.example".into(), - audiences: vec![], + audiences: vec!["test-aud".into()], + skip_audience_validation: false, keys: std::sync::Arc::new(std::sync::RwLock::new(KeyStore::single_fallback( jsonwebtoken::DecodingKey::from_secret(b"secret"), ))), @@ -1124,6 +1146,34 @@ mod tests { ); } + /// Same class as [`empty_algorithm_list_rejects_the_token`]: emptying + /// `audiences` after a valid build must not turn into "any `aud` is + /// acceptable". + #[test] + fn empty_audience_list_rejects_the_token() { + let issuer = TrustedIssuer { + issuer: "https://idp.example".into(), + audiences: vec![], + skip_audience_validation: false, + keys: std::sync::Arc::new(std::sync::RwLock::new(KeyStore::single_fallback( + jsonwebtoken::DecodingKey::from_secret(b"secret"), + ))), + algorithms: vec![jsonwebtoken::Algorithm::HS256], + leeway_seconds: 0, + source: crate::config::DecodingKeySource::Secret { secret: "k".into() }, + refresh: crate::trusted_issuer::RefreshGate::default(), + }; + let token = jwt_with_payload(r#"{"iss":"https://idp.example","sub":"alice"}"#); + + let err = validate_token(&token, &issuer) + .expect_err("an issuer with no audiences cannot skip aud checking"); + assert!( + matches!(err, ValidateError::NoAudiences), + "an empty audience list must surface as NoAudiences, got {}", + variant_of(&err) + ); + } + #[test] fn new_rejects_missing_config_block() { let cfg = PluginConfig { @@ -1148,6 +1198,7 @@ mod tests { let mut config = json!({ "trusted_issuers": [{ "issuer": "https://idp.example.com", + "audiences": ["test-aud"], "algorithms": ["HS256"], "decoding_key": { "kind": "secret", "secret": "x" }, }], @@ -1201,10 +1252,11 @@ mod tests { } } - /// The same hole one level down, and this one is a validation bypass rather - /// than a surprise: `audiences` is defaulted and an empty list turns audience - /// checking off, so a misspelling would silently accept a token minted for - /// any audience. + /// The same hole one level down: `audiences` is defaulted, and a + /// misspelling used to deserialize as an empty list that turned + /// audience checking off. Unknown keys are rejected so that cannot + /// happen; an omitted list is refused at load unless + /// `skip_audience_validation` is set. #[test] fn new_rejects_a_misspelled_issuer_key_rather_than_dropping_audience_validation() { let err = format!( @@ -1237,6 +1289,7 @@ mod tests { "algorithms": ["HS256"], "decoding_key": { "kind": "secret", "secret": "x" }, "leeway_seconds": 30, + "skip_audience_validation": false, }], "role": "client", "header": "X-Client-Token", @@ -1650,7 +1703,7 @@ mod tests { /// send. Failing at load turns both into a gateway that refuses to start. #[test] fn each_malformed_config_is_refused_at_load_with_a_message_naming_the_fault() { - let cases: [(&str, Value, &str); 5] = [ + let cases: [(&str, Value, &str); 8] = [ ( "trusted_issuers is not a list", json!({ "trusted_issuers": "https://idp.example" }), @@ -1661,17 +1714,43 @@ mod tests { json!({ "trusted_issuers": [{ "issuer": "https://idp.example", + "audiences": ["test-aud"], "algorithms": [], "decoding_key": { "kind": "secret", "secret": "x" }, }], }), "at least one algorithm", ), + ( + "an issuer entry lists no audiences", + json!({ + "trusted_issuers": [{ + "issuer": "https://idp.example", + "algorithms": ["HS256"], + "decoding_key": { "kind": "secret", "secret": "x" }, + }], + }), + "at least one audience", + ), + ( + "skip_audience_validation together with a list", + json!({ + "trusted_issuers": [{ + "issuer": "https://idp.example", + "audiences": ["test-aud"], + "skip_audience_validation": true, + "algorithms": ["HS256"], + "decoding_key": { "kind": "secret", "secret": "x" }, + }], + }), + "skip_audience_validation", + ), ( "an issuer's decoding key cannot be built", json!({ "trusted_issuers": [{ "issuer": "https://idp.example", + "audiences": ["test-aud"], "algorithms": ["RS256"], "decoding_key": { "kind": "pem", "pem": "not a pem document" }, }], @@ -1683,6 +1762,7 @@ mod tests { json!({ "trusted_issuers": [{ "issuer": "https://idp.example", + "audiences": ["test-aud"], "algorithms": ["HS256"], "decoding_key": { "kind": "secret", "secret": "x" }, }], @@ -1695,6 +1775,7 @@ mod tests { json!({ "trusted_issuers": [{ "issuer": "https://idp.example", + "audiences": ["test-aud"], "algorithms": ["HS256"], "decoding_key": { "kind": "secret", "secret": "x" }, }], @@ -1702,6 +1783,18 @@ mod tests { }), "non-empty HTTP header name", ), + ( + "an issuer entry lists an empty audiences array", + json!({ + "trusted_issuers": [{ + "issuer": "https://idp.example", + "audiences": [], + "algorithms": ["HS256"], + "decoding_key": { "kind": "secret", "secret": "x" }, + }], + }), + "at least one audience", + ), ]; for (label, config, expected) in cases { @@ -1832,6 +1925,36 @@ mod tests { ); } + /// Omitting `audiences` used to disable `aud` checking, the same + /// class as an empty algorithm list. `skip_audience_validation` is + /// the explicit hatch: a token minted for another app is accepted. + #[tokio::test] + async fn skip_audience_validation_accepts_a_token_minted_for_another_app() { + let resolver = JwtIdentityResolver::new(cfg_with_config( + "jwt", + json!({ + "trusted_issuers": [{ + "issuer": "https://idp.example", + "skip_audience_validation": true, + "algorithms": ["HS256"], + "decoding_key": { "kind": "secret", "secret": "test-secret" }, + }], + "role": "user", + }), + )) + .expect("skip_audience_validation is the hatch for no aud check"); + let token = sign_with( + b"test-secret", + &valid_claims(json!({ "aud": "some-other-api" })), + ); + let result = result_for(&resolver, &token).await; + assert!( + result.continue_processing, + "an explicit skip must accept any aud: {:?}", + result.violation + ); + } + /// Signature, expiry and audience failures each get their own code. They are /// separated because an operator reading `auth.audience_mismatch` looks at /// the audience config, and one reading `auth.signature_invalid` looks at @@ -2005,7 +2128,8 @@ mod tests { fn an_issuer_with_no_keys_reports_unavailable_rather_than_blaming_the_token() { let issuer = TrustedIssuer { issuer: "https://idp.example".into(), - audiences: vec![], + audiences: vec!["test-aud".into()], + skip_audience_validation: false, keys: std::sync::Arc::new(std::sync::RwLock::new(KeyStore::empty())), algorithms: vec![jsonwebtoken::Algorithm::HS256], leeway_seconds: 0, @@ -2029,7 +2153,8 @@ mod tests { fn a_token_whose_kid_matches_no_key_is_reported_as_an_unknown_kid() { let issuer = TrustedIssuer { issuer: "https://idp.example".into(), - audiences: vec![], + audiences: vec!["test-aud".into()], + skip_audience_validation: false, keys: std::sync::Arc::new(std::sync::RwLock::new(KeyStore::from_jwks_entries([( "key-1".to_owned(), jsonwebtoken::DecodingKey::from_secret(b"test-secret"), diff --git a/builtins/plugins/identity-jwt/src/trusted_issuer.rs b/builtins/plugins/identity-jwt/src/trusted_issuer.rs index 122e452..0c639b4 100644 --- a/builtins/plugins/identity-jwt/src/trusted_issuer.rs +++ b/builtins/plugins/identity-jwt/src/trusted_issuer.rs @@ -144,10 +144,16 @@ pub struct TrustedIssuer { pub issuer: String, /// Expected audience(s). Tokens must carry at least one matching - /// `aud` value. Empty vec means "don't check audience" - /// (only acceptable for trusted-internal flows). + /// `aud` value. Empty vec disables audience checking only when + /// [`skip_audience_validation`] is set. pub audiences: Vec, + /// When true, `aud` is not checked. Produced only from config that + /// set `skip_audience_validation: true`. An empty `audiences` list + /// without this flag is refused at load, and at verify it rejects + /// the token rather than treating any audience as acceptable. + pub skip_audience_validation: bool, + /// Decoding keys for this issuer, indexed by `kid`. For inline /// sources (Pem/Jwk/Secret) this is a single-entry store with /// no kid; for JWKS sources every advertised signature key @@ -357,6 +363,7 @@ impl std::fmt::Debug for TrustedIssuer { f.debug_struct("TrustedIssuer") .field("issuer", &self.issuer) .field("audiences", &self.audiences) + .field("skip_audience_validation", &self.skip_audience_validation) .field("algorithms", &self.algorithms) .field("leeway_seconds", &self.leeway_seconds) .field("keys", &self.keys) diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 145df4f..0c9db08 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -4,7 +4,11 @@ // APL evaluator — walks the IR against an AttributeBag and returns a Decision. // // The evaluator is sync and infallible by design. Missing attributes resolve -// to `false`; operator type mismatches resolve to `false`. +// to `false` for presence, equality, membership, and order; operator type +// mismatches resolve to `false`. `!=` is the exception: an absent key is +// not equal to a concrete value, so `NotEq` is true, matching `!(x == y)`. +// A deny rule written as `subject.role != "admin"` therefore fires when +// the role is missing rather than falling through to Allow. // The host drives the four phases separately by calling `evaluate_rules` once // per declared phase — phase orchestration lives in `praxis-policy-apl-runtime`. // @@ -105,7 +109,8 @@ fn eval_condition(cond: &Condition, bag: &AttributeBag) -> bool { .unwrap_or(false), Condition::Comparison { key, op, value } => match bag.resolve_key(key) { Some(k) => eval_comparison(&k, *op, value, bag), - None => false, + // An unresolvable interpolated path is an absent key. + None => matches!(*op, CompareOp::NotEq), }, Condition::InSet { value_key, @@ -127,7 +132,11 @@ fn eval_condition(cond: &Condition, bag: &AttributeBag) -> bool { fn eval_comparison(key: &str, op: CompareOp, lit: &Literal, bag: &AttributeBag) -> bool { let attr = match bag.get(key) { Some(v) => v, - None => return false, // missing → false + // Missing is false for every operator except `!=`. Treating + // `NotEq` as false here broke duality with `!(x == y)` and let + // `role != "admin": deny` fall through to Allow when `role` was + // omitted. + None => return matches!(op, CompareOp::NotEq), }; match op { @@ -231,8 +240,8 @@ fn numeric_compare(attr: &AttributeValue, lit: &Literal, op: OrderOp) -> bool { fn coerce_f64_attr(attr: &AttributeValue) -> Option { match attr { AttributeValue::Int(a) => Some(*a as f64), - AttributeValue::Float(a) => Some(*a), - AttributeValue::String(s) => s.trim().parse::().ok(), + AttributeValue::Float(a) => finite_f64(*a), + AttributeValue::String(s) => s.trim().parse::().ok().and_then(finite_f64), _ => None, } } @@ -246,12 +255,21 @@ fn coerce_f64_attr(attr: &AttributeValue) -> Option { fn coerce_f64_lit(lit: &Literal) -> Option { match lit { Literal::Int(b) => Some(*b as f64), - Literal::Float(b) => Some(*b), - Literal::String(s) => s.trim().parse::().ok(), + Literal::Float(b) => finite_f64(*b), + Literal::String(s) => s.trim().parse::().ok().and_then(finite_f64), _ => None, } } +/// `f64::from_str` accepts `NaN`, `inf`, and `-inf`. IEEE order tests +/// against those are all false (`NaN > 10000` is false, `-inf > 10000` +/// is false), so a string amount of `"NaN"` would slip past +/// `args.amount > 10000: deny`. Non-finite is non-numeric, same as +/// `"lots"`. +fn finite_f64(f: f64) -> Option { + f.is_finite().then_some(f) +} + /// Heuristic: does `s` look like a bag attribute reference (e.g. /// `claim.manager`, `user.sub`) rather than a literal identity (e.g. /// `alice@corp.com`, a bare username)? Used to decide whether an @@ -1093,11 +1111,11 @@ fn dispatch_parallel<'a>( // Aggregate in input order: append every branch's taints; pick // the first Halt (by branch index, not wall-clock order) as the - // overall result. Aborted / panicked branches contribute no - // taints — they didn't run to completion. A panicked branch is - // *not* converted into a Halt; we log via `tracing::warn!` and - // continue. (A misbehaving plugin shouldn't take down the - // parallel block any more than it would the host process.) + // overall result. Aborted branches contribute no taints — they + // were cancelled because a sibling already denied. A panicked + // or timed-out branch is a Halt: dropping it would let a + // sibling Allow stand in for a gate that never finished, which + // is the same fail-open shape as pairing a Deny with Aborted. let mut first_halt: Option = None; for (idx, outcome) in outcomes.into_iter().enumerate() { match outcome { @@ -1120,18 +1138,24 @@ fn dispatch_parallel<'a>( }, BranchOutcome::TimedOut => { // Unreachable today (no per-branch timeout - // configured). Treat as a no-op if it ever fires - // post-config-extension. + // configured). Fail closed if it ever fires: a + // gate that did not finish cannot become Allow. + if first_halt.is_none() { + first_halt = Some(Decision::Deny { + reason: Some(format!("parallel branch {idx} timed out (fail-closed)")), + rule_source: fallback_source.to_owned(), + }); + } }, BranchOutcome::Panicked(msg) => { - // A panicking branch is a misbehaving plugin/effect; - // dropping its output (no Halt, no taints) keeps the - // parallel block's other branches intact rather than - // taking the whole block down. praxis-policy-apl-core has no - // tracing dep — host integrations that care can - // surface the panic via praxis-policy-core's plugin error - // path. `idx`/`msg` are eaten here. - let _ = (idx, msg); + if first_halt.is_none() { + first_halt = Some(Decision::Deny { + reason: Some(format!( + "parallel branch {idx} panicked (fail-closed): {msg}" + )), + rule_source: fallback_source.to_owned(), + }); + } }, } } @@ -1814,6 +1838,10 @@ mod tests { "data.tenants[subject.tenant].data_region == 'eu'", &bag )); + assert!( + eval_pred("data.tenants[subject.tenant].data_region != 'eu'", &bag), + "NotEq on an unresolvable path must match !(==), not fall through" + ); } #[test] @@ -2089,6 +2117,63 @@ mod tests { !eval_condition(&cmp(CompareOp::Gt, 10000), &bag), "\"lots\" > 10000 is false" ); + + // `parse::()` accepts these. IEEE order tests against them + // are all false, so without the finite filter they would slip + // past `args.amount > 10000: deny`. + for non_finite in ["NaN", "nan", "inf", "-inf", "Infinity", "-Infinity"] { + bag.set("args.amount", non_finite); + assert!( + !eval_condition(&cmp(CompareOp::Gt, 10000), &bag), + "{non_finite:?} must not order-compare" + ); + } + } + + #[test] + fn missing_key_not_eq_is_true() { + // Duality: `x != "admin"` must match `!(x == "admin")` when x + // is absent. Returning false for NotEq let + // `role != "admin": deny` fall through to Allow for a request + // that simply omitted the attribute. + let bag = AttributeBag::new(); + assert!(eval_condition( + &Condition::Comparison { + key: "subject.role".into(), + op: CompareOp::NotEq, + value: "admin".into(), + }, + &bag, + )); + assert!(!eval_condition( + &Condition::Comparison { + key: "subject.role".into(), + op: CompareOp::Eq, + value: "admin".into(), + }, + &bag, + )); + } + + #[test] + fn missing_attribute_not_eq_deny_fails_closed() { + let rule = crate::parser::parse_rule(r#"subject.role != "admin": deny"#, "test") + .expect("a not-equal deny rule parses"); + let bag = AttributeBag::new(); + assert!( + matches!(evaluate_rules(&[rule], &bag), Decision::Deny { .. }), + "an allow-list deny must fire when the attribute is missing" + ); + + let rule = crate::parser::parse_rule(r#"subject.role != "admin": deny"#, "test") + .expect("a not-equal deny rule parses"); + let mut bag = AttributeBag::new(); + bag.set("subject.role", "admin"); + assert_eq!( + evaluate_rules(&[rule], &bag), + Decision::Allow, + "the matching role must not be denied" + ); } #[test] @@ -3985,6 +4070,62 @@ mod tests { } } + #[tokio::test] + async fn parallel_panic_is_fail_closed() { + // A panicking parallel branch used to be dropped, so a sibling + // Allow became the block's result — a gate that never finished + // opened the route. Fail closed: the panic is a Deny. + struct PanickingPlugin; + #[async_trait] + impl PluginInvoker for PanickingPlugin { + async fn invoke( + &self, + _name: &str, + _bag: &AttributeBag, + _invocation: PluginInvocation<'_>, + ) -> Result { + panic!("simulated plugin panic"); + } + } + + let mut bag = AttributeBag::new(); + let plugins: Arc = Arc::new(PanickingPlugin); + let steps = vec![Effect::Parallel(vec![ + Effect::Allow, + Effect::Plugin { + name: "boom".into(), + }, + ])]; + match evaluate_effects( + &steps, + &mut bag, + &(Arc::new(FakePdp { + decision: Decision::Allow, + }) as Arc), + &plugins, + &noop_delegations(), + &noop_elicitations(), + crate::step::DispatchPhase::Pre, + &mut crate::route::RoutePayload::new(serde_json::Value::Null), + ) + .await + .decision + { + Decision::Deny { reason, .. } => { + let reason = reason.expect("fail-closed deny carries a reason"); + assert!( + reason.contains("fail-closed"), + "reason must say fail-closed: {reason}" + ); + assert!( + reason.contains("panic"), + "reason must name the panic: {reason}" + ); + }, + d => panic!("a panicking parallel branch must deny, got {d:?}"), + } + } + #[tokio::test] async fn parallel_allows_when_no_branch_denies() { // Both branches are no-op Allow → overall Continue → route Allow. diff --git a/crates/ppe-core/src/executor.rs b/crates/ppe-core/src/executor.rs index 163dcd9..f8afdef 100644 --- a/crates/ppe-core/src/executor.rs +++ b/crates/ppe-core/src/executor.rs @@ -594,8 +594,45 @@ impl Executor { // Plugin writes to ctx.global_state are committed back // to the canonical store via store_context() below. + } else { + // A handler that boxed the wrong type used to be + // treated as Allow. An unreadable result is an + // execution error: Fail halts, Ignore/Disable are + // the documented knobs. + error!( + "{} plugin '{}' returned unexpected result type", + phase_label, plugin_name + ); + let e = crate::error::PluginError::Execution { + plugin_name: plugin_name.to_owned(), + message: "handler returned unexpected result type".into(), + source: None, + code: None, + details: std::collections::HashMap::new(), + proto_error_code: None, + }; + match on_error { + OnError::Fail if can_block => { + let mut v = crate::error::PluginViolation::new( + "plugin_error", + format!("Plugin '{plugin_name}' failed: {e}"), + ); + v.plugin_name = Some(plugin_name.to_owned()); + return Some(v); + }, + OnError::Fail => { + errors.push((&e).into()); + }, + OnError::Ignore => { + errors.push((&e).into()); + }, + OnError::Disable => { + errors.push((&e).into()); + entry.plugin_ref.disable(); + }, + } } - // If extract failed or no modifications — payload unchanged + // If no modifications — payload unchanged }, Ok(Err(e)) => { error!("{} plugin '{}' failed: {}", phase_label, plugin_name, e); @@ -848,10 +885,15 @@ impl Executor { }); BranchData::Deny(violation) }, - // `Some(..)` with continue_processing=true, OR - // `None` (downcast failed — historically logged - // and treated as Allow) both fall through. - _ => BranchData::Allow, + Some(_) => BranchData::Allow, + None => BranchData::Error(Box::new(PluginError::Execution { + plugin_name, + message: "handler returned unexpected result type".into(), + source: None, + code: None, + details: std::collections::HashMap::new(), + proto_error_code: None, + })), }, Err(e) => BranchData::Error(e), } @@ -1127,7 +1169,9 @@ pub struct ErasedResultFields { /// /// Takes ownership of the Box — the executor consumes the result. /// Logs a warning if the downcast fails (indicates a handler returned -/// the wrong type — a framework bug, not a plugin error). +/// the wrong type — a framework bug, not a plugin error). Callers treat +/// `None` as an execution error so a deny that cannot be read cannot +/// become Allow. pub fn extract_erased(result: Box) -> Option { if let Ok(b) = result.downcast::() { Some(*b) @@ -1297,6 +1341,8 @@ mod tests { Hang, Panic, None, + /// Boxed the wrong `Any` type, so [`extract_erased`] returns None. + WrongType, } struct MockPlugin(PluginConfig); @@ -1335,6 +1381,7 @@ mod tests { }, Failure::Panic => panic!("simulated panic inside a branch"), Failure::None => Ok(erase_result(PluginResult::::allow())), + Failure::WrongType => Ok(Box::new(0_u8)), } } @@ -1392,6 +1439,70 @@ mod tests { assert!(!entry.plugin_ref.is_disabled()); } + /// A handler that boxed the wrong type used to be treated as Allow, + /// so a deny that could not be read was dropped. Fail-closed: the + /// unreadable result is an execution error and `on_error: fail` + /// halts. + #[tokio::test] + async fn a_concurrent_unreadable_result_under_fail_is_fail_closed() { + let (r, _) = run_one(Failure::WrongType, OnError::Fail).await; + assert!(!r.continue_processing, "an unreadable result must halt"); + let v = r.violation.expect("a violation is required to halt"); + assert_eq!(v.code, "plugin_error"); + assert_eq!(v.plugin_name.as_deref(), Some("mock")); + } + + #[tokio::test] + async fn a_concurrent_unreadable_result_under_ignore_continues() { + let (r, entry) = run_one(Failure::WrongType, OnError::Ignore).await; + assert!( + r.continue_processing, + "on_error: ignore is the documented hatch" + ); + assert_eq!(r.errors.len(), 1); + assert!(!entry.plugin_ref.is_disabled()); + } + + fn serial_entry(name: &str, on_error: OnError, failure: Failure) -> HookEntry { + let cfg = PluginConfig { + name: name.into(), + mode: PluginMode::Sequential, + on_error, + ..Default::default() + }; + HookEntry { + plugin_ref: Arc::new(PluginRef::new( + Arc::new(MockPlugin(cfg.clone())), + cfg.clone(), + )), + handler: Arc::new(MockHandler(failure)), + } + } + + #[tokio::test] + async fn a_serial_unreadable_result_under_fail_is_fail_closed() { + let executor = Executor::new(ExecutorConfig { + timeout_seconds: 1, + short_circuit_on_deny: true, + }); + let entry = serial_entry("mock", OnError::Fail, Failure::WrongType); + let tracker = tokio_util::task::TaskTracker::new(); + let payload: Box = Box::new(TestPayload { value: "x".into() }); + let (r, _bg) = executor + .execute( + std::slice::from_ref(&entry), + payload, + Extensions::default(), + None, + &tracker, + ) + .await; + assert!(!r.continue_processing, "an unreadable result must halt"); + let v = r.violation.expect("a violation is required to halt"); + assert_eq!(v.code, "plugin_error"); + assert_eq!(v.plugin_name.as_deref(), Some("mock")); + } + // ---- returned error --------------------------------------------------- #[tokio::test] diff --git a/crates/ppe/Cargo.toml b/crates/ppe/Cargo.toml index f37d4ee..8f7967e 100644 --- a/crates/ppe/Cargo.toml +++ b/crates/ppe/Cargo.toml @@ -84,6 +84,7 @@ http-hyper = [ "dep:http", "dep:async-trait", "dep:tokio", + "dep:tower-service", ] # Private marker meaning "at least one builtin is enabled", which is what gates @@ -147,6 +148,8 @@ async-trait = { workspace = true, optional = true } # default features. The transport spawns nothing and runs on whatever # runtime the host is already using. tokio = { workspace = true, optional = true } +# `Service` for the DNS resolver that drops private addresses. +tower-service = { version = "0.3", optional = true } [dev-dependencies] tokio = { workspace = true, features = ["macros", "rt", "rt-multi-thread"] } diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index f5bcac1..d499faa 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -31,7 +31,10 @@ // so a stray feature unification cannot silently give a host a second // HTTP stack it did not ask for. +use std::net::{IpAddr, SocketAddr}; +use std::pin::Pin; use std::sync::OnceLock; +use std::task::{Context, Poll}; use std::time::Duration; use async_trait::async_trait; @@ -40,11 +43,18 @@ use http_body_util::{BodyExt as _, Full, LengthLimitError, Limited}; use hyper_rustls::HttpsConnector; use hyper_util::client::legacy::Client; use hyper_util::client::legacy::connect::HttpConnector; +use hyper_util::client::legacy::connect::dns::{GaiResolver, Name}; use hyper_util::rt::{TokioExecutor, TokioTimer}; use praxis_policy_core::http::{HttpRequest, HttpResponse, HttpTransport, HttpTransportError}; +use praxis_policy_core::http_addr::private_address_reason; +use tower_service::Service; + +/// Marker in resolver errors so [`classify`] can turn a filtered DNS +/// result into [`HttpTransportError::Rejected`] rather than `Connect`. +const EGRESS_DENIED_PREFIX: &str = "ppe-egress-denied:"; /// The pooling client, shared by every request this transport serves. -type HyperClient = Client, Full>; +type HyperClient = Client>, Full>; /// A `HttpTransport` backed by hyper with rustls. /// @@ -70,6 +80,9 @@ pub struct HyperTransport { pool_max_idle_per_host: usize, tcp_keepalive: Option, http2: bool, + /// When true, skip [`http_addr`](praxis_policy_core::http_addr). For a + /// local `IdP` or a test harness on loopback. Default is false. + allow_private_destinations: bool, } impl Default for HyperTransport { @@ -93,6 +106,7 @@ impl Default for HyperTransport { // ALPN offers h2 and falls back to http/1.1, so this costs // nothing against a peer that does not speak it. http2: true, + allow_private_destinations: false, } } } @@ -188,10 +202,26 @@ impl HyperTransport { self } + /// Permit destinations [`http_addr`](praxis_policy_core::http_addr) + /// would refuse: loopback, RFC 1918, link-local (including cloud + /// metadata), CGNAT. + /// + /// Default is to refuse them. Reach for this when the `IdP` is on + /// the same machine, or in tests that bind a mock on `127.0.0.1`. + /// A host that injects its own transport never sees this knob — + /// that transport's egress policy is the one that counts. + #[must_use] + pub fn with_allow_private_destinations(mut self) -> Self { + self.allow_private_destinations = true; + self + } + /// The shared client, built on first call. fn client(&self) -> &HyperClient { self.client.get_or_init(|| { - let mut http = HttpConnector::new(); + let mut http = HttpConnector::new_with_resolver(EgressResolver { + allow_private: self.allow_private_destinations, + }); // The HTTPS connector wraps this one, so it must accept the // `https` scheme rather than rejecting it as non-HTTP. http.enforce_http(false); @@ -250,10 +280,59 @@ impl HyperTransport { /// in. Guessing `Connect` for an ambiguous failure would license a retry /// that mints a second token. fn classify(err: &hyper_util::client::legacy::Error) -> HttpTransportError { + let msg = err.to_string(); + if let Some(reason) = msg.split(EGRESS_DENIED_PREFIX).nth(1) { + return HttpTransportError::Rejected(reason.trim().to_owned()); + } if err.is_connect() { - HttpTransportError::Connect(err.to_string()) + HttpTransportError::Connect(msg) } else { - HttpTransportError::Io(err.to_string()) + HttpTransportError::Io(msg) + } +} + +/// DNS resolver that drops addresses [`private_address_reason`] would +/// refuse. IP literals never hit DNS, so [`HyperTransport::execute`] +/// checks those separately; this is the connect-time check the table's +/// docs require, so a name that rebinds from public to metadata is +/// refused on the lookup that actually dials. +#[derive(Clone, Copy, Debug)] +struct EgressResolver { + allow_private: bool, +} + +impl Service for EgressResolver { + type Response = std::vec::IntoIter; + type Error = Box; + type Future = + Pin> + Send>>; + + fn poll_ready(&mut self, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn call(&mut self, name: Name) -> Self::Future { + let allow_private = self.allow_private; + let fut = GaiResolver::new().call(name); + Box::pin(async move { + let addrs: Vec = fut.await?.collect(); + if allow_private { + return Ok(addrs.into_iter()); + } + let mut kept = Vec::new(); + let mut denied = None; + for addr in addrs { + match private_address_reason(&addr.ip()) { + Some(reason) => denied = Some(reason), + None => kept.push(addr), + } + } + if kept.is_empty() { + let reason = denied.unwrap_or("no resolvable addresses"); + return Err(format!("{EGRESS_DENIED_PREFIX}{reason}").into()); + } + Ok(kept.into_iter()) + }) } } @@ -272,6 +351,19 @@ impl HttpTransport for HyperTransport { ))); } + // Build the pool even when the destination is later refused, so + // a refused first request still lands the client on the runtime + // that served it. + let client = self.client(); + + if !self.allow_private_destinations + && let Some(host) = uri.host() + && let Some(ip) = host_as_ip(host) + && let Some(reason) = private_address_reason(&ip) + { + return Err(HttpTransportError::Rejected(reason.to_owned())); + } + let mut builder = http::Request::builder().method(req.method.clone()).uri(uri); // Safe: `Request::builder()` starts with an empty header map and // no error, so the map is present until a fallible step runs. @@ -285,7 +377,6 @@ impl HttpTransport for HyperTransport { // `req.connect_timeout` is not consulted: the bound belongs to the // shared connector. See `with_connect_timeout`. The overall // deadline below still covers the connect phase. - let client = self.client(); let limit = req.max_response_bytes; // The deadline covers the *whole* exchange, headers and body. @@ -336,6 +427,21 @@ impl HttpTransport for HyperTransport { } } +/// Parse a URI host as an IP address. +/// +/// `http::Uri::host()` keeps the brackets on an IPv6 literal +/// (`[::ffff:169.254.169.254]`), and that string does not parse as +/// [`IpAddr`]. Stripping them is what makes the pre-connect check see +/// the same address hyper would dial. A hostname is `None` and goes +/// through [`EgressResolver`] instead. +fn host_as_ip(host: &str) -> Option { + let host = host + .strip_prefix('[') + .and_then(|h| h.strip_suffix(']')) + .unwrap_or(host); + host.parse().ok() +} + #[cfg(test)] #[allow( clippy::expect_used, @@ -363,6 +469,89 @@ mod tests { assert!(!err.may_have_reached_peer()); } + #[tokio::test] + async fn a_link_local_literal_is_rejected_without_dialling() { + // 169.254.169.254 is cloud metadata. The table exists so this + // transport refuses it at the address it would connect to, not + // after the bytes have left. + let t = HyperTransport::new(); + let err = t + .execute(HttpRequest::get("http://169.254.169.254/latest/meta-data/")) + .await + .expect_err("metadata is not a public destination"); + assert!(!err.may_have_reached_peer()); + match err { + HttpTransportError::Rejected(reason) => { + assert!( + reason.contains("link-local") || reason.contains("metadata"), + "the refusal must name the rule: {reason}" + ); + }, + other => panic!("expected Rejected, got {other:?}"), + } + } + + #[tokio::test] + async fn a_private_literal_is_rejected_without_dialling() { + let t = HyperTransport::new(); + let err = t + .execute(HttpRequest::get("http://10.0.0.1/jwks")) + .await + .expect_err("RFC 1918 is not a public destination"); + match err { + HttpTransportError::Rejected(reason) => { + assert!( + reason.contains("private"), + "the refusal must name the rule: {reason}" + ); + }, + other => panic!("expected Rejected, got {other:?}"), + } + } + + #[tokio::test] + async fn loopback_is_rejected_unless_the_hatch_is_set() { + let err = HyperTransport::new() + .execute(HttpRequest::get("http://127.0.0.1:1/jwks")) + .await + .expect_err("loopback is in the egress table"); + assert!( + matches!(err, HttpTransportError::Rejected(_)), + "expected Rejected, got {err:?}" + ); + } + + #[tokio::test] + async fn a_mapped_ipv6_metadata_literal_is_rejected_without_dialling() { + // `Uri::host()` keeps the brackets on an IPv6 literal. Parsing + // that string as `IpAddr` fails, which used to skip the table + // and dial. The same address in dotted v4 is already refused. + let t = HyperTransport::new(); + let err = t + .execute(HttpRequest::get( + "http://[::ffff:169.254.169.254]/latest/meta-data/", + )) + .await + .expect_err("mapped metadata is the same host"); + assert!(!err.may_have_reached_peer()); + assert!( + matches!(err, HttpTransportError::Rejected(_)), + "expected Rejected, got {err:?}" + ); + } + + #[tokio::test] + async fn an_ipv6_loopback_literal_is_rejected_without_dialling() { + let err = HyperTransport::new() + .execute(HttpRequest::get("http://[::1]:1/jwks")) + .await + .expect_err("IPv6 loopback is in the egress table"); + assert!( + matches!(err, HttpTransportError::Rejected(_)), + "expected Rejected, got {err:?}" + ); + } + #[tokio::test] async fn a_url_with_no_host_is_rejected_before_dialling() { let t = HyperTransport::new(); @@ -377,8 +566,10 @@ mod tests { async fn a_connection_refused_reports_connect_and_is_retryable() { // Port 1 on loopback: nothing listens, and the refusal arrives // without anything being sent — so a caller may safely retry - // even a token mint. - let t = HyperTransport::new(); + // even a token mint. Loopback is in the egress table, so this + // path uses the local-IdP hatch; the table itself is tested + // separately. + let t = HyperTransport::new().with_allow_private_destinations(); let err = t .execute(HttpRequest::get("http://127.0.0.1:1/jwks")) .await @@ -473,6 +664,20 @@ mod tests { ); } + #[test] + fn ipv6_uri_hosts_are_parsed_despite_brackets() { + // `http::Uri::host()` keeps brackets on IPv6. The pre-connect + // check has to strip them or every v6 literal skips the table. + let mapped: IpAddr = host_as_ip("[::ffff:169.254.169.254]").expect("mapped v6"); + assert!(private_address_reason(&mapped).is_some()); + assert!(host_as_ip("[::1]").is_some()); + assert!(host_as_ip("127.0.0.1").is_some()); + assert!( + host_as_ip("idp.example").is_none(), + "a hostname must go through DNS, not the literal table" + ); + } + #[test] fn constructing_outside_a_runtime_does_not_panic() { // Deliberately not a `#[tokio::test]`: there is no reactor here diff --git a/crates/ppe/tests/http_hyper_e2e.rs b/crates/ppe/tests/http_hyper_e2e.rs index a9ed7bd..7857fca 100644 --- a/crates/ppe/tests/http_hyper_e2e.rs +++ b/crates/ppe/tests/http_hyper_e2e.rs @@ -25,6 +25,11 @@ use bytes::Bytes; use praxis_policy::HyperTransport; use praxis_policy_core::http::{HttpRequest, HttpTransport as _, HttpTransportError}; +/// Mockito binds loopback, which the default egress table refuses. +fn local_transport() -> HyperTransport { + HyperTransport::new().with_allow_private_destinations() +} + #[tokio::test] async fn a_plain_http_get_round_trips() { // `https_or_http`, not `https_only`: identity-jwt exposes an explicit @@ -40,7 +45,7 @@ async fn a_plain_http_get_round_trips() { .create_async() .await; - let t = HyperTransport::new(); + let t = local_transport(); let resp = t .execute(HttpRequest::get(format!("{}/jwks", server.url()))) .await @@ -79,10 +84,7 @@ async fn request_headers_and_a_post_body_reach_the_server() { .header("authorization", "Basic abc") .expect("legal header"); - let resp = HyperTransport::new() - .execute(req) - .await - .expect("mock answers"); + let resp = local_transport().execute(req).await.expect("mock answers"); m.assert_async().await; assert_eq!(resp.status, 200); } @@ -100,7 +102,7 @@ async fn a_non_2xx_status_is_a_response_not_an_error() { .create_async() .await; - let resp = HyperTransport::new() + let resp = local_transport() .execute(HttpRequest::get(format!("{}/missing", server.url()))) .await .expect("a 404 is still a response"); @@ -125,10 +127,7 @@ async fn a_304_is_not_a_success_but_is_not_a_failure_either() { let req = HttpRequest::get(format!("{}/jwks", server.url())) .header("if-none-match", "\"v1\"") .expect("legal header"); - let resp = HyperTransport::new() - .execute(req) - .await - .expect("mock answers"); + let resp = local_transport().execute(req).await.expect("mock answers"); assert!(resp.is_not_modified()); assert!(!resp.is_success()); @@ -149,7 +148,7 @@ async fn an_oversized_body_is_refused_rather_than_truncated() { .await; let req = HttpRequest::get(format!("{}/big", server.url())).max_response_bytes(128); - let err = HyperTransport::new() + let err = local_transport() .execute(req) .await .expect_err("4096 bytes exceeds a 128-byte ceiling"); @@ -206,7 +205,7 @@ async fn a_server_that_stalls_mid_body_trips_the_deadline() { let req = HttpRequest::get(format!("{base}/jwks")).timeout(Duration::from_millis(200)); let started = std::time::Instant::now(); - let err = HyperTransport::new() + let err = local_transport() .execute(req) .await .expect_err("the server never finishes the body"); @@ -237,7 +236,7 @@ async fn one_transport_serves_many_requests_from_one_pool() { .create_async() .await; - let t = HyperTransport::new(); + let t = local_transport(); for _ in 0..3 { let resp = t .execute(HttpRequest::get(format!("{}/jwks", server.url()))) @@ -280,7 +279,7 @@ async fn a_transport_built_on_a_dropped_runtime_still_works() { .enable_all() .build() .expect("init runtime"); - rt.block_on(async { HyperTransport::new() }) + rt.block_on(async { local_transport() }) // `rt` drops here. }) .join() diff --git a/docs/security-analysis.md b/docs/security-analysis.md new file mode 100644 index 0000000..81aee6b --- /dev/null +++ b/docs/security-analysis.md @@ -0,0 +1,209 @@ +# Security analysis pass + +Issue: [#16](https://github.com/praxis-proxy/policy/issues/16). +Date: 2026-08-26. + +The engine decides who may call which tool, what data comes back, and +where that data may go next. A bypass is an authorization failure. This +pass ran the workspace through Cursor `/code-review` (security-review +and Bugbot) and a compound-engineering-style adversarial review of the +priority surfaces in that issue, then triaged every finding. + +Prior fixes that set the class of defect to look for: + +- an integer cast that wrapped `delegation.depth` and turned a depth + rule into a bypass +- a dropped orchestrator outcome that became `Aborted` when it was + really a `Deny` +- an empty issuer algorithm list read as "any algorithm acceptable" +- a missing `nbf` check on inbound JWTs + +Those four are already closed and tested. This write-up records what +else the reviews produced, and what was left as a deliberate fail-open +knob rather than a defect. + +## Reviews run + +| Review | Scope | Result | +|---|---|---| +| Cursor security-review | Workspace priority surfaces plus recent HTTP / cache / Cedar work | Two medium findings (SSRF table unused; OAuth leg-2 leak), both accepted | +| Cursor Bugbot | Same surfaces (natural-language change description; no feature-branch diff) | One medium finding (parallel panic fail-open), accepted | +| CE-style adversarial pass | parser, evaluator, identity-jwt, delegator-oauth, executor, engine, PDP resolvers, orchestration | Four additional P1s accepted (empty audiences, `!=` on missing, non-finite amounts, unreadable handler result); one P2 accepted as documented stub | + +`make audit` (`cargo deny check`) is part of the gate for this issue. + +## Findings + +| ID | Severity | Location | Disposition | Why | +|---|---|---|---|---| +| F1 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`dispatch_parallel`, `BranchOutcome::Panicked`) | **Fix** | A panicking parallel branch was discarded. A sibling `Allow` became the block's result, so a `parallel:` of two PDP gates could permit a request if one evaluator panicked. Same class as the dropped-Deny-became-Aborted bug. | +| F2 | Medium | `crates/ppe/src/http_hyper.rs` | **Fix** | `praxis_policy_core::http_addr` is a table with no caller. The bundled transport dialled loopback, RFC 1918, and link-local (including `169.254.169.254`) when `jwks_url` or a token endpoint pointed there. | +| F3 | Medium | `builtins/plugins/delegator-oauth/src/delegator.rs` (leg 2) | **Fix** | Leg 1 sanitizes IdP errors to the OAuth `error` code. Leg 2 appended `error_description` and, on non-JSON bodies, the raw body. Leg 2 submits the caller's bearer as `subject_token`; a hostile or buggy IdP that echoes it would put that token on `PluginViolation.reason`. Credential exposure, not an auth bypass. | +| F4 | Medium | `builtins/plugins/identity-jwt` (`audiences: []`) | **Fix** | Same class as the empty-algorithm bug. An omitted or empty `audiences` list set `validate_aud = false`, so a token minted for another app (valid `iss` + signature) was accepted. Config load now requires a list or `skip_audience_validation: true`. | +| F5 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`eval_comparison`, `NotEq`) | **Fix** | Missing attributes returned false for every operator, including `!=`. `subject.role != "admin": deny` did not fire when `role` was absent, so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. | +| F6 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`coerce_f64_*`) | **Fix** | String tool-args are coerced with `parse::()`, which accepts `NaN` / `inf`. IEEE order tests against those are all false, so `args.amount > 10000: deny` allowed `"NaN"` and `"-Infinity"`. `"lots"` was already non-numeric; non-finite now matches it. | +| F7 | Medium | `crates/ppe-core/src/executor.rs` (`extract_erased` → Allow) | **Fix** | A handler that boxed the wrong `Any` type was logged and treated as Allow in both serial and concurrent paths. A deny the framework could not decode was dropped. Unreadable results are now execution errors; `on_error: fail` halts. | +| F8 | Low | `crates/ppe-apl-core/src/evaluator.rs` (`Stage::Scan`) | **Accept with reason** | `injection.scan` / `pii.detect` emit a taint label and continue; they do not inspect the field. Tests assert Pass on arbitrary text. The stage is a taint marker so a later `require` can gate; actual detection lives in `plugin(...)`. Operator-visible: a named scan that cannot fail does not block injection by itself. | + +### F1 — parallel panic fail-open + +**Closed.** `BranchOutcome::Panicked` and `TimedOut` are now `Decision::Deny` with a reason that says `fail-closed`. `Aborted` stays a no-op: that is a sibling that already denied, and short-circuit cancelled the rest on purpose. + +Regression: `parallel_panic_is_fail_closed` in +`crates/ppe-apl-core/src/evaluator.rs`. A `parallel:` of `Allow` plus a +plugin that panics must Deny, and the reason must contain `fail-closed` +and `panic`. Removing the Halt conversion makes the test Allow. + +`TimedOut` has no per-branch timeout configured today, so there is no +injection test. The arm is fail-closed if it ever fires. + +### F2 — bundled transport ignored `http_addr` + +**Closed.** IP literals are checked before connect, including IPv6 +literals whose `Uri::host()` still has brackets (`[::1]`, +`[::ffff:169.254.169.254]`). Hostnames go through a DNS resolver that +drops addresses `private_address_reason` would refuse, which is the +connect-time check the table's docs require (a name that rebinds from +public to metadata is refused on the lookup that dials). IPv4-mapped +IPv6 literals are judged by the address they reach. + +`HyperTransport::with_allow_private_destinations` is the hatch for a +local IdP or a mock on loopback. `install_default_http_transport` does +not set it. A host that injects its own transport never sees this knob; +that transport's egress policy is the one that counts. + +Regression: `a_link_local_literal_is_rejected_without_dialling`, +`a_private_literal_is_rejected_without_dialling`, +`loopback_is_rejected_unless_the_hatch_is_set`, +`a_mapped_ipv6_metadata_literal_is_rejected_without_dialling`, and +`an_ipv6_loopback_literal_is_rejected_without_dialling` in +`crates/ppe/src/http_hyper.rs`. Each expects `HttpTransportError::Rejected` +and `may_have_reached_peer() == false`. The connection-refused test now +uses the hatch so it still exercises `Connect` on loopback. + +### F3 — leg-2 IdP errors leaked bearer material + +**Closed.** Leg 2 now matches leg 1: OAuth `error` code only, or +`token exchange rejected (HTTP {status})` when the body is not error +JSON. `error_description` and the raw body are not forwarded. + +Regression: `a_leg2_rejection_does_not_leak_error_description` and the +updated `a_leg2_rejection_with_an_unparseable_body_falls_back_to_the_status` +in `builtins/plugins/delegator-oauth/tests/oauth_e2e.rs`. Both plant a +token-shaped string in the IdP body and require it absent from the +violation. Reverting the sanitization makes those assertions fail. + +### F4 — omitted JWT audiences disabled `aud` checking + +**Closed.** `TrustedIssuerConfig::validate` requires at least one +audience unless `skip_audience_validation: true` is set. Setting both +is refused. At verify, an emptied `audiences` field without the skip +flag is `NoAudiences` rather than `validate_aud = false`. + +**Breaking** for a config that listed no audiences. The operator-visible +hatch is `skip_audience_validation: true`, which accepts a token minted +for any app (or none). + +Regression: `each_malformed_config_is_refused_at_load_with_a_message_naming_the_fault` +covers omitted and empty lists; `empty_audience_list_rejects_the_token` +covers the public-field hole; `skip_audience_validation_accepts_a_token_minted_for_another_app` +pins the hatch. Removing the load check without the skip flag makes +those load tests build. + +### F5 — `!=` on a missing attribute did not deny + +**Closed.** `eval_comparison` returns true for `NotEq` when the key is +absent, matching `!(x == y)`. Equality, membership, and order stay +false on missing, as before. + +Regression: `missing_key_not_eq_is_true` and +`missing_attribute_not_eq_deny_fails_closed` in +`crates/ppe-apl-core/src/evaluator.rs`. An empty bag against +`subject.role != "admin": deny` must Deny; the same rule with +`subject.role = "admin"` must Allow. + +### F6 — non-finite string amounts bypassed numeric deny rules + +**Closed.** `coerce_f64_attr` / `coerce_f64_lit` require `is_finite()` +after parse. Non-finite strings and floats are non-numeric. + +Regression: the non-finite loop in +`numeric_string_args_coerce_for_order_comparison`. Dropping the +`is_finite` filter makes `"NaN" > 10000` true-as-false (the comparison +returns false, so a deny rule does not fire). + +### F7 — unreadable handler result was Allow + +**Closed.** Serial and concurrent paths treat `extract_erased` returning +`None` as `PluginError::Execution`. `on_error: fail` in a blocking phase +halts; Ignore/Disable remain the documented knobs. + +Regression: `a_concurrent_unreadable_result_under_fail_is_fail_closed` +and `a_serial_unreadable_result_under_fail_is_fail_closed` in +`crates/ppe-core/src/executor.rs`. A handler that boxes `u8` instead of +`ErasedResultFields` must halt under Fail. The Ignore companion test +pins that the hatch still works. + +### F8 — `injection.scan` / `pii.detect` are taint markers + +**Accepted with reason.** The evaluator comment states the actual +detection lives in `plugin(...)` variants. Closing this would mean +rejecting the stage at parse (breaking policies that use it as a taint +label) or shipping a scanner in-tree. Operator-visible consequence: a +pipeline that only scans, and never `require`s the taint or calls a +scanner plugin, does not block injection or PII. + +## Deliberate fail-open (accepted with reason) + +These are operator-visible knobs, not defects. Closing them would remove +a documented choice. + +| Knob | Operator-visible consequence | Where | +|---|---|---| +| CEL / OPA `on_error: allow` | A runtime eval error (missing key, non-bool result) becomes Allow. Compile errors, Cedar eval errors, and OPA undefined still Deny. Logged at `error!` as fail-open. CEL cache-full under this knob also Allows; OPA cache-full still Denies. | `builtins/pdps/cel`, `builtins/pdps/opa` | +| Plugin `on_error: ignore` / APL `on_error: continue` | That plugin's deny or error does not halt the route. A later `require` or implicit allow can proceed. Delegate `on_error: continue` also swallows Deny, so the original bearer may continue. | `crates/ppe-core/src/executor.rs`, APL `delegate(..., on_error: continue)` | +| Plugin `mode: audit` / `transform` / `fire_and_forget` | Explicit Deny is suppressed (`can_block` is false). Identity JWT in audit mode cannot enforce. | `PluginMode::can_block` | +| JWT `skip_audience_validation: true` | Any `aud` (or none) is accepted if signature / `iss` / `exp` / `nbf` hold. | `identity-jwt` | +| JWT `insecure_http: true` on JWKS | Plaintext JWKS; MITM can swap keys. Default false. | `DecodingKeySource::JwksUrl` | +| OAuth `insecure_http: true` | Client secret + subject token over HTTP. Default false. | `delegator-oauth` | +| OAuth IdP omits `scope` | Subset check is skipped; requested scopes are recorded as granted (RFC 6749 “omitted = granted as requested”). | `delegator.rs` | +| Delegated-token cache `ttl_ceiling_seconds` | A cached token stays usable after an IdP-side revocation until the entry retires. Off unless `cache:` is enabled. | `delegator-oauth` | +| JWKS failed refresh keeps old keys | Withdrawn keys stay valid until a successful fetch (`refresh_secs`). | `identity-jwt` | +| `delegation_without_identity_resolution` | Config-load *alarm*, not a refused start. A `delegate` step can run without `identity:` having validated the inbound token; the IdP is the remaining backstop. | APL config visitor | +| Executor `OnError::Ignore` on concurrent panic | A plugin that declared Ignore and then panicked is skipped. Fail is Deny (`plugin_panic`). | `crates/ppe-core/src/executor.rs` | +| `restrict.on_empty: fallback` | Host may use the unconstrained backend set if the constraint prunes everything. Default is deny. | `constraint.rs` | +| JWT `leeway_seconds: 0` | Means “use the 60s resolver default”, not zero skew. Strict `exp`/`nbf` cannot be configured as zero. | `identity-jwt` | +| Identity omitted on a route | Payload flows through unauthenticated; needs `require(authenticated)`. | `RouteIdentityConfig` | +| `plugin_settings.fail_on_plugin_error` | Ignored. Operators may think it fail-closes; it does not. | `engine.rs` | +| APL empty rule list | Phase default-allows. | `evaluate_rules` | +| F8 scan stages | Taint only; see above. | `Stage::Scan` | + +Cedar has no `on_error: allow`. An evaluation error Denies even when a +sibling permit fired (`evaluation_error_denies_even_when_a_permit_fired`). + +## Rejected as false positive / already closed + +| Claim | Why it is not a finding | +|---|---| +| Empty JWT `algorithms` = any algorithm | Config load rejects an empty list. Verify path is `NoAlgorithms`. Tested. | +| Missing `nbf` | `validate_nbf` is on. Tested. | +| `delegation.depth` wrap | Saturating conversion. Tested. | +| Orchestrator outcome paired off index | Keyed `BTreeMap`; length mismatch is `executor_invariant` Deny. | +| PDP `PdpError::Dispatch` becomes Allow | Evaluator `pdp_error_is_fail_closed`. | +| Cache serving the wrong caller's token | Cache key HMACs the bearer, delegator identity, audience, and scopes. E2E isolation test exists. | +| `alg=none` | `jsonwebtoken` 10.4 has no `Algorithm::None`. | + +## Surfaces reviewed without a new accepted finding + +- `crates/ppe-apl-core/src/parser.rs` — quote stripping shares one + helper; a lone quote no longer slices `1..0`. Glob and default-deny + fallthrough were not found to invert a match. +- `builtins/plugins/identity-jwt` — empty algorithms, `nbf`, JWKS + refresh floor, unknown config keys. Empty audiences is F4. +- `crates/ppe-core/src/executor.rs` / `engine.rs` — Fail/Ignore pairing + and the invariant guard. Unreadable results are F7. The remaining + fail-open is the Ignore knob above. +- PDP resolvers — Cedar fail-closed override is tested; CEL/OPA + `on_error: allow` is the documented knob. +- `builtins/plugins/delegator-oauth` — missing audience still rejects; + omitted IdP `scope` is the documented RFC 6749 trust. From 433db169e8c648782bebfbc0a12919052b33132e Mon Sep 17 00:00:00 2001 From: mkoushni Date: Wed, 26 Aug 2026 18:37:20 +0300 Subject: [PATCH 2/7] test: stop expecting IdP error_description on exchange denials idp_rejection_surfaces_error_code still required the free-text description in the violation. That was the leak the analysis closed; keep the OAuth error code and require the description absent. Signed-off-by: mkoushni --- builtins/plugins/delegator-oauth/tests/oauth_e2e.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs index 9e328b6..f7c9a4b 100644 --- a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs +++ b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs @@ -298,7 +298,8 @@ async fn happy_path_mints_delegated_token() { /// `IdP` returns a 400 with the standard `error` / `error_description` /// shape — delegator surfaces `delegation.idp_rejected` carrying the -/// `IdP`'s machine-readable code. +/// `IdP`'s machine-readable code. The free-text description is not +/// forwarded: an IdP may echo the submitted `subject_token` there. #[tokio::test] async fn idp_rejection_surfaces_error_code() { let http = idp( @@ -323,8 +324,8 @@ async fn idp_rejection_surfaces_error_code() { violation.reason, ); assert!( - violation.reason.contains("not active"), - "reason should include the error_description; got: {}", + !violation.reason.contains("not active"), + "error_description is free text and is not forwarded: {}", violation.reason, ); } From 1c09827c011eaf32b800e8c7db60daab8157c04d Mon Sep 17 00:00:00 2001 From: mkoushni Date: Wed, 26 Aug 2026 18:41:48 +0300 Subject: [PATCH 3/7] style: backtick IdP in the oauth e2e denial comment clippy::doc_markdown fails the lint gate without it. Signed-off-by: mkoushni --- builtins/plugins/delegator-oauth/tests/oauth_e2e.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs index f7c9a4b..e49a4f1 100644 --- a/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs +++ b/builtins/plugins/delegator-oauth/tests/oauth_e2e.rs @@ -299,7 +299,7 @@ async fn happy_path_mints_delegated_token() { /// `IdP` returns a 400 with the standard `error` / `error_description` /// shape — delegator surfaces `delegation.idp_rejected` carrying the /// `IdP`'s machine-readable code. The free-text description is not -/// forwarded: an IdP may echo the submitted `subject_token` there. +/// forwarded: an `IdP` may echo the submitted `subject_token` there. #[tokio::test] async fn idp_rejection_surfaces_error_code() { let http = idp( From 9e5eeb210a20e9d5851fe9582920d5ec113bbbfa Mon Sep 17 00:00:00 2001 From: mkoushni Date: Thu, 27 Aug 2026 12:03:40 +0300 Subject: [PATCH 4/7] fix: fail-close order comparison on non-numeric amounts Treating NaN and inf as non-numeric still skipped a max-amount deny. The phase now Denies; `!` cannot invert that into Allow. Signed-off-by: mkoushni --- CHANGELOG.md | 2 +- crates/ppe-apl-core/src/evaluator.rs | 372 +++++++++++++++++++++------ docs/security-analysis.md | 22 +- 3 files changed, 305 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28e38fd..a336628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,7 +69,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - **`!=` on a missing attribute is true.** `subject.role != "admin": deny` did not fire when `role` was absent, because every missing comparison returned false, including `NotEq` — so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16)) -- **Non-finite string amounts do not order-compare.** `"NaN"`, `"inf"`, and `"-Infinity"` parse as `f64` but every IEEE order test against them is false, so `args.amount > 10000: deny` allowed them. They are now non-numeric, the same as `"lots"`. ([#16](https://github.com/praxis-proxy/policy/issues/16)) +- **A non-numeric or non-finite amount fails an order comparison closed.** `"NaN"`, `"Infinity"`, and `"lots"` made `args.amount > 10000: deny` Allow, because a failed comparison was `false` and skipped the rule. IEEE is the same trap for `NaN` and `-inf`; `inf > 10000` is true, so classifying non-finite as non-numeric also stopped `"Infinity"` from matching. The phase now Denies, and `!` cannot invert that into Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16)) - **A handler result that cannot be read is an execution error.** Downcast failure used to be treated as Allow in both the serial and concurrent executors, so a deny the framework could not decode was dropped. `on_error: fail` now halts. ([#16](https://github.com/praxis-proxy/policy/issues/16)) diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 0c9db08..63264d1 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -3,12 +3,17 @@ // APL evaluator — walks the IR against an AttributeBag and returns a Decision. // -// The evaluator is sync and infallible by design. Missing attributes resolve -// to `false` for presence, equality, membership, and order; operator type -// mismatches resolve to `false`. `!=` is the exception: an absent key is -// not equal to a concrete value, so `NotEq` is true, matching `!(x == y)`. -// A deny rule written as `subject.role != "admin"` therefore fires when -// the role is missing rather than falling through to Allow. +// The evaluator is sync. Missing attributes resolve to `false` for +// presence, equality, membership, and order; operator type mismatches on +// equality and membership resolve to `false`. `!=` is the exception: an +// absent key is not equal to a concrete value, so `NotEq` is true, +// matching `!(x == y)`. A deny rule written as `subject.role != "admin"` +// therefore fires when the role is missing rather than falling through +// to Allow. +// +// Order comparison on a *present* value that is not a finite number is +// not a boolean. `false` skips a deny rule; `true` inverts under `!`. +// The phase Denies instead, with a reason that says fail-closed. // The host drives the four phases separately by calling `evaluate_rules` once // per declared phase — phase orchestration lives in `praxis-policy-apl-runtime`. // @@ -54,8 +59,18 @@ pub enum Decision { /// pick the right entry point for the effects in the rules. pub fn evaluate_rules(rules: &[Rule], bag: &AttributeBag) -> Decision { for rule in rules { - if !eval_expression(&rule.condition, bag) { - continue; + match eval_expression(&rule.condition, bag) { + Ok(false) => continue, + // The condition could not be evaluated. Skipping the rule + // would fall through to Allow; firing it as `true` would + // invert under `!`. Deny the phase. + Err(err) => { + return Decision::Deny { + reason: Some(err.reason()), + rule_source: rule.source.clone(), + }; + }, + Ok(true) => {}, } for effect in &rule.effects { match effect { @@ -79,38 +94,53 @@ pub fn evaluate_rules(rules: &[Rule], bag: &AttributeBag) -> Decision { Decision::Allow } -fn eval_expression(expr: &Expression, bag: &AttributeBag) -> bool { +fn eval_expression(expr: &Expression, bag: &AttributeBag) -> Result { match expr { Expression::Condition(c) => eval_condition(c, bag), - Expression::And(parts) => parts.iter().all(|e| eval_expression(e, bag)), - Expression::Or(parts) => parts.iter().any(|e| eval_expression(e, bag)), - Expression::Not(inner) => !eval_expression(inner, bag), - Expression::Always => true, + Expression::And(parts) => { + for e in parts { + if !eval_expression(e, bag)? { + return Ok(false); + } + } + Ok(true) + }, + Expression::Or(parts) => { + for e in parts { + if eval_expression(e, bag)? { + return Ok(true); + } + } + Ok(false) + }, + // `?` keeps Unorderable from inverting into a permit. + Expression::Not(inner) => Ok(!eval_expression(inner, bag)?), + Expression::Always => Ok(true), } } -fn eval_condition(cond: &Condition, bag: &AttributeBag) -> bool { +fn eval_condition(cond: &Condition, bag: &AttributeBag) -> Result { match cond { // An unresolvable interpolated path (a missing request value) is // treated as an absent key: `IsTrue`/`Exists`/`Comparison` are // false, but `IsFalse` is true (absent is falsy — keeps // `require(...)` fail-closed when the keyed lookup can't resolve). - Condition::IsTrue { key } => bag + Condition::IsTrue { key } => Ok(bag .resolve_key(key) .map(|k| bag.get_bool(&k).unwrap_or(false)) - .unwrap_or(false), - Condition::IsFalse { key } => bag + .unwrap_or(false)), + Condition::IsFalse { key } => Ok(bag .resolve_key(key) .map(|k| !bag.get_bool(&k).unwrap_or(false)) - .unwrap_or(true), - Condition::Exists { key } => bag + .unwrap_or(true)), + Condition::Exists { key } => Ok(bag .resolve_key(key) .map(|k| bag.contains(&k)) - .unwrap_or(false), + .unwrap_or(false)), Condition::Comparison { key, op, value } => match bag.resolve_key(key) { Some(k) => eval_comparison(&k, *op, value, bag), // An unresolvable interpolated path is an absent key. - None => matches!(*op, CompareOp::NotEq), + None => Ok(matches!(*op, CompareOp::NotEq)), }, Condition::InSet { value_key, @@ -124,32 +154,65 @@ fn eval_condition(cond: &Condition, bag: &AttributeBag) -> bool { }, None => false, // an interpolated key didn't resolve }; - if *negate { !in_set } else { in_set } + Ok(if *negate { !in_set } else { in_set }) }, } } -fn eval_comparison(key: &str, op: CompareOp, lit: &Literal, bag: &AttributeBag) -> bool { +fn eval_comparison( + key: &str, + op: CompareOp, + lit: &Literal, + bag: &AttributeBag, +) -> Result { let attr = match bag.get(key) { Some(v) => v, // Missing is false for every operator except `!=`. Treating // `NotEq` as false here broke duality with `!(x == y)` and let // `role != "admin": deny` fall through to Allow when `role` was // omitted. - None => return matches!(op, CompareOp::NotEq), + None => return Ok(matches!(op, CompareOp::NotEq)), }; match op { - CompareOp::Contains => match (attr, lit) { + CompareOp::Contains => Ok(match (attr, lit) { (AttributeValue::StringSet(_), Literal::String(s)) => bag.set_contains(key, s), _ => false, - }, - CompareOp::Eq => values_eq(attr, lit), - CompareOp::NotEq => !values_eq(attr, lit), - CompareOp::Gt => numeric_compare(attr, lit, OrderOp::Gt), - CompareOp::GtEq => numeric_compare(attr, lit, OrderOp::GtEq), - CompareOp::Lt => numeric_compare(attr, lit, OrderOp::Lt), - CompareOp::LtEq => numeric_compare(attr, lit, OrderOp::LtEq), + }), + CompareOp::Eq => Ok(values_eq(attr, lit)), + CompareOp::NotEq => Ok(!values_eq(attr, lit)), + CompareOp::Gt => order_compare(key, attr, lit, OrderOp::Gt), + CompareOp::GtEq => order_compare(key, attr, lit, OrderOp::GtEq), + CompareOp::Lt => order_compare(key, attr, lit, OrderOp::Lt), + CompareOp::LtEq => order_compare(key, attr, lit, OrderOp::LtEq), + } +} + +fn order_compare( + key: &str, + attr: &AttributeValue, + lit: &Literal, + op: OrderOp, +) -> Result { + numeric_compare(attr, lit, op).map_err(|()| Unorderable { + key: key.to_owned(), + }) +} + +/// An order operator was applied to a value that is not a finite number. +/// There is no boolean that is safe to return: `false` skips a deny +/// rule, `true` inverts under `!`. The phase Denies instead. +#[derive(Debug)] +struct Unorderable { + key: String, +} + +impl Unorderable { + fn reason(&self) -> String { + format!( + "order comparison on `{}` failed (fail-closed): value is not a finite number", + self.key + ) } } @@ -190,41 +253,35 @@ fn values_eq(attr: &AttributeValue, lit: &Literal) -> bool { } } -fn numeric_compare(attr: &AttributeValue, lit: &Literal, op: OrderOp) -> bool { +fn numeric_compare(attr: &AttributeValue, lit: &Literal, op: OrderOp) -> Result { // Integer pairs compare exactly, without going through f64 first. Above // 2^53 a double cannot represent every i64, so distinct integers collapse // onto the same value and an ordering test answers the wrong way. This is // the common shape in practice (`args.amount > 10000`), and it is the one // where an exact answer is available for free. if let (AttributeValue::Int(a), Literal::Int(b)) = (attr, lit) { - return match op { + return Ok(match op { OrderOp::Gt => a > b, OrderOp::GtEq => a >= b, OrderOp::Lt => a < b, OrderOp::LtEq => a <= b, - }; + }); } // Every other combination needs a common type, and f64 is it. Numeric-looking // strings are coerced — LLM tool arguments routinely arrive as strings // (e.g. `"amount": "25000"`), and a policy author writing - // `args.amount > 10000` plainly means a numeric comparison. A string - // that doesn't parse as a number is genuinely non-numeric → false - // (order operators don't apply). - let a = match coerce_f64_attr(attr) { - Some(a) => a, - None => return false, - }; - let b = match coerce_f64_lit(lit) { - Some(b) => b, - None => return false, - }; - match op { + // `args.amount > 10000` plainly means a numeric comparison. A present + // value that is not a finite number cannot be ordered: `Err` so the + // phase Denies rather than treating the comparison as false. + let a = coerce_f64_attr(attr).ok_or(())?; + let b = coerce_f64_lit(lit).ok_or(())?; + Ok(match op { OrderOp::Gt => a > b, OrderOp::GtEq => a >= b, OrderOp::Lt => a < b, OrderOp::LtEq => a <= b, - } + }) } /// Coerce a bag attribute to `f64` for an order comparison: numbers pass @@ -261,11 +318,10 @@ fn coerce_f64_lit(lit: &Literal) -> Option { } } -/// `f64::from_str` accepts `NaN`, `inf`, and `-inf`. IEEE order tests -/// against those are all false (`NaN > 10000` is false, `-inf > 10000` -/// is false), so a string amount of `"NaN"` would slip past -/// `args.amount > 10000: deny`. Non-finite is non-numeric, same as -/// `"lots"`. +/// `f64::from_str` accepts `NaN`, `inf`, and `-inf`. IEEE order is not +/// a boolean we can return: `NaN > 10000` and `-inf > 10000` are false +/// (a max-amount deny would skip), `inf > 10000` is true. Non-finite +/// and non-numeric both fail the coerce so the phase Denies. fn finite_f64(f: f64) -> Option { f.is_finite().then_some(f) } @@ -702,8 +758,17 @@ async fn dispatch_effect( // Predicate-gated body — replaces the historical // `Step::Rule`. Skip silently when the condition is false; // otherwise walk the body in order and halt on first Deny. - if !eval_expression(condition, bag) { - return EffectOutcome::Continue; + // An unorderable condition Denies: skipping would drop a + // gate that never finished evaluating. + match eval_expression(condition, bag) { + Ok(false) => return EffectOutcome::Continue, + Err(err) => { + return EffectOutcome::Halt(Decision::Deny { + reason: Some(err.reason()), + rule_source: source.clone(), + }); + }, + Ok(true) => {}, } for inner in body { match Box::pin(dispatch_effect( @@ -960,10 +1025,12 @@ async fn dispatch_elicitation( // bind args — no RFC 9396 RAR. if let Some(scope_src) = &step.scope { match crate::parser::parse_predicate(scope_src) { - Ok(expr) => { - if !eval_expression(&expr, bag) { + Ok(expr) => match eval_expression(&expr, bag) { + Ok(true) => {}, + Ok(false) => { return fail(format!("elicitation scope not satisfied: `{scope_src}`")); - } + }, + Err(err) => return fail(err.reason()), }, Err(e) => { return fail(format!( @@ -1493,7 +1560,18 @@ pub async fn evaluate_pipeline( Stage::Redact { condition } => { let should_redact = match condition { None => true, - Some(expr) => eval_expression(expr, bag), + Some(expr) => match eval_expression(expr, bag) { + Ok(b) => b, + Err(err) => { + return PipelineEvaluation { + outcome: FieldOutcome::Deny { + reason: err.reason(), + stage_index: idx, + }, + taints, + }; + }, + }, }; if should_redact { current = serde_json::Value::String("[REDACTED]".into()); @@ -1677,6 +1755,20 @@ mod tests { use crate::step::{DelegationInvoker, NoopDelegationInvoker}; use std::collections::HashSet; + // Production `eval_*` return `Result` so an unorderable amount can + // Deny the phase. Tests that are not probing that path unwrap. + fn eval_condition(cond: &Condition, bag: &AttributeBag) -> bool { + super::eval_condition(cond, bag).expect("test condition is orderable") + } + + fn eval_expression(expr: &Expression, bag: &AttributeBag) -> bool { + super::eval_expression(expr, bag).expect("test expression is orderable") + } + + fn eval_comparison(key: &str, op: CompareOp, lit: &Literal, bag: &AttributeBag) -> bool { + super::eval_comparison(key, op, lit, bag).expect("test comparison is orderable") + } + fn rule(condition: Expression, effect: Effect, source: &str) -> Rule { Rule::single(condition, effect, source) } @@ -2110,24 +2202,6 @@ mod tests { !eval_condition(&cmp(CompareOp::Gt, 25000), &bag), "\"25000\" > 25000 is false" ); - - // A genuinely non-numeric string still doesn't order-compare. - bag.set("args.amount", "lots"); - assert!( - !eval_condition(&cmp(CompareOp::Gt, 10000), &bag), - "\"lots\" > 10000 is false" - ); - - // `parse::()` accepts these. IEEE order tests against them - // are all false, so without the finite filter they would slip - // past `args.amount > 10000: deny`. - for non_finite in ["NaN", "nan", "inf", "-inf", "Infinity", "-Infinity"] { - bag.set("args.amount", non_finite); - assert!( - !eval_condition(&cmp(CompareOp::Gt, 10000), &bag), - "{non_finite:?} must not order-compare" - ); - } } #[test] @@ -2176,6 +2250,128 @@ mod tests { ); } + #[test] + fn non_numeric_amount_order_deny_fails_closed() { + // `args.amount > 10000: deny` that treats a failed order + // comparison as false Allows `"NaN"` and `"-Infinity"` (IEEE + // `>` is false) and `"Infinity"` / `"lots"` once they are + // classified as non-numeric. Fail closed: the phase Denies, + // including under `!(...)`, so there is no boolean that permits. + let rule = crate::parser::parse_rule("args.amount > 10000: deny", "test") + .expect("max-amount deny parses"); + let negated = crate::parser::parse_rule("!(args.amount > 10000): deny", "test") + .expect("negated max-amount deny parses"); + + let amounts: [AttributeValue; 10] = [ + "NaN".into(), + "nan".into(), + "inf".into(), + "-inf".into(), + "Infinity".into(), + "-Infinity".into(), + "lots".into(), + f64::NAN.into(), + f64::INFINITY.into(), + f64::NEG_INFINITY.into(), + ]; + let mut bag = AttributeBag::new(); + for amount in amounts { + bag.set("args.amount", amount.clone()); + match evaluate_rules(std::slice::from_ref(&rule), &bag) { + Decision::Deny { reason, .. } => { + let reason = reason.expect("fail-closed deny carries a reason"); + assert!( + reason.contains("fail-closed"), + "{amount:?}: reason must say fail-closed: {reason}" + ); + assert!( + reason.contains("args.amount"), + "{amount:?}: reason must name the key: {reason}" + ); + }, + d => panic!("{amount:?}: a non-numeric amount must deny, got {d:?}"), + } + match evaluate_rules(std::slice::from_ref(&negated), &bag) { + Decision::Deny { reason, .. } => { + let reason = reason.expect("fail-closed deny carries a reason"); + assert!( + reason.contains("fail-closed"), + "!(...): {amount:?}: {reason}" + ); + }, + d => panic!("!(...) {amount:?} must still deny, got {d:?}"), + } + } + + bag.set("args.amount", "5000"); + assert_eq!( + evaluate_rules(std::slice::from_ref(&rule), &bag), + Decision::Allow, + "a finite amount under the cap must still allow" + ); + + bag.set("args.amount", "25000"); + match evaluate_rules(std::slice::from_ref(&rule), &bag) { + Decision::Deny { reason, .. } => assert!( + !reason.as_deref().is_some_and(|r| r.contains("fail-closed")), + "a numeric over-cap deny is the rule, not Unorderable: {reason:?}" + ), + d => panic!("\"25000\" > 10000 must deny, got {d:?}"), + } + + // Missing stays false (F5): the cap does not fire. + assert_eq!( + evaluate_rules(std::slice::from_ref(&rule), &AttributeBag::new()), + Decision::Allow, + "a missing amount is not Unorderable" + ); + } + + #[tokio::test] + async fn when_unorderable_amount_is_fail_closed() { + // `Effect::When` is a second evaluation entry: treating + // Unorderable as false would skip the body and Allow. + let mut bag = AttributeBag::new(); + bag.set("args.amount", "NaN"); + let steps = vec![Effect::When { + condition: crate::parser::parse_predicate("args.amount > 10000") + .expect("predicate parses"), + body: vec![Effect::Deny { + reason: Some("over cap".into()), + code: None, + }], + source: "when.test".into(), + }]; + match evaluate_effects( + &steps, + &mut bag, + &(Arc::new(FakePdp { + decision: Decision::Allow, + }) as Arc), + &null_plugins(), + &noop_delegations(), + &noop_elicitations(), + crate::step::DispatchPhase::Pre, + &mut crate::route::RoutePayload::new(serde_json::Value::Null), + ) + .await + .decision + { + Decision::Deny { + reason, + rule_source, + } => { + let reason = reason.expect("fail-closed deny carries a reason"); + assert!( + reason.contains("fail-closed"), + "reason must say fail-closed: {reason}" + ); + assert_eq!(rule_source, "when.test"); + }, + d => panic!("an unorderable When condition must deny, got {d:?}"), + } + } + #[test] fn string_equality_no_ordering() { let mut bag = AttributeBag::new(); @@ -2189,15 +2385,27 @@ mod tests { }, &bag, )); - // Order operators on strings → false. - assert!(!eval_condition( - &Condition::Comparison { + // Order operators on a non-numeric string cannot be a boolean: + // `false` would skip a deny rule. The phase Denies instead. + let rule = rule( + Expression::Condition(Condition::Comparison { key: "subject.id".into(), op: CompareOp::Gt, value: "alice".into(), + }), + deny("ordered strings"), + "test", + ); + match evaluate_rules(&[rule], &bag) { + Decision::Deny { reason, .. } => { + let reason = reason.expect("fail-closed deny carries a reason"); + assert!( + reason.contains("fail-closed"), + "reason must say fail-closed: {reason}" + ); }, - &bag, - )); + d => panic!("order on a non-numeric string must deny, got {d:?}"), + } } #[test] diff --git a/docs/security-analysis.md b/docs/security-analysis.md index 81aee6b..c66d4e6 100644 --- a/docs/security-analysis.md +++ b/docs/security-analysis.md @@ -41,7 +41,7 @@ knob rather than a defect. | F3 | Medium | `builtins/plugins/delegator-oauth/src/delegator.rs` (leg 2) | **Fix** | Leg 1 sanitizes IdP errors to the OAuth `error` code. Leg 2 appended `error_description` and, on non-JSON bodies, the raw body. Leg 2 submits the caller's bearer as `subject_token`; a hostile or buggy IdP that echoes it would put that token on `PluginViolation.reason`. Credential exposure, not an auth bypass. | | F4 | Medium | `builtins/plugins/identity-jwt` (`audiences: []`) | **Fix** | Same class as the empty-algorithm bug. An omitted or empty `audiences` list set `validate_aud = false`, so a token minted for another app (valid `iss` + signature) was accepted. Config load now requires a list or `skip_audience_validation: true`. | | F5 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`eval_comparison`, `NotEq`) | **Fix** | Missing attributes returned false for every operator, including `!=`. `subject.role != "admin": deny` did not fire when `role` was absent, so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. | -| F6 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`coerce_f64_*`) | **Fix** | String tool-args are coerced with `parse::()`, which accepts `NaN` / `inf`. IEEE order tests against those are all false, so `args.amount > 10000: deny` allowed `"NaN"` and `"-Infinity"`. `"lots"` was already non-numeric; non-finite now matches it. | +| F6 | Medium | `crates/ppe-apl-core/src/evaluator.rs` (`numeric_compare`) | **Fix** | String tool-args are coerced with `parse::()`, which accepts `NaN` / `inf`. IEEE `NaN > 10000` and `-inf > 10000` are false, so a max-amount deny skipped them. Treating non-finite as non-numeric (same as `"lots"`) still returned false, so the deny still skipped, and `"Infinity"` stopped matching too (`inf > 10000` is true). A present value that is not a finite number now Denies the phase; `!(...)` cannot invert that into Allow. | | F7 | Medium | `crates/ppe-core/src/executor.rs` (`extract_erased` → Allow) | **Fix** | A handler that boxed the wrong `Any` type was logged and treated as Allow in both serial and concurrent paths. A deny the framework could not decode was dropped. Unreadable results are now execution errors; `on_error: fail` halts. | | F8 | Low | `crates/ppe-apl-core/src/evaluator.rs` (`Stage::Scan`) | **Accept with reason** | `injection.scan` / `pii.detect` emit a taint label and continue; they do not inspect the field. Tests assert Pass on arbitrary text. The stage is a taint marker so a later `require` can gate; actual detection lives in `plugin(...)`. Operator-visible: a named scan that cannot fail does not block injection by itself. | @@ -124,13 +124,19 @@ Regression: `missing_key_not_eq_is_true` and ### F6 — non-finite string amounts bypassed numeric deny rules -**Closed.** `coerce_f64_attr` / `coerce_f64_lit` require `is_finite()` -after parse. Non-finite strings and floats are non-numeric. - -Regression: the non-finite loop in -`numeric_string_args_coerce_for_order_comparison`. Dropping the -`is_finite` filter makes `"NaN" > 10000` true-as-false (the comparison -returns false, so a deny rule does not fire). +**Closed.** An order comparison on a present value that is not a finite +number Denies the phase. Returning `false` skipped `args.amount > +10000: deny`; returning `true` would invert under `!`. The Deny +reason says `fail-closed` and names the key. Missing amounts stay +false, as before (F5). + +Regression: `non_numeric_amount_order_deny_fails_closed` and +`when_unorderable_amount_is_fail_closed` in +`crates/ppe-apl-core/src/evaluator.rs`. `"NaN"`, `"Infinity"`, +`"-Infinity"`, `"lots"`, and the matching `f64` values against +`args.amount > 10000: deny` must Deny, including under `!(...)`. +A finite `"5000"` still Allows; a missing amount still Allows. +Treating the comparison as false makes those assertions Allow. ### F7 — unreadable handler result was Allow From 8daf47de94a6318214f380b4c1fb9389f3c49ba9 Mon Sep 17 00:00:00 2001 From: mkoushni Date: Thu, 27 Aug 2026 12:17:53 +0300 Subject: [PATCH 5/7] docs: qualify skip_audience_validation intra-doc link Bare [`skip_audience_validation`] is not in scope; rustdoc -D warnings failed the docs job. Point it at Self::skip_audience_validation. Signed-off-by: mkoushni --- builtins/plugins/identity-jwt/src/trusted_issuer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtins/plugins/identity-jwt/src/trusted_issuer.rs b/builtins/plugins/identity-jwt/src/trusted_issuer.rs index 0c639b4..cf82b39 100644 --- a/builtins/plugins/identity-jwt/src/trusted_issuer.rs +++ b/builtins/plugins/identity-jwt/src/trusted_issuer.rs @@ -145,7 +145,7 @@ pub struct TrustedIssuer { /// Expected audience(s). Tokens must carry at least one matching /// `aud` value. Empty vec disables audience checking only when - /// [`skip_audience_validation`] is set. + /// [`skip_audience_validation`](Self::skip_audience_validation) is set. pub audiences: Vec, /// When true, `aud` is not checked. Produced only from config that From 68b51143e1f28b7e5b87c4d8d15ae81c54e7c843 Mon Sep 17 00:00:00 2001 From: mkoushni Date: Mon, 31 Aug 2026 12:52:43 +0300 Subject: [PATCH 6/7] fix: fail-close mixed int/float order past 2^53 Coercing integers through f64 for a mixed compare collapsed amounts above 2^53 and a max-amount deny could skip. Those pairs now Deny. Audience load checks the empty-list fault first; emptied audiences at request time use auth.config_error; parallel panic reasons name the effect; the bundled transport documents that it dials the filtered addresses. Signed-off-by: mkoushni --- builtins/plugins/identity-jwt/src/config.rs | 50 +++++- builtins/plugins/identity-jwt/src/resolver.rs | 34 ++++- crates/ppe-apl-core/src/evaluator.rs | 143 +++++++++++++++--- crates/ppe/src/http_hyper.rs | 6 + docs/security-analysis.md | 6 + 5 files changed, 214 insertions(+), 25 deletions(-) diff --git a/builtins/plugins/identity-jwt/src/config.rs b/builtins/plugins/identity-jwt/src/config.rs index 1100360..6dc24cc 100644 --- a/builtins/plugins/identity-jwt/src/config.rs +++ b/builtins/plugins/identity-jwt/src/config.rs @@ -767,17 +767,17 @@ impl TrustedIssuerConfig { self.issuer )); } - if self.skip_audience_validation && !self.audiences.is_empty() { + if !self.skip_audience_validation && self.audiences.is_empty() { return Err(format!( - "trusted_issuer '{}' sets skip_audience_validation together \ - with audiences; pick one", + "trusted_issuer '{}' must list at least one audience \ + (or set skip_audience_validation: true)", self.issuer )); } - if !self.skip_audience_validation && self.audiences.is_empty() { + if self.skip_audience_validation && !self.audiences.is_empty() { return Err(format!( - "trusted_issuer '{}' must list at least one audience \ - (or set skip_audience_validation: true)", + "trusted_issuer '{}' sets skip_audience_validation together \ + with audiences; pick one", self.issuer )); } @@ -1214,4 +1214,42 @@ mod tests { }; assert!(e.contains("issuer"), "{e}"); } + + fn issuer_for_audience_checks(audiences: Vec, skip: bool) -> TrustedIssuerConfig { + TrustedIssuerConfig { + issuer: "https://idp.example".into(), + audiences, + skip_audience_validation: skip, + algorithms: vec![Algorithm::HS256], + decoding_key: DecodingKeySource::Secret { secret: "s".into() }, + leeway_seconds: 0, + } + } + + /// Omitted and explicitly empty `audiences` are the same fault when skip + /// is off: the issuer would accept a token minted for any app. + #[test] + fn omitted_and_empty_audiences_share_the_same_load_error() { + let omitted = issuer_for_audience_checks(Vec::new(), false); + let explicit = issuer_for_audience_checks(vec![], false); + let omitted_err = omitted.validate().expect_err("omitted audiences must fail"); + let explicit_err = explicit.validate().expect_err("empty audiences must fail"); + assert!( + omitted_err.contains("at least one audience"), + "{omitted_err}" + ); + assert_eq!( + omitted_err, explicit_err, + "omitted and `audiences: []` must not produce different messages" + ); + } + + #[test] + fn skip_with_a_list_is_still_a_conflict() { + let err = issuer_for_audience_checks(vec!["app".into()], true) + .validate() + .expect_err("skip plus a list must fail"); + assert!(err.contains("skip_audience_validation"), "{err}"); + assert!(err.contains("pick one"), "{err}"); + } } diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index 8f5d10a..3af575e 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -41,6 +41,7 @@ // * `auth.audience_mismatch` — `aud` didn't include any configured aud // * `auth.algorithm_mismatch` — token uses unaccepted algo // * `auth.mapping_failed` — claim mapper rejected the claims +// * `auth.config_error` — issuer config was emptied after load (`audiences`) // * `auth.token_invalid` — any other validation failure use std::sync::Arc; @@ -811,12 +812,13 @@ impl HookHandler for JwtIdentityResolver { }, Err(ValidateError::NoAudiences) => { return PluginResult::deny(PluginViolation::new( - "auth.no_audiences", + "auth.config_error", format!( "issuer '{iss}' lists no audiences and did not set \ skip_audience_validation, so a token minted for any \ app would be accepted; this is a configuration fault \ - rather than a problem with the token" + discovered at request time because `audiences` is a \ + public field, not a problem with the token" ), )); }, @@ -1174,6 +1176,34 @@ mod tests { ); } + /// Config load rejects this; the runtime path is a fallback because + /// `audiences` is a public field. The deny code is a config fault, not + /// an authentication failure on the token. + #[tokio::test] + async fn emptied_audiences_after_load_deny_as_config_error() { + let resolver = resolver_on_header("authorization"); + { + let mut issuers = resolver.trusted_issuers.write().unwrap(); + let current = &issuers[0]; + let replacement = TrustedIssuer { + issuer: current.issuer.clone(), + audiences: vec![], + skip_audience_validation: false, + keys: Arc::clone(¤t.keys), + algorithms: current.algorithms.clone(), + leeway_seconds: current.leeway_seconds, + source: current.source.clone(), + refresh: crate::trusted_issuer::RefreshGate::default(), + }; + issuers[0] = Arc::new(replacement); + } + let payload = IdentityPayload::new( + jwt_with_payload(r#"{"iss":"https://idp.example","sub":"alice"}"#), + TokenSource::Bearer, + ); + assert_eq!(deny_code_for(&resolver, payload).await, "auth.config_error"); + } + #[test] fn new_rejects_missing_config_block() { let cfg = PluginConfig { diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 63264d1..5d30a92 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -199,9 +199,9 @@ fn order_compare( }) } -/// An order operator was applied to a value that is not a finite number. -/// There is no boolean that is safe to return: `false` skips a deny -/// rule, `true` inverts under `!`. The phase Denies instead. +/// An order operator was applied to a value that cannot be ordered +/// exactly. There is no boolean that is safe to return: `false` skips +/// a deny rule, `true` inverts under `!`. The phase Denies instead. #[derive(Debug)] struct Unorderable { key: String, @@ -210,7 +210,8 @@ struct Unorderable { impl Unorderable { fn reason(&self) -> String { format!( - "order comparison on `{}` failed (fail-closed): value is not a finite number", + "order comparison on `{}` failed (fail-closed): value is not a finite number, \ + or an integer that cannot be compared exactly through f64", self.key ) } @@ -274,6 +275,11 @@ fn numeric_compare(attr: &AttributeValue, lit: &Literal, op: OrderOp) -> Result< // `args.amount > 10000` plainly means a numeric comparison. A present // value that is not a finite number cannot be ordered: `Err` so the // phase Denies rather than treating the comparison as false. + // + // Integers whose magnitude exceeds 2^53 are not exact as `f64`. + // Coercing them for a mixed int/float compare would collapse distinct + // amounts and a max-amount deny could skip. Those pairs are Unorderable + // too, same as `NaN`. let a = coerce_f64_attr(attr).ok_or(())?; let b = coerce_f64_lit(lit).ok_or(())?; Ok(match op { @@ -284,19 +290,32 @@ fn numeric_compare(attr: &AttributeValue, lit: &Literal, op: OrderOp) -> Result< }) } +/// Largest `|n|` for which every integer in `[-n, n]` is a distinct `f64`. +/// Above this, mixed int/float order would collapse distinct amounts. +const F64_EXACT_INT_BOUND: i64 = 9_007_199_254_740_992; // 2^53 + +/// Convert `n` to `f64` only when the conversion is exact. +#[allow( + clippy::cast_precision_loss, + reason = "the bound below is the exact-integer range of f64" +)] +fn i64_as_exact_f64(n: i64) -> Option { + (-F64_EXACT_INT_BOUND..=F64_EXACT_INT_BOUND) + .contains(&n) + .then_some(n as f64) +} + /// Coerce a bag attribute to `f64` for an order comparison: numbers pass /// through; a string is parsed (numeric-looking strings only); anything /// else is non-numeric. /// /// Only reached for operand pairs that are not both integers, since -/// [`numeric_compare`] answers those exactly before coercing. -#[allow( - clippy::cast_precision_loss, - reason = "f64 is the only common type for a mixed int/float comparison" -)] +/// [`numeric_compare`] answers those exactly before coercing. Integers +/// outside [`F64_EXACT_INT_BOUND`] return `None` so the compare Denies +/// rather than rounding. fn coerce_f64_attr(attr: &AttributeValue) -> Option { match attr { - AttributeValue::Int(a) => Some(*a as f64), + AttributeValue::Int(a) => i64_as_exact_f64(*a), AttributeValue::Float(a) => finite_f64(*a), AttributeValue::String(s) => s.trim().parse::().ok().and_then(finite_f64), _ => None, @@ -305,13 +324,9 @@ fn coerce_f64_attr(attr: &AttributeValue) -> Option { /// Coerce a literal to `f64` for an order comparison. Same rules as /// [`coerce_f64_attr`] so `args.x > "10"` works symmetrically. -#[allow( - clippy::cast_precision_loss, - reason = "f64 is the only common type for a mixed int/float comparison" -)] fn coerce_f64_lit(lit: &Literal) -> Option { match lit { - Literal::Int(b) => Some(*b as f64), + Literal::Int(b) => i64_as_exact_f64(*b), Literal::Float(b) => finite_f64(*b), Literal::String(s) => s.trim().parse::().ok().and_then(finite_f64), _ => None, @@ -1208,17 +1223,25 @@ fn dispatch_parallel<'a>( // configured). Fail closed if it ever fires: a // gate that did not finish cannot become Allow. if first_halt.is_none() { + let label = effects + .get(idx) + .map_or_else(|| "unknown".to_owned(), parallel_branch_label); first_halt = Some(Decision::Deny { - reason: Some(format!("parallel branch {idx} timed out (fail-closed)")), + reason: Some(format!( + "parallel branch {idx} ({label}) timed out (fail-closed)" + )), rule_source: fallback_source.to_owned(), }); } }, BranchOutcome::Panicked(msg) => { if first_halt.is_none() { + let label = effects + .get(idx) + .map_or_else(|| "unknown".to_owned(), parallel_branch_label); first_halt = Some(Decision::Deny { reason: Some(format!( - "parallel branch {idx} panicked (fail-closed): {msg}" + "parallel branch {idx} ({label}) panicked (fail-closed): {msg}" )), rule_source: fallback_source.to_owned(), }); @@ -1234,6 +1257,24 @@ fn dispatch_parallel<'a>( }) } +/// Short identity for a parallel branch in fail-closed deny reasons. +fn parallel_branch_label(effect: &Effect) -> String { + match effect { + Effect::Allow => "allow".to_owned(), + Effect::Deny { .. } => "deny".to_owned(), + Effect::Plugin { name } => format!("plugin({name})"), + Effect::Delegate(_) => "delegate".to_owned(), + Effect::Elicit(_) => "elicit".to_owned(), + Effect::Taint { label, .. } => format!("taint({label})"), + Effect::Restrict { .. } => "restrict".to_owned(), + Effect::FieldOp { path, .. } => format!("field_op({path})"), + Effect::Sequential(_) => "sequential".to_owned(), + Effect::Parallel(_) => "parallel".to_owned(), + Effect::When { source, .. } => format!("when({source})"), + Effect::Pdp { call, .. } => format!("pdp({:?})", call.dialect), + } +} + /// Apply a `FieldOp` effect — resolve the path in args/result, run /// the pipeline stages, write the outcome back into the payload. /// @@ -1843,6 +1884,66 @@ mod tests { ); } + /// Mixed int/float order above 2^53 cannot go through `f64` without + /// collapsing distinct amounts. Fail closed rather than answer wrong. + #[test] + #[allow( + clippy::cast_precision_loss, + clippy::float_cmp, + reason = "the lossy conversion is the premise under test" + )] + fn mixed_int_float_order_on_large_integers_is_fail_closed() { + let big = 9_007_199_254_740_993_i64; // 2^53 + 1 + let boundary = 9_007_199_254_740_992_i64; // 2^53 + assert_eq!( + big as f64, boundary as f64, + "premise: these are indistinguishable as doubles" + ); + + let mut bag = AttributeBag::default(); + bag.set("args.amount", AttributeValue::Int(big)); + let err = super::eval_comparison( + "args.amount", + CompareOp::Gt, + &Literal::Float(boundary as f64), + &bag, + ) + .expect_err("mixed order past 2^53 must not become a boolean"); + assert!( + err.reason().contains("fail-closed"), + "reason must say fail-closed: {}", + err.reason() + ); + + bag.set("args.amount", AttributeValue::Float(boundary as f64)); + let err = super::eval_comparison("args.amount", CompareOp::Lt, &Literal::Int(big), &bag) + .expect_err("a large int literal must not coerce through f64 either"); + assert!( + err.reason().contains("fail-closed"), + "reason must say fail-closed: {}", + err.reason() + ); + } + + #[test] + fn mixed_int_float_order_within_exact_range_still_compares() { + let mut bag = AttributeBag::default(); + bag.set("args.amount", AttributeValue::Int(10_000)); + assert!( + eval_comparison("args.amount", CompareOp::Gt, &Literal::Float(9999.5), &bag), + "10000 > 9999.5 must hold" + ); + assert!( + eval_comparison( + "args.amount", + CompareOp::Lt, + &Literal::Float(10_000.5), + &bag + ), + "10000 < 10000.5 must hold" + ); + } + fn deny(reason: &str) -> Effect { Effect::Deny { reason: Some(reason.into()), @@ -4329,6 +4430,14 @@ mod tests { reason.contains("panic"), "reason must name the panic: {reason}" ); + assert!( + reason.contains("plugin(boom)"), + "reason must name the panicking effect: {reason}" + ); + assert!( + reason.contains("branch 1"), + "reason must name the branch index: {reason}" + ); }, d => panic!("a panicking parallel branch must deny, got {d:?}"), } diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index d499faa..11392d9 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -296,6 +296,12 @@ fn classify(err: &hyper_util::client::legacy::Error) -> HttpTransportError { /// checks those separately; this is the connect-time check the table's /// docs require, so a name that rebinds from public to metadata is /// refused on the lookup that actually dials. +/// +/// The connector dials the `SocketAddr`s this resolver returns. A later +/// DNS update does not change the peer: we never resolve a second time +/// between the filter and `connect`. The residual is that an address we +/// accepted is still a public host, and this transport cannot see whether +/// that host forwards to a private one. #[derive(Clone, Copy, Debug)] struct EgressResolver { allow_private: bool, diff --git a/docs/security-analysis.md b/docs/security-analysis.md index c66d4e6..18b2f24 100644 --- a/docs/security-analysis.md +++ b/docs/security-analysis.md @@ -67,6 +67,12 @@ connect-time check the table's docs require (a name that rebinds from public to metadata is refused on the lookup that dials). IPv4-mapped IPv6 literals are judged by the address they reach. +The connector dials the filtered `SocketAddr` list; it does not resolve +the name a second time. A DNS rebind after this lookup therefore cannot +change the peer we connect to. What remains is that a public address we +accepted could still forward to a private host, which this transport +cannot observe. + `HyperTransport::with_allow_private_destinations` is the hatch for a local IdP or a mock on loopback. `install_default_http_transport` does not set it. A host that injects its own transport never sees this knob; From 885503822234a7f8b4eb496e3c38f9fde61bb81b Mon Sep 17 00:00:00 2001 From: mkoushni Date: Tue, 1 Sep 2026 20:00:58 +0300 Subject: [PATCH 7/7] fix: close residual F2/F4/F6 gaps from review Hostname egress refusals were Connect because hyper hides the resolver error in source(); a missing aud claim skipped validation; string amounts past 2^53 rounded through f64. Walk the source chain, require aud when a list is configured, and parse integer strings through the exactness bound. Signed-off-by: mkoushni --- CHANGELOG.md | 6 +- builtins/plugins/identity-jwt/src/resolver.rs | 64 +++++++++++++++ crates/ppe-apl-core/src/evaluator.rs | 41 +++++++++- crates/ppe/src/http_hyper.rs | 79 ++++++++++++++++++- crates/ppe/src/lib.rs | 7 ++ docs/security-analysis.md | 35 ++++++-- 6 files changed, 216 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f89c21d..25561ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -600,15 +600,15 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - **A panicking `parallel:` branch is a Deny.** Dropping it let a sibling Allow stand in for a gate that never finished, the same fail-open shape as pairing a Deny with Aborted. The Deny reason says fail-closed and names the panic. ([#16](https://github.com/praxis-proxy/policy/issues/16)) -- **The bundled hyper transport enforces the shared address table.** Loopback, RFC 1918, link-local (including cloud metadata), and CGNAT are refused at the address that would be dialled, including IP literals that never hit DNS. `with_allow_private_destinations` is the hatch for a local `IdP`. ([#16](https://github.com/praxis-proxy/policy/issues/16)) +- **The bundled hyper transport enforces the shared address table.** Loopback, RFC 1918, link-local (including cloud metadata), and CGNAT are refused at the address that would be dialled, including IP literals that never hit DNS and hostnames that resolve only to those addresses. The refusal is `HttpTransportError::Rejected` (not a connect failure), so a token-endpoint hostname on `10.x` reports `delegation.egress_denied` rather than `idp_unreachable` and is not retried. `with_allow_private_destinations` is the hatch for a local `IdP`. ([#16](https://github.com/praxis-proxy/policy/issues/16)) - **Leg-2 token-exchange denials no longer forward `error_description` or the raw body.** Leg 1 already dropped those because an `IdP` may echo the submitted credential; leg 2 submits the caller's bearer as `subject_token` and had the same leak. The violation now carries the OAuth `error` code or the HTTP status. ([#16](https://github.com/praxis-proxy/policy/issues/16)) -- **An omitted JWT `audiences` list is refused at load.** An empty list used to disable `aud` checking, so a token minted for another app was accepted if the signature and issuer matched. The hatch is `skip_audience_validation: true`. **Breaking** for a config that listed no audiences. ([#16](https://github.com/praxis-proxy/policy/issues/16)) +- **An omitted JWT `audiences` list is refused at load.** An empty list used to disable `aud` checking, so a token minted for another app was accepted if the signature and issuer matched. A configured list also requires the token to carry an `aud` claim; omitting the claim used to pass. The hatch is `skip_audience_validation: true`, which still accepts any `aud` or none. **Breaking** for a config that listed no audiences. ([#16](https://github.com/praxis-proxy/policy/issues/16)) - **`!=` on a missing attribute is true.** `subject.role != "admin": deny` did not fire when `role` was absent, because every missing comparison returned false, including `NotEq` — so it did not match `!(subject.role == "admin")` and an unauthenticated request fell through to Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16)) -- **A non-numeric or non-finite amount fails an order comparison closed.** `"NaN"`, `"Infinity"`, and `"lots"` made `args.amount > 10000: deny` Allow, because a failed comparison was `false` and skipped the rule. IEEE is the same trap for `NaN` and `-inf`; `inf > 10000` is true, so classifying non-finite as non-numeric also stopped `"Infinity"` from matching. The phase now Denies, and `!` cannot invert that into Allow. ([#16](https://github.com/praxis-proxy/policy/issues/16)) +- **A non-numeric or non-finite amount fails an order comparison closed.** `"NaN"`, `"Infinity"`, and `"lots"` made `args.amount > 10000: deny` Allow, because a failed comparison was `false` and skipped the rule. IEEE is the same trap for `NaN` and `-inf`; `inf > 10000` is true, so classifying non-finite as non-numeric also stopped `"Infinity"` from matching. The phase now Denies, and `!` cannot invert that into Allow. Mixed int/float order on an integer whose magnitude exceeds 2^53 is the same Deny — including a string-encoded amount — because coercing through `f64` would collapse distinct values and skip a max-amount rule. ([#16](https://github.com/praxis-proxy/policy/issues/16)) - **A handler result that cannot be read is an execution error.** Downcast failure used to be treated as Allow in both the serial and concurrent executors, so a deny the framework could not decode was dropped. `on_error: fail` now halts. ([#16](https://github.com/praxis-proxy/policy/issues/16)) diff --git a/builtins/plugins/identity-jwt/src/resolver.rs b/builtins/plugins/identity-jwt/src/resolver.rs index 3af575e..b52f352 100644 --- a/builtins/plugins/identity-jwt/src/resolver.rs +++ b/builtins/plugins/identity-jwt/src/resolver.rs @@ -1054,6 +1054,11 @@ fn validate_token( } else { let aud_refs: Vec<&str> = issuer.audiences.iter().map(String::as_str).collect(); validation.set_audience(&aud_refs); + // jsonwebtoken only checks `aud` when the claim is present. + // `set_required_spec_claims` replaces the set (default is `exp`), + // so `exp` and `iss` stay required alongside `aud`. A token with + // no audience is then a missing-claim error, not a pass. + validation.set_required_spec_claims(&["exp", "iss", "aud"]); } decode::(token, key, &validation).map_err(ValidateError::Jwt) } @@ -1066,6 +1071,7 @@ fn classify_jwt_error(e: &jsonwebtoken::errors::Error) -> (&'static str, String) ErrorKind::InvalidSignature => "auth.signature_invalid", ErrorKind::ImmatureSignature => "auth.token_not_yet_valid", ErrorKind::InvalidAudience => "auth.audience_mismatch", + ErrorKind::MissingRequiredClaim(c) if c == "aud" => "auth.audience_mismatch", ErrorKind::InvalidIssuer => "auth.untrusted_issuer", ErrorKind::InvalidAlgorithm | ErrorKind::InvalidAlgorithmName => "auth.algorithm_mismatch", ErrorKind::Base64(_) | ErrorKind::Json(_) => "auth.malformed_header", @@ -1985,6 +1991,64 @@ mod tests { ); } + /// jsonwebtoken only checks `aud` when the claim is present. A + /// configured list used to pass a token that simply omitted it. + #[tokio::test] + async fn a_token_with_no_aud_claim_is_refused() { + let resolver = resolver_on_header("authorization"); + let token = sign_with( + b"test-secret", + &json!({ + "iss": "https://idp.example", + "sub": "alice", + "exp": seconds_from_now(3_600), + }), + ); + let r = result_for(&resolver, &token).await; + assert!( + !r.continue_processing, + "a token with no aud must not pass a configured list: {:?}", + r.violation + ); + assert_eq!( + r.violation.expect("deny carries a violation").code, + "auth.audience_mismatch" + ); + } + + /// The hatch's documented meaning is any `aud`, or none. Requiring + /// the claim on the default path must not leak onto this one. + #[tokio::test] + async fn skip_audience_validation_accepts_a_token_with_no_aud_claim() { + let resolver = JwtIdentityResolver::new(cfg_with_config( + "jwt", + json!({ + "trusted_issuers": [{ + "issuer": "https://idp.example", + "skip_audience_validation": true, + "algorithms": ["HS256"], + "decoding_key": { "kind": "secret", "secret": "test-secret" }, + }], + "role": "user", + }), + )) + .expect("skip_audience_validation is the hatch for no aud check"); + let token = sign_with( + b"test-secret", + &json!({ + "iss": "https://idp.example", + "sub": "alice", + "exp": seconds_from_now(3_600), + }), + ); + let result = result_for(&resolver, &token).await; + assert!( + result.continue_processing, + "the hatch accepts a token with no aud: {:?}", + result.violation + ); + } + /// Signature, expiry and audience failures each get their own code. They are /// separated because an operator reading `auth.audience_mismatch` looks at /// the audience config, and one reading `auth.signature_invalid` looks at diff --git a/crates/ppe-apl-core/src/evaluator.rs b/crates/ppe-apl-core/src/evaluator.rs index 901647e..1e6b83f 100644 --- a/crates/ppe-apl-core/src/evaluator.rs +++ b/crates/ppe-apl-core/src/evaluator.rs @@ -317,7 +317,7 @@ fn coerce_f64_attr(attr: &AttributeValue) -> Option { match attr { AttributeValue::Int(a) => i64_as_exact_f64(*a), AttributeValue::Float(a) => finite_f64(*a), - AttributeValue::String(s) => s.trim().parse::().ok().and_then(finite_f64), + AttributeValue::String(s) => coerce_f64_numeric_string(s), _ => None, } } @@ -328,11 +328,26 @@ fn coerce_f64_lit(lit: &Literal) -> Option { match lit { Literal::Int(b) => i64_as_exact_f64(*b), Literal::Float(b) => finite_f64(*b), - Literal::String(s) => s.trim().parse::().ok().and_then(finite_f64), + Literal::String(s) => coerce_f64_numeric_string(s), _ => None, } } +/// Parse a numeric-looking string for order comparison. +/// +/// Integer spellings go through [`i64_as_exact_f64`]. `parse::()` +/// rounds `"9007199254740993"` onto 2^53, which is the same collapse the +/// `Int` arm refuses — LLM tool arguments arrive as strings, so that is +/// the operand shape the bound was written for. Fractional strings still +/// parse as `f64`. +fn coerce_f64_numeric_string(s: &str) -> Option { + let s = s.trim(); + if let Ok(n) = s.parse::() { + return i64_as_exact_f64(n); + } + s.parse::().ok().and_then(finite_f64) +} + /// `f64::from_str` accepts `NaN`, `inf`, and `-inf`. IEEE order is not /// a boolean we can return: `NaN > 10000` and `-inf > 10000` are false /// (a max-amount deny would skip), `inf > 10000` is true. Non-finite @@ -1925,6 +1940,28 @@ mod tests { ); } + /// LLM tool arguments arrive as strings. `parse::()` rounds + /// `"9007199254740993"` onto 2^53, so a max-amount deny against an + /// int cap would skip. Integer spellings take the same exactness + /// bound as `Int`. + #[test] + fn a_string_encoded_large_int_does_not_round_through_f64() { + let rule = crate::parser::parse_rule("args.amount > 9007199254740992: deny", "test") + .expect("parses"); + let mut bag = AttributeBag::new(); + bag.set("args.amount", "9007199254740993"); + match evaluate_rules(std::slice::from_ref(&rule), &bag) { + Decision::Deny { .. } => {}, + other => panic!("2^53 + 1 as a string is over the cap, got {other:?}"), + } + + bag.set("args.amount", "5000"); + match evaluate_rules(std::slice::from_ref(&rule), &bag) { + Decision::Allow => {}, + other => panic!("a small string amount must still compare, got {other:?}"), + } + } + #[test] fn mixed_int_float_order_within_exact_range_still_compares() { let mut bag = AttributeBag::default(); diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index 11392d9..aa561cc 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -31,6 +31,7 @@ // so a stray feature unification cannot silently give a host a second // HTTP stack it did not ask for. +use std::error::Error; use std::net::{IpAddr, SocketAddr}; use std::pin::Pin; use std::sync::OnceLock; @@ -276,14 +277,20 @@ impl HyperTransport { /// the request: a caller uses it to decide between retrying and /// recording an indeterminate outcome. `is_connect()` is the only signal /// hyper gives us that nothing was sent, so anything else is `Io`, which -/// reads as "may have reached the peer" and is the safe direction to err +/// reads as "may have reached the peer" and the safe direction to err /// in. Guessing `Connect` for an ambiguous failure would license a retry /// that mints a second token. +/// +/// hyper-util's Display is `client error ({kind})` and does not include +/// the source chain, so matching that string never sees +/// [`EGRESS_DENIED_PREFIX`]. Walk [`Error::source`] instead; otherwise a +/// filtered hostname reports `Connect`, maps to `idp_unreachable`, and +/// is retried against a destination that can never be reached. fn classify(err: &hyper_util::client::legacy::Error) -> HttpTransportError { - let msg = err.to_string(); - if let Some(reason) = msg.split(EGRESS_DENIED_PREFIX).nth(1) { - return HttpTransportError::Rejected(reason.trim().to_owned()); + if let Some(reason) = denied_reason(err) { + return HttpTransportError::Rejected(reason); } + let msg = err.to_string(); if err.is_connect() { HttpTransportError::Connect(msg) } else { @@ -291,6 +298,19 @@ fn classify(err: &hyper_util::client::legacy::Error) -> HttpTransportError { } } +/// The private-address reason [`EgressResolver`] stuffed behind +/// [`EGRESS_DENIED_PREFIX`], if any layer of `err` carries it. +fn denied_reason(err: &(dyn Error + 'static)) -> Option { + let mut cur = Some(err); + while let Some(e) = cur { + if let Some((_, reason)) = e.to_string().split_once(EGRESS_DENIED_PREFIX) { + return Some(reason.trim().to_owned()); + } + cur = e.source(); + } + None +} + /// DNS resolver that drops addresses [`private_address_reason`] would /// refuse. IP literals never hit DNS, so [`HyperTransport::execute`] /// checks those separately; this is the connect-time check the table's @@ -504,6 +524,7 @@ mod tests { .execute(HttpRequest::get("http://10.0.0.1/jwks")) .await .expect_err("RFC 1918 is not a public destination"); + assert!(!err.may_have_reached_peer()); match err { HttpTransportError::Rejected(reason) => { assert!( @@ -525,6 +546,7 @@ mod tests { matches!(err, HttpTransportError::Rejected(_)), "expected Rejected, got {err:?}" ); + assert!(!err.may_have_reached_peer()); } #[tokio::test] @@ -556,6 +578,55 @@ mod tests { matches!(err, HttpTransportError::Rejected(_)), "expected Rejected, got {err:?}" ); + assert!(!err.may_have_reached_peer()); + } + + #[tokio::test] + async fn a_hostname_resolving_to_loopback_is_rejected_not_connect() { + // IP literals take the pre-connect check and never enter + // `EgressResolver`. A name has to, and hyper's Display is + // `client error (Connect)` — matching that string used to + // report Connect, which retried and mapped to idp_unreachable. + let err = HyperTransport::new() + .execute(HttpRequest::get("http://localhost:1/jwks")) + .await + .expect_err("localhost resolves to loopback"); + assert!( + matches!(err, HttpTransportError::Rejected(_)), + "expected Rejected, got {err:?}" + ); + assert!(!err.may_have_reached_peer()); + } + + #[test] + fn classify_walks_the_source_chain_for_the_egress_marker() { + // hyper-util formats as `client error (Connect)` and keeps the + // resolver error on `source()`, the way the live client does. + #[derive(Debug)] + struct Marker(&'static str); + impl std::fmt::Display for Marker { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } + } + impl Error for Marker {} + + #[derive(Debug)] + struct Wrap(Marker); + impl std::fmt::Display for Wrap { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("client error (Connect)") + } + } + impl Error for Wrap { + fn source(&self) -> Option<&(dyn Error + 'static)> { + Some(&self.0) + } + } + + let err = Wrap(Marker("ppe-egress-denied:loopback")); + assert_eq!(denied_reason(&err).as_deref(), Some("loopback")); + assert!(denied_reason(&Marker("client error (Connect)")).is_none()); } #[tokio::test] diff --git a/crates/ppe/src/lib.rs b/crates/ppe/src/lib.rs index 741fcb2..a357b2d 100644 --- a/crates/ppe/src/lib.rs +++ b/crates/ppe/src/lib.rs @@ -241,6 +241,13 @@ pub use http_hyper::HyperTransport; /// The transport builds its pool on first use, so calling this from a /// short-lived initialization runtime is safe. /// +/// Destinations in the shared address table — loopback, RFC 1918, +/// link-local (including cloud metadata), CGNAT — are refused as +/// [`praxis_policy_core::http::HttpTransportError::Rejected`], including +/// a hostname that resolves only to those addresses. A local `IdP` needs +/// [`HyperTransport::with_allow_private_destinations`] installed via +/// [`PolicyEngine::set_http_transport`] instead of this helper. +/// /// Returns `false` if a transport was already installed, in which case /// the existing one is kept. #[cfg(feature = "http-hyper")] diff --git a/docs/security-analysis.md b/docs/security-analysis.md index 18b2f24..af6fedb 100644 --- a/docs/security-analysis.md +++ b/docs/security-analysis.md @@ -73,6 +73,14 @@ change the peer we connect to. What remains is that a public address we accepted could still forward to a private host, which this transport cannot observe. +A filtered hostname used to surface as `HttpTransportError::Connect` +because hyper-util's Display is `client error (Connect)` and does not +include the resolver error. `classify` now walks `Error::source()`, so +the marker `EgressResolver` writes becomes `Rejected`. That is what +maps to `delegation.egress_denied` / `elicitation.egress_denied` and +what `http_retry` refuses to retry. IP literals never needed the walk: +they take the pre-connect check. + `HyperTransport::with_allow_private_destinations` is the hatch for a local IdP or a mock on loopback. `install_default_http_transport` does not set it. A host that injects its own transport never sees this knob; @@ -81,11 +89,14 @@ that transport's egress policy is the one that counts. Regression: `a_link_local_literal_is_rejected_without_dialling`, `a_private_literal_is_rejected_without_dialling`, `loopback_is_rejected_unless_the_hatch_is_set`, -`a_mapped_ipv6_metadata_literal_is_rejected_without_dialling`, and -`an_ipv6_loopback_literal_is_rejected_without_dialling` in +`a_mapped_ipv6_metadata_literal_is_rejected_without_dialling`, +`an_ipv6_loopback_literal_is_rejected_without_dialling`, and +`a_hostname_resolving_to_loopback_is_rejected_not_connect` in `crates/ppe/src/http_hyper.rs`. Each expects `HttpTransportError::Rejected` and `may_have_reached_peer() == false`. The connection-refused test now uses the hatch so it still exercises `Connect` on loopback. +`classify_walks_the_source_chain_for_the_egress_marker` pins the walk +without DNS. ### F3 — leg-2 IdP errors leaked bearer material @@ -104,17 +115,23 @@ violation. Reverting the sanitization makes those assertions fail. **Closed.** `TrustedIssuerConfig::validate` requires at least one audience unless `skip_audience_validation: true` is set. Setting both is refused. At verify, an emptied `audiences` field without the skip -flag is `NoAudiences` rather than `validate_aud = false`. +flag is `NoAudiences` rather than `validate_aud = false`. A configured +list also requires the token to carry an `aud` claim +(`set_required_spec_claims`); jsonwebtoken otherwise skips audience +checking when the claim is absent. **Breaking** for a config that listed no audiences. The operator-visible hatch is `skip_audience_validation: true`, which accepts a token minted -for any app (or none). +for any app (or none). Without the hatch, a missing `aud` is refused +the same as a mismatch (`auth.audience_mismatch`). Regression: `each_malformed_config_is_refused_at_load_with_a_message_naming_the_fault` covers omitted and empty lists; `empty_audience_list_rejects_the_token` -covers the public-field hole; `skip_audience_validation_accepts_a_token_minted_for_another_app` -pins the hatch. Removing the load check without the skip flag makes -those load tests build. +covers the public-field hole; `a_token_with_no_aud_claim_is_refused` +covers an omitted claim; `skip_audience_validation_accepts_a_token_minted_for_another_app` +and `skip_audience_validation_accepts_a_token_with_no_aud_claim` pin the +hatch. Removing the load check without the skip flag makes those load +tests build. ### F5 — `!=` on a missing attribute did not deny @@ -143,6 +160,10 @@ Regression: `non_numeric_amount_order_deny_fails_closed` and `args.amount > 10000: deny` must Deny, including under `!(...)`. A finite `"5000"` still Allows; a missing amount still Allows. Treating the comparison as false makes those assertions Allow. +Integers whose magnitude exceeds 2^53 are Unorderable on mixed +int/float (and on string-encoded integers), same as `NaN`: +`mixed_int_float_order_on_large_integers_is_fail_closed` and +`a_string_encoded_large_int_does_not_round_through_f64`. ### F7 — unreadable handler result was Allow