fix(stack-auth,stack-profile): clamp server expires_in and wipe secret buffers

.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.

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<String>,
+        access_key: Option<&str>,
         crn: Option<Crn>,
         store: Option<ProfileStore>,
         transport: Option<SharedTransport>,
@@ -143,7 +143,7 @@ impl AutoStrategy {
 
     #[cfg(target_arch = "wasm32")]
     fn detect_inner(
-        access_key: Option<String>,
+        access_key: Option<&str>,
         crn: Option<Crn>,
         transport: Option<SharedTransport>,
     ) -> Result<Self, AuthError> {
@@ -180,7 +180,8 @@ impl AutoStrategy {
 /// # }
 /// ```
 pub struct AutoStrategyBuilder {
-    access_key: Option<String>,
+    /// A [`SecretToken`] so the key is wiped on drop and masked in `Debug`.
+    access_key: Option<SecretToken>,
     crn: Option<Crn>,
     transport: Option<SharedTransport>,
 }
@@ -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<String>) -> 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<AutoStrategy, AuthError> {
         // 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<String, std::env::VarError>) -> Option<SecretToken> {
+    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());
+        }
+    }
 }

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<PathBuf>,
     /// 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<Token, AuthError> {
         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;
         }
     }
 }