diff --git a/crates/aisix-gateway/src/lib.rs b/crates/aisix-gateway/src/lib.rs index 23a01354..871d7288 100644 --- a/crates/aisix-gateway/src/lib.rs +++ b/crates/aisix-gateway/src/lib.rs @@ -51,6 +51,7 @@ pub use upstream_headers::{ RESERVED_UPSTREAM_HEADERS, }; pub use upstream_http::{ - client_builder, error_with_causes, transport_error_message, UpstreamHttpConfig, + client_builder, dispatch_client_builder, dispatch_client_fallback, error_with_causes, + transport_error_message, UpstreamHttpConfig, }; pub use upstream_tls::TlsSettings; diff --git a/crates/aisix-gateway/src/upstream_http.rs b/crates/aisix-gateway/src/upstream_http.rs index 3fcc1286..f71d3427 100644 --- a/crates/aisix-gateway/src/upstream_http.rs +++ b/crates/aisix-gateway/src/upstream_http.rs @@ -153,6 +153,53 @@ pub fn client_builder() -> reqwest::ClientBuilder { apply_tls(b, &cfg.tls) } +/// [`client_builder`] for the clients that carry a caller's request to an +/// AI provider, which additionally refuse to follow redirects. +/// +/// reqwest follows up to 10 redirects by default, and the gateway had +/// never opted out. A provider answering a dispatched POST with a `301` +/// or `302` therefore made reqwest re-issue it as a `GET` against the +/// `Location` host and hand back that response as the completion; a `307` +/// or `308` replayed the prompt body there verbatim. Only +/// `authorization`, `cookie`, and the proxy-auth headers are dropped when +/// the hop crosses hosts, so the vendor credential schemes that do not +/// use `authorization` — Azure's `api-key`, Anthropic's `x-api-key` — +/// were carried to whatever host the `Location` named. +/// +/// Nothing in the gateway was written for that: a 3xx from an upstream +/// collapses to a 502 in `BridgeError`, the access log records the +/// configured endpoint rather than the one that answered, and an operator +/// who never configured the redirect target has no way to see it. Refusing +/// the redirect turns the upstream's 3xx into the 502 the error path +/// already describes. +/// +/// The same reasoning covers the guardrail vendors: an inspection call +/// POSTs the caller's prompt to an operator-configured endpoint under a +/// vendor credential header (`Ocp-Apim-Subscription-Key`, and the rest), +/// none of which reqwest strips on a cross-host hop either. +/// +/// Not applied to every outbound client. The remaining ones either +/// already refuse redirects at their own construction site (JWKS/OIDC +/// discovery, MCP OAuth token, MCP OpenAPI tool calls, A2A) or talk to +/// an operator's own collector, where an endpoint behind a rewrite is +/// ordinary (telemetry, heartbeat, OTLP export). +pub fn dispatch_client_builder() -> reqwest::ClientBuilder { + client_builder().redirect(reqwest::redirect::Policy::none()) +} + +/// The client to fall back to when [`dispatch_client_builder`] fails to +/// build — a malformed deployment CA, say. +/// +/// The connection settings are lost either way; what must not be lost is +/// the redirect refusal, which `reqwest::Client::new()` would silently +/// restore. Builds from a policy alone, which cannot fail. +pub fn dispatch_client_fallback() -> reqwest::Client { + reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .expect("a client with only a redirect policy always builds") +} + /// Layer the outbound trust decision onto a builder. Split out so the /// per-ProviderKey clients get byte-for-byte the same treatment as the /// shared one. @@ -271,6 +318,50 @@ mod tests { assert!(client.is_ok(), "{:?}", client.err()); } + /// Both dispatch clients hand a 3xx back to the caller instead of + /// following it — including the fallback, which is reached when the + /// deployment's TLS material fails to apply and which + /// `reqwest::Client::new()` would have quietly restored to + /// following-by-default. + #[tokio::test] + async fn dispatch_clients_hand_back_a_redirect_instead_of_following_it() { + use std::io::{Read, Write}; + + let listener = std::net::TcpListener::bind(("127.0.0.1", 0)).expect("bind"); + let addr = listener.local_addr().expect("addr"); + let server = std::thread::spawn(move || { + for _ in 0..2 { + let (mut socket, _) = listener.accept().expect("accept"); + let mut buf = [0u8; 1024]; + let _ = socket.read(&mut buf); + let _ = socket.write_all( + b"HTTP/1.1 301 Moved Permanently\r\n\ + Location: http://127.0.0.1:1/elsewhere\r\n\ + Content-Length: 0\r\nConnection: close\r\n\r\n", + ); + } + }); + + for (label, client) in [ + ( + "dispatch_client_builder", + dispatch_client_builder().build().expect("builds"), + ), + ("dispatch_client_fallback", dispatch_client_fallback()), + ] { + let res = client + .post(format!("http://{addr}/v1/chat/completions")) + .body("{}") + .send() + .await + .unwrap_or_else(|e| panic!("{label}: {e}")); + // Following it would have dialed port 1, where nothing + // listens, and surfaced as a transport error instead. + assert_eq!(res.status(), 301, "{label} followed the redirect"); + } + server.join().expect("server thread"); + } + /// Every outbound HTTP client in the workspace must be built from /// [`client_builder`], or it silently keeps reqwest's defaults — no /// connect timeout, TCP keepalive off, and a 90s pooled-connection @@ -302,9 +393,16 @@ mod tests { if sanctioned_rmcp_site && line.contains("rmcp_reqwest::Client::") { continue; } - if line.contains("reqwest::Client::builder()") - || line.contains("reqwest::Client::new()") - { + // Prose about a constructor is not a call to one. + if line.trim_start().starts_with("//") { + continue; + } + // `Client::new()` unqualified, too: every one of these + // files imports the type, and the fallback arm of a + // failed build is where a bare client hides + // (`.unwrap_or_else(|_| Client::new())` gives back + // reqwest's defaults, redirect following included). + if line.contains("reqwest::Client::builder()") || line.contains("Client::new()") { offenders.push(format!("{}:{}", file.display(), n + 1)); } } @@ -460,6 +558,117 @@ mod tests { ); } + /// A client that carries a caller's request to a provider must be + /// built from [`dispatch_client_builder`], so an upstream 3xx becomes + /// the 502 the error path describes instead of a silent hop to + /// whatever host the `Location` named. + /// + /// Stated as a whitelist rather than a pattern: **every** + /// `client_builder()` site in the workspace either builds a dispatch + /// client or is named below. A rule shaped the other way — "files + /// that look like a bridge must use the dispatch builder" — passes + /// silently for a client put in `src/client.rs`, or in + /// `src/bridge/mod.rs`, or in a surface nobody thought of, which is + /// how the guardrail clients were missed the first time. + /// + /// Adding an outbound client therefore forces a decision here, and + /// the decision it forces is the safe-by-default one. + #[test] + fn every_outbound_client_is_classified_for_redirects() { + /// Sites that build a client from [`client_builder`] and are + /// *not* dispatch, with why a redirect there is not the same + /// question. Everything else must use + /// [`dispatch_client_builder`]. + const NON_DISPATCH: &[(&str, &str)] = &[ + ( + "aisix-mcp/src/oauth.rs", + "sets `Policy::none()` itself; an OAuth token endpoint never \ + legitimately redirects", + ), + ( + "aisix-mcp/src/openapi.rs", + "sets `Policy::none()` itself, for the generated tool calls", + ), + ( + "aisix-proxy/src/jwt.rs", + "sets `Policy::none()` itself; a JWKS endpoint never \ + legitimately redirects", + ), + ( + "aisix-a2a/src/bridge.rs", + "sets `Policy::none()` itself; an A2A agent does not redirect \ + its JSON-RPC endpoint", + ), + ( + "aisix-obs/src/otlp_http_sink.rs", + "the operator's own collector; an endpoint behind a rewrite is \ + ordinary and carries no vendor credential of ours", + ), + ( + "aisix-server/src/heartbeat.rs", + "the control plane the deployment is registered with", + ), + ( + "aisix-server/src/telemetry.rs", + "the control plane the deployment is registered with", + ), + ]; + + let crates_dir = std::path::Path::new(concat!(env!("CARGO_MANIFEST_DIR"), "/..")); + let mut dispatch_sites = 0; + let mut classified = std::collections::HashSet::new(); + let mut offenders = Vec::new(); + for file in rust_sources(crates_dir) { + // This module defines both builders. + if file.ends_with("upstream_http.rs") { + continue; + } + let path = file.to_string_lossy().replace('\\', "/"); + let src = std::fs::read_to_string(&file).expect("read source"); + for (n, line) in production_half(&src).lines().enumerate() { + // Prose about the builders is not a call to one. + if !line.contains("client_builder()") || line.trim_start().starts_with("//") { + continue; + } + if line.contains("dispatch_client_builder()") { + dispatch_sites += 1; + } else if let Some((named, _why)) = + NON_DISPATCH.iter().find(|(f, _)| path.ends_with(f)) + { + classified.insert(*named); + } else { + offenders.push(format!("{}:{}", file.display(), n + 1)); + } + } + } + + assert!( + offenders.is_empty(), + "these build an outbound client that follows redirects. If it \ + carries a caller's payload or a gateway-held credential, build \ + it from `dispatch_client_builder()`; if a redirect there is \ + genuinely ordinary, add it to NON_DISPATCH with the reason:\n{}", + offenders.join("\n"), + ); + assert!( + dispatch_sites >= 13, + "found {dispatch_sites} dispatch client construction sites, \ + expected at least 13 — the probe no longer matches the code and \ + this test proves nothing", + ); + let stale: Vec<_> = NON_DISPATCH + .iter() + .map(|(f, _)| *f) + .filter(|f| !classified.contains(f)) + .collect(); + assert!( + stale.is_empty(), + "these NON_DISPATCH entries no longer match a client_builder() \ + site; drop them so the list keeps meaning something:\n{}", + stale.join("\n"), + ); + } + fn rust_sources(dir: &std::path::Path) -> Vec { let mut out = Vec::new(); let Ok(entries) = std::fs::read_dir(dir) else { diff --git a/crates/aisix-gateway/src/upstream_tls.rs b/crates/aisix-gateway/src/upstream_tls.rs index 908ac655..1acb422d 100644 --- a/crates/aisix-gateway/src/upstream_tls.rs +++ b/crates/aisix-gateway/src/upstream_tls.rs @@ -257,7 +257,7 @@ fn worker_client() -> Option { } WORKER_CLIENT.with(|cell| { cell.get_or_init(|| { - match crate::upstream_http::client_builder() + match crate::upstream_http::dispatch_client_builder() .user_agent(DISPATCH_USER_AGENT) .build() { @@ -321,7 +321,8 @@ fn build_provider_key_client(tls: &ProviderKeyTls) -> Result rmcp_reqwest::Client { .get_or_init(|| { let cfg = aisix_gateway::upstream_http::config(); let mut b = rmcp_reqwest::Client::builder() + .redirect(rmcp_reqwest::redirect::Policy::none()) .pool_idle_timeout(cfg.pool_idle_timeout) .tcp_keepalive(cfg.tcp_keepalive); if let Some(d) = cfg.connect_timeout { @@ -112,7 +118,15 @@ fn shared_http_client() -> rmcp_reqwest::Client { if !cfg.tls.verify { b = b.danger_accept_invalid_certs(true); } - b.build().unwrap_or_else(|_| rmcp_reqwest::Client::new()) + // The connection settings are lost if the build fails; the + // redirect refusal must not be, which `Client::new()` would + // silently give back. + b.build().unwrap_or_else(|_| { + rmcp_reqwest::Client::builder() + .redirect(rmcp_reqwest::redirect::Policy::none()) + .build() + .expect("a client with only a redirect policy always builds") + }) }) .clone() } diff --git a/crates/aisix-provider-anthropic/src/bridge.rs b/crates/aisix-provider-anthropic/src/bridge.rs index b8d71762..c9ba8419 100644 --- a/crates/aisix-provider-anthropic/src/bridge.rs +++ b/crates/aisix-provider-anthropic/src/bridge.rs @@ -78,10 +78,10 @@ impl Default for AnthropicBridge { } fn default_client() -> Client { - aisix_gateway::client_builder() + aisix_gateway::dispatch_client_builder() .user_agent("aisix/0.1") .build() - .unwrap_or_else(|_| Client::new()) + .unwrap_or_else(|_| aisix_gateway::dispatch_client_fallback()) } /// Path suffixes the Anthropic bridge appends. If an operator diff --git a/crates/aisix-provider-azure-openai/src/bridge.rs b/crates/aisix-provider-azure-openai/src/bridge.rs index e1cece7f..80838d72 100644 --- a/crates/aisix-provider-azure-openai/src/bridge.rs +++ b/crates/aisix-provider-azure-openai/src/bridge.rs @@ -186,10 +186,10 @@ impl Default for AzureOpenAiBridge { } fn default_client() -> Client { - aisix_gateway::client_builder() + aisix_gateway::dispatch_client_builder() .user_agent("aisix/0.1") .build() - .unwrap_or_else(|_| Client::new()) + .unwrap_or_else(|_| aisix_gateway::dispatch_client_fallback()) } /// Parsed Azure upstream reference resolved from a provider_key's diff --git a/crates/aisix-provider-openai/src/bridge.rs b/crates/aisix-provider-openai/src/bridge.rs index cc613c82..60774e6a 100644 --- a/crates/aisix-provider-openai/src/bridge.rs +++ b/crates/aisix-provider-openai/src/bridge.rs @@ -159,10 +159,10 @@ impl Default for OpenAiBridge { } fn default_client() -> Client { - aisix_gateway::client_builder() + aisix_gateway::dispatch_client_builder() .user_agent("aisix/0.1") .build() - .unwrap_or_else(|_| Client::new()) + .unwrap_or_else(|_| aisix_gateway::dispatch_client_fallback()) } /// Strip a known endpoint suffix from `base`. Idempotent: if no known @@ -913,6 +913,50 @@ mod tests { } } + /// An upstream 3xx has to surface as an upstream status, which the + /// proxy renders as a 502 — the behavior `BridgeError::http_status` + /// has always described. reqwest follows up to 10 redirects by + /// default and the gateway had never opted out, so a redirected POST + /// was silently re-issued as a `GET` against the `Location` host + /// (RFC 9110 §15.4.2, as tower-http's follow-redirect middleware + /// implements it) and that host's answer came back as the completion. + /// + /// The second assertion is the one that matters: the redirect target + /// must never be dialed at all. + #[tokio::test] + async fn upstream_redirects_are_not_followed() { + let elsewhere = MockServer::start().await; + mount_ok(&elsewhere).await; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(301).insert_header( + "location", + format!("{}/chat/completions", elsewhere.uri()).as_str(), + )) + .mount(&server) + .await; + + let bridge = OpenAiBridge::new(); + let ctx = sample_ctx(&server.uri()); + let err = bridge.chat(&req(), &ctx).await.unwrap_err(); + assert_eq!( + err.http_status(), + 502, + "a redirect the gateway does not follow is 502-worthy: {err:?}", + ); + match err { + BridgeError::UpstreamStatus { status, .. } => assert_eq!(status, 301), + other => panic!("unexpected: {other:?}"), + } + assert!( + elsewhere.received_requests().await.unwrap().is_empty(), + "the redirect target was dialed; the request and the key's \ + credential left for a host the operator never configured", + ); + } + /// Audit fix (PR #323): the [`aisix_gateway::MAX_UPSTREAM_ERROR_BODY_BYTES`] /// (64 KB) cap must actually fire on an oversized upstream error /// body, otherwise a misbehaved upstream could pin a worker's diff --git a/crates/aisix-provider-vertex/src/bridge.rs b/crates/aisix-provider-vertex/src/bridge.rs index afe7d2f4..49102d5e 100644 --- a/crates/aisix-provider-vertex/src/bridge.rs +++ b/crates/aisix-provider-vertex/src/bridge.rs @@ -274,10 +274,10 @@ impl Default for VertexBridge { } fn default_client() -> Client { - aisix_gateway::client_builder() + aisix_gateway::dispatch_client_builder() .user_agent("aisix/0.1") .build() - .unwrap_or_else(|_| Client::new()) + .unwrap_or_else(|_| aisix_gateway::dispatch_client_fallback()) } /// The set of Vertex publishers we dispatch to. diff --git a/crates/aisix-proxy/src/http_client.rs b/crates/aisix-proxy/src/http_client.rs index dc407fad..8a307e28 100644 --- a/crates/aisix-proxy/src/http_client.rs +++ b/crates/aisix-proxy/src/http_client.rs @@ -14,10 +14,10 @@ use std::sync::OnceLock; pub fn client() -> &'static Client { static CLIENT: OnceLock = OnceLock::new(); CLIENT.get_or_init(|| { - aisix_gateway::client_builder() + aisix_gateway::dispatch_client_builder() .user_agent("aisix/0.1") .build() - .unwrap_or_else(|_| Client::new()) + .unwrap_or_else(|_| aisix_gateway::dispatch_client_fallback()) }) } diff --git a/tests/e2e/src/cases/upstream-redirect-not-followed-e2e.test.ts b/tests/e2e/src/cases/upstream-redirect-not-followed-e2e.test.ts new file mode 100644 index 00000000..dc887bcd --- /dev/null +++ b/tests/e2e/src/cases/upstream-redirect-not-followed-e2e.test.ts @@ -0,0 +1,149 @@ +import { createHash } from "node:crypto"; +import { createServer, type Server as HttpServer } from "node:http"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + EtcdClient, + SeedClient, + spawnApp, + startOpenAiUpstream, + waitConfigPropagation, + type OpenAiUpstream, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: an upstream that answers a dispatched request with a redirect must +// not be followed. +// +// The configured endpoint is the only host an operator has authorised the +// gateway to send a caller's prompt and the Provider Key's credential to. +// A redirect names a different one, chosen by the upstream at request +// time, and nothing on the caller's side — status, body, or access log — +// would say the answer came from somewhere else. +// +// The scenario: the configured endpoint answers `301` with a `Location` +// pointing at a second, perfectly healthy OpenAI-shaped upstream. If the +// gateway follows, the caller gets that second host's completion and the +// call looks entirely successful. It must instead fail the request as a +// bad gateway, and the second host must never be dialed. + +const CALLER_PLAINTEXT = "sk-redirect-caller"; +const CALLER_KEY_HASH = createHash("sha256") + .update(CALLER_PLAINTEXT) + .digest("hex"); + +describe("upstream redirects are not followed", () => { + let app: SpawnedApp | undefined; + let elsewhere: OpenAiUpstream | undefined; + let redirector: HttpServer | undefined; + let redirectorHits = 0; + let seed: SeedClient | undefined; + let etcdReachable = false; + + beforeAll(async () => { + const etcd = new EtcdClient(); + etcdReachable = await etcd.ping(); + if (!etcdReachable) return; + + // The host the redirect points at: a healthy upstream that would + // happily answer, so following the redirect produces a *success* + // rather than an error — the failure mode that hides itself. + elsewhere = await startOpenAiUpstream(); + + const location = `${elsewhere.baseUrl}/v1/chat/completions`; + redirector = createServer((req, res) => { + redirectorHits += 1; + req.resume(); + req.on("end", () => { + res.writeHead(301, { location }); + res.end(); + }); + }); + await new Promise((resolve) => + redirector!.listen(0, "127.0.0.1", resolve), + ); + const address = redirector.address(); + if (address === null || typeof address === "string") { + throw new Error("redirecting upstream: no listen address"); + } + + app = await spawnApp(); + seed = new SeedClient(etcd, app.etcdPrefix); + + const pk = await seed.createProviderKey({ + display_name: "redirect-pk", + secret: "sk-mock-redirect", + api_base: `http://127.0.0.1:${address.port}/v1`, + }); + await seed.createModel({ + display_name: "redirect-gpt", + provider: "openai", + model_name: "gpt-4o", + provider_key_id: pk.id, + }); + await seed.createApiKey({ + key_hash: CALLER_KEY_HASH, + allowed_models: ["redirect-gpt"], + }); + }); + + afterAll(async () => { + await app?.exit(); + await elsewhere?.close(); + await new Promise((resolve) => + redirector ? redirector.close(() => resolve()) : resolve(), + ); + }); + + test("a 301 from the configured endpoint fails the call instead of hopping", async (ctx) => { + if (!etcdReachable || !app) { + ctx.skip(); + return; + } + + await waitConfigPropagation(async () => { + try { + const models = await fetch(`${app!.proxyUrl}/v1/models`, { + headers: { authorization: `Bearer ${CALLER_PLAINTEXT}` }, + }); + if (models.status !== 200) return false; + const ids = + ((await models.json()) as { data?: Array<{ id?: string }> }).data?.map( + (m) => m.id, + ) ?? []; + return ids.includes("redirect-gpt"); + } catch { + return false; + } + }); + + const res = await fetch(`${app.proxyUrl}/v1/chat/completions`, { + method: "POST", + headers: { + "content-type": "application/json", + authorization: `Bearer ${CALLER_PLAINTEXT}`, + }, + body: JSON.stringify({ + model: "redirect-gpt", + messages: [{ role: "user", content: "hello" }], + }), + }); + + const body = (await res.json()) as { + error?: { message?: string; type?: string }; + choices?: unknown[]; + }; + + // The upstream gave the gateway nothing it can answer with, which is + // what a bad gateway is. + expect(res.status, JSON.stringify(body)).toBe(502); + expect(body.error, JSON.stringify(body)).toBeDefined(); + expect(body.choices).toBeUndefined(); + + // The point of the case: the prompt and the key's credential never + // left for the host the operator did not configure. + expect(redirectorHits).toBeGreaterThan(0); + expect( + elsewhere!.receivedRequests.map((r) => `${r.method} ${r.path}`), + ).toEqual([]); + }); +});