From ec6ec1b06febd235e8a0d1ae8f5175fd929b79e7 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 28 Sep 2026 17:43:15 +0200 Subject: [PATCH] fix: judge IPv4 embedded in IPv6 and pin the object storage test connect (#11389) * fix: judge IPv4 embedded in IPv6 and pin the object storage test connect Co-Authored-By: Claude Opus 5.5 (1M context) * fix: let the public-only object store client reach the egress proxy Co-Authored-By: Claude Opus 5.5 (1M context) * fix: refuse private IP literals and the proxy host in the public-only store client Co-Authored-By: Claude Opus 5.5 (1M context) * fix: refuse the egress proxy as a target whether named or an IP literal Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- backend/Cargo.lock | 1 + backend/Cargo.toml | 2 + backend/windmill-api-settings/src/lib.rs | 100 ++------- backend/windmill-common/src/ssrf.rs | 134 ++++++++--- backend/windmill-object-store/Cargo.toml | 2 + backend/windmill-object-store/src/lib.rs | 271 ++++++++++++++++++++++- backend/windmill-store/src/resources.rs | 107 +-------- 7 files changed, 391 insertions(+), 226 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 59eb6a4c6f..cfcc4addfb 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -15906,6 +15906,7 @@ dependencies = [ "lazy_static", "object_store", "quick_cache", + "reqwest 0.12.28", "reqwest 0.13.5", "serde", "serde_json", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 1a564f3644..8fa694ec36 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -511,6 +511,8 @@ quick-xml = { version = "^0.37", features = ["serialize"] } url = { version = "^2" , features = ["serde"]} 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`. +reqwest_object_store = { package = "reqwest", version = "0.12", default-features = false, features = ["rustls-tls-native-roots"] } eventsource-stream = "0.2.3" time = "^0" serde_urlencoded = "^0" diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index bc52d85d54..ca93d3c682 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -303,7 +303,9 @@ pub async fn test_email( use windmill_object_store::ObjectSettings; #[cfg(feature = "parquet")] -use windmill_object_store::build_object_store_from_settings; +use windmill_object_store::{ + build_object_store_from_settings, build_public_object_store_from_settings, +}; #[cfg(feature = "parquet")] pub async fn test_s3_bucket( @@ -338,9 +340,15 @@ pub async fn test_s3_bucket( })?; } - let client = build_object_store_from_settings(test_s3_bucket, Some(&db)) - .await? - .store; + // The restricted client re-judges every address it connects to: the checks above resolve the + // endpoint separately from the connect, which a rebinding name answers differently. + let client = if restrict { + build_public_object_store_from_settings(test_s3_bucket).await? + } else { + build_object_store_from_settings(test_s3_bucket, Some(&db)) + .await? + .store + }; let run = async { let mut list = client.list(Some( @@ -546,7 +554,7 @@ async fn validate_public_endpoint(endpoint: &str) -> error::Result<()> { // Reject if any resolved address is non-public, which also defeats the simplest DNS-rebinding // attempts (a name resolving to both a public and a private address). for addr in addrs { - if is_forbidden_ip(addr.ip()) { + if windmill_common::ssrf::is_private_ip(&addr.ip()) { // The resolved address stays out of the message: it is the server's resolver's // answer, and this message is only ever shown to the caller being constrained. return Err(error::Error::NotAuthorized(format!( @@ -599,51 +607,6 @@ fn extract_host(endpoint: &str) -> Option { } } -#[cfg(feature = "parquet")] -fn is_forbidden_ip(ip: std::net::IpAddr) -> bool { - use std::net::{IpAddr, Ipv4Addr}; - match ip { - IpAddr::V4(v4) => { - v4.is_loopback() - || v4.is_private() - || v4.is_link_local() // 169.254.0.0/16, incl. the cloud metadata endpoint - || v4.is_unspecified() - || v4.is_broadcast() - || v4.is_documentation() - || v4.is_multicast() - || v4.octets()[0] == 0 // 0.0.0.0/8 - || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xc0) == 64) // 100.64.0.0/10 CGNAT - } - IpAddr::V6(v6) => { - // Any IPv4 embedded in an IPv6 address (IPv4-mapped ::ffff:0:0/96, IPv4-compatible - // ::/96, or NAT64 64:ff9b::/96) is re-checked against the IPv4 rules, so e.g. - // 64:ff9b::169.254.169.254 cannot route to the metadata endpoint in a NAT64 network. - let seg = v6.segments(); - let is_v4_compatible = seg[0..6] == [0, 0, 0, 0, 0, 0]; - let is_nat64 = seg[0] == 0x0064 && seg[1] == 0xff9b && seg[2..6] == [0, 0, 0, 0]; - if let Some(v4) = v6.to_ipv4_mapped() { - return is_forbidden_ip(IpAddr::V4(v4)); - } - if is_v4_compatible || is_nat64 { - let embedded = Ipv4Addr::new( - (seg[6] >> 8) as u8, - (seg[6] & 0xff) as u8, - (seg[7] >> 8) as u8, - (seg[7] & 0xff) as u8, - ); - if is_forbidden_ip(IpAddr::V4(embedded)) { - return true; - } - } - v6.is_loopback() - || v6.is_unspecified() - || v6.is_multicast() - || (seg[0] & 0xfe00) == 0xfc00 // fc00::/7 unique local - || (seg[0] & 0xffc0) == 0xfe80 // fe80::/10 link-local - } - } -} - #[cfg(feature = "parquet")] async fn get_object_storage_usage( Extension(db): Extension, @@ -2582,8 +2545,7 @@ mod tests { #[cfg(all(test, feature = "parquet"))] mod object_storage_test_hardening { - use super::{extract_host, is_forbidden_ip, validate_object_storage_test}; - use std::net::IpAddr; + use super::{extract_host, validate_object_storage_test}; use windmill_object_store::ObjectSettings; // IP literals (not hostnames) keep validate_public_endpoint deterministic — `lookup_host` @@ -2644,40 +2606,6 @@ mod object_storage_test_hardening { } } - fn ip(s: &str) -> IpAddr { - s.parse().unwrap() - } - - #[test] - fn forbids_internal_ips() { - for s in [ - "127.0.0.1", // loopback - "169.254.169.254", // cloud metadata (link-local) - "10.0.0.5", // private - "172.16.3.4", // private - "192.168.1.10", // private - "0.0.0.0", // unspecified - "100.64.0.1", // CGNAT - "::1", // IPv6 loopback - "fe80::1", // IPv6 link-local - "fc00::1", // IPv6 unique local - "::ffff:127.0.0.1", // IPv4-mapped loopback - "::ffff:169.254.169.254", // IPv4-mapped metadata - "::169.254.169.254", // IPv4-compatible metadata - "64:ff9b::169.254.169.254", // NAT64-embedded metadata - "64:ff9b::a9fe:a9fe", // NAT64-embedded metadata (hex form) - ] { - assert!(is_forbidden_ip(ip(s)), "{s} should be forbidden"); - } - } - - #[test] - fn allows_public_ips() { - for s in ["8.8.8.8", "1.1.1.1", "52.95.110.1", "2606:4700:4700::1111"] { - assert!(!is_forbidden_ip(ip(s)), "{s} should be allowed"); - } - } - #[test] fn extracts_host_from_endpoint() { let cases = [ diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs index 0cdf7ffa4f..583b1e81ab 100644 --- a/backend/windmill-common/src/ssrf.rs +++ b/backend/windmill-common/src/ssrf.rs @@ -396,7 +396,10 @@ pub fn webhook_ssrf_error_message(e: &SsrfValidationError) -> String { } } -fn is_private_ip(ip: &IpAddr) -> bool { +/// 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. +pub fn is_private_ip(ip: &IpAddr) -> bool { match ip { IpAddr::V4(ipv4) => is_private_ipv4(ipv4), IpAddr::V6(ipv6) => is_private_ipv6(ipv6), @@ -408,6 +411,7 @@ fn is_private_ipv4(ip: &Ipv4Addr) -> bool { || ip.is_private() // 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 || ip.is_link_local() // 169.254.0.0/16 (AWS IMDS lives here) || ip.is_broadcast() // 255.255.255.255 + || ip.is_multicast() // 224.0.0.0/4 // RFC 1122 "this network": the whole /8, not just the unspecified // address `is_unspecified()` matches — stacks that map 0.x.y.z onto the // local host make `0.0.0.1` a bypass. @@ -418,17 +422,60 @@ fn is_private_ipv4(ip: &Ipv4Addr) -> bool { } fn is_private_ipv6(ip: &Ipv6Addr) -> bool { + let seg = ip.segments(); ip.is_loopback() // ::1 || ip.is_unspecified() // :: + || ip.is_multicast() // ff00::/8 // Unique local addresses (fc00::/7) - || (ip.segments()[0] & 0xfe00) == 0xfc00 - // Link-local (fe80::/10) - || (ip.segments()[0] & 0xffc0) == 0xfe80 - // IPv4-mapped addresses — check the embedded IPv4 - || match ip.to_ipv4_mapped() { - Some(ipv4) => is_private_ipv4(&ipv4), - None => false, - } + || (seg[0] & 0xfe00) == 0xfc00 + // Link-local (fe80::/10) and deprecated site-local (fec0::/10) + || (seg[0] & 0xffc0) == 0xfe80 + || (seg[0] & 0xffc0) == 0xfec0 + // NAT64 local-use prefix (64:ff9b:1::/48, RFC 8215): a site's own + // translator, whose IPv4 placement depends on the prefix length it uses. + || (seg[0] == 0x0064 && seg[1] == 0xff9b && seg[2] == 0x0001) + || embedded_ipv4(ip).is_some_and(|v4| is_private_ipv4(&v4)) +} + +/// The IPv4 address an IPv6 address carries through a transition mechanism, +/// which a dual-stack host or a translator on the path will deliver to. Guards +/// must judge that address too, else e.g. `64:ff9b::a9fe:a9fe` reaches +/// 169.254.169.254 through a NAT64 gateway. +fn embedded_ipv4(ip: &Ipv6Addr) -> Option { + let seg = ip.segments(); + let low32 = || { + Ipv4Addr::new( + (seg[6] >> 8) as u8, + seg[6] as u8, + (seg[7] >> 8) as u8, + seg[7] as u8, + ) + }; + match seg { + // IPv4-mapped ::ffff:0:0/96 + [0, 0, 0, 0, 0, 0xffff, _, _] => Some(low32()), + // IPv4-translated ::ffff:0:0:0/96 (SIIT) + [0, 0, 0, 0, 0xffff, 0, _, _] => Some(low32()), + // IPv4-compatible ::/96 (deprecated, still routed by some stacks) + [0, 0, 0, 0, 0, 0, _, _] => Some(low32()), + // NAT64 well-known prefix 64:ff9b::/96 + [0x0064, 0xff9b, 0, 0, 0, 0, _, _] => Some(low32()), + // 6to4 2002::/16: the site router's IPv4 sits right after the prefix + [0x2002, hi, lo, ..] => Some(Ipv4Addr::new( + (hi >> 8) as u8, + hi as u8, + (lo >> 8) as u8, + lo as u8, + )), + // Teredo 2001::/32: the client's IPv4 is stored bit-inverted + [0x2001, 0, ..] => Some(Ipv4Addr::new( + !(seg[6] >> 8) as u8, + !seg[6] as u8, + !(seg[7] >> 8) as u8, + !seg[7] as u8, + )), + _ => None, + } } #[cfg(test)] @@ -486,31 +533,50 @@ mod tests { } #[test] - fn test_private_ipv4() { - assert!(is_private_ipv4(&"127.0.0.1".parse().unwrap())); - assert!(is_private_ipv4(&"10.0.0.1".parse().unwrap())); - assert!(is_private_ipv4(&"172.16.0.1".parse().unwrap())); - assert!(is_private_ipv4(&"192.168.1.1".parse().unwrap())); - assert!(is_private_ipv4(&"169.254.169.254".parse().unwrap())); - assert!(is_private_ipv4(&"0.0.0.0".parse().unwrap())); - // The whole 0.0.0.0/8 block, not just the unspecified address. - assert!(is_private_ipv4(&"0.0.0.1".parse().unwrap())); - assert!(is_private_ipv4(&"0.255.255.255".parse().unwrap())); - assert!(is_private_ipv4(&"100.64.0.1".parse().unwrap())); - assert!(!is_private_ipv4(&"8.8.8.8".parse().unwrap())); - assert!(!is_private_ipv4(&"1.1.1.1".parse().unwrap())); - } - - #[test] - fn test_private_ipv6() { - assert!(is_private_ipv6(&"::1".parse().unwrap())); - assert!(is_private_ipv6(&"::".parse().unwrap())); - assert!(is_private_ipv6(&"fc00::1".parse().unwrap())); - assert!(is_private_ipv6(&"fd00::1".parse().unwrap())); - assert!(is_private_ipv6(&"fe80::1".parse().unwrap())); - // IPv4-mapped loopback - assert!(is_private_ipv6(&"::ffff:127.0.0.1".parse().unwrap())); - assert!(!is_private_ipv6(&"2001:4860:4860::8888".parse().unwrap())); + fn test_private_ip() { + for s in [ + "127.0.0.1", + "10.0.0.1", + "172.16.0.1", + "192.168.1.1", + "169.254.169.254", + "100.64.0.1", + // The whole 0.0.0.0/8 block, not just the unspecified address. + "0.0.0.0", + "0.0.0.1", + "0.255.255.255", + "::1", + "::", + "fc00::1", + "fd00::1", + "fe80::1", + // IPv4 carried inside IPv6 by a transition mechanism. + "::ffff:127.0.0.1", // IPv4-mapped + "::169.254.169.254", // IPv4-compatible + "::ffff:0:a9fe:a9fe", // IPv4-translated (SIIT) + "64:ff9b::a9fe:a9fe", // NAT64 well-known prefix + "64:ff9b:1::a9fe:a9fe", // NAT64 local-use prefix + "64:ff9b:1:ffff::1", // any address in the local-use prefix + "2002:a9fe:a9fe::1", // 6to4 + "2002:7f00:1::", // 6to4 + "2001:0:1:2:0:0:80ff:fffe", // Teredo, client 127.0.0.1 bit-inverted + "2001:0:1:2::5601:5601", // Teredo, client 169.254.169.254 bit-inverted + ] { + let ip: IpAddr = s.parse().unwrap(); + assert!(is_private_ip(&ip), "{s} should be private"); + } + for s in [ + "8.8.8.8", + "1.1.1.1", + "2001:4860:4860::8888", + "2606:4700:4700::1111", + "64:ff9b::808:808", // NAT64 of 8.8.8.8 + "2002:808:808::1", // 6to4 of 8.8.8.8 + "2001:0:1:2::f7f7:f7f7", // Teredo, client 8.8.8.8 + ] { + let ip: IpAddr = s.parse().unwrap(); + assert!(!is_private_ip(&ip), "{s} should be public"); + } } #[tokio::test] diff --git a/backend/windmill-object-store/Cargo.toml b/backend/windmill-object-store/Cargo.toml index 90af4f7e7c..f80eb88881 100644 --- a/backend/windmill-object-store/Cargo.toml +++ b/backend/windmill-object-store/Cargo.toml @@ -11,6 +11,7 @@ enterprise = [] parquet = [ "windmill-common/parquet", "dep:object_store", + "dep:reqwest_object_store", "dep:aws-sdk-sts", "dep:aws-smithy-types-convert", "dep:datafusion", @@ -48,6 +49,7 @@ globset.workspace = true axum.workspace = true object_store = { workspace = true, optional = true } +reqwest_object_store = { workspace = true, optional = true } aws-sdk-sts = { workspace = true, optional = true } aws-smithy-types-convert = { workspace = true, optional = true } datafusion = { workspace = true, optional = true } diff --git a/backend/windmill-object-store/src/lib.rs b/backend/windmill-object-store/src/lib.rs index edc3ea9149..a9551ad835 100644 --- a/backend/windmill-object-store/src/lib.rs +++ b/backend/windmill-object-store/src/lib.rs @@ -583,8 +583,175 @@ pub async fn ambient_aws_credentials( ambient_aws_credentials_provider(region).await.get().await } +/// An object-store HTTP connector whose client cannot reach a non-public address. +/// +/// Validating a caller-supplied endpoint up front does not bind the connection: object_store +/// resolves the host again when it connects, so a name can answer a public address to the check +/// and an internal one to the connect. This client resolves every host through +/// [`PublicOnlyResolver`], which judges the very addresses it hands to the connect, refuses IP-literal +/// hosts that are not public (they never reach a resolver), and follows no redirect. +/// +/// The environment's egress proxy (`HTTP(S)_PROXY`/`ALL_PROXY`) is still used, and its own host is +/// exempt from the resolver check: it is operator configuration and typically internal. A request +/// whose target is the proxy host itself is refused, so the exemption only ever serves the proxy +/// hop. A proxied request's target is resolved by the proxy, out of this client's reach (see +/// `ssrf::ValidatedTarget`). +#[cfg(feature = "parquet")] +#[derive(Debug, Clone, Copy)] +pub struct PublicOnlyConnector; + +#[cfg(feature = "parquet")] +impl object_store::client::HttpConnector for PublicOnlyConnector { + fn connect( + &self, + options: &ClientOptions, + ) -> object_store::Result { + let allow_http = options + .get_config_value(&object_store::ClientConfigKey::AllowHttp) + .is_some_and(|v| v == "true"); + let proxy_hosts: Arc<[String]> = system_proxy_hosts().into(); + let client = reqwest_object_store::Client::builder() + .dns_resolver(Arc::new(PublicOnlyResolver { + proxy_hosts: proxy_hosts.clone(), + })) + .redirect(reqwest_object_store::redirect::Policy::none()) + .connect_timeout(std::time::Duration::from_secs(5)) + .default_headers(HeaderMap::from_iter([( + reqwest::header::ACCEPT_ENCODING, + reqwest::header::HeaderValue::from_static(""), + )])) + .no_gzip() + .no_brotli() + .no_deflate() + .no_zstd() + .https_only(!allow_http) + .build() + .map_err(|e| object_store::Error::Generic { + store: "HTTP client", + source: Box::new(e), + })?; + Ok(object_store::client::HttpClient::new(PublicOnlyClient { + client, + proxy_hosts, + })) + } +} + +#[cfg(feature = "parquet")] +#[derive(Debug)] +struct PublicOnlyClient { + client: reqwest_object_store::Client, + proxy_hosts: Arc<[String]>, +} + +#[cfg(feature = "parquet")] +#[async_trait] +impl object_store::client::HttpService for PublicOnlyClient { + async fn call( + &self, + req: object_store::client::HttpRequest, + ) -> Result { + let host = req.uri().host().unwrap_or_default(); + let literal = host + .trim_start_matches('[') + .trim_end_matches(']') + .parse::() + .ok(); + let refused = if literal.is_some_and(|ip| windmill_common::ssrf::is_private_ip(&ip)) { + Some("a private, loopback, or link-local address") + } else if self + .proxy_hosts + .iter() + .any(|p| p.eq_ignore_ascii_case(host)) + { + Some("the egress proxy") + } else { + None + }; + if let Some(what) = refused { + return Err(object_store::client::HttpError::new( + object_store::client::HttpErrorKind::Unknown, + std::io::Error::other(format!("'{host}' is {what}")), + )); + } + object_store::client::HttpService::call(&self.client, req).await + } +} + +#[cfg(feature = "parquet")] +struct PublicOnlyResolver { + proxy_hosts: Arc<[String]>, +} + +/// The hosts of the proxies reqwest picks up from the environment. +#[cfg(feature = "parquet")] +fn system_proxy_hosts() -> Vec { + [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "ALL_PROXY", + "all_proxy", + ] + .into_iter() + .filter_map(|k| std::env::var(k).ok()) + .filter_map(|v| { + let v = v.trim(); + let url = if v.contains("://") { + v.to_string() + } else { + format!("http://{v}") + }; + Some( + reqwest::Url::parse(&url) + .ok()? + .host_str()? + .to_ascii_lowercase(), + ) + }) + .collect() +} + +#[cfg(feature = "parquet")] +impl reqwest_object_store::dns::Resolve for PublicOnlyResolver { + fn resolve( + &self, + name: reqwest_object_store::dns::Name, + ) -> reqwest_object_store::dns::Resolving { + let is_proxy = self + .proxy_hosts + .iter() + .any(|p| p.eq_ignore_ascii_case(name.as_str())); + Box::pin(async move { + let host = name.as_str(); + let addrs: Vec = + tokio::net::lookup_host((host, 0)).await?.collect(); + if !is_proxy + && addrs + .iter() + .any(|a| windmill_common::ssrf::is_private_ip(&a.ip())) + { + return Err(format!( + "'{host}' resolves to a private, loopback, or link-local address" + ) + .into()); + } + Ok(Box::new(addrs.into_iter()) as reqwest_object_store::dns::Addrs) + }) + } +} + #[cfg(feature = "parquet")] pub async fn build_s3_client(s3_resource_ref: &S3Resource) -> error::Result> { + build_s3_client_with(s3_resource_ref, None).await +} + +#[cfg(feature = "parquet")] +async fn build_s3_client_with( + s3_resource_ref: &S3Resource, + connector: Option, +) -> error::Result> { let static_creds = s3_resource_has_static_credentials(s3_resource_ref); let credentials_provider = if !static_creds { @@ -623,6 +790,9 @@ pub async fn build_s3_client(s3_resource_ref: &S3Resource) -> error::Result error::Result error::Result> { + build_azure_blob_client_with(azure_blob_resource_ref, None) +} + +#[cfg(feature = "parquet")] +fn build_azure_blob_client_with( + azure_blob_resource_ref: &AzureBlobResource, + connector: Option, ) -> error::Result> { let blob_resource = azure_blob_resource_ref.clone(); @@ -701,6 +879,9 @@ fn build_azure_blob_client( if !blob_resource.use_ssl.unwrap_or(false) { store_builder = store_builder.with_allow_http(true) } + if let Some(connector) = connector { + store_builder = store_builder.with_http_connector(connector); + } if let Some(key) = blob_resource.access_key { if key != "" { @@ -735,6 +916,14 @@ pub fn gcs_service_account_key_is_blank(service_account_key: &str) -> bool { #[cfg(feature = "parquet")] async fn build_gcs_client(gcs_resource_ref: &GcsResource) -> error::Result> { + build_gcs_client_with(gcs_resource_ref, None).await +} + +#[cfg(feature = "parquet")] +async fn build_gcs_client_with( + gcs_resource_ref: &GcsResource, + connector: Option, +) -> error::Result> { let gcs_resource = gcs_resource_ref.clone(); let mut store_builder = GoogleCloudStorageBuilder::new() @@ -754,6 +943,9 @@ async fn build_gcs_client(gcs_resource_ref: &GcsResource) -> error::Result String { pub async fn build_object_store_from_settings( settings: ObjectSettings, init_private_key: Option<&windmill_common::DB>, +) -> error::Result { + build_object_store_from_settings_with(settings, init_private_key, None).await +} + +/// [`build_object_store_from_settings`] for a caller-supplied endpoint: the client refuses to +/// connect to any non-public address (see [`PublicOnlyConnector`]). Filesystem and OIDC settings +/// are refused; whether S3, Azure or GCS settings carry explicit credentials (rather than falling +/// back to the server's ambient ones) is still the caller's to check. +#[cfg(feature = "parquet")] +pub async fn build_public_object_store_from_settings( + settings: ObjectSettings, +) -> error::Result> { + match settings { + ObjectSettings::S3(_) | ObjectSettings::Azure(_) | ObjectSettings::Gcs(_) => { + build_object_store_from_settings_with(settings, None, Some(PublicOnlyConnector)) + .await + .map(|x| x.store) + } + ObjectSettings::AwsOidc(_) | ObjectSettings::Filesystem(_) => { + Err(error::Error::BadRequest( + "This object storage backend cannot be restricted to public endpoints".to_string(), + )) + } + } +} + +#[cfg(feature = "parquet")] +async fn build_object_store_from_settings_with( + settings: ObjectSettings, + init_private_key: Option<&windmill_common::DB>, + connector: Option, ) -> error::Result { let located = |store: Arc, resource: ObjectStoreResource| ExpirableObjectStore { @@ -946,12 +1169,14 @@ pub async fn build_object_store_from_settings( match settings { ObjectSettings::S3(s3_settings) => { let s3_resource = s3_resource_from_settings(s3_settings); - build_s3_client(&s3_resource) + build_s3_client_with(&s3_resource, connector) .await .map(|x| located(x, ObjectStoreResource::S3(s3_resource))) } - ObjectSettings::Azure(azure_settings) => build_azure_blob_client(&azure_settings) - .map(|x| located(x, ObjectStoreResource::Azure(azure_settings))), + ObjectSettings::Azure(azure_settings) => { + build_azure_blob_client_with(&azure_settings, connector) + .map(|x| located(x, ObjectStoreResource::Azure(azure_settings))) + } ObjectSettings::AwsOidc(ref s3_aws_oidc_settings) => { let token_generator = crate::job_s3_helpers_oss::TokenGenerator::AsServerInstance(); let res = crate::job_s3_helpers_oss::generate_s3_aws_oidc_resource( @@ -969,7 +1194,7 @@ pub async fn build_object_store_from_settings( location: Some(object_store_location(&res)), }) } - ObjectSettings::Gcs(gcs_settings) => build_gcs_client(&gcs_settings) + ObjectSettings::Gcs(gcs_settings) => build_gcs_client_with(&gcs_settings, connector) .await .map(|x| located(x, ObjectStoreResource::Gcs(gcs_settings))), ObjectSettings::Filesystem(fs) => build_filesystem_client(&fs.root_path) @@ -2701,6 +2926,44 @@ mod tests { assert_ne!(a.location, b.location); } + /// The restricted store judges the addresses it connects to, not only what an earlier check + /// resolved: neither a hostname that resolves to loopback nor a loopback literal gets a + /// connection. + #[cfg(feature = "parquet")] + #[tokio::test] + async fn test_public_store_refuses_to_connect_to_private_address() { + use futures::StreamExt; + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + for endpoint in ["localhost", "127.0.0.1"] { + let settings = ObjectSettings::S3(S3Settings { + bucket: Some("windmill".to_string()), + region: Some("us-east-1".to_string()), + access_key: Some("key".to_string()), + secret_key: Some("secret".to_string()), + endpoint: Some(endpoint.to_string()), + allow_http: Some(true), + path_style: Some(true), + store_logs: None, + port: Some(listener.local_addr().unwrap().port()), + }); + let store = build_public_object_store_from_settings(settings) + .await + .unwrap(); + // The listener never answers, so a store that connects waits forever: bound it. + let err = + tokio::time::timeout(std::time::Duration::from_secs(30), store.list(None).next()) + .await + .unwrap_or_else(|_| panic!("the store connected to {endpoint}")) + .unwrap() + .unwrap_err(); + assert!( + format!("{err:?}").contains("private, loopback, or link-local"), + "{endpoint}: {err:?}" + ); + } + } + // --- get_logs_from_store test --- #[cfg(feature = "parquet")] diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 5bb15b456e..71e94b57f3 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -17,7 +17,9 @@ use windmill_api_auth::{ }; use windmill_common::db::DB; use windmill_common::per_minute_counter::PerMinuteCounter; -use windmill_common::ssrf::{private_git_host_allowed, private_git_host_hint, GitRemoteCaller}; +use windmill_common::ssrf::{ + is_private_ip, private_git_host_allowed, private_git_host_hint, GitRemoteCaller, +}; use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; use crate::secret_backend_ext::rename_vault_secret; @@ -3505,38 +3507,6 @@ struct GitRepositoryResource { branch: Option, } -/// Checks whether an IP address belongs to a private, loopback, link-local, or -/// otherwise reserved range that should not be reachable from git operations. -fn is_private_or_reserved_ip(ip: &IpAddr) -> bool { - match ip { - IpAddr::V4(v4) => { - v4.is_loopback() - || v4.is_private() - || v4.is_link_local() - // RFC 1122 "this network": the whole /8, not just the - // unspecified address `is_unspecified()` matches — stacks that - // map 0.x.y.z onto the local host make `0.0.0.1` a bypass. - || v4.octets()[0] == 0 // 0.0.0.0/8 - || v4.is_broadcast() - // 100.64.0.0/10 (Carrier-grade NAT / CGNAT) - || (v4.octets()[0] == 100 && (v4.octets()[1] & 0xC0) == 64) - } - IpAddr::V6(v6) => { - let seg = v6.segments(); - v6.is_loopback() - || v6.is_unspecified() - // fc00::/7 (unique local address) — std has no stable is_unique_local() - || (seg[0] & 0xfe00) == 0xfc00 - // fe80::/10 (link-local) — std has no stable is_unicast_link_local() - || (seg[0] & 0xffc0) == 0xfe80 - // IPv4-mapped IPv6 (::ffff:x.x.x.x) — check the inner v4 - || v6.to_ipv4_mapped().map_or(false, |v4| { - is_private_or_reserved_ip(&IpAddr::V4(v4)) - }) - } - } -} - /// Extracts the hostname from a git URL. /// /// Handles standard URLs (`https://host/path`, `ssh://user@host/path`) and @@ -3706,7 +3676,7 @@ async fn validate_git_url(url: &str, caller: GitRemoteCaller) -> Result<()> { // Check literal IP addresses if let Ok(ip) = host.parse::() { - if is_private_or_reserved_ip(&ip) { + if is_private_ip(&ip) { return Err(Error::BadRequest(format!( "Git URLs targeting private or reserved IP addresses are not allowed.{hint}" ))); @@ -3729,7 +3699,7 @@ async fn validate_git_url(url: &str, caller: GitRemoteCaller) -> Result<()> { ))); } for addr in addrs { - if is_private_or_reserved_ip(&addr.ip()) { + if is_private_ip(&addr.ip()) { return Err(Error::BadRequest(format!( "Git URL hostname resolves to a private or reserved IP address.{hint}" ))); @@ -4985,73 +4955,6 @@ mod tests { ); } - #[test] - fn test_is_private_or_reserved_ip() { - use std::net::IpAddr; - // Loopback - assert!(is_private_or_reserved_ip( - &"127.0.0.1".parse::().unwrap() - )); - assert!(is_private_or_reserved_ip( - &"127.0.0.2".parse::().unwrap() - )); - // Private ranges - assert!(is_private_or_reserved_ip( - &"10.0.0.1".parse::().unwrap() - )); - assert!(is_private_or_reserved_ip( - &"172.16.0.1".parse::().unwrap() - )); - assert!(is_private_or_reserved_ip( - &"192.168.1.1".parse::().unwrap() - )); - // Link-local / cloud metadata - assert!(is_private_or_reserved_ip( - &"169.254.169.254".parse::().unwrap() - )); - // CGNAT - assert!(is_private_or_reserved_ip( - &"100.64.0.1".parse::().unwrap() - )); - // "This network" 0.0.0.0/8, not just the unspecified address - assert!(is_private_or_reserved_ip( - &"0.0.0.0".parse::().unwrap() - )); - assert!(is_private_or_reserved_ip( - &"0.0.0.1".parse::().unwrap() - )); - // IPv6 loopback - assert!(is_private_or_reserved_ip(&"::1".parse::().unwrap())); - // IPv6 unique local address (fc00::/7) - assert!(is_private_or_reserved_ip( - &"fd00::1".parse::().unwrap() - )); - assert!(is_private_or_reserved_ip( - &"fc00::1".parse::().unwrap() - )); - // IPv6 link-local (fe80::/10) - assert!(is_private_or_reserved_ip( - &"fe80::1".parse::().unwrap() - )); - // IPv4-mapped IPv6 - assert!(is_private_or_reserved_ip( - &"::ffff:127.0.0.1".parse::().unwrap() - )); - // Public IPs should pass - assert!(!is_private_or_reserved_ip( - &"8.8.8.8".parse::().unwrap() - )); - assert!(!is_private_or_reserved_ip( - &"140.82.121.4".parse::().unwrap() - )); - // Public IPv6 should pass - assert!(!is_private_or_reserved_ip( - &"2606:2800:220:1:248:1893:25c8:1946" - .parse::() - .unwrap() - )); - } - // A caller let through to private hosts must still hit the scheme check. #[tokio::test] async fn test_validate_git_url_blocks_file_scheme() {