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

Always use CryptoRNG as the default #126

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

Rexios80
Copy link
Contributor

Fixes #125

As mentioned here

Just want to add here that you should never generate UUID using default Random() generator. Use Random.secure() for that. Reason being that the amount of unique seeds for Random() are limited to 32 bits.

So if you have lot of clients running, it is actually not that impossible to get two clients running with same seed just by accident.

The default RNG implementation should be Random.secure() not the plain Random()

@Rexios80
Copy link
Contributor Author

It is safer to assume that anyone that needs a possible performance gain by switching to MathRNG can do so, rather than assuming everyone else knows or remembers to switch to CryptoRNG every time they use this package

@julemand101
Copy link
Contributor

Another place to fix is in the README where we have:

* **Defaults to non-crypto generator, see UuidUtil for cryptoRNG**

@daegalus
Copy link
Owner

Thank you all for the changes. Let me look back at some old tickets and changes, because there was a reason I started defaulting to the non-secure one.

Also, I need to see if this is a breaking change. If it only affects B4, it should be fine. As the only way to manipulate the seed is using MathRNG explicitly, bypassing the default.

v5 I think used v4 under the hood.

And anything with a random component uses the RNG. So v1, v6, v7, and v8 are affected accordingly.

@Rexios80
Copy link
Contributor Author

It might be tempting to release this as version 5.0.0, but I would advise against that due to the sheer number of packages that depend on this, and the severity of this issue.

It should be released as version 4.6.0. Technically this isn't a breaking change since no code will need changed after updating, even if the default behavior changed.

@Rexios80
Copy link
Contributor Author

Wait the current released version is 4.4.2, but the most recent changelog entry is 4.5.0 did you never release 4.5.0?

@daegalus
Copy link
Owner

Wait the current released version is 4.4.2, but the most recent changelog entry is 4.5.0 did you never release 4.5.0?

No, I was in the process of, but I'm in the middle of a move to Europe, and the last month or so have been a complete chaos storm and I am flying next week.

I can include it in the 4.5.0 release since there are a lot of changes there as is.

@daegalus
Copy link
Owner

Ok, I went through the history.

The original move to MathRNG as default was in 0.5.1, due to performance issues and I was using a custom AES implementation back then as the crypto package and pointycastle didnt exist yet.

I then made it default in 2.0.0 again but it broke in IE11 (which was supported at the time) because Random.secure didn't work and I had switched to it.

So I think it's fine to switch to Crypto by default now.

@daegalus daegalus merged commit 5dfbdbc into daegalus:main Aug 30, 2024
1 check passed
@daegalus
Copy link
Owner

Thanks again, I will try to make a release shortly.

@julemand101
Copy link
Contributor

The original move to MathRNG as default was in 0.5.1, due to performance issues and I was using a custom AES implementation back then as the crypto package and pointycastle didnt exist yet.

If we end up do have less performance on some platforms, I do think we can get some of that back again when this gets released at some point: https://dart-review.googlesource.com/c/sdk/+/322861

Since this will fetch enough random data to fill the list in one go instead of doing multiple calls that we are doing now.

@daegalus
Copy link
Owner

The original move to MathRNG as default was in 0.5.1, due to performance issues and I was using a custom AES implementation back then as the crypto package and pointycastle didnt exist yet.

If we end up do have less performance on some platforms, I do think we can get some of that back again when this gets released at some point: https://dart-review.googlesource.com/c/sdk/+/322861

Since this will fetch enough random data to fill the list in one go instead of doing multiple calls that we are doing now.

This looks great. I'll switch to it once it's released.

@Rexios80
Copy link
Contributor Author

@daegalus Thank you for the quick turnaround on this!

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.

Same UUIDs generated every run when compiled to WASM
3 participants