diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 360c3dc804..173a3ac249 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -280,6 +280,10 @@ pub async fn fetch_jwks(url: &str) -> Result> { /// one that does not model as a JWT key is skipped, not fatal: a set may legitimately /// carry an encryption key (say `alg: "RSA-OAEP"`, which is not in jsonwebtoken's signing /// `Algorithm` enum and would fail whole-set deserialization) beside its signing keys. +/// +/// A key is kept only if its material actually decodes (`DecodingKey::from_jwk`): jsonwebtoken +/// carries `n`/`e`/`x`/`y` as strings and defers decoding to auth time, so without this a JWKS +/// whose only key is malformed would be accepted at save time and fail every token later. fn parse_jwks_keys(body: &[u8]) -> Result> { let set: serde_json::Value = serde_json::from_slice(body) .map_err(|e| Error::BadRequest(format!("JWKS is not JSON: {e}")))?; @@ -291,6 +295,7 @@ fn parse_jwks_keys(body: &[u8]) -> Result> { .iter() .filter_map(|entry| serde_json::from_value::(entry.clone()).ok()) .filter(|jwk| jwk_algorithms(jwk).is_some()) + .filter(|jwk| DecodingKey::from_jwk(jwk).is_ok()) .filter_map(|jwk| jwk.common.key_id.clone().map(|kid| (kid, jwk))) .collect(); if keys.is_empty() { @@ -485,11 +490,12 @@ mod tests { #[test] fn a_mixed_use_jwks_keeps_only_the_signing_keys() { // An encryption key (RSA-OAEP is not in jsonwebtoken's signing Algorithm enum) - // beside a signing key must not fail the whole set. + // beside a signing key must not fail the whole set. The signing key carries real + // coordinates so it survives the material check parse_jwks_keys now applies. let body = serde_json::json!({ "keys": [ {"kty":"RSA","alg":"RSA-OAEP","kid":"enc","use":"enc","n":"aa","e":"AQAB"}, - {"kty":"EC","crv":"P-256","kid":"sig","x":"aa","y":"bb"} + {"kty":"EC","crv":"P-256","kid":"sig","x":PUB1_X,"y":PUB1_Y} ] }) .to_string(); @@ -498,6 +504,18 @@ mod tests { assert!(!keys.contains_key("enc")); } + #[test] + fn a_jwks_with_only_malformed_key_material_is_refused() { + // Metadata (kty/alg/use) is fine but `n` is not valid base64url, so the key is + // unusable. Since it is the only key, configuring this JWKS must fail at save time + // rather than persist a URL whose tokens all fail later. + let body = serde_json::json!({ + "keys": [{"kty":"RSA","kid":"k1","n":"not base64url!!","e":"AQAB"}] + }) + .to_string(); + assert!(parse_jwks_keys(body.as_bytes()).is_err()); + } + #[test] fn key_ops_without_verify_is_refused() { let enc = jwk(serde_json::json!({"kty":"RSA","key_ops":["encrypt"],"n":"aa","e":"AQAB"})); @@ -561,8 +579,9 @@ ZQIDAQAB\n\ // A complete, valid PKCS#1 RSA *private* key (the counterpart of RSA_PUBLIC) with its // armor relabelled `RSA PUBLIC KEY`. jsonwebtoken's from_rsa_pem accepts it under that - // label; the SPKI parse is what refuses it. Complete on purpose: malformed DER would fail - // for the wrong reason and let a real bypass through unnoticed. + // label; the structural check refuses it (a 9-field RSAPrivateKey is neither an SPKI nor + // a 2-field RsaPublicKey). Complete on purpose: malformed DER would fail for the wrong + // reason and let a real bypass through unnoticed. const RSA_PKCS1_PRIVATE_AS_PUBLIC: &str = "-----BEGIN RSA PUBLIC KEY-----\n\ MIIEogIBAAKCAQEAx3J0fQcHp2ZlMI4rCVsYtirATZPWyPD7exoYWPInhV5xjbY2\n\ Fe8IVFaZszQcQbCXZjBtFp2fj0tBTow8BeOyX9LJPyKeho/j68FycuDVg7JCzG0T\n\ @@ -596,8 +615,8 @@ y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\ // A verification key must be public and must never be stored otherwise: it is served // back through the settings response. jsonwebtoken keys the public/private split off // the PEM label, so private DER relabelled with a public armor slips a label check; - // only parsing the DER as a SubjectPublicKeyInfo refuses it. A real public key still - // parses, so the guard is not vacuous. + // only parsing the DER as a public-key structure (SPKI or PKCS#1 RSA public) refuses + // it. A real public key still parses, so the guard is not vacuous. assert!(decoding_key_from_pem(RSA_PUBLIC).is_ok()); // The same key with its body on one line (not 64-column wrapped): jsonwebtoken accepts // any wrapping, so the guard must too rather than lean on the strict RFC 7468 decoder.