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

Get rid of the getrandom and OsRng requirement #86

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

stevefan1999-personal
Copy link
Contributor

@stevefan1999-personal stevefan1999-personal commented Sep 25, 2024

This PR solves the getrandom dependency problem, due to OsRng being added to the library code itself. Since OsRng assumed to be running under an operating system, this makes it impossible to run in freestanding environment such as bare-metal kernel and isolated raw WebAssembly.

It has now been removed and the dependency on RNG source in the provider itself has been inversed, meaning that the user has to supply a RNG provider from the start in the initial provider initialization call, and during execution, we take get take back the RNG implementation in rustls global code.

Assuming the code that needs randomness in this library would only be called by the library itself (e.g. RustCrypto Provider -> ECDHE key gen), we can safely assume there has to be a crypto provider and it has to be our own library.

no_std users can still enable getrandom but they have to make the implementation manually. This can simply be done by adding the getrandom crate in their end, and then set the default RNG using OsRng themselves, rather than pushing the responsibility to us to always assume OsRng to be available.

Now, it is really debatable whether CryptoRng is also needed or not for RNG, as it is intended for embedded system where getrandom is unlikely to be ported to the platform, so the user might just want to use a good initial entropy (like from CPU jitter or other micro-movements) then shove it to a PRNG that is fast and generates random numbers for the rest of the lifetime. This is not cryptographically safe at all, but so far this is a necessary evil if you want it to run in no_std environment where the existence of a good CSPRNG implementation is not guaranteed.

Thus, impl CryptoRng for CryptoProviderRng can be considered a phony implementation out of necessity for the code to compile. It is now user-controlled and does not guarantee the output to be of cryptographic quality.

@newpavlov
Copy link
Member

I don't think I like this approach. It feels really hacky to me.

What exact problem are you trying to solve? Building/testing on no_std targets?

@stevefan1999-personal
Copy link
Contributor Author

@newpavlov Agreed. I rethought about it and changed it like that without using OnceCell

@newpavlov
Copy link
Member

newpavlov commented Sep 25, 2024

I had a more fundamental question: why exactly do you want to make dependency on getrandom optional?

If you want a way to change entropy source (e.g. for deterministic testing), then I think its better to introduce ProviderWithRng generic over CryptoRng + Default and define alias type Provider = ProviderWithRng<OsRng>; gated on the getrandom feature. With this approach you can define a testing CSPRNG seeded with a fixed seed and state stored in a static.

@stevefan1999-personal
Copy link
Contributor Author

stevefan1999-personal commented Sep 25, 2024

I had a more fundamental question: why exactly do you want to make dependency on getrandom optional?

If you want a way to change entropy source (e.g. for deterministic testing), then I think its better to introduce ProviderWithRng generic over CryptoRng + Default and define alias type Provider = ProviderWithRng<OsRng>; gated on the getrandom feature. With this approach you can define a testing CSPRNG seeded with a fixed seed and state stored in a static.

For one, it broke no_std as the automatic target selection of getrandom looks very arcane (https://docs.rs/getrandom/latest/getrandom/), that means whenever you have a new triplet you are not likely to port getrandom on day one, and second it assumes the existence of an operating system, at least within the definition of rand_core, which is not always the case (as I raised the example of using it in MCUs).

But I do think your approach would also be alright, let me try that out as well.

@newpavlov
Copy link
Member

it broke no_std as the automatic target selection of getrandom looks very arcane

Do you perhaps mean the custom backend? For most targets supported out-of-box automatic selection is a pretty straightforward cfg_if!.

As for the custom backend, I agree that it's not the most straightforward thing, but, unfortunately, Rust does not provide good tools for this problem. We tried to describe how to enable custom backends in the register_custom_getrandom! docs. I don't think it's the most complex thing required for configuring a no_std target. Note that there is also an additional escape hatch in the form of patching the getrandom dependency by a custom crate.

that means whenever you have a new triplet you are not likely to port getrandom on day one

It's no surprise that you need additional configuration on no_std targets. I think it's much better to do it in one place than have several crates to invent their own ad hoc way of selecting user-defined entropy sources.

second it assumes the existence of an operating system

No, it does not. OsRng is a somewhat misleading name, but it works just fine on no_std targets assuming getrandom was configured properly.

@stevefan1999-personal
Copy link
Contributor Author

it broke no_std as the automatic target selection of getrandom looks very arcane

Do you perhaps mean the custom backend? For most targets supported out-of-box automatic selection is a pretty straightforward cfg_if!.

As for the custom backend, I agree that it's not the most straightforward thing, but, unfortunately, Rust does not provide good tools for this problem. We tried to describe how to enable custom backends in the register_custom_getrandom! docs. I don't think it's the most complex thing required for configuring a no_std target. Note that there is also an additional escape hatch in the form of patching the getrandom dependency by a custom crate.

that means whenever you have a new triplet you are not likely to port getrandom on day one

It's no surprise that you need additional configuration on no_std targets. I think it's much better to do it in one place than have several crates to invent their own ad hoc way of selecting user-defined entropy sources.

second it assumes the existence of an operating system

No, it does not. OsRng is a somewhat misleading name, but it works just fine on no_std targets assuming getrandom was configured properly.

I would still argue that we should unify the RNG to use Rustls provider supplied one, rather than assuming getrandom is always available.

I'm talking about things like this:

let priv_key = x25519_dalek::EphemeralSecret::random_from_rng(rand_core::OsRng);

@newpavlov
Copy link
Member

newpavlov commented Sep 27, 2024

rather than assuming getrandom is always available.

getrandom's raison d'etre is to provide "system randomness" source. If we need "system randomness", then it's better to simply assume that getrandom exists and make users to configure it appropriately for targets which do not enjoy out-of-box support.

If you have ideas how we could make it easier to configure getrandom, we are open to suggestions. It's a good time to introduce such changes since we are in the process of preparing the next breaking release.

x25519_dalek::EphemeralSecret::random_from_rng(rand_core::OsRng);

I don't see any major issues with this code. It may make more sense to use ThreadRng or something similar instead, but otherwise it looks fine to me.

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