From 849724feb257cdd231ac4d681f4c1842750067ac Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 17:19:05 +0300 Subject: [PATCH 01/11] feat!: embedding hooks + configurable edge routes Add axum-free Tier-2 embedding hooks so a service gets the full stateless REST/OIDC/forward-auth edge by adding the crate, with no axum in its own Cargo.toml: - AuthDecider: in-process forward-auth/PDP gate, runs inline on proxied routes and backs /verify (path set via with_verify_path) - OidcBackend: stateless discovery/JWKS/userinfo, paths consumer-supplied - with_extra_routes: framework-agnostic route adapter (request/response parts) - Make health and metrics endpoint paths configurable and disablable - Fix RedisStore intra-doc link in no-redis builds; docs.rs builds with the redis feature so the type is documented Hook contracts use only http/bytes/serde_json, so AuthDecider adds zero new dependencies. Standalone binary behavior is unchanged. BREAKING CHANGE: ProxyConfig gains `health` and `metrics` fields; code that builds ProxyConfig with a struct literal must add them. YAML configs are unaffected (serde defaults fill them in). Closes #66 --- Cargo.toml | 18 ++ README.md | 59 ++++- src/config.rs | 112 +++++++++ src/embed.rs | 287 +++++++++++++++++++++ src/embed/tests.rs | 590 ++++++++++++++++++++++++++++++++++++++++++++ src/hooks.rs | 193 +++++++++++++++ src/lib.rs | 226 +++++++++++++---- src/shield/store.rs | 26 +- tests/embedded.rs | 2 + tests/hooks.rs | 309 +++++++++++++++++++++++ 10 files changed, 1764 insertions(+), 58 deletions(-) create mode 100644 src/embed.rs create mode 100644 src/embed/tests.rs create mode 100644 src/hooks.rs create mode 100644 tests/hooks.rs diff --git a/Cargo.toml b/Cargo.toml index c16a8fa..c12feae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,11 @@ readme = "README.md" keywords = ["grpc", "rest", "proxy", "transcoding", "protobuf"] categories = ["network-programming", "web-programming::http-server"] +# docs.rs builds with the `redis` feature on top of the defaults so the +# Redis-backed rate-limit store (and its intra-doc links) are documented. +[package.metadata.docs.rs] +features = ["redis"] + [[bin]] name = "structured-proxy" path = "src/main.rs" @@ -25,6 +30,12 @@ path = "src/lib.rs" axum = { version = "0.8", features = ["macros"] } tower = "0.5" tower-http = { version = "0.7", features = ["cors", "trace"] } +# Foundational HTTP types, used directly by the framework-agnostic embedding +# hooks (src/hooks.rs) so an embedder never names `axum`. Already in the tree +# transitively via both axum and tonic; pinned here to make the public API deps +# explicit. +http = "1" +bytes = "1" # gRPC client (to upstream service) tonic = "0.14" @@ -112,6 +123,13 @@ redis = ["dep:redis"] tokio = { version = "1", features = ["macros", "rt-multi-thread"] } tower = { version = "0.5", features = ["util"] } http-body-util = "0.1" +# The embedding-hooks integration test (tests/hooks.rs) writes hook impls using +# only these (the same crates a real embedder uses — none is an HTTP framework) +# and drives the resulting router via axum + tower for assertions. +axum = { version = "0.8", features = ["macros"] } +http = "1" +bytes = "1" +async-trait = "0.1" ed25519-dalek = "2" rand = "0.10" chrono = "0.4" diff --git a/README.md b/README.md index 00fa239..1922069 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Works with **any** gRPC service via proto descriptor files. No code generation, ## Non-goals -- **Session / BFF management** (cookie-based login, server-side token storage, refresh flows). This proxy is a stateless transcoding data plane with stateless auth primitives; session lifecycle is a separate, stateful concern. Put a dedicated BFF (e.g. `oauth2-proxy`, Pomerium) in front, or drive auth through the forward-auth / external-authz hooks above. +- **Session / BFF management** (cookie-based login, server-side token storage, refresh flows) and **stateful OIDC** (`authorize` / `token` with auth codes / PKCE state). The **default build** is a stateless transcoding data plane with stateless auth primitives; session lifecycle is a separate, stateful concern. Put a dedicated BFF (e.g. `oauth2-proxy`, Pomerium) in front, or drive auth through the stateless forward-auth / external-authz hooks below. (A stateful surface behind an opt-in, default-off `bff` Cargo feature is planned; it does not affect the default data-plane build.) ## Quick Start @@ -76,6 +76,20 @@ aliases: - from: "/api/v1/*" to: "/my.package.v1.MyService/*" +# Optional: health-probe endpoints. Paths are configurable (relocate behind an +# internal prefix) and the whole group can be disabled. Defaults shown. +health: + enabled: true + path: "/health" + live_path: "/health/live" + ready_path: "/health/ready" # checks the upstream gRPC health + startup_path: "/health/startup" + +# Optional: Prometheus metrics endpoint. Path configurable; can be disabled. +metrics: + enabled: true + path: "/metrics" + # Optional: maintenance mode (returns 503 except for exempt paths) maintenance: enabled: false @@ -184,6 +198,49 @@ async fn main() -> anyhow::Result<()> { } ``` +### Embedding hooks (axum-free) + +Inject *stateless* service-specific logic without naming an HTTP framework in +your own crate: implement the hook traits with foundational types (`http`, +`bytes`, `serde_json`) only, and `cargo tree -i axum` in your crate shows `axum` +solely under `structured-proxy`. + +```rust +use std::sync::Arc; +use structured_proxy::{config::ProxyConfig, ProxyServer}; +use structured_proxy::hooks::{AuthDecider, Decision, RequestParts}; + +struct MyPdp; // your forward-auth / policy decision + +#[async_trait::async_trait] +impl AuthDecider for MyPdp { + async fn decide(&self, req: &RequestParts<'_>) -> Decision { + // method / path / headers / peer in, a decision out — no axum types + Decision::Allow { inject_headers: http::HeaderMap::new() } + } +} + +# async fn run(config: ProxyConfig) -> anyhow::Result<()> { +ProxyServer::from_config(config) + .with_auth_decider(Arc::new(MyPdp)) // inline gate + /verify endpoint + // .with_oidc_backend(...) // stateless discovery / JWKS / userinfo + // .with_extra_routes(...) // extra stateless routes, axum-free + .serve() + .await +# } +``` + +The hooks are: + +- **`with_auth_decider`** — an in-process forward-auth / PDP decision, run inline + on every proxied request and exposed at `/verify` (path configurable via + `with_verify_path`). +- **`with_oidc_backend`** — backs the stateless OIDC surface (discovery, JWKS, + userinfo) with your key/client metadata; supersedes the config-driven static + discovery. +- **`with_extra_routes`** — registers extra stateless routes through a + framework-agnostic adapter (request parts in, response parts out). + ## How It Works 1. Load the proto descriptor from a pre-compiled descriptor file diff --git a/src/config.rs b/src/config.rs index 4c7edb6..2483450 100644 --- a/src/config.rs +++ b/src/config.rs @@ -50,6 +50,14 @@ pub struct ProxyConfig { #[serde(default)] pub oidc_discovery: Option, + /// Health-probe endpoints (paths configurable; can be disabled). + #[serde(default)] + pub health: HealthConfig, + + /// Prometheus metrics endpoint (path configurable; can be disabled). + #[serde(default)] + pub metrics: MetricsConfig, + /// Maintenance mode. #[serde(default)] pub maintenance: MaintenanceConfig, @@ -447,6 +455,81 @@ fn default_algorithm() -> String { "EdDSA".into() } +/// Health-probe endpoint configuration. +/// +/// Paths are configurable so an embedder can relocate the probes (e.g. behind a +/// `/internal/` prefix) or disable them when a fronting platform supplies its +/// own. Defaults match the conventional `/health*` layout. +#[derive(Debug, Clone, Deserialize)] +#[non_exhaustive] +pub struct HealthConfig { + /// Mount the health endpoints. Default: true. + #[serde(default = "default_true")] + pub enabled: bool, + /// Aggregate health endpoint. Default: `/health`. + #[serde(default = "default_health_path")] + pub path: String, + /// Liveness probe. Default: `/health/live`. + #[serde(default = "default_health_live_path")] + pub live_path: String, + /// Readiness probe (checks the upstream gRPC health). Default: `/health/ready`. + #[serde(default = "default_health_ready_path")] + pub ready_path: String, + /// Startup probe. Default: `/health/startup`. + #[serde(default = "default_health_startup_path")] + pub startup_path: String, +} + +fn default_health_path() -> String { + "/health".into() +} +fn default_health_live_path() -> String { + "/health/live".into() +} +fn default_health_ready_path() -> String { + "/health/ready".into() +} +fn default_health_startup_path() -> String { + "/health/startup".into() +} + +impl Default for HealthConfig { + fn default() -> Self { + Self { + enabled: true, + path: default_health_path(), + live_path: default_health_live_path(), + ready_path: default_health_ready_path(), + startup_path: default_health_startup_path(), + } + } +} + +/// Prometheus metrics endpoint configuration. +#[derive(Debug, Clone, Deserialize)] +#[non_exhaustive] +pub struct MetricsConfig { + /// Mount the metrics endpoint. Default: true. + #[serde(default = "default_true")] + pub enabled: bool, + /// Scrape path. Default: `/metrics`. + #[serde(default = "default_metrics_path")] + pub path: String, +} + +fn default_metrics_path() -> String { + "/metrics".into() +} + +impl Default for MetricsConfig { + fn default() -> Self { + Self { + enabled: true, + path: default_metrics_path(), + } + } +} + /// Maintenance mode config. #[derive(Debug, Clone, Deserialize)] #[non_exhaustive] @@ -586,6 +669,35 @@ upstream: assert!(config.shield.is_none()); } + #[test] + fn health_and_metrics_defaults_and_overrides() { + // Defaults: enabled, conventional paths. + let min: ProxyConfig = + serde_yaml::from_str("upstream:\n default: \"grpc://x:1\"\n").unwrap(); + assert!(min.health.enabled); + assert_eq!(min.health.path, "/health"); + assert_eq!(min.health.ready_path, "/health/ready"); + assert!(min.metrics.enabled); + assert_eq!(min.metrics.path, "/metrics"); + + // Overrides apply; unspecified sub-paths keep their defaults. + let yaml = r#" +upstream: + default: "grpc://x:1" +health: + path: "/internal/health" +metrics: + enabled: false + path: "/internal/metrics" +"#; + let cfg: ProxyConfig = serde_yaml::from_str(yaml).unwrap(); + assert_eq!(cfg.health.path, "/internal/health"); + // live_path was not overridden, so it stays at the default. + assert_eq!(cfg.health.live_path, "/health/live"); + assert!(!cfg.metrics.enabled); + assert_eq!(cfg.metrics.path, "/internal/metrics"); + } + #[test] fn test_zero_sse_keep_alive_is_rejected() { // A zero keep-alive would make axum's SSE timer fire continuously diff --git a/src/embed.rs b/src/embed.rs new file mode 100644 index 0000000..da76ec6 --- /dev/null +++ b/src/embed.rs @@ -0,0 +1,287 @@ +//! Axum glue for the framework-agnostic embedding hooks. +//! +//! This module is the *only* place that bridges the `axum`-free public hook +//! traits ([`crate::hooks`]) to the running axum server: it converts live axum +//! requests into the borrowed/owned hook views, runs the embedder's trait +//! impls, and renders their results back into axum responses. Keeping the +//! conversion here is what lets an embedder depend on the hook traits without +//! ever naming `axum`. + +use std::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; +use std::sync::Arc; + +use axum::body::Body; +use axum::extract::{ConnectInfo, Request, State}; +use axum::http::header::{CONTENT_TYPE, LOCATION}; +use axum::http::{HeaderMap, StatusCode}; +use axum::middleware::Next; +use axum::response::{IntoResponse, Response}; +use axum::routing::{get, on, MethodFilter, MethodRouter}; +use axum::{Json, Router}; + +use crate::hooks::{AuthDecider, Decision, ExtraRoute, OidcBackend, RequestParts, RouteRequest}; +use crate::ProxyState; + +/// Cap on the body an extra-route handler will buffer (16 MiB). Extra routes are +/// a stateless escape hatch, not a bulk-upload path; a bounded buffer keeps a +/// single request from exhausting memory. +const MAX_EXTRA_ROUTE_BODY: usize = 16 * 1024 * 1024; + +/// Fallback peer when the listener was not configured with `ConnectInfo` +/// (e.g. in `oneshot` tests). Real serving always supplies the connecting peer. +fn unknown_peer() -> SocketAddr { + SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, 0)) +} + +/// The connecting peer, or [`unknown_peer`] when `ConnectInfo` is absent. +fn peer_of(req: &Request) -> SocketAddr { + req.extensions() + .get::>() + .map(|ci| ci.0) + .unwrap_or_else(unknown_peer) +} + +/// Inline gate: run the embedder's [`AuthDecider`] on every proxied request. +/// +/// On `Allow`, the decider-controlled headers are injected onto the request +/// after stripping any client-supplied copies (so a client cannot forge them), +/// then the request proceeds upstream. +pub(crate) async fn auth_decider_gate( + State(decider): State>, + mut request: Request, + next: Next, +) -> Response { + let peer = peer_of(&request); + let decision = { + let uri = request.uri(); + let parts = RequestParts { + method: request.method(), + path: uri.path(), + query: uri.query(), + headers: request.headers(), + peer, + }; + decider.decide(&parts).await + }; + + match decision { + Decision::Allow { inject_headers } => { + let dst = request.headers_mut(); + strip_then_insert(dst, &inject_headers); + next.run(request).await + } + Decision::Deny { status, body } => deny_response(status, body), + // Inline (browser-facing) path: drive a real redirect. + Decision::Redirect { location } => redirect_response(StatusCode::FOUND, &location), + } +} + +/// The `/verify` forward-auth endpoint, backed by the same [`AuthDecider`]. +/// +/// A fronting proxy (nginx `auth_request`, Traefik `forwardAuth`, Envoy +/// ext-authz HTTP) sub-requests this path; the original method/URI arrive via +/// `x-forwarded-*` / `x-original-*` headers. `Allow` answers `200` with the +/// verified headers for the fronting proxy to copy upstream. +pub(crate) async fn verify_via_decider( + decider: Arc, + request: Request, +) -> Response { + let peer = peer_of(&request); + let headers = request.headers().clone(); + let method = original_method(&headers).unwrap_or_else(|| request.method().clone()); + let (path, query) = original_target(&headers).unwrap_or_else(|| { + let uri = request.uri(); + (uri.path().to_string(), uri.query().map(str::to_string)) + }); + + let decision = { + let parts = RequestParts { + method: &method, + path: &path, + query: query.as_deref(), + headers: &headers, + peer, + }; + decider.decide(&parts).await + }; + + match decision { + Decision::Allow { inject_headers } => (StatusCode::OK, inject_headers).into_response(), + Decision::Deny { status, body } => deny_response(status, body), + // Forward-auth path: a fronting proxy expects 401 (+ Location to drive + // its own error-page redirect), not a 302 it would have to follow. + Decision::Redirect { location } => redirect_response(StatusCode::UNAUTHORIZED, &location), + } +} + +/// Routes for the stateless OIDC surface supplied by an [`OidcBackend`]. +pub(crate) fn oidc_backend_routes(backend: Arc) -> Router { + let mut router = Router::new(); + + // Static metadata documents (openid-configuration, provider-specific docs). + for doc in backend.metadata_documents() { + let json = doc.json; + router = router.route( + &doc.path, + get(move || { + let json = json.clone(); + async move { Json(json) } + }), + ); + } + + // JWKS, with the RFC 7517 media type. + let jwks = backend.jwks(); + let jwks_body = + serde_json::to_string(&jwks.json).unwrap_or_else(|_| "{\"keys\":[]}".to_string()); + router = router.route( + &jwks.path, + get(move || { + let body = jwks_body.clone(); + async move { ([(CONTENT_TYPE, "application/jwk-set+json")], body) } + }), + ); + + // UserInfo: bearer token in, claims out (401 when the backend rejects it). + let userinfo_path = backend.userinfo_path(); + let userinfo_backend = backend.clone(); + router.route( + &userinfo_path, + get(move |headers: HeaderMap| { + let backend = userinfo_backend.clone(); + async move { + let token = bearer_token(&headers).unwrap_or_default(); + match backend.userinfo(&token).await { + Some(claims) => Json(claims).into_response(), + None => deny_response( + StatusCode::UNAUTHORIZED, + bytes::Bytes::from_static( + br#"{"error":"invalid_token","message":"invalid or expired token"}"#, + ), + ), + } + } + }), + ) +} + +/// Build a router for the embedder's extra stateless routes. +/// +/// Routes that share a path but differ in method are merged into one +/// [`MethodRouter`], so registering `GET /x` and `POST /x` does not panic. +pub(crate) fn extra_routes_router(routes: &[ExtraRoute]) -> Router { + use std::collections::HashMap; + + let mut by_path: HashMap> = HashMap::new(); + for route in routes { + let Ok(filter) = MethodFilter::try_from(route.method.clone()) else { + tracing::warn!( + method = %route.method, + path = %route.path, + "skipping extra route: unsupported HTTP method" + ); + continue; + }; + let handler = route.handler.clone(); + let service = on(filter, move |request: Request| { + let handler = handler.clone(); + async move { + let peer = peer_of(&request); + let (parts, body) = request.into_parts(); + let body = axum::body::to_bytes(body, MAX_EXTRA_ROUTE_BODY) + .await + .unwrap_or_default(); + let resp = handler + .handle(RouteRequest { + method: parts.method, + uri: parts.uri, + headers: parts.headers, + body, + peer, + }) + .await; + let mut response = Response::new(Body::from(resp.body)); + *response.status_mut() = resp.status; + *response.headers_mut() = resp.headers; + response + } + }); + match by_path.remove(&route.path) { + Some(existing) => { + by_path.insert(route.path.clone(), existing.merge(service)); + } + None => { + by_path.insert(route.path.clone(), service); + } + } + } + + let mut router = Router::new(); + for (path, method_router) in by_path { + router = router.route(&path, method_router); + } + router +} + +/// Remove any incoming copies of the soon-to-be-injected header names, then +/// insert the decider's values, so a client cannot forge them onto the upstream. +fn strip_then_insert(dst: &mut HeaderMap, inject: &HeaderMap) { + for name in inject.keys() { + while dst.remove(name).is_some() {} + } + for (name, value) in inject { + dst.append(name.clone(), value.clone()); + } +} + +/// Render a `Decision::Deny` as a JSON response. +fn deny_response(status: StatusCode, body: bytes::Bytes) -> Response { + (status, [(CONTENT_TYPE, "application/json")], body).into_response() +} + +/// Render a `Decision::Redirect` at the given status with a `Location` header. +/// A malformed `location` (not a valid header value) yields the bare status. +fn redirect_response(status: StatusCode, location: &str) -> Response { + let mut response = status.into_response(); + if let Ok(value) = location.parse() { + response.headers_mut().insert(LOCATION, value); + } + response +} + +/// The bearer token from an `Authorization` header (prefix stripped), if present. +fn bearer_token(headers: &HeaderMap) -> Option { + let value = headers.get("authorization")?.to_str().ok()?; + let token = value + .strip_prefix("Bearer ") + .or_else(|| value.strip_prefix("bearer "))? + .trim(); + (!token.is_empty()).then(|| token.to_string()) +} + +/// The original request method from a fronting proxy's forwarding headers. +fn original_method(headers: &HeaderMap) -> Option { + let raw = forwarded(headers, &["x-forwarded-method", "x-original-method"])?; + axum::http::Method::from_bytes(raw.to_ascii_uppercase().as_bytes()).ok() +} + +/// The original request path and query from a fronting proxy's forwarding headers. +fn original_target(headers: &HeaderMap) -> Option<(String, Option)> { + let raw = forwarded(headers, &["x-forwarded-uri", "x-original-uri"])?; + Some(match raw.split_once('?') { + Some((path, query)) => (path.to_string(), Some(query.to_string())), + None => (raw, None), + }) +} + +/// First non-empty value among `names`. +fn forwarded(headers: &HeaderMap, names: &[&str]) -> Option { + names + .iter() + .filter_map(|n| headers.get(*n).and_then(|v| v.to_str().ok())) + .find(|v| !v.is_empty()) + .map(str::to_string) +} + +#[cfg(test)] +mod tests; diff --git a/src/embed/tests.rs b/src/embed/tests.rs new file mode 100644 index 0000000..2e09319 --- /dev/null +++ b/src/embed/tests.rs @@ -0,0 +1,590 @@ +//! Tests for the axum glue bridging the framework-agnostic embedding hooks. + +use std::sync::Arc; + +use async_trait::async_trait; +use axum::body::Body; +use axum::http::{HeaderMap, Method, Request, StatusCode}; +use axum::routing::get; +use axum::Router; +use bytes::Bytes; +use tower::ServiceExt; + +use super::*; +use crate::hooks::{ + AuthDecider, Decision, ExtraRoute, ExtraRouteHandler, MetadataDocument, OidcBackend, + RequestParts, RouteRequest, RouteResponse, +}; + +// --- helpers ------------------------------------------------------------- + +async fn body_string(resp: Response) -> String { + let bytes = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .unwrap(); + String::from_utf8(bytes.to_vec()).unwrap() +} + +fn allow_with(name: &'static str, value: &'static str) -> Decision { + let mut h = HeaderMap::new(); + h.insert(name, value.parse().unwrap()); + Decision::Allow { inject_headers: h } +} + +/// An [`AuthDecider`] whose decision is computed from the request by a closure, +/// so tests can assert behaviour against method/path/headers. +struct FnDecider(F); + +#[async_trait] +impl AuthDecider for FnDecider +where + F: Fn(&RequestParts<'_>) -> Decision + Send + Sync, +{ + async fn decide(&self, req: &RequestParts<'_>) -> Decision { + (self.0)(req) + } +} + +fn decider(f: F) -> Arc +where + F: Fn(&RequestParts<'_>) -> Decision + Send + Sync + 'static, +{ + Arc::new(FnDecider(f)) +} + +// --- helper unit tests --------------------------------------------------- + +#[test] +fn bearer_token_parses_either_case_and_rejects_other_schemes() { + let mut h = HeaderMap::new(); + h.insert("authorization", "Bearer abc.def".parse().unwrap()); + assert_eq!(bearer_token(&h).as_deref(), Some("abc.def")); + + let mut lower = HeaderMap::new(); + lower.insert("authorization", "bearer xyz".parse().unwrap()); + assert_eq!(bearer_token(&lower).as_deref(), Some("xyz")); + + let mut basic = HeaderMap::new(); + basic.insert("authorization", "Basic xyz".parse().unwrap()); + assert_eq!(bearer_token(&basic), None); + assert_eq!(bearer_token(&HeaderMap::new()), None); +} + +#[test] +fn original_method_and_target_read_forwarding_headers() { + let mut h = HeaderMap::new(); + h.insert("x-forwarded-method", "post".parse().unwrap()); + h.insert( + "x-forwarded-uri", + "/v1/admin/things?page=2".parse().unwrap(), + ); + assert_eq!(original_method(&h), Some(Method::POST)); + assert_eq!( + original_target(&h), + Some(("/v1/admin/things".to_string(), Some("page=2".to_string()))) + ); + + // Falls back to x-original-* and tolerates a missing query. + let mut alt = HeaderMap::new(); + alt.insert("x-original-method", "GET".parse().unwrap()); + alt.insert("x-original-uri", "/v1/public".parse().unwrap()); + assert_eq!(original_method(&alt), Some(Method::GET)); + assert_eq!( + original_target(&alt), + Some(("/v1/public".to_string(), None)) + ); + + assert_eq!(original_method(&HeaderMap::new()), None); + assert_eq!(original_target(&HeaderMap::new()), None); +} + +#[test] +fn strip_then_insert_overrides_client_supplied_values() { + let mut dst = HeaderMap::new(); + dst.insert("x-user", "forged".parse().unwrap()); + dst.insert("x-other", "keep".parse().unwrap()); + + let mut inject = HeaderMap::new(); + inject.insert("x-user", "verified".parse().unwrap()); + + strip_then_insert(&mut dst, &inject); + + // The forged value is gone, only the verified one remains. + let users: Vec<_> = dst.get_all("x-user").iter().collect(); + assert_eq!(users.len(), 1); + assert_eq!(dst["x-user"], "verified"); + // Unrelated client headers are untouched. + assert_eq!(dst["x-other"], "keep"); +} + +// --- auth_decider_gate (inline gate) ------------------------------------ + +/// Build a tiny app with the gate in front of a handler that echoes the +/// `x-user` header the upstream would have received. +fn gated_app(decider: Arc) -> Router { + let echo = |headers: HeaderMap| async move { + headers + .get("x-user") + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_string() + }; + Router::new() + .route("/x", get(echo)) + .layer(axum::middleware::from_fn_with_state( + decider, + auth_decider_gate, + )) +} + +#[tokio::test] +async fn gate_allow_injects_headers_and_reaches_handler() { + let app = gated_app(decider(|_| allow_with("x-user", "alice"))); + let resp = app + .oneshot(Request::get("/x").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(body_string(resp).await, "alice"); +} + +#[tokio::test] +async fn gate_allow_strips_client_forged_inject_header() { + // The decider injects x-user; a client-supplied x-user must not survive. + let app = gated_app(decider(|_| allow_with("x-user", "real"))); + let resp = app + .oneshot( + Request::get("/x") + .header("x-user", "forged") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(body_string(resp).await, "real"); +} + +#[tokio::test] +async fn gate_deny_short_circuits_with_status_and_body() { + let app = gated_app(decider(|_| Decision::Deny { + status: StatusCode::FORBIDDEN, + body: Bytes::from_static(b"{\"error\":\"nope\"}"), + })); + let resp = app + .oneshot(Request::get("/x").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FORBIDDEN); + assert_eq!(resp.headers()["content-type"], "application/json"); + assert_eq!(body_string(resp).await, "{\"error\":\"nope\"}"); +} + +#[tokio::test] +async fn gate_redirect_is_302_with_location() { + let app = gated_app(decider(|_| Decision::Redirect { + location: "https://login.example.com".to_string(), + })); + let resp = app + .oneshot(Request::get("/x").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FOUND); + assert_eq!(resp.headers()["location"], "https://login.example.com"); +} + +#[tokio::test] +async fn gate_sees_request_path() { + // Allow only /x; the gate must observe the real path. + let app = gated_app(decider(|req| { + if req.path == "/x" { + allow_with("x-user", "ok") + } else { + Decision::Deny { + status: StatusCode::FORBIDDEN, + body: Bytes::new(), + } + } + })); + let resp = app + .oneshot(Request::get("/x").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} + +// --- verify_via_decider (/verify endpoint) ------------------------------ + +fn verify_app(decider: Arc) -> Router { + Router::new().route( + "/auth/verify", + axum::routing::any(move |req: axum::extract::Request| { + let decider = decider.clone(); + async move { verify_via_decider(decider, req).await } + }), + ) +} + +#[tokio::test] +async fn verify_allow_returns_200_with_verified_headers() { + let app = verify_app(decider(|_| allow_with("x-forwarded-user", "alice"))); + let resp = app + .oneshot( + Request::get("/auth/verify") + .header("x-forwarded-method", "GET") + .header("x-forwarded-uri", "/v1/admin/things") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.headers()["x-forwarded-user"], "alice"); +} + +#[tokio::test] +async fn verify_uses_original_method_and_path_from_forwarding_headers() { + // The sub-request verb is GET to /auth/verify, but the decision must be + // taken on the original POST /v1/admin/things. + let app = verify_app(decider(|req| { + if req.method == Method::POST && req.path == "/v1/admin/things" { + allow_with("x-ok", "1") + } else { + Decision::Deny { + status: StatusCode::FORBIDDEN, + body: Bytes::new(), + } + } + })); + let resp = app + .oneshot( + Request::get("/auth/verify") + .header("x-forwarded-method", "POST") + .header("x-forwarded-uri", "/v1/admin/things") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.headers()["x-ok"], "1"); +} + +#[tokio::test] +async fn verify_redirect_is_401_with_location() { + // A fronting proxy wants 401 (+ Location), not a 302 to follow. + let app = verify_app(decider(|_| Decision::Redirect { + location: "https://login.example.com".to_string(), + })); + let resp = app + .oneshot(Request::get("/auth/verify").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert_eq!(resp.headers()["location"], "https://login.example.com"); +} + +// --- oidc_backend_routes ------------------------------------------------- + +struct TestOidc; + +#[async_trait] +impl OidcBackend for TestOidc { + fn metadata_documents(&self) -> Vec { + vec![MetadataDocument::new( + "/.well-known/openid-configuration", + serde_json::json!({ "issuer": "https://idp.example.com" }), + )] + } + fn jwks(&self) -> MetadataDocument { + MetadataDocument::new( + "/.well-known/jwks.json", + serde_json::json!({ "keys": [{ "kty": "OKP" }] }), + ) + } + async fn userinfo(&self, bearer: &str) -> Option { + (bearer == "good").then(|| serde_json::json!({ "sub": "user-1" })) + } +} + +fn oidc_app() -> Router { + oidc_backend_routes(Arc::new(TestOidc)).with_state(crate::test_state()) +} + +#[tokio::test] +async fn oidc_serves_discovery_and_jwks() { + let app = oidc_app(); + let disc = app + .clone() + .oneshot( + Request::get("/.well-known/openid-configuration") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(disc.status(), StatusCode::OK); + + let jwks = app + .oneshot( + Request::get("/.well-known/jwks.json") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(jwks.status(), StatusCode::OK); + assert_eq!(jwks.headers()["content-type"], "application/jwk-set+json"); +} + +#[tokio::test] +async fn oidc_userinfo_requires_valid_bearer() { + let app = oidc_app(); + let ok = app + .clone() + .oneshot( + Request::get("/userinfo") + .header("authorization", "Bearer good") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(ok.status(), StatusCode::OK); + assert!(body_string(ok).await.contains("user-1")); + + let bad = app + .oneshot( + Request::get("/userinfo") + .header("authorization", "Bearer wrong") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(bad.status(), StatusCode::UNAUTHORIZED); +} + +// --- extra_routes_router ------------------------------------------------- + +struct EchoHandler(&'static str); + +#[async_trait] +impl ExtraRouteHandler for EchoHandler { + async fn handle(&self, req: RouteRequest) -> RouteResponse { + let body = format!("{} {} {}", self.0, req.method, req.uri.path()); + RouteResponse::new(StatusCode::OK, Bytes::from(body)) + } +} + +#[tokio::test] +async fn extra_route_get_handler_runs() { + let routes = vec![ExtraRoute::new( + Method::GET, + "/custom", + Arc::new(EchoHandler("hit")), + )]; + let app = extra_routes_router(&routes).with_state(crate::test_state()); + let resp = app + .oneshot(Request::get("/custom").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(body_string(resp).await, "hit GET /custom"); +} + +#[tokio::test] +async fn extra_routes_merge_methods_on_same_path() { + // GET and POST on the same path must both work (merged MethodRouter), not + // panic on overlapping route registration. + let routes = vec![ + ExtraRoute::new(Method::GET, "/c", Arc::new(EchoHandler("g"))), + ExtraRoute::new(Method::POST, "/c", Arc::new(EchoHandler("p"))), + ]; + let app = extra_routes_router(&routes).with_state(crate::test_state()); + + let g = app + .clone() + .oneshot(Request::get("/c").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(body_string(g).await, "g GET /c"); + + let p = app + .oneshot(Request::post("/c").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(body_string(p).await, "p POST /c"); +} + +// --- edge cases: extra routes ------------------------------------------- + +#[tokio::test] +async fn extra_route_unsupported_method_is_skipped() { + // A non-standard extension method has no MethodFilter; the route is skipped + // (logged), not panicked, so the path is simply left unmounted. + let purge = Method::from_bytes(b"PURGE").unwrap(); + let routes = vec![ExtraRoute::new( + purge.clone(), + "/c", + Arc::new(EchoHandler("x")), + )]; + let app = extra_routes_router(&routes).with_state(crate::test_state()); + let resp = app + .oneshot( + Request::builder() + .method(purge) + .uri("/c") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); +} + +/// Echoes a request header and the body, to prove both reach the handler. +struct EchoBodyHandler; + +#[async_trait] +impl ExtraRouteHandler for EchoBodyHandler { + async fn handle(&self, req: RouteRequest) -> RouteResponse { + let hdr = req + .headers + .get("x-in") + .and_then(|v| v.to_str().ok()) + .unwrap_or(""); + let body = format!("{hdr}|{}", String::from_utf8_lossy(&req.body)); + RouteResponse::new(StatusCode::OK, Bytes::from(body)) + } +} + +#[tokio::test] +async fn extra_route_handler_receives_body_and_headers() { + let routes = vec![ExtraRoute::new( + Method::POST, + "/echo", + Arc::new(EchoBodyHandler), + )]; + let app = extra_routes_router(&routes).with_state(crate::test_state()); + let resp = app + .oneshot( + Request::post("/echo") + .header("x-in", "hdr-val") + .body(Body::from("payload")) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(body_string(resp).await, "hdr-val|payload"); +} + +// --- edge cases: verify + redirect -------------------------------------- + +#[tokio::test] +async fn verify_without_forwarding_headers_uses_request_target() { + // With no x-forwarded-*/x-original-*, the decision falls back to the verify + // request's own method and path. + let app = verify_app(decider(|req| { + if req.method == Method::GET && req.path == "/auth/verify" { + allow_with("x-ok", "1") + } else { + Decision::Deny { + status: StatusCode::FORBIDDEN, + body: Bytes::new(), + } + } + })); + let resp = app + .oneshot(Request::get("/auth/verify").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(resp.headers()["x-ok"], "1"); +} + +#[tokio::test] +async fn gate_redirect_with_invalid_location_omits_header() { + // A location that is not a valid header value yields the bare status, never + // a panic or a malformed header. + let app = gated_app(decider(|_| Decision::Redirect { + location: "bad\nlocation".to_string(), + })); + let resp = app + .oneshot(Request::get("/x").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FOUND); + assert!(!resp.headers().contains_key("location")); +} + +// --- edge cases: OIDC backend (consumer-configurable paths) -------------- + +/// A backend with a non-default userinfo path and two metadata documents, to +/// prove every OIDC path is supplied by the consumer, not hardcoded. +struct CustomPathOidc; + +#[async_trait] +impl OidcBackend for CustomPathOidc { + fn metadata_documents(&self) -> Vec { + vec![ + MetadataDocument::new( + "/.well-known/openid-configuration", + serde_json::json!({ "issuer": "https://idp" }), + ), + MetadataDocument::new( + "/.well-known/sid-configuration", + serde_json::json!({ "custom": true }), + ), + ] + } + fn jwks(&self) -> MetadataDocument { + MetadataDocument::new("/oauth/keys", serde_json::json!({ "keys": [] })) + } + fn userinfo_path(&self) -> String { + "/oauth/userinfo".to_string() + } + async fn userinfo(&self, _bearer: &str) -> Option { + Some(serde_json::json!({ "sub": "s" })) + } +} + +#[tokio::test] +async fn oidc_paths_are_consumer_configurable() { + let app = oidc_backend_routes(Arc::new(CustomPathOidc)).with_state(crate::test_state()); + + // Both metadata documents are served at their consumer-chosen paths. + for path in [ + "/.well-known/openid-configuration", + "/.well-known/sid-configuration", + ] { + let resp = app + .clone() + .oneshot(Request::get(path).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK, "missing metadata at {path}"); + } + + // JWKS at the consumer's custom path (not the default well-known path). + let jwks = app + .clone() + .oneshot(Request::get("/oauth/keys").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(jwks.status(), StatusCode::OK); + assert_eq!(jwks.headers()["content-type"], "application/jwk-set+json"); + + // UserInfo at the consumer's custom path; the default /userinfo is unmounted. + let custom = app + .clone() + .oneshot(Request::get("/oauth/userinfo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(custom.status(), StatusCode::OK); + + let default = app + .oneshot(Request::get("/userinfo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(default.status(), StatusCode::NOT_FOUND); +} diff --git a/src/hooks.rs b/src/hooks.rs new file mode 100644 index 0000000..fa2d2bf --- /dev/null +++ b/src/hooks.rs @@ -0,0 +1,193 @@ +//! Framework-agnostic extension points for embedding the proxy. +//! +//! These traits let an embedding crate inject *stateless* service-specific logic +//! (a forward-auth/PDP decision, an OIDC discovery/JWKS/userinfo backing, extra +//! routes) without naming an HTTP framework in its own code or `Cargo.toml`. +//! All signatures use the foundational [`http`] crate (already in the tree via +//! both `axum` and `tonic`), [`bytes::Bytes`], and `serde_json::Value` (never an +//! `axum` type), so `cargo tree -i axum` in an embedder shows axum only under +//! `structured-proxy`. +//! +//! Stateful concerns (BFF sessions, OIDC `authorize`/`token`) are deliberately +//! absent: the default build is a stateless data plane (see the crate README +//! Non-goals). They are planned behind an opt-in `bff` feature. + +use std::net::SocketAddr; +use std::sync::Arc; + +use async_trait::async_trait; +use bytes::Bytes; +use http::{HeaderMap, Method, StatusCode}; + +/// Borrowed view of an incoming request, passed to an [`AuthDecider`]. +/// +/// All fields borrow from the live request: building this is allocation-free, so +/// the per-request gate stays cheap. The body is intentionally absent: an auth +/// decision is taken from method, path, query, headers, and peer alone. +#[derive(Debug)] +pub struct RequestParts<'a> { + /// Request method (the *original* method on the `/verify` path, recovered + /// from the fronting proxy's forwarding headers). + pub method: &'a Method, + /// Request path, query stripped. + pub path: &'a str, + /// Raw query string, if any (without the leading `?`). + pub query: Option<&'a str>, + /// Request headers. + pub headers: &'a HeaderMap, + /// Direct peer socket address (the connecting client, or the fronting proxy). + pub peer: SocketAddr, +} + +/// The outcome of an [`AuthDecider`] evaluation. +pub enum Decision { + /// Allow the request; merge these (decider-controlled) headers onto it before + /// it continues upstream. The proxy strips any client-supplied copies of + /// these header names first, so a client cannot forge them. + Allow { + /// Headers to inject for the upstream (e.g. a verified `x-user-id`). + inject_headers: HeaderMap, + }, + /// Reject the request with this status and body (served as `application/json`). + Deny { + /// HTTP status to return (e.g. 401 / 403). + status: StatusCode, + /// Response body bytes. + body: Bytes, + }, + /// Redirect the client (e.g. to a login URL); returned as `302 Found`. + Redirect { + /// Absolute or relative `Location` URL. + location: String, + }, +} + +/// The per-request authorization gate. +/// +/// Implemented by the embedder for its forward-auth / policy-decision logic +/// (e.g. JWT verification + a policy engine + header translation). Called inline +/// on every proxied request *and* by the `/verify` forward-auth endpoint: same +/// trait, two call sites. +#[async_trait] +pub trait AuthDecider: Send + Sync { + /// Decide whether to allow, deny, or redirect the request. + async fn decide(&self, req: &RequestParts<'_>) -> Decision; +} + +/// A static JSON document served at a fixed path (an OIDC metadata document or a +/// JWKS document). +#[derive(Debug, Clone)] +pub struct MetadataDocument { + /// Path to serve at (e.g. `/.well-known/openid-configuration`). + pub path: String, + /// JSON body. + pub json: serde_json::Value, +} + +impl MetadataDocument { + /// Construct a metadata document. + pub fn new(path: impl Into, json: serde_json::Value) -> Self { + Self { + path: path.into(), + json, + } + } +} + +/// Backing for the *stateless* OIDC surface the proxy hosts. +/// +/// The proxy owns the HTTP routes (discovery, JWKS, userinfo); the embedder +/// supplies their content from its own key/client metadata. No `authorize` / +/// `token` here: those are stateful and out of scope for the data plane. +#[async_trait] +pub trait OidcBackend: Send + Sync { + /// Static metadata documents to serve as `GET` routes, e.g. the + /// `openid-configuration` and any provider-specific discovery document. + fn metadata_documents(&self) -> Vec; + + /// The JWKS document and the path it is advertised at. + fn jwks(&self) -> MetadataDocument; + + /// The path of the UserInfo endpoint. Defaults to `/userinfo`. + fn userinfo_path(&self) -> String { + "/userinfo".to_string() + } + + /// Resolve UserInfo claims for a verified bearer token. `None` yields `401`. + /// `bearer` is the raw token (the `Bearer ` prefix already stripped), or an + /// empty string when no credentials were presented. + async fn userinfo(&self, bearer: &str) -> Option; +} + +/// Owned view of a request handed to an [`ExtraRouteHandler`]. +/// +/// Unlike [`RequestParts`], this owns its data (including the full body), since +/// an extra route may consume the body to produce a response. +#[derive(Debug)] +pub struct RouteRequest { + /// Request method. + pub method: Method, + /// Full request URI (path + query). + pub uri: http::Uri, + /// Request headers. + pub headers: HeaderMap, + /// Request body bytes. + pub body: Bytes, + /// Direct peer socket address. + pub peer: SocketAddr, +} + +/// Response produced by an [`ExtraRouteHandler`]. +pub struct RouteResponse { + /// HTTP status. + pub status: StatusCode, + /// Response headers. + pub headers: HeaderMap, + /// Response body bytes. + pub body: Bytes, +} + +impl RouteResponse { + /// A response with the given status and body and no extra headers. + pub fn new(status: StatusCode, body: impl Into) -> Self { + Self { + status, + headers: HeaderMap::new(), + body: body.into(), + } + } +} + +/// A stateless handler for an extra route registered via +/// [`ProxyServer::with_extra_routes`](crate::ProxyServer::with_extra_routes). +/// +/// The framework-agnostic seam (request parts in, response parts out) the +/// embedder uses for service-specific endpoints without naming `axum`. +#[async_trait] +pub trait ExtraRouteHandler: Send + Sync { + /// Handle a request and produce a response. + async fn handle(&self, req: RouteRequest) -> RouteResponse; +} + +/// A single extra route: a method, a path, and the handler to run. +#[derive(Clone)] +pub struct ExtraRoute { + pub(crate) method: Method, + pub(crate) path: String, + pub(crate) handler: Arc, +} + +impl ExtraRoute { + /// Register `handler` for `method` requests to `path`. + pub fn new( + method: Method, + path: impl Into, + handler: Arc, + ) -> Self { + Self { + method, + path: path.into(), + handler, + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 04ede9a..38d3928 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -27,6 +27,8 @@ compile_error!("exactly one JWT crypto backend must be enabled: `rust_crypto` or pub mod auth; pub mod config; +mod embed; +pub mod hooks; pub mod oidc; pub mod openapi; pub mod shield; @@ -43,7 +45,10 @@ use std::net::SocketAddr; use tower_http::cors::{AllowOrigin, CorsLayer}; use tower_http::trace::TraceLayer; +use std::sync::Arc; + use config::{DescriptorSource, ProxyConfig}; +use hooks::{AuthDecider, ExtraRoute, OidcBackend}; /// Shared state for all proxy handlers. #[derive(Clone, Debug)] @@ -75,6 +80,14 @@ pub struct ProxyServer { config: ProxyConfig, /// Optional pre-loaded descriptor pool (for embedded mode). descriptor_pool: Option, + /// Optional in-process forward-auth/PDP gate (embedded Tier-2 hook). + auth_decider: Option>, + /// Optional stateless OIDC surface backing (embedded Tier-2 hook). + oidc_backend: Option>, + /// Embedder-supplied extra stateless routes (embedded Tier-2 hook). + extra_routes: Vec, + /// Override for the `/verify` forward-auth path of an injected AuthDecider. + verify_path: Option, } impl ProxyServer { @@ -83,6 +96,10 @@ impl ProxyServer { Self { config, descriptor_pool: None, + auth_decider: None, + oidc_backend: None, + extra_routes: Vec::new(), + verify_path: None, } } @@ -92,6 +109,44 @@ impl ProxyServer { self } + /// Inject an in-process forward-auth / PDP decision (embedded Tier-2 hook). + /// + /// The decider gates every proxied request inline and also backs the + /// `/verify` forward-auth endpoint. Its signature is `axum`-free (see + /// [`hooks::AuthDecider`]), so the embedder never names an HTTP framework. + pub fn with_auth_decider(mut self, decider: Arc) -> Self { + self.auth_decider = Some(decider); + self + } + + /// Back the stateless OIDC surface (discovery, JWKS, userinfo) with the + /// embedder's key/client metadata (embedded Tier-2 hook). + /// + /// When set, this supersedes the config-driven static `oidc_discovery` + /// routes. See [`hooks::OidcBackend`]. + pub fn with_oidc_backend(mut self, backend: Arc) -> Self { + self.oidc_backend = Some(backend); + self + } + + /// Register extra stateless routes through an `axum`-free adapter (embedded + /// Tier-2 hook). See [`hooks::ExtraRoute`] / [`hooks::ExtraRouteHandler`]. + pub fn with_extra_routes(mut self, routes: impl IntoIterator) -> Self { + self.extra_routes.extend(routes); + self + } + + /// Set the path at which the injected [`AuthDecider`] answers forward-auth + /// sub-requests (`/verify`). Independent of any JWT `forward_auth` config, so + /// a decider-only embedder can place it without a JWT block. + /// + /// Resolution order for the path: this override, then + /// `auth.forward_auth.path` from config, then the default `/auth/verify`. + pub fn with_verify_path(mut self, path: impl Into) -> Self { + self.verify_path = Some(path.into()); + self + } + /// Load descriptor pool from configured sources. /// /// Multiple descriptor files are merged into a single pool, @@ -183,51 +238,70 @@ impl ProxyServer { )); } - // Health routes - let health_service_name = service_name.clone(); - let health_routes = Router::new() - .route( - "/health", - get({ - let name = health_service_name.clone(); - move || async move { - Json(serde_json::json!({ - "status": "ok", - "service": name, - })) - } - }), - ) - .route("/health/live", get(|| async { StatusCode::OK })) - .route( - "/health/ready", - get(|State(state): State| async move { - let mut client = - tonic_health::pb::health_client::HealthClient::new(state.grpc_channel); - match client - .check(tonic_health::pb::HealthCheckRequest { - service: String::new(), - }) - .await - { - Ok(resp) => { - let status = resp.into_inner().status; - if status - == tonic_health::pb::health_check_response::ServingStatus::Serving - as i32 - { - StatusCode::OK - } else { - StatusCode::SERVICE_UNAVAILABLE + // Embedded Tier-2 in-process gate: an injected AuthDecider runs inline on + // the proxied routes (it sees the JWT-injected identity headers, like the + // ext_authz layer above). Added after authz so authz, when both are set, + // runs first (inner layer). + if let Some(decider) = &self.auth_decider { + transcode_routes = transcode_routes.layer(axum::middleware::from_fn_with_state( + decider.clone(), + embed::auth_decider_gate, + )); + } + + // Health routes. Paths are configurable; the whole group is skippable. + let health_routes = if self.config.health.enabled { + let health = &self.config.health; + let health_service_name = service_name.clone(); + Router::new() + .route( + &health.path, + get({ + let name = health_service_name.clone(); + move || async move { + Json(serde_json::json!({ + "status": "ok", + "service": name, + })) + } + }), + ) + .route(&health.live_path, get(|| async { StatusCode::OK })) + .route( + &health.ready_path, + get(|State(state): State| async move { + let mut client = + tonic_health::pb::health_client::HealthClient::new(state.grpc_channel); + match client + .check(tonic_health::pb::HealthCheckRequest { + service: String::new(), + }) + .await + { + Ok(resp) => { + let status = resp.into_inner().status; + if status + == tonic_health::pb::health_check_response::ServingStatus::Serving + as i32 + { + StatusCode::OK + } else { + StatusCode::SERVICE_UNAVAILABLE + } } + Err(_) => StatusCode::SERVICE_UNAVAILABLE, } - Err(_) => StatusCode::SERVICE_UNAVAILABLE, - } - }), - ) - .route("/health/startup", get(|| async { StatusCode::OK })) - .route( - "/metrics", + }), + ) + .route(&health.startup_path, get(|| async { StatusCode::OK })) + } else { + Router::new() + }; + + // Metrics route. Path is configurable; the endpoint is skippable. + let metrics_routes = if self.config.metrics.enabled { + Router::new().route( + &self.config.metrics.path, get(|| async { let encoder = prometheus::TextEncoder::new(); let metric_families = prometheus::default_registry().gather(); @@ -244,18 +318,26 @@ impl ProxyServer { Err(_) => StatusCode::INTERNAL_SERVER_ERROR.into_response(), } }), - ); + ) + } else { + Router::new() + }; // OpenAPI + docs routes (if enabled). let openapi_routes = self.build_openapi_routes(&pool); - // OIDC discovery routes (if enabled). Public, like the health endpoints. - let oidc_routes = match &self.config.oidc_discovery { - Some(cfg) => oidc::Oidc::build(cfg) - .map_err(|e| anyhow::anyhow!("invalid oidc_discovery config: {e}"))? - .map(|o| o.routes()) - .unwrap_or_default(), - None => Router::new(), + // OIDC routes (public, like the health endpoints). An injected + // OidcBackend supersedes the config-driven static discovery: the proxy + // hosts the HTTP surface, the embedder supplies the content. + let oidc_routes = match &self.oidc_backend { + Some(backend) => embed::oidc_backend_routes(backend.clone()), + None => match &self.config.oidc_discovery { + Some(cfg) => oidc::Oidc::build(cfg) + .map_err(|e| anyhow::anyhow!("invalid oidc_discovery config: {e}"))? + .map(|o| o.routes()) + .unwrap_or_default(), + None => Router::new(), + }, }; // Rate limiting (Shield), if configured and enabled. @@ -275,8 +357,10 @@ impl ProxyServer { let mut router = Router::new() .merge(health_routes) + .merge(metrics_routes) .merge(openapi_routes) .merge(oidc_routes) + .merge(embed::extra_routes_router(&self.extra_routes)) .merge(transcode_routes) .layer(cors); @@ -293,7 +377,27 @@ impl ProxyServer { router = router.layer(axum::middleware::from_fn_with_state(auth, auth::middleware)); } - if let Some(forward_auth) = &forward_auth { + // Forward-auth `/verify` endpoint. An injected AuthDecider owns it when + // present (in-process PDP); otherwise the config-driven JWT ForwardAuth + // backs it. Mounted after the auth layer so it is not itself JWT-gated. + if let Some(decider) = &self.auth_decider { + let verify_path = self.verify_path.clone().unwrap_or_else(|| { + self.config + .auth + .as_ref() + .and_then(|a| a.forward_auth.as_ref()) + .map(|fa| fa.path.clone()) + .unwrap_or_else(|| "/auth/verify".to_string()) + }); + let decider = decider.clone(); + router = router.route( + &verify_path, + axum::routing::any(move |req: axum::extract::Request| { + let decider = decider.clone(); + async move { embed::verify_via_decider(decider, req).await } + }), + ); + } else if let Some(forward_auth) = &forward_auth { router = router.merge(forward_auth.routes()); } @@ -436,6 +540,24 @@ pub(crate) fn test_channel() -> tonic::transport::Channel { .connect_lazy() } +/// A minimal [`ProxyState`] for tests that only need a state to satisfy a +/// `Router` (the hook routers do not read it). +#[cfg(test)] +pub(crate) fn test_state() -> ProxyState { + ProxyState { + service_name: "test".into(), + grpc_upstream: "http://127.0.0.1:1".into(), + grpc_channel: test_channel(), + maintenance_mode: false, + maintenance_exempt: vec![], + maintenance_message: String::new(), + forwarded_headers: vec![], + metrics_namespace: "test".into(), + metrics_classes: vec![], + sse_keep_alive_secs: 15, + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/shield/store.rs b/src/shield/store.rs index 204e137..be72866 100644 --- a/src/shield/store.rs +++ b/src/shield/store.rs @@ -1,10 +1,18 @@ //! Rate-limit counter storage. //! //! [`RateLimitStore`] abstracts where per-key counters live. [`MemoryStore`] is -//! the default and keeps counters in-process (per replica). [`RedisStore`] -//! (behind the `redis` feature) shares counters across replicas, which is what -//! a multi-instance deployment behind a load balancer needs for correct global -//! limits. +//! the default and keeps counters in-process (per replica). +// The `RedisStore` reference is an intra-doc link only when the `redis` feature +// (and thus the type) is compiled in; otherwise a plain code span, so `cargo +// doc` stays warning-free in every feature combination. +#![cfg_attr( + feature = "redis", + doc = "[`RedisStore`] (behind the `redis` feature) shares counters across replicas, which is what a multi-instance deployment behind a load balancer needs for correct global limits." +)] +#![cfg_attr( + not(feature = "redis"), + doc = "`RedisStore` (behind the `redis` feature) shares counters across replicas, which is what a multi-instance deployment behind a load balancer needs for correct global limits." +)] use std::time::Duration; @@ -36,7 +44,15 @@ pub trait RateLimitStore: Send + Sync { /// In-process fixed-window counter store (per replica). /// /// Counters are not shared between replicas, so global limits only hold for a -/// single instance. Use [`RedisStore`] for multi-instance deployments. +/// single instance. +#[cfg_attr( + feature = "redis", + doc = "Use [`RedisStore`] for multi-instance deployments." +)] +#[cfg_attr( + not(feature = "redis"), + doc = "Use `RedisStore` (feature `redis`) for multi-instance deployments." +)] #[derive(Debug)] pub struct MemoryStore { // no-std: caller-provided Clock + spin/hashbrown map. diff --git a/tests/embedded.rs b/tests/embedded.rs index 1b3553f..06d2f1f 100644 --- a/tests/embedded.rs +++ b/tests/embedded.rs @@ -33,6 +33,8 @@ fn embedded_config_is_constructible() { auth: None, shield: None, oidc_discovery: None, + health: Default::default(), + metrics: Default::default(), maintenance: Default::default(), cors: Default::default(), logging: Default::default(), diff --git a/tests/hooks.rs b/tests/hooks.rs new file mode 100644 index 0000000..d29d73e --- /dev/null +++ b/tests/hooks.rs @@ -0,0 +1,309 @@ +//! End-to-end test of the embedded Tier-2 hooks through the public API. +//! +//! Acceptance-criterion guard: the hook implementations below are written using +//! only `http`, `bytes`, `serde_json`, and `async-trait` (the crates a real +//! embedder uses), and reference **no `axum` type**. The proxy wires them into +//! its router via [`ProxyServer`]; the test then drives that router with axum + +//! tower purely as the assertion harness (that is the proxy's concern, not the +//! embedder's). + +use std::sync::Arc; + +use async_trait::async_trait; +use http::{HeaderMap, Method, StatusCode}; +use structured_proxy::config::ProxyConfig; +use structured_proxy::hooks::{ + AuthDecider, Decision, ExtraRoute, ExtraRouteHandler, MetadataDocument, OidcBackend, + RequestParts, RouteRequest, RouteResponse, +}; +use structured_proxy::ProxyServer; + +// --- embedder-supplied hook impls (axum-free) ---------------------------- + +/// Allows `/v1/public/**`, injects a verified `x-user` for everything else, and +/// redirects an explicit `/login`. Mirrors a real PDP shape (path + headers in, +/// decision out) without any framework types. +struct DemoDecider; + +#[async_trait] +impl AuthDecider for DemoDecider { + async fn decide(&self, req: &RequestParts<'_>) -> Decision { + if req.path == "/login" { + return Decision::Redirect { + location: "https://login.example.com".to_string(), + }; + } + if req.path.starts_with("/v1/public/") { + return Decision::Allow { + inject_headers: HeaderMap::new(), + }; + } + if req.headers.get("authorization").is_some() { + let mut h = HeaderMap::new(); + h.insert("x-user", "verified-user".parse().unwrap()); + Decision::Allow { inject_headers: h } + } else { + Decision::Deny { + status: StatusCode::UNAUTHORIZED, + body: bytes::Bytes::from_static(b"{\"error\":\"unauthenticated\"}"), + } + } + } +} + +struct DemoOidc; + +#[async_trait] +impl OidcBackend for DemoOidc { + fn metadata_documents(&self) -> Vec { + vec![MetadataDocument::new( + "/.well-known/openid-configuration", + serde_json::json!({ "issuer": "https://idp.example.com" }), + )] + } + fn jwks(&self) -> MetadataDocument { + MetadataDocument::new("/.well-known/jwks.json", serde_json::json!({ "keys": [] })) + } + async fn userinfo(&self, bearer: &str) -> Option { + (bearer == "token-123").then(|| serde_json::json!({ "sub": "user-1", "email": "u@x" })) + } +} + +struct PingHandler; + +#[async_trait] +impl ExtraRouteHandler for PingHandler { + async fn handle(&self, _req: RouteRequest) -> RouteResponse { + RouteResponse::new(StatusCode::OK, bytes::Bytes::from_static(b"pong")) + } +} + +// --- harness (axum + tower) ---------------------------------------------- + +use axum::body::Body; +use tower::ServiceExt; + +fn server() -> ProxyServer { + let config = ProxyConfig::from_yaml_str( + r#" +upstream: + default: "http://127.0.0.1:50051" +service: + name: "hooks-test" +"#, + ) + .unwrap(); + + ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .with_oidc_backend(Arc::new(DemoOidc)) + .with_verify_path("/auth/verify") + .with_extra_routes([ExtraRoute::new(Method::GET, "/ping", Arc::new(PingHandler))]) +} + +async fn body_string(resp: axum::response::Response) -> String { + let bytes = axum::body::to_bytes(resp.into_body(), 64 * 1024) + .await + .unwrap(); + String::from_utf8(bytes.to_vec()).unwrap() +} + +#[tokio::test] +async fn verify_endpoint_is_backed_by_the_decider() { + let app = server().router().unwrap(); + + // Authenticated original request → 200 with the injected identity. + let ok = app + .clone() + .oneshot( + axum::http::Request::get("/auth/verify") + .header("x-forwarded-method", "GET") + .header("x-forwarded-uri", "/v1/things") + .header("authorization", "Bearer whatever") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(ok.status(), StatusCode::OK); + assert_eq!(ok.headers()["x-user"], "verified-user"); + + // No credentials → the decider denies. + let denied = app + .oneshot( + axum::http::Request::get("/auth/verify") + .header("x-forwarded-method", "GET") + .header("x-forwarded-uri", "/v1/things") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(denied.status(), StatusCode::UNAUTHORIZED); +} + +#[tokio::test] +async fn verify_redirect_becomes_401_with_location() { + let app = server().router().unwrap(); + let resp = app + .oneshot( + axum::http::Request::get("/auth/verify") + .header("x-forwarded-method", "GET") + .header("x-forwarded-uri", "/login") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert_eq!(resp.headers()["location"], "https://login.example.com"); +} + +#[tokio::test] +async fn oidc_backend_surface_is_served() { + let app = server().router().unwrap(); + + let disc = app + .clone() + .oneshot( + axum::http::Request::get("/.well-known/openid-configuration") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(disc.status(), StatusCode::OK); + assert!(body_string(disc).await.contains("idp.example.com")); + + let jwks = app + .clone() + .oneshot( + axum::http::Request::get("/.well-known/jwks.json") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(jwks.status(), StatusCode::OK); + assert_eq!(jwks.headers()["content-type"], "application/jwk-set+json"); + + let userinfo = app + .oneshot( + axum::http::Request::get("/userinfo") + .header("authorization", "Bearer token-123") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(userinfo.status(), StatusCode::OK); + assert!(body_string(userinfo).await.contains("user-1")); +} + +#[tokio::test] +async fn verify_path_defaults_when_not_configured() { + // No with_verify_path and no JWT forward_auth config: the decider still + // answers at the default /auth/verify. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let app = ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .router() + .unwrap(); + let resp = app + .oneshot( + axum::http::Request::get("/auth/verify") + .header("x-forwarded-uri", "/v1/public/info") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); +} + +#[tokio::test] +async fn health_and_metrics_paths_are_configurable() { + // Relocate the probes and metrics, and confirm the defaults no longer exist. + let config = ProxyConfig::from_yaml_str( + r#" +upstream: + default: "http://127.0.0.1:50051" +health: + path: "/internal/health" + live_path: "/internal/health/live" +metrics: + path: "/internal/metrics" +"#, + ) + .unwrap(); + let app = ProxyServer::from_config(config).router().unwrap(); + + for path in [ + "/internal/health", + "/internal/health/live", + "/internal/metrics", + ] { + let resp = app + .clone() + .oneshot(axum::http::Request::get(path).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK, "expected route at {path}"); + } + + // The default paths are gone now that they were relocated. + let default_health = app + .oneshot( + axum::http::Request::get("/health") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(default_health.status(), StatusCode::NOT_FOUND); +} + +#[tokio::test] +async fn health_and_metrics_can_be_disabled() { + let config = ProxyConfig::from_yaml_str( + r#" +upstream: + default: "http://127.0.0.1:50051" +health: + enabled: false +metrics: + enabled: false +"#, + ) + .unwrap(); + let app = ProxyServer::from_config(config).router().unwrap(); + + for path in ["/health", "/health/live", "/metrics"] { + let resp = app + .clone() + .oneshot(axum::http::Request::get(path).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "{path} should be unmounted when disabled" + ); + } +} + +#[tokio::test] +async fn extra_route_is_mounted() { + let app = server().router().unwrap(); + let resp = app + .oneshot( + axum::http::Request::get("/ping") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(body_string(resp).await, "pong"); +} From eaeeee01f1c9dd164ce92f38cfc8ce1085d68d63 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 22:56:59 +0300 Subject: [PATCH 02/11] fix(embed): harden extra-route body read, userinfo challenge, bearer scheme - Reject oversized/unreadable extra-route bodies with 413 instead of passing an empty payload to the handler (a body-parsing handler would misread it) - Add RFC 6750 WWW-Authenticate Bearer challenge to userinfo 401 responses (plain Bearer when no credentials, error="invalid_token" when rejected) - Match the Authorization Bearer scheme case-insensitively --- src/embed.rs | 68 +++++++++++++++++++++++++++++++++++----------- src/embed/tests.rs | 64 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 16 deletions(-) diff --git a/src/embed.rs b/src/embed.rs index da76ec6..d20f07b 100644 --- a/src/embed.rs +++ b/src/embed.rs @@ -12,7 +12,7 @@ use std::sync::Arc; use axum::body::Body; use axum::extract::{ConnectInfo, Request, State}; -use axum::http::header::{CONTENT_TYPE, LOCATION}; +use axum::http::header::{CONTENT_TYPE, LOCATION, WWW_AUTHENTICATE}; use axum::http::{HeaderMap, StatusCode}; use axum::middleware::Next; use axum::response::{IntoResponse, Response}; @@ -150,15 +150,25 @@ pub(crate) fn oidc_backend_routes(backend: Arc) -> Router Json(claims).into_response(), - None => deny_response( - StatusCode::UNAUTHORIZED, - bytes::Bytes::from_static( - br#"{"error":"invalid_token","message":"invalid or expired token"}"#, - ), - ), + // RFC 6750 §3: a Bearer challenge lets clients classify the + // failure. Plain `Bearer` when no credentials were sent; + // `error="invalid_token"` when a presented token was rejected. + None => { + let challenge = if token.is_some() { + r#"Bearer error="invalid_token""# + } else { + "Bearer" + }; + unauthorized_with_challenge( + bytes::Bytes::from_static( + br#"{"error":"invalid_token","message":"invalid or expired token"}"#, + ), + challenge, + ) + } } } }), @@ -188,9 +198,20 @@ pub(crate) fn extra_routes_router(routes: &[ExtraRoute]) -> Router { async move { let peer = peer_of(&request); let (parts, body) = request.into_parts(); - let body = axum::body::to_bytes(body, MAX_EXTRA_ROUTE_BODY) - .await - .unwrap_or_default(); + // A failed/oversized read must NOT reach the handler as an empty + // body: a handler that verifies or parses the body would treat + // the truncation as a real empty payload. Reject instead. + let body = match axum::body::to_bytes(body, MAX_EXTRA_ROUTE_BODY).await { + Ok(bytes) => bytes, + Err(_) => { + return deny_response( + StatusCode::PAYLOAD_TOO_LARGE, + bytes::Bytes::from_static( + br#"{"error":"payload_too_large","message":"request body exceeded limit or could not be read"}"#, + ), + ) + } + }; let resp = handler .handle(RouteRequest { method: parts.method, @@ -239,6 +260,19 @@ fn deny_response(status: StatusCode, body: bytes::Bytes) -> Response { (status, [(CONTENT_TYPE, "application/json")], body).into_response() } +/// A `401` JSON response carrying an RFC 6750 `WWW-Authenticate` Bearer challenge. +fn unauthorized_with_challenge(body: bytes::Bytes, challenge: &'static str) -> Response { + ( + StatusCode::UNAUTHORIZED, + [ + (CONTENT_TYPE, "application/json"), + (WWW_AUTHENTICATE, challenge), + ], + body, + ) + .into_response() +} + /// Render a `Decision::Redirect` at the given status with a `Location` header. /// A malformed `location` (not a valid header value) yields the bare status. fn redirect_response(status: StatusCode, location: &str) -> Response { @@ -250,12 +284,14 @@ fn redirect_response(status: StatusCode, location: &str) -> Response { } /// The bearer token from an `Authorization` header (prefix stripped), if present. +/// The `Bearer` scheme name is matched case-insensitively per RFC 7235. fn bearer_token(headers: &HeaderMap) -> Option { let value = headers.get("authorization")?.to_str().ok()?; - let token = value - .strip_prefix("Bearer ") - .or_else(|| value.strip_prefix("bearer "))? - .trim(); + let (scheme, rest) = value.split_once(' ')?; + if !scheme.eq_ignore_ascii_case("bearer") { + return None; + } + let token = rest.trim(); (!token.is_empty()).then(|| token.to_string()) } diff --git a/src/embed/tests.rs b/src/embed/tests.rs index 2e09319..e348548 100644 --- a/src/embed/tests.rs +++ b/src/embed/tests.rs @@ -64,6 +64,11 @@ fn bearer_token_parses_either_case_and_rejects_other_schemes() { lower.insert("authorization", "bearer xyz".parse().unwrap()); assert_eq!(bearer_token(&lower).as_deref(), Some("xyz")); + // Scheme name is case-insensitive (RFC 7235). + let mut upper = HeaderMap::new(); + upper.insert("authorization", "BEARER tok".parse().unwrap()); + assert_eq!(bearer_token(&upper).as_deref(), Some("tok")); + let mut basic = HeaderMap::new(); basic.insert("authorization", "Basic xyz".parse().unwrap()); assert_eq!(bearer_token(&basic), None); @@ -588,3 +593,62 @@ async fn oidc_paths_are_consumer_configurable() { .unwrap(); assert_eq!(default.status(), StatusCode::NOT_FOUND); } + +// --- edge cases: userinfo WWW-Authenticate challenge -------------------- + +#[tokio::test] +async fn userinfo_401_carries_bearer_challenge() { + let app = oidc_backend_routes(Arc::new(TestOidc)).with_state(crate::test_state()); + + // No credentials → plain `Bearer` challenge. + let missing = app + .clone() + .oneshot(Request::get("/userinfo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(missing.status(), StatusCode::UNAUTHORIZED); + assert_eq!(missing.headers()["www-authenticate"], "Bearer"); + + // Presented-but-rejected token → invalid_token challenge. + let bad = app + .oneshot( + Request::get("/userinfo") + .header("authorization", "Bearer wrong") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(bad.status(), StatusCode::UNAUTHORIZED); + assert_eq!( + bad.headers()["www-authenticate"], + "Bearer error=\"invalid_token\"" + ); +} + +// --- edge cases: extra-route oversized body ----------------------------- + +#[tokio::test] +async fn extra_route_oversized_body_is_rejected_not_emptied() { + // A body past MAX_EXTRA_ROUTE_BODY must yield 413, never reach the handler + // as an empty payload (which a body-parsing handler would misread). + let routes = vec![ExtraRoute::new( + Method::POST, + "/echo", + Arc::new(EchoBodyHandler), + )]; + let app = extra_routes_router(&routes).with_state(crate::test_state()); + let oversized = vec![b'x'; MAX_EXTRA_ROUTE_BODY + 1]; + let resp = app + .oneshot( + Request::post("/echo") + .header("x-in", "h") + .body(Body::from(oversized)) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::PAYLOAD_TOO_LARGE); + // The handler's echo body ("h|...") must NOT appear: it never ran. + assert!(!body_string(resp).await.starts_with("h|")); +} From ede1678a59e249121d0fe497d4eaf83d8302be27 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 22:57:10 +0300 Subject: [PATCH 03/11] fix(config): dedupe edge paths, keep relocated paths exempt and collision-free - Reject duplicate built-in endpoint paths at config load (e.g. health.path equal to the default live_path) instead of panicking on duplicate route registration - Derive the maintenance exemption list from the resolved configured health, metrics, and verify paths so relocating them does not 503 probe/forward-auth traffic that was exempt at the default paths - Fail with a clear error when the verify path collides with a built-in GET endpoint path --- src/config.rs | 58 ++++++++++++++++++++++++++++++++++++ src/lib.rs | 75 +++++++++++++++++++++++++++++++++++++++++------ tests/hooks.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 9 deletions(-) diff --git a/src/config.rs b/src/config.rs index 2483450..4877ba4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -636,6 +636,30 @@ impl ProxyConfig { if self.streaming.sse_keep_alive_secs == 0 { anyhow::bail!("streaming.sse_keep_alive_secs must be greater than 0"); } + self.validate_edge_paths()?; + Ok(()) + } + + /// Reject duplicate built-in edge paths up front, so the router does not + /// panic at construction registering the same path twice (e.g. setting + /// `health.path` to the default `live_path`). + fn validate_edge_paths(&self) -> anyhow::Result<()> { + let mut seen = std::collections::HashSet::new(); + let mut check = |label: &str, path: &str| -> anyhow::Result<()> { + if !seen.insert(path.to_string()) { + anyhow::bail!("duplicate endpoint path {path:?} ({label}); each built-in endpoint must have a distinct path"); + } + Ok(()) + }; + if self.health.enabled { + check("health.path", &self.health.path)?; + check("health.live_path", &self.health.live_path)?; + check("health.ready_path", &self.health.ready_path)?; + check("health.startup_path", &self.health.startup_path)?; + } + if self.metrics.enabled { + check("metrics.path", &self.metrics.path)?; + } Ok(()) } @@ -698,6 +722,40 @@ metrics: assert_eq!(cfg.metrics.path, "/internal/metrics"); } + #[test] + fn duplicate_probe_paths_are_rejected() { + // health.path set to the default live_path collides on a single GET + // route; reject at load instead of panicking in the router. + let yaml = r#" +upstream: + default: "grpc://x:1" +health: + path: "/health/live" +"#; + let err = ProxyConfig::from_yaml_str(yaml).unwrap_err(); + assert!(err.to_string().contains("duplicate endpoint path")); + + // A health path colliding with the metrics path is also rejected. + let yaml2 = r#" +upstream: + default: "grpc://x:1" +metrics: + path: "/health" +"#; + let err2 = ProxyConfig::from_yaml_str(yaml2).unwrap_err(); + assert!(err2.to_string().contains("duplicate endpoint path")); + + // Disabling a group frees its paths from the collision check. + let yaml3 = r#" +upstream: + default: "grpc://x:1" +health: + enabled: false + path: "/metrics" +"#; + assert!(ProxyConfig::from_yaml_str(yaml3).is_ok()); + } + #[test] fn test_zero_sse_keep_alive_is_rejected() { // A zero keep-alive would make axum's SSE timer fire continuously diff --git a/src/lib.rs b/src/lib.rs index 38d3928..4370e4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -187,6 +187,35 @@ impl ProxyServer { Ok(pool) } + /// Resolve the `/verify` forward-auth path: the `with_verify_path` override, + /// then `auth.forward_auth.path` from config, then the default `/auth/verify`. + fn resolved_verify_path(&self) -> String { + self.verify_path.clone().unwrap_or_else(|| { + self.config + .auth + .as_ref() + .and_then(|a| a.forward_auth.as_ref()) + .map(|fa| fa.path.clone()) + .unwrap_or_else(|| "/auth/verify".to_string()) + }) + } + + /// The built-in `GET` paths that are actually mounted (enabled health probes + /// and metrics), used to detect collisions before registering more routes. + fn builtin_get_paths(&self) -> Vec { + let mut paths = Vec::new(); + if self.config.health.enabled { + paths.push(self.config.health.path.clone()); + paths.push(self.config.health.live_path.clone()); + paths.push(self.config.health.ready_path.clone()); + paths.push(self.config.health.startup_path.clone()); + } + if self.config.metrics.enabled { + paths.push(self.config.metrics.path.clone()); + } + paths + } + /// Build the axum router with all endpoints. pub fn router(&self) -> anyhow::Result { // Enforce cross-field invariants on the embedded path too, where the @@ -204,12 +233,40 @@ impl ProxyServer { let service_name = self.config.service.name.clone(); let metrics_namespace = service_name.replace('-', "_"); + let verify_path = self.resolved_verify_path(); + + // Keep the actually-configured probe / metrics / verify paths reachable + // under maintenance mode. The default exempt list names the default + // paths; once those are relocated via config, the relocated paths must + // be exempted too, or maintenance would 503 probe and forward-auth + // traffic that was intentionally exempt before. + let mut maintenance_exempt = self.config.maintenance.exempt_paths.clone(); + if self.config.health.enabled { + maintenance_exempt.push(self.config.health.path.clone()); + maintenance_exempt.push(self.config.health.live_path.clone()); + maintenance_exempt.push(self.config.health.ready_path.clone()); + maintenance_exempt.push(self.config.health.startup_path.clone()); + } + if self.config.metrics.enabled { + maintenance_exempt.push(self.config.metrics.path.clone()); + } + if self.auth_decider.is_some() + || self + .config + .auth + .as_ref() + .and_then(|a| a.forward_auth.as_ref()) + .is_some_and(|fa| fa.enabled) + { + maintenance_exempt.push(verify_path.clone()); + } + let state = ProxyState { service_name: service_name.clone(), grpc_upstream, grpc_channel, maintenance_mode: self.config.maintenance.enabled, - maintenance_exempt: self.config.maintenance.exempt_paths.clone(), + maintenance_exempt, maintenance_message: self.config.maintenance.message.clone(), forwarded_headers: self.config.forwarded_headers.clone(), metrics_namespace, @@ -381,14 +438,14 @@ impl ProxyServer { // present (in-process PDP); otherwise the config-driven JWT ForwardAuth // backs it. Mounted after the auth layer so it is not itself JWT-gated. if let Some(decider) = &self.auth_decider { - let verify_path = self.verify_path.clone().unwrap_or_else(|| { - self.config - .auth - .as_ref() - .and_then(|a| a.forward_auth.as_ref()) - .map(|fa| fa.path.clone()) - .unwrap_or_else(|| "/auth/verify".to_string()) - }); + // Guard against the verify path colliding with a built-in GET route + // (axum panics on duplicate path registration); fail with a clear + // error instead. `verify_path` was resolved once above. + if self.builtin_get_paths().iter().any(|p| p == &verify_path) { + anyhow::bail!( + "verify path {verify_path:?} collides with a health/metrics endpoint path" + ); + } let decider = decider.clone(); router = router.route( &verify_path, diff --git a/tests/hooks.rs b/tests/hooks.rs index d29d73e..7e74415 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -293,6 +293,85 @@ metrics: } } +#[tokio::test] +async fn verify_path_colliding_with_probe_is_a_clean_error() { + // A verify path that collides with a built-in GET route must surface a + // config error, not an axum duplicate-route panic. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let result = ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .with_verify_path("/health") + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("collides with a health/metrics endpoint")); +} + +#[tokio::test] +async fn relocated_paths_stay_exempt_under_maintenance() { + // With maintenance enabled, relocated probe / metrics / verify paths must + // stay reachable (they were exempt at their default locations). + let config = ProxyConfig::from_yaml_str( + r#" +upstream: + default: "http://127.0.0.1:50051" +maintenance: + enabled: true +health: + path: "/internal/health" +metrics: + path: "/internal/metrics" +"#, + ) + .unwrap(); + let app = ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .with_verify_path("/internal/verify") + .router() + .unwrap(); + + // Probe + metrics reachable despite maintenance. + for path in ["/internal/health", "/internal/metrics"] { + let resp = app + .clone() + .oneshot(axum::http::Request::get(path).body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "{path} blocked by maintenance" + ); + } + + // Relocated verify endpoint is exempt and answers the decider's decision. + let verify = app + .clone() + .oneshot( + axum::http::Request::get("/internal/verify") + .header("x-forwarded-uri", "/v1/public/info") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(verify.status(), StatusCode::OK); + + // A non-exempt proxied path still gets the 503 maintenance response. + let blocked = app + .oneshot( + axum::http::Request::get("/v1/anything") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(blocked.status(), StatusCode::SERVICE_UNAVAILABLE); +} + #[tokio::test] async fn extra_route_is_mounted() { let app = server().router().unwrap(); From 8a58a4bae99c2309822ac0983eac02b6c8daea41 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 22:57:19 +0300 Subject: [PATCH 04/11] docs(readme): note async-trait is required for embedding hooks The hook traits are #[async_trait], so embedders need async-trait alongside http/bytes/serde_json. None is an HTTP framework. --- README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 1922069..d0bd61e 100644 --- a/README.md +++ b/README.md @@ -202,8 +202,9 @@ async fn main() -> anyhow::Result<()> { Inject *stateless* service-specific logic without naming an HTTP framework in your own crate: implement the hook traits with foundational types (`http`, -`bytes`, `serde_json`) only, and `cargo tree -i axum` in your crate shows `axum` -solely under `structured-proxy`. +`bytes`, `serde_json`) plus `async-trait` (the traits are `#[async_trait]`), +none of which is an HTTP framework. `cargo tree -i axum` in your crate then +shows `axum` solely under `structured-proxy`. ```rust use std::sync::Arc; @@ -215,7 +216,7 @@ struct MyPdp; // your forward-auth / policy decision #[async_trait::async_trait] impl AuthDecider for MyPdp { async fn decide(&self, req: &RequestParts<'_>) -> Decision { - // method / path / headers / peer in, a decision out — no axum types + // method / path / headers / peer in, a decision out (no axum types) Decision::Allow { inject_headers: http::HeaderMap::new() } } } From a7270fe7766719d0322150e906a2852a88b52d42 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 30 Jun 2026 23:17:09 +0300 Subject: [PATCH 05/11] fix: short-circuit userinfo without token; guard forward-auth path collision - /userinfo now returns the 401 Bearer challenge without invoking the OIDC backend when no credentials are present (never call it with an empty token) - Apply the verify-path / built-in-GET collision guard before both the injected decider route and the config-driven JWT forward-auth merge, so plain forward-auth also returns a clean error instead of an axum duplicate-route panic --- src/embed.rs | 37 +++++++++++++++++++------------------ src/embed/tests.rs | 38 +++++++++++++++++++++++++++++++++++++- src/lib.rs | 21 +++++++++++++-------- tests/hooks.rs | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 27 deletions(-) diff --git a/src/embed.rs b/src/embed.rs index d20f07b..35b4b97 100644 --- a/src/embed.rs +++ b/src/embed.rs @@ -150,25 +150,26 @@ pub(crate) fn oidc_backend_routes(backend: Arc) -> Router Json(claims).into_response(), - // RFC 6750 §3: a Bearer challenge lets clients classify the - // failure. Plain `Bearer` when no credentials were sent; - // `error="invalid_token"` when a presented token was rejected. - None => { - let challenge = if token.is_some() { - r#"Bearer error="invalid_token""# - } else { - "Bearer" - }; - unauthorized_with_challenge( - bytes::Bytes::from_static( - br#"{"error":"invalid_token","message":"invalid or expired token"}"#, - ), - challenge, - ) - } + // Presented token rejected by the backend. + None => unauthorized_with_challenge( + bytes::Bytes::from_static( + br#"{"error":"invalid_token","message":"invalid or expired token"}"#, + ), + r#"Bearer error="invalid_token""#, + ), } } }), diff --git a/src/embed/tests.rs b/src/embed/tests.rs index e348548..c0c570a 100644 --- a/src/embed/tests.rs +++ b/src/embed/tests.rs @@ -580,9 +580,15 @@ async fn oidc_paths_are_consumer_configurable() { assert_eq!(jwks.headers()["content-type"], "application/jwk-set+json"); // UserInfo at the consumer's custom path; the default /userinfo is unmounted. + // A bearer token is required (the no-credentials path short-circuits to 401). let custom = app .clone() - .oneshot(Request::get("/oauth/userinfo").body(Body::empty()).unwrap()) + .oneshot( + Request::get("/oauth/userinfo") + .header("authorization", "Bearer any") + .body(Body::empty()) + .unwrap(), + ) .await .unwrap(); assert_eq!(custom.status(), StatusCode::OK); @@ -652,3 +658,33 @@ async fn extra_route_oversized_body_is_rejected_not_emptied() { // The handler's echo body ("h|...") must NOT appear: it never ran. assert!(!body_string(resp).await.starts_with("h|")); } + +// --- edge case: userinfo does not call the backend without credentials -- + +/// Panics if `userinfo` is ever invoked, proving the no-credentials path +/// short-circuits before reaching the backend. +struct NeverCalledOidc; + +#[async_trait] +impl OidcBackend for NeverCalledOidc { + fn metadata_documents(&self) -> Vec { + Vec::new() + } + fn jwks(&self) -> MetadataDocument { + MetadataDocument::new("/jwks", serde_json::json!({ "keys": [] })) + } + async fn userinfo(&self, _bearer: &str) -> Option { + panic!("userinfo must not be called when no bearer token is present"); + } +} + +#[tokio::test] +async fn userinfo_without_token_does_not_invoke_backend() { + let app = oidc_backend_routes(Arc::new(NeverCalledOidc)).with_state(crate::test_state()); + let resp = app + .oneshot(Request::get("/userinfo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + assert_eq!(resp.headers()["www-authenticate"], "Bearer"); +} diff --git a/src/lib.rs b/src/lib.rs index 4370e4a..d37ce94 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -428,6 +428,18 @@ impl ProxyServer { auth::forward::ForwardAuth::build(self.config.auth.as_ref()?, built.clone()) }); + // Guard the verify path against colliding with a built-in GET route + // (axum panics on duplicate path registration); fail with a clear error + // instead. Covers BOTH a verify endpoint owned by an injected decider + // and a config-driven JWT forward-auth mount (both land at `verify_path`). + if (self.auth_decider.is_some() || forward_auth.is_some()) + && self.builtin_get_paths().iter().any(|p| p == &verify_path) + { + anyhow::bail!( + "verify path {verify_path:?} collides with a health/metrics endpoint path" + ); + } + // Auth runs inside Shield (added first = inner): rate limiting sheds // load before any signature verification work. if let Some(auth) = auth { @@ -438,14 +450,7 @@ impl ProxyServer { // present (in-process PDP); otherwise the config-driven JWT ForwardAuth // backs it. Mounted after the auth layer so it is not itself JWT-gated. if let Some(decider) = &self.auth_decider { - // Guard against the verify path colliding with a built-in GET route - // (axum panics on duplicate path registration); fail with a clear - // error instead. `verify_path` was resolved once above. - if self.builtin_get_paths().iter().any(|p| p == &verify_path) { - anyhow::bail!( - "verify path {verify_path:?} collides with a health/metrics endpoint path" - ); - } + // Collision with a built-in GET path was already rejected above. let decider = decider.clone(); router = router.route( &verify_path, diff --git a/tests/hooks.rs b/tests/hooks.rs index 7e74415..bea3862 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -293,6 +293,41 @@ metrics: } } +#[tokio::test] +async fn config_forward_auth_path_colliding_with_probe_is_a_clean_error() { + // The same collision guard must cover plain JWT forward-auth (no decider): + // a forward_auth.path equal to a built-in GET path is a clean error, not an + // axum duplicate-route panic. + const PUB_PEM: &str = "-----BEGIN PUBLIC KEY-----\n\ + MCowBQYDK2VwAyEARCMxEnaM2/dblLuPNgBZpTvSUXO5ir+XQ1nyzJm4CFw=\n\ + -----END PUBLIC KEY-----\n"; + let pem_path = std::env::temp_dir().join(format!("sp_hooks_fa_{}.pem", std::process::id())); + std::fs::write(&pem_path, PUB_PEM).unwrap(); + + let config = ProxyConfig::from_yaml_str(&format!( + r#" +upstream: + default: "http://127.0.0.1:50051" +auth: + mode: "jwt" + jwt: + public_key_pem_file: "{}" + forward_auth: + enabled: true + path: "/health" +"#, + pem_path.display() + )) + .unwrap(); + let result = ProxyServer::from_config(config).router(); + let _ = std::fs::remove_file(&pem_path); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("collides with a health/metrics endpoint")); +} + #[tokio::test] async fn verify_path_colliding_with_probe_is_a_clean_error() { // A verify path that collides with a built-in GET route must surface a From 374cab67dfcf580ba26972d7a72a1c0ef5f5a63e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 00:20:52 +0300 Subject: [PATCH 06/11] fix: cover all built-in routes in verify guard; order authz before decider - builtin_get_paths now includes the enabled OpenAPI spec/docs paths, so a verify path colliding with /openapi.json or /docs is rejected with a clear error instead of an axum duplicate-route panic - validate_edge_paths rejects paths without a leading '/' (axum would reject the route shape at construction otherwise) - order the proxied-route gates authz -> AuthDecider -> handler (the decider layer is added inner, authz outer), so ext_authz runs first and the decider sees headers it injected; the previous order was inverted - correct the OidcBackend::userinfo doc: it only ever receives a present, non-empty bearer token (missing credentials are rejected before the call) --- src/config.rs | 22 ++++++++++++++++++++-- src/hooks.rs | 9 ++++++--- src/lib.rs | 36 +++++++++++++++++++++--------------- tests/hooks.rs | 28 ++++++++++++++++++++++++++-- 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/src/config.rs b/src/config.rs index 4877ba4..6b7ab31 100644 --- a/src/config.rs +++ b/src/config.rs @@ -640,12 +640,16 @@ impl ProxyConfig { Ok(()) } - /// Reject duplicate built-in edge paths up front, so the router does not - /// panic at construction registering the same path twice (e.g. setting + /// Reject malformed or duplicate built-in edge paths up front, so the router + /// does not panic at construction (axum rejects a route that does not start + /// with `/`, and panics on a path registered twice, e.g. setting /// `health.path` to the default `live_path`). fn validate_edge_paths(&self) -> anyhow::Result<()> { let mut seen = std::collections::HashSet::new(); let mut check = |label: &str, path: &str| -> anyhow::Result<()> { + if !path.starts_with('/') { + anyhow::bail!("endpoint path {path:?} ({label}) must start with '/'"); + } if !seen.insert(path.to_string()) { anyhow::bail!("duplicate endpoint path {path:?} ({label}); each built-in endpoint must have a distinct path"); } @@ -756,6 +760,20 @@ health: assert!(ProxyConfig::from_yaml_str(yaml3).is_ok()); } + #[test] + fn malformed_edge_path_is_rejected() { + // A path without a leading '/' would make axum reject the route at + // construction; catch it at config load with a clear message. + let yaml = r#" +upstream: + default: "grpc://x:1" +health: + path: "health" +"#; + let err = ProxyConfig::from_yaml_str(yaml).unwrap_err(); + assert!(err.to_string().contains("must start with '/'")); + } + #[test] fn test_zero_sse_keep_alive_is_rejected() { // A zero keep-alive would make axum's SSE timer fire continuously diff --git a/src/hooks.rs b/src/hooks.rs index fa2d2bf..ef90934 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -113,9 +113,12 @@ pub trait OidcBackend: Send + Sync { "/userinfo".to_string() } - /// Resolve UserInfo claims for a verified bearer token. `None` yields `401`. - /// `bearer` is the raw token (the `Bearer ` prefix already stripped), or an - /// empty string when no credentials were presented. + /// Resolve UserInfo claims for a bearer token. `None` yields `401`. + /// + /// `bearer` is always a present, non-empty token (the `Bearer ` prefix + /// already stripped): a request with no credentials is rejected with a + /// `401` Bearer challenge before this method is called, so implementations + /// never receive an empty string. async fn userinfo(&self, bearer: &str) -> Option; } diff --git a/src/lib.rs b/src/lib.rs index d37ce94..1e71e53 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -200,8 +200,11 @@ impl ProxyServer { }) } - /// The built-in `GET` paths that are actually mounted (enabled health probes - /// and metrics), used to detect collisions before registering more routes. + /// The built-in `GET` paths that are actually mounted (enabled health probes, + /// metrics, and OpenAPI spec/docs), used to detect collisions before + /// registering more routes (e.g. the verify endpoint). Must list EVERY + /// built-in route mounted before verify, or a colliding verify path slips + /// past the guard and panics axum at route registration. fn builtin_get_paths(&self) -> Vec { let mut paths = Vec::new(); if self.config.health.enabled { @@ -213,6 +216,10 @@ impl ProxyServer { if self.config.metrics.enabled { paths.push(self.config.metrics.path.clone()); } + if let Some(openapi) = self.config.openapi.as_ref().filter(|o| o.enabled) { + paths.push(openapi.path.clone()); + paths.push(openapi.docs_path.clone()); + } paths } @@ -288,23 +295,24 @@ impl ProxyServer { .map_err(|e| anyhow::anyhow!("invalid authz config: {e}"))?, None => None, }; - if let Some(authz) = authz { - transcode_routes = transcode_routes.layer(axum::middleware::from_fn_with_state( - authz, - auth::authz::middleware, - )); - } - // Embedded Tier-2 in-process gate: an injected AuthDecider runs inline on - // the proxied routes (it sees the JWT-injected identity headers, like the - // ext_authz layer above). Added after authz so authz, when both are set, - // runs first (inner layer). + // Order matters: in axum the LAST-added layer is outermost and runs + // FIRST. We want `authz -> AuthDecider -> handler`, so add the decider + // layer first (inner) and the authz layer second (outer). That way, when + // both are configured, ext_authz runs first and the in-process decider + // sees any headers the authz Check injected. if let Some(decider) = &self.auth_decider { transcode_routes = transcode_routes.layer(axum::middleware::from_fn_with_state( decider.clone(), embed::auth_decider_gate, )); } + if let Some(authz) = authz { + transcode_routes = transcode_routes.layer(axum::middleware::from_fn_with_state( + authz, + auth::authz::middleware, + )); + } // Health routes. Paths are configurable; the whole group is skippable. let health_routes = if self.config.health.enabled { @@ -435,9 +443,7 @@ impl ProxyServer { if (self.auth_decider.is_some() || forward_auth.is_some()) && self.builtin_get_paths().iter().any(|p| p == &verify_path) { - anyhow::bail!( - "verify path {verify_path:?} collides with a health/metrics endpoint path" - ); + anyhow::bail!("verify path {verify_path:?} collides with a built-in endpoint path"); } // Auth runs inside Shield (added first = inner): rate limiting sheds diff --git a/tests/hooks.rs b/tests/hooks.rs index bea3862..dde0fc8 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -325,7 +325,31 @@ auth: assert!(result .unwrap_err() .to_string() - .contains("collides with a health/metrics endpoint")); + .contains("collides with a built-in endpoint")); +} + +#[tokio::test] +async fn verify_path_colliding_with_openapi_docs_is_a_clean_error() { + // The collision guard must cover every built-in GET route mounted before + // verify, including the OpenAPI spec/docs paths (not just health/metrics). + let config = ProxyConfig::from_yaml_str( + r#" +upstream: + default: "http://127.0.0.1:50051" +openapi: + enabled: true +"#, + ) + .unwrap(); + let result = ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .with_verify_path("/docs") + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("collides with a built-in endpoint")); } #[tokio::test] @@ -342,7 +366,7 @@ async fn verify_path_colliding_with_probe_is_a_clean_error() { assert!(result .unwrap_err() .to_string() - .contains("collides with a health/metrics endpoint")); + .contains("collides with a built-in endpoint")); } #[tokio::test] From ee8f57eb3d8147f1ef9c2f5465cbf0a40412da78 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 00:51:00 +0300 Subject: [PATCH 07/11] fix: make verify-path collision guard cover all mounted routes - reserved_get_paths now collects every route mounted before verify: health, metrics, OpenAPI, the OIDC surface (injected backend or config-driven static discovery), embedder extra routes, and the transcoded REST routes (transcode::route_paths). A verify path colliding with any of them is now a clear error instead of an axum duplicate-route panic - reject a verify path without a leading '/' before route registration - validate_edge_paths also checks OpenAPI spec/docs paths for shape and duplicates --- src/config.rs | 4 ++++ src/lib.rs | 55 ++++++++++++++++++++++++++++++++------------ src/oidc/mod.rs | 9 ++++++++ src/transcode/mod.rs | 26 +++++++++++++++++++++ tests/hooks.rs | 39 ++++++++++++++++++++++++++++--- 5 files changed, 115 insertions(+), 18 deletions(-) diff --git a/src/config.rs b/src/config.rs index 6b7ab31..249db5c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -664,6 +664,10 @@ impl ProxyConfig { if self.metrics.enabled { check("metrics.path", &self.metrics.path)?; } + if let Some(openapi) = self.openapi.as_ref().filter(|o| o.enabled) { + check("openapi.path", &openapi.path)?; + check("openapi.docs_path", &openapi.docs_path)?; + } Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index 1e71e53..ed63073 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -200,12 +200,14 @@ impl ProxyServer { }) } - /// The built-in `GET` paths that are actually mounted (enabled health probes, - /// metrics, and OpenAPI spec/docs), used to detect collisions before - /// registering more routes (e.g. the verify endpoint). Must list EVERY - /// built-in route mounted before verify, or a colliding verify path slips - /// past the guard and panics axum at route registration. - fn builtin_get_paths(&self) -> Vec { + /// Every path mounted before the verify endpoint, used to reject a colliding + /// verify path with a clear error instead of an axum duplicate-route panic. + /// + /// Must stay exhaustive: health probes, metrics, OpenAPI spec/docs, the OIDC + /// surface (injected backend or config-driven static discovery), embedder + /// extra routes, and the transcoded REST routes. A category omitted here lets + /// a colliding verify path slip past the guard and panic at registration. + fn reserved_get_paths(&self, pool: &DescriptorPool) -> anyhow::Result> { let mut paths = Vec::new(); if self.config.health.enabled { paths.push(self.config.health.path.clone()); @@ -220,7 +222,23 @@ impl ProxyServer { paths.push(openapi.path.clone()); paths.push(openapi.docs_path.clone()); } - paths + // OIDC: an injected backend supersedes config-driven static discovery. + if let Some(backend) = &self.oidc_backend { + paths.extend(backend.metadata_documents().into_iter().map(|d| d.path)); + paths.push(backend.jwks().path); + paths.push(backend.userinfo_path()); + } else if let Some(cfg) = &self.config.oidc_discovery { + if let Some(oidc) = oidc::Oidc::build(cfg) + .map_err(|e| anyhow::anyhow!("invalid oidc_discovery config: {e}"))? + { + paths.extend(oidc.paths()); + } + } + for route in &self.extra_routes { + paths.push(route.path.clone()); + } + paths.extend(transcode::route_paths(pool, &self.config.aliases)); + Ok(paths) } /// Build the axum router with all endpoints. @@ -436,14 +454,21 @@ impl ProxyServer { auth::forward::ForwardAuth::build(self.config.auth.as_ref()?, built.clone()) }); - // Guard the verify path against colliding with a built-in GET route - // (axum panics on duplicate path registration); fail with a clear error - // instead. Covers BOTH a verify endpoint owned by an injected decider - // and a config-driven JWT forward-auth mount (both land at `verify_path`). - if (self.auth_decider.is_some() || forward_auth.is_some()) - && self.builtin_get_paths().iter().any(|p| p == &verify_path) - { - anyhow::bail!("verify path {verify_path:?} collides with a built-in endpoint path"); + // Guard the verify endpoint (owned by an injected decider or a + // config-driven JWT forward-auth mount, both at `verify_path`) against a + // malformed or colliding path, so the router returns a clear error + // instead of axum panicking at route registration. + if self.auth_decider.is_some() || forward_auth.is_some() { + if !verify_path.starts_with('/') { + anyhow::bail!("verify path {verify_path:?} must start with '/'"); + } + if self + .reserved_get_paths(&pool)? + .iter() + .any(|p| p == &verify_path) + { + anyhow::bail!("verify path {verify_path:?} collides with an already-mounted route"); + } } // Auth runs inside Shield (added first = inner): rate limiting sheds diff --git a/src/oidc/mod.rs b/src/oidc/mod.rs index d966a81..74d03d8 100644 --- a/src/oidc/mod.rs +++ b/src/oidc/mod.rs @@ -72,6 +72,15 @@ impl Oidc { })) } + /// The paths this serves (discovery document + JWKS), for collision checks + /// against other routes mounted on the same server. + pub(crate) fn paths(&self) -> Vec { + vec![ + "/.well-known/openid-configuration".to_string(), + self.jwks_path.clone(), + ] + } + /// Routes serving the discovery document and the JWKS. pub fn routes(&self) -> Router where diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index 3545dd6..87ddfa6 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -179,6 +179,32 @@ pub fn routes(pool: &DescriptorPool, aliases: &[AliasConfig]) router } +/// The axum paths [`routes`] would register for this pool and aliases. +/// +/// Mirrors the registration in [`routes`] (unary RPCs, their config aliases, and +/// server-streaming RPCs) without building handlers, so callers can detect path +/// collisions before mounting additional routes (e.g. a forward-auth endpoint). +pub fn route_paths(pool: &DescriptorPool, aliases: &[AliasConfig]) -> Vec { + let mut paths = Vec::new(); + for entry in extract_routes(pool) { + paths.push(proto_path_to_axum(&entry.http_path)); + for alias in aliases { + if let Some(suffix) = entry.http_path.strip_prefix(&alias.to) { + if alias.from.ends_with("/{path}") { + let prefix = alias.from.trim_end_matches("/{path}"); + paths.push(format!("{prefix}{suffix}")); + } + } + } + } + for entry in extract_streaming_routes(pool) { + if matches!(entry.http_method, HttpMethod::Get | HttpMethod::Post) { + paths.push(proto_path_to_axum(&entry.http_path)); + } + } + paths +} + /// JSON serialization options shared by the unary and streaming response paths, /// so a given message serializes identically regardless of RPC kind. fn response_serialize_options() -> SerializeOptions { diff --git a/tests/hooks.rs b/tests/hooks.rs index dde0fc8..0ae36d0 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -325,7 +325,7 @@ auth: assert!(result .unwrap_err() .to_string() - .contains("collides with a built-in endpoint")); + .contains("collides with an already-mounted route")); } #[tokio::test] @@ -349,7 +349,40 @@ openapi: assert!(result .unwrap_err() .to_string() - .contains("collides with a built-in endpoint")); + .contains("collides with an already-mounted route")); +} + +#[tokio::test] +async fn verify_path_colliding_with_oidc_route_is_a_clean_error() { + // The reserved-path set must include OIDC backend routes; a verify path on + // top of the discovery document is a clean error, not a panic. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let result = ProxyServer::from_config(config) + .with_oidc_backend(Arc::new(DemoOidc)) + .with_auth_decider(Arc::new(DemoDecider)) + .with_verify_path("/.well-known/openid-configuration") + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("collides with an already-mounted route")); +} + +#[tokio::test] +async fn malformed_verify_path_is_a_clean_error() { + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let result = ProxyServer::from_config(config) + .with_auth_decider(Arc::new(DemoDecider)) + .with_verify_path("auth/verify") // missing leading '/' + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("must start with '/'")); } #[tokio::test] @@ -366,7 +399,7 @@ async fn verify_path_colliding_with_probe_is_a_clean_error() { assert!(result .unwrap_err() .to_string() - .contains("collides with a built-in endpoint")); + .contains("collides with an already-mounted route")); } #[tokio::test] From 7d8438bebade68a5387c9062681c808c951f7a2f Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 01:43:41 +0300 Subject: [PATCH 08/11] fix: reject duplicate edge routes before building the router Run the reserved-path duplicate scan across the whole mounted edge (built-in routes, OIDC surface, extra routes, transcoded paths, and the verify endpoint) up front, before any router is constructed. Any collision now returns a clear error instead of reaching axum and panicking at .route/.merge during startup. --- src/lib.rs | 40 ++++++++++++++++++++++++---------------- tests/hooks.rs | 28 ++++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index ed63073..8f9dd4a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -260,6 +260,28 @@ impl ProxyServer { let verify_path = self.resolved_verify_path(); + // Reject duplicate routes across the WHOLE mounted edge BEFORE any router + // is built, so a collision (between built-in routes, the OIDC surface, + // embedder extra routes, transcoded paths, or the verify endpoint) is a + // clear error instead of an axum duplicate-route panic at `.route`/`.merge`. + let verify_mounted = self.auth_decider.is_some() + || self.config.auth.as_ref().is_some_and(|a| { + a.mode == "jwt" && a.forward_auth.as_ref().is_some_and(|fa| fa.enabled) + }); + let mut mounted = self.reserved_get_paths(&pool)?; + if verify_mounted { + if !verify_path.starts_with('/') { + anyhow::bail!("verify path {verify_path:?} must start with '/'"); + } + mounted.push(verify_path.clone()); + } + let mut seen = std::collections::HashSet::with_capacity(mounted.len()); + for path in &mounted { + if !seen.insert(path.as_str()) { + anyhow::bail!("route path {path:?} is registered by more than one endpoint"); + } + } + // Keep the actually-configured probe / metrics / verify paths reachable // under maintenance mode. The default exempt list names the default // paths; once those are relocated via config, the relocated paths must @@ -454,22 +476,8 @@ impl ProxyServer { auth::forward::ForwardAuth::build(self.config.auth.as_ref()?, built.clone()) }); - // Guard the verify endpoint (owned by an injected decider or a - // config-driven JWT forward-auth mount, both at `verify_path`) against a - // malformed or colliding path, so the router returns a clear error - // instead of axum panicking at route registration. - if self.auth_decider.is_some() || forward_auth.is_some() { - if !verify_path.starts_with('/') { - anyhow::bail!("verify path {verify_path:?} must start with '/'"); - } - if self - .reserved_get_paths(&pool)? - .iter() - .any(|p| p == &verify_path) - { - anyhow::bail!("verify path {verify_path:?} collides with an already-mounted route"); - } - } + // Duplicate-route collisions (including the verify path) were already + // rejected up front, before any router was built. // Auth runs inside Shield (added first = inner): rate limiting sheds // load before any signature verification work. diff --git a/tests/hooks.rs b/tests/hooks.rs index 0ae36d0..5fbc668 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -325,7 +325,7 @@ auth: assert!(result .unwrap_err() .to_string() - .contains("collides with an already-mounted route")); + .contains("registered by more than one endpoint")); } #[tokio::test] @@ -349,7 +349,7 @@ openapi: assert!(result .unwrap_err() .to_string() - .contains("collides with an already-mounted route")); + .contains("registered by more than one endpoint")); } #[tokio::test] @@ -367,7 +367,27 @@ async fn verify_path_colliding_with_oidc_route_is_a_clean_error() { assert!(result .unwrap_err() .to_string() - .contains("collides with an already-mounted route")); + .contains("registered by more than one endpoint")); +} + +#[tokio::test] +async fn extra_route_colliding_with_builtin_is_a_clean_error() { + // A collision BETWEEN mounted routes (here an extra route over the health + // endpoint), not involving verify, is also a clean error rather than a panic. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let result = ProxyServer::from_config(config) + .with_extra_routes([ExtraRoute::new( + Method::GET, + "/health", + Arc::new(PingHandler), + )]) + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("registered by more than one endpoint")); } #[tokio::test] @@ -399,7 +419,7 @@ async fn verify_path_colliding_with_probe_is_a_clean_error() { assert!(result .unwrap_err() .to_string() - .contains("collides with an already-mounted route")); + .contains("registered by more than one endpoint")); } #[tokio::test] From 6cc0439ec30343c706333533408ba68704960b0c Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 02:08:04 +0300 Subject: [PATCH 09/11] fix: guard and exempt the verify path that is actually mounted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The with_verify_path override applies only to an injected AuthDecider's verify route; config-driven JWT forward-auth always mounts at auth.forward_auth.path. Introduce mounted_verify_path() that returns the branch-correct path (decider → override/config/default; config forward-auth → forward_auth.path, only when mode is jwt), and use it for both the duplicate-route guard and the maintenance-exempt list. Previously a with_verify_path override hid a config forward_auth.path collision and exempted the wrong path under maintenance. --- src/lib.rs | 63 ++++++++++++++++++++++++++++++++------------------ tests/hooks.rs | 39 +++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 23 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8f9dd4a..806e5ec 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -187,9 +187,11 @@ impl ProxyServer { Ok(pool) } - /// Resolve the `/verify` forward-auth path: the `with_verify_path` override, - /// then `auth.forward_auth.path` from config, then the default `/auth/verify`. - fn resolved_verify_path(&self) -> String { + /// The path an injected [`AuthDecider`] answers `/verify` at: the + /// `with_verify_path` override, then `auth.forward_auth.path`, then the + /// default `/auth/verify`. Only meaningful when a decider is set (the + /// override does not apply to config-driven JWT forward-auth). + fn decider_verify_path(&self) -> String { self.verify_path.clone().unwrap_or_else(|| { self.config .auth @@ -200,6 +202,30 @@ impl ProxyServer { }) } + /// The verify path that is ACTUALLY mounted, or `None` when no verify route + /// is mounted. This is what the collision guard and maintenance-exempt list + /// must use, since the two mount sites use different paths: + /// - an injected decider mounts at [`decider_verify_path`](Self::decider_verify_path) + /// (the `with_verify_path` override applies), whereas + /// - config-driven JWT forward-auth mounts `forward_auth.routes()` at + /// `auth.forward_auth.path` (the override does NOT apply, and it mounts + /// only when `auth.mode == "jwt"`, since the endpoint shares the built JWT + /// `Auth`). + fn mounted_verify_path(&self) -> Option { + if self.auth_decider.is_some() { + return Some(self.decider_verify_path()); + } + self.config.auth.as_ref().and_then(|a| { + if a.mode != "jwt" { + return None; + } + a.forward_auth + .as_ref() + .filter(|fa| fa.enabled) + .map(|fa| fa.path.clone()) + }) + } + /// Every path mounted before the verify endpoint, used to reject a colliding /// verify path with a clear error instead of an axum duplicate-route panic. /// @@ -258,22 +284,19 @@ impl ProxyServer { let service_name = self.config.service.name.clone(); let metrics_namespace = service_name.replace('-', "_"); - let verify_path = self.resolved_verify_path(); + // The verify path that is actually mounted (branch-correct), if any. + let verify_path = self.mounted_verify_path(); // Reject duplicate routes across the WHOLE mounted edge BEFORE any router // is built, so a collision (between built-in routes, the OIDC surface, // embedder extra routes, transcoded paths, or the verify endpoint) is a // clear error instead of an axum duplicate-route panic at `.route`/`.merge`. - let verify_mounted = self.auth_decider.is_some() - || self.config.auth.as_ref().is_some_and(|a| { - a.mode == "jwt" && a.forward_auth.as_ref().is_some_and(|fa| fa.enabled) - }); let mut mounted = self.reserved_get_paths(&pool)?; - if verify_mounted { - if !verify_path.starts_with('/') { - anyhow::bail!("verify path {verify_path:?} must start with '/'"); + if let Some(vp) = &verify_path { + if !vp.starts_with('/') { + anyhow::bail!("verify path {vp:?} must start with '/'"); } - mounted.push(verify_path.clone()); + mounted.push(vp.clone()); } let mut seen = std::collections::HashSet::with_capacity(mounted.len()); for path in &mounted { @@ -297,15 +320,8 @@ impl ProxyServer { if self.config.metrics.enabled { maintenance_exempt.push(self.config.metrics.path.clone()); } - if self.auth_decider.is_some() - || self - .config - .auth - .as_ref() - .and_then(|a| a.forward_auth.as_ref()) - .is_some_and(|fa| fa.enabled) - { - maintenance_exempt.push(verify_path.clone()); + if let Some(vp) = &verify_path { + maintenance_exempt.push(vp.clone()); } let state = ProxyState { @@ -489,10 +505,11 @@ impl ProxyServer { // present (in-process PDP); otherwise the config-driven JWT ForwardAuth // backs it. Mounted after the auth layer so it is not itself JWT-gated. if let Some(decider) = &self.auth_decider { - // Collision with a built-in GET path was already rejected above. + // Collision / shape of this path was already validated above. let decider = decider.clone(); + let path = self.decider_verify_path(); router = router.route( - &verify_path, + &path, axum::routing::any(move |req: axum::extract::Request| { let decider = decider.clone(); async move { embed::verify_via_decider(decider, req).await } diff --git a/tests/hooks.rs b/tests/hooks.rs index 5fbc668..8768d0b 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -405,6 +405,45 @@ async fn malformed_verify_path_is_a_clean_error() { .contains("must start with '/'")); } +#[tokio::test] +async fn config_forward_auth_guard_uses_config_path_not_override() { + // With no decider, config-driven JWT forward-auth mounts at + // auth.forward_auth.path; the with_verify_path override does NOT apply there. + // The guard must validate the CONFIG path (which collides with /health), not + // be fooled by the override pointing somewhere harmless. + const PUB_PEM: &str = "-----BEGIN PUBLIC KEY-----\n\ + MCowBQYDK2VwAyEARCMxEnaM2/dblLuPNgBZpTvSUXO5ir+XQ1nyzJm4CFw=\n\ + -----END PUBLIC KEY-----\n"; + let pem_path = std::env::temp_dir().join(format!("sp_hooks_ov_{}.pem", std::process::id())); + std::fs::write(&pem_path, PUB_PEM).unwrap(); + + let config = ProxyConfig::from_yaml_str(&format!( + r#" +upstream: + default: "http://127.0.0.1:50051" +auth: + mode: "jwt" + jwt: + public_key_pem_file: "{}" + forward_auth: + enabled: true + path: "/health" +"#, + pem_path.display() + )) + .unwrap(); + // Override points elsewhere, but it is ignored for config-driven forward-auth. + let result = ProxyServer::from_config(config) + .with_verify_path("/elsewhere") + .router(); + let _ = std::fs::remove_file(&pem_path); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("registered by more than one endpoint")); +} + #[tokio::test] async fn verify_path_colliding_with_probe_is_a_clean_error() { // A verify path that collides with a built-in GET route must surface a From eeed1a0b9ca371034f851c1d4b5321d03a51cc0e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 03:23:40 +0300 Subject: [PATCH 10/11] fix: validate every mounted path shape, not just verify reserved_get_paths() includes consumer-supplied OIDC-backend and extra-route paths that are handed to Router::route(), which panics on a path without a leading '/'. Validate the leading '/' for every mounted path (not only the verify path) before building the router, so a malformed backend/extra-route path is a clean build error. Also embed the temp PEM path in test YAML with forward slashes so the config parses on Windows runners. --- src/lib.rs | 16 +++++++++------- tests/hooks.rs | 26 ++++++++++++++++++++++++-- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 806e5ec..586cba5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -287,19 +287,21 @@ impl ProxyServer { // The verify path that is actually mounted (branch-correct), if any. let verify_path = self.mounted_verify_path(); - // Reject duplicate routes across the WHOLE mounted edge BEFORE any router - // is built, so a collision (between built-in routes, the OIDC surface, - // embedder extra routes, transcoded paths, or the verify endpoint) is a - // clear error instead of an axum duplicate-route panic at `.route`/`.merge`. + // Validate the WHOLE mounted edge BEFORE any router is built, so a + // malformed path (missing leading '/') or a collision (between built-in + // routes, the OIDC surface, embedder extra routes, transcoded paths, or + // the verify endpoint) is a clear error instead of an axum panic at + // `.route`/`.merge`. Consumer-supplied paths (OIDC backend, extra routes, + // verify) are not otherwise validated, so check every entry here. let mut mounted = self.reserved_get_paths(&pool)?; if let Some(vp) = &verify_path { - if !vp.starts_with('/') { - anyhow::bail!("verify path {vp:?} must start with '/'"); - } mounted.push(vp.clone()); } let mut seen = std::collections::HashSet::with_capacity(mounted.len()); for path in &mounted { + if !path.starts_with('/') { + anyhow::bail!("route path {path:?} must start with '/'"); + } if !seen.insert(path.as_str()) { anyhow::bail!("route path {path:?} is registered by more than one endpoint"); } diff --git a/tests/hooks.rs b/tests/hooks.rs index 8768d0b..ece59df 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -316,7 +316,10 @@ auth: enabled: true path: "/health" "#, - pem_path.display() + // Forward slashes are valid in file paths on every platform and, unlike + // Windows backslashes, are not escape sequences in a double-quoted YAML + // scalar, so the generated config parses on Windows runners too. + pem_path.display().to_string().replace('\\', "/") )) .unwrap(); let result = ProxyServer::from_config(config).router(); @@ -390,6 +393,22 @@ async fn extra_route_colliding_with_builtin_is_a_clean_error() { .contains("registered by more than one endpoint")); } +#[tokio::test] +async fn malformed_extra_route_path_is_a_clean_error() { + // A consumer-supplied path without a leading '/' (here an extra route) would + // panic axum at registration; it must be a clean build error instead. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let result = ProxyServer::from_config(config) + .with_extra_routes([ExtraRoute::new(Method::GET, "ping", Arc::new(PingHandler))]) + .router(); + assert!(result.is_err()); + assert!(result + .unwrap_err() + .to_string() + .contains("must start with '/'")); +} + #[tokio::test] async fn malformed_verify_path_is_a_clean_error() { let config = @@ -429,7 +448,10 @@ auth: enabled: true path: "/health" "#, - pem_path.display() + // Forward slashes are valid in file paths on every platform and, unlike + // Windows backslashes, are not escape sequences in a double-quoted YAML + // scalar, so the generated config parses on Windows runners too. + pem_path.display().to_string().replace('\\', "/") )) .unwrap(); // Override points elsewhere, but it is ignored for config-driven forward-auth. From 62bde6ce5ec6b90cb805bf2473298b8cea8fe235 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Wed, 1 Jul 2026 05:32:28 +0300 Subject: [PATCH 11/11] fix: key route-collision detection on (method, path), not path alone Same-path routes with different methods (e.g. an embedder registering GET /c and POST /c through the extra-route adapter, which merges them by method) are a legal shape. The collision guard now keys on (method, path): only a repeated (method, path) is a conflict. The verify endpoint answers all methods, so it is tracked as '*' and conflicts with any route on its path. route_paths and reserved_routes now carry the HTTP method for each route. --- src/lib.rs | 74 ++++++++++++++++++++++++++++---------------- src/transcode/mod.rs | 34 +++++++++++++++----- tests/hooks.rs | 33 ++++++++++++++++++++ 3 files changed, 107 insertions(+), 34 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 586cba5..2a6c8bd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -226,45 +226,53 @@ impl ProxyServer { }) } - /// Every path mounted before the verify endpoint, used to reject a colliding - /// verify path with a clear error instead of an axum duplicate-route panic. + /// Every `(method, path)` route mounted before the verify endpoint, used to + /// reject a real collision with a clear error instead of an axum + /// duplicate-route panic. `method` is the uppercase HTTP token; same-path + /// routes with different methods do NOT collide (the extra-route adapter and + /// axum merge them), so the key is the pair, not the path alone. /// /// Must stay exhaustive: health probes, metrics, OpenAPI spec/docs, the OIDC /// surface (injected backend or config-driven static discovery), embedder - /// extra routes, and the transcoded REST routes. A category omitted here lets - /// a colliding verify path slip past the guard and panic at registration. - fn reserved_get_paths(&self, pool: &DescriptorPool) -> anyhow::Result> { - let mut paths = Vec::new(); + /// extra routes, and the transcoded REST routes. All built-in surfaces here + /// are `GET`. + fn reserved_routes(&self, pool: &DescriptorPool) -> anyhow::Result> { + let mut routes = Vec::new(); + let mut get = |path: String| routes.push(("GET".to_string(), path)); if self.config.health.enabled { - paths.push(self.config.health.path.clone()); - paths.push(self.config.health.live_path.clone()); - paths.push(self.config.health.ready_path.clone()); - paths.push(self.config.health.startup_path.clone()); + get(self.config.health.path.clone()); + get(self.config.health.live_path.clone()); + get(self.config.health.ready_path.clone()); + get(self.config.health.startup_path.clone()); } if self.config.metrics.enabled { - paths.push(self.config.metrics.path.clone()); + get(self.config.metrics.path.clone()); } if let Some(openapi) = self.config.openapi.as_ref().filter(|o| o.enabled) { - paths.push(openapi.path.clone()); - paths.push(openapi.docs_path.clone()); + get(openapi.path.clone()); + get(openapi.docs_path.clone()); } // OIDC: an injected backend supersedes config-driven static discovery. if let Some(backend) = &self.oidc_backend { - paths.extend(backend.metadata_documents().into_iter().map(|d| d.path)); - paths.push(backend.jwks().path); - paths.push(backend.userinfo_path()); + for doc in backend.metadata_documents() { + get(doc.path); + } + get(backend.jwks().path); + get(backend.userinfo_path()); } else if let Some(cfg) = &self.config.oidc_discovery { if let Some(oidc) = oidc::Oidc::build(cfg) .map_err(|e| anyhow::anyhow!("invalid oidc_discovery config: {e}"))? { - paths.extend(oidc.paths()); + for path in oidc.paths() { + get(path); + } } } for route in &self.extra_routes { - paths.push(route.path.clone()); + routes.push((route.method.as_str().to_string(), route.path.clone())); } - paths.extend(transcode::route_paths(pool, &self.config.aliases)); - Ok(paths) + routes.extend(transcode::route_paths(pool, &self.config.aliases)); + Ok(routes) } /// Build the axum router with all endpoints. @@ -291,20 +299,32 @@ impl ProxyServer { // malformed path (missing leading '/') or a collision (between built-in // routes, the OIDC surface, embedder extra routes, transcoded paths, or // the verify endpoint) is a clear error instead of an axum panic at - // `.route`/`.merge`. Consumer-supplied paths (OIDC backend, extra routes, - // verify) are not otherwise validated, so check every entry here. - let mut mounted = self.reserved_get_paths(&pool)?; + // `.route`/`.merge`. Collisions are keyed by (method, path): same-path + // routes with different methods are legal (they merge), so only a + // repeated (method, path) — or any overlap with the verify endpoint, + // which answers ALL methods (`*`) — is a real conflict. + let mut mounted = self.reserved_routes(&pool)?; if let Some(vp) = &verify_path { - mounted.push(vp.clone()); + mounted.push(("*".to_string(), vp.clone())); } - let mut seen = std::collections::HashSet::with_capacity(mounted.len()); - for path in &mounted { + let mut methods_by_path: std::collections::HashMap<&str, std::collections::HashSet<&str>> = + std::collections::HashMap::new(); + for (method, path) in &mounted { if !path.starts_with('/') { anyhow::bail!("route path {path:?} must start with '/'"); } - if !seen.insert(path.as_str()) { + let methods = methods_by_path.entry(path.as_str()).or_default(); + // `*` (the verify endpoint) claims every method, so it conflicts with + // any other route on the same path, and vice versa. + let conflict = if method == "*" { + !methods.is_empty() + } else { + methods.contains("*") || methods.contains(method.as_str()) + }; + if conflict { anyhow::bail!("route path {path:?} is registered by more than one endpoint"); } + methods.insert(method.as_str()); } // Keep the actually-configured probe / metrics / verify paths reachable diff --git a/src/transcode/mod.rs b/src/transcode/mod.rs index 87ddfa6..2b0b32c 100644 --- a/src/transcode/mod.rs +++ b/src/transcode/mod.rs @@ -75,6 +75,19 @@ enum HttpMethod { Delete, } +impl HttpMethod { + /// The uppercase HTTP method token (e.g. `"GET"`). + fn as_str(self) -> &'static str { + match self { + HttpMethod::Get => "GET", + HttpMethod::Post => "POST", + HttpMethod::Put => "PUT", + HttpMethod::Patch => "PATCH", + HttpMethod::Delete => "DELETE", + } + } +} + /// Build transcoded REST→gRPC routes from a descriptor pool. /// /// Takes a `DescriptorPool` and optional path aliases from config. @@ -182,27 +195,34 @@ pub fn routes(pool: &DescriptorPool, aliases: &[AliasConfig]) /// The axum paths [`routes`] would register for this pool and aliases. /// /// Mirrors the registration in [`routes`] (unary RPCs, their config aliases, and -/// server-streaming RPCs) without building handlers, so callers can detect path +/// server-streaming RPCs) without building handlers, so callers can detect route /// collisions before mounting additional routes (e.g. a forward-auth endpoint). -pub fn route_paths(pool: &DescriptorPool, aliases: &[AliasConfig]) -> Vec { - let mut paths = Vec::new(); +/// +/// Each entry is `(method, path)` where `method` is the uppercase HTTP token, so +/// callers can distinguish same-path/different-method routes from real conflicts. +pub fn route_paths(pool: &DescriptorPool, aliases: &[AliasConfig]) -> Vec<(String, String)> { + let mut routes = Vec::new(); for entry in extract_routes(pool) { - paths.push(proto_path_to_axum(&entry.http_path)); + let method = entry.http_method.as_str().to_string(); + routes.push((method.clone(), proto_path_to_axum(&entry.http_path))); for alias in aliases { if let Some(suffix) = entry.http_path.strip_prefix(&alias.to) { if alias.from.ends_with("/{path}") { let prefix = alias.from.trim_end_matches("/{path}"); - paths.push(format!("{prefix}{suffix}")); + routes.push((method.clone(), format!("{prefix}{suffix}"))); } } } } for entry in extract_streaming_routes(pool) { if matches!(entry.http_method, HttpMethod::Get | HttpMethod::Post) { - paths.push(proto_path_to_axum(&entry.http_path)); + routes.push(( + entry.http_method.as_str().to_string(), + proto_path_to_axum(&entry.http_path), + )); } } - paths + routes } /// JSON serialization options shared by the unary and streaming response paths, diff --git a/tests/hooks.rs b/tests/hooks.rs index ece59df..9d5b018 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -393,6 +393,39 @@ async fn extra_route_colliding_with_builtin_is_a_clean_error() { .contains("registered by more than one endpoint")); } +#[tokio::test] +async fn extra_routes_sharing_a_path_with_different_methods_are_allowed() { + // GET /c and POST /c are a legal shape (the adapter merges them by method); + // the collision guard must key on (method, path) and NOT reject them. + let config = + ProxyConfig::from_yaml_str("upstream:\n default: \"http://127.0.0.1:50051\"\n").unwrap(); + let app = ProxyServer::from_config(config) + .with_extra_routes([ + ExtraRoute::new(Method::GET, "/c", Arc::new(PingHandler)), + ExtraRoute::new(Method::POST, "/c", Arc::new(PingHandler)), + ]) + .router() + .unwrap(); + for method in [Method::GET, Method::POST] { + let resp = app + .clone() + .oneshot( + axum::http::Request::builder() + .method(method.clone()) + .uri("/c") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "{method} /c should be served" + ); + } +} + #[tokio::test] async fn malformed_extra_route_path_is_a_clean_error() { // A consumer-supplied path without a leading '/' (here an extra route) would