Use secure_string for more secure Password and SecretKey

.gitignore

@@ -13,3 +13,6 @@ target/
 *.db
 *.sqlite3
 *.sqlite3-journal
+
+# VSCode
+.vscode/

Cargo.toml

@@ -96,6 +96,7 @@ mime_guess = { version = "2", default-features = false }
 mockall = "0.13"
 multer = "3"
 password-auth = { version = "1", default-features = false }
+secure-string = "0.3.0"
 petgraph = { version = "0.8", default-features = false }
 pin-project-lite = "0.2"
 prettyplease = "0.2"

cot/Cargo.toml

@@ -43,6 +43,7 @@ indexmap.workspace = true
 mime_guess.workspace = true
 multer.workspace = true
 password-auth = { workspace = true, features = ["std", "argon2"] }
+secure-string.workspace = true
 pin-project-lite.workspace = true
 schemars = { workspace = true, optional = true }
 sea-query = { workspace = true, optional = true }

cot/src/common_types.rs

@@ -15,6 +15,9 @@ use cot::db::impl_postgres::PostgresValueRef;
 #[cfg(feature = "sqlite")]
 use cot::db::impl_sqlite::SqliteValueRef;
 use email_address::EmailAddress;
+#[cfg(not(miri))]
+use secure_string::SecureString;
+use subtle::ConstantTimeEq;
 
 #[cfg(feature = "db")]
 use crate::db::{ColumnType, DatabaseField, DbValue, FromDbValue, SqlxValueRef, ToDbValue};
@@ -45,30 +48,50 @@ const MAX_EMAIL_LENGTH: u32 = 254;
 ///    compare it with the other password. This method uses constant-time
 ///    equality comparison, which protects against timing attacks.
 ///
-/// 2. An alternative is to use the [`Password::as_str`] method and compare the
-///    strings directly. This approach uses non-constant-time comparison, which
-///    is less secure but may be acceptable in certain legitimate use cases
-///    where the security tradeoff is understood, e.g., when you're creating a
-///    user registration form with the "retype your password" field, where both
-///    passwords come from the same source anyway.
+/// 2. An alternative is to compare 2 instances of the [`Password`] type
+///    directly because this password struct implements the [`PartialEq`] trait
+///    which also uses constant-time comparison. Comparing 2 instances of the
+///    [`Password`] type is less secure than using [`PasswordHash::verify`], but
+///    may be acceptable in certain legitimate use cases where the security
+///    tradeoff is understood, e.g., when you're creating a user registration
+///    form with the "retype your password" field, in this case this approach
+///    might save on hashing costs.
 ///
 /// # Examples
 ///
 /// ```
 /// use cot::auth::Password;
 ///
 /// let password = Password::new("pass");
-/// assert_eq!(&format!("{:?}", password), "Password(\"**********\")");
+/// assert_eq!(&format!("{:?}", password), "Password(***SECRET***)");
+///
+/// let another_password = Password::new("pass");
+/// assert_eq!(password, another_password);
 /// ```
-#[derive(Clone)]
-pub struct Password(String);
-
-impl Debug for Password {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.debug_tuple("Password").field(&"**********").finish()
+#[derive(Clone, Debug)]
+#[cfg(not(miri))]
+pub struct Password(SecureString);
+
+impl PartialEq for Password {
+    fn eq(&self, other: &Self) -> bool {
+        self.as_str()
+            .as_bytes()
+            .ct_eq(other.as_str().as_bytes())
+            .into()
     }
 }
 
+impl Eq for Password {}
+
+/// A password - simplified version for Miri testing.
+///
+/// When running under Miri, we use a simple String wrapper instead of
+/// SecureString to avoid the mlock system call that Miri doesn't support.
+#[derive(Clone)]
+#[cfg(miri)]
+pub struct Password(String);
+
+#[cfg(not(miri))]
 impl Password {
     /// Creates a new password object.
     ///
@@ -81,7 +104,7 @@ impl Password {
     /// ```
     #[must_use]
     pub fn new<T: Into<String>>(password: T) -> Self {
-        Self(password.into())
+        Self(SecureString::from(password.into()))
     }
 
     /// Returns the password as a string.
@@ -96,7 +119,7 @@ impl Password {
     /// ```
     #[must_use]
     pub fn as_str(&self) -> &str {
-        &self.0
+        self.0.unsecure()
     }
 
     /// Consumes the object and returns the password as a string.
@@ -110,6 +133,27 @@ impl Password {
     /// assert_eq!(password.into_string(), "password");
     /// ```
     #[must_use]
+    pub fn into_string(self) -> String {
+        self.0.into_unsecure()
+    }
+}
+
+#[cfg(miri)]
+impl Password {
+    /// Creates a new password object - Miri version.
+    #[must_use]
+    pub fn new<T: Into<String>>(password: T) -> Self {
+        Self(password.into())
+    }
+
+    /// Returns the password as a string - Miri version.
+    #[must_use]
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+
+    /// Consumes the object and returns the password as a string - Miri version.
+    #[must_use]
     pub fn into_string(self) -> String {
         self.0
     }
@@ -133,6 +177,13 @@ impl From<String> for Password {
     }
 }
 
+#[cfg(miri)]
+impl Debug for Password {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Password(***SECRET***)")
+    }
+}
+
 /// A validated email address.
 ///
 /// This is a newtype wrapper around
@@ -422,7 +473,21 @@ mod tests {
     #[test]
     fn password_debug() {
         let password = Password::new("password");
-        assert_eq!(format!("{password:?}"), "Password(\"**********\")");
+        assert_eq!(format!("{password:?}"), "Password(***SECRET***)");
+    }
+
+    #[test]
+    fn password_eq() {
+        let password1 = Password::new("password");
+        let password2 = Password::new("password");
+        assert_eq!(password1, password2);
+    }
+
+    #[test]
+    fn password_ne() {
+        let password1 = Password::new("password");
+        let password2 = Password::new("password2");
+        assert_ne!(password1, password2);
     }
 
     #[test]

cot/src/config.rs

@@ -19,6 +19,8 @@ use std::time::Duration;
 
 use derive_builder::Builder;
 use derive_more::with_trait::{Debug, From};
+#[cfg(not(miri))]
+use secure_string::SecureBytes;
 use serde::{Deserialize, Serialize};
 use subtle::ConstantTimeEq;
 
@@ -819,11 +821,12 @@ impl SessionMiddlewareConfigBuilder {
 ///
 /// # Security
 ///
-/// The implementation of the [`PartialEq`] trait for this type is constant-time
-/// to prevent timing attacks.
+/// The implementation of the [`PartialEq`] trait for this type uses
+/// constant-time comparison to prevent timing attacks.
 ///
-/// The implementation of the [`Debug`] trait for this type hides the secret key
-/// to prevent it from being leaked in logs or other debug output.
+/// The implementation of the [`Debug`] trait for this type is inherited from
+/// [`SecureBytes`], which hides the secret key to prevent it from being leaked
+/// in logs or other debug output.
 ///
 /// # Examples
 ///
@@ -834,10 +837,30 @@ impl SessionMiddlewareConfigBuilder {
 /// assert_eq!(key.as_bytes(), &[1, 2, 3]);
 /// ```
 #[repr(transparent)]
-#[derive(Clone, Serialize, Deserialize)]
+#[derive(Clone, Deserialize, Debug)]
 #[serde(from = "String")]
+#[cfg(not(miri))]
+pub struct SecretKey(SecureBytes);
+
+/// A secret key - simplified version for Miri testing.
+///
+/// When running under Miri, we use a simple Box<[u8]> wrapper instead of
+/// SecureBytes to avoid the mlock system call that Miri doesn't support.
+#[repr(transparent)]
+#[derive(Clone, Deserialize, Debug)]
+#[serde(from = "String")]
+#[cfg(miri)]
 pub struct SecretKey(Box<[u8]>);
 
+impl Serialize for SecretKey {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        serializer.serialize_bytes(self.as_bytes())
+    }
+}
+
 impl SecretKey {
     /// Create a new [`SecretKey`] from a byte array.
     ///
@@ -850,6 +873,14 @@ impl SecretKey {
     /// assert_eq!(key.as_bytes(), &[1, 2, 3]);
     /// ```
     #[must_use]
+    #[cfg(not(miri))]
+    pub fn new(hash: &[u8]) -> Self {
+        Self(SecureBytes::new(hash.to_vec()))
+    }
+
+    /// Create a new [`SecretKey`] from a byte array - Miri version.
+    #[must_use]
+    #[cfg(miri)]
     pub fn new(hash: &[u8]) -> Self {
         Self(Box::from(hash))
     }
@@ -865,6 +896,14 @@ impl SecretKey {
     /// assert_eq!(key.as_bytes(), &[1, 2, 3]);
     /// ```
     #[must_use]
+    #[cfg(not(miri))]
+    pub fn as_bytes(&self) -> &[u8] {
+        self.0.unsecure()
+    }
+
+    /// Get the byte array stored in the [`SecretKey`] - Miri version.
+    #[must_use]
+    #[cfg(miri)]
     pub fn as_bytes(&self) -> &[u8] {
         &self.0
     }
@@ -880,6 +919,15 @@ impl SecretKey {
     /// assert_eq!(key.into_bytes(), Box::from([1, 2, 3]));
     /// ```
     #[must_use]
+    #[cfg(not(miri))]
+    pub fn into_bytes(self) -> Box<[u8]> {
+        self.0.unsecure().to_vec().into_boxed_slice()
+    }
+
+    /// Consume the [`SecretKey`] and return the byte array stored in it - Miri
+    /// version.
+    #[must_use]
+    #[cfg(miri)]
     pub fn into_bytes(self) -> Box<[u8]> {
         self.0
     }
@@ -905,19 +953,12 @@ impl From<&str> for SecretKey {
 
 impl PartialEq for SecretKey {
     fn eq(&self, other: &Self) -> bool {
-        self.0.ct_eq(&other.0).into()
+        self.as_bytes().ct_eq(other.as_bytes()).into()
     }
 }
 
 impl Eq for SecretKey {}
 
-impl Debug for SecretKey {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        // write in single line, regardless whether alternate mode was used or not
-        write!(f, "SecretKey(\"**********\")")
-    }
-}
-
 impl Default for SecretKey {
     fn default() -> Self {
         Self::new(&[])
@@ -1004,6 +1045,8 @@ impl Debug for DatabaseUrl {
 
 #[cfg(test)]
 mod tests {
+    use serde_json;
+
     use super::*;
 
     #[test]
@@ -1078,4 +1121,12 @@ mod tests {
             StaticFilesPathRewriteMode::QueryParam
         );
     }
+
+    #[test]
+    fn secret_key_serialize_json() {
+        let key = SecretKey::from("abc123");
+        let serialized = serde_json::to_string(&key).unwrap();
+        // Should serialize as a byte array
+        assert_eq!(serialized, "[97,98,99,49,50,51]");
+    }
 }

deny.toml

@@ -20,4 +20,5 @@ allow = [
     "0BSD",
     "BSD-3-Clause",
     "Zlib",
+    "Unlicense",
 ]