Repository: duesee/imap-codec (language: python)

Issue:
bug: The `Eq` implementation of `Secret` has unfortunate side-effects.
For example, a sync literal is equal to a non-sync literal.


Discussion / hints:

Fix patch:
diff --git a/assets/demos/tokio_server/src/main.rs b/assets/demos/tokio_server/src/main.rs
index 3ebf78f5..0bf60728 100644
--- a/assets/demos/tokio_server/src/main.rs
+++ b/assets/demos/tokio_server/src/main.rs
@@ -52,7 +52,7 @@ async fn main() {
                     }
                     (tag, CommandBody::Login { username, password }) => {
                         let rsp = if username.as_ref() == b"alice"
-                            && password.compare_ct("password")
+                            && password.compare_with("password")
                         {
                             Response::Status(
                                 Status::ok(Some(tag), None, "LOGIN succeeded").unwrap(),
diff --git a/imap-types/src/imap4rev1/command.rs b/imap-types/src/imap4rev1/command.rs
index dea7d188..c7a39927 100644
--- a/imap-types/src/imap4rev1/command.rs
+++ b/imap-types/src/imap4rev1/command.rs
@@ -27,7 +27,7 @@ use crate::{
     },
     core::{AString, Literal},
     message::{AuthMechanism, Charset, DateTime, Flag, Mailbox, Tag},
-    security::Secret,
+    secret::Secret,
 };
 #[cfg(feature = "ext_enable")]
 use crate::{core::NonEmptyVec, extensions::enable::CapabilityEnable};
@@ -1630,7 +1630,7 @@ mod tests {
         },
         core::{AString, IString, Literal},
         message::{AuthMechanism, Charset, DateTime, Flag, Mailbox, Part, Section},
-        security::Secret,
+        secret::Secret,
     };
     #[cfg(feature = "ext_enable")]
     use crate::{
diff --git a/imap-types/src/lib.rs b/imap-types/src/lib.rs
index 1fda6d2e..b6bc0d51 100644
--- a/imap-types/src/lib.rs
+++ b/imap-types/src/lib.rs
@@ -112,7 +112,7 @@
 #[cfg(feature = "arbitrary")]
 mod arbitrary;
 mod imap4rev1;
-pub mod security;
+pub mod secret;
 pub mod utils;
 
 // -- API -----------------------------------------------------------------------------------
diff --git a/imap-types/src/security.rs b/imap-types/src/secret.rs
similarity index 51%
rename from imap-types/src/security.rs
rename to imap-types/src/secret.rs
index 9942690d..e5ed6b7b 100644
--- a/imap-types/src/security.rs
+++ b/imap-types/src/secret.rs
@@ -1,4 +1,7 @@
-use std::fmt::{Debug, Formatter};
+use std::{
+    borrow::Cow,
+    fmt::{Debug, Formatter},
+};
 
 #[cfg(feature = "arbitrary")]
 use arbitrary::Arbitrary;
@@ -8,6 +11,8 @@ use bounded_static::ToStatic;
 use serde::{Deserialize, Serialize};
 use subtle::ConstantTimeEq;
 
+use crate::core::{AString, IString, Literal};
+
 #[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
 #[cfg_attr(feature = "bounded-static", derive(ToStatic))]
 #[cfg_attr(feature = "serde", derive(Deserialize, Serialize))]
@@ -17,12 +22,15 @@ use subtle::ConstantTimeEq;
 #[derive(Clone, Hash)]
 pub struct Secret<T>(T);
 
+/// A trait to ensure that secrets are neither logged nor compared in non-constant time.
 impl<T> Secret<T> {
+    /// Crate a new secret.
     pub fn new(inner: T) -> Self {
         Self(inner)
     }
 
-    pub fn expose_secret(&self) -> &T {
+    /// Expose the inner secret (opting-out of all guarantees).
+    pub fn declassify(&self) -> &T {
         &self.0
     }
 }
@@ -33,47 +41,124 @@ impl<T> From<T> for Secret<T> {
     }
 }
 
-impl<S> Secret<S>
+impl<T> Secret<T>
 where
-    S: AsRef<[u8]>,
+    T: AsRef<[u8]>,
 {
-    pub fn compare_ct<O>(&self, other: O) -> bool
+    /// Compare this secret value with another value in constant time.
+    ///
+    /// Note: The comparison is made by converting both values as bytes first.
+    pub fn compare_with<B>(&self, other: B) -> bool
     where
-        O: AsRef<[u8]>,
+        B: AsRef<[u8]>,
     {
-        self.0.as_ref().ct_eq(other.as_ref()).unwrap_u8() == 1
+        self.declassify().as_ref().ct_eq(other.as_ref()).unwrap_u8() == 1
     }
 }
 
-impl<T> Debug for Secret<T> {
+impl<T> PartialEq for Secret<T>
+where
+    T: CompareCT<T>,
+{
+    fn eq(&self, other: &Self) -> bool {
+        self.declassify().compare_ct(&other.0)
+    }
+}
+
+impl<T> Eq for Secret<T> where T: CompareCT<T> {}
+
+impl<T> Debug for Secret<T>
+where
+    T: Debug,
+{
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        write!(f, "/* REDACTED */")
+        #[cfg(not(debug_assertions))]
+        return write!(f, "/* REDACTED */");
+        #[cfg(debug_assertions)]
+        return self.0.fmt(f);
     }
 }
 
-impl<T> Eq for Secret<T> where T: AsRef<[u8]> {}
+pub trait CompareCT<T> {
+    #[must_use]
+    fn compare_ct(&self, other: &T) -> bool;
+}
+
+impl<'a, T> CompareCT<T> for Cow<'a, [u8]>
+where
+    T: AsRef<[u8]>,
+{
+    fn compare_ct(&self, other: &T) -> bool {
+        self.as_ref().ct_eq(other.as_ref()).unwrap_u8() == 1
+    }
+}
 
-impl<T> PartialEq for Secret<T>
+impl<T> CompareCT<T> for Vec<u8>
 where
     T: AsRef<[u8]>,
 {
-    fn eq(&self, other: &Self) -> bool {
-        self.0.as_ref().ct_eq(other.0.as_ref()).unwrap_u8() == 1
+    fn compare_ct(&self, other: &T) -> bool {
+        self.as_slice().ct_eq(other.as_ref()).unwrap_u8() == 1
+    }
+}
+
+impl<'a> CompareCT<AString<'a>> for AString<'a> {
+    fn compare_ct(&self, other: &AString<'a>) -> bool {
+        match (self, other) {
+            (AString::Atom(lhs), AString::Atom(rhs)) => {
+                lhs.as_ref()
+                    .as_bytes()
+                    .ct_eq(rhs.as_ref().as_bytes())
+                    .unwrap_u8()
+                    == 1
+            }
+            (AString::String(lhs), AString::String(rhs)) => lhs.compare_ct(rhs),
+            _ => false,
+        }
+    }
+}
+
+impl<'a> CompareCT<IString<'a>> for IString<'a> {
+    fn compare_ct(&self, other: &IString<'a>) -> bool {
+        match (self, other) {
+            (IString::Quoted(lhs), IString::Quoted(rhs)) => {
+                lhs.as_ref()
+                    .as_bytes()
+                    .ct_eq(rhs.as_ref().as_bytes())
+                    .unwrap_u8()
+                    == 1
+            }
+            (IString::Literal(lhs), IString::Literal(rhs)) => lhs.compare_ct(rhs),
+            _ => false,
+        }
+    }
+}
+
+impl<'a> CompareCT<Literal<'a>> for Literal<'a> {
+    fn compare_ct(&self, other: &Literal<'a>) -> bool {
+        #[cfg(not(feature = "ext_literal"))]
+        return self.as_ref().ct_eq(other.as_ref()).unwrap_u8() == 1;
+        #[cfg(feature = "ext_literal")]
+        return self.as_ref().ct_eq(other.as_ref()).unwrap_u8() == 1 && self.sync == other.sync;
     }
 }
 
 #[cfg(test)]
 mod tests {
     #[cfg(feature = "ext_literal")]
-    use crate::message::AuthMechanism;
+    use crate::core::Literal;
     use crate::{
-        command::{AuthenticateData, CommandBody},
-        security::Secret,
+        command::{Command, CommandBody},
+        core::{AString, Atom, Quoted},
+        secret::Secret,
     };
 
     #[test]
+    #[cfg(not(debug_assertions))]
     #[allow(clippy::redundant_clone)]
     fn test_that_secret_is_redacted() {
+        use crate::{command::AuthenticateData, message::AuthMechanism};
+
         let secret = Secret("xyz123");
         let got = format!("{:?}", secret);
         println!("{}", got);
@@ -123,7 +208,7 @@ mod tests {
     #[allow(clippy::redundant_clone)]
     fn test_that_eq_is_constant_time() {
         let took_constant = {
-            fn compare_eq(a: Secret<String>, b: Secret<String>) -> u128 {
+            fn compare_eq(a: Secret<AString>, b: Secret<AString>) -> u128 {
                 let tik = std::time::Instant::now();
                 assert_eq!(a, b);
                 let tok = std::time::Instant::now();
@@ -131,7 +216,7 @@ mod tests {
                 tok.duration_since(tik).as_nanos()
             }
 
-            fn compare_ne(a: Secret<String>, b: Secret<String>) -> u128 {
+            fn compare_ne(a: Secret<AString>, b: Secret<AString>) -> u128 {
                 let tik = std::time::Instant::now();
                 assert_ne!(a, b);
                 let tok = std::time::Instant::now();
@@ -139,8 +224,14 @@ mod tests {
                 tok.duration_since(tik).as_nanos()
             }
 
-            let a = Secret::new(str::repeat("A", 1024 * 1024));
-            let b = Secret::new(str::repeat("B", 1024 * 1024));
+            let a = Secret::new(AString::from(Atom::unchecked(str::repeat(
+                "A",
+                1024 * 1024,
+            ))));
+            let b = Secret::new(AString::from(Atom::unchecked(str::repeat(
+                "B",
+                1024 * 1024,
+            ))));
 
             let took1 = compare_eq(a.clone(), a.clone());
             println!("{}", took1);
@@ -193,78 +284,45 @@ mod tests {
         }
     }
 
-    /// A best effort test to ensure that constant-time comparison works.
     #[test]
-    #[allow(clippy::redundant_clone)]
-    fn test_that_compare_ct_is_constant_time() {
-        let took_constant = {
-            fn compare_eq(a: Secret<String>, b: Secret<String>) -> u128 {
-                let tik = std::time::Instant::now();
-                assert!(a.compare_ct(b.expose_secret()));
-                let tok = std::time::Instant::now();
-
-                tok.duration_since(tik).as_nanos()
-            }
-
-            fn compare_ne(a: Secret<String>, b: Secret<String>) -> u128 {
-                let tik = std::time::Instant::now();
-                assert!(!a.compare_ct(b.expose_secret()));
-                let tok = std::time::Instant::now();
-
-                tok.duration_since(tik).as_nanos()
-            }
-
-            let a = Secret::new(str::repeat("A", 1024 * 1024));
-            let b = Secret::new(str::repeat("B", 1024 * 1024));
-
-            let took1 = compare_eq(a.clone(), a.clone());
-            println!("{}", took1);
-            let took2 = compare_ne(a.clone(), b.clone());
-            println!("{}", took2);
-            let took3 = compare_ne(b.clone(), a.clone());
-            println!("{}", took3);
-            let took4 = compare_eq(b.clone(), b.clone());
-            println!("{}", took4);
-
-            (took1 + took2 + took3 + took4) / 4
-        };
-
-        let took_variable = {
-            fn compare_eq(a: String, b: String) -> u128 {
-                let tik = std::time::Instant::now();
-                assert_eq!(a, b);
-                let tok = std::time::Instant::now();
-
-                tok.duration_since(tik).as_nanos()
-            }
-
-            fn compare_ne(a: String, b: String) -> u128 {
-                let tik = std::time::Instant::now();
-                assert_ne!(a, b);
-                let tok = std::time::Instant::now();
-
-                tok.duration_since(tik).as_nanos()
-            }
-
-            let a = str::repeat("A", 1024 * 1024);
-            let b = str::repeat("B", 1024 * 1024);
-
-            let took1 = compare_eq(a.clone(), a.clone());
-            println!("{}", took1);
-            let took2 = compare_ne(a.clone(), b.clone());
-            println!("{}", took2);
-            let took3 = compare_ne(b.clone(), a.clone());
-            println!("{}", took3);
-            let took4 = compare_eq(b.clone(), b.clone());
-            println!("{}", took4);
-
-            (took1 + took2 + took3 + took4) / 4
-        };
-
-        let times = took_constant / took_variable;
-        println!("{took_constant} vs {took_variable} ({times} times slower)");
-        if times < 10 {
-            panic!("expected slowdown >= 10, got {}", times);
-        }
+    fn test_that_secret_has_no_side_effects_on_eq() {
+        assert_ne!(
+            Command::new(
+                "A",
+                CommandBody::login(
+                    AString::from(Atom::unchecked("user")),
+                    AString::from(Atom::unchecked("pass")),
+                )
+                .unwrap(),
+            ),
+            Command::new(
+                "A",
+                CommandBody::login(
+                    AString::from(Atom::unchecked("user")),
+                    AString::from(Quoted::unchecked("pass")),
+                )
+                .unwrap(),
+            )
+        );
+
+        #[cfg(feature = "ext_literal")]
+        assert_ne!(
+            Command::new(
+                "A",
+                CommandBody::login(
+                    Literal::try_from("").unwrap(),
+                    Literal::try_from("A").unwrap(),
+                )
+                .unwrap(),
+            ),
+            Command::new(
+                "A",
+                CommandBody::login(
+                    Literal::try_from("").unwrap(),
+                    Literal::try_from("A").unwrap().into_non_sync(),
+                )
+                .unwrap(),
+            )
+        );
     }
 }
diff --git a/src/codec/encode.rs b/src/codec/encode.rs
index 773e5e15..051b99d3 100644
--- a/src/codec/encode.rs
+++ b/src/codec/encode.rs
@@ -96,10 +96,10 @@ impl<'a> Encode for CommandBody<'a> {
                     // RFC 4959 (https://datatracker.ietf.org/doc/html/rfc4959#section-3)
                     // "To send a zero-length initial response, the client MUST send a single pad character ("=").
                     // This indicates that the response is present, but is a zero-length string."
-                    if ir.expose_secret().is_empty() {
+                    if ir.declassify().is_empty() {
                         writer.write_all(b"=")?;
                     } else {
-                        writer.write_all(base64.encode(ir.expose_secret()).as_bytes())?;
+                        writer.write_all(base64.encode(ir.declassify()).as_bytes())?;
                     };
                 };
 
@@ -110,7 +110,7 @@ impl<'a> Encode for CommandBody<'a> {
                 writer.write_all(b" ")?;
                 username.encode(writer)?;
                 writer.write_all(b" ")?;
-                password.expose_secret().encode(writer)
+                password.declassify().encode(writer)
             }
             CommandBody::Select { mailbox } => {
                 writer.write_all(b"SELECT")?;
@@ -360,7 +360,7 @@ impl<'a> Encode for AuthMechanismOther<'a> {
 
 impl Encode for AuthenticateData {
     fn encode(&self, writer: &mut impl Write) -> std::io::Result<()> {
-        let encoded = base64.encode(self.0.expose_secret());
+        let encoded = base64.encode(self.0.declassify());
         writer.write_all(encoded.as_bytes())
     }
 }
diff --git a/src/imap4rev1/command.rs b/src/imap4rev1/command.rs
index 7ba4b59c..0b6e48bf 100644
--- a/src/imap4rev1/command.rs
+++ b/src/imap4rev1/command.rs
@@ -10,7 +10,7 @@ use imap_types::{
     },
     core::AString,
     message::{AuthMechanism, Flag},
-    security::Secret,
+    secret::Secret,
 };
 use nom::{
     branch::alt,
diff --git a/src/lib.rs b/src/lib.rs
index b8b9ce0f..bca0b935 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -117,7 +117,7 @@ mod imap4rev1;
 #[cfg(test)]
 mod testing;
 
-pub use imap_types::{command, core, message, response, security, state, utils};
+pub use imap_types::{command, core, message, response, secret, state, utils};
 
 // ----------- Compatibility modules -----------
 
diff --git a/src/tokio/server.rs b/src/tokio/server.rs
index da1a8d46..3175052e 100644
--- a/src/tokio/server.rs
+++ b/src/tokio/server.rs
@@ -191,7 +191,7 @@ mod tests {
     use imap_types::{
         command::{Command, CommandBody},
         core::{AString, AtomExt, IString, Literal},
-        security::Secret,
+        secret::Secret,
     };
     use tokio_util::codec::Decoder;
 


Test patch:
diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
index 90f5defb..8928c9f5 100644
--- a/.github/workflows/build_and_test.yml
+++ b/.github/workflows/build_and_test.yml
@@ -55,6 +55,7 @@ jobs:
           do
             for feature_mode in "" "--all-features";
             do
+              echo "# Testing" ${build_mode} ${feature_mode}
               cargo test --workspace ${build_mode} ${feature_mode} --doc
               cargo test --workspace ${build_mode} ${feature_mode} --all-targets --exclude imap-codec-fuzz --exclude imap-types-fuzz
             done
@@ -68,6 +69,7 @@ jobs:
 
           foreach ($build_mode in $build_modes) {
             foreach ($feature_mode in $feature_modes) {
+              echo "# Testing" ${build_mode} ${feature_mode}
               cargo test --workspace ${build_mode} ${feature_mode} --doc
               cargo test --workspace ${build_mode} ${feature_mode} --all-targets --exclude imap-codec-fuzz --exclude imap-types-fuzz
             }
diff --git a/imap-types/tests/readme.rs b/imap-types/tests/readme.rs
index 79d08eba..f78e696b 100644
--- a/imap-types/tests/readme.rs
+++ b/imap-types/tests/readme.rs
@@ -6,7 +6,7 @@ use imap_types::{
 use imap_types::{
     core::{AString, Atom},
     message::Tag,
-    security::Secret,
+    secret::Secret,
 };
 
 #[test]
diff --git a/tests/trace.rs b/tests/trace.rs
index 0c19952d..3116eacd 100644
--- a/tests/trace.rs
+++ b/tests/trace.rs
@@ -14,7 +14,7 @@ use imap_codec::{
         },
         Code, Data, Greeting, Response, Status,
     },
-    security::Secret,
+    secret::Secret,
 };
 
 enum Who {