From 02591432605ca2290dc8f6cb7e2f962e62153c35 Mon Sep 17 00:00:00 2001 From: Jasper Date: Wed, 19 Aug 2026 22:45:10 +0000 Subject: [PATCH] fix(security): bind Foundry API keys to request origin (#11347) Signed-off-by: Jasper Hugo --- crates/goose-providers/src/api_client.rs | 29 ++++- crates/goose-providers/src/azure_foundry.rs | 123 +++++++++++++++++- .../goose/src/providers/azure_foundry_def.rs | 83 +++++++----- 3 files changed, 198 insertions(+), 37 deletions(-) diff --git a/crates/goose-providers/src/api_client.rs b/crates/goose-providers/src/api_client.rs index 39e3bd19d..d5df392ff 100644 --- a/crates/goose-providers/src/api_client.rs +++ b/crates/goose-providers/src/api_client.rs @@ -34,11 +34,12 @@ pub struct ApiClient { transport_policy: TransportPolicy, } -#[derive(Clone, Copy)] +#[derive(Clone)] enum TransportPolicy { Default, HttpsOnly, LoopbackHttp, + SameOrigin(url::Origin), } pub enum AuthMethod { @@ -313,7 +314,7 @@ impl ApiClient { fn rebuild_client(&mut self) -> Result<()> { let mut client_builder = Self::client_builder(self.timeout).default_headers(self.default_headers.clone()); - client_builder = Self::configure_transport(client_builder, self.transport_policy); + client_builder = Self::configure_transport(client_builder, &self.transport_policy); // Configure TLS if needed if let Some(ref tls_config) = self.tls_config { @@ -326,7 +327,7 @@ impl ApiClient { fn configure_transport( client_builder: reqwest::ClientBuilder, - transport_policy: TransportPolicy, + transport_policy: &TransportPolicy, ) -> reqwest::ClientBuilder { match transport_policy { TransportPolicy::Default => client_builder, @@ -353,6 +354,19 @@ impl ApiClient { } })) } + TransportPolicy::SameOrigin(origin) => { + let origin = origin.clone(); + client_builder.redirect(Policy::custom(move |attempt| { + if attempt.previous().len() >= 10 { + return attempt.error("too many redirects"); + } + if attempt.url().origin() == origin { + attempt.follow() + } else { + attempt.error("redirect crosses the authenticated request origin") + } + })) + } } } @@ -427,6 +441,15 @@ impl ApiClient { Ok(self) } + pub fn with_same_origin_redirects(mut self) -> Result { + let origin = url::Url::parse(&self.host) + .map_err(|error| anyhow::anyhow!("Invalid base URL: {}", error))? + .origin(); + self.transport_policy = TransportPolicy::SameOrigin(origin); + self.rebuild_client()?; + Ok(self) + } + pub fn request<'a>(&'a self, path: &'a str) -> ApiRequestBuilder<'a> { ApiRequestBuilder { client: self, diff --git a/crates/goose-providers/src/azure_foundry.rs b/crates/goose-providers/src/azure_foundry.rs index 39c260d70..536d62915 100644 --- a/crates/goose-providers/src/azure_foundry.rs +++ b/crates/goose-providers/src/azure_foundry.rs @@ -374,7 +374,11 @@ fn configured_client( tls_config: Option, request_builder: Option, ) -> Result { + let restrict_redirects = matches!(&auth, AuthMethod::ApiKey { .. }); let mut client = ApiClient::new_with_tls(host, auth, tls_config)?; + if restrict_redirects { + client = client.with_same_origin_redirects()?; + } if let Some(request_builder) = request_builder { client = client.with_request_builder(request_builder); } @@ -547,7 +551,7 @@ impl Provider for AzureFoundryProvider { mod tests { use super::*; use serde_json::json; - use wiremock::matchers::{body_partial_json, method, path, query_param}; + use wiremock::matchers::{body_partial_json, header, method, path, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; fn project_endpoint(server: &MockServer) -> String { @@ -733,6 +737,123 @@ mod tests { ); } + #[tokio::test] + async fn foundry_api_keys_do_not_follow_cross_origin_redirects() { + for header_name in ["api-key", "x-api-key"] { + let destination = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/capture")) + .respond_with(ResponseTemplate::new(200)) + .mount(&destination) + .await; + + let source = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/redirect")) + .and(header(header_name, "foundry-secret")) + .respond_with( + ResponseTemplate::new(307) + .append_header("location", format!("{}/capture", destination.uri())), + ) + .expect(1) + .mount(&source) + .await; + + let client = configured_client( + source.uri(), + AuthMethod::ApiKey { + header_name: header_name.to_string(), + key: "foundry-secret".to_string(), + }, + None, + None, + ) + .unwrap(); + + assert!(client.response_get("redirect").await.is_err()); + assert!(destination.received_requests().await.unwrap().is_empty()); + } + } + + #[tokio::test] + async fn foundry_api_keys_follow_same_origin_redirects() { + for header_name in ["api-key", "x-api-key"] { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/redirect")) + .respond_with(ResponseTemplate::new(307).append_header("location", "/final")) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/final")) + .and(header(header_name, "foundry-secret")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&server) + .await; + + let client = configured_client( + server.uri(), + AuthMethod::ApiKey { + header_name: header_name.to_string(), + key: "foundry-secret".to_string(), + }, + None, + None, + ) + .unwrap(); + + assert!(client + .response_get("redirect") + .await + .unwrap() + .status() + .is_success()); + } + } + + #[tokio::test] + async fn foundry_bearer_redirects_keep_reqwest_authorization_behavior() { + let destination = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/capture")) + .respond_with(ResponseTemplate::new(200)) + .expect(1) + .mount(&destination) + .await; + + let source = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/redirect")) + .and(header("authorization", "Bearer foundry-token")) + .respond_with( + ResponseTemplate::new(307) + .append_header("location", format!("{}/capture", destination.uri())), + ) + .expect(1) + .mount(&source) + .await; + + let client = configured_client( + source.uri(), + AuthMethod::BearerToken("foundry-token".to_string()), + None, + None, + ) + .unwrap(); + + assert!(client + .response_get("redirect") + .await + .unwrap() + .status() + .is_success()); + let requests = destination.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + assert!(requests[0].headers.get("authorization").is_none()); + } + #[test] fn deployment_metadata_enriches_context_without_pricing() { let info = model_info_for_deployment("production-chat", "gpt-5"); diff --git a/crates/goose/src/providers/azure_foundry_def.rs b/crates/goose/src/providers/azure_foundry_def.rs index ad961059f..6837834c8 100644 --- a/crates/goose/src/providers/azure_foundry_def.rs +++ b/crates/goose/src/providers/azure_foundry_def.rs @@ -24,6 +24,20 @@ struct AzureFoundryAuthProvider { header: AuthHeader, } +fn auth_method(auth: &Arc, header: AuthHeader) -> AuthMethod { + match (auth.credential_type(), header) { + (AzureCredentials::ApiKey(key), AuthHeader::ApiKey) => AuthMethod::ApiKey { + header_name: "api-key".to_string(), + key: key.clone(), + }, + (AzureCredentials::ApiKey(key), AuthHeader::Bearer) => AuthMethod::BearerToken(key.clone()), + (_, header) => AuthMethod::Custom(Box::new(AzureFoundryAuthProvider { + auth: Arc::clone(auth), + header, + })), + } +} + #[async_trait] impl AuthProvider for AzureFoundryAuthProvider { async fn get_auth_header(&self) -> Result<(String, String)> { @@ -89,18 +103,12 @@ pub async fn from_env(tls_config: Option) -> Result AuthMethod::ApiKey { header_name: "x-api-key".to_string(), key: key.clone(), }, - _ => auth_method(AuthHeader::Bearer), + _ => auth_method(&auth, AuthHeader::Bearer), }; let api_key_auth_header = || match auth.credential_type() { AzureCredentials::ApiKey(_) => AuthHeader::ApiKey, @@ -115,10 +123,10 @@ pub async fn from_env(tls_config: Option) -> Result) -> Result, - ad_token: Option<&str>, - header: AuthHeader, - ) -> (String, String) { - let auth = Arc::new( + fn auth(api_key: Option<&str>, ad_token: Option<&str>) -> Arc { + Arc::new( AzureAuth::new_with_resource( api_key.map(str::to_string), ad_token.map(str::to_string), AZURE_PROJECT_ENTRA_RESOURCE.to_string(), ) .unwrap(), - ); - AzureFoundryAuthProvider { auth, header } - .get_auth_header() - .await - .unwrap() + ) } - #[tokio::test] - async fn project_api_key_uses_api_key_header() { - assert_eq!( - header(Some("key"), None, AuthHeader::ApiKey).await, - ("api-key".to_string(), "key".to_string()) - ); + async fn header( + api_key: Option<&str>, + ad_token: Option<&str>, + header: AuthHeader, + ) -> (String, String) { + AzureFoundryAuthProvider { + auth: auth(api_key, ad_token), + header, + } + .get_auth_header() + .await + .unwrap() } - #[tokio::test] - async fn maas_api_key_uses_bearer_header() { - assert_eq!( - header(Some("key"), None, AuthHeader::Bearer).await, - ("Authorization".to_string(), "Bearer key".to_string()) - ); + #[test] + fn project_api_key_uses_origin_bound_auth_method() { + match auth_method(&auth(Some("key"), None), AuthHeader::ApiKey) { + AuthMethod::ApiKey { header_name, key } => { + assert_eq!(header_name, "api-key"); + assert_eq!(key, "key"); + } + _ => panic!("project API keys must use the origin-bound API key auth method"), + } + } + + #[test] + fn maas_api_key_uses_standard_bearer_auth_method() { + match auth_method(&auth(Some("key"), None), AuthHeader::Bearer) { + AuthMethod::BearerToken(token) => assert_eq!(token, "key"), + _ => panic!("MaaS API keys must use the standard bearer auth method"), + } } #[tokio::test]