Skip to content

crypto-common: add TryKeyInit trait #2092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+83 −2
Closed

crypto-common: add TryKeyInit trait #2092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 83 additions & 2 deletions crypto-common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ pub trait AlgorithmName {
fn write_alg_name(f: &mut fmt::Formatter<'_>) -> fmt::Result;
}

/// Types which can be initialized from key.
/// Types which can be initialized from a key.
pub trait KeyInit: KeySizeUser + Sized {
/// Create new value from fixed size key.
fn new(key: &Key<Self>) -> Self;
Expand Down Expand Up @@ -209,7 +209,7 @@ pub trait KeyInit: KeySizeUser + Sized {
}
}

/// Types which can be initialized from key and initialization vector (nonce).
/// Types which can be initialized from a key and initialization vector (nonce).
pub trait KeyIvInit: KeySizeUser + IvSizeUser + Sized {
/// Create new value from fixed length key and nonce.
fn new(key: &Key<Self>, iv: &Iv<Self>) -> Self;
Expand Down Expand Up @@ -323,6 +323,73 @@ pub trait KeyIvInit: KeySizeUser + IvSizeUser + Sized {
}
}

/// Types which can be fallibly initialized from a key.
pub trait TryKeyInit: KeySizeUser + Sized {
/// Create new value from a fixed-size key.
///
/// # Errors
/// - if the key is considered invalid according to rules specific to the implementing type
fn new(key: &Key<Self>) -> Result<Self, InvalidKey>;

/// Create new value from a variable size key.
///
/// # Errors
/// - if the provided slice is the wrong length
/// - if the key is considered invalid by [`TryKeyInit::new`]
#[inline]
fn new_from_slice(key: &[u8]) -> Result<Self, InvalidKey> {
<&Key<Self>>::try_from(key)
.map_err(|_| InvalidKey)
.and_then(Self::new)
}

/// Generate random key using the operating system's secure RNG.
///
/// # Errors
/// - if the system random number generator encountered an internal error
#[cfg(feature = "getrandom")]
#[inline]
fn generate_key() -> Result<Key<Self>, getrandom::Error> {
// Use rejection sampling to find a key which initializes successfully in an unbiased manner
loop {
let mut key = Key::<Self>::default();
getrandom::fill(&mut key)?;

if Self::new(&key).is_ok() {
return Ok(key);
}
}
}
Comment on lines +352 to +362
Copy link
Member Author

@tarcieri tarcieri Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #2060 implementing random bytestring generation in hybrid-array (which I still think is an independently good idea) won't suffice for these use cases, since they need algorithm-specific logic to select valid keys.

The approach in this PR is able to provide a generic unbiased key generation implementation which can have algorithm-specific overrides to improve efficiency.

I think we could potentially drop the keygen for KeyInit and point to hybrid-array for such cases, but having generic keygen for TryKeyInit is still independently useful, IMO.

Copy link
Member

@newpavlov newpavlov Nov 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the first glance, it certainly could be useful to have unbiased generation as part of the trait. Especially for algorithms which do not use all most significant bits in the key. Using naive rejection sampling can be really inefficient in such cases.

But looking at the current code, I can not say I am happy with it... Not only it forces us to keep the rand_core/getrandom features in crypto-common, but it also could be inefficient. For example, let key = Alg::generate_key()?; let alg = Alg::new(&key).unwrap(); results in an unnecessary key validation as part of the new method (which is not guaranteed to be optimized out by the compiler), but also forces users to use unwrap or handle unreachable error.

How about introducing an associated type for "validated" key? Something like this:

pub trait TryKeyInit: KeySizeUser + Sized {
    type ValidatedKey: Into<Key<Self>> + TryFrom<Key<Self>, Error = InvalidKey>;
    fn new_from_validated(key: &Self::ValidatedKey) -> Self;
    fn new(key: &Key<Self>) -> Result<Self, InvalidKey> {
        key.try_into().map(Self::new_from_validated)
    }
}

This way we will be able to move key generation code to the validated key type without burdening the TryKeyInit trait. I haven't thought much about method names, so feel free to rename them.

Copy link
Member Author

@tarcieri tarcieri Nov 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let key = Alg::generate_key()?; let alg = Alg::new(&key).unwrap(); results in an unnecessary key validation as part of the new method

This problem does not exist with the Generate/FromCryptoRng traits, it's inhereted by following the existing pattern of KeyInit, which is why I suggested separating the key generation traits out of KeyInit (#2060) and implemented an alternative which can generate random keys of any kind which you receive fully initialized (#2096), which fully addresses this concern.


/// Generate random key using the provided [`CryptoRng`].
#[cfg(feature = "rand_core")]
#[inline]
fn generate_key_with_rng<R: CryptoRng + ?Sized>(rng: &mut R) -> Key<Self> {
let Ok(key) = Self::try_generate_key_with_rng(rng);
key
}

/// Generate random key using the provided [`TryCryptoRng`].
///
/// # Errors
/// - if the provided [`TryCryptoRng`] encountered an internal error
#[cfg(feature = "rand_core")]
#[inline]
fn try_generate_key_with_rng<R: TryCryptoRng + ?Sized>(
rng: &mut R,
) -> Result<Key<Self>, R::Error> {
// Use rejection sampling to find a key which initializes successfully in an unbiased manner
loop {
let mut key = Key::<Self>::default();
rng.try_fill_bytes(&mut key)?;

if Self::new(&key).is_ok() {
return Ok(key);
}
}
}
}

/// Types which can be initialized from another type (usually block ciphers).
///
/// Usually used for initializing types from block ciphers.
Expand Down Expand Up @@ -462,6 +529,20 @@ where
}
*/

/// Error type for [`TryKeyInit`] for cases where the provided bytes do not correspond to a
/// valid key.
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub struct InvalidKey;

impl fmt::Display for InvalidKey {
#[inline]
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
f.write_str("WeakKey")
}
}

impl core::error::Error for InvalidKey {}

/// The error type returned when key and/or IV used in the [`KeyInit`],
/// [`KeyIvInit`], and [`InnerIvInit`] slice-based methods had
/// an invalid length.
Expand Down