diff --git a/.changeset/auth-expiry-and-secret-wiping.md b/.changeset/auth-expiry-and-secret-wiping.md new file mode 100644 index 0000000000..7146e61144 --- /dev/null +++ b/.changeset/auth-expiry-and-secret-wiping.md @@ -0,0 +1,12 @@ +--- +"@cipherstash/auth": patch +--- + +Sign-in and token refresh no longer trust an unbounded `expires_in` from the +auth server. A token lifetime longer than 30 days is treated as 30 days, and a +device code is polled for at most one hour, so an oversized value can no +longer panic the process or wrap to an already-expired token. The buffers that +carry saved tokens and keys to and from `~/.cipherstash`, and an access key +read from `CS_CLIENT_ACCESS_KEY`, are now wiped from memory once used. +A device code's lifetime now runs from when it was issued, and polling stops +at that deadline rather than at the next poll after it. diff --git a/Cargo.lock b/Cargo.lock index 6964dbbf09..5548762390 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3410,6 +3410,7 @@ dependencies = [ "tempfile", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/languages/golang/auth/guest/Cargo.lock b/languages/golang/auth/guest/Cargo.lock index 970b1cd538..ba6588228c 100644 --- a/languages/golang/auth/guest/Cargo.lock +++ b/languages/golang/auth/guest/Cargo.lock @@ -1847,6 +1847,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/languages/golang/encrypt/guest/Cargo.lock b/languages/golang/encrypt/guest/Cargo.lock index 7f542b6ded..46b22fa357 100644 --- a/languages/golang/encrypt/guest/Cargo.lock +++ b/languages/golang/encrypt/guest/Cargo.lock @@ -2126,6 +2126,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/eql/Cargo.lock b/packages/eql/Cargo.lock index 5c90c00475..2eda5f8a01 100644 --- a/packages/eql/Cargo.lock +++ b/packages/eql/Cargo.lock @@ -4205,6 +4205,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-auth/fuzz/Cargo.lock b/packages/stack-auth/fuzz/Cargo.lock index 99bbb9c36a..699f876fac 100644 --- a/packages/stack-auth/fuzz/Cargo.lock +++ b/packages/stack-auth/fuzz/Cargo.lock @@ -2643,6 +2643,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-auth/src/auto_strategy.rs b/packages/stack-auth/src/auto_strategy.rs index 8a28f618f7..457bcf3ea3 100644 --- a/packages/stack-auth/src/auto_strategy.rs +++ b/packages/stack-auth/src/auto_strategy.rs @@ -9,7 +9,7 @@ use crate::transport::{self, SharedTransport}; use crate::HttpTransport; #[cfg(not(target_arch = "wasm32"))] use crate::Token; -use crate::{AuthError, AuthStrategy, ServiceToken}; +use crate::{AuthError, AuthStrategy, SecretToken, ServiceToken}; /// An [`AuthStrategy`] that automatically detects available credentials /// and delegates to the appropriate inner strategy. @@ -106,7 +106,7 @@ impl AutoStrategy { /// unreachable (no filesystem) — callers must supply an access key. #[cfg(not(target_arch = "wasm32"))] fn detect_inner( - access_key: Option, + access_key: Option<&str>, crn: Option, store: Option, transport: Option, @@ -143,7 +143,7 @@ impl AutoStrategy { #[cfg(target_arch = "wasm32")] fn detect_inner( - access_key: Option, + access_key: Option<&str>, crn: Option, transport: Option, ) -> Result { @@ -180,7 +180,8 @@ impl AutoStrategy { /// # } /// ``` pub struct AutoStrategyBuilder { - access_key: Option, + /// A [`SecretToken`] so the key is wiped on drop and masked in `Debug`. + access_key: Option, crn: Option, transport: Option, } @@ -195,7 +196,7 @@ impl AutoStrategyBuilder { /// Provide an explicit access key. Takes precedence over env vars. pub fn with_access_key(mut self, access_key: impl Into) -> Self { - self.access_key = Some(access_key.into()); + self.access_key = Some(SecretToken::new(access_key)); self } @@ -214,9 +215,11 @@ impl AutoStrategyBuilder { /// 4. [`AuthError::NotAuthenticated`] pub fn detect(self) -> Result { // Merge explicit values with env vars (explicit wins) + // Wrapped as soon as it is read, so this copy is wiped on drop. The + // process environment keeps its own copy, which is beyond our reach. let access_key = self .access_key - .or_else(|| std::env::var("CS_CLIENT_ACCESS_KEY").ok()); + .or_else(|| access_key_from_env(std::env::var("CS_CLIENT_ACCESS_KEY"))); let crn = match self.crn { Some(crn) => Some(crn), @@ -238,11 +241,20 @@ impl AutoStrategyBuilder { None } }; - AutoStrategy::detect_inner(access_key, crn, store, self.transport) + AutoStrategy::detect_inner( + access_key.as_ref().map(SecretToken::as_str), + crn, + store, + self.transport, + ) } #[cfg(target_arch = "wasm32")] { - AutoStrategy::detect_inner(access_key, crn, self.transport) + AutoStrategy::detect_inner( + access_key.as_ref().map(SecretToken::as_str), + crn, + self.transport, + ) } } } @@ -256,6 +268,25 @@ impl AuthStrategy for &AutoStrategy { } } +/// Wrap an access key read from the environment, wiping it if it is unusable. +/// +/// A value that is not valid UTF-8 comes back as [`VarError::NotUnicode`], +/// which owns the whole key. It cannot be a valid access key, so it is +/// treated as unset, as before; but it is wiped first rather than dropped, +/// since it is still a credential. +/// +/// [`VarError::NotUnicode`]: std::env::VarError::NotUnicode +fn access_key_from_env(value: Result) -> Option { + match value { + Ok(key) => Some(SecretToken::new(key)), + Err(std::env::VarError::NotUnicode(key)) => { + drop(zeroize::Zeroizing::new(key.into_encoded_bytes())); + None + } + Err(std::env::VarError::NotPresent) => None, + } +} + #[cfg(test)] mod tests { use super::*; @@ -319,7 +350,7 @@ mod tests { #[cfg(feature = "http")] // builds a strategy with no transport of its own fn access_key_with_valid_crn() { let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), + Some("CSAKtestKeyId.testKeySecret"), Some(valid_crn()), None, None, @@ -331,24 +362,16 @@ mod tests { #[test] fn access_key_without_crn_returns_missing_workspace_crn() { - let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), - None, - None, - None, - ); + let result = + AutoStrategy::detect_inner(Some("CSAKtestKeyId.testKeySecret"), None, None, None); assert!(matches!(result, Err(AuthError::MissingWorkspaceCrn(_)))); } #[test] fn invalid_access_key_format_returns_invalid_access_key() { - let result = AutoStrategy::detect_inner( - Some("not-a-valid-key".into()), - Some(valid_crn()), - None, - None, - ); + let result = + AutoStrategy::detect_inner(Some("not-a-valid-key"), Some(valid_crn()), None, None); assert!(matches!(result, Err(AuthError::InvalidAccessKey(_)))); } @@ -389,7 +412,7 @@ mod tests { let store = write_token_store(dir.path()); let result = AutoStrategy::detect_inner( - Some("CSAKtestKeyId.testKeySecret".into()), + Some("CSAKtestKeyId.testKeySecret"), Some(valid_crn()), Some(store), None, @@ -450,4 +473,26 @@ mod tests { assert!(matches!(result, Err(AuthError::InvalidAccessKey(_)))); } } + + mod access_key_from_env { + use super::super::access_key_from_env; + use std::env::VarError; + + #[test] + fn wraps_a_set_value() { + let key = access_key_from_env(Ok("CSAKid.secret".to_string())).unwrap(); + assert_eq!(key.as_str(), "CSAKid.secret"); + } + + #[test] + fn treats_an_unset_value_as_absent() { + assert!(access_key_from_env(Err(VarError::NotPresent)).is_none()); + } + + #[test] + fn treats_a_non_utf8_value_as_absent() { + let value = std::ffi::OsString::from("CSAKid.secret"); + assert!(access_key_from_env(Err(VarError::NotUnicode(value))).is_none()); + } + } } diff --git a/packages/stack-auth/src/device_code/mod.rs b/packages/stack-auth/src/device_code/mod.rs index eb8f659d41..d343cb1022 100644 --- a/packages/stack-auth/src/device_code/mod.rs +++ b/packages/stack-auth/src/device_code/mod.rs @@ -18,6 +18,16 @@ use protocol::{ #[cfg(test)] mod tests; +/// The longest device-code lifetime this crate polls for, in seconds (one +/// hour). +/// +/// `expires_in` is server-supplied, and [`PendingDeviceCode::poll_for_token`] +/// adds it to an [`Instant`](tokio::time::Instant), which panics on overflow +/// in every build profile. RFC 8628 device codes live for minutes, so a +/// larger value is clamped rather than trusted: at worst the user restarts +/// sign-in after an hour. +const MAX_DEVICE_CODE_LIFETIME_SECS: u64 = 60 * 60; + // Keep the browser boundary visible to callers of `open_in_browser`. fn launch_browser(uri: &str) -> std::io::Result<()> { #[cfg(test)] @@ -142,9 +152,11 @@ impl DeviceCodeStrategy { let token_url = self.base_url.join("oauth/device/token")?; + let expires_in = code.expires_in.min(MAX_DEVICE_CODE_LIFETIME_SECS); tracing::debug!( user_code = %code.user_code, - expires_in = code.expires_in, + expires_in, + server_expires_in = code.expires_in, "device code received" ); @@ -156,7 +168,11 @@ impl DeviceCodeStrategy { user_code: code.user_code, verification_uri: code.verification_uri, verification_uri_complete: code.verification_uri_complete, - expires_in: code.expires_in, + expires_in, + // Fixed now, when the code is issued, so a caller that waits + // before polling does not get a fresh window. `expires_in` is + // clamped above, so this cannot overflow. + deadline: tokio::time::Instant::now() + tokio::time::Duration::from_secs(expires_in), profile_dir: self.profile_dir.clone(), device_identity: self.device_identity.clone(), }) @@ -265,8 +281,12 @@ pub struct PendingDeviceCode { verification_uri: String, /// The full verification URI with the user code pre-filled. verification_uri_complete: String, - /// How many seconds the device code remains valid. + /// How many seconds the device code remains valid, clamped to + /// [`MAX_DEVICE_CODE_LIFETIME_SECS`]. expires_in: u64, + /// When the device code expires: when it was received plus `expires_in`. + /// Polling stops here, including mid-request and mid-sleep. + deadline: tokio::time::Instant, /// Profile directory override. Falls back to `~/.cipherstash`. profile_dir: Option, /// Device identity to associate with the token. @@ -289,7 +309,12 @@ impl PendingDeviceCode { &self.verification_uri_complete } - /// How many seconds the device code remains valid. + /// How many seconds the device code was valid for when it was issued. + /// + /// This is the server's `expires_in`, capped at one hour. The lifetime + /// runs from when [`begin`](DeviceCodeStrategy::begin) received the + /// code, not from when polling starts: polling stops at that point even + /// if the server claimed a longer lifetime. pub fn expires_in(&self) -> u64 { self.expires_in } @@ -316,8 +341,7 @@ impl PendingDeviceCode { pub async fn poll_for_token(self) -> Result { let transport = bundled_transport(); let mut interval = tokio::time::Duration::from_secs(5); - let deadline = - tokio::time::Instant::now() + tokio::time::Duration::from_secs(self.expires_in); + let deadline = self.deadline; tracing::debug!( url = %self.token_url, @@ -331,16 +355,22 @@ impl PendingDeviceCode { return Err(AuthError::TokenExpired(crate::error::TokenExpired)); } - let resp = transport::post_form( - &transport, - self.token_url.clone(), - &TokenRequest { - client_id: &self.client_id, - device_code: &self.device_code, - grant_type: "urn:ietf:params:oauth:grant-type:device_code", - }, + // A response that arrives after the deadline is dropped rather + // than saved: the code has expired, whatever the server says. + let resp = tokio::time::timeout_at( + deadline, + transport::post_form( + &transport, + self.token_url.clone(), + &TokenRequest { + client_id: &self.client_id, + device_code: &self.device_code, + grant_type: "urn:ietf:params:oauth:grant-type:device_code", + }, + ), ) - .await?; + .await + .map_err(|_| AuthError::TokenExpired(crate::error::TokenExpired))??; if resp.is_success() { tracing::debug!("token received"); @@ -352,7 +382,7 @@ impl PendingDeviceCode { let mut token = Token { access_token: token_resp.access_token, token_type: token_resp.token_type, - expires_at: now + token_resp.expires_in, + expires_at: crate::token::expires_at_after(now, token_resp.expires_in), refresh_token: token_resp.refresh_token, region: None, client_id: None, @@ -417,7 +447,9 @@ impl PendingDeviceCode { } } - tokio::time::sleep(interval).await; + // Wake at the deadline rather than sleeping past it; the check at + // the top of the loop then reports the expiry. + tokio::time::sleep_until(deadline.min(tokio::time::Instant::now() + interval)).await; } } } diff --git a/packages/stack-auth/src/device_code/tests.rs b/packages/stack-auth/src/device_code/tests.rs index c277b0bacd..d77a7f6227 100644 --- a/packages/stack-auth/src/device_code/tests.rs +++ b/packages/stack-auth/src/device_code/tests.rs @@ -423,16 +423,13 @@ async fn test_poll_for_token_authorization_pending_then_success() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); mock_code_endpoint(&mut mocks); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("authorization_pending")); - }); + mock_token_errors(&mut mocks, "authorization_pending", 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; // Use tokio::join! so the swap future can borrow server.mocks() directly // (the shared RwLock) rather than cloning the MockSet. - // First poll at T=5s returns "authorization_pending". + // Polls at T=0s and T=5s return "authorization_pending". // At T=6s the mock is swapped. Second poll at T=10s returns success. let (result, _) = tokio::join!(pending.poll_for_token(), async { tokio::time::sleep(tokio::time::Duration::from_secs(6)).await; @@ -456,10 +453,7 @@ async fn test_poll_for_token_slow_down_then_success() { let dir = TempDir::new().unwrap(); let mut mocks = MockSet::new(); mock_code_endpoint(&mut mocks); - mocks.mock(|when, then| { - when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("slow_down")); - }); + mock_token_errors(&mut mocks, "slow_down", 2); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; @@ -502,16 +496,199 @@ async fn test_poll_for_token_slow_down_increases_interval() { "expires_in": 12 })); }); + mock_token_errors(&mut mocks, "slow_down", 2); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); +} + +/// Answer the token endpoint with `error` for at most `polls` requests, +/// then with `access_denied`. A test whose server never stops answering +/// `authorization_pending` or `slow_down` would spin forever if polling +/// stopped waiting between requests (paused time only advances when the +/// runtime is idle); the cap turns that into a fast failure instead. +fn mock_token_errors(mocks: &mut MockSet, error: &'static str, polls: usize) { + mocks.mock_with_options(1, Some(polls), move |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json(error)); + }); + mocks.mock_with_options(2, None, |when, then| { + when.post().path("/oauth/device/token"); + then.bad_request().json(error_json("access_denied")); + }); +} + +fn device_code_json_expiring_in(expires_in: u64) -> serde_json::Value { + let mut json = device_code_json(); + json["expires_in"] = expires_in.into(); + json +} + +/// A server-supplied device-code lifetime past the cap is clamped, so the +/// poll deadline (`Instant::now() + expires_in`) cannot overflow and panic. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_clamps_an_oversized_device_code_lifetime() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(u64::MAX)); + }); mocks.mock(|when, then| { when.post().path("/oauth/device/token"); - then.bad_request().json(error_json("slow_down")); + then.json(token_json()); + }); + let server = start_server(mocks).await; + + let pending = begin_pending(&server, &dir).await; + assert_eq!( + pending.expires_in(), + 3_600, + "an oversized device-code lifetime must be clamped to one hour" + ); + + let token = pending + .poll_for_token() + .await + .expect("polling must not panic on an oversized device-code lifetime"); + assert!(!token.is_expired()); +} + +/// The clamped deadline is still enforced: once it passes, polling stops +/// with `TokenExpired` rather than running for as long as the server said. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_expires_at_the_clamped_deadline() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(u64::MAX)); + }); + mock_token_errors(&mut mocks, "authorization_pending", 721); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); + let elapsed = started.elapsed().as_secs(); + assert!( + (MAX_DEVICE_CODE_LIFETIME_SECS..MAX_DEVICE_CODE_LIFETIME_SECS + 10).contains(&elapsed), + "polling should stop at the one-hour cap; stopped after {elapsed}s" + ); +} + +fn mock_pending_for(mocks: &mut MockSet, expires_in: u64, polls: usize) { + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(expires_in)); }); + mock_token_errors(mocks, "authorization_pending", polls); +} + +/// The lifetime runs from when the code was received, not from when +/// polling starts: a caller that waits past it gets no fresh window. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_counts_the_lifetime_from_begin() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mock_pending_for(&mut mocks, 900, 1); let server = start_server(mocks).await; let pending = begin_pending(&server, &dir).await; + tokio::time::advance(tokio::time::Duration::from_secs(901)).await; + let started = tokio::time::Instant::now(); let err = pending.poll_for_token().await.unwrap_err(); assert!(matches!(err, AuthError::TokenExpired(_))); + assert_eq!( + started.elapsed().as_secs(), + 0, + "an already-expired code must not be polled for another window" + ); +} + +/// Polling stops at the deadline itself, not at the first poll after it. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_does_not_sleep_past_the_deadline() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + // Polls at T=0 and T=5; the next would be T=10, past the 7s deadline. + mock_pending_for(&mut mocks, 7, 2); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!(matches!(err, AuthError::TokenExpired(_))); + assert_eq!( + started.elapsed().as_secs(), + 7, + "polling should stop at the deadline, not sleep past it" + ); +} + +/// Polling waits the interval between requests: the server allows exactly +/// the polls that fit before the deadline, so one more fails the test. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_waits_the_interval_between_polls() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/device/code"); + then.json(device_code_json_expiring_in(12)); + }); + // Polls at T=0, 5 and 10; the next would be T=15, past the deadline. + mock_token_errors(&mut mocks, "authorization_pending", 3); + let server = start_server(mocks).await; + let pending = begin_pending(&server, &dir).await; + let started = tokio::time::Instant::now(); + + let err = pending.poll_for_token().await.unwrap_err(); + + assert!( + matches!(err, AuthError::TokenExpired(_)), + "expected the deadline to end polling after three polls, got {err:?}" + ); + assert_eq!(started.elapsed().as_secs(), 12); +} + +/// A server-supplied token lifetime past the cap is clamped on the +/// device-code path too, rather than wrapping `now + expires_in` to a +/// timestamp in the past. +#[tokio::test(start_paused = true)] +async fn test_poll_for_token_clamps_an_oversized_token_lifetime() { + let dir = TempDir::new().unwrap(); + let mut mocks = MockSet::new(); + mock_code_endpoint(&mut mocks); + mocks.mock(|when, then| { + when.post().path("/oauth/device/token"); + then.json(serde_json::json!({ + "access_token": test_access_token(), + "token_type": "Bearer", + "expires_in": u64::MAX + })); + }); + let server = start_server(mocks).await; + + let token = begin_pending(&server, &dir) + .await + .poll_for_token() + .await + .unwrap(); + + let max = crate::token::MAX_TOKEN_LIFETIME_SECS; + assert!(!token.is_expired(), "the token must not wrap to expired"); + assert!( + (max - 2..=max).contains(&token.expires_in()), + "token lifetime should be clamped; got {}", + token.expires_in() + ); } // ---- ensure_trailing_slash / URL join tests ---- diff --git a/packages/stack-auth/src/token.rs b/packages/stack-auth/src/token.rs index a66d10499c..02f98b5f8a 100644 --- a/packages/stack-auth/src/token.rs +++ b/packages/stack-auth/src/token.rs @@ -17,6 +17,29 @@ impl stack_profile::ProfileData for Token { /// callers can still use the current token. const EXPIRY_LEEWAY_SECS: u64 = 90; +/// The longest token lifetime this crate accepts from a server, in seconds +/// (30 days). +/// +/// `expires_in` is server-supplied. A larger value is clamped to this rather +/// than trusted: a token that claims to outlive it is refreshed early, which +/// is safe, whereas an unbounded value would overflow `now + expires_in` +/// (see [`expires_at_after`]). +pub(crate) const MAX_TOKEN_LIFETIME_SECS: u64 = 30 * 24 * 60 * 60; + +/// The absolute expiry (seconds since the Unix epoch) of a token issued at +/// `now` with a server-supplied lifetime of `expires_in` seconds. +/// +/// Every place that turns an OAuth `expires_in` into a [`Token::expires_at`] +/// goes through here. The lifetime is clamped to [`MAX_TOKEN_LIFETIME_SECS`] +/// and the addition saturates, so a buggy or hostile server cannot make it +/// panic (debug builds) or wrap to an already-expired timestamp and send the +/// caller into a refresh loop (release builds, which carry no +/// `overflow-checks`). This is the same rule [`Token::is_expired_at`] +/// follows for its leeway. +pub(crate) fn expires_at_after(now: u64, expires_in: u64) -> u64 { + now.saturating_add(expires_in.min(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 @@ -329,7 +352,7 @@ impl Token { Ok(Token { access_token: token_resp.access_token, token_type: token_resp.token_type, - expires_at: now_unix_secs() + token_resp.expires_in, + expires_at: expires_at_after(now_unix_secs(), token_resp.expires_in), refresh_token: token_resp.refresh_token, region: None, client_id: None, @@ -477,6 +500,39 @@ mod tests { ); } + // ---- expires_at_after ---- + + #[test] + fn expires_at_after_passes_an_ordinary_lifetime_through() { + assert_eq!(expires_at_after(1_000, 3_600), 4_600); + } + + #[test] + fn the_token_lifetime_cap_is_thirty_days() { + assert_eq!(MAX_TOKEN_LIFETIME_SECS, 2_592_000); + } + + #[test] + fn expires_at_after_clamps_an_oversized_lifetime() { + assert_eq!( + expires_at_after(1_000, u64::MAX), + 1_000 + MAX_TOKEN_LIFETIME_SECS, + "a server-supplied lifetime past the cap must be clamped, not trusted" + ); + assert_eq!( + expires_at_after(1_000, MAX_TOKEN_LIFETIME_SECS), + 1_000 + MAX_TOKEN_LIFETIME_SECS, + "the cap itself is accepted as-is" + ); + } + + #[test] + fn expires_at_after_saturates_instead_of_wrapping() { + // A wrapped sum would be a timestamp in the past: an already-expired + // token, and so a refresh loop. + assert_eq!(expires_at_after(u64::MAX - 10, 3_600), u64::MAX); + } + // ---- refresh() tests ---- // // Grouped under one feature gate rather than one per item: every helper @@ -536,6 +592,37 @@ mod tests { assert!((3598..=3600).contains(&refreshed.expires_in())); } + #[tokio::test] + async fn test_refresh_clamps_an_oversized_expires_in() { + let mut mocks = MockSet::new(); + mocks.mock(|when, then| { + when.post().path("/oauth/token"); + then.json(serde_json::json!({ + "access_token": "new-access-token", + "token_type": "Bearer", + "expires_in": u64::MAX, + })); + }); + let server = start_server(mocks).await; + let base_url = server.url(""); + + let refresh_token = SecretToken::new("test-refresh-token"); + let refreshed = Token::refresh(&refresh_token, &base_url, "cli", None) + .await + .expect("an oversized expires_in must not fail or panic the refresh"); + + assert!( + !refreshed.is_expired(), + "the token must not wrap to an already-expired timestamp" + ); + assert!( + (MAX_TOKEN_LIFETIME_SECS - 2..=MAX_TOKEN_LIFETIME_SECS) + .contains(&refreshed.expires_in()), + "lifetime should be clamped to MAX_TOKEN_LIFETIME_SECS; got {}", + refreshed.expires_in() + ); + } + #[tokio::test] async fn test_refresh_invalid_grant() { let mut mocks = MockSet::new(); diff --git a/packages/stack-encrypt/fuzz/Cargo.lock b/packages/stack-encrypt/fuzz/Cargo.lock index 1663a68989..e2f5a569cc 100644 --- a/packages/stack-encrypt/fuzz/Cargo.lock +++ b/packages/stack-encrypt/fuzz/Cargo.lock @@ -2058,6 +2058,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-kms/fuzz/Cargo.lock b/packages/stack-kms/fuzz/Cargo.lock index a18c2eba9f..8c5b3705a6 100644 --- a/packages/stack-kms/fuzz/Cargo.lock +++ b/packages/stack-kms/fuzz/Cargo.lock @@ -2009,6 +2009,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "uuid", + "zeroize", ] [[package]] diff --git a/packages/stack-profile/Cargo.toml b/packages/stack-profile/Cargo.toml index b2b57354e2..fecede5af5 100644 --- a/packages/stack-profile/Cargo.toml +++ b/packages/stack-profile/Cargo.toml @@ -17,6 +17,9 @@ serde = { workspace = true } serde_json = { workspace = true } thiserror = { workspace = true } uuid = { workspace = true } +# auth.json and secretkey.json hold bearer tokens and keys: the JSON buffers +# that carry them to and from disk are wiped (see `secret_buf`). +zeroize = { workspace = true } # The default device name is the hostname, which only the creating half of # `DeviceIdentity` needs — and that half is native-only: the crate builds for diff --git a/packages/stack-profile/src/lib.rs b/packages/stack-profile/src/lib.rs index 1455102f45..bbf6c98d5a 100644 --- a/packages/stack-profile/src/lib.rs +++ b/packages/stack-profile/src/lib.rs @@ -66,6 +66,7 @@ mod device_identity; pub mod diagnostic; mod error; mod profile_store; +mod secret_buf; pub use device_identity::DeviceIdentity; pub use diagnostic::ErrorPayload; diff --git a/packages/stack-profile/src/profile_store.rs b/packages/stack-profile/src/profile_store.rs index f32b47be9f..9ea23583df 100644 --- a/packages/stack-profile/src/profile_store.rs +++ b/packages/stack-profile/src/profile_store.rs @@ -3,6 +3,7 @@ use std::path::{Path, PathBuf}; use serde::de::DeserializeOwned; use serde::Serialize; +use crate::secret_buf::SecretBuf; use crate::{ProfileData, ProfileError}; const CS_CONFIG_PATH_ENV: &str = "CS_CONFIG_PATH"; @@ -291,8 +292,12 @@ impl ProfileStore { Self::validate_filename(filename)?; std::fs::create_dir_all(&self.dir)?; let path = self.dir.join(filename); - let json = serde_json::to_string_pretty(value)?; - Self::write_to_path(&path, &json, _mode) + // The serialised form of a profile can carry secrets (`auth.json`'s + // tokens, `secretkey.json`'s key), so it is built in a buffer that is + // wiped on drop and on every reallocation, never in a plain `String`. + let mut json = SecretBuf::new(); + serde_json::to_writer_pretty(&mut json, value)?; + Self::write_to_path(&path, json.as_bytes(), _mode) } /// Atomically write JSON content to an absolute path, optionally setting @@ -324,7 +329,7 @@ impl ProfileStore { /// processes won't trip this — but to defend against third-party /// readers we retry the rename a handful of times with brief backoff /// before giving up. - fn write_to_path(path: &Path, json: &str, _mode: Option) -> Result<(), ProfileError> { + fn write_to_path(path: &Path, json: &[u8], _mode: Option) -> Result<(), ProfileError> { use std::io::Write; let parent = path.parent().ok_or_else(|| { @@ -363,7 +368,7 @@ impl ProfileStore { } opts.open(&tmp_path)? }; - file.write_all(json.as_bytes())?; + file.write_all(json)?; file.sync_all()?; drop(file); @@ -451,9 +456,28 @@ impl ProfileStore { pub fn load(&self, filename: &str) -> Result { Self::validate_filename(filename)?; let path = self.dir.join(filename); - match std::fs::read_to_string(&path) { + // Read into a buffer wiped on drop and on growth: the file may hold + // secrets (see `write`). `fs::read` would not do: it reallocates, and + // frees an unwiped copy, whenever the file's metadata under-reports + // its length. + // + // serde_json still decodes a string containing escape sequences + // through a scratch buffer of its own, which it does not wipe. Tokens + // and keys are base64 or JWTs, which need no escaping, so that path + // is not taken for them. + match std::fs::File::open(&path).and_then(SecretBuf::read_from) { Ok(contents) => { - let value: T = serde_json::from_str(&contents)?; + // Validate the whole file as UTF-8 first, as `read_to_string` + // did: `from_slice` skips the bytes of fields it ignores, so + // it would accept invalid UTF-8 in an unknown field. This + // borrows the buffer, so it makes no copy. + let text = std::str::from_utf8(contents.as_bytes()).map_err(|_| { + ProfileError::Io(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "profile file is not valid UTF-8", + )) + })?; + let value: T = serde_json::from_str(text)?; Ok(value) } Err(e) if e.kind() == std::io::ErrorKind::NotFound => { @@ -621,6 +645,32 @@ mod tests { assert!(matches!(err, ProfileError::NotFound { .. })); } + fn assert_invalid_utf8_is_an_io_error(contents: &[u8]) { + let dir = tempfile::tempdir().unwrap(); + let store = ProfileStore::new(dir.path()); + std::fs::write(dir.path().join("data.json"), contents).unwrap(); + + let err = store.load::("data.json").unwrap_err(); + assert!( + matches!(&err, ProfileError::Io(e) if e.kind() == std::io::ErrorKind::InvalidData), + "a non-UTF-8 file should be an InvalidData I/O error, as before; got {err:?}" + ); + } + + #[test] + fn load_rejects_invalid_utf8_in_a_known_field() { + assert_invalid_utf8_is_an_io_error(b"{\"name\": \"\xff\", \"value\": 1}"); + } + + #[test] + fn load_rejects_invalid_utf8_in_an_unknown_field() { + // serde_json skips an ignored field's bytes without validating them, + // so this file would load if UTF-8 were left to the parser. + assert_invalid_utf8_is_an_io_error( + b"{\"name\": \"ok\", \"value\": 1, \"extra\": \"\xff\"}", + ); + } + #[test] fn clear_removes_existing_file() { let dir = tempfile::tempdir().unwrap(); diff --git a/packages/stack-profile/src/secret_buf.rs b/packages/stack-profile/src/secret_buf.rs new file mode 100644 index 0000000000..cce52a53f5 --- /dev/null +++ b/packages/stack-profile/src/secret_buf.rs @@ -0,0 +1,272 @@ +//! A byte buffer for secrets that is wiped on drop and whenever it grows. + +use std::io::Read; + +use zeroize::Zeroizing; + +/// The capacity a [`SecretBuf`] starts with. Profile files are small (a token +/// file is well under 4 KiB), so most buffers never grow. +const INITIAL_CAPACITY: usize = 4096; + +/// How much room [`SecretBuf::read_from`] makes when it grows the buffer. +const READ_CHUNK: usize = 4096; + +/// The least spare room [`SecretBuf::read_from`] reads into before it grows +/// the buffer. A file that fits in the initial capacity with this much to +/// spare is read, end-of-file probe included, without growing. +const MIN_READ_SPARE: usize = 512; + +/// A byte buffer that is zeroized when dropped, including every allocation it +/// outgrows. +/// +/// Wrapping a `Vec` in [`Zeroizing`] alone is not enough for a buffer +/// that grows: when a `Vec` reallocates, it frees the old allocation without +/// wiping it, leaving a copy of the bytes so far on the heap. `SecretBuf` +/// never lets its `Vec` reallocate itself. When it needs more room it +/// allocates a larger buffer, copies the contents across, and drops the old +/// 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>); + +impl SecretBuf { + pub(crate) fn new() -> Self { + Self::with_capacity(INITIAL_CAPACITY) + } + + fn with_capacity(capacity: usize) -> Self { + Self(Zeroizing::new(Vec::with_capacity(capacity))) + } + + /// Read `reader` to its end. + /// + /// Unlike [`Read::read_to_end`], this does not depend on a correct size + /// 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. + pub(crate) fn read_from(mut reader: impl Read) -> std::io::Result { + let mut buf = Self::new(); + loop { + if buf.0.capacity() - buf.0.len() < MIN_READ_SPARE { + buf.reserve(READ_CHUNK)?; + } + let filled = buf.0.len(); + // At most one chunk, so each read zero-fills a bounded window + // rather than the whole spare tail; and never past the + // capacity, so this does not reallocate. + let window_end = buf.0.capacity().min(filled + READ_CHUNK); + buf.0.resize(window_end, 0); + let result = reader.read(&mut buf.0[filled..]); + buf.0.truncate(filled + *result.as_ref().unwrap_or(&0)); + match result { + Ok(0) => return Ok(buf), + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::Interrupted => {} + Err(e) => return Err(e), + } + } + } + + /// The bytes held so far. + pub(crate) fn as_bytes(&self) -> &[u8] { + &self.0 + } + + /// Make room for `additional` more bytes without letting the `Vec` + /// reallocate in place. + fn reserve(&mut self, additional: usize) -> std::io::Result<()> { + let needed = self + .0 + .len() + .checked_add(additional) + .ok_or(std::io::ErrorKind::OutOfMemory)?; + if needed <= self.0.capacity() { + return Ok(()); + } + let capacity = needed.max(self.0.capacity().saturating_mul(2)); + // `try_reserve_exact`, not `Vec::with_capacity`: an oversized input + // must come back as an error, not abort the process. + let mut grown = Zeroizing::new(Vec::new()); + grown + .try_reserve_exact(capacity) + .map_err(|_| std::io::ErrorKind::OutOfMemory)?; + grown.extend_from_slice(&self.0); + // The old buffer is dropped here, and `Zeroizing` wipes it. + self.0 = grown; + Ok(()) + } +} + +impl std::io::Write for SecretBuf { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.reserve(buf.len())?; + self.0.extend_from_slice(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use std::io::Write; + + use super::*; + + /// Yields its bytes a few at a time, with an `Interrupted` in between, + /// the way a slow or signalled file read can. + struct Trickle { + bytes: Vec, + pos: usize, + interrupt_next: bool, + } + + impl Read for Trickle { + fn read(&mut self, out: &mut [u8]) -> std::io::Result { + self.interrupt_next = !self.interrupt_next; + if self.interrupt_next { + return Err(std::io::ErrorKind::Interrupted.into()); + } + let n = out.len().min(3).min(self.bytes.len() - self.pos); + out[..n].copy_from_slice(&self.bytes[self.pos..self.pos + n]); + self.pos += n; + Ok(n) + } + } + + struct Failing; + + impl Read for Failing { + fn read(&mut self, _: &mut [u8]) -> std::io::Result { + Err(std::io::ErrorKind::PermissionDenied.into()) + } + } + + fn payload(len: usize) -> Vec { + (0..=255).cycle().take(len).collect() + } + + #[test] + fn writes_hold_exactly_what_was_written() { + let mut buf = SecretBuf::new(); + buf.write_all(b"hello, ").unwrap(); + buf.write_all(b"world").unwrap(); + assert_eq!(buf.as_bytes(), b"hello, world"); + } + + #[test] + fn writes_grow_past_the_capacity_without_losing_bytes() { + let mut buf = SecretBuf::with_capacity(4); + let payload = payload(10_000); + for chunk in payload.chunks(7) { + buf.write_all(chunk).unwrap(); + } + assert_eq!(buf.as_bytes(), payload.as_slice()); + } + + #[test] + fn writes_match_serde_json_pretty_output() { + let value = serde_json::json!({ "access_token": "secret", "expires_at": 1 }); + let mut buf = SecretBuf::with_capacity(1); + serde_json::to_writer_pretty(&mut buf, &value).unwrap(); + assert_eq!( + buf.as_bytes(), + serde_json::to_string_pretty(&value).unwrap().as_bytes() + ); + } + + #[test] + fn read_from_reads_everything_across_many_growths() { + let payload = payload(3 * INITIAL_CAPACITY + 17); + let buf = SecretBuf::read_from(payload.as_slice()).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + } + + #[test] + fn read_from_does_not_grow_for_a_file_that_fits() { + let payload = payload(2_000); + let buf = SecretBuf::read_from(payload.as_slice()).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + assert_eq!( + buf.0.capacity(), + INITIAL_CAPACITY, + "a profile-sized file should be read without growing the buffer" + ); + } + + #[test] + fn reserve_returns_an_error_instead_of_aborting() { + let mut buf = SecretBuf::with_capacity(1); + buf.write_all(b"x").unwrap(); + let err = buf.reserve(isize::MAX as usize).unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::OutOfMemory); + assert_eq!(buf.as_bytes(), b"x", "a failed reserve keeps the contents"); + } + + /// Records the largest slice it is asked to fill, and fills at most + /// 1 KiB of it, the way a pipe or network filesystem can. + struct ShortReads { + bytes: Vec, + pos: usize, + largest_window: usize, + } + + impl Read for &mut ShortReads { + fn read(&mut self, out: &mut [u8]) -> std::io::Result { + self.largest_window = self.largest_window.max(out.len()); + let n = out.len().min(1024).min(self.bytes.len() - self.pos); + out[..n].copy_from_slice(&self.bytes[self.pos..self.pos + n]); + self.pos += n; + Ok(n) + } + } + + #[test] + fn read_from_zero_fills_at_most_one_chunk_per_read() { + // Each read zero-fills the window it reads into. If the window were + // all the spare capacity, short reads into a large buffer would + // re-zero its whole tail every time: quadratic in the file size. + let payload = payload(64 * 1024); + let mut reader = ShortReads { + bytes: payload.clone(), + pos: 0, + largest_window: 0, + }; + let buf = SecretBuf::read_from(&mut reader).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + assert!( + reader.largest_window <= READ_CHUNK, + "each read should fill at most {READ_CHUNK} bytes; one was offered {}", + reader.largest_window + ); + } + + #[test] + fn read_from_reads_an_empty_reader() { + let buf = SecretBuf::read_from(&[][..]).unwrap(); + assert!(buf.as_bytes().is_empty()); + } + + #[test] + fn read_from_retries_interrupted_and_short_reads() { + let payload = payload(50); + let reader = Trickle { + bytes: payload.clone(), + pos: 0, + interrupt_next: false, + }; + let buf = SecretBuf::read_from(reader).unwrap(); + assert_eq!(buf.as_bytes(), payload.as_slice()); + } + + #[test] + fn read_from_returns_a_read_error() { + let err = SecretBuf::read_from(Failing).err().unwrap(); + assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); + } +}