From 944114e788209c46c58deeed65ac40a428cc8fe8 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Wed, 2 Sep 2026 17:00:39 -0400 Subject: [PATCH 1/7] fix(apl): dispatch a glob route's policy body A name selector matches by glob and a route annotates its body under the pattern as written, but the lookup asked for the name the request arrived under. Those agree for an exact selector and diverge for a glob, so a route like `tool: "hr-*"` installed a handler and dispatched no policy: an operator wrote a deny and the request got an allow. Resolve the route and ask again under the name that matched, which is what the `http:` selector already did. Gated on the config declaring a glob, so an exact-only deployment pays nothing. Three tests pinned the old behavior as expected; they now assert the body runs, and the precedence and shadowing cases are covered beside them. Signed-off-by: Frederico Araujo --- CHANGELOG.md | 23 +++ crates/ppe-apl-runtime/src/visitor.rs | 27 ++-- .../ppe-apl-runtime/tests/http_route_e2e.rs | 149 +++++++++++++++++- crates/ppe-core/src/engine.rs | 100 ++++++++++++ 4 files changed, 279 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a64d2cb..db4f6462 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -818,6 +818,29 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Fixed +- **A glob route under `tool:` / `resource:` / `prompt:` / `llm:` evaluates its + policy body.** A name selector matches by glob, and a route annotates its + compiled body under the pattern as written, but the annotation lookup asked for + the name the request arrived under. Those agree for an exact selector and + diverge for a glob, so `tool: "hr-*"` carrying `authorization:` resolved for + everything else it declares, installed a handler, and dispatched no policy at + all. An operator wrote a deny and the request got an allow, with no diagnostic + either way. + + The lookup now resolves the route and asks again under the name that matched, + which is what the `http:` selector already did: `matched_http_name` exists so + that "the cache and the annotation table key on the config rather than on the + traffic", and the named path was the half still keying on the traffic. Gated on + the configuration declaring a glob, so a deployment writing only exact names and + lists pays neither the route walk nor a second pair of lookups, and the + resolution an `assertions:` contract already needed is reused where present. + + Specificity is unchanged, and two consequences of it are worth knowing. An + exact selector still outranks a glob that also matches, and it is found before + the route is resolved at all. And an exact route carrying no policy still + shadows a glob that has one, because the more specific route won and declares + nothing; that is the one shape where adding a route removes enforcement. + - **A bundle joined through both `meta.tags` and `groups:` no longer runs its `authentication:` steps twice.** Bundle membership is now deduplicated before authentication and assertion layers are resolved, keeping their inheritance diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index 4c545b31..2d0af503 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -2146,8 +2146,8 @@ routes: } /// A glob route under one of the four MCP selectors. The annotation is - /// installed under the pattern as written, and the lookup is exact - /// equality, so a request named by a glob never reaches the body. + /// installed under the pattern as written, so reaching the body means + /// resolving the request's name to that pattern first. const GLOB_TOOL_ROUTE: &str = r#" engine_settings: dispatch: policy @@ -2159,10 +2159,10 @@ routes: "#; #[tokio::test] - async fn a_glob_tool_route_still_does_not_evaluate_its_policy_body() { + async fn a_glob_tool_route_evaluates_its_policy_body() { let mgr = engine_with(GLOB_TOOL_ROUTE).await; - let (allowed, _bg) = mgr + let (denied, _bg) = mgr .invoke_named::( HOOK_CMF_TOOL_PRE_INVOKE, payload(), @@ -2171,25 +2171,24 @@ routes: ) .await; assert!( - allowed.continue_processing, - "a name the glob matches does not equal the pattern the handler is \ - installed under, so the body does not evaluate; violation = {:?}", - allowed.violation + !denied.continue_processing, + "the route denies and the name the glob covers is governed by it" ); - // The handler exists and its body denies, so the line above is the - // lookup and not a missing installation. - let (denied, _bg) = mgr + // A name the pattern does not cover resolves no route, so the body that + // denies is not reached and the request is not governed by it. + let (allowed, _bg) = mgr .invoke_named::( HOOK_CMF_TOOL_PRE_INVOKE, payload(), - tool_request("hr-*"), + tool_request("finance-close"), None, ) .await; assert!( - !denied.continue_processing, - "the body is installed under the pattern as written" + allowed.continue_processing, + "a name outside the pattern reaches no body; violation = {:?}", + allowed.violation ); } diff --git a/crates/ppe-apl-runtime/tests/http_route_e2e.rs b/crates/ppe-apl-runtime/tests/http_route_e2e.rs index 05d344d1..f20b1334 100644 --- a/crates/ppe-apl-runtime/tests/http_route_e2e.rs +++ b/crates/ppe-apl-runtime/tests/http_route_e2e.rs @@ -785,8 +785,13 @@ routes: /// the pattern as written, and the lookup is exact, so its policy body never /// evaluates. With the activation list gone there is no chain behind it either, /// so the route reaches nothing. +/// A glob selector annotates under the pattern it writes, and a request arrives +/// under a name of its own, so finding the body means resolving the route first. +/// This used to dispatch nothing: the route resolved for everything else it +/// declared and its policy never ran, which is a deny an operator wrote and an +/// allow the request got. #[tokio::test] -async fn a_glob_entity_routes_body_never_evaluates_and_nothing_replaces_it() { +async fn a_glob_entity_route_dispatches_its_body() { const YAML: &str = r#" engine_settings: dispatch: policy @@ -802,11 +807,139 @@ routes: "#; let (mgr, ledger) = engine_with(YAML).await; + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-get-salary")).await); + assert_eq!( + fired(&ledger), + vec!["body-audit".to_owned()], + "the pattern the route writes is what its annotation is keyed on, so the \ + request's own name has to resolve to that pattern before the body is found" + ); +} + +/// Two names under one pattern reach the same compiled body, the way two paths +/// under one `path_prefix` do. +#[tokio::test] +async fn many_names_under_one_glob_share_its_body() { + const YAML: &str = r#" +engine_settings: + dispatch: policy +plugins: + - name: body-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] +routes: + - tool: "hr-*" + authorization: + pre_invocation: + - "run(body-audit)" +"#; + let (mgr, ledger) = engine_with(YAML).await; + + for name in ["hr-get-salary", "hr-adjust-comp"] { + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request(name)).await); + } + assert_eq!( + fired(&ledger), + vec!["body-audit".to_owned(), "body-audit".to_owned()], + "every name the pattern covers reaches the one body it wrote" + ); +} + +/// A name the pattern does not cover resolves no route and reaches no body. +#[tokio::test] +async fn a_name_outside_the_glob_reaches_no_body() { + const YAML: &str = r#" +engine_settings: + dispatch: policy +plugins: + - name: body-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] +routes: + - tool: "hr-*" + authorization: + pre_invocation: + - "run(body-audit)" +"#; + let (mgr, ledger) = engine_with(YAML).await; + + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("finance-close")).await); + assert!( + fired(&ledger).is_empty(), + "resolving the route is what finds a glob's body, and this name resolves none" + ); +} + +/// An exact selector outranks a glob that also matches, so the exact route's +/// body is the one that runs. +#[tokio::test] +async fn an_exact_entity_route_outranks_a_glob_that_also_matches() { + const YAML: &str = r#" +engine_settings: + dispatch: policy +plugins: + - name: glob-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] + - name: exact-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] +routes: + - tool: "hr-*" + authorization: + pre_invocation: + - "run(glob-audit)" + - tool: hr-get-salary + authorization: + pre_invocation: + - "run(exact-audit)" +"#; + let (mgr, ledger) = engine_with(YAML).await; + + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-get-salary")).await); + assert_eq!( + fired(&ledger), + vec!["exact-audit".to_owned()], + "the exact name is looked up before the route is resolved at all, so the \ + glob never gets the chance to answer for a name spelled out beside it" + ); + + clear(&ledger); + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-adjust-comp")).await); + assert_eq!( + fired(&ledger), + vec!["glob-audit".to_owned()], + "a name only the pattern covers still reaches the pattern's body" + ); +} + +/// An exact route carrying no policy shadows a glob that would have governed the +/// name. Worth pinning: it is the one shape where adding a route removes +/// enforcement, and it follows from specificity rather than from anything the +/// annotation lookup does. +#[tokio::test] +async fn an_exact_route_with_no_body_shadows_a_glob_that_has_one() { + const YAML: &str = r#" +engine_settings: + dispatch: policy +plugins: + - name: glob-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] +routes: + - tool: "hr-*" + authorization: + pre_invocation: + - "run(glob-audit)" + - tool: hr-get-salary +"#; + let (mgr, ledger) = engine_with(YAML).await; + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-get-salary")).await); assert!( fired(&ledger).is_empty(), - "the glob's annotation is keyed on the pattern, so an exact lookup for \ - `hr-get-salary` finds nothing and nothing else activates a plugin" + "the exact route wins on specificity and declares no policy, so the glob's \ + body does not stand in for it" ); } @@ -1517,9 +1650,13 @@ routes: tool_request("get_weather"), "tool-audit", ), - // The glob's annotation is keyed on the pattern as written and the - // lookup is exact, so nothing answers for a name the glob covers. - ("cmf.tool_pre_invoke", tool_request("hr-get-salary"), ""), + // The glob's annotation is keyed on the pattern as written, and the + // name resolves to that pattern before the lookup, so its body runs. + ( + "cmf.tool_pre_invoke", + tool_request("hr-get-salary"), + "glob-audit", + ), ( "cmf.resource_pre_fetch", entity_request("resource", "file:///data.csv"), diff --git a/crates/ppe-core/src/engine.rs b/crates/ppe-core/src/engine.rs index 8cacf645..15151802 100644 --- a/crates/ppe-core/src/engine.rs +++ b/crates/ppe-core/src/engine.rs @@ -160,6 +160,42 @@ fn declares_assertions(config: &PolicyConfig) -> bool { || config.routes.iter().any(|route| route.assertions.is_some()) } +/// Whether any route selects a named entity with a glob pattern. +/// +/// The annotation table is keyed on the name the configuration writes, and for +/// the four named entity types a request arrives under a name of its own. Those +/// two agree for an exact selector and diverge for a glob, so a glob route needs +/// its route resolved before its annotation can be found. Resolving is a route +/// table walk, and this is what keeps it off every deployment that writes no +/// glob, which is the ordinary one. +/// +/// A list selector matches by equality, so it is not a glob: `matched_selector_name` +/// returns the element that matched and that is the name the annotation carries. +fn declares_glob_named_routes(config: &PolicyConfig) -> bool { + config.routes.iter().any(|route| { + [ + route.tool.as_ref(), + route.resource.as_ref(), + route.prompt.as_ref(), + route.llm.as_ref(), + ] + .into_iter() + .flatten() + .any(is_glob_selector) + }) +} + +/// Whether a name selector can match a name it is not equal to. +/// +/// Only the single-pattern shape can: `wildmatch` reads `*` and `?` as +/// metacharacters, and a list matches by equality. +fn is_glob_selector(selector: &config::StringOrList) -> bool { + match selector { + config::StringOrList::Single(pattern) => pattern.as_str().contains(['*', '?']), + config::StringOrList::List(_) => false, + } +} + /// Turn a refused assertion into a denial, keeping what the pipeline recorded. /// /// `PipelineResult::denied` constructs with no errors and no metadata, so a bare @@ -493,6 +529,11 @@ struct RuntimeSnapshot { /// under. Read on the generic-HTTP path when a request carries no readable /// path, for the same reason its authentication counterpart is. http_routes_declaring_assertions: Arc<[String]>, + + /// Whether any route selects a named entity with a glob. False for every + /// config that writes only exact names and lists, which is what keeps the + /// route resolution a glob annotation needs out of those deployments. + declares_glob_named_routes: bool, } /// Composite key for route annotations. Includes the hook name so a single @@ -823,6 +864,7 @@ fn snapshot_from_config(registry: PluginRegistry, policy_config: PolicyConfig) - let http_routes_declaring_authentication = http_routes_declaring_authentication(&policy_config); let declares_assertions = declares_assertions(&policy_config); let http_routes_declaring_assertions = http_routes_declaring_assertions(&policy_config); + let declares_glob_named_routes = declares_glob_named_routes(&policy_config); RuntimeSnapshot { registry, executor, @@ -832,6 +874,7 @@ fn snapshot_from_config(registry: PluginRegistry, policy_config: PolicyConfig) - http_routes_declaring_authentication, declares_assertions, http_routes_declaring_assertions, + declares_glob_named_routes, } } @@ -848,6 +891,7 @@ impl PolicyEngine { http_routes_declaring_authentication: Arc::from(Vec::new()), declares_assertions: false, http_routes_declaring_assertions: Arc::from(Vec::new()), + declares_glob_named_routes: false, }; Self { runtime: arc_swap::ArcSwap::from_pointee(snapshot), @@ -2495,6 +2539,62 @@ impl PolicyEngine { hook_name: hook_name.to_owned(), }) }); + // A glob selector annotates under the pattern it writes, and the + // two lookups above ask for the name the request arrived under, so + // neither can find it. Resolve the route and ask again under the + // name that matched. Without this a route like `tool: get_*` + // carrying `authorization:` resolved for everything else it + // declares and dispatched no policy at all, which is a deny an + // operator wrote and an allow the request got. + // + // Gated on the config declaring a glob, so a deployment writing + // only exact names and lists pays neither the walk nor a second + // pair of lookups. `early_named` is the same resolution, already + // done when an `assertions:` contract needed it. + let glob_matched = if candidate.is_none() && snapshot.declares_glob_named_routes { + early_named.as_ref().map_or_else( + || { + routing_config.and_then(|policy_config| { + config::resolve_route( + policy_config, + config::RouteQuery::named(et, en).with_scope(request_scope), + ) + }) + }, + |matched| { + Some(config::MatchedRoute { + route: matched.route, + name: matched.name.clone(), + }) + }, + ) + } else { + None + }; + // Only when the route resolved to a name other than the request's: + // an equal name is what the two lookups above already asked for. + let candidate = candidate.or_else(|| { + let matched = glob_matched.as_ref()?; + if matched.name == en { + return None; + } + let scoped = request_scope.and_then(|s| { + snapshot.route_annotations.get(&AnnotationKey { + entity_type: et.to_owned(), + entity_name: matched.name.clone(), + scope: Some(s.to_owned()), + hook_name: hook_name.to_owned(), + }) + }); + scoped.or_else(|| { + snapshot.route_annotations.get(&AnnotationKey { + entity_type: et.to_owned(), + entity_name: matched.name.clone(), + scope: None, + hook_name: hook_name.to_owned(), + }) + }) + }); if let Some(entry) = candidate { return Ok(( Arc::new(vec![entry.clone()]), From 89739697ee09eff9dde28c39bcac3a26b1ff6378 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Wed, 2 Sep 2026 17:02:08 -0400 Subject: [PATCH 2/7] docs: complete the host half of the upgrade guide The Rust API section is written for a host embedding the engine, and it named one of the four changes that break one. Add the HTTP hook family move, the Subject::claims shape change, and the HttpTransport a host must install, which is the only one of the four a clean build does not catch. Its perform_http half reaches a configuration rather than a host, so the introduction points a config-only reader at it. Signed-off-by: Frederico Araujo --- CHANGELOG.md | 12 ++++++ docs/upgrade-apl.md | 96 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index db4f6462..bd553264 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -818,6 +818,18 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Fixed +- **`docs/upgrade-apl.md` documents the host half of the release.** Its "Rust API + changes" section is written for a host embedding the engine, and it named one of + the four changes that actually break one: the `routing_enabled()` rename. It now + also carries the generic-HTTP hook family move (`HOOK_CMF_HTTP_REQUEST` to + `http_hook::HOOK_HTTP_REQUEST`, dispatched through `HttpHook` with an + `HttpPayload`), the `Subject::claims` change from `String` to `Value` with the + flattening a host that wants the old shape writes, and the `HttpTransport` a host + must install. The transport is the one that matters most there, because it is the + only break in the section a clean build does not catch: it surfaces at + `PolicyEngine::initialize()`. Its `perform_http` half reaches a configuration + rather than a host, so the guide's introduction points a config-only reader at it. + - **A glob route under `tool:` / `resource:` / `prompt:` / `llm:` evaluates its policy body.** A name selector matches by glob, and a route annotates its compiled body under the pattern as written, but the annotation lookup asked for diff --git a/docs/upgrade-apl.md b/docs/upgrade-apl.md index 9f9d9809..a9e1bc63 100644 --- a/docs/upgrade-apl.md +++ b/docs/upgrade-apl.md @@ -12,6 +12,11 @@ means every key in it does something. Work through the sections in order. The first two change the shape of the document, and the rest are local rewrites. +One item in section 10 reaches a configuration even though the section is written +for a host: a plugin that fetches a JWKS, exchanges a token, or dispatches a CIBA +prompt must now declare the `perform_http` capability. Read that subsection even +if you write no Rust. + --- ## 1. `engine_settings:`, and the dispatch mode @@ -428,6 +433,97 @@ For a host embedding the engine rather than only writing configuration. | `GlobalConfig.identity`, `PolicyGroup.identity`, `RouteEntry.identity` | `.authentication` | | `compile_config`, `ConfigYaml`, `CompiledConfig` | deleted; see below | | `ParseError::RenamedField`, `::ConflictingAuthorizationForms` | deleted | +| `cmf::constants::HOOK_CMF_HTTP_REQUEST` | `http_hook::HOOK_HTTP_REQUEST` | +| `cmf::constants::HOOK_CMF_HTTP_RESPONSE` | `http_hook::HOOK_HTTP_RESPONSE` | +| `invoke_named::` on either HTTP hook | `invoke_named::`, with `HttpPayload` | +| `HookHandler` for an HTTP hook | `HookHandler` | +| `HookFamily::for_entity -> HookFamily` | `-> Option` | +| `Subject::claims: HashMap` | `HashMap` | + +### The generic-HTTP hooks moved families + +Both names and both constants moved, and so did the type a handler is written +against. The two names are `http.request` and `http.response`, the constants live +in `praxis_policy_core::http_hook`, and the payload is `HttpPayload`, which +carries no fields: + +```rust +// before +use praxis_policy_core::cmf::constants::HOOK_CMF_HTTP_REQUEST; +let payload = MessagePayload { message: Message::text(Role::User, "") }; +mgr.invoke_named::(HOOK_CMF_HTTP_REQUEST, payload, ext, None).await; +``` + +```rust +// after +use praxis_policy_core::http_hook::{HOOK_HTTP_REQUEST, HttpHook, HttpPayload}; +mgr.invoke_named::(HOOK_HTTP_REQUEST, HttpPayload, ext, None).await; +``` + +The placeholder message is what the split removes. Nothing on the HTTP path +filled it, so a content-inspecting plugin registered on an HTTP hook scanned a +fabricated message and reported clean, and an always-passing scanner is worse +than no scanner. A `hooks:` entry in YAML naming `cmf.http_request` or +`cmf.http_response` fails the load and names the replacement. + +### Subject claims keep their JSON shape + +`Subject::claims` is `HashMap`. It was `HashMap`, +where a structured claim had already been flattened for you. + +A host that wants the old flat strings does the flattening itself, and wants +`as_str()` on the string arm rather than `to_string()`, or every string claim +arrives quoted: + +```rust +let flat = value.as_str().map_or_else(|| value.to_string(), str::to_owned); +``` + +### A host must install an HTTP transport + +**This one compiles.** It is the only change in this section a clean build does +not catch, so read it even if your build is green. + +PPE performs no outbound HTTP of its own. `identity-jwt`, `delegator-oauth` and +`elicitation-ciba` borrow a transport the host installs, and with none installed +a `jwks_url` issuer fails at `PolicyEngine::initialize()` rather than at load. + +A host that already has an HTTP stack should lend that one, so the process keeps +one connection pool, one TLS trust store, and one egress path: + +```rust +mgr.set_http_transport(Arc::new(MyTransport::new())); +``` + +A host without one enables the non-default `http-hyper` feature on +`praxis-policy` and installs the bundled implementation. It is deliberately not +folded into `install_builtins`, so wiring an egress path stays one explicit line: + +```rust +praxis_policy::install_builtins(&mgr); +praxis_policy::install_default_http_transport(&mgr); +``` + +The bundled transport builds its pool on first use, so installing it from a +short-lived initialization runtime is safe. + +**Breaking for existing configuration too**, and it is checked at +initialization: a plugin using `jwks_url`, an OAuth delegator, or a CIBA approver +must declare the `perform_http` capability. + +```yaml +plugins: + - name: jwt-user + kind: identity/jwt + capabilities: + - perform_http +``` + +Withholding it stops the call rather than degrading it, because a plugin that +quietly skipped its `IdP` call would fail open. The two failures report +separately: `ServiceError::NotInstalled` says the embedding host installed no +transport, which is a wiring problem, and `NotPermitted` names the capability to +add, which is a configuration one. `Phase` and `CompiledRoute` both derive `Serialize`, so **the serialized keys change too**: a phase serializes as `pre_invocation` / `post_invocation`, and a From 141dc1224fc4d48430be22d4c895b66d08746d53 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Wed, 2 Sep 2026 17:25:52 -0400 Subject: [PATCH 3/7] fix(http): name the crypto provider the transport builds with with_webpki_roots() builds its ClientConfig through rustls::ClientConfig::builder(), which reads the process-level CryptoProvider, and the host's dependency graph is what sets that. A graph carrying both ring and aws-lc-rs has none, so rustls panics on the first connection instead of choosing. That is praxis: pingora and its TLS stack pull aws-lc-rs while this transport pulls ring, so install_default_http_transport worked standalone and panicked in the gateway on the first JWKS fetch. Build the config against ring explicitly rather than installing a process default, which would race a host installing its own. client() is fallible as a result, since naming a provider means asking it for protocol versions. Signed-off-by: Frederico Araujo --- CHANGELOG.md | 20 ++++++++ Cargo.lock | 2 + crates/ppe/Cargo.toml | 12 +++++ crates/ppe/src/http_hyper.rs | 99 ++++++++++++++++++++++++++++++++---- 4 files changed, 123 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd553264..ea4b7dad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -830,6 +830,26 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). `PolicyEngine::initialize()`. Its `perform_http` half reaches a configuration rather than a host, so the guide's introduction points a config-only reader at it. +- **The bundled hyper transport names its crypto provider instead of reading the + process default.** `HttpsConnectorBuilder::with_webpki_roots()` builds its + `ClientConfig` through `rustls::ClientConfig::builder()`, which reads the + process-level `CryptoProvider`, and a host's own dependency graph is what sets + that. A graph carrying both `ring` and `aws-lc-rs` has no unambiguous default, + so rustls panicked on the first connection rather than choosing. Praxis is + exactly that graph: pingora and its TLS stack pull `aws-lc-rs` while this + transport pulls `ring`, so `install_default_http_transport` worked standalone + and panicked inside the gateway, on the first JWKS fetch. + + The transport now builds its configuration against `ring` explicitly, so the + decision stays inside it. Installing a process default instead would reach + outside the transport and could lose a race with a host installing its own; a + test asserts that none is installed as a side effect. `HyperTransport::client` + is fallible as a result, since naming a provider means asking it for protocol + versions, and a TLS configuration failure now reports as + `HttpTransportError::Connect` rather than unwrapping. `rustls` and + `webpki-roots` become direct dependencies under `http-hyper`; both were already + in the graph through `hyper-rustls`. + - **A glob route under `tool:` / `resource:` / `prompt:` / `llm:` evaluates its policy body.** A name selector matches by glob, and a route annotates its compiled body under the pattern as written, but the annotation lookup asked for diff --git a/Cargo.lock b/Cargo.lock index 670fa19a..795882de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2379,7 +2379,9 @@ dependencies = [ "praxis-policy-plugin-elicitation-ciba", "praxis-policy-plugin-identity-jwt", "praxis-policy-session-valkey", + "rustls", "tokio", + "webpki-roots", ] [[package]] diff --git a/crates/ppe/Cargo.toml b/crates/ppe/Cargo.toml index 502401e4..4e823df4 100644 --- a/crates/ppe/Cargo.toml +++ b/crates/ppe/Cargo.toml @@ -78,6 +78,8 @@ valkey = ["_builtin", "dep:praxis-policy-session-valkey"] http-hyper = [ "dep:hyper-util", "dep:hyper-rustls", + "dep:rustls", + "dep:webpki-roots", "dep:http-body-util", "dep:bytes", "dep:http", @@ -136,6 +138,16 @@ hyper-rustls = { version = "0.27", optional = true, default-features = false, fe "tls12", "webpki-roots", ] } +# Named directly so the transport can build its `ClientConfig` against an +# explicit crypto provider rather than the process-level default. A host's own +# graph decides that default, and a graph carrying both `ring` and `aws-lc-rs` +# has none: rustls refuses to choose and panics on first use. Praxis is exactly +# that graph, since pingora and its own TLS stack pull `aws-lc-rs`. +rustls = { version = "0.23", optional = true, default-features = false, features = [ + "ring", + "tls12", +] } +webpki-roots = { version = "1", optional = true } http-body-util = { version = "0.1", optional = true } bytes = { workspace = true, optional = true } http = { workspace = true, optional = true } diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index f5bcac19..81ebcbeb 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -189,8 +189,28 @@ impl HyperTransport { } /// The shared client, built on first call. - fn client(&self) -> &HyperClient { - self.client.get_or_init(|| { + /// + /// Fallible because building the TLS configuration is: naming a crypto + /// provider means asking it for protocol versions, and that answer is a + /// `Result`. Returning it beats unwrapping, which is the panic this whole + /// path exists to remove. + fn client(&self) -> Result<&HyperClient, HttpTransportError> { + if let Some(client) = self.client.get() { + return Ok(client); + } + let built = Self::build_client(self)?; + // A racing caller may have won `set`; either client is equivalent, and + // `get_or_init`'s own contract is the same. Read back whichever landed + // so every caller shares one pool. + let _ = self.client.set(built); + self.client + .get() + .ok_or_else(|| HttpTransportError::Connect("HTTP client init raced".to_owned())) + } + + /// Build the pooling client. Called once per transport, from `client`. + fn build_client(&self) -> Result { + { let mut http = HttpConnector::new(); // The HTTPS connector wraps this one, so it must accept the // `https` scheme rather than rejecting it as non-HTTP. @@ -204,11 +224,37 @@ impl HyperTransport { http.set_nodelay(true); http.set_keepalive(self.tcp_keepalive); + // An explicit provider, not the process-level default. + // + // `with_webpki_roots()` builds its `ClientConfig` through + // `rustls::ClientConfig::builder()`, which reads the default + // provider, and a host's own dependency graph is what sets + // that. A graph carrying both `ring` and `aws-lc-rs` has no + // unambiguous default, so rustls panics on the first + // connection rather than choosing; Praxis is exactly that + // graph, because pingora and its TLS stack pull `aws-lc-rs` + // while this transport pulls `ring`. + // + // Naming the provider here keeps that decision inside the + // transport. Installing a process default instead would + // reach outside it and could lose a race with a host + // installing its own. + // + // Webpki roots rather than the system store, matching what + // the reqwest path resolved to and keeping the trust set + // identical across every deployment. + let mut roots = rustls::RootCertStore::empty(); + roots.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + let tls_config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new( + rustls::crypto::ring::default_provider(), + )) + .with_safe_default_protocol_versions() + .map_err(|e| HttpTransportError::Connect(format!("rustls client configuration: {e}")))? + .with_root_certificates(roots) + .with_no_client_auth(); + let tls = hyper_rustls::HttpsConnectorBuilder::new() - // Webpki roots rather than the system store, matching - // what the reqwest path resolved to and keeping the - // trust set identical across every deployment. - .with_webpki_roots() + .with_tls_config(tls_config) // `https_or_http`, not `https_only`: `identity-jwt` // supports an explicit `insecure_http: true` for local // development, and it already refuses plaintext by @@ -226,7 +272,7 @@ impl HyperTransport { tls.enable_http1().wrap_connector(http) }; - Client::builder(TokioExecutor::new()) + let client = Client::builder(TokioExecutor::new()) // Without a timer, `pool_idle_timeout` silently does // nothing and idle connections are never evicted. With // `pool_max_idle_per_host` defaulting to unlimited, that @@ -235,8 +281,10 @@ impl HyperTransport { .pool_timer(TokioTimer::new()) .pool_idle_timeout(self.pool_idle_timeout) .pool_max_idle_per_host(self.pool_max_idle_per_host) - .build(https) - }) + .build(https); + + Ok(client) + } } } @@ -285,7 +333,7 @@ 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 client = self.client()?; let limit = req.max_response_bytes; // The deadline covers the *whole* exchange, headers and body. @@ -408,6 +456,37 @@ mod tests { assert!(t.client.get().is_some(), "first use must build the pool"); } + #[tokio::test] + async fn the_pool_builds_with_no_process_default_crypto_provider() { + // The graph guard, and the reason the TLS configuration names its + // provider. `ClientConfig::builder()` reads the process-level default, + // and a host's own dependency graph is what sets that: a graph carrying + // both `ring` and `aws-lc-rs` has no unambiguous default, so rustls + // panicked on the first connection rather than choosing. Praxis is + // exactly that graph. + // + // This test process installs no default, which is the same condition + // from the other direction: if the connector went back to reading it, + // building the pool would panic here. + assert!( + rustls::crypto::CryptoProvider::get_default().is_none(), + "the guard only means something while nothing has installed a default" + ); + let t = HyperTransport::new(); + // A closed port, so this reaches the connector and stops there. What is + // under test is that building it produced a client at all. + let _ = t.execute(HttpRequest::get("https://127.0.0.1:1/x")).await; + assert!( + t.client.get().is_some(), + "the pool must build against the named provider" + ); + assert!( + rustls::crypto::CryptoProvider::get_default().is_none(), + "naming a provider must not install one process-wide, which would \ + race a host installing its own" + ); + } + #[test] fn the_defaults_bound_the_pool_and_disable_nagle() { // These three are the difference between "works in a demo" and From a2b5921b24992027e2ce0688a0e382206138f7c1 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Wed, 2 Sep 2026 21:15:52 -0400 Subject: [PATCH 4/7] fix(apl): credit delegation and elicitation to their own hooks The per-hook reachability tally credited every plugin a route reaches with that route's CMF hook pair. A delegate step invokes its plugin under token.delegate and an elicitation verb under elicit, whatever entity the route selects, so a delegator came out covered on cmf.tool_pre_invoke and uncovered on the one hook it declares. plugin_narrowed_by_policy then fired on every config that delegates, which is most of them. The three demo policies raised four warnings and one was real. The plugin-level tally was already right, so this was noise rather than a load failure. The report still fires for a delegator declaring a CMF hook nothing reaches. Signed-off-by: Frederico Araujo --- CHANGELOG.md | 16 ++++ crates/ppe-apl-runtime/src/dispatch_plan.rs | 32 ++++++++ crates/ppe-apl-runtime/src/visitor.rs | 20 +++++ .../tests/dispatch_mode_e2e.rs | 77 +++++++++++++++++++ 4 files changed, 145 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea4b7dad..38e1cae6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -830,6 +830,22 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). `PolicyEngine::initialize()`. Its `perform_http` half reaches a configuration rather than a host, so the guide's introduction points a config-only reader at it. +- **A delegator or an elicitation handler is no longer reported as narrowed.** The + per-hook reachability tally credited every plugin a route reaches with that + route's CMF hook pair, but a `delegate(...)` step invokes its plugin under + `token.delegate` and an elicitation verb invokes its handler under `elicit`, + whichever entity the route selects. So a delegator declaring `token.delegate` + came out covered on `cmf.tool_pre_invoke` and uncovered on the one hook it + actually has, and `plugin_narrowed_by_policy` fired on every configuration that + delegates or elicits. An alarm that always fires is one nobody reads, and this + one exists to name a real coverage gap. + + The two families are now credited under their own hook. The plugin-level tally + was already right, so this was never a load failure, only noise: the three demo + policies raised four of these warnings between them and one was real. The report + still fires for a delegator that declares a CMF hook nothing reaches, so the + alarm was narrowed rather than muted. + - **The bundled hyper transport names its crypto provider instead of reading the process default.** `HttpsConnectorBuilder::with_webpki_roots()` builds its `ClientConfig` through `rustls::ClientConfig::builder()`, which reads the diff --git a/crates/ppe-apl-runtime/src/dispatch_plan.rs b/crates/ppe-apl-runtime/src/dispatch_plan.rs index f5999fd0..41705c2e 100644 --- a/crates/ppe-apl-runtime/src/dispatch_plan.rs +++ b/crates/ppe-apl-runtime/src/dispatch_plan.rs @@ -637,6 +637,38 @@ pub(crate) fn collect_plugin_names_by_half(route: &CompiledRoute) -> (Vec Vec<(String, &'static str)> { + let mut out: Vec<(String, &'static str)> = Vec::new(); + let mut visit = |e: &Effect| { + let pair = match e { + Effect::Delegate(ds) => (ds.plugin_name.clone(), HOOK_TOKEN_DELEGATE), + Effect::Elicit(es) => (es.plugin_name.clone(), HOOK_ELICIT), + _ => return, + }; + if !out.contains(&pair) { + out.push(pair); + } + }; + walk_effects(&route.pre_invocation, &mut visit); + walk_effects(&route.post_invocation, &mut visit); + out +} + /// Compute the union of capabilities declared by every plugin a /// `CompiledRoute` can dispatch to (with per-route overrides applied). /// diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index 2d0af503..9e50cf8e 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -518,13 +518,25 @@ impl AplConfigVisitor { /// routes below it. fn record_reached_plugins(&self, route: &CompiledRoute, hook_pre: &str, hook_post: &str) { let (pre, post) = crate::dispatch_plan::collect_plugin_names_by_half(route); + // A delegator and an elicitation handler dispatch under their own + // family's hook, not the route's entity pair, so they are credited + // separately below. Crediting them with the CMF hook made their declared + // `token.delegate` / `elicit` read as uncovered, and the narrowing report + // then fired on every configuration that delegates. + let family_fixed = crate::dispatch_plan::collect_family_fixed_plugin_hooks(route); let mut state = self .state .write() .unwrap_or_else(std::sync::PoisonError::into_inner); + let family_fixed_names: std::collections::HashSet<&str> = + family_fixed.iter().map(|(name, _)| name.as_str()).collect(); for (names, hook) in [(pre, hook_pre), (post, hook_post)] { for name in names { state.reached_plugin_names.insert(name.clone()); + if family_fixed_names.contains(name.as_str()) { + // Reached, but on its own family's hook. Recorded below. + continue; + } state .reached_plugin_hooks .entry(name) @@ -532,6 +544,14 @@ impl AplConfigVisitor { .insert(hook.to_owned()); } } + for (name, hook) in family_fixed { + state.reached_plugin_names.insert(name.clone()); + state + .reached_plugin_hooks + .entry(name) + .or_default() + .insert(hook.to_owned()); + } } /// Tally the plugins a layer's steps name, without a hook. diff --git a/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs b/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs index a52340fa..a7ba5ff4 100644 --- a/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs +++ b/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs @@ -556,6 +556,83 @@ routes: ); } +/// A delegator reached by a `delegate(...)` step is not narrowed. +/// +/// Its hook is `token.delegate`, fixed by its own family rather than by the +/// entity the route selects. The tally credited every reached plugin with the +/// route's CMF hook pair, so a delegator came out covered on +/// `cmf.tool_pre_invoke` and uncovered on the one hook it actually declares. +/// That fired on every configuration that delegates, which is most of them, and +/// an alarm that always fires is one nobody reads. +#[test] +fn a_delegator_reached_by_a_delegate_step_is_not_reported_as_narrowed() { + let alarms = alarms_raised_by_loading( + " +plugins: + - name: workday-oauth + kind: builtin + hooks: [token.delegate] +routes: + - tool: get_compensation + authorization: + pre_invocation: + - \"delegate(workday-oauth, target: workday-api, audience: workday-api)\" +", + ); + assert!( + !alarms.contains(&NARROWED.to_owned()), + "the step reaches it under `token.delegate`, which is the hook it \ + declares, so there is nothing uncovered: {alarms:?}" + ); +} + +/// The same for an elicitation handler, whose hook is `elicit`. +#[test] +fn an_elicitation_handler_reached_by_a_verb_is_not_reported_as_narrowed() { + let alarms = alarms_raised_by_loading( + " +plugins: + - name: manager-approver + kind: builtin + hooks: [elicit] +routes: + - tool: adjust_compensation + authorization: + pre_invocation: + - \"require_approval(manager-approver, from: claim.manager, channel: \\\"ciba\\\")\" +", + ); + assert!( + !alarms.contains(&NARROWED.to_owned()), + "the verb reaches it under `elicit`, which is the hook it declares: \ + {alarms:?}" + ); +} + +/// And the report still fires for a delegator that genuinely declares a hook +/// nothing reaches, so the fix above narrowed the alarm rather than muting it. +#[test] +fn a_delegator_declaring_an_unreached_cmf_hook_is_still_reported() { + let alarms = alarms_raised_by_loading( + " +plugins: + - name: workday-oauth + kind: builtin + hooks: [token.delegate, cmf.tool_post_invoke] +routes: + - tool: get_compensation + authorization: + pre_invocation: + - \"delegate(workday-oauth, target: workday-api, audience: workday-api)\" +", + ); + assert!( + alarms.contains(&NARROWED.to_owned()), + "`cmf.tool_post_invoke` is declared and no step reaches it there: \ + {alarms:?}" + ); +} + // ---- the core-side backstop ------------------------------------------- /// A host that registers no orchestrator gets the flipped default with no From 4017c38f7ced29c83be90cb9cf6d90dc2313f3c6 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Thu, 3 Sep 2026 09:10:54 -0400 Subject: [PATCH 5/7] docs: tighten comments across the four fixes Comments, doc prose and assertion messages only. No logic, no dependency or feature changes. Signed-off-by: Frederico Araujo --- CHANGELOG.md | 84 ++++--------------- crates/ppe-apl-runtime/src/dispatch_plan.rs | 15 +--- crates/ppe-apl-runtime/src/visitor.rs | 14 +--- .../tests/dispatch_mode_e2e.rs | 20 ++--- .../ppe-apl-runtime/tests/http_route_e2e.rs | 40 +++------ crates/ppe-core/src/engine.rs | 38 ++------- crates/ppe/Cargo.toml | 7 +- crates/ppe/src/http_hyper.rs | 49 ++--------- docs/upgrade-apl.md | 59 +++++-------- 9 files changed, 71 insertions(+), 255 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e1cae6..92e61dfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -818,76 +818,20 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Fixed -- **`docs/upgrade-apl.md` documents the host half of the release.** Its "Rust API - changes" section is written for a host embedding the engine, and it named one of - the four changes that actually break one: the `routing_enabled()` rename. It now - also carries the generic-HTTP hook family move (`HOOK_CMF_HTTP_REQUEST` to - `http_hook::HOOK_HTTP_REQUEST`, dispatched through `HttpHook` with an - `HttpPayload`), the `Subject::claims` change from `String` to `Value` with the - flattening a host that wants the old shape writes, and the `HttpTransport` a host - must install. The transport is the one that matters most there, because it is the - only break in the section a clean build does not catch: it surfaces at - `PolicyEngine::initialize()`. Its `perform_http` half reaches a configuration - rather than a host, so the guide's introduction points a config-only reader at it. - -- **A delegator or an elicitation handler is no longer reported as narrowed.** The - per-hook reachability tally credited every plugin a route reaches with that - route's CMF hook pair, but a `delegate(...)` step invokes its plugin under - `token.delegate` and an elicitation verb invokes its handler under `elicit`, - whichever entity the route selects. So a delegator declaring `token.delegate` - came out covered on `cmf.tool_pre_invoke` and uncovered on the one hook it - actually has, and `plugin_narrowed_by_policy` fired on every configuration that - delegates or elicits. An alarm that always fires is one nobody reads, and this - one exists to name a real coverage gap. - - The two families are now credited under their own hook. The plugin-level tally - was already right, so this was never a load failure, only noise: the three demo - policies raised four of these warnings between them and one was real. The report - still fires for a delegator that declares a CMF hook nothing reaches, so the - alarm was narrowed rather than muted. - -- **The bundled hyper transport names its crypto provider instead of reading the - process default.** `HttpsConnectorBuilder::with_webpki_roots()` builds its - `ClientConfig` through `rustls::ClientConfig::builder()`, which reads the - process-level `CryptoProvider`, and a host's own dependency graph is what sets - that. A graph carrying both `ring` and `aws-lc-rs` has no unambiguous default, - so rustls panicked on the first connection rather than choosing. Praxis is - exactly that graph: pingora and its TLS stack pull `aws-lc-rs` while this - transport pulls `ring`, so `install_default_http_transport` worked standalone - and panicked inside the gateway, on the first JWKS fetch. - - The transport now builds its configuration against `ring` explicitly, so the - decision stays inside it. Installing a process default instead would reach - outside the transport and could lose a race with a host installing its own; a - test asserts that none is installed as a side effect. `HyperTransport::client` - is fallible as a result, since naming a provider means asking it for protocol - versions, and a TLS configuration failure now reports as - `HttpTransportError::Connect` rather than unwrapping. `rustls` and - `webpki-roots` become direct dependencies under `http-hyper`; both were already - in the graph through `hyper-rustls`. - -- **A glob route under `tool:` / `resource:` / `prompt:` / `llm:` evaluates its - policy body.** A name selector matches by glob, and a route annotates its - compiled body under the pattern as written, but the annotation lookup asked for - the name the request arrived under. Those agree for an exact selector and - diverge for a glob, so `tool: "hr-*"` carrying `authorization:` resolved for - everything else it declares, installed a handler, and dispatched no policy at - all. An operator wrote a deny and the request got an allow, with no diagnostic - either way. - - The lookup now resolves the route and asks again under the name that matched, - which is what the `http:` selector already did: `matched_http_name` exists so - that "the cache and the annotation table key on the config rather than on the - traffic", and the named path was the half still keying on the traffic. Gated on - the configuration declaring a glob, so a deployment writing only exact names and - lists pays neither the route walk nor a second pair of lookups, and the - resolution an `assertions:` contract already needed is reused where present. - - Specificity is unchanged, and two consequences of it are worth knowing. An - exact selector still outranks a glob that also matches, and it is found before - the route is resolved at all. And an exact route carrying no policy still - shadows a glob that has one, because the more specific route won and declares - nothing; that is the one shape where adding a route removes enforcement. +- **Delegators and elicitation handlers are no longer reported as narrowed when + their family-specific hooks are reached.** `delegate(...)` is credited to + `token.delegate`, and elicitation verbs to `elicit`. Unreached hooks declared by + those plugins are still reported. + +- **The bundled hyper transport selects `ring` explicitly.** It no longer depends + on rustls's process default, which is ambiguous when a host includes both `ring` + and `aws-lc-rs`. It neither reads nor installs that default, and TLS setup errors + now return `HttpTransportError::Connect` instead of panicking. + +- **Glob routes under `tool:`, `resource:`, `prompt:`, and `llm:` now evaluate + their policy bodies.** Annotation lookup now resolves a request name to the + configured pattern. Exact selectors still outrank globs, including when the + exact route has no policy body. - **A bundle joined through both `meta.tags` and `groups:` no longer runs its `authentication:` steps twice.** Bundle membership is now deduplicated before diff --git a/crates/ppe-apl-runtime/src/dispatch_plan.rs b/crates/ppe-apl-runtime/src/dispatch_plan.rs index 41705c2e..d9865b59 100644 --- a/crates/ppe-apl-runtime/src/dispatch_plan.rs +++ b/crates/ppe-apl-runtime/src/dispatch_plan.rs @@ -637,19 +637,10 @@ pub(crate) fn collect_plugin_names_by_half(route: &CompiledRoute) -> (Vec Vec<(String, &'static str)> { diff --git a/crates/ppe-apl-runtime/src/visitor.rs b/crates/ppe-apl-runtime/src/visitor.rs index 9e50cf8e..6bc4fd86 100644 --- a/crates/ppe-apl-runtime/src/visitor.rs +++ b/crates/ppe-apl-runtime/src/visitor.rs @@ -518,11 +518,7 @@ impl AplConfigVisitor { /// routes below it. fn record_reached_plugins(&self, route: &CompiledRoute, hook_pre: &str, hook_post: &str) { let (pre, post) = crate::dispatch_plan::collect_plugin_names_by_half(route); - // A delegator and an elicitation handler dispatch under their own - // family's hook, not the route's entity pair, so they are credited - // separately below. Crediting them with the CMF hook made their declared - // `token.delegate` / `elicit` read as uncovered, and the narrowing report - // then fired on every configuration that delegates. + // Delegation and elicitation use family-specific hooks, recorded below. let family_fixed = crate::dispatch_plan::collect_family_fixed_plugin_hooks(route); let mut state = self .state @@ -534,7 +530,6 @@ impl AplConfigVisitor { for name in names { state.reached_plugin_names.insert(name.clone()); if family_fixed_names.contains(name.as_str()) { - // Reached, but on its own family's hook. Recorded below. continue; } state @@ -2165,9 +2160,7 @@ routes: ); } - /// A glob route under one of the four MCP selectors. The annotation is - /// installed under the pattern as written, so reaching the body means - /// resolving the request's name to that pattern first. + /// A glob route's annotation is keyed by its pattern, not the request name. const GLOB_TOOL_ROUTE: &str = r#" engine_settings: dispatch: policy @@ -2195,8 +2188,7 @@ routes: "the route denies and the name the glob covers is governed by it" ); - // A name the pattern does not cover resolves no route, so the body that - // denies is not reached and the request is not governed by it. + // A name outside the pattern reaches no route body. let (allowed, _bg) = mgr .invoke_named::( HOOK_CMF_TOOL_PRE_INVOKE, diff --git a/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs b/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs index a7ba5ff4..c1c1c230 100644 --- a/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs +++ b/crates/ppe-apl-runtime/tests/dispatch_mode_e2e.rs @@ -556,14 +556,7 @@ routes: ); } -/// A delegator reached by a `delegate(...)` step is not narrowed. -/// -/// Its hook is `token.delegate`, fixed by its own family rather than by the -/// entity the route selects. The tally credited every reached plugin with the -/// route's CMF hook pair, so a delegator came out covered on -/// `cmf.tool_pre_invoke` and uncovered on the one hook it actually declares. -/// That fired on every configuration that delegates, which is most of them, and -/// an alarm that always fires is one nobody reads. +/// A `delegate(...)` step reaches its plugin on `token.delegate`. #[test] fn a_delegator_reached_by_a_delegate_step_is_not_reported_as_narrowed() { let alarms = alarms_raised_by_loading( @@ -581,12 +574,11 @@ routes: ); assert!( !alarms.contains(&NARROWED.to_owned()), - "the step reaches it under `token.delegate`, which is the hook it \ - declares, so there is nothing uncovered: {alarms:?}" + "`token.delegate` is covered: {alarms:?}" ); } -/// The same for an elicitation handler, whose hook is `elicit`. +/// An elicitation verb reaches its handler on `elicit`. #[test] fn an_elicitation_handler_reached_by_a_verb_is_not_reported_as_narrowed() { let alarms = alarms_raised_by_loading( @@ -604,13 +596,11 @@ routes: ); assert!( !alarms.contains(&NARROWED.to_owned()), - "the verb reaches it under `elicit`, which is the hook it declares: \ - {alarms:?}" + "`elicit` is covered: {alarms:?}" ); } -/// And the report still fires for a delegator that genuinely declares a hook -/// nothing reaches, so the fix above narrowed the alarm rather than muting it. +/// Family-specific reachability does not hide other uncovered hooks. #[test] fn a_delegator_declaring_an_unreached_cmf_hook_is_still_reported() { let alarms = alarms_raised_by_loading( diff --git a/crates/ppe-apl-runtime/tests/http_route_e2e.rs b/crates/ppe-apl-runtime/tests/http_route_e2e.rs index f20b1334..299c1ea7 100644 --- a/crates/ppe-apl-runtime/tests/http_route_e2e.rs +++ b/crates/ppe-apl-runtime/tests/http_route_e2e.rs @@ -781,15 +781,7 @@ routes: } } -/// A glob route under one of the entity selectors installs its annotation under -/// the pattern as written, and the lookup is exact, so its policy body never -/// evaluates. With the activation list gone there is no chain behind it either, -/// so the route reaches nothing. -/// A glob selector annotates under the pattern it writes, and a request arrives -/// under a name of its own, so finding the body means resolving the route first. -/// This used to dispatch nothing: the route resolved for everything else it -/// declared and its policy never ran, which is a deny an operator wrote and an -/// allow the request got. +/// A glob selector resolves request names to the annotation keyed by its pattern. #[tokio::test] async fn a_glob_entity_route_dispatches_its_body() { const YAML: &str = r#" @@ -811,13 +803,11 @@ routes: assert_eq!( fired(&ledger), vec!["body-audit".to_owned()], - "the pattern the route writes is what its annotation is keyed on, so the \ - request's own name has to resolve to that pattern before the body is found" + "the request name must resolve to the glob annotation" ); } -/// Two names under one pattern reach the same compiled body, the way two paths -/// under one `path_prefix` do. +/// Every name matched by a glob shares its compiled body. #[tokio::test] async fn many_names_under_one_glob_share_its_body() { const YAML: &str = r#" @@ -845,7 +835,7 @@ routes: ); } -/// A name the pattern does not cover resolves no route and reaches no body. +/// A name outside the glob reaches no body. #[tokio::test] async fn a_name_outside_the_glob_reaches_no_body() { const YAML: &str = r#" @@ -864,14 +854,10 @@ routes: let (mgr, ledger) = engine_with(YAML).await; assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("finance-close")).await); - assert!( - fired(&ledger).is_empty(), - "resolving the route is what finds a glob's body, and this name resolves none" - ); + assert!(fired(&ledger).is_empty(), "the name matches no route"); } -/// An exact selector outranks a glob that also matches, so the exact route's -/// body is the one that runs. +/// An exact selector outranks a matching glob. #[tokio::test] async fn an_exact_entity_route_outranks_a_glob_that_also_matches() { const YAML: &str = r#" @@ -900,8 +886,7 @@ routes: assert_eq!( fired(&ledger), vec!["exact-audit".to_owned()], - "the exact name is looked up before the route is resolved at all, so the \ - glob never gets the chance to answer for a name spelled out beside it" + "the exact route must win" ); clear(&ledger); @@ -913,10 +898,7 @@ routes: ); } -/// An exact route carrying no policy shadows a glob that would have governed the -/// name. Worth pinning: it is the one shape where adding a route removes -/// enforcement, and it follows from specificity rather than from anything the -/// annotation lookup does. +/// An exact route without policy still shadows a matching glob. #[tokio::test] async fn an_exact_route_with_no_body_shadows_a_glob_that_has_one() { const YAML: &str = r#" @@ -938,8 +920,7 @@ routes: assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-get-salary")).await); assert!( fired(&ledger).is_empty(), - "the exact route wins on specificity and declares no policy, so the glob's \ - body does not stand in for it" + "the glob body must not replace the more specific route's empty body" ); } @@ -1650,8 +1631,7 @@ routes: tool_request("get_weather"), "tool-audit", ), - // The glob's annotation is keyed on the pattern as written, and the - // name resolves to that pattern before the lookup, so its body runs. + // Resolve the request name to the glob pattern used as the annotation key. ( "cmf.tool_pre_invoke", tool_request("hr-get-salary"), diff --git a/crates/ppe-core/src/engine.rs b/crates/ppe-core/src/engine.rs index 15151802..c9afa902 100644 --- a/crates/ppe-core/src/engine.rs +++ b/crates/ppe-core/src/engine.rs @@ -160,17 +160,9 @@ fn declares_assertions(config: &PolicyConfig) -> bool { || config.routes.iter().any(|route| route.assertions.is_some()) } -/// Whether any route selects a named entity with a glob pattern. +/// Whether any named route uses a glob selector. /// -/// The annotation table is keyed on the name the configuration writes, and for -/// the four named entity types a request arrives under a name of its own. Those -/// two agree for an exact selector and diverge for a glob, so a glob route needs -/// its route resolved before its annotation can be found. Resolving is a route -/// table walk, and this is what keeps it off every deployment that writes no -/// glob, which is the ordinary one. -/// -/// A list selector matches by equality, so it is not a glob: `matched_selector_name` -/// returns the element that matched and that is the name the annotation carries. +/// This avoids resolving routes before annotation lookup in exact/list-only configs. fn declares_glob_named_routes(config: &PolicyConfig) -> bool { config.routes.iter().any(|route| { [ @@ -185,10 +177,7 @@ fn declares_glob_named_routes(config: &PolicyConfig) -> bool { }) } -/// Whether a name selector can match a name it is not equal to. -/// -/// Only the single-pattern shape can: `wildmatch` reads `*` and `?` as -/// metacharacters, and a list matches by equality. +/// Whether a selector can match a different name through wildcards. fn is_glob_selector(selector: &config::StringOrList) -> bool { match selector { config::StringOrList::Single(pattern) => pattern.as_str().contains(['*', '?']), @@ -530,9 +519,7 @@ struct RuntimeSnapshot { /// path, for the same reason its authentication counterpart is. http_routes_declaring_assertions: Arc<[String]>, - /// Whether any route selects a named entity with a glob. False for every - /// config that writes only exact names and lists, which is what keeps the - /// route resolution a glob annotation needs out of those deployments. + /// Whether any named route uses a glob selector. declares_glob_named_routes: bool, } @@ -2539,18 +2526,8 @@ impl PolicyEngine { hook_name: hook_name.to_owned(), }) }); - // A glob selector annotates under the pattern it writes, and the - // two lookups above ask for the name the request arrived under, so - // neither can find it. Resolve the route and ask again under the - // name that matched. Without this a route like `tool: get_*` - // carrying `authorization:` resolved for everything else it - // declares and dispatched no policy at all, which is a deny an - // operator wrote and an allow the request got. - // - // Gated on the config declaring a glob, so a deployment writing - // only exact names and lists pays neither the walk nor a second - // pair of lookups. `early_named` is the same resolution, already - // done when an `assertions:` contract needed it. + // Glob annotations are keyed by their pattern, not the request name. + // Resolve only when a glob exists, reusing assertion resolution when available. let glob_matched = if candidate.is_none() && snapshot.declares_glob_named_routes { early_named.as_ref().map_or_else( || { @@ -2571,8 +2548,7 @@ impl PolicyEngine { } else { None }; - // Only when the route resolved to a name other than the request's: - // an equal name is what the two lookups above already asked for. + // An equal name was already checked above. let candidate = candidate.or_else(|| { let matched = glob_matched.as_ref()?; if matched.name == en { diff --git a/crates/ppe/Cargo.toml b/crates/ppe/Cargo.toml index 4e823df4..a1897ebc 100644 --- a/crates/ppe/Cargo.toml +++ b/crates/ppe/Cargo.toml @@ -138,11 +138,8 @@ hyper-rustls = { version = "0.27", optional = true, default-features = false, fe "tls12", "webpki-roots", ] } -# Named directly so the transport can build its `ClientConfig` against an -# explicit crypto provider rather than the process-level default. A host's own -# graph decides that default, and a graph carrying both `ring` and `aws-lc-rs` -# has none: rustls refuses to choose and panics on first use. Praxis is exactly -# that graph, since pingora and its own TLS stack pull `aws-lc-rs`. +# Direct dependencies let the transport select `ring` without reading or setting +# rustls's process-wide default provider. rustls = { version = "0.23", optional = true, default-features = false, features = [ "ring", "tls12", diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index 81ebcbeb..86b6013f 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -188,27 +188,20 @@ impl HyperTransport { self } - /// The shared client, built on first call. - /// - /// Fallible because building the TLS configuration is: naming a crypto - /// provider means asking it for protocol versions, and that answer is a - /// `Result`. Returning it beats unwrapping, which is the panic this whole - /// path exists to remove. + /// Return the shared client, building it on first use. fn client(&self) -> Result<&HyperClient, HttpTransportError> { if let Some(client) = self.client.get() { return Ok(client); } let built = Self::build_client(self)?; - // A racing caller may have won `set`; either client is equivalent, and - // `get_or_init`'s own contract is the same. Read back whichever landed - // so every caller shares one pool. + // Another caller may win the race; use whichever equivalent client was stored. let _ = self.client.set(built); self.client .get() .ok_or_else(|| HttpTransportError::Connect("HTTP client init raced".to_owned())) } - /// Build the pooling client. Called once per transport, from `client`. + /// Build a pooling client. fn build_client(&self) -> Result { { let mut http = HttpConnector::new(); @@ -224,25 +217,9 @@ impl HyperTransport { http.set_nodelay(true); http.set_keepalive(self.tcp_keepalive); - // An explicit provider, not the process-level default. - // - // `with_webpki_roots()` builds its `ClientConfig` through - // `rustls::ClientConfig::builder()`, which reads the default - // provider, and a host's own dependency graph is what sets - // that. A graph carrying both `ring` and `aws-lc-rs` has no - // unambiguous default, so rustls panics on the first - // connection rather than choosing; Praxis is exactly that - // graph, because pingora and its TLS stack pull `aws-lc-rs` - // while this transport pulls `ring`. - // - // Naming the provider here keeps that decision inside the - // transport. Installing a process default instead would - // reach outside it and could lose a race with a host - // installing its own. - // - // Webpki roots rather than the system store, matching what - // the reqwest path resolved to and keeping the trust set - // identical across every deployment. + // Select `ring` locally: a host may load both supported providers, + // leaving rustls without a process default. Do not install one here. + // Keep the webpki roots used by the previous connector configuration. let mut roots = rustls::RootCertStore::empty(); roots.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); let tls_config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new( @@ -458,23 +435,13 @@ mod tests { #[tokio::test] async fn the_pool_builds_with_no_process_default_crypto_provider() { - // The graph guard, and the reason the TLS configuration names its - // provider. `ClientConfig::builder()` reads the process-level default, - // and a host's own dependency graph is what sets that: a graph carrying - // both `ring` and `aws-lc-rs` has no unambiguous default, so rustls - // panicked on the first connection rather than choosing. Praxis is - // exactly that graph. - // - // This test process installs no default, which is the same condition - // from the other direction: if the connector went back to reading it, - // building the pool would panic here. + // Building against the named provider must neither require nor install a default. assert!( rustls::crypto::CryptoProvider::get_default().is_none(), "the guard only means something while nothing has installed a default" ); let t = HyperTransport::new(); - // A closed port, so this reaches the connector and stops there. What is - // under test is that building it produced a client at all. + // A closed loopback port builds the client, then fails to connect. let _ = t.execute(HttpRequest::get("https://127.0.0.1:1/x")).await; assert!( t.client.get().is_some(), diff --git a/docs/upgrade-apl.md b/docs/upgrade-apl.md index a9e1bc63..f9ee5fac 100644 --- a/docs/upgrade-apl.md +++ b/docs/upgrade-apl.md @@ -12,10 +12,8 @@ means every key in it does something. Work through the sections in order. The first two change the shape of the document, and the rest are local rewrites. -One item in section 10 reaches a configuration even though the section is written -for a host: a plugin that fetches a JWKS, exchanges a token, or dispatches a CIBA -prompt must now declare the `perform_http` capability. Read that subsection even -if you write no Rust. +Section 10 also covers the `perform_http` capability required by plugins that +fetch JWKS, exchange tokens, or dispatch CIBA prompts. --- @@ -442,10 +440,8 @@ For a host embedding the engine rather than only writing configuration. ### The generic-HTTP hooks moved families -Both names and both constants moved, and so did the type a handler is written -against. The two names are `http.request` and `http.response`, the constants live -in `praxis_policy_core::http_hook`, and the payload is `HttpPayload`, which -carries no fields: +The hook names are now `http.request` and `http.response`; their constants, +handler type, and empty payload live in `praxis_policy_core::http_hook`: ```rust // before @@ -460,20 +456,13 @@ use praxis_policy_core::http_hook::{HOOK_HTTP_REQUEST, HttpHook, HttpPayload}; mgr.invoke_named::(HOOK_HTTP_REQUEST, HttpPayload, ext, None).await; ``` -The placeholder message is what the split removes. Nothing on the HTTP path -filled it, so a content-inspecting plugin registered on an HTTP hook scanned a -fabricated message and reported clean, and an always-passing scanner is worse -than no scanner. A `hooks:` entry in YAML naming `cmf.http_request` or -`cmf.http_response` fails the load and names the replacement. +HTTP handlers no longer receive a fabricated placeholder message. YAML using +`cmf.http_request` or `cmf.http_response` now fails with the replacement name. ### Subject claims keep their JSON shape -`Subject::claims` is `HashMap`. It was `HashMap`, -where a structured claim had already been flattened for you. - -A host that wants the old flat strings does the flattening itself, and wants -`as_str()` on the string arm rather than `to_string()`, or every string claim -arrives quoted: +`Subject::claims` changed from `HashMap` to +`HashMap`. To retain flat strings without quoting string values: ```rust let flat = value.as_str().map_or_else(|| value.to_string(), str::to_owned); @@ -481,35 +470,28 @@ let flat = value.as_str().map_or_else(|| value.to_string(), str::to_owned); ### A host must install an HTTP transport -**This one compiles.** It is the only change in this section a clean build does -not catch, so read it even if your build is green. +This requirement is checked at initialization, not compile time. -PPE performs no outbound HTTP of its own. `identity-jwt`, `delegator-oauth` and -`elicitation-ciba` borrow a transport the host installs, and with none installed -a `jwks_url` issuer fails at `PolicyEngine::initialize()` rather than at load. +`identity-jwt`, `delegator-oauth`, and `elicitation-ciba` use a transport supplied +by the host. Without one, HTTP-dependent plugins fail at +`PolicyEngine::initialize()`. -A host that already has an HTTP stack should lend that one, so the process keeps -one connection pool, one TLS trust store, and one egress path: +To reuse the host's connection pool, trust store, and egress path: ```rust mgr.set_http_transport(Arc::new(MyTransport::new())); ``` -A host without one enables the non-default `http-hyper` feature on -`praxis-policy` and installs the bundled implementation. It is deliberately not -folded into `install_builtins`, so wiring an egress path stays one explicit line: +Otherwise, enable `praxis-policy`'s non-default `http-hyper` feature and install +the bundled transport explicitly: ```rust praxis_policy::install_builtins(&mgr); praxis_policy::install_default_http_transport(&mgr); ``` -The bundled transport builds its pool on first use, so installing it from a -short-lived initialization runtime is safe. - -**Breaking for existing configuration too**, and it is checked at -initialization: a plugin using `jwks_url`, an OAuth delegator, or a CIBA approver -must declare the `perform_http` capability. +The bundled transport builds its pool on first use. Each plugin using `jwks_url`, +OAuth delegation, or CIBA must also declare `perform_http`: ```yaml plugins: @@ -519,11 +501,8 @@ plugins: - perform_http ``` -Withholding it stops the call rather than degrading it, because a plugin that -quietly skipped its `IdP` call would fail open. The two failures report -separately: `ServiceError::NotInstalled` says the embedding host installed no -transport, which is a wiring problem, and `NotPermitted` names the capability to -add, which is a configuration one. +`ServiceError::NotInstalled` indicates a missing host transport; +`ServiceError::NotPermitted` indicates a missing capability. `Phase` and `CompiledRoute` both derive `Serialize`, so **the serialized keys change too**: a phase serializes as `pre_invocation` / `post_invocation`, and a From 5ffebcf3cf7ab9a48c66bf71150195d6a5ac7e09 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Thu, 3 Sep 2026 09:25:31 -0400 Subject: [PATCH 6/7] build(deps): bump uuid to 1.26.0 From the cargo-minor-patch dependabot PR (#61), folded in here because it touches Cargo.lock and would otherwise conflict with this branch. Lock only, no manifest change. Signed-off-by: Frederico Araujo --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 795882de..a740e30c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3872,9 +3872,9 @@ checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" [[package]] name = "uuid" -version = "1.24.1" +version = "1.26.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2cefc03fd367c0c6d4305de1b312cf00248c4114f4a0418ce6a6af769e3b0bd9" +checksum = "b5772d71c9be8a8a6ac2117d949c5b224c1b72241bb611d9a3012edcf8af7812" dependencies = [ "getrandom 0.4.3", "js-sys", From 441e17cfcdbb71da19caefd69e0a19092f872aa8 Mon Sep 17 00:00:00 2001 From: Frederico Araujo Date: Thu, 3 Sep 2026 09:56:41 -0400 Subject: [PATCH 7/7] refactor(http): flatten build_client, pin bracket-literal behaviour Two review points on #72. build_client kept the brace from the get_or_init closure it was extracted from. Removed; body is unchanged. The glob detector was asked to also treat `[` as a wildcard. It should not: wildmatch defines only `*` and `?` and no escapes, so `hr-[a-z]` is eight literal characters and matches nothing else. A literal selector needs no resolution, since its annotation key equals the request name whenever it matches. Recorded that in the function's doc and pinned it with a test. Signed-off-by: Frederico Araujo --- .../ppe-apl-runtime/tests/http_route_e2e.rs | 38 ++++++ crates/ppe-core/src/engine.rs | 5 + crates/ppe/src/http_hyper.rs | 114 +++++++++--------- 3 files changed, 99 insertions(+), 58 deletions(-) diff --git a/crates/ppe-apl-runtime/tests/http_route_e2e.rs b/crates/ppe-apl-runtime/tests/http_route_e2e.rs index 299c1ea7..a3f3b2a2 100644 --- a/crates/ppe-apl-runtime/tests/http_route_e2e.rs +++ b/crates/ppe-apl-runtime/tests/http_route_e2e.rs @@ -835,6 +835,44 @@ routes: ); } +/// A bracket expression is a literal, not a character class. +/// +/// `wildmatch` reads `*` and `?` and nothing else, so `hr-[a-z]` matches the +/// eight characters `hr-[a-z]` and no other name. That makes it an exact +/// selector: the annotation key and the request name agree whenever it matches, +/// which is why the glob detector does not look for `[`. +#[tokio::test] +async fn a_bracket_selector_is_matched_literally() { + const YAML: &str = r#" +engine_settings: + dispatch: policy +plugins: + - name: body-audit + kind: test/record + hooks: [cmf.tool_pre_invoke] +routes: + - tool: "hr-[a-z]" + authorization: + pre_invocation: + - "run(body-audit)" +"#; + let (mgr, ledger) = engine_with(YAML).await; + + // The name a character class would have covered. + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-x")).await); + assert!(fired(&ledger).is_empty(), "`hr-x` matches no route"); + + // The literal spelling is what the selector names, and the exact lookup + // finds its annotation with no glob resolution. + clear(&ledger); + assert!(fire(&mgr, "cmf.tool_pre_invoke", tool_request("hr-[a-z]")).await); + assert_eq!( + fired(&ledger), + vec!["body-audit".to_owned()], + "the literal name reaches the body" + ); +} + /// A name outside the glob reaches no body. #[tokio::test] async fn a_name_outside_the_glob_reaches_no_body() { diff --git a/crates/ppe-core/src/engine.rs b/crates/ppe-core/src/engine.rs index c9afa902..ccfc4f17 100644 --- a/crates/ppe-core/src/engine.rs +++ b/crates/ppe-core/src/engine.rs @@ -178,6 +178,11 @@ fn declares_glob_named_routes(config: &PolicyConfig) -> bool { } /// Whether a selector can match a different name through wildcards. +/// +/// `*` and `?` are the whole set: `wildmatch` defines no others and no escapes, +/// so `hr-[a-z]` is the literal eight characters rather than a character class. +/// A literal selector needs nothing here, because its annotation key and the +/// request name agree whenever it matches. fn is_glob_selector(selector: &config::StringOrList) -> bool { match selector { config::StringOrList::Single(pattern) => pattern.as_str().contains(['*', '?']), diff --git a/crates/ppe/src/http_hyper.rs b/crates/ppe/src/http_hyper.rs index 86b6013f..6fce73aa 100644 --- a/crates/ppe/src/http_hyper.rs +++ b/crates/ppe/src/http_hyper.rs @@ -203,65 +203,63 @@ impl HyperTransport { /// Build a pooling client. fn build_client(&self) -> Result { - { - let mut http = HttpConnector::new(); - // The HTTPS connector wraps this one, so it must accept the - // `https` scheme rather than rejecting it as non-HTTP. - http.enforce_http(false); - http.set_connect_timeout(Some(self.connect_timeout)); - // hyper-util defaults this to false; reqwest sets it true. - // Leaving Nagle on would let a small request body — a token - // exchange form is a couple of hundred bytes — sit waiting to - // coalesce with data that never comes, adding tens of - // milliseconds to a call on the request path. - http.set_nodelay(true); - http.set_keepalive(self.tcp_keepalive); - - // Select `ring` locally: a host may load both supported providers, - // leaving rustls without a process default. Do not install one here. - // Keep the webpki roots used by the previous connector configuration. - let mut roots = rustls::RootCertStore::empty(); - roots.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); - let tls_config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new( - rustls::crypto::ring::default_provider(), - )) - .with_safe_default_protocol_versions() - .map_err(|e| HttpTransportError::Connect(format!("rustls client configuration: {e}")))? - .with_root_certificates(roots) - .with_no_client_auth(); - - let tls = hyper_rustls::HttpsConnectorBuilder::new() - .with_tls_config(tls_config) - // `https_or_http`, not `https_only`: `identity-jwt` - // supports an explicit `insecure_http: true` for local - // development, and it already refuses plaintext by - // default one layer up. Enforcing here as well would - // make that setting silently ineffective. - .https_or_http(); - - // `enable_all_versions` advertises ALPN `h2, http/1.1`; - // `enable_http1` advertises none. Either way a peer that - // cannot do HTTP/2 gets HTTP/1.1, and plaintext gets it - // regardless since there is no ALPN without TLS. - let https = if self.http2 { - tls.enable_all_versions().wrap_connector(http) - } else { - tls.enable_http1().wrap_connector(http) - }; + let mut http = HttpConnector::new(); + // The HTTPS connector wraps this one, so it must accept the + // `https` scheme rather than rejecting it as non-HTTP. + http.enforce_http(false); + http.set_connect_timeout(Some(self.connect_timeout)); + // hyper-util defaults this to false; reqwest sets it true. + // Leaving Nagle on would let a small request body — a token + // exchange form is a couple of hundred bytes — sit waiting to + // coalesce with data that never comes, adding tens of + // milliseconds to a call on the request path. + http.set_nodelay(true); + http.set_keepalive(self.tcp_keepalive); + + // Select `ring` locally: a host may load both supported providers, + // leaving rustls without a process default. Do not install one here. + // Keep the webpki roots used by the previous connector configuration. + let mut roots = rustls::RootCertStore::empty(); + roots.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned()); + let tls_config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new( + rustls::crypto::ring::default_provider(), + )) + .with_safe_default_protocol_versions() + .map_err(|e| HttpTransportError::Connect(format!("rustls client configuration: {e}")))? + .with_root_certificates(roots) + .with_no_client_auth(); + + let tls = hyper_rustls::HttpsConnectorBuilder::new() + .with_tls_config(tls_config) + // `https_or_http`, not `https_only`: `identity-jwt` + // supports an explicit `insecure_http: true` for local + // development, and it already refuses plaintext by + // default one layer up. Enforcing here as well would + // make that setting silently ineffective. + .https_or_http(); + + // `enable_all_versions` advertises ALPN `h2, http/1.1`; + // `enable_http1` advertises none. Either way a peer that + // cannot do HTTP/2 gets HTTP/1.1, and plaintext gets it + // regardless since there is no ALPN without TLS. + let https = if self.http2 { + tls.enable_all_versions().wrap_connector(http) + } else { + tls.enable_http1().wrap_connector(http) + }; - let client = Client::builder(TokioExecutor::new()) - // Without a timer, `pool_idle_timeout` silently does - // nothing and idle connections are never evicted. With - // `pool_max_idle_per_host` defaulting to unlimited, that - // is unbounded socket growth against a busy `IdP`, which - // is a slow leak rather than an error anyone would see. - .pool_timer(TokioTimer::new()) - .pool_idle_timeout(self.pool_idle_timeout) - .pool_max_idle_per_host(self.pool_max_idle_per_host) - .build(https); - - Ok(client) - } + let client = Client::builder(TokioExecutor::new()) + // Without a timer, `pool_idle_timeout` silently does + // nothing and idle connections are never evicted. With + // `pool_max_idle_per_host` defaulting to unlimited, that + // is unbounded socket growth against a busy `IdP`, which + // is a slow leak rather than an error anyone would see. + .pool_timer(TokioTimer::new()) + .pool_idle_timeout(self.pool_idle_timeout) + .pool_max_idle_per_host(self.pool_max_idle_per_host) + .build(https); + + Ok(client) } }