Skip to content
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.

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

Use CryptoRng instead of plain RngCore #132

Closed
wants to merge 1 commit into from

Conversation

baloo
Copy link

@baloo baloo commented Mar 9, 2025

@ebfull
Copy link
Collaborator

ebfull commented Mar 10, 2025

This seems like too strict of a bound to categorically place over all field implementations, since even in cryptography contexts there are legitimate situations where random field elements are needed but a cryptographically secure Rng is not necessarily needed or wanted -- I've ran into these situations before.

In this case I'm not sure why this bound is needed even to address the TODO in the k256 crate that I saw upthread.

@baloo
Copy link
Author

baloo commented Mar 10, 2025

I'm not sure I can judge whether that's required or not. cc @tarcieri

@tarcieri
Copy link
Contributor

Yeah, the k256 issue can probably be solved by moving the actual RNG implementation to the Field::try_from_rng impl, then having Scalar::generate_vartime call Field::try_from_rng, which would allow Scalar::generate_vartime to bound on TryCryptoRng

@baloo
Copy link
Author

baloo commented Mar 10, 2025

Scalar::generate_vartime can't have a stricter bound than the Field::try_from_rng though.

@tarcieri
Copy link
Contributor

I tried the change I was suggesting locally and it worked. Opened a PR: RustCrypto/elliptic-curves#1132

@tarcieri
Copy link
Contributor

IMO this can be closed (I think we discussed it at one point in the past, but now I can't find it)

@baloo
Copy link
Author

baloo commented Mar 10, 2025

Ha right by reversing the call ><.

@baloo baloo closed this Mar 10, 2025
@baloo baloo deleted the baloo/crypto-rng branch March 10, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants