Skip to content
New issue

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

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

Already on GitHub? # to your account

kem: remove tests, update to rand_core v0.9 #1757

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Feb 20, 2025

As discussed here the tests are not particularity useful and block migration to rand_core v0.9.

cc @rozbb

@newpavlov newpavlov requested a review from tarcieri February 20, 2025 09:08
@@ -20,7 +20,7 @@ pub trait Encapsulate<EK, SS> {
type Error: Debug;

/// Encapsulates a fresh shared secret
fn encapsulate(&self, rng: &mut impl CryptoRngCore) -> Result<(EK, SS), Self::Error>;
fn encapsulate(&self, rng: &mut impl CryptoRng) -> Result<(EK, SS), Self::Error>;
Copy link
Member Author

@newpavlov newpavlov Feb 20, 2025

Choose a reason for hiding this comment

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

Here I used CryptoRng instead of TryCryptoRng since it's not clear how implementers should deal with potential RNG errors.

One option is to define two separate methods:

// Find a better name?
pub enum EncRngError<EncErr, RngErr> {
    // Encapsulation error
    Enc(EncErr),
    // RNG error
    Rng(RngErr),
}

pub trait Encapsulate<EK, SS> {
    type Error: Debug;

    fn encapsulate<R: CryptoRng>(&self, rng: &mut R) -> Result<(EK, SS), Self::Error> {
        self.encapsulate_with_try_rng(rng).map_err(|EncRngError::Enc(err)| err)
    }
    // Find a better name?
    fn encapsulate_with_try_rng<R: TryCryptoRng>(
        &self,
        rng: &mut R,
    ) -> Result<(EK, SS), EncRngError<Self::Error, R::Error>>;
}

But I think it's better to do it in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a detailed discussion for why TryCryptoRng is necessary? In the failure modes I’m imagining (finicky external hardware), the solution would be to just poll the hardware to get 32B and then seed a Chacha12Rng. It’d be nice to not support Try*

Copy link
Member Author

Choose a reason for hiding this comment

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

It's primarily to support IO-based RNGs like OsRng (i.e. "to just poll hardware" in your comment) and to replace potential (albeit highly unlikely in practice) panic branches with explicit error processing. If you do not want to deal with this, you always can use the UnwrapErr wrapper.

@newpavlov newpavlov merged commit 1fdcdc7 into master Feb 21, 2025
81 checks passed
@newpavlov newpavlov deleted the kem/rand_core_upd branch February 21, 2025 08:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants