From 77b24b20cddc70f7c3b28b955129ebf202cb002b Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 3 Sep 2026 08:49:18 +0000 Subject: [PATCH] fix: address review findings on the guest JWT entry - Resolve a guest JWT on the lowercased email. Accounts are stored lowercased, so a mixed-case `email` claim slipped past the no-account gate and resolved an account holder to a guest, and split the activity rows the seat count reads. `has_any_account` now normalises its input too (index-friendly, not `lower(email)`). - Cap the auth-cache entry for a guest JWT at 5 minutes rather than the token's `exp` (up to 24h). A guest JWT is revocable only by the workspace switch or by rotating the key; the short entry makes a rotated or cleared key bite on re-verification, and makes the day-keyed activity dedupe reachable across a midnight (the second-day row was never written). - Audit `users.login_guest` only when the upsert freshly inserts the row (`xmax = 0`), decided atomically by the DB, so concurrent first requests and separate API nodes emit it at most once a day. - JWKS hardening: read the body with a 1MB cap instead of buffering any size; an alg-less RSA key accepts the whole RSA family instead of being forced to RS256; a failed fetch serves the last good keys (or a short negative entry) so an unreachable issuer is hit at most once per 30s however much unauthenticated `jwt_guest_` traffic arrives, and an unknown `kid` never triggers a fetch; lower the fetch timeouts to 5s/10s. - Settings copy: note that the JWKS should point at an issuer you control, since neither `iss` nor `aud` is bound. - Tests: a mixed-case account and an over-24h lifetime are refused; unit tests pin `jwk_algorithms` (including the alg-less RSA family) and a JWK-derived key verifying a real token. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- ...5983be237f8a3013c680d2b1967e703384e6.json} | 16 +- backend/tests/app_guest_jwt_entry.rs | 13 ++ backend/windmill-api-auth/src/auth.rs | 123 ++++++----- backend/windmill-api-users/src/users.rs | 1 - backend/windmill-common/src/guest_jwt.rs | 205 ++++++++++++++---- backend/windmill-common/src/users.rs | 6 +- .../(logged)/workspace_settings/+page.svelte | 4 +- 7 files changed, 267 insertions(+), 101 deletions(-) rename backend/.sqlx/{query-f893c06e7eb4c1c66e55f8fc477c7d4e5e94889ef5e72b4961ba4838e79bfffa.json => query-5a52b42ddae68a7d280cfe1c9e5e5983be237f8a3013c680d2b1967e703384e6.json} (50%) diff --git a/backend/.sqlx/query-f893c06e7eb4c1c66e55f8fc477c7d4e5e94889ef5e72b4961ba4838e79bfffa.json b/backend/.sqlx/query-5a52b42ddae68a7d280cfe1c9e5e5983be237f8a3013c680d2b1967e703384e6.json similarity index 50% rename from backend/.sqlx/query-f893c06e7eb4c1c66e55f8fc477c7d4e5e94889ef5e72b4961ba4838e79bfffa.json rename to backend/.sqlx/query-5a52b42ddae68a7d280cfe1c9e5e5983be237f8a3013c680d2b1967e703384e6.json index 48500fc05a..d7418e3ed1 100644 --- a/backend/.sqlx/query-f893c06e7eb4c1c66e55f8fc477c7d4e5e94889ef5e72b4961ba4838e79bfffa.json +++ b/backend/.sqlx/query-5a52b42ddae68a7d280cfe1c9e5e5983be237f8a3013c680d2b1967e703384e6.json @@ -1,15 +1,23 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO guest_activity (email, workspace_id, day, jwt_entry)\n VALUES ($1, $2, CURRENT_DATE, true)\n ON CONFLICT (email, workspace_id, day)\n DO UPDATE SET jwt_entry = true, last_seen_at = now()", + "query": "INSERT INTO guest_activity (email, workspace_id, day, jwt_entry)\n VALUES ($1, $2, CURRENT_DATE, true)\n ON CONFLICT (email, workspace_id, day)\n DO UPDATE SET jwt_entry = true, last_seen_at = now()\n RETURNING (xmax = 0) AS \"inserted!\"", "describe": { - "columns": [], + "columns": [ + { + "ordinal": 0, + "name": "inserted!", + "type_info": "Bool" + } + ], "parameters": { "Left": [ "Varchar", "Varchar" ] }, - "nullable": [] + "nullable": [ + null + ] }, - "hash": "f893c06e7eb4c1c66e55f8fc477c7d4e5e94889ef5e72b4961ba4838e79bfffa" + "hash": "5a52b42ddae68a7d280cfe1c9e5e5983be237f8a3013c680d2b1967e703384e6" } diff --git a/backend/tests/app_guest_jwt_entry.rs b/backend/tests/app_guest_jwt_entry.rs index 62348fa9cd..aacf19dabc 100644 --- a/backend/tests/app_guest_jwt_entry.rs +++ b/backend/tests/app_guest_jwt_entry.rs @@ -268,6 +268,17 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { c.app_path = "u/test-user/members_app".to_string(); let not_guest_app = bearer(&c, PRIV1, Algorithm::ES256); + // an existing account addressed in a different case still counts as an account: + // the base fixture holds `test@windmill.dev`. + let mut c = Claims::valid(); + c.email = "Test@Windmill.Dev".to_string(); + let mixed_case_account = bearer(&c, PRIV1, Algorithm::ES256); + + // a lifetime past the 24h cap, even with a valid signature. + let mut c = Claims::valid(); + c.exp = now() + 25 * 3600; + let over_lifetime_cap = bearer(&c, PRIV1, Algorithm::ES256); + for (label, token) in [ ("wrong workspace", wrong_ws), ("wrong key", wrong_key), @@ -275,6 +286,8 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { ("HS256", hs256), ("email with an account", has_account), ("app not in guest mode", not_guest_app), + ("mixed-case account", mixed_case_account), + ("over the 24h lifetime cap", over_lifetime_cap), ] { let resp = whoami(port, ws, &token).send().await?; assert_eq!(resp.status(), 401, "{label} must be refused"); diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index f6686f5b79..f7a58ea2db 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -243,9 +243,14 @@ impl AuthCache { return None; } } + // Resolve on the lowercased email: accounts are stored lowercased, so a + // mixed-case claim would otherwise slip past the no-account gate and + // resolve an account holder to a guest, and split the activity rows the + // seat count reads. + let email = claims.email.to_lowercase(); // A guest is someone with no account at all; an account holder is refused, // never downgraded (the same rule as the signed-in guest mint). - match windmill_common::users::has_any_account(&self.db, &claims.email).await { + match windmill_common::users::has_any_account(&self.db, &email).await { Ok(false) => {} Ok(true) => return None, Err(e) => { @@ -253,16 +258,20 @@ impl AuthCache { return None; } } - record_guest_jwt_activity(&self.db, w_id, &claims).await; - let expiry = chrono::Utc.timestamp_nanos(claims.exp as i64 * 1_000_000_000); - // The label alone makes a DB token a guest; a JWT has none, so the sentinel - // is what governs it, exactly as it governs a guest-derived token. - let scopes = Some(crate::scopes::with_guest_sentinel( - crate::scopes::guest_session_scopes(&claims.app_path), - )); + record_guest_jwt_activity(&self.db, w_id, &email, &claims.app_path).await; + // guest_session_scopes already carries the sentinel, and it is the whole + // grant; a JWT has no label, so the sentinel is what governs it. + let scopes = Some(crate::scopes::guest_session_scopes(&claims.app_path)); + // The JWT's own expiry caps a token minted from this session. The auth + // cache entry itself is capped far shorter (GUEST_JWT_CACHE_TTL) so a + // rotated or cleared key stops the session on re-verification, within + // minutes, rather than only at exp (up to 24h away). + let credential_expiry = + chrono::Utc.timestamp_nanos(claims.exp as i64 * 1_000_000_000); + let cache_expiry = credential_expiry.min(chrono::Utc::now() + GUEST_JWT_CACHE_TTL); let authed = ApiAuthed { - username: claims.email.clone(), - email: claims.email, + username: email.clone(), + email, is_admin: false, is_operator: true, groups: vec![], @@ -274,12 +283,11 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only: false, job_id: None, - // Carried to the derived-token mint: its expiry caps on this. - credential_expiry: Some(expiry), + credential_expiry: Some(credential_expiry), }; AUTH_CACHE.insert( key, - ExpiringAuthCache { authed: authed.clone(), expiry, job_id: None }, + ExpiringAuthCache { authed: authed.clone(), expiry: cache_expiry, job_id: None }, ); Some(OptJobAuthed { authed, job_id: None }) } @@ -697,61 +705,72 @@ impl AuthCache { } } +/// How long a guest JWT resolves from the auth cache before the arm re-runs (and +/// re-reads the key). A guest JWT is not revocable except by the workspace switch or +/// by rotating the key, so the entry must be short enough that a rotated key bites +/// soon, unlike a normal token whose row can be deleted. Also what makes the +/// day-keyed activity dedupe below reachable across a midnight. +const GUEST_JWT_CACHE_TTL: chrono::Duration = chrono::Duration::minutes(5); + lazy_static::lazy_static! { // One `guest_activity` upsert and one `users.login_guest` audit per email, - // workspace and day: a guest JWT is a bearer sent on every call, and neither the - // seat scan nor the audit trail wants one row per request. LRU-bounded; the day is - // in the key, so a new day writes again. + // workspace and day: the arm re-runs every GUEST_JWT_CACHE_TTL, and neither the + // seat scan nor the audit trail wants a write each time. LRU-bounded; the day is in + // the key, so a new day writes again. static ref GUEST_JWT_ACTIVITY_CACHE: Cache = Cache::new(2000); } /// Record that a JWT guest was seen today (the only durable trace of a guest, since it /// leaves no `usr` row), and audit the login the first time. `jwt_entry` marks the row -/// so the seat telemetry can tell a JWT guest from a signed-in one. Idempotent and -/// cached, so a bearer replayed every request writes at most once a day. -async fn record_guest_jwt_activity( - db: &DB, - w_id: &str, - claims: &windmill_common::guest_jwt::GuestJwtClaims, -) { - let cache_key = format!("{}|{w_id}|{}", claims.email, chrono::Utc::now().date_naive()); +/// so the seat telemetry can tell a JWT guest from a signed-in one. The audit is gated +/// on the upsert freshly inserting the row (`xmax = 0`), decided atomically by the DB, +/// so concurrent first requests and separate API nodes emit `users.login_guest` at +/// most once a day. `email` is already lowercased by the caller. +async fn record_guest_jwt_activity(db: &DB, w_id: &str, email: &str, app_path: &str) { + let cache_key = format!("{email}|{w_id}|{}", chrono::Utc::now().date_naive()); if GUEST_JWT_ACTIVITY_CACHE.get(&cache_key).is_some() { return; } - if let Err(e) = sqlx::query!( - "INSERT INTO guest_activity (email, workspace_id, day, jwt_entry) + let inserted = sqlx::query_scalar!( + r#"INSERT INTO guest_activity (email, workspace_id, day, jwt_entry) VALUES ($1, $2, CURRENT_DATE, true) ON CONFLICT (email, workspace_id, day) - DO UPDATE SET jwt_entry = true, last_seen_at = now()", - claims.email, + DO UPDATE SET jwt_entry = true, last_seen_at = now() + RETURNING (xmax = 0) AS "inserted!""#, + email, w_id, ) - .execute(db) - .await - { - tracing::error!("recording guest JWT activity for {w_id}: {e:#}"); - return; - } - let author = windmill_common::audit::AuditAuthor { - email: claims.email.clone(), - username: claims.email.clone(), - username_override: None, - token_prefix: None, + .fetch_one(db) + .await; + let inserted = match inserted { + Ok(v) => v, + Err(e) => { + tracing::error!("recording guest JWT activity for {w_id}: {e:#}"); + return; + } }; - if let Err(e) = windmill_audit::audit_oss::audit_log( - db, - &author, - "users.login_guest", - windmill_audit::ActionKind::Create, - w_id, - Some(claims.app_path.as_str()), - Some([("entry", "jwt")].into()), - ) - .await - { - tracing::error!("auditing guest JWT login for {w_id}: {e:#}"); - } GUEST_JWT_ACTIVITY_CACHE.insert(cache_key, ()); + if inserted { + let author = windmill_common::audit::AuditAuthor { + email: email.to_string(), + username: email.to_string(), + username_override: None, + token_prefix: None, + }; + if let Err(e) = windmill_audit::audit_oss::audit_log( + db, + &author, + "users.login_guest", + windmill_audit::ActionKind::Create, + w_id, + Some(app_path), + Some([("entry", "jwt")].into()), + ) + .await + { + tracing::error!("auditing guest JWT login for {w_id}: {e:#}"); + } + } } pub(crate) async fn extract_token(parts: &mut Parts, state: &S) -> Option { diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index b3c97b5d6b..42fc84534c 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2937,7 +2937,6 @@ lazy_static::lazy_static! { .unwrap_or(8 * 60 * 60); } - /// Mint a browser session for someone the identity provider authenticated who is a /// member of no workspace, so they can open one guest-mode app. Writes no `password` /// and no `usr` row: that absence is what keeps a guest off every seat counter, so diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index ec461c14b0..de5eba567a 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -23,7 +23,9 @@ use crate::DB; /// would otherwise stay valid until it leaked. pub const MAX_LIFETIME_SECS: u64 = 24 * 60 * 60; -/// Bearer prefix. Stateless: verified per request and cached until `exp`, no row. +/// Bearer prefix. Stateless: no `token` row. Verified against the workspace's key and +/// resolved in the auth cache, whose entry is short-lived (not the token's full `exp`) +/// so a rotated key revokes within minutes. See the arm in `windmill-api-auth`. pub const BEARER_PREFIX: &str = "jwt_guest_"; /// Segment that marks a guest JWT on an app share link, right before the token: @@ -94,24 +96,26 @@ pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algori )) } -/// The one algorithm a JWKS key verifies, or `None` if the key is unusable here: a +/// The algorithms a JWKS key may verify, or `None` if the key is unusable here: a /// symmetric key (HS*, a shared secret the embedder would then have to hold), an -/// unsupported family, or a key not marked for signatures. -pub fn jwk_algorithm(jwk: &Jwk) -> Option { +/// unsupported family, or a key not marked for signatures. A key that names its `alg` +/// pins that one; an RSA key that omits it accepts the whole RSA family, and an EC key +/// the algorithm its curve implies, mirroring how a PEM key is accepted. +pub fn jwk_algorithms(jwk: &Jwk) -> Option> { if jwk.common.public_key_use.is_some() && jwk.common.public_key_use != Some(PublicKeyUse::Signature) { return None; } match (&jwk.algorithm, jwk.common.algorithm) { - (AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => Some(alg), - (AlgorithmParameters::RSA(_), None) => Some(Algorithm::RS256), + (AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => Some(vec![alg]), + (AlgorithmParameters::RSA(_), None) => Some(RSA_ALGORITHMS.to_vec()), (AlgorithmParameters::EllipticCurve(_), Some(alg)) if EC_ALGORITHMS.contains(&alg) => { - Some(alg) + Some(vec![alg]) } (AlgorithmParameters::EllipticCurve(p), None) => match p.curve { - jsonwebtoken::jwk::EllipticCurve::P256 => Some(Algorithm::ES256), - jsonwebtoken::jwk::EllipticCurve::P384 => Some(Algorithm::ES384), + jsonwebtoken::jwk::EllipticCurve::P256 => Some(vec![Algorithm::ES256]), + jsonwebtoken::jwk::EllipticCurve::P384 => Some(vec![Algorithm::ES384]), _ => None, }, _ => None, @@ -161,24 +165,33 @@ pub fn verify( struct JwksEntry { keys: Arc>, - fetched_at: Instant, + /// When this entry stops being served and the next request refetches. A good fetch + /// is served for `JWKS_TTL`, a failed one for `JWKS_NEGATIVE_TTL` (serving the last + /// good keys if there are any), so an unreachable issuer cannot be turned into one + /// outbound fetch per request by unauthenticated traffic. + expires_at: Instant, } lazy_static::lazy_static! { static ref JWKS_CACHE: Cache> = Cache::new(200); } -/// How long a fetched key set is served before being refreshed. The same cadence as -/// the instance-level external JWKS. +/// How long a good key set is served before a refresh; also the lag before a +/// rotated-in `kid` is picked up. The cadence of the instance-level external JWKS. const JWKS_TTL: Duration = Duration::from_secs(15 * 60); -/// A `kid` missing from a set fetched longer ago than this refetches once: that is -/// what a key rotation looks like. Floored so unknown `kid`s cannot drive fetches. -const JWKS_MISS_REFETCH_FLOOR: Duration = Duration::from_secs(60); +/// How long a failed fetch is remembered before retrying, so an unreachable issuer is +/// hit at most once per this interval however much guest-JWT traffic arrives. +const JWKS_NEGATIVE_TTL: Duration = Duration::from_secs(30); +/// A JWKS body larger than this is refused rather than buffered: the URL is admin-set +/// but the server it names may be attacker-controlled, and a real key set is a few KB. +const JWKS_MAX_BYTES: usize = 1 << 20; /// 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. +/// 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; let target = crate::ssrf::validate_guest_jwks_url(url) .await .map_err(|e| Error::BadRequest(format!("JWKS URL is not allowed: {e}")))?; @@ -186,23 +199,33 @@ pub async fn fetch_jwks(url: &str) -> Result> { .apply_dns_pinning(crate::utils::configure_client(reqwest::ClientBuilder::new())) .user_agent("windmill/beta") .redirect(reqwest::redirect::Policy::none()) - .connect_timeout(Duration::from_secs(10)) - .timeout(Duration::from_secs(30)) + .connect_timeout(Duration::from_secs(5)) + .timeout(Duration::from_secs(10)) .build() .map_err(|e| Error::internal_err(format!("building JWKS client: {e}")))?; - let set = client + let resp = client .get(url) .send() .await .and_then(|r| r.error_for_status()) - .map_err(|e| Error::BadRequest(format!("could not fetch JWKS: {e}")))? - .json::() - .await + .map_err(|e| Error::BadRequest(format!("could not fetch JWKS: {e}")))?; + let mut stream = resp.bytes_stream(); + let mut body: Vec = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| Error::BadRequest(format!("reading JWKS: {e}")))?; + if body.len() + chunk.len() > JWKS_MAX_BYTES { + return Err(Error::BadRequest(format!( + "JWKS is larger than {JWKS_MAX_BYTES} bytes" + ))); + } + body.extend_from_slice(&chunk); + } + let set: JwkSet = serde_json::from_slice(&body) .map_err(|e| Error::BadRequest(format!("JWKS is not a JSON Web Key Set: {e}")))?; let keys: HashMap = set .keys .into_iter() - .filter(|jwk| jwk_algorithm(jwk).is_some()) + .filter(|jwk| jwk_algorithms(jwk).is_some()) .filter_map(|jwk| jwk.common.key_id.clone().map(|kid| (kid, jwk))) .collect(); if keys.is_empty() { @@ -213,42 +236,60 @@ pub async fn fetch_jwks(url: &str) -> Result> { Ok(keys) } +/// The workspace's JWKS, from cache when fresh. A cold or stale entry is refetched +/// once; a failed refetch serves the last good keys (or an empty set) for +/// `JWKS_NEGATIVE_TTL`, so an unreachable issuer is fetched at most once per that +/// interval whatever the guest-JWT traffic. Fetches thus follow a schedule, never a +/// per-request, attacker-chosen `kid`. async fn cached_jwks(url: &str) -> Result> { if let Some(entry) = JWKS_CACHE.get(url) { - if entry.fetched_at.elapsed() < JWKS_TTL { + if entry.expires_at > Instant::now() { return Ok(entry); } } - refetch_jwks(url).await + match fetch_jwks(url).await { + Ok(keys) => { + let entry = + Arc::new(JwksEntry { keys: Arc::new(keys), expires_at: Instant::now() + JWKS_TTL }); + JWKS_CACHE.insert(url.to_string(), entry.clone()); + Ok(entry) + } + Err(e) => { + let keys = JWKS_CACHE.get(url).map(|stale| stale.keys.clone()).unwrap_or_default(); + let empty = keys.is_empty(); + let entry = + Arc::new(JwksEntry { keys, expires_at: Instant::now() + JWKS_NEGATIVE_TTL }); + JWKS_CACHE.insert(url.to_string(), entry.clone()); + // Nothing good was ever cached: surface the fetch error rather than an + // empty key set that reads as "kid not found". + if empty { + return Err(e); + } + Ok(entry) + } + } } -async fn refetch_jwks(url: &str) -> Result> { - let keys = fetch_jwks(url).await?; - let entry = Arc::new(JwksEntry { keys: Arc::new(keys), fetched_at: Instant::now() }); - JWKS_CACHE.insert(url.to_string(), entry.clone()); - Ok(entry) -} - -/// The key a token's header selects from the workspace's JWKS, by `kid`. -pub async fn jwks_key_for(url: &str, token: &str) -> Result<(DecodingKey, Algorithm)> { +/// The key a token's header selects from the workspace's JWKS, by `kid`, and the +/// algorithms it may verify. An unknown `kid` is refused against the cached set rather +/// than triggering a fetch, so varying `kid` cannot drive outbound requests; a +/// genuinely rotated-in key is picked up within `JWKS_TTL`. +pub async fn jwks_key_for(url: &str, token: &str) -> Result<(DecodingKey, Vec)> { let header = jsonwebtoken::decode_header(token) .map_err(|e| Error::NotAuthorized(format!("guest JWT refused: {e}")))?; let kid = header.kid.ok_or_else(|| { Error::NotAuthorized("guest JWT refused: no kid in the header".to_string()) })?; - let mut entry = cached_jwks(url).await?; - if !entry.keys.contains_key(&kid) && entry.fetched_at.elapsed() >= JWKS_MISS_REFETCH_FLOOR { - entry = refetch_jwks(url).await?; - } + let entry = cached_jwks(url).await?; let jwk = entry.keys.get(&kid).ok_or_else(|| { Error::NotAuthorized(format!("guest JWT refused: kid {kid} is not in the JWKS")) })?; - let alg = jwk_algorithm(jwk).ok_or_else(|| { + let algs = jwk_algorithms(jwk).ok_or_else(|| { Error::NotAuthorized(format!("guest JWT refused: kid {kid} is not a signing key")) })?; let key = DecodingKey::from_jwk(jwk) .map_err(|e| Error::internal_err(format!("unusable JWK {kid}: {e}")))?; - Ok((key, alg)) + Ok((key, algs)) } /// Verify `token` for `w_id` against whatever key the workspace configured. A PEM key @@ -265,8 +306,88 @@ pub async fn verify_for_workspace(db: &DB, w_id: &str, token: &str) -> Result { - let (key, alg) = jwks_key_for(&url, token).await?; - verify(token, &key, &[alg], w_id) + let (key, algorithms) = jwks_key_for(&url, token).await?; + verify(token, &key, &algorithms, w_id) } } } + +#[cfg(test)] +mod tests { + use super::*; + + fn jwk(v: serde_json::Value) -> Jwk { + serde_json::from_value(v).unwrap() + } + + #[test] + fn rsa_key_with_alg_pins_it() { + let k = jwk(serde_json::json!({"kty":"RSA","alg":"RS384","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), Some(vec![Algorithm::RS384])); + } + + #[test] + fn rsa_key_without_alg_takes_the_whole_family() { + // The bug this pins: an alg-less RSA key must not be forced to RS256, which + // would reject valid RS384/512 or PS* tokens. + let k = jwk(serde_json::json!({"kty":"RSA","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), Some(RSA_ALGORITHMS.to_vec())); + } + + #[test] + fn ec_key_takes_its_curve_algorithm() { + let k = jwk(serde_json::json!({"kty":"EC","crv":"P-256","x":"aa","y":"bb"})); + assert_eq!(jwk_algorithms(&k), Some(vec![Algorithm::ES256])); + } + + #[test] + fn symmetric_key_is_refused() { + let k = jwk(serde_json::json!({"kty":"oct","k":"c2VjcmV0"})); + assert_eq!(jwk_algorithms(&k), None); + } + + #[test] + fn a_key_marked_for_encryption_is_refused() { + let k = jwk(serde_json::json!({"kty":"RSA","use":"enc","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), None); + } + + // PUB1's coordinates and its matching PKCS8 private key, for the JWKS-derived + // verification test. + const PUB1_X: &str = "zAfqyCh34iYOCW0vg4ejq_zzJlzLSZScjnVyPjLGTao"; + const PUB1_Y: &str = "RMKOIHOv8tWLnXf7-eMCodDnX038wCjD1sf9jVsf7oI"; + const PRIV1: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; + + #[test] + fn a_jwks_key_verifies_a_real_token() { + // The one path the PEM tests do not cover: a key rebuilt from a JWK verifies a + // token signed by its private half, and the algorithms come from the JWK. + let jwk = jwk(serde_json::json!({ + "kty": "EC", "crv": "P-256", "kid": "k1", "x": PUB1_X, "y": PUB1_Y + })); + let algs = jwk_algorithms(&jwk).expect("EC signing key"); + assert_eq!(algs, vec![Algorithm::ES256]); + let key = jsonwebtoken::DecodingKey::from_jwk(&jwk).expect("usable JWK"); + let payload = serde_json::json!({ + "email": "g@example.com", + "workspace_id": "ws", + "app_path": "u/a/app", + "exp": jsonwebtoken::get_current_timestamp() + 600, + }); + let token = jsonwebtoken::encode( + &jsonwebtoken::Header::new(Algorithm::ES256), + &payload, + &jsonwebtoken::EncodingKey::from_ec_pem(PRIV1.as_bytes()).unwrap(), + ) + .unwrap(); + let out = verify(&token, &key, &algs, "ws").expect("verifies"); + assert_eq!(out.email, "g@example.com"); + // The workspace pin is part of verify. + assert!(verify(&token, &key, &algs, "other-ws").is_err()); + } + + #[test] + fn a_non_key_pem_is_rejected() { + assert!(decoding_key_from_pem("-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----").is_err()); + } +} diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 486d13c19b..3ea7843558 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -41,6 +41,10 @@ pub const PERMISSIONED_AS_MAX_LEN: usize = 55; /// account must not read as absent) or a `usr` row in any workspace (what a service /// account has instead of a password). A guest is someone with none: the single rule /// that keeps an account holder from ever holding a cheaper guest identity. +/// +/// The address is lowercased before the lookup: accounts are stored lowercased, so a +/// mixed-case address would otherwise miss an existing account and be let through. The +/// comparison stays a plain equality (not `lower(email)`), so it uses the email index. pub async fn has_any_account<'c, E: sqlx::Executor<'c, Database = sqlx::Postgres>>( executor: E, email: &str, @@ -49,7 +53,7 @@ pub async fn has_any_account<'c, E: sqlx::Executor<'c, Database = sqlx::Postgres "SELECT EXISTS(SELECT 1 FROM password WHERE email = $1) OR EXISTS(SELECT 1 FROM usr WHERE email = $1)", ) - .bind(email) + .bind(email.to_lowercase()) .fetch_one(executor) .await .map_err(|e| crate::error::Error::internal_err(format!("checking account for {email}: {e:#}"))) diff --git a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte index e1213b4ba9..ee248538f0 100644 --- a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte @@ -2247,7 +2247,9 @@ export async function main( exp (lifetime capped at 24h); it opens only the app named by app_path. Accepted algorithms: RS256/384/512, PS256/384/512, ES256/384. Symmetric algorithms (HS*) are refused. Configure one key, a PEM - public key or a JWKS URL. + public key or a JWKS URL. Point it at an issuer you control: any token that + key signs carrying these claims is accepted, so a shared multi-tenant issuer + is not a good fit. {#snippet children({ item })}