Skip to content

Removal of .unwrap() and .expect() in library code #46

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
npajkovsky wants to merge 4 commits into from
Closed

Removal of .unwrap() and .expect() in library code #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecced8d
Remove DEFAULT_HASH_NAME constants and eliminate unwrap() in HashFact…
nikolapajkovsky Jun 26, 2026
9091617
Remove DEFAULT_KDF_NAME constants and eliminate unwrap() in KDFFactor…
nikolapajkovsky Jun 26, 2026
bda139f
Remove DEFAULT_KDF_NAME constants and eliminate unwrap() in KDFFactor…
nikolapajkovsky Jun 26, 2026
09b9f53
Eliminate unwrap() and assert!() in ML-DSA signature verification
nikolapajkovsky Jun 26, 2026
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
11 changes: 3 additions & 8 deletions crypto/factory/src/hash_factory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,6 @@ use bouncycastle_sha2::{SHA224_NAME, SHA256_NAME, SHA384_NAME, SHA512_NAME};
use bouncycastle_sha3 as sha3;
use bouncycastle_sha3::{SHA3_224_NAME, SHA3_256_NAME, SHA3_384_NAME, SHA3_512_NAME};

/*** Defaults ***/
pub const DEFAULT_HASH_NAME: &str = SHA3_256_NAME;
pub const DEFAULT_128BIT_HASH_NAME: &str = SHA3_256_NAME;
pub const DEFAULT_256BIT_HASH_NAME: &str = SHA3_512_NAME;

@ounsworth ounsworth Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm. This change will make it hard in the future if we want to make the library defaults driven from a config file or env var, but I guess for now this saves us 3 unwraps per factory. Ok.

(also, I think the whole factory crate needs a big re-visit, see #23)

/// All members must impl Hash.
/// Note: no SHAKE because SHAKE is not NIST approved as a hash function. See FIPS 202 section A.2.
pub enum HashFactory {
Expand All @@ -55,16 +50,16 @@ pub enum HashFactory {

impl Default for HashFactory {
fn default() -> HashFactory {
Self::new(DEFAULT_HASH_NAME).unwrap()
Self::SHA3_256(sha3::SHA3_256::new())
}
}

impl AlgorithmFactory for HashFactory {
fn default_128_bit() -> HashFactory {
Self::new(DEFAULT_128BIT_HASH_NAME).unwrap()
Self::SHA3_256(sha3::SHA3_256::new())
}
fn default_256_bit() -> HashFactory {
Self::new(DEFAULT_256BIT_HASH_NAME).unwrap()
Self::SHA3_512(sha3::SHA3_512::new())
}

fn new(alg_name: &str) -> Result<Self, FactoryError> {
Expand Down
11 changes: 3 additions & 8 deletions crypto/factory/src/kdf_factory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,6 @@ use bouncycastle_sha3::{
SHA3_224_NAME, SHA3_256_NAME, SHA3_384_NAME, SHA3_512_NAME, SHAKE128_NAME, SHAKE256_NAME,
};

/*** Defaults ***/
pub const DEFAULT_KDF_NAME: &str = HKDF_SHA512_NAME;
pub const DEFAULT_128BIT_KDF_NAME: &str = HKDF_SHA256_NAME;
pub const DEFAULT_256BIT_KDF_NAME: &str = HKDF_SHA512_NAME;

// All members must impl KDF.
pub enum KDFFactory {
#[allow(non_camel_case_types)]
Expand All @@ -79,17 +74,17 @@ pub enum KDFFactory {

impl Default for KDFFactory {
fn default() -> Self {
KDFFactory::new(DEFAULT_KDF_NAME).unwrap()
Self::HKDF_SHA512(hkdf::HKDF_SHA512::new())
}
}

impl AlgorithmFactory for KDFFactory {
fn default_128_bit() -> Self {
KDFFactory::new(DEFAULT_128BIT_KDF_NAME).unwrap()
Self::HKDF_SHA256(hkdf::HKDF_SHA256::new())
}

fn default_256_bit() -> Self {
KDFFactory::new(DEFAULT_256BIT_KDF_NAME).unwrap()
Self::HKDF_SHA512(hkdf::HKDF_SHA512::new())
}

fn new(alg_name: &str) -> Result<Self, FactoryError> {
Expand Down
11 changes: 3 additions & 8 deletions crypto/factory/src/rng_factory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,6 @@ use bouncycastle_core::traits::{RNG, SecurityStrength};
use bouncycastle_rng as rng;
use bouncycastle_rng::{HASH_DRBG_SHA256_NAME, HASH_DRBG_SHA512_NAME};

/*** Defaults ***/
pub const DEFAULT_DRBG_NAME: &str = HASH_DRBG_SHA512_NAME;
pub const DEFAULT_128BIT_DRBG_NAME: &str = HASH_DRBG_SHA256_NAME;
pub const DEFAULT_256BIT_DRBG_NAME: &str = HASH_DRBG_SHA512_NAME;

/// All members must impl RNG.
pub enum RNGFactory {
#[allow(non_camel_case_types)]
Expand All @@ -65,16 +60,16 @@ pub enum RNGFactory {

impl Default for RNGFactory {
fn default() -> Self {
Self::new(DEFAULT_DRBG_NAME).unwrap()
Self::HashDRBG_SHA512(rng::HashDRBG_SHA512::new_from_os())
}
}

impl AlgorithmFactory for RNGFactory {
fn default_128_bit() -> Self {
Self::new(DEFAULT_128BIT_DRBG_NAME).unwrap()
Self::HashDRBG_SHA256(rng::HashDRBG_SHA256::new_from_os())
}
fn default_256_bit() -> Self {
Self::new(DEFAULT_256BIT_DRBG_NAME).unwrap()
Self::HashDRBG_SHA512(rng::HashDRBG_SHA512::new_from_os())
}

fn new(alg_name: &str) -> Result<Self, FactoryError> {
Expand Down
54 changes: 22 additions & 32 deletions crypto/mldsa/src/mldsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,15 +1536,12 @@ impl<
sig: &[u8],
) -> Result<(), SignatureError> {
let mu = MuBuilder::compute_mu(&pk.compute_tr(), msg, ctx)?;

if sig.len() != SIG_LEN {
return Err(SignatureError::LengthError("Signature value is not the correct length."));
}
if Self::verify_mu_internal(&pk.pk, &pk.A_hat(), &mu, &sig[..SIG_LEN].try_into().unwrap()) {
Ok(())
} else {
Err(SignatureError::SignatureVerificationFailed)
}
let sig: &[u8; SIG_LEN] = sig.try_into().map_err(|_| {
SignatureError::LengthError("Signature value is not the correct length.")
})?;
Self::verify_mu_internal(&pk.pk, &pk.A_hat(), &mu, sig)
.then_some(())
.ok_or(SignatureError::SignatureVerificationFailed)
}

/// Algorithm 8 ML-DSA.Verify_internal(𝑝𝑘, 𝑀′, 𝜎)
Expand Down Expand Up @@ -2062,15 +2059,12 @@ impl<
{
fn verify(pk: &PK, msg: &[u8], ctx: Option<&[u8]>, sig: &[u8]) -> Result<(), SignatureError> {
let mu = MuBuilder::compute_mu(&pk.compute_tr(), msg, ctx)?;

if sig.len() != SIG_LEN {
return Err(SignatureError::LengthError("Signature value is not the correct length."));
}
if Self::verify_mu_internal(pk, &pk.A_hat(), &mu, &sig.try_into().unwrap()) {
Ok(())
} else {
Err(SignatureError::SignatureVerificationFailed)
}
let sig: &[u8; SIG_LEN] = sig.try_into().map_err(|_| {
SignatureError::LengthError("Signature value is not the correct length.")
})?;
Self::verify_mu_internal(pk, &pk.A_hat(), &mu, sig)
.then_some(())
.ok_or(SignatureError::SignatureVerificationFailed)
}

fn verify_init(pk: &PK, ctx: Option<&[u8]>) -> Result<Self, SignatureError> {
Expand All @@ -2091,20 +2085,16 @@ impl<
fn verify_final(self, sig: &[u8]) -> Result<(), SignatureError> {
let mu = self.mu_builder.do_final();

assert!(
self.pk.is_some(),
"Somehow you managed to construct a streaming verifier without a public key, impressive!"
);
let pk: &PK = &self.pk.unwrap();

if sig.len() != SIG_LEN {
return Err(SignatureError::LengthError("Signature value is not the correct length."));
}
if Self::verify_mu_internal(pk, &pk.A_hat(), &mu, &sig[..SIG_LEN].try_into().unwrap()) {
Ok(())
} else {
Err(SignatureError::SignatureVerificationFailed)
}
let pk: &PK = self
.pk
.as_ref()
.ok_or(SignatureError::GenericError("No public key set on streaming verifier."))?;
let sig: &[u8; SIG_LEN] = sig.try_into().map_err(|_| {
SignatureError::LengthError("Signature value is not the correct length.")
})?;
Self::verify_mu_internal(pk, &pk.A_hat(), &mu, sig)
.then_some(())
.ok_or(SignatureError::SignatureVerificationFailed)
}
}

Expand Down
Loading