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

deps: replace rand with fastrand for fast-rng feature #774

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

roderickvd
Copy link

Replace the rand crate with fastrand in the fast-rng feature path. fastrand is a simpler and lighter-weight RNG implementation that better suits the needs of the fast-rng feature.

Replace the rand crate with fastrand in the fast-rng feature path.
fastrand is a simpler and lighter-weight RNG implementation that
better suits the needs of the fast-rng feature.
@KodrAus
Copy link
Member

KodrAus commented Nov 26, 2024

Thanks for the PR @roderickvd! We try stick to CSPRNG implementations in uuid for generating random data, so unfortunately I don't think we can use fastrand, which is not cryptographically secure. The fast-rng feature itself only lets us pull in more dependencies to support faster RNG, not weaken the algorithm we use.

@roderickvd
Copy link
Author

I understand that rationale. Maybe add it to the documentation for future onlookers like me?

I was thrown off because in some other crates, including rand itself with SmallRng, not being cryptographically secure is an acceptable compromise for a lighter-weight RNG.

I know you were not using SmallRng before but I thought the feature-flag implied that desire.

@roderickvd
Copy link
Author

Or, would you entertain having this as a feature flag additional to the current ones for those who don't need the cryptographic security?

@KodrAus
Copy link
Member

KodrAus commented Nov 28, 2024

I understand that rationale. Maybe add it to the documentation for future onlookers like me?

That's a fair point, it should've occurred to me that it could be interpreted that way when I picked the name (and just used the feature name rand). I actually just wanted to use rand for randomness because getrandom is very slow on Windows, but was met with complaints about additional dependencies. The fast-rng feature was a compromise I'd still like to undo at some point in the future.

I think we can't really do this using Cargo features unfortunately, because they're additive, so while your code may find a weaker RNG acceptable, other code that also uses UUIDs in your binary may not. I think for now, the best way to use a faster RNG is to use uuid::Builder, like in this example.

@roderickvd
Copy link
Author

Thanks for the hint on the builder.

I guess what we could do is have a fast-rng and insecure-rng. The one would pull in rand in addition to getrandom and the other fastrand in addition to getrandom. Both rand and fastrand depend on getrandom for seeding so that's OK.

I could whip it up if you find the idea interesting. Otherwise don't worry about closing this.

@KodrAus
Copy link
Member

KodrAus commented Nov 28, 2024

I guess what we could do is have a fast-rng and insecure-rng.

I'm not immediately against this idea, but still a bit uncomfortable with it making a decision about the randomness of UUIDs on behalf of your entire dependency tree. I think we should be trying to move away from using Cargo features for what are effectively API configurations, and instead encourage using the Builder for plugging in alternative approaches.

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.

2 participants