diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index de5eba567a..248c8d4686 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -174,6 +174,9 @@ struct JwksEntry { lazy_static::lazy_static! { static ref JWKS_CACHE: Cache> = Cache::new(200); + /// Per-URL fetch lock: only one refresh per URL is in flight at a time, so a cold + /// or stale entry under a burst triggers one fetch, not one per request. + static ref JWKS_FETCH_LOCKS: Cache>> = Cache::new(200); } /// How long a good key set is served before a refresh; also the lag before a @@ -236,15 +239,40 @@ pub async fn fetch_jwks(url: &str) -> Result> { Ok(keys) } +/// A cached entry that holds no keys is a remembered failure; serving it would report +/// an unreachable issuer as an unknown `kid`. Map it to an issuer-unreachable error. +fn servable(entry: Arc) -> Result> { + if entry.keys.is_empty() { + Err(Error::NotAuthorized( + "guest JWT refused: the JWKS issuer is unreachable".to_string(), + )) + } else { + Ok(entry) + } +} + /// 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`. +/// under a per-URL lock, so a burst triggers one fetch, not one per request; a failed +/// refetch serves the last good keys, or a short-lived empty entry that reads as +/// "issuer unreachable" rather than "kid not found", so an unreachable issuer is hit at +/// most once per `JWKS_NEGATIVE_TTL`. Fetches 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.expires_at > Instant::now() { - return Ok(entry); + return servable(entry); + } + } + // Single-flight: hold the per-URL lock across the fetch. `get_or_insert_with` + // creates the lock atomically, so two cold requests share one. + let lock = JWKS_FETCH_LOCKS + .get_or_insert_with(url, || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))) + .unwrap(); + let _guard = lock.lock().await; + // Another task may have refreshed while we waited for the lock. + if let Some(entry) = JWKS_CACHE.get(url) { + if entry.expires_at > Instant::now() { + return servable(entry); } } match fetch_jwks(url).await { @@ -256,13 +284,11 @@ async fn cached_jwks(url: &str) -> Result> { } 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 { + // Nothing good was ever cached: surface the fetch error itself. + if entry.keys.is_empty() { return Err(e); } Ok(entry) diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 3ea7843558..3d00c5d312 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -31,11 +31,6 @@ pub const USERNAME_GROUP_PREFIX: &str = "group-"; /// columns runnables and triggers store one in. pub const PERMISSIONED_AS_MAX_LEN: usize = 55; -/// An email-shaped username is its own principal, which is how a superadmin acting without a -/// `usr` row is named (`usr.username` is constrained to `[\w-]+`, so a member never is). It is -/// decided before the group convention — an address is never a group's username — and one -/// containing `/` is prefixed, since readers split on the first `/` and would otherwise take -/// `g/alice@example.com` for a group. /// Whether any account exists for `email`: a `password` row (deactivated ones /// included, since the sign-in path filters `disabled = false` and a re-enabled /// account must not read as absent) or a `usr` row in any workspace (what a service @@ -59,6 +54,11 @@ pub async fn has_any_account<'c, E: sqlx::Executor<'c, Database = sqlx::Postgres .map_err(|e| crate::error::Error::internal_err(format!("checking account for {email}: {e:#}"))) } +/// An email-shaped username is its own principal, which is how a superadmin acting without a +/// `usr` row is named (`usr.username` is constrained to `[\w-]+`, so a member never is). It is +/// decided before the group convention — an address is never a group's username — and one +/// containing `/` is prefixed, since readers split on the first `/` and would otherwise take +/// `g/alice@example.com` for a group. pub fn username_to_permissioned_as(user: &str) -> String { if user.contains('@') { return if user.contains('/') { diff --git a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte index ee248538f0..17c82afcf6 100644 --- a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte @@ -2258,11 +2258,15 @@ export async function main( {/snippet} {#if guestJwtKeyType === 'pem'} - + /> {:else}