Add OsRng to getrandom #751

dhardy · 2025-11-10T16:07:59Z

This was mentioned elsewhere: rust-random/rand#1675

Adds rand_core as a dependency, which is one small crate.

This attempts to enable testing of rng in all CI tests.

dhardy · 2025-11-18T08:55:09Z

@newpavlov your opinion on this please?

There is an alternative: the rand_os crate. I slightly prefer fewer crates but either is acceptable.

newpavlov · 2025-11-18T15:44:37Z

As I wrote previously, personally, I have a slight preference for a separate crate. But I would be fine with this PR as well, if others prefer it.

P.S.: Note that rand_os recently got an "unmaintained" advisory: rustsec/advisory-db#2462

josephlr

As I wrote previously, personally, I have a slight preference for a separate crate. But I would be fine with this PR as well, if others prefer it.

I think I would slightly lean towards having this as part of getrandom. A non-default additive feature seems like a good fit. I think the only complexity is figuring out what (if anything) this crate should re-export from rand_core.

josephlr · 2025-11-19T07:57:19Z

.github/workflows/tests.yml

      - run: cargo test
      # Make sure enabling the std feature doesn't break anything
-      - run: cargo test --features=std
+      - run: cargo test --features=std,rng


Should we have a test somewhere that rng works without the std feature? Maybe here we could have:

Suggested change

- run: cargo test --features=std,rng

- run: cargo test --features=std

- run: cargo test --features=rng

- run: cargo test --features=std,rng

I agree that having most of the testing use --features=std,rng is correct.

josephlr · 2025-11-19T08:05:43Z

Cargo.toml

 ]

 [package.metadata.docs.rs]
 features = ["std"]


This should be updated to include "rng" so that users can see the generated docs for stuff gated behind rng

josephlr · 2025-11-19T08:11:40Z

src/rng.rs

+/// An RNG over the operating-system's random data source
+///
+/// This is a zero-sized struct. It can be freely constructed with just `OsRng`.
+///
+/// This struct is also available as [`rand::rngs::OsRng`] when using [rand].
+///
+/// # Usage example
+/// ```
+/// use getrandom::{rand_core::{TryRngCore, RngCore}, OsRng};
+///
+/// let mut key = [0u8; 32];
+/// OsRng.try_fill_bytes(&mut key).unwrap();
+///
+/// let mut rng = OsRng.unwrap_err();
+/// let random_u64 = rng.next_u64();
+/// ```
+///
+/// [rand]: https://crates.io/crates/rand
+/// [`rand::rngs::OsRng`]: https://docs.rs/rand/latest/rand/rngs/struct.OsRng.html


Similar to the docs for getrandom::Error, we should mention that this will require a crate to enable the rng feature. We should also mention here if the rng feature will have different MSRV or stability guarantees going forward. Are we allowed to increase the minor version of rand_core (from say 0.10 to 0.11) when moving from getrandom 0.4.0 to getrandom 0.4.1?

Not sure what our policy should be, but we should document it.

doc-auto-cfg was stabilised so docs will note that OsRng requires feature rng, if that's what you mean.

Are we allowed to increase the minor version of rand_core (from say 0.10 to 0.11) when moving from getrandom 0.4.0 to getrandom 0.4.1?

That would break semver since rand_core_0_10::TryRngCore and rand_core_0_11::TryRngCore are considered separate traits, so it should be clear that we cannot do that.

We should also mention here if the rng feature will have different MSRV or stability guarantees going forward.

I don't see why it would.

doc-auto-cfg was stabilised

Are you sure? I thought it was just merged into doc_cfg. In RustCrypto we still use #![cfg_attr(docsrs, feature(doc_cfg))].

josephlr · 2025-11-19T08:42:09Z

src/rng.rs

+#[test]
+fn test_os_rng() {
+    let x = OsRng.try_next_u64().unwrap();
+    let y = OsRng.try_next_u64().unwrap();
+    assert!(x != 0);
+    assert!(x != y);
+}
+
+#[test]
+fn test_construction() {
+    assert!(OsRng.try_next_u64().unwrap() != 0);
+}


These should be in a mod test with #[cfg(test)]

josephlr · 2025-11-19T08:46:24Z

src/lib.rs

+#[cfg(feature = "rng")]
+mod rng;
+#[cfg(feature = "rng")]
+pub extern crate rand_core;


Suggested change

pub extern crate rand_core;

pub use rand_core;

This is necessary for rustdoc to correctly:

Render descriptions for all the other items

Mark this re-export as gated behind the rng feature.

Mark this re-export as gated behind the rng feature.

It's not, but it does appear to cause render issues (all following text is fixed width / code). Weird.

josephlr · 2025-11-19T08:53:12Z

src/rng.rs

+/// # Usage example
+/// ```
+/// use getrandom::{rand_core::{TryRngCore, RngCore}, OsRng};


Suggested change

/// # Usage example

/// ```

/// use getrandom::{rand_core::{TryRngCore, RngCore}, OsRng};

/// # Usage example

///

/// The functionality of this struct is only accessible though its

/// [`TryRngCore`] implementation:

/// ```

/// use getrandom::{OsRng, rand_core::TryRngCore};

We should make it clear that the only way to use OsRng is via TryRngCore.

josephlr · 2025-11-19T09:03:55Z

src/lib.rs

+#[cfg(feature = "rng")]
+pub extern crate rand_core;


Is there a reason to re-export all of rand_core? Would it make more sense to only re-export TryRngCore as that's the only non-marker trait OsRng implements?

We could then also add additional documentation explaining the re-export:

Suggested change

#[cfg(feature = "rng")]

pub extern crate rand_core;

#[cfg(feature = "rng")]

/// Re-exported base RNG trait from [`rand_core`]

///

pub use rand_core::TryRngCore;

@newpavlov convinced me in the past that this is a good general policy, and I agree:

It allows downstream crates to access rand_core functionality without explicitly depending on that

It does not pretend that TryRngCore is part of getrandom

It reduces the chance of people hitting "multiple versions of rand_core" errors

IMO re-exporting rand_core is a must have and should be done by all RNG implementation crates.

dhardy · 2025-11-19T10:24:04Z

Thanks for moving this forward @josephlr. I think I addressed your concerns (except as commented).

newpavlov · 2025-11-19T10:40:51Z

src/rng.rs

+/// [`rand::rngs::OsRng`]: https://docs.rs/rand/latest/rand/rngs/struct.OsRng.html
+/// [`RngCore`]: rand_core::RngCore
+#[derive(Clone, Copy, Debug, Default)]
+pub struct OsRng;


I would prefer for it to be named as SysRng, since OsRng can be a confusing name on no_std targets. To make migration easier we could add type OsRng = SysRng; alias in rand (with or without #[deprecated] attribute).

Makes some sense... some backends like RDRAND are not OS features.

@josephlr?

newpavlov · 2025-11-19T10:41:49Z

Cargo.toml

 wasm_js = ["dep:wasm-bindgen", "dep:js-sys"]

+# Provide OsRng over rand_core
+rng = ["dep:rand_core"]


How about naming the feature os_rng (or sys_rng if the type will be renamed)?

newpavlov · 2025-11-19T10:46:46Z

src/rng.rs

+impl TryCryptoRng for OsRng {}
+
+#[cfg(test)]
+mod test {


I think it's better to define these tests in tests/rng.rs or integrate them into the doctest.

Motivation? Having tests close to the feature definition is slightly preferable in my opinion (where tests are not too extensive at least).

My general preference is to keep public API tests in integration tests (i.e. in the tests folder) and/or doctest, while using test modules (i.e. unit tests) only for private API testing.

Since these tests are quite small, I think they can be simply merged into the OsRng doctest, potentially with # used to hide asserts.

If the motivation is to improve visibility of integration tests, then moving to tests makes sense.

I'm disinclined to push these into a doc-test since (a) it hides it, (b) I would normally assume that doc-tests are only for documentation purposes and (c) failure reporting for doc tests is worse (slower and worse messages) than for other types of test.

So I don't understand why you prefer both integration tests and doc tests over a local test module.

It's just a matter of not duplicating tests which are already covered by doctests. I am fine with keeping the example as-is and moving the test module into the tests/ folder.

josephlr requested changes Nov 19, 2025

View reviewed changes

dhardy added 3 commits November 19, 2025 09:56

Add rand_core as an optional dependency

d4a480a

Add OsRng

d2f6a1d

Test rng feature in CI

bdbe38d

dhardy force-pushed the push-tqlwtzsrywpy branch from 654872c to 5b2ab43 Compare November 19, 2025 10:22

dhardy added 5 commits November 19, 2025 10:25

Test std / rng / std,rng feature combinations

26cb7e9

Use rng feature in docs builds

254be58

Use test module

ff68f2a

pub use rand_core

8bdcacf

Document what OsRng implements

1e873b4

dhardy force-pushed the push-tqlwtzsrywpy branch from 5b2ab43 to 1e873b4 Compare November 19, 2025 10:25

newpavlov reviewed Nov 19, 2025

View reviewed changes

Add OsRng to getrandom #751

Are you sure you want to change the base?

Add OsRng to getrandom #751

Conversation

dhardy commented Nov 10, 2025

Uh oh!

dhardy commented Nov 18, 2025

Uh oh!

newpavlov commented Nov 18, 2025

Uh oh!

josephlr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephlr Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephlr Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardy commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardy Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

josephlr Nov 19, 2025 •

edited

Loading

josephlr Nov 19, 2025 •

edited

Loading

dhardy Nov 19, 2025 •

edited

Loading