From 1eb24d76f37e579330763390d33dffa7459349e1 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 14:47:32 -0700 Subject: [PATCH 1/7] fix(federation): don't await a cross-request lock for backend credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `multistore_oidc_provider`'s `AwsBackendAuth` caches federated credentials behind a `futures::lock::Mutex` that single-flights concurrent misses on a role ARN. On Cloudflare Workers that lock is fatal: a request parked on an in-memory waker has no pending I/O of its own, and the runtime cancels any request whose code has run with "no events left in the event loop" — "your Worker's code had hung and would never generate a response". Every request that arrived while a sibling held the lock died at ~5ms with a 500, including ones whose credentials were already cached, since the fast path takes the same lock. All of data.source.coop's open data sits behind one connection (`aws-opendata-us-west-2`, one role ARN, hence one lock), so a single cold isolate took out every concurrent read on it. In a 5-minute prod tail, 167 of 167 such errors were products on that connection. Replace the middleware with `federation::AwsFederation`: fresh credentials come from a plain map under a sync lock that is never held across an `.await`, and a miss runs its own `AssumeRoleWithWebIdentity`. Concurrent misses duplicate that exchange — a few extra STS calls per isolate per credential lifetime — rather than queueing behind one. Verified against the local integration harness (stub API + wrangler dev), 50 concurrent federated reads: before: 49x 500 "hung and would never generate a response", 1x 502 after: 50x 502 (the real STS answer; CI's throwaway key can't federate) Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 1 + Cargo.toml | 7 ++ src/federation.rs | 198 ++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 40 ++++----- tests/federation.rs | 76 +++++++++++++++++ 5 files changed, 296 insertions(+), 26 deletions(-) create mode 100644 src/federation.rs create mode 100644 tests/federation.rs diff --git a/Cargo.lock b/Cargo.lock index c6cd587..b663766 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1892,6 +1892,7 @@ dependencies = [ name = "source-data-proxy" version = "2.2.2" dependencies = [ + "chrono", "console_error_panic_hook", "getrandom 0.4.3", "hmac", diff --git a/Cargo.toml b/Cargo.toml index cd16c3c..e981222 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,10 @@ path = "tests/object_path.rs" name = "fixtures" path = "tests/fixtures.rs" +[[test]] +name = "federation" +path = "tests/federation.rs" + [dependencies] # Multistore multistore = { version = "0.7.0", features = ["azure", "gcp"] } @@ -49,6 +53,9 @@ serde_json = "1" http = "1" percent-encoding = "2" +# Timestamps (backend credential expiry — matches multistore's own chrono types) +chrono = { version = "0.4", default-features = false, features = ["clock"] } + # Hashing (HMAC-SHA256 client-IP hashing for analytics privacy) hmac = "0.12" sha2 = "0.10" diff --git a/src/federation.rs b/src/federation.rs new file mode 100644 index 0000000..aa3947f --- /dev/null +++ b/src/federation.rs @@ -0,0 +1,198 @@ +//! Backend federation: exchange the proxy's OIDC identity for a connection's +//! AWS role credentials, cached isolate-wide. +//! +//! This replaces `multistore_oidc_provider::backend_auth::AwsBackendAuth`. That +//! middleware caches credentials behind a `futures::lock::Mutex`, which +//! single-flights concurrent misses on the same role: the first caller runs +//! `AssumeRoleWithWebIdentity` while the rest `.await` the lock. +//! +//! On Cloudflare Workers, awaiting that lock is fatal. A request parked on an +//! in-memory waker has no pending I/O of its own, and the runtime cancels any +//! request whose "code has executed and no events are left in the event loop" +//! with *"your Worker's code had hung and would never generate a response"*. +//! So every request that arrived while a sibling held the lock died at ~5ms +//! with a 500 — including the ones whose credentials were already cached, since +//! the fast path takes the same lock. All of `data.source.coop`'s open data sits +//! behind one connection (one role ARN, hence one lock), so a single cold +//! isolate took out every concurrent read on it. +//! +//! The fix is to keep no async lock on the request path at all. Fresh +//! credentials are read from a plain map under a sync lock that is never held +//! across an `.await`; a miss runs its own exchange. Concurrent misses each do +//! their own `AssumeRoleWithWebIdentity` rather than queueing behind one — a +//! handful of duplicate STS calls per isolate per credential lifetime, which is +//! the price of never parking a request on another request's I/O. + +use std::borrow::Cow; +use std::collections::HashMap; +use std::sync::{Arc, Mutex, OnceLock}; + +use chrono::{Duration, Utc}; +use multistore::error::ProxyError; +use multistore::middleware::{DispatchContext, Middleware, Next}; +use multistore::route_handler::HandlerAction; +use multistore_oidc_provider::exchange::aws::AwsExchange; +use multistore_oidc_provider::exchange::CredentialExchange; +use multistore_oidc_provider::jwt::JwtSigner; +use multistore_oidc_provider::{BackendCredentials, HttpExchange}; + +/// Treat credentials as stale this long before they actually expire, so one is +/// never handed to a backend request that outlives it. Matches multistore's own +/// refresh lead. +const REFRESH_LEAD_SECS: i64 = 60; + +/// Backend option keys consumed by this middleware — stripped once resolved so +/// they never reach the store builder. +const OIDC_OPTION_KEYS: [&str; 3] = ["auth_type", "oidc_role_arn", "oidc_subject"]; + +type Creds = Arc; + +/// Isolate-shared credentials, keyed by role ARN. +/// +/// A `std::sync::Mutex` rather than an async one, deliberately: the guard is +/// only ever held for a map get/insert and never across an `.await`, which is +/// what keeps a waiting request from being cancelled as hung. Workers is +/// single-threaded, so the lock is never actually contended. +static CACHE: OnceLock>> = OnceLock::new(); + +pub(crate) fn cache() -> std::sync::MutexGuard<'static, HashMap> { + CACHE + .get_or_init(|| Mutex::new(HashMap::new())) + .lock() + // A poisoned lock means some *other* request panicked mid-map-write. + // The map is still structurally sound (insert/get are not interruptible + // in a way that leaves it torn), and refusing to federate over it would + // turn one panic into a permanently broken isolate. + .unwrap_or_else(|e| e.into_inner()) +} + +/// Cached credentials for `role_arn`, if they are still comfortably unexpired. +pub(crate) fn cached(role_arn: &str) -> Option { + let cutoff = Utc::now() + Duration::seconds(REFRESH_LEAD_SECS); + cache() + .get(role_arn) + .filter(|creds| creds.expiration > cutoff) + .cloned() +} + +/// Sanitize an OIDC subject into an AWS `RoleSessionName` (`[\w+=,.@-]{2,64}`). +/// +/// The proxy's per-connection subject (`scv1:conn:{id}`) contains `:`, which STS +/// rejects. This is only a CloudTrail attribution label — the role's trust policy +/// gates on the JWT `sub`/`aud`, not the session name — so a truncation collision +/// is cosmetic. Falls back to `s3-proxy` when sanitizing leaves fewer than the two +/// characters STS requires. +pub(crate) fn session_name(subject: &str) -> String { + let name: String = subject + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() || "+=,.@-_".contains(c) { + c + } else { + '_' + } + }) + .take(64) + .collect(); + if name.len() < 2 { + "s3-proxy".to_string() + } else { + name + } +} + +/// Middleware that resolves `auth_type=oidc` backend options into real AWS +/// credentials via `AssumeRoleWithWebIdentity`. +/// +/// A no-op for connections without `auth_type=oidc` (public buckets served +/// unsigned), which is every request that never reaches the exchange below. +pub(crate) struct AwsFederation { + signer: JwtSigner, + http: H, + issuer: String, + audience: String, +} + +impl AwsFederation { + pub(crate) fn new(signer: JwtSigner, http: H, issuer: String, audience: String) -> Self { + Self { + signer, + http, + issuer, + audience, + } + } + + /// Cached credentials for `role_arn`, minting and exchanging a fresh + /// assertion on a miss. + async fn credentials(&self, role_arn: &str, subject: &str) -> Result { + if let Some(creds) = cached(role_arn) { + return Ok(creds); + } + let token = self + .signer + .sign(subject, &self.issuer, &self.audience, &[])?; + let mut exchange = AwsExchange::new(role_arn.to_string()); + // Name the assumed-role session after the subject so CloudTrail + // attributes each exchange to the originating connection. + exchange.session_name = session_name(subject); + let creds: Creds = Arc::new(exchange.exchange(&self.http, &token).await?); + cache().insert(role_arn.to_string(), creds.clone()); + Ok(creds) + } +} + +impl Middleware for AwsFederation { + async fn handle<'a>( + &'a self, + mut ctx: DispatchContext<'a>, + next: Next<'a>, + ) -> Result { + // Read everything needed out of the borrowed config first, so the + // borrow ends before the config is taken and rewritten below. + let target = ctx.bucket_config.as_deref().and_then(|config| { + (config.option("auth_type") == Some("oidc")).then(|| { + ( + config.backend_type.clone(), + config.option("oidc_role_arn").map(str::to_string), + config + .option("oidc_subject") + .unwrap_or("s3-proxy") + .to_string(), + ) + }) + }); + let Some((backend_type, role_arn, subject)) = target else { + return next.run(ctx).await; + }; + + // STS credentials only sign S3 requests. + if backend_type != "s3" { + return Err(ProxyError::ConfigError(format!( + "OIDC backend auth not yet supported for backend_type '{backend_type}'" + ))); + } + let role_arn = role_arn.ok_or_else(|| { + ProxyError::ConfigError( + "auth_type=oidc requires 'oidc_role_arn' in backend_options".into(), + ) + })?; + + let creds = self.credentials(&role_arn, &subject).await?; + + // `apply_to` sets the credential option keys and clears `skip_signature` + // so the backend request is signed. + let mut resolved = ctx + .bucket_config + .take() + .expect("bucket_config present: `target` is Some only when it is") + .into_owned(); + creds.apply_to(&mut resolved); + for key in OIDC_OPTION_KEYS { + resolved.backend_options.remove(key); + } + ctx.bucket_config = Some(Cow::Owned(resolved)); + + next.run(ctx).await + } +} diff --git a/src/lib.rs b/src/lib.rs index 05eb846..a8f74be 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,13 +4,14 @@ //! / STS-disabled) → rewrite `/{account}/{product}/{key}` to an internal //! `account:product` bucket → dispatch through the multistore gateway → emit //! analytics + location telemetry → apply CORS. Isolate-shared statics (HTTP -//! client, JWKS cache, OIDC provider) initialize lazily from the first +//! client, JWKS cache, federation credentials) initialize lazily from the first //! request's config. mod analytics; mod authz; mod backend_auth; mod config; +mod federation; mod handlers; mod location; mod object_path; @@ -29,9 +30,8 @@ use multistore_cf_workers::{ collect_js_body, GatewayResponseExt, NoopCredentialRegistry, RequestParts, WorkerBackend, WorkerSubscriber, }; -use multistore_oidc_provider::backend_auth::{AwsBackendAuth, MaybeOidcAuth}; use multistore_oidc_provider::route_handler::OidcRouterExt; -use multistore_oidc_provider::{HttpExchange, OidcCredentialProvider, OidcProviderError}; +use multistore_oidc_provider::{HttpExchange, OidcProviderError}; use multistore_path_mapping::{MappedRegistry, PathMapping}; use multistore_sts::jwks::JwksCache; use multistore_sts::route_handler::StsRouterExt; @@ -71,7 +71,7 @@ fn jwks_cache() -> JwksCache { /// returns a proper S3 `ServiceUnavailable` XML error (HttpError → BackendError /// → 503) the client can parse and retry. STS normally answers in well under a /// second, so this only trips on genuine stalls — which only happen on a cold -/// isolate, since the OIDC provider caches credentials across requests once warm. +/// isolate, since `federation` caches the credentials across requests once warm. // ponytail: fixed 10s; promote to an env var if a deployment ever needs to tune it. const STS_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); @@ -108,13 +108,6 @@ impl HttpExchange for FetchHttpExchange { } } -/// Isolate-shared OIDC credential provider for backend federation. The gateway -/// (and its middleware) are rebuilt per request, but the provider — and its -/// credential cache — must persist so the proxy doesn't re-mint a JWT and re-run -/// `AssumeRoleWithWebIdentity` on every request to the same role. Initialized -/// from the first request's signing config, which is constant for the isolate. -static OIDC_PROVIDER: OnceLock> = OnceLock::new(); - #[event(fetch)] async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result { console_error_panic_hook::set_once(); @@ -272,21 +265,16 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result Arc { + Arc::new(BackendCredentials { + access_key_id: "ASIAEXAMPLE".into(), + secret_access_key: "secret".into(), + session_token: "token".into(), + expiration: Utc::now() + Duration::seconds(expires_in_secs), + }) +} + +// ── credential freshness ─────────────────────────────────────────── +// +// The cache is a process-wide static, so each test uses its own role ARN as a +// key rather than clearing shared state. + +#[test] +fn returns_credentials_that_are_comfortably_unexpired() { + cache().insert("arn:fresh".into(), creds(3600)); + assert_eq!( + cached("arn:fresh").map(|c| c.access_key_id.clone()), + Some("ASIAEXAMPLE".to_string()) + ); +} + +#[test] +fn misses_on_an_unknown_role() { + assert!(cached("arn:never-stored").is_none()); +} + +#[test] +fn treats_expired_credentials_as_a_miss() { + cache().insert("arn:expired".into(), creds(-1)); + assert!(cached("arn:expired").is_none()); +} + +/// The refresh lead is the point of the freshness check: credentials that are +/// *technically* still valid but about to expire must not be handed to a backend +/// request that could outlive them. +#[test] +fn treats_nearly_expired_credentials_as_a_miss() { + cache().insert("arn:nearly-expired".into(), creds(30)); + assert!(cached("arn:nearly-expired").is_none()); +} + +// ── RoleSessionName sanitization ─────────────────────────────────── + +#[test] +fn replaces_characters_sts_rejects() { + assert_eq!(session_name("scv1:conn:abc-123"), "scv1_conn_abc-123"); +} + +#[test] +fn passes_through_an_already_valid_name() { + assert_eq!(session_name("s3-proxy"), "s3-proxy"); +} + +#[test] +fn clamps_to_the_64_character_maximum() { + assert_eq!(session_name(&"a".repeat(100)).len(), 64); +} + +#[test] +fn falls_back_when_sanitizing_leaves_too_few_characters() { + assert_eq!(session_name(":"), "s3-proxy"); +} From caee8fe188cad06a2aad4136c2f219a514b51bbf Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 18:34:08 -0700 Subject: [PATCH 2/7] fix(federation): key the credential cache on role ARN + subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keying on the role ARN alone — inherited from multistore's `AwsBackendAuth`, whose cache key is the ARN — is unsound when two connections point at the same role. The role's trust policy conditions on the assertion's `sub` (`scv1:conn:{id}`), so a connection whose subject that policy would reject could be served another connection's cached credentials, succeeding where STS would have refused. It also silently misattributes CloudTrail, which is the whole point of `sts_session_name`. Spotted in the review of #175, which keys its L2 cache on (RoleArn, RoleSessionName) for the same reason. Co-Authored-By: Claude Opus 5 (1M context) --- src/federation.rs | 34 +++++++++++++++++++++++++++------- tests/federation.rs | 36 ++++++++++++++++++++++++++++-------- 2 files changed, 55 insertions(+), 15 deletions(-) diff --git a/src/federation.rs b/src/federation.rs index aa3947f..b873e03 100644 --- a/src/federation.rs +++ b/src/federation.rs @@ -47,7 +47,7 @@ const OIDC_OPTION_KEYS: [&str; 3] = ["auth_type", "oidc_role_arn", "oidc_subject type Creds = Arc; -/// Isolate-shared credentials, keyed by role ARN. +/// Isolate-shared credentials, keyed by [`cache_key`]. /// /// A `std::sync::Mutex` rather than an async one, deliberately: the guard is /// only ever held for a map get/insert and never across an `.await`, which is @@ -55,7 +55,22 @@ type Creds = Arc; /// single-threaded, so the lock is never actually contended. static CACHE: OnceLock>> = OnceLock::new(); -pub(crate) fn cache() -> std::sync::MutexGuard<'static, HashMap> { +/// Cache identity for a federation exchange: the role *and* the subject the +/// assertion is minted for. +/// +/// Keying on the role ARN alone (which multistore's `AwsBackendAuth` did) is +/// unsound when two connections point at the same role: the role's trust policy +/// conditions on the assertion's `sub` (`scv1:conn:{id}`), so a connection whose +/// subject that policy would *reject* could instead be served another +/// connection's cached credentials — succeeding where STS would have said no. +/// The subject is part of the identity being cached, so it belongs in the key. +fn cache_key(role_arn: &str, subject: &str) -> String { + // `\u{1f}` (unit separator) can't occur in an ARN or in a subject, so the + // join is unambiguous without escaping either half. + format!("{role_arn}\u{1f}{subject}") +} + +fn cache() -> std::sync::MutexGuard<'static, HashMap> { CACHE .get_or_init(|| Mutex::new(HashMap::new())) .lock() @@ -66,15 +81,20 @@ pub(crate) fn cache() -> std::sync::MutexGuard<'static, HashMap> .unwrap_or_else(|e| e.into_inner()) } -/// Cached credentials for `role_arn`, if they are still comfortably unexpired. -pub(crate) fn cached(role_arn: &str) -> Option { +/// Cached credentials for `role_arn` as `subject`, if still comfortably unexpired. +pub(crate) fn cached(role_arn: &str, subject: &str) -> Option { let cutoff = Utc::now() + Duration::seconds(REFRESH_LEAD_SECS); cache() - .get(role_arn) + .get(&cache_key(role_arn, subject)) .filter(|creds| creds.expiration > cutoff) .cloned() } +/// Cache `creds` as the credentials for `role_arn` assumed as `subject`. +pub(crate) fn store(role_arn: &str, subject: &str, creds: Creds) { + cache().insert(cache_key(role_arn, subject), creds); +} + /// Sanitize an OIDC subject into an AWS `RoleSessionName` (`[\w+=,.@-]{2,64}`). /// /// The proxy's per-connection subject (`scv1:conn:{id}`) contains `:`, which STS @@ -126,7 +146,7 @@ impl AwsFederation { /// Cached credentials for `role_arn`, minting and exchanging a fresh /// assertion on a miss. async fn credentials(&self, role_arn: &str, subject: &str) -> Result { - if let Some(creds) = cached(role_arn) { + if let Some(creds) = cached(role_arn, subject) { return Ok(creds); } let token = self @@ -137,7 +157,7 @@ impl AwsFederation { // attributes each exchange to the originating connection. exchange.session_name = session_name(subject); let creds: Creds = Arc::new(exchange.exchange(&self.http, &token).await?); - cache().insert(role_arn.to_string(), creds.clone()); + store(role_arn, subject, creds.clone()); Ok(creds) } } diff --git a/tests/federation.rs b/tests/federation.rs index e9a57a3..939b7d2 100644 --- a/tests/federation.rs +++ b/tests/federation.rs @@ -6,10 +6,12 @@ mod federation; use chrono::{Duration, Utc}; -use federation::{cache, cached, session_name}; +use federation::{cached, session_name, store}; use multistore::types::BackendCredentials; use std::sync::Arc; +const SUBJECT: &str = "scv1:conn:abc"; + fn creds(expires_in_secs: i64) -> Arc { Arc::new(BackendCredentials { access_key_id: "ASIAEXAMPLE".into(), @@ -26,22 +28,22 @@ fn creds(expires_in_secs: i64) -> Arc { #[test] fn returns_credentials_that_are_comfortably_unexpired() { - cache().insert("arn:fresh".into(), creds(3600)); + store("arn:fresh", SUBJECT, creds(3600)); assert_eq!( - cached("arn:fresh").map(|c| c.access_key_id.clone()), + cached("arn:fresh", SUBJECT).map(|c| c.access_key_id.clone()), Some("ASIAEXAMPLE".to_string()) ); } #[test] fn misses_on_an_unknown_role() { - assert!(cached("arn:never-stored").is_none()); + assert!(cached("arn:never-stored", SUBJECT).is_none()); } #[test] fn treats_expired_credentials_as_a_miss() { - cache().insert("arn:expired".into(), creds(-1)); - assert!(cached("arn:expired").is_none()); + store("arn:expired", SUBJECT, creds(-1)); + assert!(cached("arn:expired", SUBJECT).is_none()); } /// The refresh lead is the point of the freshness check: credentials that are @@ -49,8 +51,26 @@ fn treats_expired_credentials_as_a_miss() { /// request that could outlive them. #[test] fn treats_nearly_expired_credentials_as_a_miss() { - cache().insert("arn:nearly-expired".into(), creds(30)); - assert!(cached("arn:nearly-expired").is_none()); + store("arn:nearly-expired", SUBJECT, creds(30)); + assert!(cached("arn:nearly-expired", SUBJECT).is_none()); +} + +// ── cache key identity ───────────────────────────────────────────── + +/// The role's trust policy conditions on the assertion's `sub`, so a second +/// connection pointing at the same role must not be served the first one's +/// credentials — that would succeed where STS would have refused. +#[test] +fn does_not_share_credentials_between_subjects_on_one_role() { + store("arn:shared-role", "scv1:conn:allowed", creds(3600)); + assert!(cached("arn:shared-role", "scv1:conn:allowed").is_some()); + assert!(cached("arn:shared-role", "scv1:conn:other").is_none()); +} + +#[test] +fn does_not_share_credentials_between_roles_for_one_subject() { + store("arn:role-a", SUBJECT, creds(3600)); + assert!(cached("arn:role-b", SUBJECT).is_none()); } // ── RoleSessionName sanitization ─────────────────────────────────── From 203e693132f16cd36f0d36fb7d2ec13a31ed959e Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 22:16:01 -0700 Subject: [PATCH 3/7] fix(federation): single-flight renewals, and pin chrono's wasmbind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review of this PR. Renewals were a stampede, not a single duplicate. `lookup`'s predecessor reported a miss for the whole 60s refresh lead, so on a busy isolate *every* concurrent request in that window minted an RSA assertion and ran its own `AssumeRoleWithWebIdentity` — recurring once per credential lifetime per isolate, and liable to earn a `Throttling` → 502 burst from AWS. The discarded credential was still valid for up to 60 more seconds. Serve it. Inside the lead, one caller claims the renewal and the rest keep using the credential they already have. That is single-flight without a lock, and it works precisely because latecomers never need the claimant's result — the thing a mutex cannot give us here. An expired credential is still never served, and a failed exchange releases the slot so the next request retries rather than waiting out the window. Cold isolates still exchange concurrently: with nothing valid to serve there is no single-flight to do that would not mean blocking. Separately, `chrono` was declared `default-features = false, features = ["clock"]`, which drops `wasmbind`. Without it `Utc::now()` resolves to the `SystemTime::now()` branch — std's unimplemented stub on wasm32-unknown-unknown, which panics, i.e. every federated request would 500 at `lookup`. It only worked because multistore and worker pull chrono with defaults on; name it explicitly rather than depend on that. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 12 ++++- src/federation.rs | 114 +++++++++++++++++++++++++++++++++++--------- tests/federation.rs | 75 ++++++++++++++++++++++------- 3 files changed, 161 insertions(+), 40 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e981222..e84333f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,8 +53,16 @@ serde_json = "1" http = "1" percent-encoding = "2" -# Timestamps (backend credential expiry — matches multistore's own chrono types) -chrono = { version = "0.4", default-features = false, features = ["clock"] } +# Timestamps (backend credential expiry — matches multistore's own chrono types). +# `wasmbind` is required, not optional: without it `Utc::now()` compiles to the +# `SystemTime::now()` branch, which on wasm32-unknown-unknown is std's +# unimplemented stub and panics at runtime. It is in chrono's default features +# (which we turn off), and is only present today because multistore depends on +# chrono with defaults on — so name it here rather than rely on that holding. +chrono = { version = "0.4", default-features = false, features = [ + "clock", + "wasmbind", +] } # Hashing (HMAC-SHA256 client-IP hashing for analytics privacy) hmac = "0.12" diff --git a/src/federation.rs b/src/federation.rs index b873e03..9d27a0e 100644 --- a/src/federation.rs +++ b/src/federation.rs @@ -16,12 +16,19 @@ //! behind one connection (one role ARN, hence one lock), so a single cold //! isolate took out every concurrent read on it. //! -//! The fix is to keep no async lock on the request path at all. Fresh -//! credentials are read from a plain map under a sync lock that is never held -//! across an `.await`; a miss runs its own exchange. Concurrent misses each do -//! their own `AssumeRoleWithWebIdentity` rather than queueing behind one — a -//! handful of duplicate STS calls per isolate per credential lifetime, which is -//! the price of never parking a request on another request's I/O. +//! The fix is to keep no async lock on the request path at all. Credentials +//! live in a plain map under a sync lock that is never held across an `.await`, +//! and a request that has nothing usable runs its own exchange rather than +//! queueing behind one. +//! +//! That leaves a duplicate-work window, which [`lookup`] narrows to the case +//! where duplication is unavoidable. A *renewal* is single-flighted without +//! anyone blocking: the credential is still valid inside the refresh lead, so +//! one request claims the exchange while the rest keep serving what they +//! already have. Latecomers never need the refresher's result, which is exactly +//! why this works where a lock cannot. Only a genuinely empty or expired entry +//! makes concurrent requests each exchange — a cold isolate, where there is no +//! usable credential to serve and nothing to single-flight *to*. use std::borrow::Cow; use std::collections::HashMap; @@ -47,13 +54,31 @@ const OIDC_OPTION_KEYS: [&str; 3] = ["auth_type", "oidc_role_arn", "oidc_subject type Creds = Arc; +/// A cached credential plus whether someone is already renewing it. +struct Entry { + creds: Creds, + /// Set by the request that claimed the renewal for this key, so the others + /// keep serving `creds` (still valid) instead of piling onto STS. Cleared + /// when a renewal stores its result, or fails. + renewing: bool, +} + +/// What a caller should do for a given key. +#[derive(Debug)] +pub(crate) enum Action { + /// Use these credentials as-is; no exchange needed. + Serve(Creds), + /// Nothing usable, or this caller claimed the renewal: run the exchange. + Exchange, +} + /// Isolate-shared credentials, keyed by [`cache_key`]. /// /// A `std::sync::Mutex` rather than an async one, deliberately: the guard is /// only ever held for a map get/insert and never across an `.await`, which is /// what keeps a waiting request from being cancelled as hung. Workers is /// single-threaded, so the lock is never actually contended. -static CACHE: OnceLock>> = OnceLock::new(); +static CACHE: OnceLock>> = OnceLock::new(); /// Cache identity for a federation exchange: the role *and* the subject the /// assertion is minted for. @@ -70,7 +95,7 @@ fn cache_key(role_arn: &str, subject: &str) -> String { format!("{role_arn}\u{1f}{subject}") } -fn cache() -> std::sync::MutexGuard<'static, HashMap> { +fn cache() -> std::sync::MutexGuard<'static, HashMap> { CACHE .get_or_init(|| Mutex::new(HashMap::new())) .lock() @@ -81,18 +106,55 @@ fn cache() -> std::sync::MutexGuard<'static, HashMap> { .unwrap_or_else(|e| e.into_inner()) } -/// Cached credentials for `role_arn` as `subject`, if still comfortably unexpired. -pub(crate) fn cached(role_arn: &str, subject: &str) -> Option { - let cutoff = Utc::now() + Duration::seconds(REFRESH_LEAD_SECS); - cache() - .get(&cache_key(role_arn, subject)) - .filter(|creds| creds.expiration > cutoff) - .cloned() +/// Decide what a caller should do for `role_arn` as `subject`, claiming the +/// renewal slot when this caller is the one that must run the exchange. +/// +/// Not a lock: no caller ever waits on another. A credential inside the refresh +/// lead has not actually expired, so everyone but the claimant keeps using it +/// and only the claimant pays for the exchange. Without this, the whole lead +/// window is a miss for *every* concurrent request, and a busy isolate renews +/// with a burst of simultaneous `AssumeRoleWithWebIdentity` calls (which AWS may +/// then throttle into 502s) once per credential lifetime. +pub(crate) fn lookup(role_arn: &str, subject: &str) -> Action { + let now = Utc::now(); + let mut map = cache(); + let Some(entry) = map.get_mut(&cache_key(role_arn, subject)) else { + return Action::Exchange; + }; + if entry.creds.expiration > now + Duration::seconds(REFRESH_LEAD_SECS) { + return Action::Serve(entry.creds.clone()); + } + // Inside the lead but genuinely still valid, and someone is already on it. + if entry.creds.expiration > now && entry.renewing { + return Action::Serve(entry.creds.clone()); + } + // Either we're first into the lead window, or the credential has actually + // expired and there is nothing safe left to serve. Claim the slot — when + // expired this does not gate anyone (they have no usable credential either), + // it just records that a renewal is under way. + entry.renewing = true; + Action::Exchange +} + +/// Release the renewal slot after a failed exchange, so the next request retries +/// instead of waiting out the lead window. Retries stay serialized: whoever +/// retries claims the slot again via [`lookup`]. +fn release(role_arn: &str, subject: &str) { + if let Some(entry) = cache().get_mut(&cache_key(role_arn, subject)) { + entry.renewing = false; + } } -/// Cache `creds` as the credentials for `role_arn` assumed as `subject`. +/// Cache `creds` as the credentials for `role_arn` assumed as `subject`, +/// releasing the renewal slot. pub(crate) fn store(role_arn: &str, subject: &str, creds: Creds) { - cache().insert(cache_key(role_arn, subject), creds); + cache().insert( + cache_key(role_arn, subject), + Entry { + creds, + renewing: false, + }, + ); } /// Sanitize an OIDC subject into an AWS `RoleSessionName` (`[\w+=,.@-]{2,64}`). @@ -144,19 +206,27 @@ impl AwsFederation { } /// Cached credentials for `role_arn`, minting and exchanging a fresh - /// assertion on a miss. + /// assertion when [`lookup`] says this request must. async fn credentials(&self, role_arn: &str, subject: &str) -> Result { - if let Some(creds) = cached(role_arn, subject) { - return Ok(creds); + match lookup(role_arn, subject) { + Action::Serve(creds) => return Ok(creds), + Action::Exchange => {} } let token = self .signer - .sign(subject, &self.issuer, &self.audience, &[])?; + .sign(subject, &self.issuer, &self.audience, &[]) + .inspect_err(|_| release(role_arn, subject))?; let mut exchange = AwsExchange::new(role_arn.to_string()); // Name the assumed-role session after the subject so CloudTrail // attributes each exchange to the originating connection. exchange.session_name = session_name(subject); - let creds: Creds = Arc::new(exchange.exchange(&self.http, &token).await?); + let creds = match exchange.exchange(&self.http, &token).await { + Ok(creds) => Arc::new(creds), + Err(e) => { + release(role_arn, subject); + return Err(e.into()); + } + }; store(role_arn, subject, creds.clone()); Ok(creds) } diff --git a/tests/federation.rs b/tests/federation.rs index 939b7d2..4d83804 100644 --- a/tests/federation.rs +++ b/tests/federation.rs @@ -6,12 +6,24 @@ mod federation; use chrono::{Duration, Utc}; -use federation::{cached, session_name, store}; +use federation::{lookup, session_name, store, Action}; use multistore::types::BackendCredentials; use std::sync::Arc; const SUBJECT: &str = "scv1:conn:abc"; +/// The access key `lookup` would serve, or `None` if it wants an exchange. +fn served(role_arn: &str, subject: &str) -> Option { + match lookup(role_arn, subject) { + Action::Serve(creds) => Some(creds.access_key_id.clone()), + Action::Exchange => None, + } +} + +fn wants_exchange(role_arn: &str, subject: &str) -> bool { + served(role_arn, subject).is_none() +} + fn creds(expires_in_secs: i64) -> Arc { Arc::new(BackendCredentials { access_key_id: "ASIAEXAMPLE".into(), @@ -27,32 +39,63 @@ fn creds(expires_in_secs: i64) -> Arc { // key rather than clearing shared state. #[test] -fn returns_credentials_that_are_comfortably_unexpired() { +fn serves_credentials_that_are_comfortably_unexpired() { store("arn:fresh", SUBJECT, creds(3600)); assert_eq!( - cached("arn:fresh", SUBJECT).map(|c| c.access_key_id.clone()), + served("arn:fresh", SUBJECT), Some("ASIAEXAMPLE".to_string()) ); } #[test] -fn misses_on_an_unknown_role() { - assert!(cached("arn:never-stored", SUBJECT).is_none()); +fn exchanges_for_an_unknown_role() { + assert!(wants_exchange("arn:never-stored", SUBJECT)); } +/// An expired credential must never be served, even though a renewal is +/// recorded as under way — there is nothing safe left to hand out. #[test] -fn treats_expired_credentials_as_a_miss() { +fn never_serves_an_expired_credential() { store("arn:expired", SUBJECT, creds(-1)); - assert!(cached("arn:expired", SUBJECT).is_none()); + assert!(wants_exchange("arn:expired", SUBJECT)); + assert!(wants_exchange("arn:expired", SUBJECT)); +} + +// ── renewal single-flight ────────────────────────────────────────── + +/// Inside the refresh lead the credential is still valid, so exactly one caller +/// takes the exchange and everyone else keeps serving what they have. Without +/// this, a busy isolate renews with a burst of simultaneous STS calls. +#[test] +fn only_the_first_caller_in_the_refresh_lead_exchanges() { + store("arn:renewing", SUBJECT, creds(30)); + + assert!( + wants_exchange("arn:renewing", SUBJECT), + "first caller claims the renewal" + ); + for _ in 0..10 { + assert_eq!( + served("arn:renewing", SUBJECT), + Some("ASIAEXAMPLE".to_string()), + "later callers serve the still-valid credential instead of piling on" + ); + } } -/// The refresh lead is the point of the freshness check: credentials that are -/// *technically* still valid but about to expire must not be handed to a backend -/// request that could outlive them. +/// A renewed credential is served straight from the cache again, and the +/// renewal slot is released so the *next* lead window gets its own single flight. #[test] -fn treats_nearly_expired_credentials_as_a_miss() { - store("arn:nearly-expired", SUBJECT, creds(30)); - assert!(cached("arn:nearly-expired", SUBJECT).is_none()); +fn storing_a_renewal_releases_the_slot() { + store("arn:renewed", SUBJECT, creds(30)); + assert!(wants_exchange("arn:renewed", SUBJECT)); + + store("arn:renewed", SUBJECT, creds(3600)); + assert!(served("arn:renewed", SUBJECT).is_some()); + + // Age back into the lead: a caller must be able to claim it again. + store("arn:renewed", SUBJECT, creds(30)); + assert!(wants_exchange("arn:renewed", SUBJECT)); } // ── cache key identity ───────────────────────────────────────────── @@ -63,14 +106,14 @@ fn treats_nearly_expired_credentials_as_a_miss() { #[test] fn does_not_share_credentials_between_subjects_on_one_role() { store("arn:shared-role", "scv1:conn:allowed", creds(3600)); - assert!(cached("arn:shared-role", "scv1:conn:allowed").is_some()); - assert!(cached("arn:shared-role", "scv1:conn:other").is_none()); + assert!(served("arn:shared-role", "scv1:conn:allowed").is_some()); + assert!(wants_exchange("arn:shared-role", "scv1:conn:other")); } #[test] fn does_not_share_credentials_between_roles_for_one_subject() { store("arn:role-a", SUBJECT, creds(3600)); - assert!(cached("arn:role-b", SUBJECT).is_none()); + assert!(wants_exchange("arn:role-b", SUBJECT)); } // ── RoleSessionName sanitization ─────────────────────────────────── From 442acc20f0ac56ca8eb6069594c9773cb03718e2 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 22:42:57 -0700 Subject: [PATCH 4/7] fix(federation): floor the serve window and reap abandoned renewal claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the non-blocking renewal added in 203e693, found in review. A non-claimant could be served a credential with milliseconds of life left: the serve branch only required `expiration > now`. The backend request it signs still has to be sent and answered, so it could land after expiry — S3 answers `ExpiredToken`, which reaches the caller as a misleading `AccessDenied` rather than a retryable server-side error. Reachable whenever renewals keep failing (STS throttling): each attempt releases the claim, the next request claims and fails too, and everyone else keeps serving an ever-older credential for the whole 60s lead. `MIN_SERVE_SECS` now floors it at 5s; below that a request mints its own. A claim was also never reaped. `renewing` was cleared only by `store` or `release`, so a claimant whose request died between claiming and either — a cancelled or evicted request — pinned the key as renewing until the credential expired, at which point every concurrent request exchanged at once: exactly the burst the claim exists to prevent. `renewing` becomes `renewing_since`, and a claim older than `CLAIM_TIMEOUT_SECS` (30s, comfortably above the 10s `STS_REQUEST_TIMEOUT`) can be re-taken. Also corrects `release`'s doc comment, which claimed retries stay serialized unconditionally. That holds only while the credential is still servable; below the floor there is nothing safe to serve, so concurrent requests each exchange — correct behaviour, but not what the comment said. Matches developmentseed/multistore#133, which upstreams this design. Co-Authored-By: Claude Opus 5 (1M context) --- src/federation.rs | 82 +++++++++++++++++++++++++++++++++------------ tests/federation.rs | 28 +++++++++++++++- 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/src/federation.rs b/src/federation.rs index 9d27a0e..ca09578 100644 --- a/src/federation.rs +++ b/src/federation.rs @@ -26,9 +26,17 @@ //! anyone blocking: the credential is still valid inside the refresh lead, so //! one request claims the exchange while the rest keep serving what they //! already have. Latecomers never need the refresher's result, which is exactly -//! why this works where a lock cannot. Only a genuinely empty or expired entry -//! makes concurrent requests each exchange — a cold isolate, where there is no -//! usable credential to serve and nothing to single-flight *to*. +//! why this works where a lock cannot. +//! +//! Only a genuinely empty or expired entry makes concurrent requests each +//! exchange. Collapsing *those* would mean waiting, and the one way to wait on +//! Workers without being cancelled is polling a real timer — which costs about +//! as long as the exchange it avoids, while adding a spin loop and a dependency +//! on the claimant surviving. The duplication is bounded to one burst per +//! isolate cold start, against an STS endpoint provisioned for it. +//! +//! Upstreamed as developmentseed/multistore#133; delete this module and go back +//! to `AwsBackendAuth` once that releases. use std::borrow::Cow; use std::collections::HashMap; @@ -48,19 +56,34 @@ use multistore_oidc_provider::{BackendCredentials, HttpExchange}; /// refresh lead. const REFRESH_LEAD_SECS: i64 = 60; +/// Never serve a credential with less than this much life left, even while a +/// renewal is in flight: the backend request it signs still has to be sent and +/// answered. Below this the request mints its own instead — S3 rejects an +/// expired credential with `ExpiredToken`, which reaches the caller as a +/// misleading `AccessDenied` rather than a retryable server-side error. +const MIN_SERVE_SECS: i64 = 5; + +/// Treat a renewal claim older than this as abandoned and let another request +/// take it. A claimant cancelled between claiming and storing would otherwise +/// leave the key marked as renewing until the credential expires, at which point +/// every concurrent request exchanges at once — the burst the claim exists to +/// prevent. Comfortably above `STS_REQUEST_TIMEOUT`, so a live-but-slow exchange +/// is never mistaken for an abandoned one. +const CLAIM_TIMEOUT_SECS: i64 = 30; + /// Backend option keys consumed by this middleware — stripped once resolved so /// they never reach the store builder. const OIDC_OPTION_KEYS: [&str; 3] = ["auth_type", "oidc_role_arn", "oidc_subject"]; type Creds = Arc; -/// A cached credential plus whether someone is already renewing it. +/// A cached credential and the renewal claim on it, if any. struct Entry { creds: Creds, - /// Set by the request that claimed the renewal for this key, so the others - /// keep serving `creds` (still valid) instead of piling onto STS. Cleared - /// when a renewal stores its result, or fails. - renewing: bool, + /// When a request claimed the renewal of `creds`, so the others keep serving + /// it (still valid) instead of piling onto STS. `None` when no renewal is in + /// flight; see [`CLAIM_TIMEOUT_SECS`] for how a stuck claim is reaped. + renewing_since: Option>, } /// What a caller should do for a given key. @@ -124,39 +147,56 @@ pub(crate) fn lookup(role_arn: &str, subject: &str) -> Action { if entry.creds.expiration > now + Duration::seconds(REFRESH_LEAD_SECS) { return Action::Serve(entry.creds.clone()); } - // Inside the lead but genuinely still valid, and someone is already on it. - if entry.creds.expiration > now && entry.renewing { + // Inside the lead, someone is already on it, and there is enough life left + // for the backend request this signs to complete. + let claimed = entry + .renewing_since + .is_some_and(|at| now < at + Duration::seconds(CLAIM_TIMEOUT_SECS)); + if claimed && entry.creds.expiration > now + Duration::seconds(MIN_SERVE_SECS) { return Action::Serve(entry.creds.clone()); } - // Either we're first into the lead window, or the credential has actually - // expired and there is nothing safe left to serve. Claim the slot — when - // expired this does not gate anyone (they have no usable credential either), - // it just records that a renewal is under way. - entry.renewing = true; + // Either we're first into the lead window, the previous claim went stale, or + // the credential is too near expiry to hand out. Claim and exchange — when + // there is nothing usable left this gates no one, since concurrent requests + // have no credential to serve either. + entry.renewing_since = Some(now); Action::Exchange } -/// Release the renewal slot after a failed exchange, so the next request retries -/// instead of waiting out the lead window. Retries stay serialized: whoever -/// retries claims the slot again via [`lookup`]. +/// Release the renewal claim after a failed exchange, so the next request +/// retries instead of coasting on a credential nobody is renewing. +/// +/// Retries are serialized only while the credential is still servable: the next +/// request claims via [`lookup`] and the rest keep serving. Once it drops below +/// [`MIN_SERVE_SECS`] there is nothing safe to serve, so every concurrent +/// request exchanges — which is the correct behaviour, not a lapse. fn release(role_arn: &str, subject: &str) { if let Some(entry) = cache().get_mut(&cache_key(role_arn, subject)) { - entry.renewing = false; + entry.renewing_since = None; } } /// Cache `creds` as the credentials for `role_arn` assumed as `subject`, -/// releasing the renewal slot. +/// releasing the renewal claim. pub(crate) fn store(role_arn: &str, subject: &str, creds: Creds) { cache().insert( cache_key(role_arn, subject), Entry { creds, - renewing: false, + renewing_since: None, }, ); } +/// Mark a renewal as claimed at `at`. Test-only seam for the abandoned-claim +/// path, which is otherwise only reachable by a request dying mid-exchange. +#[cfg(test)] +pub(crate) fn mark_renewing_since(role_arn: &str, subject: &str, at: chrono::DateTime) { + if let Some(entry) = cache().get_mut(&cache_key(role_arn, subject)) { + entry.renewing_since = Some(at); + } +} + /// Sanitize an OIDC subject into an AWS `RoleSessionName` (`[\w+=,.@-]{2,64}`). /// /// The proxy's per-connection subject (`scv1:conn:{id}`) contains `:`, which STS diff --git a/tests/federation.rs b/tests/federation.rs index 4d83804..da8dd0a 100644 --- a/tests/federation.rs +++ b/tests/federation.rs @@ -6,7 +6,7 @@ mod federation; use chrono::{Duration, Utc}; -use federation::{lookup, session_name, store, Action}; +use federation::{lookup, mark_renewing_since, session_name, store, Action}; use multistore::types::BackendCredentials; use std::sync::Arc; @@ -83,6 +83,32 @@ fn only_the_first_caller_in_the_refresh_lead_exchanges() { } } +/// A credential too close to expiry is never served, even with a renewal +/// claimed. Signing with it risks S3 rejecting an expired token mid-flight, +/// which reaches the caller as a misleading `AccessDenied` rather than a +/// retryable server-side error. +#[test] +fn never_serves_a_credential_about_to_expire() { + store("arn:about-to-expire", SUBJECT, creds(2)); + // First caller claims the renewal and leaves the near-expired credential. + assert!(wants_exchange("arn:about-to-expire", SUBJECT)); + // A concurrent caller must mint rather than sign with 2s of runway. + assert!(wants_exchange("arn:about-to-expire", SUBJECT)); +} + +/// A claimant that never completes — a request cancelled mid-exchange — must not +/// pin the key as renewing until the credential expires, which would leave every +/// concurrent request to stampede at once when it does. +#[test] +fn reclaims_an_abandoned_renewal() { + store("arn:abandoned", SUBJECT, creds(30)); + mark_renewing_since("arn:abandoned", SUBJECT, Utc::now() - Duration::seconds(31)); + assert!( + wants_exchange("arn:abandoned", SUBJECT), + "a stale claim should be re-takeable" + ); +} + /// A renewed credential is served straight from the cache again, and the /// renewal slot is released so the *next* lead window gets its own single flight. #[test] From 38d9c3a15b8dda4289ba787a474446e0348472da Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 23:04:50 -0700 Subject: [PATCH 5/7] docs(federation): size the cold-isolate burst and pin the serve-floor overtake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirrors developmentseed/multistore#133 (bcf3024), from review of that PR. "Bounded to one burst per isolate cold start" undersold the case that matters. One cold isolate's burst is small; a deploy or mass eviction cools every isolate at once, so the bursts coincide fleet-wide and reach STS together — where throttling becomes a 502 per request. Name that, and point at an L2 tier (#175) as the mitigation rather than a bigger in-memory cache, which is exactly what a deploy discards. Also state, in the module docs and at the call site, that inside MIN_SERVE_SECS a live claim is deliberately overtaken so several exchanges can run at once at the edge of expiry. Implied by the code but not written down; the existing test now asserts it rather than leaving it looking accidental. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) --- src/federation.rs | 33 +++++++++++++++++++++++++-------- tests/federation.rs | 5 ++++- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/federation.rs b/src/federation.rs index ca09578..936b429 100644 --- a/src/federation.rs +++ b/src/federation.rs @@ -29,11 +29,23 @@ //! why this works where a lock cannot. //! //! Only a genuinely empty or expired entry makes concurrent requests each -//! exchange. Collapsing *those* would mean waiting, and the one way to wait on -//! Workers without being cancelled is polling a real timer — which costs about -//! as long as the exchange it avoids, while adding a spin loop and a dependency -//! on the claimant surviving. The duplication is bounded to one burst per -//! isolate cold start, against an STS endpoint provisioned for it. +//! exchange, with no cap beyond how many arrive before the first one stores. +//! Collapsing *those* would mean waiting, and the one way to wait on Workers +//! without being cancelled is polling a real timer — which costs about as long +//! as the exchange it avoids, while adding a spin loop and a dependency on the +//! claimant surviving. +//! +//! One cold isolate's burst is small. A deploy or mass eviction is the case to +//! watch: it cools every isolate at once, so the bursts coincide fleet-wide and +//! reach STS together, where throttling would become a 502 for each request in +//! the burst. The mitigation is an L2 tier (Cache API / KV) inside the exchange, +//! not a bigger in-memory cache — that tier is exactly what a deploy discards. +//! See #175, which builds one. +//! +//! Inside [`MIN_SERVE_SECS`] a live claim is deliberately overtaken, so a few +//! exchanges can run at once right at the edge of expiry: nothing safe is left +//! to serve, and the alternatives are making the request wait or failing one +//! that could have succeeded. //! //! Upstreamed as developmentseed/multistore#133; delete this module and go back //! to `AwsBackendAuth` once that releases. @@ -156,9 +168,14 @@ pub(crate) fn lookup(role_arn: &str, subject: &str) -> Action { return Action::Serve(entry.creds.clone()); } // Either we're first into the lead window, the previous claim went stale, or - // the credential is too near expiry to hand out. Claim and exchange — when - // there is nothing usable left this gates no one, since concurrent requests - // have no credential to serve either. + // the credential is now inside MIN_SERVE_SECS and too near expiry to hand + // out. + // + // That last case deliberately ignores a live claim, so in the final + // MIN_SERVE_SECS a claimant can be overtaken and several exchanges run at + // once. Intended: nothing safe is left to serve, so the alternatives are + // making the request wait (which the module docs rule out) or failing one + // that could have succeeded. Bounded to the requests arriving in that window. entry.renewing_since = Some(now); Action::Exchange } diff --git a/tests/federation.rs b/tests/federation.rs index da8dd0a..acf60e0 100644 --- a/tests/federation.rs +++ b/tests/federation.rs @@ -92,7 +92,10 @@ fn never_serves_a_credential_about_to_expire() { store("arn:about-to-expire", SUBJECT, creds(2)); // First caller claims the renewal and leaves the near-expired credential. assert!(wants_exchange("arn:about-to-expire", SUBJECT)); - // A concurrent caller must mint rather than sign with 2s of runway. + // A concurrent caller overtakes that live claim rather than signing with 2s + // of runway — deliberate, and the reason several exchanges can run at once + // right at the edge of expiry. + assert!(wants_exchange("arn:about-to-expire", SUBJECT)); assert!(wants_exchange("arn:about-to-expire", SUBJECT)); } From d80fc6e58953c94d22693be078ba0ba73a4ecfaa Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 23:16:57 -0700 Subject: [PATCH 6/7] refactor(federation): use multistore's upstream credential cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit multistore#133 merged the fix upstream, so the local `federation` middleware is now a duplicate of `AwsBackendAuth`. Delete it and go back to the upstream wiring: `MaybeOidcAuth::Enabled(AwsBackendAuth::new(provider))` over the isolate-shared `OIDC_PROVIDER`, whose cache no longer blocks. `chrono` goes with it — it was a direct dependency only for the local cache's expiry checks; upstream owns those now, so the `wasmbind` hazard is upstream's to hold (multistore depends on chrono with default features, which include it). `[patch.crates-io]` temporarily points every multistore crate at the exact commit release-please will cut 0.7.2 from (multistore#134), so the PR preview exercises the real upstream code on a live Workers deployment — the only place this failure mode is observable. Pinned to a rev rather than `branch = "main"`, so an unrelated merge cannot silently change what is being verified. The patch must not reach production: once 0.7.2 publishes, drop it and move the dependencies to `version = "0.7.2"`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 27 ++-- Cargo.toml | 34 +++-- src/federation.rs | 345 -------------------------------------------- src/lib.rs | 40 +++-- tests/federation.rs | 168 --------------------- 5 files changed, 55 insertions(+), 559 deletions(-) delete mode 100644 src/federation.rs delete mode 100644 tests/federation.rs diff --git a/Cargo.lock b/Cargo.lock index b663766..7475fd3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1034,9 +1034,8 @@ dependencies = [ [[package]] name = "multistore" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "10553bb6d41a7caf40663151d2ebb73f0032bed3474031bb69c81ce8a0a46b39" +version = "0.7.1" +source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" dependencies = [ "async-trait", "base64", @@ -1063,9 +1062,8 @@ dependencies = [ [[package]] name = "multistore-cf-workers" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89cb288f2d08ba9fa1b1970e3323266da7cee51dea786d3be6c73cbb85ebeac6" +version = "0.7.1" +source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" dependencies = [ "async-trait", "bytes", @@ -1090,13 +1088,11 @@ dependencies = [ [[package]] name = "multistore-oidc-provider" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b743bc10e0fdfb5c70f9f302e0bf756714b7e09cfd41d837918425db3c6b36eb" +version = "0.7.1" +source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" dependencies = [ "base64", "chrono", - "futures", "multistore", "quick-xml", "rsa", @@ -1110,9 +1106,8 @@ dependencies = [ [[package]] name = "multistore-path-mapping" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "17ce2241c2cdd6bbbf239498ec7caa9a4488b28c159f58288261435b4f090975" +version = "0.7.1" +source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" dependencies = [ "multistore", "percent-encoding", @@ -1121,9 +1116,8 @@ dependencies = [ [[package]] name = "multistore-sts" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee8407b2bc78eeadd420fc6c952cc9c6ac5f51c7e52643417d3ea048859cdad2" +version = "0.7.1" +source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" dependencies = [ "aes-gcm", "base64", @@ -1892,7 +1886,6 @@ dependencies = [ name = "source-data-proxy" version = "2.2.2" dependencies = [ - "chrono", "console_error_panic_hook", "getrandom 0.4.3", "hmac", diff --git a/Cargo.toml b/Cargo.toml index e84333f..c028844 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,10 +34,6 @@ path = "tests/object_path.rs" name = "fixtures" path = "tests/fixtures.rs" -[[test]] -name = "federation" -path = "tests/federation.rs" - [dependencies] # Multistore multistore = { version = "0.7.0", features = ["azure", "gcp"] } @@ -53,17 +49,6 @@ serde_json = "1" http = "1" percent-encoding = "2" -# Timestamps (backend credential expiry — matches multistore's own chrono types). -# `wasmbind` is required, not optional: without it `Utc::now()` compiles to the -# `SystemTime::now()` branch, which on wasm32-unknown-unknown is std's -# unimplemented stub and panics at runtime. It is in chrono's default features -# (which we turn off), and is only present today because multistore depends on -# chrono with defaults on — so name it here rather than rely on that holding. -chrono = { version = "0.4", default-features = false, features = [ - "clock", - "wasmbind", -] } - # Hashing (HMAC-SHA256 client-IP hashing for analytics privacy) hmac = "0.12" sha2 = "0.10" @@ -101,3 +86,22 @@ web-sys = { version = "0.3", features = [ ] } worker = { version = "=0.7.5", features = ["http"] } worker-macros = { version = "=0.7.5", features = ["http"] } + +# ── TEMPORARY: verify multistore 0.7.2 before it is cut ────────────── +# The credential-cache fix this branch needs (developmentseed/multistore#133) +# is merged to multistore `main` but not yet published; release-please has it +# queued as 0.7.2 in multistore#134. Point at the exact commit that release will +# be cut from, so the PR preview exercises the real upstream code on a live +# Workers deployment — the only place this failure mode is observable. +# +# Pinned to a rev, not `branch = "main"`: a branch would let an unrelated merge +# silently change what the preview builds and invalidate the verification. +# +# REMOVE BEFORE MERGE — replace with `version = "0.7.2"` on the dependencies +# above once multistore#134 lands. This must not reach production. +[patch.crates-io] +multistore = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } +multistore-oidc-provider = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } +multistore-path-mapping = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } +multistore-sts = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } +multistore-cf-workers = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } diff --git a/src/federation.rs b/src/federation.rs deleted file mode 100644 index 936b429..0000000 --- a/src/federation.rs +++ /dev/null @@ -1,345 +0,0 @@ -//! Backend federation: exchange the proxy's OIDC identity for a connection's -//! AWS role credentials, cached isolate-wide. -//! -//! This replaces `multistore_oidc_provider::backend_auth::AwsBackendAuth`. That -//! middleware caches credentials behind a `futures::lock::Mutex`, which -//! single-flights concurrent misses on the same role: the first caller runs -//! `AssumeRoleWithWebIdentity` while the rest `.await` the lock. -//! -//! On Cloudflare Workers, awaiting that lock is fatal. A request parked on an -//! in-memory waker has no pending I/O of its own, and the runtime cancels any -//! request whose "code has executed and no events are left in the event loop" -//! with *"your Worker's code had hung and would never generate a response"*. -//! So every request that arrived while a sibling held the lock died at ~5ms -//! with a 500 — including the ones whose credentials were already cached, since -//! the fast path takes the same lock. All of `data.source.coop`'s open data sits -//! behind one connection (one role ARN, hence one lock), so a single cold -//! isolate took out every concurrent read on it. -//! -//! The fix is to keep no async lock on the request path at all. Credentials -//! live in a plain map under a sync lock that is never held across an `.await`, -//! and a request that has nothing usable runs its own exchange rather than -//! queueing behind one. -//! -//! That leaves a duplicate-work window, which [`lookup`] narrows to the case -//! where duplication is unavoidable. A *renewal* is single-flighted without -//! anyone blocking: the credential is still valid inside the refresh lead, so -//! one request claims the exchange while the rest keep serving what they -//! already have. Latecomers never need the refresher's result, which is exactly -//! why this works where a lock cannot. -//! -//! Only a genuinely empty or expired entry makes concurrent requests each -//! exchange, with no cap beyond how many arrive before the first one stores. -//! Collapsing *those* would mean waiting, and the one way to wait on Workers -//! without being cancelled is polling a real timer — which costs about as long -//! as the exchange it avoids, while adding a spin loop and a dependency on the -//! claimant surviving. -//! -//! One cold isolate's burst is small. A deploy or mass eviction is the case to -//! watch: it cools every isolate at once, so the bursts coincide fleet-wide and -//! reach STS together, where throttling would become a 502 for each request in -//! the burst. The mitigation is an L2 tier (Cache API / KV) inside the exchange, -//! not a bigger in-memory cache — that tier is exactly what a deploy discards. -//! See #175, which builds one. -//! -//! Inside [`MIN_SERVE_SECS`] a live claim is deliberately overtaken, so a few -//! exchanges can run at once right at the edge of expiry: nothing safe is left -//! to serve, and the alternatives are making the request wait or failing one -//! that could have succeeded. -//! -//! Upstreamed as developmentseed/multistore#133; delete this module and go back -//! to `AwsBackendAuth` once that releases. - -use std::borrow::Cow; -use std::collections::HashMap; -use std::sync::{Arc, Mutex, OnceLock}; - -use chrono::{Duration, Utc}; -use multistore::error::ProxyError; -use multistore::middleware::{DispatchContext, Middleware, Next}; -use multistore::route_handler::HandlerAction; -use multistore_oidc_provider::exchange::aws::AwsExchange; -use multistore_oidc_provider::exchange::CredentialExchange; -use multistore_oidc_provider::jwt::JwtSigner; -use multistore_oidc_provider::{BackendCredentials, HttpExchange}; - -/// Treat credentials as stale this long before they actually expire, so one is -/// never handed to a backend request that outlives it. Matches multistore's own -/// refresh lead. -const REFRESH_LEAD_SECS: i64 = 60; - -/// Never serve a credential with less than this much life left, even while a -/// renewal is in flight: the backend request it signs still has to be sent and -/// answered. Below this the request mints its own instead — S3 rejects an -/// expired credential with `ExpiredToken`, which reaches the caller as a -/// misleading `AccessDenied` rather than a retryable server-side error. -const MIN_SERVE_SECS: i64 = 5; - -/// Treat a renewal claim older than this as abandoned and let another request -/// take it. A claimant cancelled between claiming and storing would otherwise -/// leave the key marked as renewing until the credential expires, at which point -/// every concurrent request exchanges at once — the burst the claim exists to -/// prevent. Comfortably above `STS_REQUEST_TIMEOUT`, so a live-but-slow exchange -/// is never mistaken for an abandoned one. -const CLAIM_TIMEOUT_SECS: i64 = 30; - -/// Backend option keys consumed by this middleware — stripped once resolved so -/// they never reach the store builder. -const OIDC_OPTION_KEYS: [&str; 3] = ["auth_type", "oidc_role_arn", "oidc_subject"]; - -type Creds = Arc; - -/// A cached credential and the renewal claim on it, if any. -struct Entry { - creds: Creds, - /// When a request claimed the renewal of `creds`, so the others keep serving - /// it (still valid) instead of piling onto STS. `None` when no renewal is in - /// flight; see [`CLAIM_TIMEOUT_SECS`] for how a stuck claim is reaped. - renewing_since: Option>, -} - -/// What a caller should do for a given key. -#[derive(Debug)] -pub(crate) enum Action { - /// Use these credentials as-is; no exchange needed. - Serve(Creds), - /// Nothing usable, or this caller claimed the renewal: run the exchange. - Exchange, -} - -/// Isolate-shared credentials, keyed by [`cache_key`]. -/// -/// A `std::sync::Mutex` rather than an async one, deliberately: the guard is -/// only ever held for a map get/insert and never across an `.await`, which is -/// what keeps a waiting request from being cancelled as hung. Workers is -/// single-threaded, so the lock is never actually contended. -static CACHE: OnceLock>> = OnceLock::new(); - -/// Cache identity for a federation exchange: the role *and* the subject the -/// assertion is minted for. -/// -/// Keying on the role ARN alone (which multistore's `AwsBackendAuth` did) is -/// unsound when two connections point at the same role: the role's trust policy -/// conditions on the assertion's `sub` (`scv1:conn:{id}`), so a connection whose -/// subject that policy would *reject* could instead be served another -/// connection's cached credentials — succeeding where STS would have said no. -/// The subject is part of the identity being cached, so it belongs in the key. -fn cache_key(role_arn: &str, subject: &str) -> String { - // `\u{1f}` (unit separator) can't occur in an ARN or in a subject, so the - // join is unambiguous without escaping either half. - format!("{role_arn}\u{1f}{subject}") -} - -fn cache() -> std::sync::MutexGuard<'static, HashMap> { - CACHE - .get_or_init(|| Mutex::new(HashMap::new())) - .lock() - // A poisoned lock means some *other* request panicked mid-map-write. - // The map is still structurally sound (insert/get are not interruptible - // in a way that leaves it torn), and refusing to federate over it would - // turn one panic into a permanently broken isolate. - .unwrap_or_else(|e| e.into_inner()) -} - -/// Decide what a caller should do for `role_arn` as `subject`, claiming the -/// renewal slot when this caller is the one that must run the exchange. -/// -/// Not a lock: no caller ever waits on another. A credential inside the refresh -/// lead has not actually expired, so everyone but the claimant keeps using it -/// and only the claimant pays for the exchange. Without this, the whole lead -/// window is a miss for *every* concurrent request, and a busy isolate renews -/// with a burst of simultaneous `AssumeRoleWithWebIdentity` calls (which AWS may -/// then throttle into 502s) once per credential lifetime. -pub(crate) fn lookup(role_arn: &str, subject: &str) -> Action { - let now = Utc::now(); - let mut map = cache(); - let Some(entry) = map.get_mut(&cache_key(role_arn, subject)) else { - return Action::Exchange; - }; - if entry.creds.expiration > now + Duration::seconds(REFRESH_LEAD_SECS) { - return Action::Serve(entry.creds.clone()); - } - // Inside the lead, someone is already on it, and there is enough life left - // for the backend request this signs to complete. - let claimed = entry - .renewing_since - .is_some_and(|at| now < at + Duration::seconds(CLAIM_TIMEOUT_SECS)); - if claimed && entry.creds.expiration > now + Duration::seconds(MIN_SERVE_SECS) { - return Action::Serve(entry.creds.clone()); - } - // Either we're first into the lead window, the previous claim went stale, or - // the credential is now inside MIN_SERVE_SECS and too near expiry to hand - // out. - // - // That last case deliberately ignores a live claim, so in the final - // MIN_SERVE_SECS a claimant can be overtaken and several exchanges run at - // once. Intended: nothing safe is left to serve, so the alternatives are - // making the request wait (which the module docs rule out) or failing one - // that could have succeeded. Bounded to the requests arriving in that window. - entry.renewing_since = Some(now); - Action::Exchange -} - -/// Release the renewal claim after a failed exchange, so the next request -/// retries instead of coasting on a credential nobody is renewing. -/// -/// Retries are serialized only while the credential is still servable: the next -/// request claims via [`lookup`] and the rest keep serving. Once it drops below -/// [`MIN_SERVE_SECS`] there is nothing safe to serve, so every concurrent -/// request exchanges — which is the correct behaviour, not a lapse. -fn release(role_arn: &str, subject: &str) { - if let Some(entry) = cache().get_mut(&cache_key(role_arn, subject)) { - entry.renewing_since = None; - } -} - -/// Cache `creds` as the credentials for `role_arn` assumed as `subject`, -/// releasing the renewal claim. -pub(crate) fn store(role_arn: &str, subject: &str, creds: Creds) { - cache().insert( - cache_key(role_arn, subject), - Entry { - creds, - renewing_since: None, - }, - ); -} - -/// Mark a renewal as claimed at `at`. Test-only seam for the abandoned-claim -/// path, which is otherwise only reachable by a request dying mid-exchange. -#[cfg(test)] -pub(crate) fn mark_renewing_since(role_arn: &str, subject: &str, at: chrono::DateTime) { - if let Some(entry) = cache().get_mut(&cache_key(role_arn, subject)) { - entry.renewing_since = Some(at); - } -} - -/// Sanitize an OIDC subject into an AWS `RoleSessionName` (`[\w+=,.@-]{2,64}`). -/// -/// The proxy's per-connection subject (`scv1:conn:{id}`) contains `:`, which STS -/// rejects. This is only a CloudTrail attribution label — the role's trust policy -/// gates on the JWT `sub`/`aud`, not the session name — so a truncation collision -/// is cosmetic. Falls back to `s3-proxy` when sanitizing leaves fewer than the two -/// characters STS requires. -pub(crate) fn session_name(subject: &str) -> String { - let name: String = subject - .chars() - .map(|c| { - if c.is_ascii_alphanumeric() || "+=,.@-_".contains(c) { - c - } else { - '_' - } - }) - .take(64) - .collect(); - if name.len() < 2 { - "s3-proxy".to_string() - } else { - name - } -} - -/// Middleware that resolves `auth_type=oidc` backend options into real AWS -/// credentials via `AssumeRoleWithWebIdentity`. -/// -/// A no-op for connections without `auth_type=oidc` (public buckets served -/// unsigned), which is every request that never reaches the exchange below. -pub(crate) struct AwsFederation { - signer: JwtSigner, - http: H, - issuer: String, - audience: String, -} - -impl AwsFederation { - pub(crate) fn new(signer: JwtSigner, http: H, issuer: String, audience: String) -> Self { - Self { - signer, - http, - issuer, - audience, - } - } - - /// Cached credentials for `role_arn`, minting and exchanging a fresh - /// assertion when [`lookup`] says this request must. - async fn credentials(&self, role_arn: &str, subject: &str) -> Result { - match lookup(role_arn, subject) { - Action::Serve(creds) => return Ok(creds), - Action::Exchange => {} - } - let token = self - .signer - .sign(subject, &self.issuer, &self.audience, &[]) - .inspect_err(|_| release(role_arn, subject))?; - let mut exchange = AwsExchange::new(role_arn.to_string()); - // Name the assumed-role session after the subject so CloudTrail - // attributes each exchange to the originating connection. - exchange.session_name = session_name(subject); - let creds = match exchange.exchange(&self.http, &token).await { - Ok(creds) => Arc::new(creds), - Err(e) => { - release(role_arn, subject); - return Err(e.into()); - } - }; - store(role_arn, subject, creds.clone()); - Ok(creds) - } -} - -impl Middleware for AwsFederation { - async fn handle<'a>( - &'a self, - mut ctx: DispatchContext<'a>, - next: Next<'a>, - ) -> Result { - // Read everything needed out of the borrowed config first, so the - // borrow ends before the config is taken and rewritten below. - let target = ctx.bucket_config.as_deref().and_then(|config| { - (config.option("auth_type") == Some("oidc")).then(|| { - ( - config.backend_type.clone(), - config.option("oidc_role_arn").map(str::to_string), - config - .option("oidc_subject") - .unwrap_or("s3-proxy") - .to_string(), - ) - }) - }); - let Some((backend_type, role_arn, subject)) = target else { - return next.run(ctx).await; - }; - - // STS credentials only sign S3 requests. - if backend_type != "s3" { - return Err(ProxyError::ConfigError(format!( - "OIDC backend auth not yet supported for backend_type '{backend_type}'" - ))); - } - let role_arn = role_arn.ok_or_else(|| { - ProxyError::ConfigError( - "auth_type=oidc requires 'oidc_role_arn' in backend_options".into(), - ) - })?; - - let creds = self.credentials(&role_arn, &subject).await?; - - // `apply_to` sets the credential option keys and clears `skip_signature` - // so the backend request is signed. - let mut resolved = ctx - .bucket_config - .take() - .expect("bucket_config present: `target` is Some only when it is") - .into_owned(); - creds.apply_to(&mut resolved); - for key in OIDC_OPTION_KEYS { - resolved.backend_options.remove(key); - } - ctx.bucket_config = Some(Cow::Owned(resolved)); - - next.run(ctx).await - } -} diff --git a/src/lib.rs b/src/lib.rs index a8f74be..05eb846 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,14 +4,13 @@ //! / STS-disabled) → rewrite `/{account}/{product}/{key}` to an internal //! `account:product` bucket → dispatch through the multistore gateway → emit //! analytics + location telemetry → apply CORS. Isolate-shared statics (HTTP -//! client, JWKS cache, federation credentials) initialize lazily from the first +//! client, JWKS cache, OIDC provider) initialize lazily from the first //! request's config. mod analytics; mod authz; mod backend_auth; mod config; -mod federation; mod handlers; mod location; mod object_path; @@ -30,8 +29,9 @@ use multistore_cf_workers::{ collect_js_body, GatewayResponseExt, NoopCredentialRegistry, RequestParts, WorkerBackend, WorkerSubscriber, }; +use multistore_oidc_provider::backend_auth::{AwsBackendAuth, MaybeOidcAuth}; use multistore_oidc_provider::route_handler::OidcRouterExt; -use multistore_oidc_provider::{HttpExchange, OidcProviderError}; +use multistore_oidc_provider::{HttpExchange, OidcCredentialProvider, OidcProviderError}; use multistore_path_mapping::{MappedRegistry, PathMapping}; use multistore_sts::jwks::JwksCache; use multistore_sts::route_handler::StsRouterExt; @@ -71,7 +71,7 @@ fn jwks_cache() -> JwksCache { /// returns a proper S3 `ServiceUnavailable` XML error (HttpError → BackendError /// → 503) the client can parse and retry. STS normally answers in well under a /// second, so this only trips on genuine stalls — which only happen on a cold -/// isolate, since `federation` caches the credentials across requests once warm. +/// isolate, since the OIDC provider caches credentials across requests once warm. // ponytail: fixed 10s; promote to an env var if a deployment ever needs to tune it. const STS_REQUEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); @@ -108,6 +108,13 @@ impl HttpExchange for FetchHttpExchange { } } +/// Isolate-shared OIDC credential provider for backend federation. The gateway +/// (and its middleware) are rebuilt per request, but the provider — and its +/// credential cache — must persist so the proxy doesn't re-mint a JWT and re-run +/// `AssumeRoleWithWebIdentity` on every request to the same role. Initialized +/// from the first request's signing config, which is constant for the isolate. +static OIDC_PROVIDER: OnceLock> = OnceLock::new(); + #[event(fetch)] async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result { console_error_panic_hook::set_once(); @@ -265,16 +272,21 @@ async fn fetch(req: web_sys::Request, env: Env, ctx: Context) -> Result Option { - match lookup(role_arn, subject) { - Action::Serve(creds) => Some(creds.access_key_id.clone()), - Action::Exchange => None, - } -} - -fn wants_exchange(role_arn: &str, subject: &str) -> bool { - served(role_arn, subject).is_none() -} - -fn creds(expires_in_secs: i64) -> Arc { - Arc::new(BackendCredentials { - access_key_id: "ASIAEXAMPLE".into(), - secret_access_key: "secret".into(), - session_token: "token".into(), - expiration: Utc::now() + Duration::seconds(expires_in_secs), - }) -} - -// ── credential freshness ─────────────────────────────────────────── -// -// The cache is a process-wide static, so each test uses its own role ARN as a -// key rather than clearing shared state. - -#[test] -fn serves_credentials_that_are_comfortably_unexpired() { - store("arn:fresh", SUBJECT, creds(3600)); - assert_eq!( - served("arn:fresh", SUBJECT), - Some("ASIAEXAMPLE".to_string()) - ); -} - -#[test] -fn exchanges_for_an_unknown_role() { - assert!(wants_exchange("arn:never-stored", SUBJECT)); -} - -/// An expired credential must never be served, even though a renewal is -/// recorded as under way — there is nothing safe left to hand out. -#[test] -fn never_serves_an_expired_credential() { - store("arn:expired", SUBJECT, creds(-1)); - assert!(wants_exchange("arn:expired", SUBJECT)); - assert!(wants_exchange("arn:expired", SUBJECT)); -} - -// ── renewal single-flight ────────────────────────────────────────── - -/// Inside the refresh lead the credential is still valid, so exactly one caller -/// takes the exchange and everyone else keeps serving what they have. Without -/// this, a busy isolate renews with a burst of simultaneous STS calls. -#[test] -fn only_the_first_caller_in_the_refresh_lead_exchanges() { - store("arn:renewing", SUBJECT, creds(30)); - - assert!( - wants_exchange("arn:renewing", SUBJECT), - "first caller claims the renewal" - ); - for _ in 0..10 { - assert_eq!( - served("arn:renewing", SUBJECT), - Some("ASIAEXAMPLE".to_string()), - "later callers serve the still-valid credential instead of piling on" - ); - } -} - -/// A credential too close to expiry is never served, even with a renewal -/// claimed. Signing with it risks S3 rejecting an expired token mid-flight, -/// which reaches the caller as a misleading `AccessDenied` rather than a -/// retryable server-side error. -#[test] -fn never_serves_a_credential_about_to_expire() { - store("arn:about-to-expire", SUBJECT, creds(2)); - // First caller claims the renewal and leaves the near-expired credential. - assert!(wants_exchange("arn:about-to-expire", SUBJECT)); - // A concurrent caller overtakes that live claim rather than signing with 2s - // of runway — deliberate, and the reason several exchanges can run at once - // right at the edge of expiry. - assert!(wants_exchange("arn:about-to-expire", SUBJECT)); - assert!(wants_exchange("arn:about-to-expire", SUBJECT)); -} - -/// A claimant that never completes — a request cancelled mid-exchange — must not -/// pin the key as renewing until the credential expires, which would leave every -/// concurrent request to stampede at once when it does. -#[test] -fn reclaims_an_abandoned_renewal() { - store("arn:abandoned", SUBJECT, creds(30)); - mark_renewing_since("arn:abandoned", SUBJECT, Utc::now() - Duration::seconds(31)); - assert!( - wants_exchange("arn:abandoned", SUBJECT), - "a stale claim should be re-takeable" - ); -} - -/// A renewed credential is served straight from the cache again, and the -/// renewal slot is released so the *next* lead window gets its own single flight. -#[test] -fn storing_a_renewal_releases_the_slot() { - store("arn:renewed", SUBJECT, creds(30)); - assert!(wants_exchange("arn:renewed", SUBJECT)); - - store("arn:renewed", SUBJECT, creds(3600)); - assert!(served("arn:renewed", SUBJECT).is_some()); - - // Age back into the lead: a caller must be able to claim it again. - store("arn:renewed", SUBJECT, creds(30)); - assert!(wants_exchange("arn:renewed", SUBJECT)); -} - -// ── cache key identity ───────────────────────────────────────────── - -/// The role's trust policy conditions on the assertion's `sub`, so a second -/// connection pointing at the same role must not be served the first one's -/// credentials — that would succeed where STS would have refused. -#[test] -fn does_not_share_credentials_between_subjects_on_one_role() { - store("arn:shared-role", "scv1:conn:allowed", creds(3600)); - assert!(served("arn:shared-role", "scv1:conn:allowed").is_some()); - assert!(wants_exchange("arn:shared-role", "scv1:conn:other")); -} - -#[test] -fn does_not_share_credentials_between_roles_for_one_subject() { - store("arn:role-a", SUBJECT, creds(3600)); - assert!(wants_exchange("arn:role-b", SUBJECT)); -} - -// ── RoleSessionName sanitization ─────────────────────────────────── - -#[test] -fn replaces_characters_sts_rejects() { - assert_eq!(session_name("scv1:conn:abc-123"), "scv1_conn_abc-123"); -} - -#[test] -fn passes_through_an_already_valid_name() { - assert_eq!(session_name("s3-proxy"), "s3-proxy"); -} - -#[test] -fn clamps_to_the_64_character_maximum() { - assert_eq!(session_name(&"a".repeat(100)).len(), 64); -} - -#[test] -fn falls_back_when_sanitizing_leaves_too_few_characters() { - assert_eq!(session_name(":"), "s3-proxy"); -} From 32449009d9a721cef85d721600e14af21edfff3f Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 27 Jul 2026 23:31:48 -0700 Subject: [PATCH 7/7] fix(deps): bump multistore to 0.7.2 for the non-blocking credential cache 0.7.2 (developmentseed/multistore#133) is published, so the temporary `[patch.crates-io]` pin comes out and the dependencies move to the released version. Every multistore crate now resolves from crates.io with a checksum again; no git sources remain in the lock. The pinned rev and 0.7.2 are the same code, and the PR preview was A/B'd against `data.staging.source.coop` on it: 22 hung cancellations there over 320 concurrent federated requests, zero on the preview. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 25 +++++++++++++++---------- Cargo.toml | 29 +++++------------------------ 2 files changed, 20 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c1514c..f18ca45 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1034,8 +1034,9 @@ dependencies = [ [[package]] name = "multistore" -version = "0.7.1" -source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cbf0d34cce7e97df06f76b1a711d6bd655ce079dc7b32bea049d2432cfa2780d" dependencies = [ "async-trait", "base64", @@ -1062,8 +1063,9 @@ dependencies = [ [[package]] name = "multistore-cf-workers" -version = "0.7.1" -source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8d3c198c3e31ea7903e15f7af15c10b5d5b1c62f16b62a00e458872d5ae1d72" dependencies = [ "async-trait", "bytes", @@ -1088,8 +1090,9 @@ dependencies = [ [[package]] name = "multistore-oidc-provider" -version = "0.7.1" -source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "03ecb56666012a49cc8fdbaf60564337b04c1f89230bace11240604c4ddf56ef" dependencies = [ "base64", "chrono", @@ -1106,8 +1109,9 @@ dependencies = [ [[package]] name = "multistore-path-mapping" -version = "0.7.1" -source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22e0738451d593e53eafdb2e33f7c2ceef622d2d8b21a42f59e389649a2c294d" dependencies = [ "multistore", "percent-encoding", @@ -1116,8 +1120,9 @@ dependencies = [ [[package]] name = "multistore-sts" -version = "0.7.1" -source = "git+https://github.com/developmentseed/multistore?rev=11a5556c910209a91284eaeef5421f7d726ea998#11a5556c910209a91284eaeef5421f7d726ea998" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "449762c5aac6de3ca7f96430dc6af2e668b1f93c041e58aa181148bb5dd7bebb" dependencies = [ "aes-gcm", "base64", diff --git a/Cargo.toml b/Cargo.toml index a0c9353..b3e08b9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,10 +36,10 @@ path = "tests/fixtures.rs" [dependencies] # Multistore -multistore = { version = "0.7.1", features = ["azure", "gcp"] } -multistore-oidc-provider = "0.7.1" -multistore-path-mapping = "0.7.1" -multistore-sts = "0.7.1" +multistore = { version = "0.7.2", features = ["azure", "gcp"] } +multistore-oidc-provider = "0.7.2" +multistore-path-mapping = "0.7.2" +multistore-sts = "0.7.2" # Serialization serde = { version = "1", features = ["derive"] } @@ -67,7 +67,7 @@ getrandom = { version = "0.4", features = ["wasm_js"] } # ring is transitive (via object_store); this direct entry only turns the # feature on for the wasm target. ring = { version = "0.17", features = ["wasm32_unknown_unknown_js"] } -multistore-cf-workers = { version = "0.7.1", features = ["azure", "gcp"] } +multistore-cf-workers = { version = "0.7.2", features = ["azure", "gcp"] } # On wasm32-unknown-unknown reqwest selects its browser `fetch` backend by # target arch (not a cargo feature), so the default TLS/backend features are # unnecessary here. The `form` feature gates `.form()`, which the STS @@ -86,22 +86,3 @@ web-sys = { version = "0.3", features = [ ] } worker = { version = "=0.7.5", features = ["http"] } worker-macros = { version = "=0.7.5", features = ["http"] } - -# ── TEMPORARY: verify multistore 0.7.2 before it is cut ────────────── -# The credential-cache fix this branch needs (developmentseed/multistore#133) -# is merged to multistore `main` but not yet published; release-please has it -# queued as 0.7.2 in multistore#134. Point at the exact commit that release will -# be cut from, so the PR preview exercises the real upstream code on a live -# Workers deployment — the only place this failure mode is observable. -# -# Pinned to a rev, not `branch = "main"`: a branch would let an unrelated merge -# silently change what the preview builds and invalidate the verification. -# -# REMOVE BEFORE MERGE — replace with `version = "0.7.2"` on the dependencies -# above once multistore#134 lands. This must not reach production. -[patch.crates-io] -multistore = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } -multistore-oidc-provider = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } -multistore-path-mapping = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } -multistore-sts = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" } -multistore-cf-workers = { git = "https://github.com/developmentseed/multistore", rev = "11a5556c910209a91284eaeef5421f7d726ea998" }