From b0c97b34caa6343db278a9c5c09228248acf970c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 17 Oct 2026 10:36:22 +0200 Subject: [PATCH] fix: let ALLOW_PRIVATE_AI_BASE_URLS name the private hosts it allows (#11841) * fix: refuse dot segments in AI proxy paths Co-Authored-By: Claude Opus 5.5 * fix: refuse spaces, query and fragment markers in AI proxy paths Co-Authored-By: Claude Opus 5.5 * fix: refuse percent signs in AI proxy paths Co-Authored-By: Claude Opus 5.5 * fix: let ALLOW_PRIVATE_AI_BASE_URLS name the private hosts it allows Co-Authored-By: Claude Opus 5.5 * fix: refuse loose and wildcard allowlist entries, judge embedded v4 Co-Authored-By: Claude Opus 5.5 * fix: never admit local-use NAT64 addresses through an allowlist net Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- backend/Cargo.lock | 1 + backend/Cargo.toml | 1 + backend/windmill-ai/src/ai_providers.rs | 45 +++-- backend/windmill-ai/src/utils.rs | 23 +-- backend/windmill-api/src/ai.rs | 21 +-- backend/windmill-common/Cargo.toml | 1 + backend/windmill-common/src/ssrf.rs | 238 ++++++++++++++++++++++++ 7 files changed, 275 insertions(+), 55 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index b17f8976f1..2b553c9396 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -16347,6 +16347,7 @@ dependencies = [ "hmac 0.12.1", "hyper 1.12.0", "indexmap 2.14.2", + "ipnet", "itertools 0.14.0", "jsonwebtoken 10.4.0", "lazy_static", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 7513c66af1..070ea1ce5f 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -501,6 +501,7 @@ mail-send = { version = "0.4.0", features = ["builder"], default-features=false urlencoding = "^2" quick-xml = { version = "^0.37", features = ["serialize"] } url = { version = "^2" , features = ["serde"]} +ipnet = "2" async-oauth2 = "0.5.1" reqwest = { version = "^0.13", features = ["json", "stream", "gzip", "multipart", "query", "form"] } # The reqwest object_store builds its HTTP clients with, for a custom `HttpConnector`. diff --git a/backend/windmill-ai/src/ai_providers.rs b/backend/windmill-ai/src/ai_providers.rs index 3b3af833d6..311c8d3eb7 100644 --- a/backend/windmill-ai/src/ai_providers.rs +++ b/backend/windmill-ai/src/ai_providers.rs @@ -5,6 +5,9 @@ use serde::{Deserialize, Deserializer, Serialize}; use windmill_common::db::DB; use windmill_common::error::{Error, Result}; +use windmill_common::ssrf::{PrivateHostAllowance, SsrfValidationError, ValidatedTarget}; + +const ALLOW_PRIVATE_AI_BASE_URLS_ENV: &str = "ALLOW_PRIVATE_AI_BASE_URLS"; /// Deserializes an Option where empty strings become None. /// Use with `#[serde(default, deserialize_with = "empty_string_as_none")]` @@ -20,10 +23,10 @@ where lazy_static::lazy_static! { static ref OPENAI_AZURE_BASE_PATH: Option = std::env::var("OPENAI_AZURE_BASE_PATH").ok(); - pub static ref ALLOW_PRIVATE_AI_BASE_URLS: bool = std::env::var("ALLOW_PRIVATE_AI_BASE_URLS") - .ok() - .map(|v| v == "true" || v == "1") - .unwrap_or(false); + /// Covers every AI base URL, whoever supplied it: the workspace or instance AI config, a + /// resource a member picked, or an inline resource value sent to the proxy. + pub static ref ALLOW_PRIVATE_AI_BASE_URLS: PrivateHostAllowance = + PrivateHostAllowance::from_env(ALLOW_PRIVATE_AI_BASE_URLS_ENV); /// Drops the cache breakpoints from agent-step requests on every Anthropic platform, /// not just the one that motivates it: a Google Cloud project can have explicit prompt /// caching turned off (by request to Cloud support), and Vertex then rejects any request @@ -131,6 +134,23 @@ pub enum AIProvider { Cloudflare, } +/// Check an AI endpoint URL against the SSRF rules, allowing the private hosts +/// `ALLOW_PRIVATE_AI_BASE_URLS` names. Callers that connect next pin the returned target. +pub async fn validate_ai_url(url: &str) -> Result { + windmill_common::ssrf::validate_url_for_ssrf_allowing(url, &ALLOW_PRIVATE_AI_BASE_URLS) + .await + .map_err(|e| match e { + // Only a private target is fixed by the env var; for a malformed URL or bad + // scheme the hint would send users the wrong way (issue #9171). + e @ SsrfValidationError::Private { .. } => Error::BadRequest(format!( + "{e}. To use a private/internal AI endpoint, add its host (host, host:port or \ + CIDR range) to the comma-separated {ALLOW_PRIVATE_AI_BASE_URLS_ENV} environment \ + variable, or set it to true to allow every private host" + )), + e => Error::from(e), + }) +} + /// The Workers AI models URL of a Cloudflare account. The id goes into the path, so it is held to /// the characters an account id has rather than trusted to stay inside its segment. pub fn cloudflare_workers_ai_base_url(account_id: Option<&str>) -> Result { @@ -161,22 +181,7 @@ impl AIProvider { /// Get the base URL for the AI provider pub async fn get_base_url(&self, resource_base_url: Option, db: &DB) -> Result { if let Some(base_url) = resource_base_url { - if !*ALLOW_PRIVATE_AI_BASE_URLS { - use windmill_common::ssrf::SsrfValidationError; - windmill_common::ssrf::validate_url_for_ssrf(&base_url) - .await - .map_err(|e| match e { - // The env-var hint is only actionable when the URL is - // well-formed but blocked for targeting a private - // address. For a malformed URL or bad scheme, surface - // the real error so users fix the URL (issue #9171). - e @ SsrfValidationError::Private { .. } => Error::BadRequest(format!( - "{e}. If you need to use private/internal AI endpoints, \ - set the ALLOW_PRIVATE_AI_BASE_URLS=true environment variable" - )), - e => Error::from(e), - })?; - } + validate_ai_url(&base_url).await?; return Ok(base_url); } diff --git a/backend/windmill-ai/src/utils.rs b/backend/windmill-ai/src/utils.rs index 328d2e0458..de7efda183 100644 --- a/backend/windmill-ai/src/utils.rs +++ b/backend/windmill-ai/src/utils.rs @@ -88,30 +88,17 @@ pub fn ai_http_client_builder() -> reqwest::ClientBuilder { /// after the check (DNS-rebinding TOCTOU). /// /// Returns the shared pooled [`AI_HTTP_CLIENT`] unchanged when there is nothing -/// to pin — an IP-literal host, or a deployment that opted into private AI -/// endpoints via `ALLOW_PRIVATE_AI_BASE_URLS`. The same opt-out and error hint -/// as `AIProvider::get_base_url` apply, so this is consistent with the +/// to pin — an IP-literal host, or a host `ALLOW_PRIVATE_AI_BASE_URLS` allows +/// by name or wholesale. The same allowlist and error hint as +/// `AIProvider::get_base_url` apply, so this is consistent with the /// credential-time validation while additionally closing the connect-time window. pub async fn pinned_ai_client_for( url: &str, ) -> windmill_common::error::Result> { use std::borrow::Cow; - use windmill_common::error::{to_anyhow, Error}; - use windmill_common::ssrf::SsrfValidationError; + use windmill_common::error::to_anyhow; - if *crate::ai_providers::ALLOW_PRIVATE_AI_BASE_URLS { - return Ok(Cow::Borrowed(&AI_HTTP_CLIENT)); - } - - let target = windmill_common::ssrf::validate_url_for_ssrf(url) - .await - .map_err(|e| match e { - e @ SsrfValidationError::Private { .. } => Error::BadRequest(format!( - "{e}. If you need to use private/internal AI endpoints, \ - set the ALLOW_PRIVATE_AI_BASE_URLS=true environment variable" - )), - e => Error::from(e), - })?; + let target = crate::ai_providers::validate_ai_url(url).await?; if target.pinned_addrs().is_empty() { return Ok(Cow::Borrowed(&AI_HTTP_CLIENT)); diff --git a/backend/windmill-api/src/ai.rs b/backend/windmill-api/src/ai.rs index fcffa7b227..8de5e16c11 100644 --- a/backend/windmill-api/src/ai.rs +++ b/backend/windmill-api/src/ai.rs @@ -166,27 +166,14 @@ fn ai_http_client_builder() -> reqwest::ClientBuilder { /// after the check (DNS-rebinding TOCTOU). /// /// Returns the shared pooled [`HTTP_CLIENT`] unchanged when there is nothing to -/// pin — an IP-literal host, or a deployment that opted into private AI -/// endpoints via `ALLOW_PRIVATE_AI_BASE_URLS`. The same opt-out and error hint -/// as `get_base_url` apply, so the guard here is consistent with save-time +/// pin — an IP-literal host, or a host `ALLOW_PRIVATE_AI_BASE_URLS` allows +/// by name or wholesale. The same allowlist and error hint as `get_base_url` +/// apply, so the guard here is consistent with save-time /// validation while additionally closing the connect-time window. async fn pinned_ai_client_for(url: &str) -> Result> { use std::borrow::Cow; - use windmill_common::ssrf::SsrfValidationError; - if *windmill_ai::ai_providers::ALLOW_PRIVATE_AI_BASE_URLS { - return Ok(Cow::Borrowed(&HTTP_CLIENT)); - } - - let target = windmill_common::ssrf::validate_url_for_ssrf(url) - .await - .map_err(|e| match e { - e @ SsrfValidationError::Private { .. } => Error::BadRequest(format!( - "{e}. If you need to use private/internal AI endpoints, \ - set the ALLOW_PRIVATE_AI_BASE_URLS=true environment variable" - )), - e => Error::from(e), - })?; + let target = windmill_ai::ai_providers::validate_ai_url(url).await?; if target.pinned_addrs().is_empty() { return Ok(Cow::Borrowed(&HTTP_CLIENT)); diff --git a/backend/windmill-common/Cargo.toml b/backend/windmill-common/Cargo.toml index e447baf563..e085fa2f78 100644 --- a/backend/windmill-common/Cargo.toml +++ b/backend/windmill-common/Cargo.toml @@ -110,6 +110,7 @@ strum.workspace = true strum_macros.workspace = true windmill-types.workspace = true url.workspace = true +ipnet.workspace = true urlencoding.workspace = true async-recursion.workspace = true pep440_rs.workspace = true diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs index 583b1e81ab..3f6dda0327 100644 --- a/backend/windmill-common/src/ssrf.rs +++ b/backend/windmill-common/src/ssrf.rs @@ -396,6 +396,188 @@ pub fn webhook_ssrf_error_message(e: &SsrfValidationError) -> String { } } +/// Which private hosts an `ALLOW_PRIVATE_*` opt-in lets a URL reach. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub enum PrivateHostAllowance { + /// Private hosts are refused. + #[default] + None, + /// Every private host is allowed; only the URL's scheme and host are checked (`true` / `1`). + All, + /// Only the private hosts these entries name are allowed. + Hosts(Vec), +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum AllowedPrivateHost { + /// A host name, trusted whatever it resolves to. + Name { name: String, port: Option }, + /// An address or network a URL's host may resolve into. + Net { net: ipnet::IpNet, port: Option }, +} + +impl AllowedPrivateHost { + fn parse(entry: &str) -> Option { + if let Ok(net) = entry.parse::() { + return Some(Self::Net { net: net.trunc(), port: None }); + } + let unbracketed = entry + .strip_prefix('[') + .and_then(|e| e.strip_suffix(']')) + .unwrap_or(entry); + if let Ok(ip) = unbracketed.parse::() { + return Some(Self::Net { net: ip.into(), port: None }); + } + if let Ok(addr) = entry.parse::() { + return Some(Self::Net { net: addr.ip().into(), port: Some(addr.port()) }); + } + let (host, port) = match entry.rsplit_once(':') { + Some((host, port)) => (host, Some(port.parse::().ok()?)), + None => (entry, None), + }; + // Normalized the way `url::Url` normalizes the host it is compared against. An address + // reaching here is a loose WHATWG form (`10` is 0.0.0.10, not 10/8) and a `*` is not a + // wildcard, so both are refused rather than read as something the operator never meant. + match url::Host::parse(host.trim_end_matches('.')).ok()? { + url::Host::Domain(name) if !name.is_empty() && !name.contains('*') => { + Some(Self::Name { name, port }) + } + _ => None, + } + } +} + +fn port_allowed(allowed: Option, port: u16) -> bool { + allowed.map_or(true, |p| p == port) +} + +impl PrivateHostAllowance { + /// `true`/`1` allows every private host; anything else is a comma-separated list of + /// `host`, `host:port`, IP address, `ip:port` (`[v6]:port`) or CIDR entries. Also returns + /// the entries that parse as none of these, which allow nothing. + pub fn parse(value: &str) -> (Self, Vec) { + match value.trim() { + "true" | "1" => return (Self::All, vec![]), + "" | "false" | "0" => return (Self::None, vec![]), + _ => {} + } + let mut hosts = vec![]; + let mut invalid = vec![]; + for entry in value.split(',').map(str::trim).filter(|e| !e.is_empty()) { + match AllowedPrivateHost::parse(entry) { + Some(host) => hosts.push(host), + None => invalid.push(entry.to_string()), + } + } + let allowance = if hosts.is_empty() { + Self::None + } else { + Self::Hosts(hosts) + }; + (allowance, invalid) + } + + pub fn from_env(var: &str) -> Self { + let Ok(value) = std::env::var(var) else { + return Self::None; + }; + let (allowance, invalid) = Self::parse(&value); + for entry in invalid { + tracing::warn!( + "Ignoring {var} entry {entry:?}: expected a host, host:port, IP address or CIDR range" + ); + } + allowance + } + + fn allows_name(&self, name: &str, port: u16) -> bool { + let name = name.trim_end_matches('.'); + matches!(self, Self::Hosts(hosts) if hosts.iter().any(|h| matches!(h, + AllowedPrivateHost::Name { name: n, port: p } if n == name && port_allowed(*p, port)))) + } + + fn allows_ip(&self, ip: &IpAddr, port: u16) -> bool { + // Judged by where it is delivered: an allowed v6 range such as an operator's NAT64 + // prefix must not carry a request on to an embedded private v4 like 169.254.169.254. + let ip = &match ip { + // The local-use NAT64 prefix places its v4 by a prefix length only the site knows, + // so the destination cannot be judged and no network entry admits it. + IpAddr::V6(v6) if v6.segments()[..3] == [0x0064, 0xff9b, 0x0001] => return false, + // `::1` and `::` sit inside the IPv4-compatible block but are v6's own. + IpAddr::V6(v6) if !v6.is_loopback() && !v6.is_unspecified() => { + embedded_ipv4(v6).map_or(*ip, IpAddr::V4) + } + IpAddr::V6(_) => *ip, + IpAddr::V4(_) => *ip, + }; + matches!(self, Self::Hosts(hosts) if hosts.iter().any(|h| matches!(h, + AllowedPrivateHost::Net { net, port: p } if net.contains(ip) && port_allowed(*p, port)))) + } +} + +/// [`validate_url_for_ssrf`], except that the URL may also reach the private hosts +/// `allowance` names. +/// +/// A host matched by name is trusted whatever it resolves to, so it is left unpinned. Any +/// other host must resolve only to public addresses or ones inside an allowed network, and +/// those addresses are pinned as usual. +pub async fn validate_url_for_ssrf_allowing( + url: &str, + allowance: &PrivateHostAllowance, +) -> Result { + if *allowance == PrivateHostAllowance::None { + return validate_url_for_ssrf(url).await; + } + + let parsed = + url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?; + let port = match parsed.scheme() { + "http" | "https" => parsed.port_or_known_default().unwrap_or(80), + scheme => return Err(SsrfValidationError::DisallowedScheme(scheme.to_string())), + }; + let host_str = parsed.host_str().ok_or(SsrfValidationError::MissingHost)?; + + if *allowance == PrivateHostAllowance::All { + return Ok(ValidatedTarget::unpinned(host_str)); + } + + let ip = match parsed.host().ok_or(SsrfValidationError::MissingHost)? { + url::Host::Domain(name) => { + if allowance.allows_name(name, port) { + return Ok(ValidatedTarget::unpinned(host_str)); + } + None + } + url::Host::Ipv4(ip) => Some(IpAddr::V4(ip)), + url::Host::Ipv6(ip) => Some(IpAddr::V6(ip)), + }; + + if let Some(ip) = ip { + if is_private_ip(&ip) && !allowance.allows_ip(&ip, port) { + return Err(SsrfValidationError::Private { resolved: false }); + } + return Ok(ValidatedTarget::unpinned(host_str)); + } + + let addrs: Vec = tokio::net::lookup_host(format!("{host_str}:{port}")) + .await + .map_err(|e| SsrfValidationError::ResolutionFailed { + host: host_str.to_string(), + source: e.to_string(), + })? + .collect(); + if addrs.is_empty() { + return Err(SsrfValidationError::NoAddresses(host_str.to_string())); + } + for addr in &addrs { + if is_private_ip(&addr.ip()) && !allowance.allows_ip(&addr.ip(), port) { + return Err(SsrfValidationError::Private { resolved: true }); + } + } + + Ok(ValidatedTarget { host: host_str.to_string(), addrs }) +} + /// Whether `ip` is loopback, private, link-local, or otherwise not a public /// internet destination. The one predicate every server-side outbound-URL guard /// (SSRF checks, git remotes, object storage tests) judges addresses with. @@ -877,4 +1059,60 @@ mod tests { !saml_ssrf_error_message(&invalid_error).contains(ALLOW_PRIVATE_SAML_METADATA_URLS_ENV) ); } + + #[test] + fn private_host_allowance_parses_flags_and_entries() { + assert_eq!(PrivateHostAllowance::parse("true").0, PrivateHostAllowance::All); + assert_eq!(PrivateHostAllowance::parse("false").0, PrivateHostAllowance::None); + let (parsed, invalid) = + PrivateHostAllowance::parse("Gateway.Internal., 10.1.0.0/16, x:y, 10, *.internal"); + // A bare `10` would otherwise be 0.0.0.10, and `*` is no wildcard. + assert_eq!(invalid, ["x:y", "10", "*.internal"]); + let PrivateHostAllowance::Hosts(hosts) = parsed else { + panic!("{parsed:?}") + }; + assert_eq!( + hosts[0], + AllowedPrivateHost::Name { name: "gateway.internal".into(), port: None } + ); + } + + #[tokio::test] + async fn private_host_allowance_admits_only_listed_hosts() { + let a = PrivateHostAllowance::parse( + "localhost:8080, 10.1.0.0/16, 127.0.0.2:9000, [::1]:443, 64:ff9b::/96, 64:ff9b:1::/48", + ) + .0; + let ok = |u: &'static str| { + let a = a.clone(); + async move { validate_url_for_ssrf_allowing(u, &a).await.is_ok() } + }; + // Trusted by name on its port only, so nothing to pin. + let target = validate_url_for_ssrf_allowing("http://localhost:8080/v1", &a) + .await + .unwrap(); + assert!(target.pinned_addrs().is_empty()); + assert!(!ok("http://localhost:8081/v1").await); + assert!(ok("http://10.1.2.3/v1").await); + assert!(ok("http://[::ffff:10.1.2.3]/v1").await); + assert!(!ok("http://10.2.0.1/v1").await); + assert!(ok("http://127.0.0.2:9000/v1").await); + assert!(!ok("http://127.0.0.2/v1").await); + assert!(ok("https://[::1]/v1").await); + assert!(!ok("http://[::1]/v1").await); + // An allowed v6 range does not reach the private v4 it embeds. + assert!(!ok("http://[64:ff9b::a9fe:a9fe]/").await); + assert!(!ok("http://[64:ff9b:1::a9fe:a9fe]/").await); + assert!(!ok("http://169.254.169.254/latest/meta-data").await); + assert!(ok("http://8.8.8.8/v1").await); + } + + #[tokio::test] + async fn private_host_allowance_pins_hosts_resolved_into_allowed_nets() { + let a = PrivateHostAllowance::parse("127.0.0.0/8, ::1").0; + let target = validate_url_for_ssrf_allowing("http://localhost:1234/", &a) + .await + .unwrap(); + assert!(!target.pinned_addrs().is_empty()); + } }