Cleaning up crate features #672

Open
opened 2023-05-06 04:40:27 -05:00 by KodrAus · 8 comments
KodrAus commented 2023-05-06 04:40:27 -05:00 (Migrated from github.com)

This is a meta issue to take a fresh look at the library’s dependency tree. In particular:

  • rng support: we have a fast-rng feature that adds rand as a dependency because getrandom is slow on Windows. Is that still necessary?
  • js: we pull in wasm-bindgen and getrandom unconditionally when building for the web. Could we use our crate features more cleverly to only pull in features we need?
  • macros: we have a macro-diagnostics feature that enables a proc-macro for better error reporting of compile-time UUID literals. Is this actually used? Can we emit more meaningful errors without needing a proc-macro on more recent compilers?

We can consider a major 2.x version of uuid that is semver-compatible with 1.x (no actual API changes and re-export 2.x in 1.x), but that removes or rejigs our crate features as necessary to keep our dependency tree sane.

This is a meta issue to take a fresh look at the library’s dependency tree. In particular: - rng support: we have a `fast-rng` feature that adds `rand` as a dependency because `getrandom` is slow on Windows. Is that still necessary? - js: we pull in `wasm-bindgen` and `getrandom` unconditionally when building for the web. Could we use our crate features more cleverly to only pull in features we need? - macros: we have a `macro-diagnostics` feature that enables a proc-macro for better error reporting of compile-time UUID literals. Is this actually used? Can we emit more meaningful errors without needing a proc-macro on more recent compilers? We can consider a major 2.x version of `uuid` that is semver-compatible with 1.x (no actual API changes and re-export 2.x in 1.x), but that removes or rejigs our crate features as necessary to keep our dependency tree sane.
TylerBloom commented 2023-07-17 11:54:18 -05:00 (Migrated from github.com)

js: we pull in wasm-bindgen and getrandom unconditionally when building for the web. Could we use our crate features more cleverly to only pull in features we need?

From my understanding, you only need the js when you are compiling to WASM and need the rnd feature. (You might need it for atomics too, but the following still holds with a few additions). So, we can change the definition of the rnd feature to be: rnd = ["wasm-bindgen", "getrandom"]. Then, the dependencies imports can be gated by target:

rnd = ["wasm-bindgen", "getrandom"]

[dependencies]
getrandom = { version = "0.2" }

[target.'cfg(target_arch = "wasm32")'.dependencies]
getrandom = { version = "0.2", features = ["js"], optional = true }
wasm-bindgen = { version = "0.2", optional = true }

This also makes the js feature obsolete.

> js: we pull in wasm-bindgen and getrandom unconditionally when building for the web. Could we use our crate features more cleverly to only pull in features we need? From my understanding, you only need the `js` when you are compiling to WASM and need the `rnd` feature. (You might need it for atomics too, but the following still holds with a few additions). So, we can change the definition of the `rnd` feature to be: `rnd = ["wasm-bindgen", "getrandom"]`. Then, the dependencies imports can be gated by target: ```toml rnd = ["wasm-bindgen", "getrandom"] [dependencies] getrandom = { version = "0.2" } [target.'cfg(target_arch = "wasm32")'.dependencies] getrandom = { version = "0.2", features = ["js"], optional = true } wasm-bindgen = { version = "0.2", optional = true } ``` This also makes the `js` feature obsolete.
KodrAus commented 2024-01-18 16:22:24 -06:00 (Migrated from github.com)

That's interesting 🤔 Would that rnd feature simply ignore the wasm-bindgen and getrandom features on targets that don't specify them as dependencies?

That's interesting 🤔 Would that `rnd` feature simply ignore the `wasm-bindgen` and `getrandom` features on targets that don't specify them as dependencies?
TylerBloom commented 2024-01-18 17:45:35 -06:00 (Migrated from github.com)

That's interesting 🤔 Would that rnd feature simply ignore the wasm-bindgen and getrandom features on targets that don't specify them as dependencies?

Ya, that's correct. I don't know how this applies to user-selected features (i.e. features that you specify via cargo's --feature flag), but, for features that enable other features, cargo doesn't care. That or cargo is aware that wasm-bindgen is a valid feature sometimes, so if it gets specified and is not used that's ok.

> That's interesting 🤔 Would that `rnd` feature simply ignore the `wasm-bindgen` and `getrandom` features on targets that don't specify them as dependencies? Ya, that's correct. I don't know how this applies to user-selected features (i.e. features that you specify via cargo's `--feature` flag), but, for features that enable other features, cargo doesn't care. That or cargo is aware that `wasm-bindgen` is a valid feature sometimes, so if it gets specified and is not used that's ok.
roderickvd commented 2024-11-25 14:33:24 -06:00 (Migrated from github.com)

For fast-rng with rand you would want to use SmallRng but we can do better: use fastrand instead. It's considerably lighter-weight than rand. I can put in a PR.

For `fast-rng` with `rand` you would want to use [`SmallRng`](https://docs.rs/rand/latest/rand/rngs/struct.SmallRng.html) but we can do better: use [`fastrand`](https://crates.io/crates/fastrand) instead. It's considerably lighter-weight than `rand`. I can put in a PR.
vks commented 2025-03-11 10:44:56 -05:00 (Migrated from github.com)

For fast-rng with rand you would want to use SmallRng but we can do better: use fastrand instead. It's considerably lighter-weight than rand. I can put in a PR.

I don't think this is a good idea. SmallRng and fastrand use predictable RNGs, which means any UUID generated by them would be predictable. This can result in security issues for some UUID v4 applications.

> For fast-rng with rand you would want to use SmallRng but we can do better: use fastrand instead. It's considerably lighter-weight than rand. I can put in a PR. I don't think this is a good idea. `SmallRng` and `fastrand` use predictable RNGs, which means any UUID generated by them would be predictable. This can result in security issues for some UUID v4 applications.
roderickvd commented 2025-03-11 10:49:32 -05:00 (Migrated from github.com)

PR was already closed, so not pursuing it anymore.

PR was already closed, so not pursuing it anymore.
marcomq commented 2026-02-05 01:20:37 -06:00 (Migrated from github.com)

I don't think this is a good idea. SmallRng and fastrand use predictable RNGs, which means any UUID generated by them would be predictable. This can result in security issues for some UUID v4 applications.

Couldn't it be an issue when using fast-rng just for some local high-throughput location? When using fast-rng as feature, you might use it everywhere by accident. It might even be injected into another authentication crate you are using that is using uuid-v4 for token generation. And then, this authentication crate would also generate uuid-v4 tokens with fast-rng as the uuid version might fit and features are just considered optional and non destructive.

Wouldn't it be better to have explicit fast versions as separate function, separate module or maybe even separate crate?
I just created an own fast-uuid-v7 crate because I didn't knew fast-rng existed. But I probably would also avoid such a feature for security reasons.

> I don't think this is a good idea. SmallRng and fastrand use predictable RNGs, which means any UUID generated by them would be predictable. This can result in security issues for some UUID v4 applications. Couldn't it be an issue when using `fast-rng` just for some local high-throughput location? When using `fast-rng` as feature, you might use it everywhere by accident. It might even be injected into another authentication crate you are using that is using uuid-v4 for token generation. And then, this authentication crate would also generate uuid-v4 tokens with `fast-rng` as the `uuid` version might fit and features are just considered optional and non destructive. Wouldn't it be better to have explicit fast versions as separate function, separate module or maybe even separate crate? I just created an own fast-uuid-v7 crate because I didn't knew `fast-rng` existed. But I probably would also avoid such a feature for security reasons.
KodrAus commented 2026-02-05 19:02:25 -06:00 (Migrated from github.com)

@marcomq We wouldn't make the source of the fast-rng feature any less secure than the default. It's a feature that's mostly only useful on Windows, but may not even be necessary there anymore now that getrandom appears to be using a different underlying Windows API.

@marcomq We wouldn't make the source of the `fast-rng` feature any less secure than the default. It's a feature that's mostly only useful on Windows, but may not even be necessary there anymore now that `getrandom` appears to be using a different underlying Windows API.
Sign in to join this conversation.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-uuid-rs/uuid#672
No description provided.