diff --git a/cli/src/helpers.rs b/cli/src/helpers.rs index 62a7741..36fd1fa 100644 --- a/cli/src/helpers.rs +++ b/cli/src/helpers.rs @@ -17,7 +17,7 @@ pub(crate) fn read_from_file(filename: &str) -> Vec { match hex::decode(&buf) { Ok(decoded) => decoded, Err(_) => { - // well, it's not hex, so return it raw + // it's not hex, so return it raw buf } } @@ -47,7 +47,7 @@ pub(crate) fn read_from_file_or_stdin(filename: &Option) -> Vec { match hex::decode(&buf) { Ok(decoded) => decoded, Err(_) => { - // well, it's not hex, so return it raw + // it's not hex, so return it raw buf } } @@ -98,7 +98,7 @@ pub(crate) fn parse_seed(bytes: &[u8]) -> Result::from_bytes_as_type(&seed_bytes, KeyType::Seed).unwrap(); if seed.key_type() == KeyType::Zeroized || seed.security_strength() < SecurityStrength::_256bit diff --git a/cli/src/mldsa_cmd.rs b/cli/src/mldsa_cmd.rs index 8a1a4e3..82c7e8c 100644 --- a/cli/src/mldsa_cmd.rs +++ b/cli/src/mldsa_cmd.rs @@ -1,5 +1,5 @@ -//! Yup, this file is as absolutely atrocious mess of duplicate code that could be much improved -//! by using generics or macros. I just, haven't ... yet. +//! Work in progress. +//! TODO: Use generic macros to eliminate duplicated code. use crate::helpers::{parse_seed, read_from_file, read_from_file_or_stdin, write_bytes_or_hex}; use bouncycastle::core::traits::{Signature, SignaturePrivateKey, SignaturePublicKey}; diff --git a/crypto/base64/src/lib.rs b/crypto/base64/src/lib.rs index b3e57d8..bf47f03 100644 --- a/crypto/base64/src/lib.rs +++ b/crypto/base64/src/lib.rs @@ -1,7 +1,8 @@ //! Good old fashioned base64 encoder and decoder. //! -//! It should just work the way you expect: [encode] takes any bytes-like rust type -//! and returns a String, while [decode] takes a String (which can be in any bytes-like container) +//! It should just work the way it is normally expected: +//! [encode] takes any bytes-like rust type and returns a String, +//! while [decode] takes a String (which can be in any bytes-like container) //! and returns a `Vec`. //! //!``` @@ -31,10 +32,10 @@ //! Unlike Hex, Base64 does not align cleanly to byte boundaries. //! That means that the above one-shot APIs should only be used if you have the entire content to //! process at the same time. -//! In other words, if you arbitrarily break your data into chunks and hand it to the one-shot [encode] and [decode] APIs, -//! you will get incorrect results. -//! If you need to process your data in chunks, you need to use the streaming API that allows -//! repeated calls to `do_update`, producing output as it goes, and correctly holds on to the unprocessed +//! In other words, if data is arbitrarily broken into chunks and handed to the one-shot [encode] and [decode] APIs, +//! the results obtained will be incorrect. +//! Whenever it is necessary to process data in chunks, the streaming API that allows repeated calls to `do_update` +//! must be used. This produces output as it goes, and correctly holds on to the unprocessed //! partial block until either `do_update` or `do_final` is called. //! //! ``` @@ -60,15 +61,15 @@ //! //! > [Util::Lookup: Exploiting key decoding in cryptographic libraries (Sieck, 2021)](https://arxiv.org/pdf/2108.04600.pdf), //! -//! As this is a cryptography library, we are assuming that this base64 implementation will be used to encode +//! As this is a cryptography library, it can be assumed that this base64 implementation will be used to encode //! and decode private keys in PEM and JWK formats and so we are only providing a constant-time implementation //! in order to remove the temptation to shoot yourself in the foot in the name of a small performance gain. //! -//! In our testing, a naïve lookup table-based implementation of base64::decode was 1.7x faster than -//! our constant-time implementation, and we are quite sure that optimized base64 implementations exist that -//! provide still better performance. -//! So if you find yourself in a position of needing to base64 encode gigabytes of non-sensitive data, then -//! we recommend you use one of the good, fast, but non-constant-time base64 implementations available from other projects. +//! During testing, a naïve lookup table-based implementation of base64::decode was 1.7x faster than +//! a constant-time implementation. +//! We are quite sure that optimized base64 implementations exist that provide still better performance. +//! It is necessary to encode gigabytes of non-sensitive data on base64, it is advised to use +//! one of the good, fast, but non-constant-time base64 implementations available from other projects. //! //! //! # Alphabets: @@ -100,7 +101,7 @@ pub enum Base64Error { /// pass the same input to do_final(). Note that do_final() is tolerant of incomplete padding blocks, /// so even if an additional padding character is contained in the next chunk of input, do_final() will still produce /// the correct output -- ie any additional chunks held by the caller can be discarded. - PaddingEnconteredDuringDoUpdate, + PaddingEncounteredDuringDoUpdate, /// Input contained a character that was not in the base64 alphabet. The index of the illegal character is included in the output. InvalidB64Character(usize), @@ -288,7 +289,7 @@ impl Base64Decoder { i += 1; self.vals_in_buf += 1; - // here we get to assume that the buffer contains no padding. + // here, it can be assumed that the buffer contains no padding. if self.vals_in_buf == 4 { // decode block out.push(self.buf[0] << 2 | self.buf[1] >> 4); @@ -302,23 +303,23 @@ impl Base64Decoder { Ok(out) } - /// As you would expect, do_final() consumes the object. + /// As can be expected, do_final() consumes the object. pub fn do_final>(mut self, input: T) -> Result, Base64Error> { // process as much as we can the usual way. let mut out = match self.decode_internal(input, false) { Ok(out) => out, - Err(Base64Error::PaddingEnconteredDuringDoUpdate) => { + Err(Base64Error::PaddingEncounteredDuringDoUpdate) => { panic!( - "rollback_if_padding = false should not produce a Base64Error::PaddingEnconteredDuringDoUpdate" + "rollback_if_padding = false should not produce a Base64Error::PaddingEncounteredDuringDoUpdate" ); } Err(e) => return Err(e), }; - // now we only, maybe, have a single block containing padding to deal with. + // now, a single block containing padding remains to be dealt with. if self.vals_in_buf != 0 { // be tolerant of missing padding - // if we're at the end and it's not a complete block, then imagine the missing padding. + // if it is not a complete block at the end, the infer the byte count from the number of leftover symbols let pad_count: u8 = 3 - (self.vals_in_buf as u8 - 1); out.push(self.buf[0] << 2 | self.buf[1] >> 4); diff --git a/crypto/core/src/key_material.rs b/crypto/core/src/key_material.rs index 48f903e..ebb000f 100644 --- a/crypto/core/src/key_material.rs +++ b/crypto/core/src/key_material.rs @@ -44,7 +44,7 @@ use bouncycastle_utils::{ct, min}; use core::cmp::{Ordering, PartialOrd}; use core::fmt; -/// Sometimes you just need a zero-length dummy key. +/// When it is necessary to get a zero-length dummy key. pub type KeyMaterial0 = KeyMaterial<0>; pub type KeyMaterial128 = KeyMaterial<16>; @@ -389,9 +389,11 @@ impl KeyMaterialTrait for KeyMaterial { self.key_len = key_len; Ok(()) } + fn key_type(&self) -> KeyType { self.key_type.clone() } + fn set_key_type(&mut self, key_type: KeyType) -> Result<(), KeyMaterialError> { if !self.allow_hazardous_operations { return Err(KeyMaterialError::HazardousOperationNotPermitted); @@ -399,6 +401,7 @@ impl KeyMaterialTrait for KeyMaterial { self.key_type = key_type.clone(); Ok(()) } + fn security_strength(&self) -> SecurityStrength { self.security_strength.clone() } @@ -453,6 +456,7 @@ impl KeyMaterialTrait for KeyMaterial { self.drop_hazardous_operations(); Ok(()) } + /// Sets this instance to be able to perform potentially hazardous operations such as /// casting a KeyMaterial of type RawUnknownEntropy or RawLowEntropy into RawFullEntropy or SymmetricCipherKey. /// @@ -463,10 +467,12 @@ impl KeyMaterialTrait for KeyMaterial { fn allow_hazardous_operations(&mut self) { self.allow_hazardous_operations = true; } + /// Resets this instance to not be able to perform potentially hazardous operations. fn drop_hazardous_operations(&mut self) { self.allow_hazardous_operations = false; } + /// Sets the key_type of this KeyMaterial object. /// Does not perform any operations on the actual key material, other than changing the key_type field. /// If allow_hazardous_operations is true, this method will allow conversion to any KeyType, otherwise @@ -529,6 +535,7 @@ impl KeyMaterialTrait for KeyMaterial { self.drop_hazardous_operations(); Ok(()) } + fn is_full_entropy(&self) -> bool { match self.key_type { KeyType::BytesFullEntropy diff --git a/crypto/core/src/traits.rs b/crypto/core/src/traits.rs index 089e282..72f8149 100644 --- a/crypto/core/src/traits.rs +++ b/crypto/core/src/traits.rs @@ -1,4 +1,4 @@ -//! Provides simplified abstracted APIs over classes of cryptigraphic primitives, such as Hash, KDF, etc. +//! Provides simplified abstracted APIs over classes of cryptographic primitives, such as Hash, KDF, etc. use crate::errors::{HashError, KDFError, KEMError, MACError, RNGError, SignatureError}; use crate::key_material::KeyMaterialTrait; @@ -108,7 +108,7 @@ pub trait KDF: Default { /// [KeyType::BytesLowEntropy] -- for example, seeding SHA3-256 with a [KeyMaterial] containing /// only 128 bits of key material. /// - /// An implement can, and in most cases SHOULD, return a [HashError] if provided + /// An implementation can, and in most cases SHOULD, return a [HashError] if provided /// with a [KeyMaterial] of type [KeyType::Zeroized]. /// /// # Additional Input diff --git a/crypto/core/tests/key_material_tests.rs b/crypto/core/tests/key_material_tests.rs index 5e773fd..d022480 100644 --- a/crypto/core/tests/key_material_tests.rs +++ b/crypto/core/tests/key_material_tests.rs @@ -26,7 +26,7 @@ mod test_key_material { _ => panic!("Expected InvalidLength"), } - // But you can slice it down. + // This can be sliced down. match KeyMaterial512::from_bytes(&DUMMY_KEY_TOO_LONG[..64]) { Ok(key) => assert_eq!(key.key_len(), 64), _ => panic!("Expected InvalidLength"), @@ -47,7 +47,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::Zeroized); assert_eq!(key.key_len(), 16); - // ... but we can force it. + // however, it can be forced by allowing hazardous operations. key.allow_hazardous_operations(); key.set_key_type(KeyType::BytesLowEntropy).unwrap(); key.drop_hazardous_operations(); @@ -59,13 +59,12 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert_eq!(key.security_strength(), SecurityStrength::None); - // but it'll allow it if you allow hazardous operations first. + // it can be enabled by allowing hazardous operations first. let key_bytes = [0u8; 16]; let mut key = KeyMaterial256::new(); key.allow_hazardous_operations(); key.set_bytes_as_type(&key_bytes, KeyType::BytesLowEntropy).unwrap(); assert_eq!(key.key_type(), KeyType::BytesLowEntropy); - // nothing else requires setting hazardous operations. } @@ -89,7 +88,7 @@ mod test_key_material { assert_eq!(key.mut_ref_to_bytes().unwrap()[..16], [1u8; 16]); assert_eq!(key.mut_ref_to_bytes().unwrap()[16..], [0u8; 16]); - // and I can set them + // Then they can be set key.mut_ref_to_bytes().unwrap().copy_from_slice(&[2u8; 32]); key.set_key_len(32).unwrap(); assert_eq!(key.ref_to_bytes(), &[2u8; 32]); @@ -247,7 +246,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert!(!key.is_full_entropy()); - // Note: can't use the usual assert_eq!() here because that requires PartialEq, but we're in a no_std context here. + // Note: the usual assert_eq!() can't be used here because that requires PartialEq, but the current context is no_std. match key.key_type() { KeyType::BytesLowEntropy => { /* good */ } _ => panic!("Expected BytesLowEntropy"), @@ -343,12 +342,13 @@ mod test_key_material { } let zero_key = KeyMaterial256::from_bytes(&[0u8; 19]).unwrap(); - // it should have set the key bytes and length, but also set the key type to Zeroized. + // key bytes and length should be set accordingly, + // as well as the key type should be set to Zeroized. assert_eq!(zero_key.key_type(), KeyType::Zeroized); assert_eq!(zero_key.key_len(), 19); assert_eq!(zero_key.ref_to_bytes(), &[0u8; 19]); - // But it's totally fine if you give it non-zero input data. + // It also admits non-zero input data. let not_zero_key = KeyMaterial256::from_bytes(&[1u8; 19]).unwrap(); assert_eq!(not_zero_key.key_type(), KeyType::BytesLowEntropy); @@ -364,10 +364,10 @@ mod test_key_material { panic!("should have thrown a KeyMaterialError::ActingOnZeroizedKey error.") } } - // but it should still have set the key bytes; it's just giving you a friendly warning + // This should still set the key bytes; only giving a friendly warning that the key is zeroized assert_eq!(zero_key.key_type(), KeyType::Zeroized); - // ... but will allow it if you set .allow_hazardous_operations() first. + // The operation is allowed by setting .allow_hazardous_operations() first. let mut zero_key = KeyMaterial256::new(); zero_key.allow_hazardous_operations(); zero_key.set_bytes_as_type(&[0u8; 19], KeyType::MACKey).unwrap(); @@ -402,7 +402,7 @@ mod test_key_material { _ => panic!("Expected HazardousConversion"), } - /* Should work if you allow hazardous conversions. */ + /* Should work when hazardous conversions are allowed. */ key = KeyMaterial256::from_bytes(&DUMMY_KEY[..32]).unwrap(); key.allow_hazardous_operations(); key.convert_key_type(KeyType::BytesFullEntropy).unwrap(); @@ -445,7 +445,8 @@ mod test_key_material { assert_eq!(key1.key_type(), KeyType::MACKey); assert_eq!(key1.security_strength(), SecurityStrength::_256bit); - // success case: same size using default From impl; only works if the sizes are the same (ie the compiler knows that they are the same type. + // success case: same size using default From impl; + // only works if the sizes are the same (i.e. the compiler knows that they are the same type). let key2 = KeyMaterial256::from(key1.clone()); assert_eq!(key1.key_len(), key2.key_len()); assert_eq!(key1.key_type(), key2.key_type()); @@ -490,7 +491,7 @@ mod test_key_material { _ => panic!("Expected HazardousConversion"), } - // should work if you allow hazardous conversions. + // should work when hazardous conversions are allowed. key.allow_hazardous_operations(); key.convert_key_type(KeyType::SymmetricCipherKey).unwrap(); } @@ -540,7 +541,7 @@ mod test_key_material { assert_eq!(key.key_type(), KeyType::BytesFullEntropy); assert_eq!(key.security_strength(), SecurityStrength::None); - // even if it's long enough, BytesLowEntropy or Zeroized always get ::None + // even if it's long enough, BytesLowEntropy or Zeroized always get ::None as security strength let key = KeyMaterial512::from_bytes_as_type(DUMMY_KEY, KeyType::BytesLowEntropy).unwrap(); assert_eq!(key.key_type(), KeyType::BytesLowEntropy); assert_eq!(key.key_len(), 64); @@ -683,6 +684,7 @@ mod test_key_material { b"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F", ) .unwrap(); + let key2 = KeyMaterial256::from_bytes( b"\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F", ) diff --git a/crypto/factory/src/hash_factory.rs b/crypto/factory/src/hash_factory.rs index 9d12117..05edc13 100644 --- a/crypto/factory/src/hash_factory.rs +++ b/crypto/factory/src/hash_factory.rs @@ -14,7 +14,7 @@ //! let h = bouncycastle_factory::hash_factory::HashFactory::new(sha3::SHA3_256_NAME).unwrap(); //! let output: Vec = h.hash(data); //! ``` -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; diff --git a/crypto/factory/src/kdf_factory.rs b/crypto/factory/src/kdf_factory.rs index f3deffb..85e5b3e 100644 --- a/crypto/factory/src/kdf_factory.rs +++ b/crypto/factory/src/kdf_factory.rs @@ -9,7 +9,7 @@ //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! @@ -17,14 +17,14 @@ //! let new_key = h.derive_key(&seed_key, additional_input).unwrap(); //! ``` //! -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! @@ -32,14 +32,14 @@ //! let new_key = h.derive_key(&seed_key, additional_input).unwrap(); //! ``` //! -//! Or if you don't particularly care which algorithm is used, you can use the built-in default: +//! If the algorithm used is not particularly important, the built-in default may be used: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; //! use bouncycastle_core::traits::KDF; //! use bouncycastle_factory::AlgorithmFactory; //! -//! // get your key material from a secure place; here we'll use the default RNG, seeded from the OS +//! // Obtain key material from a secure place; here the default RNG is used, seeded from the OS is used //! let seed_key = KeyMaterial256::from_rng(&mut bouncycastle_rng::DefaultRNG::default()).unwrap(); //! let additional_input: &[u8] = b"some additional input"; //! diff --git a/crypto/factory/src/mac_factory.rs b/crypto/factory/src/mac_factory.rs index 141c59f..f5efc79 100644 --- a/crypto/factory/src/mac_factory.rs +++ b/crypto/factory/src/mac_factory.rs @@ -34,7 +34,7 @@ //! } //! ``` //! -//! You can equivalently construct an instance of [MACFactory] by string instead of using the constant: +//! Equivalently, an instance of [MACFactory] may be constructed by string instead of using the constant: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; @@ -52,7 +52,7 @@ //! let hmac = MACFactory::new("HMAC-SHA256", &key).unwrap(); //! ``` //! -//! Or if you don't particularly care which algorithm is used, you can use the built-in default: +//! If the algorithm used is not particularly important, the built-in default may be used: //! //! ``` //! use bouncycastle_core::key_material::{KeyMaterial256, KeyType}; @@ -138,12 +138,12 @@ impl MACFactory { } impl MAC for MACFactory { - /// This is a dummy function, required by the [MAC] trait. Don't call it, it doesn't do anything. + /// This is a dummy function, required by the [MAC] trait. DO NOT call it, it does not do anything. fn new(_key: &impl KeyMaterialTrait) -> Result { unimplemented!() } - /// This is a dummy function, required by the [MAC] trait. Don't call it, it doesn't do anything. + /// This is a dummy function, required by the [MAC] trait. DO NOT call it, it does not do anything. fn new_allow_weak_key(_key: &impl KeyMaterialTrait) -> Result { unimplemented!() } diff --git a/crypto/factory/src/rng_factory.rs b/crypto/factory/src/rng_factory.rs index 9f1b8e0..4925f25 100644 --- a/crypto/factory/src/rng_factory.rs +++ b/crypto/factory/src/rng_factory.rs @@ -29,8 +29,8 @@ //! let h = bouncycastle_factory::hash_factory::HashFactory::new(sha3::SHA3_256_NAME).unwrap(); //! let output: Vec = h.hash(data); //! ``` -//! You can equivalently invoke this by string instead of using the constant: -//! +//! Equivalently, it may be invoked by passing a string instead of using the constant: +//! //! ``` //! use bouncycastle_factory::AlgorithmFactory; //! use bouncycastle_core::traits::Hash; diff --git a/crypto/factory/src/xof_factory.rs b/crypto/factory/src/xof_factory.rs index e35e86e..24d5127 100644 --- a/crypto/factory/src/xof_factory.rs +++ b/crypto/factory/src/xof_factory.rs @@ -16,7 +16,7 @@ //! h.absorb(data); //! let output: Vec = h.squeeze(16); //! ``` -//! You can equivalently invoke this by string instead of using the constant: +//! Equivalently, it may be invoked by passing a string instead of using the constant: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; @@ -24,8 +24,7 @@ //! //! let mut h = XOFFactory::new("SHAKE128"); //! ``` -//! -//! Or, if you don't particularly care which algorithm you get, you can use the configured default: +//! If the algorithm used is not particularly important, the configured default may be used: //! //! ``` //! use bouncycastle_factory::AlgorithmFactory; diff --git a/crypto/factory/tests/kdf_factory_tests.rs b/crypto/factory/tests/kdf_factory_tests.rs index 16e6949..d34af5f 100644 --- a/crypto/factory/tests/kdf_factory_tests.rs +++ b/crypto/factory/tests/kdf_factory_tests.rs @@ -63,7 +63,7 @@ mod kdf_factory_tests { fn hkdf_tests() { /* HKDF-SHA256 */ // Note: this value is not checked against any external reference implementation, - // I just hard-coded the value to make sure it stays consistent. + // The value is hard-coded to ensure consistency. let key_material = KeyMaterial256::from_bytes_as_type(&DUMMY_SEED_512[..32], KeyType::MACKey).unwrap(); let derived_key = @@ -73,7 +73,7 @@ mod kdf_factory_tests { /* HKDF-SHA512 */ // Note: this value is not checked against any external reference implementation, - // I just hard-coded the value to make sure it stays consistent. + // The value is hard-coded to ensure consistency. let key_material = KeyMaterial512::from_bytes(&DUMMY_SEED_512[..64]).unwrap(); let derived_key = KDFFactory::new("HKDF-SHA512").unwrap().derive_key(&key_material, &[0u8; 0]).unwrap(); diff --git a/crypto/hex/src/lib.rs b/crypto/hex/src/lib.rs index 6fcf787..865c50f 100644 --- a/crypto/hex/src/lib.rs +++ b/crypto/hex/src/lib.rs @@ -3,9 +3,9 @@ //! This one is implemented using constant-time operations in the conversions //! from Strings to byte values, so it is safe to use on cryptographic secret values. //! -//! It should just work the way you expect: encode takes any bytes-like rust type -//! and returns a String, decode takes a String (which can be in any bytes-like container) -//! and returns a `Vec`. +//! It should just work as expected: +//! encode takes any bytes-like rust type and returns a String, +//! decode takes a String (which can be in any bytes-like container) and returns a `Vec`. //! //! ``` //! use bouncycastle_hex as hex; @@ -63,7 +63,8 @@ pub fn encode_out>(input: T, out: &mut [u8]) -> Result::is_within_range(c as i64, 0, 9); let in_af = Condition::::is_within_range(c as i64, 10, 15); - // TODO: redo this once we have ct::u8 implemented ... the i64 is wasteful + // TODO: redo this once we have ct::u8 implemented + // The i64 is wasteful let c_09: i64 = '0' as i64 + (c as i64); let c_az: i64 = 'a' as i64 + (c as i64 - 10); @@ -101,7 +102,8 @@ pub fn decode_out>(input: T, out: &mut [u8]) -> Result { i += 1; @@ -146,7 +148,8 @@ pub fn decode_out>(input: T, out: &mut [u8]) -> Result::is_within_range(b as i64, 65, 70); - // TODO: redo this once we have ct::u8 implemented ... the i64 is wasteful + // TODO: redo this once we have ct::u8 implemented + // The i64 is wasteful let c_09: i64 = b as i64 - ('0' as i64); #[allow(non_snake_case)] diff --git a/crypto/hkdf/src/lib.rs b/crypto/hkdf/src/lib.rs index 76b47f0..3deee1e 100644 --- a/crypto/hkdf/src/lib.rs +++ b/crypto/hkdf/src/lib.rs @@ -164,7 +164,7 @@ use bouncycastle_core::traits::XOF; /*** Constants ***/ // Slightly hacky, but set this to accomodate the underlying hash primitive with the largest output size. -// Would be better to somehow pull that at compile time from H, but I'm not sure how to do that. +// It would be better to pull that at compile time from H, but the implementation does not currently do that. const HMAC_BLOCK_LEN: usize = 64; /*** String constants ***/ @@ -179,7 +179,7 @@ pub type HKDF_SHA256 = HKDF; pub type HKDF_SHA512 = HKDF; pub struct HKDF { - hmac: Option>, // Optional because we can't construct an HMAC until they give us a key + hmac: Option>, // Optional because an HMAC cannot be constructed until a key is provided // to initialize it with. // None should correspond to a state of Uninitialized. entropy: HkdfEntropyTracker, @@ -239,7 +239,7 @@ impl HkdfEntropyTracker { } } -// Since I don't want this struct to be public, the tests have to go here. +// Because this struct is not public, the tests have to go here. #[test] fn test_entropy_tracker() { let mut entropy = HkdfEntropyTracker::::new(); @@ -400,7 +400,7 @@ impl HKDF { let out: &mut [u8] = okm.mut_ref_to_bytes()?; // Could potentially speed this up by unrolling T(0) and T(1) - // We're gonna have to kludge the prk key type to MACKey to make HMAC happy, but we'll set it back to the original value afterwards. + // The prk key type must be temporarily changed to MACKey to satisfy HMAC, then restored afterwards. let prk_as_mac_key = KeyMaterial::::from_bytes_as_type(prk.ref_to_bytes(), KeyType::MACKey)?; @@ -487,7 +487,7 @@ impl HKDF { }; // Often HMAC is initialized with a zero salt, - // So we're gonna ignore key strength errors here + // Key strength errors are ignored here. // This will all be tabulated correctly via entropy.credit_entropy() self.hmac = Some(HMAC::::new_allow_weak_key(salt)?); diff --git a/crypto/mlkem/src/lib.rs b/crypto/mlkem/src/lib.rs index 4de7d42..aa84425 100644 --- a/crypto/mlkem/src/lib.rs +++ b/crypto/mlkem/src/lib.rs @@ -115,13 +115,13 @@ //! | ML-KEM-1024_expanded | 1568 | 10272 | 3168 | 12418 | //! //! All values are in bytes. The "in memory" sizes are measured by rust's `std::mem::size_of`. -//! Values in parentheses are the usual sizes in our un-optimized implementation in the \[bouncycastle_mldsa] crate. +//! Values in parentheses are the usual sizes in the un-optimized implementation in the \[bouncycastle_mldsa] crate. //! //! # Security //! All functionality exposed by this crate is considered secure to use. //! In other words, this crate does not contain any "hazmat" except for the obvious points about //! handling your private keys properly: if you post your private key to github, or you generate -//! production keys from a weak seed, I can't help you, that's on you. +//! production keys from a weak seed, that use is unsupported //! It is worth mentioning, however, that if using a [MLKEM::keygen_from_seed], then it is your //! responsibility to ensure that the seed is cryptographically random and unpredictable. //! And also that [MLKEM::encaps_internal] requires you to provide the randomness, so the ciphertext @@ -133,8 +133,8 @@ //! constructions. That should give this implementation reasonably good resistance to timing and //! power analysis key extraction attacks, however: A) this is a "best-effort" and not formally verified, //! and B) the Rust compiler does not guarantee constant-time behaviour no matter how clever your code, -//! so like all Safe Rust code (ie Rust code that does not include inline assembly), we are at the mercy -//! of the Rust compiler's optimizer for whether our bitshift-and-xor code actually remains +//! so like all Safe Rust code (ie Rust code that does not include inline assembly), the Rust compiler's optimizer +//! determines whether the bitshift-and-xor code actually remains //! constant-time after compilation. #![no_std] @@ -143,13 +143,13 @@ #![allow(incomplete_features)] // needed because currently generic_const_exprs is experimental #![feature(generic_const_exprs)] #![feature(adt_const_params)] -// These are because I'm matching variable names exactly against FIPS 204, for example both 'K' and 'k', +// These are because variable names are matched exactly against FIPS 204, for example both 'K' and 'k', // or 'A' and 'a' are used and have specific meanings. // But need to tell the rust linter to not care. #![allow(non_snake_case)] #![allow(non_upper_case_globals)] -// so I can use private traits to hide internal stuff that needs to be generic within the -// MLKEM implementation, but I don't want accessed from outside, such as FIPS-internal functions. +// so private traits can hide internal items that need to be generic within the +// MLKEM implementation, but should not be accessed from outside, such as FIPS-internal functions. #![allow(private_bounds)] // imports needed just for docs diff --git a/crypto/sha2/src/sha256.rs b/crypto/sha2/src/sha256.rs index 7be7f47..617cb54 100644 --- a/crypto/sha2/src/sha256.rs +++ b/crypto/sha2/src/sha256.rs @@ -154,7 +154,8 @@ pub struct SHA256Internal { byte_count: u64, x_buf: [u8; 64], x_buf_off: usize, - // TODO: should we add a maximum message size according to FIPS 180-4? (2^64 for SHA256 and 2^128 for SHA512) + // TODO: Investigate whether maximum message size (according to FIPS 180-4) should be added + // (2^64 for SHA256 and 2^128 for SHA512) } impl Drop for SHA256Internal { @@ -279,7 +280,7 @@ impl Hash for SHA256Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits( self, @@ -291,7 +292,7 @@ impl Hash for SHA256Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits_out( self, diff --git a/crypto/sha2/src/sha512.rs b/crypto/sha2/src/sha512.rs index b4ae990..541c1a9 100644 --- a/crypto/sha2/src/sha512.rs +++ b/crypto/sha2/src/sha512.rs @@ -163,7 +163,8 @@ impl Sha512State { pub struct Sha512Internal { _params: std::marker::PhantomData, state: Sha512State, - byte_count: u64, // NOTE We only support 2^67 bits, not the full 2^128 + // NOTE The code currently only supports 2^67 bits, not the full 2^128 + byte_count: u64, x_buf: [u8; 128], x_buf_off: usize, } @@ -290,7 +291,7 @@ impl Hash for Sha512Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits( self, @@ -302,7 +303,7 @@ impl Hash for Sha512Internal { /// TODO: This is defined in FIPS 180-4 s. 5.1.2 /// TODO: - /// TODO: Could implement if there is demand. + /// TODO: It can be implemented if required #[allow(unused)] fn do_final_partial_bits_out( self, diff --git a/crypto/utils/src/ct.rs b/crypto/utils/src/ct.rs index 96b1950..a3f93e2 100644 --- a/crypto/utils/src/ct.rs +++ b/crypto/utils/src/ct.rs @@ -75,7 +75,7 @@ impl Condition { Self::is_negative(x - y) } - // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero. + // Note: this cannot currently be marked as const, since it either needs a (non-const) not (!) or a boolean OR is_zero. pub fn is_lte(x: i64, y: i64) -> Self { !Self::is_gt(x, y) } @@ -84,7 +84,7 @@ impl Condition { Self::is_lt(y, x) } - // Note: haven't found a clever way to make this const, since it either needs a (non-const) not (!) or a boolean OR is_zero. + // Note: this cannot currently be marked as const, since it either needs a (non-const) not (!) or a boolean OR is_zero. pub fn is_gte(x: i64, y: i64) -> Self { !Self::is_lt(x, y) } @@ -262,7 +262,7 @@ where } /// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels, but we'll try. +/// and optimization levels. The following presents the standard constant-time shape. pub fn ct_eq_bytes(a: &[u8], b: &[u8]) -> bool { if a.len() != b.len() { return false; @@ -275,7 +275,7 @@ pub fn ct_eq_bytes(a: &[u8], b: &[u8]) -> bool { } /// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels, but we'll try. +/// and optimization levels. The following presents the standard constant-time shape. pub fn ct_eq_zero_bytes(a: &[u8]) -> bool { let mut result = 0u8; for i in 0..a.len() { diff --git a/crypto/utils/src/lib.rs b/crypto/utils/src/lib.rs index 67644be..3f6a385 100644 --- a/crypto/utils/src/lib.rs +++ b/crypto/utils/src/lib.rs @@ -2,12 +2,12 @@ pub mod ct; -/// Basic max function. If they are equal, you get back the first one. +/// Basic max function. If they are equal, it returns the first one. pub fn max<'a, T: PartialOrd>(x: &'a T, y: &'a T) -> &'a T { if x >= y { x } else { y } } -/// Basic min function. If they are equal, you get back the first one. +/// Basic min function. If they are equal, it returns the first one. pub fn min<'a, T: PartialOrd>(x: &'a T, y: &'a T) -> &'a T { if x <= y { x } else { y } } diff --git a/src/bench_mldsa_mem_usage.rs b/src/bench_mldsa_mem_usage.rs index 55bdc24..2717da4 100644 --- a/src/bench_mldsa_mem_usage.rs +++ b/src/bench_mldsa_mem_usage.rs @@ -5,15 +5,16 @@ //! //! > ms_print massif.out.835000 //! -//! or, shoved all into one line: +//! alternatively, as a one line command: //! //! > clear; clear; valgrind --tool=massif --heap=no --stacks=yes -- target/release/bench_mldsa_mem_usage > /dev/null; ms_print massif.out.*; rm massif.out.* //! //! Make sure you build in release mode! //! -//! Note: I'm using print!() to force the compiler not to optimize away the actual code. -//! I'm printing the important stuff for benchmarking to stderr so that I can pipe the junk to /dev/null -//! (I'm not doing it the other way because /usr/bin/time prints its useful stuff to stderr as well) +//! Note: +//! The code is using print!() to force the compiler not to optimize away the actual code. +//! It is printing important outputs for benchmarking to stderr so that the rest can be mapped to /dev/null +//! (this is because /usr/bin/time prints useful outputs to stderr as well) //! //! Main is at the bottom, controls which this was actually run. @@ -25,7 +26,7 @@ use bouncycastle_core_interface::traits::{Signature, SignaturePublicKey}; use bouncycastle_hex as hex; use bouncycastle_mldsa::MLDSA44PublicKey; -/// This exists so I can use /usr/bin/time to measure the base memory footprint of the cargo bench harness +/// This exists so that /usr/bin/time can be used to measure the base memory footprint of the cargo bench harness fn bench_do_nothing() { eprintln!("DoNothing");