Tracking Issue for secure random data generation in std

[joboet]

Feature gate: #![feature(random)]

This is a tracking issue for secure random data generation support in std.

Central to this feature are the Random and RandomSource traits inside core::random. The Random trait defines a method to create a new random value of the implementing type from random bytes generated by a RandomSource. std also exposes the platform's secure random number generator via the DefaultRandomSource type which can be conveniently access via the random::random function.

Public API

// core::random

pub trait RandomSource {
    fn fill_bytes(&mut self, bytes: &mut [u8]);
}

pub trait Random {
    fn random(source: &mut (impl RandomSource + ?Sized)) -> Self;
}

impl Random for bool { ... }
impl Random for /* all integer types */ { ... }

// std::random (additionally)

pub struct DefaultRandomSource;

impl RandomSource for DefaultRandomSource { ... }

pub fn random<T: Random>() -> T { ... }

Steps / History

• ACP: Simple secure random number generation libs-team#393
• Implementation: std: implement the random feature (alternative version) #129201
• Final comment period (FCP)¹
• Stabilization PR

Unresolved Questions

• Naming: the ACP used gen_bytes and DefaultRng, the implementation PR uses fill_bytes and DefaultRandomSource (see arguments pro gen_bytes and pro fill_bytes)
• Concerns listed at Simple secure random number generation libs-team#393 (comment) should be addressed

Footnotes

1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

[newpavlov]

Disclaimer: I am one of the getrandom developers.

I think it's important for RandomSource methods to properly return potential errors. Getting randomness is an IO operation and it may fail. In some context it's important to process such errors instead of panicking. The error may be either io::Error or something like getrandom::Error (i.e. a thin wrapper around NonZeroU32).

It may be worth to add the following methods to RandomSource:

• fill_bytes which works with uninitialized buffers, e.g. based on BorrowedBuf. Yes, zeroization of buffers is usually a very small cost compared to a syscall, but it still goes against the zero-cost spirit.
• Generation of u32 and u64. Some platforms support direct generation of such values (e.g. RDRAND, WASI, etc.). Going through fill_bytes will be a bit less efficient in such cases.
• Methods for potentially "insecure" generation of random values, but which are less prone to blocking. The HashMap seeding is the most obvious use-case for this.

It's also not clear whether it's allowed to overwrite the default RandomSource supplied with std similarly to GlobalAlloc.

[joboet]

Would you consider rust-lang/libs-team#159 to be a better solution? That one used the Read trait to fulfil everything you mention.

[newpavlov]

No, I don't think it's an appropriate solution. Firstly, it relies on io::Error, while IIUC you intend for this API to be available in core. Secondly, it does not provide methods for generation of u32 and u64. As I wrote, going through the byte interface is not always efficient. Finally, most of io::Read methods are not relevant here.

For the last point I guess we could define a separate DefaultInsecureRandomSource type.

[bstrie]

Firstly, it relies on io::Error, while IIUC you intend for this API to be available in core.

I don't think this needs to be a blocker. IMO a lot of std::io should be moved into core--not the OS-specific implementations obviously, but all the cross-platform things like type definitions, same as what happened with core::net.

[wwylele]

fn random(source: &mut (impl RandomSource + ?Sized)) -> Self;

Would it be better to make this explicit with generics? Like fn random<R: RandomSource + ?Sized>(source: &mut R) -> Self;. This gives user the ability to specify the type name when needed.

[ericlagergren]

I think it's important for RandomSource methods to properly return potential errors.

I (mostly) disagree. CSPRNGs should almost never fail. When they do, users are almost never qualified to diagnose the problem.

For example: golang/go#66821

A compromise is something like this:

trait RandomSource {
    type Error;
    fn fill_bytes(...) {
        self.try_fill_bytes(...).unwrap();
    }
    fn try_fill_bytes(...) -> Result<..., Self::Error>
}

This allows most CSPRNGs to use Error = Infallible, but still has support for weird HSMs, etc.

[newpavlov]

@ericlagergren
I've assumed this trait is for a "system" RNG, which will work together with a #[global_allocator]-like way to register implementation. I don't think that we need a general RNG trait in std/core as I wrote in this comment.

As for design of fallible RNG traits, see the new rand_core crate.

[dhardy]

pub trait Random {
    fn random(source: &mut (impl RandomSource + ?Sized)) -> Self;
}

This trait (and the topic of random value generation) should be removed from this discussion entirely in my opinion, focussing only on "secure random data generation" as in the title. Why: because (1) provision of secure random data is an important topic by itself (with many users only wanting a byte slice and with methods like from_ne_bytes already providing safe conversion) and (2) because random value generation is a whole other topic (including uniform-ranged samples and much more).

Disclaimer: I am one of the rand developers. rand originally had a similar trait which got removed; the closest surviving equivalent is StandardUniform.

[hanna-kruppe]

@newpavlov

It's also not clear whether it's allowed to overwrite the default RandomSource supplied with std similarly to GlobalAlloc.

Overriding the default source in an application that already has one from linking std seems questionable. It's not that I can't imagine any use case for it, but the established pattern for such overrides allows any crate in the dependency graph to do it (it's only an error if you link two such crates), instead of putting the leaf binary/cdylib/staticlib artifact in charge. As you articulated in the context of getrandom, that's a security risk for applications. So a RandomSource equivalent should probably be more restrictive in who can override it, but that's not the existing pattern. It also doesn't seem to fit with the proposed generalization of that pattern via "externally implementable functions" (rust-lang/rfcs#3632) -- if that RFC is accepted, any new API surface should use it instead of adding new one-off mechanisms.

If overriding the std source isn't supported, then it could work the same way as #[panic_handler]: you must supply an implementation if you don't link std, but if you do link std then supplying your own is an error. This would still be extremely useful. Currently, every crate that's (optionally) no_std and needs some randomness, most commonly for seeding Hashers, has to cobble together some sub-par ad-hoc solution to try and get some entropy from somewhere. There's a bunch of partial solutions that are better than nothing (const-random, taking addresses of global/local variables and praying that there's some ASLR, a global counter when atomics are available, cfg-gated access to target-specific sources like CPU cycle counters or x86 RDRAND) but:

1. How well this works ends up highly platform-specific, in particular none of them work well for wasm32-unknown-unknown and wasm32v1-none targets .
2. Applications that have access to a better source of entropy and (directly or transitively) use such libraries don't have a good way to enumerate them and make all of them use the better source.

This wouldn't be a problem if the entire ecosystem could agree to always delegate this problem to on one specific crate (version) with appropriate hooks, like getrandom, but evidently that's not happening. Putting this capability into core (or a new no_std sysroot crate, comparable to alloc) has a better chance of solving this coordination problem. Well, at least eventually, once everyone's MSRV has caught up.

Edit: almost forgot that even std::collections::Hash{Map,Set} depend on having a source of random seeds. A way to supply such a source without linking std could help with moving those types to alloc, although as #27242 (comment) points out, it's not backwards compatible to make such a source mandatory for no_std + alloc applications.

[newpavlov]

@hanna-kruppe

Overriding the default source in an application that already has one from linking std seems questionable.

There is a number of reasons to allow overriding:

1. An alternative interface may be more efficient than the default one (e.g. reading the RNDR register vs doing syscall)
2. It may help reduce binary size and eliminate potentially problematic fallback paths (e.g. if you know that you do not need the file fallback on Linux)
3. In some cases it's useful to eliminate non-deterministic inputs (testing, fuzzing)

So a RandomSource equivalent should probably be more restrictive in who can override it, but that's not the existing pattern.

Yes. How about following the getrandom path and allow override only when a special configuration flag is passed to the compiler?

Either way, overriding is probably can be left for later. I think we both agree that we need a way to expose "system" entropy source in std and a way to define this source for std-less targets.

It also doesn't seem to fit with the proposed generalization of that pattern via "externally implementable functions" -- if that RFC is accepted, any new API surface should use it instead of adding new one-off mechanisms.

I agree that ideally we need a unified approach for this kind of problem. I made a similar proposal once upon a time.

But I think it fits fine? Targets with std could implicitly use std_random_impl crate for "external implementation" of the getrandom-like functions and users will be able to override it in application crates if necessary.

How well this works ends up highly platform-specific, in particular none of them work well for wasm32-unknown-unknown and wasm32v1-none targets .

I believe that having std for wasm32-unknown-unknown was a big mistake in the first place and the wasm32v1-none target is a good step in the direction of amending it. So I hope we will not give too much attention to its special circumstances.

This wouldn't be a problem if the entire ecosystem could agree to always delegate this problem to on one specific crate (version) with appropriate hooks, like getrandom, but evidently that's not happening.

Well, it has happened, sort of. getrandom is reasonably popular in the ecosystem even after excluding rand users.

The problem is that std already effectively includes its variant of getrandom for HashMap seeding and people reasonably want to get access to that. And I think problem of getting "system" entropy is fundamental enough for having it in std (well, not in the std per se, let's say in the sysroot crate set).

A way to supply such a source without linking std could help with moving those types to alloc, although as #27242 (comment) points out, it's not backwards compatible to make such a source mandatory for no_std + alloc applications.

Can we add yet another sysroot crate for HashMap which will depend on both alloc and the hypothetical "system entropy" crate?

[hanna-kruppe]

But I think it fits fine? Targets with std could implicitly use std_random_impl crate for "external implementation" of the getrandom-like functions and users will be able to override it in application crates if necessary.

The RFC (and the competing ones I've looked at) only supports a default implementation in the crate that "declares" the externally-implementable thing. If that crate isn't std, then an implementation from std would not count as "default" but conflict with any other definition. So we'd need another special carve-out for std (the very thing we'd want to avoid by adding a general language feature), or the language feature needs to become much more general to support overrideable default implementations from another source.

I believe that having std for wasm32-unknown-unknown was a big mistake in the first place and the wasm32v1-none target is a good step in the direction of amending it. So I hope we will not give too much attention to its special circumstances.

I was specifically talking about no_std libraries, for which the two targets are basically equivalent. Both don't have any source of entropy implied by the target tuple (instruction set, OS, env, etc.), and if you want to add one it'll have to involve whatever application-specific interface the wasm module has with its host.

Well, it has happened, sort of. getrandom is reasonably popular in the ecosystem even after excluding rand users.

Not to point any fingers but a counter example that's fresh on my mind because I looked at its code recently is foldhash. As another example, ahash only uses getrandom optionally (though it's on by default). If you're only using ahash indirectly through another library that disables the feature, then it's not gonna use getrandom unless you happen to notice this and add a direct dependency to enable the feature. In that case there is a solution, at least, but it's still not discoverable.

Can we add yet another sysroot crate for HashMap which will depend on both alloc and the hypothetical "system entropy" crate?

Possibly, but people may object to a proliferation of sysroot crates so let's hope there's a better solution.

[newpavlov]

or the language feature needs to become much more general to support overrideable default implementations from another source.

Yes, it's not as if RFC is a technical specification which must be followed word-by-word. There is a number of cases where the original RFC vision has somewhat changed during implementation stages. If anything, I would say it's an oversight/deficiency of the RFC to not cover cases like this.

foldhash

If a crate aims to minimize its number of dependencies as far as possible even at the cost of code quality and security, it obviously will not depend on getrandom, despite it being the de facto standard for getting system entropy. I think most people will agree that hacks like this have a strong smell. The same applies to fastrand (amusingly, it still uses getrandom for Web WASM) and other "partial solutions" listed by you. As you can notice, both hack their way into using std to get system entropy and either use a fixed seed or pile even more hacks when std is not available.

[hanna-kruppe]

As I said, I have no intention of pointing fingers at any crates. They have to navigate tricky trade-offs and complexities due to Rust's standard library (as a whole, not just std) not yet providing any entropy access. Let's keep this issue focused on changing that.

[workingjubilee]

The Random trait seems weakly motivated in terms of coupling it to RandomSource, as its design seems like it will be a much more hotly contested space, and it is (mostly) unrelated to RandomSource.

[theemathas]

Note that @dhardy (maintainer of rand) wrote some criticism of this at rust-lang/libs-team#393 (comment)

[abgros]

Could Random be implemented for arrays? That way, we could write something like:

let random_array: [u64; 100] = random();

which is a lot more convenient than using the fill_bytes() method.

[sorairolake]

I think it would be useful to have a data type like rand::rngs::mock::StepRng to represent a source of randomness. This is useful for testing whether Random for an arbitrary data type T is implemented as expected.

[dhardy]

I think it would be useful to have a data type like rand::rngs::mock::StepRng to represent a source of randomness. This is useful for testing whether Random for an arbitrary data type T is implemented as expected.

I disagree that it is useful in testing implementations of traits like Random, since there should not be any implied restrictions on such a distribution except that outputs are uniformly distributed, given that the outputs of the random source are truly random. Testing the distribution of output values requires statistical tools, e.g. the KS test as we have here.

[hanna-kruppe]

As far as I know there’s no plan to make the RandomSource trait sealed, so other implementations can be provided by third party crates without having to debate whether it’s a good idea for std to include them.

[jstarks]

It seems like one might regret adding Random later--for some times (e.g., u32), there's a natural definition and implementation that produces a uniform distribution, but it happens to correspond to using RandomSource with something like zerocopy/bytemuck/the safe transmute effort. I.e., there's already a reasonable solution for these types.

For more complicated types (e.g., f32 or (T, U)), Random is under-defined. It is unclear what distribution is expected, and without implementations on tuples or a derive for enums/structs, there's really no "reference implementation" for third-party code to model.

It seems better to leave this out rather than leave it in this poorly specified state. Just stabilize RandomSource.

Although... even RandomSource is lacking, in that it doesn't support writing to an uninitialized buffer... Would it be better to wait until this problem is solved in Read (#78485) before committing to this new interface? Adding BorrowedBuf support later will be a headache.

[hanna-kruppe]

Would it be better to wait until this problem is solved in Read (#78485) before committing to this new interface?

I really don't think so. The current interface works perfectly fine, it's just slightly suboptimal in some use cases (fewer than in the context of general byte-centric I/O). Providing only a Read::read_buf-style API would be strictly worse for many common use cases, in that it would be more complicated to use for no benefit. Usually you either want to generate one conveniently-sized integer at a time (probably should have a dedicated trait method for performance reasons), or you want to fill a small fixed-size buffer completely (e.g., cryptographic key material). Scenarios where the cost of buffer initialization is significant, e.g. preparing a large ephemeral buffer but only filling a small part of it before discarding it, are rare.

So I think it's pretty clear that the method for filling a &mut [u8] should exist even if io::Read had already arrived at a stable solution for reading into uninitialized buffers. Adding the latter after stabilizing the former does pose some challenges, but io::Read already has to solve similar challenges -- more of them, actually, because it also has to work well for several layers of I/O adapters, while PRNGs are rarely composed in a way that stresses the performance of data flowing through several layers of composition.

[Kixunil]

@hanna-kruppe

Perhaps something like this would work:

// Users don't need to concern themselves with this type.
pub strut OutBuf([MaybeUninit<u8>]);

impl OutBuf {
    // various methods to construct it and write to it, no method to read from it
}

// Users don't need to worry about implementing this, just use it
pub unsafe trait AsOutBuf {
    type Init: ?Sized;

    fn as_out_buf(&mut self) -> &mut OutBuf;
    // Only allowed if the entire buffer was overwritten.
    unsafe fn assume_init(&mut self) -> &mut Self::Init;
}

impl AsOutBuf for [u8] { type Init = [u8]; ... }
impl AsOutBuf for [MaybeUninit<u8>] { type Init = [u8]; ... }
impl<const N: usize> AsOutBuf for [u8; N] { type Init = [u8; N]; ... }
impl<const N: usize> AsOutBuf for [MaybeUninit<u8>; N] { type Init = [u8; N]; ... }

// Users can pass in any byte slice, byte array or their uninitialized counterparts
// The entire value is guaranteed to get overwritten, so if using the returned value is undesirable due to borrowing issues the users can simply use the original value safely, if it was an initialized type to begin with or they may just soundly call `assume_init` on it.
pub fn fill_random_bytes<T: AsOutBuf>(buf: &mut T) -> &mut T::Init { ... }

This way it already supports all reasonable cases and people are not forced into using uninitialized API. They can write let mut buf = [0; 32]; fill_random_bytes(&mut buf); exactly as if they were using the simple API or calling Read::read_exact.

[hanna-kruppe]

That helps the simple call sites remain simple, but it has several downsides:

1. The more complicated signature makes it less obvious that it can be used like that for simple cases, both for users who aren't reading the rustdoc page from start to finish, and also to the compiler's type inference in some cases.
2. The reliance on generics means RandomSource is no longer dyn-compatible. This could be fixed by gating the AsOutBuf-using methods on Self: Sized and providing other methods that are dyn-compatible, but at that point you might as well provide two concrete methods for the initialized and possibly-uninitialized case.
3. The new abstractions compete with Borrowed{Buf,Cursor} and other proposed APIs for "writing into a possibly-uninitialized buffer". std should minimize the number of subtly different and incompatible abstractions for the same problem space, especially if it's something third party libraries would want to use as vocabulary type/trait. Going in this direction would give more reasons to block stabilization of secure random data generation on other APIs that have been in limbo for a long time.
4. Even if we ignore the previous point and focus only on the narrow use case of RandomSource, there's considerable design space to be explored there. For example, a downside of the specific approach you outlined is that it forces every RandomSource implementation to use unsafe internally while also requiring every caller who wants to use the MaybeUninit capabilities to use another bit of unsafe justified by "the implementation is correct" (and we'd have to make RandomSource unsafe to implement to make this sound). In contrast, the more complicated interface of Borrowed{Buf,Cursor} provides a bunch of useful functionality as safe APIs.

[newpavlov]

I think that makes sense, but only if we want to add other randomness functions in the future (otherwise it'd be a module with just one function)

Yes, this is why it's important to decide whether we plan to add rand_core-like traits in future or not. Frankly, std::io is a weird place for a piece of functionality which arguably should have a separate sysroot crate.

I think the specific anti-pattern that we want to avoid is something like random::secure_random() vs. random::random() or some other API distinction where users who don't have deep familiarity with CSPRNGs will find the non-CSPRNG to be the obvious API.

Note that I intentionally used read_insecure_sys_entropy following the "syntactic salt" principle. Also, Rust is very different from Python. For example, in cryptographic libraries (e.g. RustCrypto) we heavily rely on the CryptoRng marker trait. Users simply are not able to accidentally pass a weak RNG into such APIs, their code will be rejected at compile time.

[alex]

The different traits work great for APIs that take an RNG as an argument. But there's also plenty of usecases for things like "I would like 16 random bytes please", and for those it can be relatively easy to mix up a CSPRNG vs. a non-CSPRNG.

[jstarks]

I agree with the limited scope, i.e. starting with a relatively simple potentially panicking function with &mut [u8]-based API. But it may be worth to introduce two functions: read_sys_entropy and read_insecure_sys_entropy. The latter function would expose the code which is currently used by HashMap, while the former would need to be implemented separately on targets which support non-blocking entropy generation.

Keep it simple, leave out read_insecure_sys_entropy for now. It's highly specialized, its behavior is hard to specify, and it invites misuse.

You can always add it later if it really becomes important (i.e., to make it easier for non-std random-seeded data structures to be written to run on obscure platforms or early in boot on Linux--is this really that common a use case?).

[dhardy]

The exact path (module, name) seems less important than the question raised about scope of the feature.

The scope is what I would like to see: an interface suitable for initializing a CSPRNG, rendering the getrandom crate largely obsolete (for most uses I think; some may make use of the u32 / u64 / fill_uninit interfaces or support for custom backends).

I agree with @newpavlov: std::random::read_sys_entropy is a good name (saying at a glance that this is an interface over an external random data source).

[newpavlov]

Keep it simple, leave out read_insecure_sys_entropy for now. It's highly specialized, its behavior is hard to specify, and it invites misuse.

No one says it should be stabilized together with read_sys_entropy. The main argument for exposing it is that std uses it internally already.

And it's pretty easy to specify in my opinion: "reasonable" effort randomness source which may emit predictable random numbers in the worst case scenario, to the point of using PRNG seeded with a fixed seed (e.g. on WASM targets).

I would say we have a bigger issue with read_sys_entropy. Not all std targets provide a suitable API for it. WASM is the most prominent example, but there is also non-x86 Hermit and maybe some other targets.

[hanna-kruppe]

I would say we have a bigger issue with read_sys_entropy. Not all std targets provide a suitable API for it. WASM is the most prominent example, but there is also non-x86 Hermit and maybe some other targets.

This was discussed before, see #130703 (comment) and surrounding comments -- it affects many other std APIs on various targets besides wasm and is generally handled by panicking or returning an error. This tracking issue is really getting far too long, even the people who have been involved in it the entire time are going in circles because it's become so unwieldy 🙃

[Kixunil]

endering the getrandom crate largely obsolete

I might be biased but no_std crates are quite frequent. It'd suck if they still had to use getrandom. (Though std is a good start.)

@newpavlov I don't get what's the deal with WASM. It's not OS/hardware, so it shouldn't even have std but specific platform like wasi does and that one also has some sort of randomness: https://github.com/WebAssembly/wasi-random

[ericlagergren]

This issue is for secure random data generation: reading bytes directly from the system's CSPRNG. Everything else should happen in a separate issue.

[newpavlov]

It's not OS/hardware, so it shouldn't even have std but specific platform like wasi does and that one also has some sort of randomness

I agree, but the unfortunate reality is that Rust does provide std for wasm32-unknown-unknown (I strongly believe it was a mistake, but alas) and will continue to do so because of the backward compatibility guarantees. Yes, we could provide an always panicking implementation of read_sys_entropy on this target, but considering that we don't have a clear path towards a #[global_allocator]-like ability to overwrite the system entropy source, usefulness of the introduced function will be limited. People would have to continue to rely on crates like getrandom for a more universal interface.

Imagine a function/method like KeyInit::generate_key which would be documented as "does not work on no_std targets, always panics on some std targets". Would you be happy with such API?

[hanna-kruppe]

Libraries that don’t require std or want to work on targets with half-broken std are common, but it’s significantly less common for entire applications to be #[no_std] or use the exact same main function on e.g., Linux and wasm32-unknown-unknown. If the program entry point selects the entropy source and explicitly passes it through all libraries, a whole lot of programs written in Rust could still use the std API even if libraries have reasons to avoid it calling it directly. It’s different from the model offered by getrandom (globally accessible source of entropy even in no_std code) and more annoying to write code in this style, but there are also software engineering advantages to explicit dependency injection for such non-determinism. So while this may not be a drop in replacement for getrandom, the delta amounts to “provide a way for consumers to plug in their own global source on platforms where std doesn’t have one” — something std could provide as well in theory, but also doesn’t have to, especially since the best interface isn’t clear.

[briansmith]

I agree, but the unfortunate reality is that Rust does provide std for wasm32-unknown-unknown (I strongly believe it was a mistake, but alas) and will continue to do so because of the backward compatibility guarantees. Yes, we could provide an always panicking implementation of read_sys_entropy on this target,

I think that is the best thing to do. The real issue is that we haven't deprecated the wasm32-unknown-unknown target yet; it is inevitable, but not everybody sees it yet. wasm32-unknown-unknown stuff shouldn't block progress on this.

[Kixunil]

will continue to do so because of the backward compatibility guarantees

I don't see any reason to not deprecate it at least and provide more sensible alternative. std already has a bunch of stuff deprecated and replaced with something better. I don't know what the specific use case for wasm32-unknown-unknown is but it seems that any other more specific target is just better when it come to std.

"does not work on no_std targets, always panics on some std targets". Would you be happy with such API?

Being unavailable on no_std and not having a stable RNG trait sucks to be honest, speaking from experience. But that's the current state of things anyway. Some sort of global registry similar to global allocator would indeed help. Anyway, if the second part was "panics on deprecated std targets that go replaced with something sensible years ago" I wouldn't mind.

it’s significantly less common for entire applications to be #[no_std]

Well, minimizing WASM module size by getting rid of std is nice. But that potentially has other challenges.

[bstrie]

This issue is for secure random data generation: reading bytes directly from the system's CSPRNG. Everything else should happen in a separate issue.

Seconded. All I want is something like io::read_entropy as an alternative to the getrandom crate. Does the libs team need to accept an ACP first or should someone presumptuously make an issue so we can move discussion there?

[joshtriplett]

We discussed this at length in today's @rust-lang/libs-api meeting. We didn't come to any final conclusions, but here's a summary of the discussion:

We were generally amenable to the idea of exposing low-level randomness in the style of getrandom, if that's something people find useful.

We felt that that was an independent decision, separate from the higher-level interfaces in the style of rand, and we did not discuss treating this as an "instead" rather than an "in addition to". (Whatever interfaces we want for that in core will need more design.)

We discussed whether the low-level method being requested here is effectively the existing std::random::DefaultRandomSource::fill_bytes method.

In discussing the possible differences, we talked about whether RandomSource should be exposing the IO error or not. Originally when reviewing and accepting the ACP that led to this, when we talked about whether RandomSource should expose the IO error or not, we had leaned against, because that'd make implementations of Random harder to write. (if Random doesn't expose the error, because people aren't prepared to deal with that). We also read the comment at #130703 (comment) . On the other hand, we observed that for a low-level function that's meant to replace getrandom, it may make sense to expose any potential IO error. #[must_use] should ensure that people don't ignore the error.

We did agree that, if we want to expose the IO error, we should do so by having trait RandomSource: Read, and dropping the fill_bytes method. That would then mean people could use read, and for that matter read_buf to work with an uninitialized buffer.

Everyone present in the meeting was generally in favor of exposing the IO error, and doing so by having RandomSource: Read. That will complicate implementations of the current trait Random (making them unwrap), but that didn't seem like a problem. Some in the meeting felt that wasn't a problem because the error should be propagated all the way up (e.g. rolling a die or shuffling an array should return an error); others in the meeting felt it wasn't a problem because the interfaces for higher-level randomness (e.g. rolling a die or shuffling an array) need redesigning anyway and we shouldn't block this change on that.

Summary:

• Remove RandomSource::fill_bytes
• Make trait RandomSource: Read
• The low-level method people are looking for is provided by a call to read on DefaultRandomSource.
• Random and other higher-level bits still need further design. RandomSource and DefaultRandomSource could be stabilized independently.

[ericlagergren]

@joshtriplett Implementing Read provides some useful features. But what is the answer to "users cannot safely handle errors from the system CSPRNG"? There are several problems that arise:

1. Users fall back to an insecure source of randomness (the system time is a common one)
2. Users retry the operation, but the system CSPRNG is broken and the results are unpredictable
3. Users ignore the error (like you mentioned, this is generally solved by #[must_use])

Also: is RandomSource going to be documented as returning cryptographically secure bytes?

[newpavlov]

Make trait RandomSource: Read

This looks really bad for the potential future move of this trait to core. As a getrandom maintainer, I would strongly advice against tying it to anything std::io-related. Why can't std just add a blanket impl<T: RandomSource> io::Read for T { ... }, if we want some compatibility between the traits? As for uninit buffer support, we could just add a read_buf-like method directly to RandomSource in future, I don't see the need to reuse Read::read_buf here.

[hanna-kruppe]

Another (smaller) issue with reusing io::Read is that its set of methods is a poor fit for the "all or nothing" nature any RNG wants. Read::read permits short reads and ErrorKind::Interrupted. While the standard library could and should promise that the impl Read for DefaultRandomSource never does either (i.e., always reads as many bytes as the caller wanted or fails with a "real" error), it'll probably still set of my code review alarm bells to see code like:

let mut buf = [0; N];
random_source.read(&mut buf).expect("CSPRNG shouldn't fail");
something_important(&buf);

Any effort spent flagging this and either rewriting it with Read::read_exact or looking up again that it this specific Read impl doesn't have that problem would be wasted.

[abgros]

Chiming in to note that getrandom is allowed to succeed while only partially filling the buffer (see the man page):

On success, getrandom() returns the number of bytes that were
copied to the buffer buf.  This may be less than the number of
bytes requested via buflen if either GRND_RANDOM was specified in
flags and insufficient entropy was present in the random source or
the system call was interrupted by a signal.

I guess the question is whether the standard library should try to account for this, or just count it as a failure.

[newpavlov]

@abgros
While talking about getrandom most people in this thread mean the getrandom crate, not the getrandom syscall. The crate guarantees to either fully fill the buffer, or return an error. When the syscall returns partially filled buffer, it tries to fill the remainder by calling the syscall again.

I don't see a practical reason to expose a Read::read-like method in the proposed randomness trait.

[joshtriplett]

@joshtriplett Implementing Read provides some useful features. But what is the answer to "users cannot safely handle errors from the system CSPRNG"? There are several problems that arise:

1. Users fall back to an insecure source of randomness (the system time is a common one)

I certainly hope not. I would expect the handling of any such error to be a graceful exit, and the only reason to expose it rather than panicking is the ability to handle it gracefully.

2. Users retry the operation, but the system CSPRNG is broken and the results are unpredictable

I would not in general expect such errors to be recoverable.

Also: is RandomSource going to be documented as returning cryptographically secure bytes?

DefaultRandomSource should be, yes.

[joshtriplett]

Make trait RandomSource: Read

This looks really bad for the potential future move of this trait to core.

If we're going to expose errors from the OS, the obvious type for that would be io::Error. I would love to see that move to core, if not for the boxed variant it has. If we don't use io::Error, we'd have to invent our own error type, while ideally being able to store any error from the OS without loss of information, which effectively means we're re-inventing io::Error.

If we decide to panic, or use a different error type, then I agree that we can't use Read.

As for uninit buffer support, we could just add a read_buf-like method directly to RandomSource in future, I don't see the need to reuse Read::read_buf here.

We could absolutely do that, but the question is what we should do about error handling, if anything. If we use io::Error, then Read seems like the obvious choice. If we're avoiding io::Error, then I agree that we should provide a method directly, since it needs a different signature.

[joshtriplett]

Another (smaller) issue with reusing io::Read is that its set of methods is a poor fit for the "all or nothing" nature any RNG wants. Read::read permits short reads
...
Any effort spent flagging this and either rewriting it with Read::read_exact or looking up again that it this specific Read impl doesn't have that problem would be wasted.

That's a valid point.

[newpavlov]

If we're going to expose errors from the OS, the obvious type for that would be io::Error

The obvious approach is to use the Error associated type in the trait definition. Then SystemSource defined in std could use Error = io::Error while PRNG implementations in third-party crates could use Error = !. Though I would prefer for SystemSource to use a getrandom::Error-like type.

Somewhat tangential rant:

I believe that io::Error has a somewhat problematic design and should be avoided whenever possible.

It covers two very distinct cases: "error code from OS" and "any error under the sun abstracted by Box<dyn core::error::Error> and assembled into error stack". In my opinion, ideally, io::Error should've focused only on the first case, while the second cases should be handled by third-party crates like anyhow.

Yes, we need some limited extendability for things like Error::WRITE_ALL_EOF and potentially for custom user codes, but it's probably a solvable problem (e.g. by using "distributed slices"). Especially considering that targets usually have well defined ranges of possible system error codes, so we can carve out a range for custom error codes defined on the Rust side.

Making io::Error "thin wrapper" around RawOsError would've also made it trivial to move it to core, together with most of the io traits. But unfortunately we are stuck with the current design...

[joshtriplett]

The obvious approach is to use the Error associated type in the trait definition.

I don't think that's obvious, and it has disadvantages: code that's generic over RandomSource would then have to either box the error or thread that generic through its own error type. I would sooner suggest a std::random::Error type instead, if we don't want to use io::Error.

It covers two very distinct cases: "error code from OS" and "any error under the sun abstracted by Box<dyn core::error::Error> and assembled into error stack".

I agree with you completely, and that's a substantial problem. In the absence of that boxed variant, io::Error (and traits depending on it) would likely have moved into core years ago.

[newpavlov]

I don't think that's obvious, and it has disadvantages: code that's generic over RandomSource would then have to either box the error or thread that generic through its own error type.

Judging by the rand experience, it's not really a problem in practice. Most users rely on infallible PRNGs while OsRng is mostly used for seeding. In other words, almost all users will see only two options: Error = ! or Error = io::Error/random::Error. We also could introduce an UnwrapErr-like wrapper for users who don't want to bother with handling of extremely rare errors.

Cryptographic code is a bit different in this regard since it's more common to use OsRng and even specialized HW RNGs directly (e.g. for key generation) without relying on user-space PRNGs (they are also used, but for less "critical" material such as nonces). In RustCrypto we handle it by providing two sets of methods: fallible and infallible (e.g. see the KeyInit trait).

[jstarks]

In the absence of that boxed variant, io::Error (and traits depending on it) would likely have moved into core years ago.

Is that really true? I had assumed, perhaps incorrectly, that core was meant to contain only things that don't depend on the operating system, kind of the equivalent of what's required from a "freestanding" implementation in C, as opposed to a "hosted" one (which corresponds to what's in std). And then alloc is just "stuff that's not OS-specific but requires an allocator".

But io::Error is certainly OS specific: its interpretation of the OS error code depends on the environment it's built for. I don't see how it makes sense in core based how I understood its definition. And I have no idea how io::Error would behave for targets like x86_64-unknown-none.

Is there anything else in core that is OS specific in this way?

Related, when people talk about having DefaultRandomSource in core, is the idea that it would have the actual batteries-included, OS-specific implementation that we're currently talking about putting in std? Or is the request for some kind of #[global_default_random_source] attribute, or whatever, to allow the binary crate author to plug in what they want?

Because if it's the former then, again, I don't understand what core is supposed to be.

[ericlagergren]

@joshtriplett Implementing Read provides some useful features. But what is the answer to "users cannot safely handle errors from the system CSPRNG"? There are several problems that arise:

1. Users fall back to an insecure source of randomness (the system time is a common one)

I certainly hope not. I would expect the handling of any such error to be a graceful exit, and the only reason to expose it rather than panicking is the ability to handle it gracefully.

Unfortunately, users do fall back to insecure sources of randomness. :/

2. Users retry the operation, but the system CSPRNG is broken and the results are unpredictable

I would not in general expect such errors to be recoverable.

Also: is RandomSource going to be documented as returning cryptographically secure bytes?

DefaultRandomSource should be, yes.

If there is going to be a RandomSource trait and if it is not documented as being cryptographically secure then I'd like to push for a CryptoRng (or similar) marker trait.

My goal here is just to make sure these new APIs are misuse resistant.

[joshtriplett]

In the absence of that boxed variant, io::Error (and traits depending on it) would likely have moved into core years ago.

Is that really true? I had assumed, perhaps incorrectly, that core was meant to contain only things that don't depend on the operating system, kind of the equivalent of what's required from a "freestanding" implementation in C, as opposed to a "hosted" one (which corresponds to what's in std). And then alloc is just "stuff that's not OS-specific but requires an allocator".

But io::Error is certainly OS specific: its interpretation of the OS error code depends on the environment it's built for. I don't see how it makes sense in core based how I understood its definition. And I have no idea how io::Error would behave for targets like x86_64-unknown-none.

(Note: further discussion on this would be off topic for this tracking issue; for that, I'd suggest #t-libs on Zulip, or internals.rust-lang.org.)

core is still built for a particular target, and is specific to that target. It uses the target's ABI, the target's types and sizes (e.g. core::ffi), some target-specific startup machinery, and potentially in the future some SIMD machinery and thread-local storage machinery.

Teasing apart the parts of io::Error that make sense for core would be a complicated process. I would expect, for instance, that the version in core would include io::ErrorKind, but not the boxed variants or raw OS error codes.

Related, when people talk about having DefaultRandomSource in core,

I would not expect DefaultRandomSource to be in core; that belongs in std. I would expect the RandomSource trait to be in core, along with various APIs that use randomness (e.g. shuffling a slice, or generating a random number in a range).

[newpavlov]

@jstarks
(The io::Error discussion is off topic, so I will not reply to those parts, you can search for previous discussions to get more context)

Related, when people talk about having DefaultRandomSource in core, is the idea that it would have the actual batteries-included, OS-specific implementation that we're currently talking about putting in std? Or is the request for some kind of #[global_default_random_source] attribute, or whatever, to allow the binary crate author to plug in what they want?

The idea is to start with the former and maybe move to the latter in future (in a separate sysroot crate). IIUC the main blocker for the latter is lack of a unified way to handle #[global_allocator]-like items.

Having the randomness trait defined in core is desirable not only because of the hypothetical #[global_default_random_source] but also because it would provide a standard API for RNG implementations which would be used by both RNG implementers and users in the wider ecosystem.