diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 1a56f22f5d..b8099f913f 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -247,12 +247,26 @@ const JWKS_MAX_BYTES: usize = 1 << 20; /// retained material instead; a real set is a few KB, so this is invisible to a legitimate one. const JWKS_MAX_RETAINED_BYTES: usize = 64 * 1024; +/// The retained-bytes cap counts key material; this caps the number of keys so the per-key +/// fixed cost (each `Jwk` and its map slot) is bounded too, not just their string content. +const JWKS_MAX_KEYS: usize = 50; + +/// Both JWKS caches key on the admin-supplied URL string. The column is unbounded `TEXT`, so +/// without this a workspace admin rotating long URLs could grow the caches by the URL bytes +/// alone. A real JWKS URL is well under this; the check runs before the URL is ever cached. +const MAX_JWKS_URL_LEN: usize = 2048; + /// Fetch a JWKS, keeping only the keys usable here. The URL was set by a workspace /// admin, so it is validated against private ranges and the connect is pinned to the /// validated addresses; redirects are not followed for the same reason. The body is /// read with a cap so a hostile endpoint cannot exhaust memory. pub async fn fetch_jwks(url: &str) -> Result> { use futures::StreamExt; + if url.len() > MAX_JWKS_URL_LEN { + return Err(Error::BadRequest(format!( + "JWKS URL is longer than {MAX_JWKS_URL_LEN} bytes" + ))); + } let target = crate::ssrf::validate_guest_jwks_url(url) .await .map_err(|e| Error::BadRequest(format!("JWKS URL is not allowed: {e}")))?; @@ -311,8 +325,14 @@ fn parse_jwks_keys(body: &[u8]) -> Result> { "JWKS holds no RSA or EC signing key with a kid".to_string(), )); } - // Bound the retained material by bytes, measured after filtering so a large mixed-use set - // (many encryption keys, few signing) is not refused for its size. + // Bound the usable keys two ways, both measured after filtering so a large mixed-use set + // (many encryption keys, few signing) is not refused for its size: their count (the per-key + // fixed cost) and their combined material bytes. + if keys.len() > JWKS_MAX_KEYS { + return Err(Error::BadRequest(format!( + "JWKS holds more than {JWKS_MAX_KEYS} usable signing keys" + ))); + } let retained: usize = keys .values() .filter_map(|jwk| serde_json::to_vec(jwk).ok().map(|v| v.len())) @@ -536,17 +556,40 @@ mod tests { } #[test] - fn a_jwks_retaining_too_many_bytes_is_refused() { - // Every key has real coordinates, so all are usable and this exercises the - // retained-bytes cap rather than the empty-set path: their combined material exceeds - // JWKS_MAX_RETAINED_BYTES, which the byte-per-response and entry-count caps do not bound. - let keys: Vec<_> = (0..600) + fn too_many_usable_keys_is_refused() { + // All keys are usable (real coordinates), so this trips the count cap, not the + // empty-set path. Small material, so it is the count that refuses them, not the bytes. + let keys: Vec<_> = (0..=JWKS_MAX_KEYS) .map(|i| { serde_json::json!({"kty":"EC","crv":"P-256","kid":format!("k{i}"),"x":PUB1_X,"y":PUB1_Y}) }) .collect(); let body = serde_json::json!({ "keys": keys }).to_string(); - assert!(parse_jwks_keys(body.as_bytes()).is_err()); + let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string(); + assert!(err.contains("more than"), "{err}"); + } + + #[test] + fn keys_retaining_too_many_bytes_is_refused() { + // Under the key count cap, but the material of these usable RSA keys exceeds the byte + // cap. `from_jwk` decodes any-length `n`, so this is reachable without the count cap. + let big_n = "A".repeat(2000); + let keys: Vec<_> = (0..40) + .map(|i| serde_json::json!({"kty":"RSA","kid":format!("k{i}"),"n":big_n,"e":"AQAB"})) + .collect(); + let body = serde_json::json!({ "keys": keys }).to_string(); + let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string(); + assert!(err.contains("retain more than"), "{err}"); + } + + #[tokio::test] + async fn an_overlong_jwks_url_is_refused() { + // The cache keys on the URL string, so an unbounded URL is refused before it is cached. + let url = format!( + "https://issuer.example.com/{}", + "a".repeat(MAX_JWKS_URL_LEN) + ); + assert!(fetch_jwks(&url).await.is_err()); } #[test]