Complete re-do of Secret and Zeroize

alpha_0.1.2_release_notes.md

@@ -2,36 +2,13 @@
 
 [remove this section before publication]
 
-* ML-DSA & ML-KEM
-    * Check the crate release checklist and run claude against the style guide (maybe Francis could cross-check me)
-    * Run Crucible testing
-    * Add factories for ML-DSA and ML-KEM (if we are keeping factories, see below)
-    * After merging the Signer/Verifier, Encrypter/Decrypter split, check if the keygen_from_rng() is still on the right
-      trait.
-* Split the Signature trait into a Signer and a Verifier so that, for example, we can implement the verifier for MTC in
-  a different struct from the signer; or so that you can get FIPS compliance on old algorithms that are currently only
-  FIPS-allowed for verification of existing signatures but not for creation of new ones.
-* Check out Megan's email May 13 about KeyMaterial: "I was wondering if there might be scope for a closure based
-  approach that could guarantee encapsulation of the state change from safe to hazardous back to safe again."
-* Go back to previous algs and apply memory optimization tricks like internal functions. And add a docs section "Memory
-  Usage" that measures with valgrind.
-* Ensure that all crates have `#![forbid(missing_docs)]`
-* Apply Secret trait consistently across the library --> study the `Zeroize` trait in RustCrypto
-* Change all "[u8;0]" to "[]" throughout the code and docs ... or better yet, change the APIs to take an Option<>
-* Change all `-> Vec<u8>` to `-> [u8; CONST_LEN]`, and the `output: &mut [u8]` to `output: &mut [u8; CONST_LEN]` where
-  appropriate.
-* Probably it makes sense to leave Hex and Base64 as requiring std; ... or maybe add a no_std version that uses
-  fixed-sized blocks?
-* Create a cargo feature #[cfg(feature='rng')] and put it around things like keygen that takes an rng so that the build
-  dependency on bouncycastle_rng is optional.
-* Factories ... Are they worth it? Michael Richardson says Very Yes. If we are keeping them, then we need a serious
-  re-engineering of them because I really dislike that currently they make it hard for the underlying primitive to have
-  static one-shot APIs.
 * Deal with as many of the inline TODOs as possible
 * Close all open github issues and document them in this file.
+* Clean up `cargo doc --all` warnings
+* On release/0.1.2alpha, get Claude to make a cosmetic change to the docs for all crates to use [`Secret`] instead
+  of [Secret] because this renders better.
 * After everything is merged, circle back to crucible, and make sure that the harness still works
-* Search for all the uses of .unwrap() in non-test code and replace each with either a comment or an expect with a
-  meaningful error string.
+* Delete this file and stuff it into the Description of a github Release.
 
 # 0.1.2 Features / Changelog
 
@@ -47,16 +24,47 @@
   potentially across versions of the library. The intended use case is if you are processing a large input that depends
   on one or more network round-trips and you wish to suspend to a cache and potentially transfer to a new host while
   waiting for network IO.
+* dyn RNG: anywhere that consumes randomness (such as keygen and non-deterministic sign / encaps functions) can now be
+  handed an instance of an object that impl's `bouncycastle-core::traits::RNG`.
+* Rework of the Secret system for protecting secret data against leakage via returning to the memory pool unzeroized,
+  or being logged in debug messages, stack traces, and crash dumps. Now properly uses `core::mem::write_volatile` to
+  prevent
+  the compiler from eliding writes on drop, and introduced a new type system `Secret<T>` that is used across the library
+  to give more fine-grained control over which objects (and which fields within objects) get this extra protection.
+  Bonus: this is a public type that you can use to protect your application data as well!
 
 ## Minor features / bug fixes
 
-* All public `*_out(.., out: &mut [u8])` functions now begin by zeroizing the entire output buffer with `.fill(0)`,
-  preventing exposure of stale data in oversized output buffers or on early error returns.
+Trait system:
+
+* Split the Signature trait into a Signer and a Verifier trait. This is for two reasons: 1) some of the future signature
+  algorithms (like hash-based signatures) the verifier code is substantially lighter than the signer code, or we may not
+  even want to implement a signer in software, and 2) NIST likes to soft-deprecate algorithms by disallowing generation
+  of new signatures, but still allowing verification of existing signatures.
+* Added traits for symmetric ciphers in the block cipher, stream cipher, and AEAD families. We don't have any of these
+  algorithms implemented yet, but they're coming!
+
+The KeyMaterial object:
+
 * Reworked the way KeyMaterial hazardous operations work; instead of a stateful .allow_hazardous_operations() /
   .drop_hazardous_operations(), it now uses a closure-based do_hazardous_operations(). Github issue #39.
 * Renamed KeyMaterial::KeyType's and deleted KeyMaterial::concatenate in order to give a better intuition and
   FIPS-alignment.
-* Removed the dependence on nightly / experimental compiler features; the library now buildds on stable.
-* Github issues resolved:
-    * #6: https://github.com/bcgit/bc-rust/issues/6, thanks to Q. T. Felix (github: @Quant-TheodoreFelix)
-    * #10: https://github.com/bcgit/bc-rust/issues/10, thanks to Nicola Tuveri (github: @romen)
+* Tightened up the entropy-tracking behaviour of the KeyMaterial object, thanks to Q. T. Felix (github:
+  @Quant-TheodoreFelix, github issue #6)
+
+Docs:
+
+* All crypto algorithm crates now have Memory Usage docs that list the stack memory usage of the implementation.
+* All crypto algorithm crates now have `#![forbid(missing_docs)]` to ensure that they have a fully-documented public
+  API.
+
+* Other miscellaneous Github issues resolved:
+    * #10: https://github.com/bcgit/bc-rust/issues/10, thanks to Nicola Tuveri (github: @romen)
+    * #18: All public `*_out(.., out: &mut [u8])` functions now begin by zeroizing the entire provided output buffer
+      with `.fill(0)`,
+      preventing exposure of stale data in oversized output buffers or on early error returns. Thanks to Q. T. Felix (
+      github: @Quant-TheodoreFelix)
+    * #27: "SHAKE absorb-after-squeeze": clarified and hardened the behaviour of SHAKE with respect to absorbing more
+      input after having been squeezed.
+    * #28: Removed the dependence on nightly / experimental compiler features; the library now builds on stable.

crypto/core/src/key_material.rs

@@ -51,8 +51,8 @@
 //! See [do_hazardous_operations] for documentation and sample code.
 
 use crate::errors::{KeyMaterialError, SuspendableError};
-use crate::traits::{RNG, Secret, SecurityStrength};
-use bouncycastle_utils::{ct, min};
+use crate::traits::{RNG, SecurityStrength};
+use bouncycastle_utils::{ct, min, secret::Secret};
 
 use core::cmp::{Ordering, PartialOrd};
 use core::fmt;
@@ -215,15 +215,13 @@ pub trait KeyMaterialTrait: KeyMaterialInternalTrait {
 /// The capacity of the internal buffer can be set at compile-time via the <KEY_LEN> param.
 #[derive(Clone)]
 pub struct KeyMaterial<const KEY_LEN: usize> {
-    buf: [u8; KEY_LEN],
-    key_len: usize,
+    buf: Secret<[u8; KEY_LEN]>,
+    key_len: Secret<usize>,
     key_type: KeyType,
     security_strength: SecurityStrength,
     allow_hazardous_operations: bool,
 }
 
-impl<const KEY_LEN: usize> Secret for KeyMaterial<KEY_LEN> {}
-
 // The explicit `#[repr(u8)]` discriminants are the stable on-the-wire encoding used by
 // `SerializableState` implementations (see the `TryFrom<u8>` impl below). Pin each value to its
 // variant name: reordering variants is fine, but never reuse or renumber an existing discriminant,
@@ -287,8 +285,8 @@ impl<const KEY_LEN: usize> KeyMaterial<KEY_LEN> {
     /// If you want a properly populated instance, use [KeyMaterial::from_rng].
     pub fn new() -> Self {
         Self {
-            buf: [0u8; KEY_LEN],
-            key_len: 0,
+            buf: Secret::new(),
+            key_len: Secret::new(),
             key_type: KeyType::Zeroized,
             security_strength: SecurityStrength::None,
             allow_hazardous_operations: false,
@@ -305,7 +303,7 @@ impl<const KEY_LEN: usize> KeyMaterial<KEY_LEN> {
             Ok(())
         })?;
 
-        key.key_len = KEY_LEN;
+        *key.key_len = KEY_LEN;
         key.key_type = KeyType::CryptographicRandom;
         key.security_strength = rng.security_strength();
         Ok(key)
@@ -347,14 +345,11 @@ impl<const KEY_LEN: usize> KeyMaterial<KEY_LEN> {
             return Err(KeyMaterialError::InputDataLongerThanKeyCapacity);
         }
 
-        let mut key = Self {
-            buf: [0u8; KEY_LEN],
-            key_len: other.key_len(),
-            key_type: other.key_type(),
-            security_strength: SecurityStrength::None,
-            allow_hazardous_operations: false,
-        };
+        let mut key = Self::new();
         key.buf[..other.key_len()].copy_from_slice(other.ref_to_bytes());
+        *key.key_len = other.key_len();
+        key.key_type = other.key_type();
+        key.security_strength = other.security_strength();
         Ok(key)
     }
 }
@@ -378,7 +373,7 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
         };
 
         self.buf[..source.len()].copy_from_slice(source);
-        self.key_len = source.len();
+        *self.key_len = source.len();
         self.key_type = new_key_type;
 
         do_hazardous_operations(self, |s| {
@@ -399,22 +394,22 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
     }
 
     fn ref_to_bytes(&self) -> &[u8] {
-        &self.buf[..self.key_len]
+        &self.buf[..*self.key_len]
     }
 
     fn ref_to_bytes_mut(&mut self) -> Result<&mut [u8], KeyMaterialError> {
         if !self.allow_hazardous_operations {
             return Err(KeyMaterialError::HazardousOperationNotPermitted);
         }
-        Ok(&mut self.buf)
+        Ok(self.buf.as_mut())
     }
 
     fn capacity(&self) -> usize {
         KEY_LEN
     }
 
     fn key_len(&self) -> usize {
-        self.key_len
+        *self.key_len
     }
 
     fn set_key_len(&mut self, key_len: usize) -> Result<(), KeyMaterialError> {
@@ -423,7 +418,7 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
         }
 
         // are we extending the key length, or truncating?
-        if key_len <= self.key_len {
+        if key_len <= *self.key_len {
             // truncation is always allowed (not hazardous)
 
             self.security_strength =
@@ -433,14 +428,14 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
                 self.key_type = KeyType::Zeroized;
             }
 
-            self.key_len = key_len;
+            *self.key_len = key_len;
 
             Ok(())
         } else {
             if !self.allow_hazardous_operations {
                 return Err(KeyMaterialError::HazardousOperationNotPermitted);
             }
-            self.key_len = key_len;
+            *self.key_len = key_len;
             Ok(())
         }
     }
@@ -557,8 +552,8 @@ impl<const KEY_LEN: usize> KeyMaterialTrait for KeyMaterial<KEY_LEN> {
     }
 
     fn zeroize(&mut self) {
-        self.buf.fill(0u8);
-        self.key_len = 0;
+        self.buf.zeroize();
+        self.key_len.zeroize();
         self.key_type = KeyType::Zeroized;
     }
 
@@ -579,7 +574,7 @@ impl<const KEY_LEN: usize> PartialEq for KeyMaterial<KEY_LEN> {
         if self.key_len != other.key_len {
             return false;
         }
-        ct::ct_eq_bytes(&self.buf[..self.key_len], &other.buf[..self.key_len])
+        ct::ct_eq_bytes(&self.buf[..*self.key_len], &other.buf[..*self.key_len])
     }
 }
 impl<const KEY_LEN: usize> Eq for KeyMaterial<KEY_LEN> {}
@@ -618,32 +613,27 @@ impl PartialOrd for KeyType {
 /// Block accidental logging of the internal key material buffer.
 impl<const KEY_LEN: usize> fmt::Display for KeyMaterial<KEY_LEN> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // deref the key_len explicitly so that Secret doesn't render it as "<redacted>"
         write!(
             f,
-            "KeyMaterial {{ len: {}, key_type: {:?}, security_strength: {:?} }}",
-            self.key_len, self.key_type, self.security_strength
+            "KeyMaterial<{}>{{ len: {}, key_type: {:?}, security_strength: {:?} }}",
+            KEY_LEN, *self.key_len, self.key_type, self.security_strength
         )
     }
 }
 
 /// Block accidental logging of the internal key material buffer.
 impl<const KEY_LEN: usize> fmt::Debug for KeyMaterial<KEY_LEN> {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // deref the key_len explicitly so that Secret doesn't render it as "<redacted>"
         write!(
             f,
-            "KeyMaterial {{ len: {}, key_type: {:?}, security_strength: {:?} }}",
-            self.key_len, self.key_type, self.security_strength
+            "KeyMaterial<{}>{{ len: {}, key_type: {:?}, security_strength: {:?} }}",
+            KEY_LEN, *self.key_len, self.key_type, self.security_strength
         )
     }
 }
 
-/// Zeroize the key material on drop.
-impl<const KEY_LEN: usize> Drop for KeyMaterial<KEY_LEN> {
-    fn drop(&mut self) {
-        self.zeroize()
-    }
-}
-
 /* Hazardous Operations Runner */
 
 /// Internal-use trait holding the low-level hazardous-operations guard toggle.

crypto/core/src/traits.rs

@@ -36,9 +36,7 @@ pub trait AlgorithmOID {
 /// ciphertexts that are incompatible with other implementations as ciphers in more complex modes, such
 /// as AEADs or stream ciphers may need to stick extra data either at the beginning or end of the ciphertext.
 /// See the documentation of the underlying implementation for more details.
-pub trait SymmetricCipher<const KEY_LEN: usize, const INIT_DATA_LEN: usize>:
-    Algorithm + Secret
-{
+pub trait SymmetricCipher<const KEY_LEN: usize, const INIT_DATA_LEN: usize>: Algorithm {
     #[cfg(feature = "std")]
     /// A one-shot API to encrypt some plaintext with the given key.
     /// This function returns the ciphertext as a Vec<u8>, and therefore is only available when compiling with std.
@@ -522,7 +520,7 @@ pub trait KEMPublicKey<const PK_LEN: usize>:
 }
 
 /// A private key for a KEM algorithm, often denoted "sk" (for "secret key").
-pub trait KEMPrivateKey<const SK_LEN: usize>: PartialEq + Eq + Clone + Secret + Sized {
+pub trait KEMPrivateKey<const SK_LEN: usize>: PartialEq + Eq + Clone + Sized {
     /// Write it out to bytes in its standard encoding.
     fn encode(&self) -> [u8; SK_LEN];
     /// Write it out to bytes in its standard encoding.
@@ -768,14 +766,6 @@ pub trait RNG {
     fn security_strength(&self) -> SecurityStrength;
 }
 
-/// A trait that forces an object to implement a zeroizing Drop() as well as Debug and Display that
-/// will not log the sensitive contents, even in error or crash-dump scenarios.
-// Since rust auto-implements Drop, there's a lint that explicitly bounding on Drop is useless.
-// I disagree because I want to force things that are secrets to manually implement Drop that zeroizes the data.
-// So I'm turning off this lint.
-#[allow(drop_bounds)]
-pub trait Secret: Drop + Debug + Display {}
-
 /// Allows a stateful object to suspend its operation by serializing its state into a byte array
 ///so that it can be resumed later, potentially from a different host.
 ///
@@ -925,9 +915,7 @@ pub trait SignaturePublicKey<const PK_LEN: usize>:
 }
 
 /// A private key for a signature algorithm, often denoted "sk" (for "secret key").
-pub trait SignaturePrivateKey<const SK_LEN: usize>:
-    PartialEq + Eq + Clone + Secret + Sized
-{
+pub trait SignaturePrivateKey<const SK_LEN: usize>: PartialEq + Eq + Clone + Sized {
     /// Write it out to bytes in its standard encoding.
     fn encode(&self) -> [u8; SK_LEN];
     /// Write it out to bytes in its standard encoding.