starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-07 08:02:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: validate JWKS key material at config time; refresh stale comments

Browse Source 
parse_jwks_keys filtered on metadata only (kty/alg/use/key_ops), but jsonwebtoken
carries n/e/x/y as strings and defers decoding to auth time, so a JWKS whose only
key had malformed material passed save-time validation and every token failed
later. Keep a key only if DecodingKey::from_jwk decodes it. This is the single
source for both edit_guest_jwt_key and per-request verify.

Update two test comments that credited the SPKI parse alone now that the guard
also accepts a PKCS#1 RSA public key.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3
This commit is contained in:
Ruben Fiszel
2026-09-04 10:43:14 +02:00
co-authored by Claude Opus 4.8
parent f5713621c8
commit e1b3a2e19f
1 changed files with 25 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/windmill-common/src/guest_jwt.rs
+25 -6
View File
@@ -280,6 +280,10 @@ pub async fn fetch_jwks(url: &str) -> Result<HashMap<String, Jwk>> {
/// 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<HashMap<String, Jwk>> {
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<HashMap<String, Jwk>> {
.iter()
.filter_map(|entry| serde_json::from_value::<Jwk>(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.