diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml index 6cbd589332..27b3cb59a1 100644 --- a/.cargo/mutants.toml +++ b/.cargo/mutants.toml @@ -63,7 +63,7 @@ exclude_re = [ # `a_builder_without_a_transport_is_refused_when_there_is_no_bundled_one`. 'stack-auth/src/error\.rs:\d+:\d+: replace RequestError::is_no_transport -> bool with false$', 'stack-auth/src/auto_strategy\.rs:150:9: replace AutoStrategy::detect_inner -> Result with Ok\(Default::default\(\)\)$', - 'stack-auth/src/token_store\.rs:(261|269):9: replace >::(load|save)', + 'stack-auth/src/token_store\.rs:(273|281):9: replace >::(load|save)', # stack-auth — equivalent. 'stack-auth/src/(access_key|oidc)_refresher\.rs:\d+:\d+: replace )?>::try_credential -> Option with Some\(Default::default\(\)\)$', 'stack-auth/src/error\.rs:\d+:\d+: replace AuthErrorKind::payload -> serde_json::Map with Default::default\(\)$', diff --git a/.changeset/auth-authorise-expiry-cap-and-body-wiping.md b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md new file mode 100644 index 0000000000..d899f3237e --- /dev/null +++ b/.changeset/auth-authorise-expiry-cap-and-body-wiping.md @@ -0,0 +1,20 @@ +--- +'@cipherstash/auth': patch +--- + +Cap the access-token expiry from CTS's `/api/authorise` at 30 days from now, +as a relative `expires_in` already is. A token loaded from a token store, or +passed in when the client is built, that claims to last longer than that is +refreshed before it is used. A bad `expiry` value can no longer make a token look +valid for ever, which kept the client sending a dead token until the process +restarted. + +Token JSON saved by `InMemoryTokenStore`, callback token stores (including the +`loadToken`/`saveToken` callbacks of the Node and WASM builds), and the request +bodies sent to CTS (access keys, OIDC JWTs, refresh tokens), are now +serialised into a buffer that wipes itself as it grows, so no partial copies +are left in memory. + +A response body from CTS, which carries the new access token, is now taken over +from the HTTP client's buffer where that buffer is not shared, rather than always +copied, so it is wiped with the response instead of being freed unwiped. diff --git a/Cargo.lock b/Cargo.lock index 5548762390..d95f26510e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3244,6 +3244,7 @@ dependencies = [ "base64 0.22.1", "bytes", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "mocktail", @@ -3305,6 +3306,7 @@ dependencies = [ "serde-wasm-bindgen", "serde_json", "stack-auth", + "stack-profile", "wasm-bindgen", "wasm-bindgen-futures", "wasm-bindgen-test", diff --git a/languages/golang/auth/guest/Cargo.lock b/languages/golang/auth/guest/Cargo.lock index ba6588228c..6e631a8cff 100644 --- a/languages/golang/auth/guest/Cargo.lock +++ b/languages/golang/auth/guest/Cargo.lock @@ -1785,6 +1785,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/languages/golang/encrypt/guest/Cargo.lock b/languages/golang/encrypt/guest/Cargo.lock index 46b22fa357..7670c5dfa1 100644 --- a/languages/golang/encrypt/guest/Cargo.lock +++ b/languages/golang/encrypt/guest/Cargo.lock @@ -2002,6 +2002,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/languages/typescript/packages/auth/src/lib.rs b/languages/typescript/packages/auth/src/lib.rs index 3e5dff27bf..c34fca1f1b 100644 --- a/languages/typescript/packages/auth/src/lib.rs +++ b/languages/typescript/packages/auth/src/lib.rs @@ -296,12 +296,16 @@ impl TokenStore for NapiTokenStore { } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token).map(Zeroizing::new) else { + // Serialise into a `SecretBuf`, which wipes every buffer it + // outgrows, then move the bytes into the `String` the JS callback + // takes. + let Ok(mut json) = stack_profile::SecretBuf::json_string(token) else { return; }; + let json = std::mem::take(&mut *json); let (tx, rx) = oneshot::channel::>(); let status = self.save.call_with_return_value( - json.to_string(), + json, ThreadsafeFunctionCallMode::NonBlocking, move |promise: Promise<()>| { let _ = tx.send(promise); diff --git a/languages/typescript/packages/stack-auth-wasm/Cargo.toml b/languages/typescript/packages/stack-auth-wasm/Cargo.toml index edeed0d6de..329808f1a4 100644 --- a/languages/typescript/packages/stack-auth-wasm/Cargo.toml +++ b/languages/typescript/packages/stack-auth-wasm/Cargo.toml @@ -14,6 +14,8 @@ crate-type = ["cdylib", "rlib"] [dependencies] stack-auth = { workspace = true, features = ["http"] } +# `SecretBuf`, to serialise saved tokens without leaving unwiped copies. +stack-profile = { workspace = true } cts-common = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } diff --git a/languages/typescript/packages/stack-auth-wasm/src/lib.rs b/languages/typescript/packages/stack-auth-wasm/src/lib.rs index 68e4d82727..5153b05f99 100644 --- a/languages/typescript/packages/stack-auth-wasm/src/lib.rs +++ b/languages/typescript/packages/stack-auth-wasm/src/lib.rs @@ -154,7 +154,9 @@ impl TokenStore for JsTokenStore { } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token).map(Zeroizing::new) else { + // Serialise into a `SecretBuf`, which wipes every buffer it + // outgrows, and the result when it is dropped. + let Ok(json) = stack_profile::SecretBuf::json_string(token) else { return; }; let promise = match self.save.call1(&JsValue::NULL, &JsValue::from_str(&json)) { diff --git a/packages/stack-auth/Cargo.toml b/packages/stack-auth/Cargo.toml index 3fae08b092..01078bcccb 100644 --- a/packages/stack-auth/Cargo.toml +++ b/packages/stack-auth/Cargo.toml @@ -28,6 +28,9 @@ serde_json = { workspace = true } # `.form()` does (it uses this crate), so the bundled transport and a host's # own transport send byte-identical requests. serde_urlencoded = "0.7" +# `serde_urlencoded` only writes into a `form_urlencoded::Serializer`, which +# `transport::form_body` builds itself so it can size the target up front. +form_urlencoded = "1.2" # `OidcFederationStrategy` keys its per-user token cache on the SHA-256 of # the provider JWT, and stamps that digest on the tokens it persists. sha2 = "0.10" diff --git a/packages/stack-auth/fuzz/Cargo.lock b/packages/stack-auth/fuzz/Cargo.lock index 699f876fac..15e6b787f5 100644 --- a/packages/stack-auth/fuzz/Cargo.lock +++ b/packages/stack-auth/fuzz/Cargo.lock @@ -2600,6 +2600,7 @@ dependencies = [ "base64", "bytes", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/packages/stack-auth/src/access_key_refresher.rs b/packages/stack-auth/src/access_key_refresher.rs index 5327ceefc5..f33880515d 100644 --- a/packages/stack-auth/src/access_key_refresher.rs +++ b/packages/stack-auth/src/access_key_refresher.rs @@ -78,8 +78,9 @@ impl Refresher for AccessKeyRefresher { let auth_resp: AuthoriseResponse = resp.json()?; // The response → Token mapping (including the absolute-epoch `expiry` - // handling that CIP-3233 fixed) lives on `From`. - Ok(auth_resp.into()) + // handling that CIP-3233 fixed, and the 30-day cap) lives on + // `AuthoriseResponse::into_token`. + Ok(auth_resp.into_token(crate::token::now_unix_secs())) } } @@ -209,6 +210,35 @@ mod tests { ); } + /// A CTS that answers with an absurd absolute `expiry` (buggy or hostile) + /// must not mint a token that never expires: `AutoRefresh` would then keep + /// sending a dead token until the process restarted. The expiry is capped + /// at 30 days from now, the same cap a relative `expires_in` gets. + #[tokio::test] + async fn access_key_expiry_is_capped_at_thirty_days() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/api/authorise"); + then.json(serde_json::json!({ "accessToken": "tok", "expiry": u64::MAX })); + }); + let server = start_server(mocks).await; + + let refresher = AccessKeyRefresher::new( + SecretToken::new("CSAKid.secret"), + server.url(""), + None, + default_transport(), + ); + let token = refresher.refresh(&()).await.unwrap(); + + assert!( + token.expires_in() <= crate::token::MAX_TOKEN_LIFETIME_SECS, + "expiry must be capped at 30 days; got expires_in = {}", + token.expires_in() + ); + assert!(!token.is_expired(), "a capped token is still a live token"); + } + // ---- Initial auth tests ---- #[tokio::test] diff --git a/packages/stack-auth/src/authorize_dto.rs b/packages/stack-auth/src/authorize_dto.rs index 96719a04e5..e40799e2f5 100644 --- a/packages/stack-auth/src/authorize_dto.rs +++ b/packages/stack-auth/src/authorize_dto.rs @@ -7,6 +7,7 @@ //! the wire contract lives here in one place. The request bodies differ //! (different credential fields) and stay private to each exchange. +use crate::token::cap_expires_at; use crate::{SecretToken, Token}; /// Success response from `POST /api/authorise`. @@ -17,24 +18,27 @@ pub(crate) struct AuthoriseResponse { pub(crate) expiry: u64, } -/// A `/api/authorise` success response *is* a complete CTS service token. Both -/// the access-key and OIDC federation flows hit the same endpoint and mint the -/// same kind of token, so the response → [`Token`] mapping lives here once -/// rather than being duplicated in each refresher — duplication is exactly how -/// the CIP-3233 expiry bug survived in the OIDC refresher after being fixed for -/// access keys. -impl From for Token { - fn from(resp: AuthoriseResponse) -> Self { +impl AuthoriseResponse { + /// A `/api/authorise` success response *is* a complete CTS service token. + /// Both the access-key and OIDC federation flows hit the same endpoint and + /// mint the same kind of token, so the response → [`Token`] mapping lives + /// here once rather than being duplicated in each refresher — duplication + /// is exactly how the CIP-3233 expiry bug survived in the OIDC refresher + /// after being fixed for access keys. + /// + /// `now` is the current Unix time, used only to cap the server's expiry. + pub(crate) fn into_token(self, now: u64) -> Token { Token { - access_token: resp.access_token, + access_token: self.access_token, token_type: "Bearer".to_string(), // CTS `/api/authorise` returns `expiry` as an ABSOLUTE Unix epoch (it is // the JWT `exp` claim), NOT a relative duration. Adding `now` here would // push the local expiry decades into the future, so `AutoRefresh` would // never consider the token expired and never refresh it — the token would // then silently die at its real (~15 min) `exp` and every request would - // fail until the process restarted. Use the value as-is. See CIP-3233. - expires_at: resp.expiry, + // fail until the process restarted. Use the value as-is (see CIP-3233), + // capped at 30 days from now so an absurd value cannot do the same. + expires_at: cap_expires_at(now, self.expiry), // `/api/authorise` issues no refresh token and carries no region / client // / device metadata; those are populated only by the OAuth device flow. refresh_token: None, @@ -49,6 +53,7 @@ impl From for Token { #[cfg(test)] mod tests { use super::*; + use crate::token::MAX_TOKEN_LIFETIME_SECS; /// The single source of truth for the response → `Token` mapping: CTS /// `expiry` is an ABSOLUTE Unix epoch and must be used verbatim. Pre-fix the @@ -61,7 +66,7 @@ mod tests { expiry: 1_900_000_000, // an absolute Unix epoch, not a duration }; - let token = Token::from(resp); + let token = resp.into_token(1_899_999_100); assert_eq!( token.expires_at(), @@ -75,4 +80,34 @@ mod tests { "/api/authorise issues no refresh token" ); } + + /// An `expiry` more than 30 days after `now` is cut back to `now` plus 30 + /// days, however large it is. + #[test] + fn authorise_response_caps_expiry_at_thirty_days_from_now() { + let now = 1_900_000_000; + for expiry in [now + MAX_TOKEN_LIFETIME_SECS + 1, u64::MAX] { + let resp = AuthoriseResponse { + access_token: SecretToken::new("cts-token"), + expiry, + }; + assert_eq!( + resp.into_token(now).expires_at(), + now + MAX_TOKEN_LIFETIME_SECS + ); + } + } + + /// An `expiry` exactly 30 days out is kept, and so is one already past. + #[test] + fn authorise_response_keeps_an_expiry_within_the_cap() { + let now = 1_900_000_000; + for expiry in [now + MAX_TOKEN_LIFETIME_SECS, now - 1] { + let resp = AuthoriseResponse { + access_token: SecretToken::new("cts-token"), + expiry, + }; + assert_eq!(resp.into_token(now).expires_at(), expiry); + } + } } diff --git a/packages/stack-auth/src/auto_refresh.rs b/packages/stack-auth/src/auto_refresh.rs index a459eb3efe..a5bd97d379 100644 --- a/packages/stack-auth/src/auto_refresh.rs +++ b/packages/stack-auth/src/auto_refresh.rs @@ -256,8 +256,37 @@ impl AutoRefresh { } } +/// A freshly refreshed `token` with its expiry capped at 30 days after `now`. +/// +/// A refreshed token was issued just now, so 30 days from now is the most it +/// can legitimately claim. Tokens minted since `/api/authorise` and +/// `expires_in` were capped satisfy this already; the cap here keeps the +/// engine safe from a [`Refresher`] that does not. +fn capped(mut token: Token, now: u64) -> Token { + token.expires_at = crate::token::cap_expires_at(now, token.expires_at); + token +} + +/// A `token` whose issue time is unknown (the one the engine is built with, +/// or one loaded from a [`TokenStore`]), treated as already expired if it +/// claims to last more than 30 days from `now`. +/// +/// A store can hand back a token saved before the `/api/authorise` cap +/// existed, or one written by whoever can write to the store (a cookie, a KV +/// entry). Capping it at `now + 30 days` would not do: every new engine would +/// cap it afresh, so a process restarting more often than every 30 days would +/// use an `expires_at` of `u64::MAX` for ever. No legitimate token claims more +/// than the cap, so one that does is refreshed before use. +fn distrust_overlong(mut token: Token, now: u64) -> Token { + if crate::token::cap_expires_at(now, token.expires_at) < token.expires_at { + token.expires_at = now; + } + token +} + impl AutoRefresh { fn new(refresher: R, store: S, token: Option, clock: SharedClock) -> Self { + let token = token.map(|t| distrust_overlong(t, clock.now_unix_secs())); Self { shared: Arc::new(RefreshState { refresher, @@ -276,6 +305,12 @@ impl AutoRefresh { Self::new(refresher, store, None, system_clock()) } + // Its only callers are in `expiry_crossing_regression`, which needs `http`. + #[cfg(all(test, feature = "http"))] + pub(crate) fn with_store_and_clock(refresher: R, store: S, clock: SharedClock) -> Self { + Self::new(refresher, store, None, clock) + } + pub(crate) fn refresher(&self) -> &R { &self.shared.refresher } @@ -308,7 +343,7 @@ impl RefreshState { let loaded = self.store.load().await; state = self.state.clone().lock_owned().await; if state.token.is_none() { - state.token = loaded; + state.token = loaded.map(|t| distrust_overlong(t, self.clock.now_unix_secs())); } } @@ -411,7 +446,7 @@ impl RefreshState { /// [`save_refreshed_token`](Self::save_refreshed_token). fn install_refreshed_token(&self, state: &mut State, new_token: Token) -> ServiceToken { let service_token = ServiceToken::new(new_token.access_token().clone()); - state.token = Some(new_token); + state.token = Some(capped(new_token, self.clock.now_unix_secs())); // A success proves whatever previously refused us has changed its mind. state.denial = None; state.last_refresh_error = None; @@ -1935,6 +1970,144 @@ mod expiry_crossing_regression { } } + /// A refresher whose `refresh` returns at once, counting its calls. + fn ready_refresher(clock: &TestClock, calls: &Arc) -> GatedRefresher { + let gate = Arc::new(Notify::new()); + // A stored permit: the first `refresh` passes the gate without waiting. + gate.notify_one(); + GatedRefresher { + started: Arc::new(Notify::new()), + gate, + calls: Arc::clone(calls), + refreshed_expires_at: clock.now() + 3600, + } + } + + use crate::token::MAX_TOKEN_LIFETIME_SECS; + + /// A token that reaches the engine claiming to expire at `u64::MAX` (one + /// saved before the `/api/authorise` cap existed, or written into a + /// caller-writable store) is refreshed before its first use, not used for + /// ever. + #[tokio::test] + async fn an_initial_token_claiming_to_never_expire_is_refreshed_before_use() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let strategy = AutoRefresh::with_token_and_clock( + ready_refresher(&clock, &calls), + make_token("forever-token", u64::MAX), + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// The same applies to a token loaded from a `TokenStore`. + #[tokio::test] + async fn a_stored_token_claiming_to_never_expire_is_refreshed_before_use() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = crate::InMemoryTokenStore::new(); + store.save(&make_token("forever-token", u64::MAX)).await; + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + store, + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// A stored token within the cap is used as is. + #[tokio::test] + async fn a_stored_token_within_thirty_days_is_used_without_refreshing() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = crate::InMemoryTokenStore::new(); + store + .save(&make_token( + "stored-token", + clock.now() + MAX_TOKEN_LIFETIME_SECS, + )) + .await; + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + store, + clock.shared(), + ); + + assert_eq!(strategy.get_token().await.unwrap().as_str(), "stored-token"); + assert_eq!(calls.load(Ordering::SeqCst), 0); + } + + /// A freshly refreshed token is capped at 30 days rather than distrusted: + /// it was issued just now. + #[tokio::test] + async fn a_refreshed_token_claiming_to_never_expire_is_capped_at_thirty_days() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let mut refresher = ready_refresher(&clock, &calls); + refresher.refreshed_expires_at = u64::MAX; + let strategy = AutoRefresh::with_token_and_clock( + refresher, + make_token("expired-token", clock.now()), + clock.shared(), + ); + + assert_eq!( + strategy.get_token().await.unwrap().as_str(), + "refreshed-token" + ); + clock.advance(MAX_TOKEN_LIFETIME_SECS - 1_000); + // Bounded: a second refresh would wait on the gate for ever, and a + // cap that is too short must fail here, not hang the suite. + let token = tokio::time::timeout(std::time::Duration::from_secs(5), strategy.get_token()) + .await + .expect("the refreshed token stays usable within the cap, with no second refresh") + .unwrap(); + assert_eq!(token.as_str(), "refreshed-token"); + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + /// A stored token that claims to outlive the 30-day cap is not trusted + /// again on every load. Re-capping it from each load's "now" would let a + /// process that restarts more often than every 30 days use it for ever, + /// because each fresh engine would see another 30 days left. + #[tokio::test] + async fn restarts_do_not_renew_a_stored_token_that_claims_to_never_expire() { + let clock = TestClock::new(1_000_000); + let calls = Arc::new(AtomicUsize::new(0)); + let store = Arc::new(crate::InMemoryTokenStore::new()); + store.save(&make_token("forever-token", u64::MAX)).await; + + // Three short-lived processes, 20 days apart, sharing the store. + for restart in 0..3 { + let calls_before = calls.load(Ordering::SeqCst); + let strategy = AutoRefresh::with_store_and_clock( + ready_refresher(&clock, &calls), + Arc::clone(&store), + clock.shared(), + ); + assert_ne!( + strategy.get_token().await.unwrap().as_str(), + "forever-token", + "restart {restart}: a token claiming to outlive the cap must be refreshed" + ); + assert_eq!(calls.load(Ordering::SeqCst), calls_before + 1); + // Put the over-long token back, as a stale or hostile writer would. + store.save(&make_token("forever-token", u64::MAX)).await; + clock.advance(20 * 24 * 60 * 60); + } + } + #[tokio::test(start_paused = true)] async fn initiating_caller_receives_the_refreshed_token() { for elapsed in [0, 9, 10, 11] { @@ -2113,6 +2286,16 @@ mod expiry_crossing_regression { /// `notified().await`) exists to make correct. #[tokio::test] async fn waiters_get_expired_when_in_flight_refresh_fails() { + // Bounded, so a broken engine fails here instead of hanging the suite. + tokio::time::timeout( + std::time::Duration::from_secs(10), + waiters_get_expired_when_in_flight_refresh_fails_unbounded(), + ) + .await + .expect("the test must finish"); + } + + async fn waiters_get_expired_when_in_flight_refresh_fails_unbounded() { let clock = TestClock::new(1_000_000); let started = Arc::new(Notify::new()); let gate = Arc::new(Notify::new()); @@ -2309,6 +2492,16 @@ mod expiry_crossing_regression { /// every caller except the one that happened to perform it. #[tokio::test] async fn waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal() { + // Bounded, so a broken engine fails here instead of hanging the suite. + tokio::time::timeout( + std::time::Duration::from_secs(10), + waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal_unbounded(), + ) + .await + .expect("the test must finish"); + } + + async fn waiters_get_the_issuers_error_even_when_it_is_not_an_account_refusal_unbounded() { let clock = TestClock::new(1_000_000); let started = Arc::new(Notify::new()); let gate = Arc::new(Notify::new()); diff --git a/packages/stack-auth/src/auto_refresh/tests/cancellation.rs b/packages/stack-auth/src/auto_refresh/tests/cancellation.rs index 821a034d18..61fae44513 100644 --- a/packages/stack-auth/src/auto_refresh/tests/cancellation.rs +++ b/packages/stack-auth/src/auto_refresh/tests/cancellation.rs @@ -120,7 +120,18 @@ enum Phase { Install, } +/// [`cancelled_at_unbounded`], bounded (virtual time) so a broken engine fails +/// instead of hanging. async fn cancelled_at(phase: Phase, usable: bool, initial: bool) { + tokio::time::timeout( + Duration::from_secs(60), + cancelled_at_unbounded(phase, usable, initial), + ) + .await + .expect("each phase must finish"); +} + +async fn cancelled_at_unbounded(phase: Phase, usable: bool, initial: bool) { let clock = TestClock::new(1_000); let issuer = Arc::new(Issuer::new(initial)); let store = Arc::new(GatedStore { diff --git a/packages/stack-auth/src/device_client.rs b/packages/stack-auth/src/device_client.rs index 7490c1cf66..e131b7e738 100644 --- a/packages/stack-auth/src/device_client.rs +++ b/packages/stack-auth/src/device_client.rs @@ -120,9 +120,7 @@ pub async fn bind_client_device(store: &ProfileStore) -> Result<(), DeviceClient let url = zerokms_url.join(CreateClientRequest::ENDPOINT)?; // Provisioning is native-only, so it always uses the bundled transport. - let body = zeroize::Zeroizing::new( - serde_json::to_vec(&request).map_err(|e| RequestError(Box::new(e)))?, - ); + let body = transport::json_body(&request)?; let response = transport::post( &transport::share(ReqwestTransport::default()), url, diff --git a/packages/stack-auth/src/oidc_federation_strategy.rs b/packages/stack-auth/src/oidc_federation_strategy.rs index 177789e53c..fba8131c0c 100644 --- a/packages/stack-auth/src/oidc_federation_strategy.rs +++ b/packages/stack-auth/src/oidc_federation_strategy.rs @@ -1707,7 +1707,13 @@ mod tests { ); gate.add_permits(1); - let token = second.await.expect("task").expect("the survivor completes"); + // Bounded, so a broken exchange (one that never takes the permit) + // fails here instead of hanging the suite. + let token = tokio::time::timeout(std::time::Duration::from_secs(10), second) + .await + .expect("the survivor finishes") + .expect("task") + .expect("the survivor completes"); assert_eq!(subject(&token), "CS|header.payload.signature"); assert_eq!( strategy.cached_jwts(), diff --git a/packages/stack-auth/src/oidc_refresher.rs b/packages/stack-auth/src/oidc_refresher.rs index 779e89d486..340f1b39cc 100644 --- a/packages/stack-auth/src/oidc_refresher.rs +++ b/packages/stack-auth/src/oidc_refresher.rs @@ -197,8 +197,9 @@ impl OidcFederation { let auth_resp: AuthoriseResponse = resp.json()?; // The response → Token mapping (including the absolute-epoch `expiry` - // handling that CIP-3233 fixed) lives on `From`. - Ok(auth_resp.into()) + // handling that CIP-3233 fixed, and the 30-day cap) lives on + // `AuthoriseResponse::into_token`. + Ok(auth_resp.into_token(crate::token::now_unix_secs())) } } @@ -401,6 +402,30 @@ mod tests { ); } + /// The federation step caps an absurd absolute `expiry` at 30 days from + /// now, exactly as the access-key path does: both go through one mapping. + #[tokio::test] + async fn oidc_expiry_is_capped_at_thirty_days() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/api/authorise"); + then.json(serde_json::json!({ "accessToken": "tok", "expiry": u64::MAX })); + }); + let server = start_server(mocks).await; + + let token = federation(&server) + .federate(&SecretToken::new("jwt-0")) + .await + .unwrap(); + + assert!( + token.expires_in() <= crate::token::MAX_TOKEN_LIFETIME_SECS, + "expiry must be capped at 30 days; got expires_in = {}", + token.expires_in() + ); + assert!(!token.is_expired(), "a capped token is still a live token"); + } + /// The federation step sends exactly the JWT it was given, and nothing it /// mints is stamped — stamping is the refresher's job, so a token that /// came straight from the endpoint is not mistaken for a cached one. diff --git a/packages/stack-auth/src/test_support.rs b/packages/stack-auth/src/test_support.rs index ec6b92ba31..80303b24c7 100644 --- a/packages/stack-auth/src/test_support.rs +++ b/packages/stack-auth/src/test_support.rs @@ -10,14 +10,15 @@ use cts_common::Crn; use crate::{SecretToken, Token}; -/// A [`Token`] with the given raw access-token string and a far-future expiry, -/// so it reads as valid. The access token need not be a JWT — pass any string to -/// exercise the not-a-JWT paths. +/// A [`Token`] with the given raw access-token string, expiring an hour from +/// now, so it reads as valid. (Not `u64::MAX`: the engine treats a token that +/// claims to outlive the 30-day cap as already expired.) The access token +/// need not be a JWT — pass any string to exercise the not-a-JWT paths. pub(crate) fn raw_token(access_token: &str) -> Token { Token { access_token: SecretToken::new(access_token), token_type: "Bearer".to_string(), - expires_at: u64::MAX, + expires_at: crate::token::now_unix_secs() + 3600, refresh_token: Some(SecretToken::new("refresh-token")), region: None, client_id: None, diff --git a/packages/stack-auth/src/token.rs b/packages/stack-auth/src/token.rs index 02f98b5f8a..d6d128d648 100644 --- a/packages/stack-auth/src/token.rs +++ b/packages/stack-auth/src/token.rs @@ -40,12 +40,26 @@ pub(crate) fn expires_at_after(now: u64, expires_in: u64) -> u64 { now.saturating_add(expires_in.min(MAX_TOKEN_LIFETIME_SECS)) } +/// A server-supplied ABSOLUTE expiry (seconds since the Unix epoch), capped +/// at [`MAX_TOKEN_LIFETIME_SECS`] after `now`. +/// +/// The counterpart of [`expires_at_after`] for servers that send an absolute +/// `exp` rather than a lifetime, such as CTS `/api/authorise`. No arithmetic +/// on the server's value is involved, so it cannot overflow, but it is still +/// unbounded: an `expiry` of `u64::MAX` would make the token look valid for +/// ever, so [`AutoRefresh`](crate::auto_refresh::AutoRefresh) would keep +/// sending it after the server stopped accepting it. Capping it means the +/// worst a bad value can do is make the token refresh within 30 days. +pub(crate) fn cap_expires_at(now: u64, expires_at: u64) -> u64 { + expires_at.min(now.saturating_add(MAX_TOKEN_LIFETIME_SECS)) +} + /// The current Unix time in whole seconds, from the system wall clock. /// /// Delegates to [`SystemClock`](crate::clock::SystemClock) so the crate has a /// single definition of "now"; the `*_at` methods take an explicit `now` for /// tests that drive a [`Clock`](crate::clock::Clock). -fn now_unix_secs() -> u64 { +pub(crate) fn now_unix_secs() -> u64 { use crate::clock::{Clock, SystemClock}; SystemClock.now_unix_secs() } diff --git a/packages/stack-auth/src/token_store.rs b/packages/stack-auth/src/token_store.rs index 99c2547dfd..ad9933becf 100644 --- a/packages/stack-auth/src/token_store.rs +++ b/packages/stack-auth/src/token_store.rs @@ -55,6 +55,7 @@ let strategy = AccessKeyStrategy::builder(crn, key) use std::future::Future; use std::sync::Arc; +use stack_profile::SecretBuf; use tokio::sync::Mutex; use zeroize::Zeroizing; @@ -137,12 +138,15 @@ impl TokenStore for NoStore { /// /// Internally stores the JSON-serialised form of the token wrapped in /// [`Zeroizing`] so the buffer is wiped on overwrite and on store drop. The -/// [`SecretToken`](crate::SecretToken) wrapped inside [`Token`] is +/// JSON is written into a [`SecretBuf`], so no partial copy is left behind +/// while it is serialised either. +/// +/// The [`SecretToken`](crate::SecretToken) wrapped inside [`Token`] is /// [`ZeroizeOnDrop`](zeroize::ZeroizeOnDrop), so we deliberately don't clone /// the in-memory `Token` value — round-tripping through serde gives us a /// fresh `SecretToken` on each `load` without violating that invariant. pub struct InMemoryTokenStore { - state: Mutex>>, + state: Mutex>>>, } impl InMemoryTokenStore { @@ -164,19 +168,27 @@ impl TokenStore for InMemoryTokenStore { async fn load(&self) -> Option { let guard = self.state.lock().await; let json = guard.as_ref()?; - serde_json::from_str(json).ok() + serde_json::from_slice(json).ok() } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Ok(json) = SecretBuf::json(token) else { tracing::warn!("InMemoryTokenStore: failed to serialise token"); return; }; let mut guard = self.state.lock().await; - *guard = Some(Zeroizing::new(json)); + *guard = Some(json); } } +/// `token` as an owned JSON `String` for a `save` closure, moved rather than +/// copied out of the buffer it was serialised into. +fn token_json_string(token: &Token) -> Option { + SecretBuf::json_string(token) + .ok() + .map(|mut json| std::mem::take(&mut *json)) +} + /// [`TokenStore`] backed by user-supplied `load` and `save` async closures. /// /// This is the *persistence layer* primitive — it plugs into an existing @@ -241,7 +253,7 @@ where } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Some(json) = token_json_string(token) else { tracing::warn!("TokenStoreFn: failed to serialise token"); return; }; @@ -266,7 +278,7 @@ where } async fn save(&self, token: &Token) { - let Ok(json) = serde_json::to_string(token) else { + let Some(json) = token_json_string(token) else { tracing::warn!("TokenStoreFn: failed to serialise token"); return; }; diff --git a/packages/stack-auth/src/transport.rs b/packages/stack-auth/src/transport.rs index d0dd7fead3..fd51ba8160 100644 --- a/packages/stack-auth/src/transport.rs +++ b/packages/stack-auth/src/transport.rs @@ -25,6 +25,7 @@ use std::fmt; use std::future::Future; use std::sync::{Arc, OnceLock}; +use stack_profile::SecretBuf; use url::Url; use zeroize::Zeroizing; @@ -343,27 +344,78 @@ pub(crate) async fn post_json( url: Url, body: &B, ) -> Result { - let body = Zeroizing::new(serde_json::to_vec(body).map_err(|e| RequestError(Box::new(e)))?); + let body = json_body(body)?; post(transport, url, "application/json", Vec::new(), body).await } +/// `body` as JSON in a [`SecretBuf`]. +/// +/// Request bodies carry access keys, OIDC JWTs and refresh tokens, and a +/// `SecretBuf` leaves no unwiped partial copy of them on the heap. +pub(crate) fn json_body(body: &B) -> Result>, RequestError> { + SecretBuf::json(body).map_err(|e| RequestError(Box::new(e))) +} + +/// An [`io::Write`](std::io::Write) that counts the bytes written to it and +/// keeps none of them. +struct ByteCount(usize); + +impl std::io::Write for ByteCount { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0 = self.0.saturating_add(buf.len()); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +/// `body` form-encoded, into a buffer that never grows. +/// +/// `serde_urlencoded` can only write into a `String`, which would free each +/// outgrown buffer unwiped, the same leak [`json_body`] avoids. So the +/// `String` is given its worst-case size up front. The JSON form of the body +/// holds every key and value character at least once, and form-encoding turns +/// a byte into at most three (`%XX`), with one separator per key and value +/// that JSON's own quotes more than pay for. Three times the JSON length is +/// therefore always enough. The JSON is only measured, never stored, so it +/// leaves no copy either. +fn form_body(body: &B) -> Result>, RequestError> { + let mut json_len = ByteCount(0); + serde_json::to_writer(&mut json_len, body).map_err(|e| RequestError(Box::new(e)))?; + let bound = json_len.0.saturating_mul(3); + let mut form = Zeroizing::new(String::new()); + form.try_reserve_exact(bound) + .map_err(|e| RequestError(Box::new(e)))?; + let capacity = form.capacity(); + { + let mut encoder = form_urlencoded::Serializer::new(&mut *form); + let _ = body + .serialize(serde_urlencoded::Serializer::new(&mut encoder)) + .map_err(|e| RequestError(Box::new(e)))?; + } + debug_assert_eq!( + form.capacity(), + capacity, + "the form body outgrew its reserved size, leaving an unwiped copy" + ); + // `take` moves the bytes out, leaving an empty `String` behind to drop. + Ok(Zeroizing::new(std::mem::take(&mut *form).into_bytes())) +} + /// `POST` a form (`application/x-www-form-urlencoded`) body. pub(crate) async fn post_form( transport: &SharedTransport, url: Url, body: &B, ) -> Result { - let body = Zeroizing::new( - serde_urlencoded::to_string(body) - .map_err(|e| RequestError(Box::new(e)))? - .into_bytes(), - ); post( transport, url, "application/x-www-form-urlencoded", Vec::new(), - body, + form_body(body)?, ) .await } @@ -464,11 +516,20 @@ impl HttpTransport for ReqwestTransport { ) }) .collect(); - let body = response.bytes().await?.to_vec(); + let body = response_body(response.bytes().await?); Ok(HttpResponse::new(status, headers, body)) } } +/// The bytes of a response body, moved out of reqwest's buffer rather than +/// copied when nothing else shares it. A minted token arrives in this body, +/// and [`HttpResponse`] wipes its own copy on drop; a copy would free +/// reqwest's buffer, token and all, unwiped. +#[cfg(feature = "http")] +fn response_body(bytes: bytes::Bytes) -> Vec { + Vec::from(bytes) +} + /// The bundled transport, boxed for the strategies. #[cfg(feature = "http")] pub(crate) fn default_transport() -> SharedTransport { @@ -741,6 +802,55 @@ mod tests { } } + /// A form body whose every byte needs percent-encoding is the worst case + /// for the buffer [`form_body`] reserves up front: each byte becomes three. + /// The bytes must match `serde_urlencoded` exactly, and (checked by the + /// `debug_assert` inside) the buffer must never have had to grow, since a + /// grown `String` frees its old allocation unwiped. + #[test] + fn form_body_encodes_like_serde_urlencoded_without_growing() { + let secret: String = "é&=+ /\u{1F600}%".repeat(500); + let body = [ + ("grant_type", "refresh_token"), + ("refresh_token", secret.as_str()), + ("", ""), + ]; + + let encoded = form_body(&body).unwrap(); + + assert_eq!( + encoded.as_slice(), + serde_urlencoded::to_string(body).unwrap().as_bytes() + ); + } + + /// `post_json` sends exactly what `serde_json` writes, however long. + #[tokio::test] + async fn post_json_sends_the_serialised_body() { + let stub = Arc::new(Stub::replying(200, "{}")); + let transport: SharedTransport = stub.clone(); + let body = serde_json::json!({ "accessKey": "k".repeat(20_000) }); + + post_json(&transport, base_url(), &body).await.unwrap(); + + let (_, _, _, sent) = seen(&stub); + assert_eq!(sent, serde_json::to_vec(&body).unwrap()); + } + + /// The response body takes over reqwest's allocation instead of copying + /// it, so the token in it is wiped with [`HttpResponse`]. + #[test] + #[cfg(feature = "http")] + fn response_body_takes_over_the_buffer_it_is_given() { + let bytes = bytes::Bytes::from(br#"{"accessToken":"secret"}"#.to_vec()); + let ptr = bytes.as_ptr(); + + let body = response_body(bytes); + + assert_eq!(body, br#"{"accessToken":"secret"}"#); + assert_eq!(body.as_ptr(), ptr, "the buffer must be moved, not copied"); + } + #[test] fn the_user_agent_names_the_crate_version_and_platform() { assert_eq!(user_agent(), expected_user_agent()); diff --git a/packages/stack-encrypt/fuzz/Cargo.lock b/packages/stack-encrypt/fuzz/Cargo.lock index e2f5a569cc..ae0c7f1786 100644 --- a/packages/stack-encrypt/fuzz/Cargo.lock +++ b/packages/stack-encrypt/fuzz/Cargo.lock @@ -1957,6 +1957,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/packages/stack-kms/fuzz/Cargo.lock b/packages/stack-kms/fuzz/Cargo.lock index 8c5b3705a6..aada3bdff4 100644 --- a/packages/stack-kms/fuzz/Cargo.lock +++ b/packages/stack-kms/fuzz/Cargo.lock @@ -1942,6 +1942,7 @@ dependencies = [ "aquamarine", "base64", "cts-common", + "form_urlencoded", "jsonwebtoken", "miette", "open", diff --git a/packages/stack-profile/src/lib.rs b/packages/stack-profile/src/lib.rs index bbf6c98d5a..fb719a8e35 100644 --- a/packages/stack-profile/src/lib.rs +++ b/packages/stack-profile/src/lib.rs @@ -72,6 +72,7 @@ pub use device_identity::DeviceIdentity; pub use diagnostic::ErrorPayload; pub use error::ProfileError; pub use profile_store::{FileLockGuard, ProfileStore}; +pub use secret_buf::SecretBuf; /// A type that can be stored in a profile directory. pub trait ProfileData: Serialize + DeserializeOwned { diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs index cce52a53f5..b5a8362eb6 100644 --- a/packages/stack-profile/src/secret_buf.rs +++ b/packages/stack-profile/src/secret_buf.rs @@ -27,13 +27,32 @@ const MIN_READ_SPARE: usize = 512; /// one, which [`Zeroizing`] wipes. Spare capacity is wiped too, since /// `Vec`'s `Zeroize` covers the whole allocation. /// -/// Writing to it ([`io::Write`](std::io::Write)) and reading into it -/// ([`read_from`](Self::read_from)) both write straight into the buffer, so -/// no intermediate copy of the bytes is left behind. -pub(crate) struct SecretBuf(Zeroizing>); +/// Writing to it ([`io::Write`](std::io::Write)) writes straight into the +/// buffer, so no intermediate copy of the bytes is left behind. +/// +/// Use it wherever a secret is serialised: `serde_json::to_string` and +/// `to_vec` grow a plain buffer as they go, freeing a partial copy of the +/// secret at each step. +/// +/// ``` +/// use stack_profile::SecretBuf; +/// +/// let body = SecretBuf::json(&serde_json::json!({ "token": "s3cret" }))?; +/// // `body` is wiped when it is dropped. +/// assert_eq!(body.as_slice(), br#"{"token":"s3cret"}"#); +/// # Ok::<(), serde_json::Error>(()) +/// ``` +pub struct SecretBuf(Zeroizing>); + +impl Default for SecretBuf { + fn default() -> Self { + Self::new() + } +} impl SecretBuf { - pub(crate) fn new() -> Self { + /// An empty buffer with room for a typical profile file or token. + pub fn new() -> Self { Self::with_capacity(INITIAL_CAPACITY) } @@ -47,6 +66,13 @@ impl SecretBuf { /// hint to avoid reallocating, and reads through no unwiped scratch /// buffer: a file whose metadata under-reports its length (as some /// network and FUSE filesystems do) still leaves no copy behind. + /// + /// # Errors + /// + /// Returns the reader's error, other than + /// [`Interrupted`](std::io::ErrorKind::Interrupted), which is retried, or + /// [`OutOfMemory`](std::io::ErrorKind::OutOfMemory) when the buffer cannot + /// grow. The bytes read so far are wiped either way. pub(crate) fn read_from(mut reader: impl Read) -> std::io::Result { let mut buf = Self::new(); loop { @@ -75,6 +101,53 @@ impl SecretBuf { &self.0 } + /// The buffer itself, still wiped on drop. + /// + /// The allocation is handed over, not copied. Do not grow the returned + /// `Vec`: it is an ordinary `Vec` again, and growing it leaves an unwiped + /// copy behind. + pub fn into_zeroizing(self) -> Zeroizing> { + self.0 + } + + /// `value` as JSON, serialised straight into a `SecretBuf`. + /// + /// Use this, not `serde_json::to_vec` or `to_string`, for any value that + /// holds a secret: those grow a plain buffer as they write and free each + /// outgrown copy unwiped. + /// + /// # Errors + /// + /// Returns the serialiser's error. The bytes written so far are wiped. + pub fn json(value: &T) -> serde_json::Result>> { + let mut buf = Self::new(); + serde_json::to_writer(&mut buf, value)?; + Ok(buf.into_zeroizing()) + } + + /// [`json`](Self::json) as a `String`, for an API that takes one. + /// + /// The bytes are moved into the `String`, not copied. Take it out with + /// `std::mem::take(&mut *json)` when it must be handed over by value. + /// + /// # Errors + /// + /// Returns the serialiser's error. The bytes written so far are wiped. + pub fn json_string( + value: &T, + ) -> serde_json::Result> { + let mut bytes = Self::json(value)?; + match String::from_utf8(std::mem::take(&mut *bytes)) { + Ok(json) => Ok(Zeroizing::new(json)), + // serde_json only writes UTF-8, so this is unreachable; the bytes + // are still wiped if it ever is not. + Err(e) => { + drop(Zeroizing::new(e.into_bytes())); + Err(serde::ser::Error::custom("serialised JSON was not UTF-8")) + } + } + } + /// Make room for `additional` more bytes without letting the `Vec` /// reallocate in place. fn reserve(&mut self, additional: usize) -> std::io::Result<()> { @@ -151,6 +224,21 @@ mod tests { (0..=255).cycle().take(len).collect() } + #[test] + fn json_serialises_into_the_buffer() { + let value = serde_json::json!({ "token": "s3cret" }); + assert_eq!( + SecretBuf::json(&value).unwrap().as_slice(), + br#"{"token":"s3cret"}"#, + "the bytes must be the JSON of the value", + ); + assert_eq!( + SecretBuf::json_string(&value).unwrap().as_str(), + r#"{"token":"s3cret"}"#, + "the string must be the JSON of the value", + ); + } + #[test] fn writes_hold_exactly_what_was_written() { let mut buf = SecretBuf::new(); @@ -269,4 +357,32 @@ mod tests { let err = SecretBuf::read_from(Failing).err().unwrap(); assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); } + + #[test] + fn serialised_json_comes_out_byte_for_byte() { + // Large enough that the buffer grows several times on the way. + let secret = "s".repeat(20_000); + let mut buf = SecretBuf::new(); + serde_json::to_writer(&mut buf, &serde_json::json!({ "token": secret })).unwrap(); + + let expected = serde_json::to_vec(&serde_json::json!({ "token": secret })).unwrap(); + assert_eq!(buf.as_bytes(), expected.as_slice()); + assert_eq!(buf.into_zeroizing().as_slice(), expected.as_slice()); + } + + /// `into_zeroizing` hands over the buffer itself. A copy would leave the + /// original allocation behind for `SecretBuf`'s own drop to wipe, but the copy + /// would be an exact-size `Vec` the caller might grow, which is what the type + /// exists to prevent. + #[test] + fn into_zeroizing_moves_the_allocation_rather_than_copying_it() { + let mut buf = SecretBuf::new(); + buf.write_all(b"bearer-token").unwrap(); + let before = buf.as_bytes().as_ptr(); + + let bytes = buf.into_zeroizing(); + + assert_eq!(bytes.as_ptr(), before); + assert_eq!(bytes.as_slice(), b"bearer-token"); + } }