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

rand_core::RngCore & CryptoRng support for CryptoProvider #1853

Closed
1 task done
pinkforest opened this issue Mar 18, 2024 · 7 comments
Closed
1 task done

rand_core::RngCore & CryptoRng support for CryptoProvider #1853

pinkforest opened this issue Mar 18, 2024 · 7 comments

Comments

@pinkforest
Copy link

pinkforest commented Mar 18, 2024

Checklist

  • I've searched the issue tracker for similar requests

Is your feature request related to a problem? Please describe.
Low level cryptography primitives, e.g. x25519-dalek::EphemeralSecret constructors expect to be provided a CryptoRng impl

This was made so no_std environments can provide their own chosen randomness, e.g. wasm32-unknown-unknown with a runtime that has a virtual machine that provides source of secure randomness that can be presented via CryptoRng trait.

Lower level crypto primitives have started pulling the rand_core trait so different random implementations don't need to be supported at lower level primitive.

Meanwhile SecureRandom is neither accessible within the interface to lower level primitives within CryptoProvider:

but this is not included in the interface with rustls: it is assumed that the cryptography library provides for this itself.

Lower level primitives don't generally offer a lot of choice when it comes to source of randomness leading to limited platform support where the primitive can be used unless it is provided the appropriate source of randomness.

Describe the solution you'd like
Ideally it would be great of rustls relays rand_core::CryptoRng that implements several methods over tls::SecureRandom for crypto primitive use that can be used to pass the primitives within ecosystem which expects this trait implementation.

Describe alternatives you've considered
I thought about using slow getrandom = ["custom"] - primitives I'm dealing with use rand_core::OsRng when constructor is called with rand_core/getrandom feature
Also thought about storing a state within the wrapper provider struct but then it leads to complications of navigating Arc<Mutex<>> landscape (which typically also requires std) which should be left to runtimes which may not implement threading etc. permutations involved.

Additional context

@ctz
Copy link
Member

ctz commented Mar 18, 2024

I avoided the rand_core crate because I find the whole rand ecosystem muddled and poorly designed; I would like to avoid any dependency on it in this crate. As an example, rand_core::RngCore abstracts over both fallible and infallible generators and consumers, so an fallible generator can be used with an infallible consumer (the docs suggest panicking or retrying endlessly; terrible options both), and an infallible generator can be used with a fallible consumer (unreachable and untestable error handling).

I chose not to pass the random provider down to lower level primitives because:

  • rustls::crypto::SecureRandom is the lowest-level primitive that rustls needs to implement TLS, not one that is intended to be useful elsewhere the ecosystem. Passing it down suggests it should be used in preference to something else, but I don't believe that is the case.
  • the set of operations that require randomness is actually quite implementation-dependent. For example, loading an RSA key requires (if implementing the standards like FIPS186) a random source, but other key types do not. As it happens, none the RSA interfaces I've seen in the ecosystem actually reflect that fact (they either use their own random source under the hood, or are non-standard with respect to FIPS186).

One option you have is having interior mutability inside whatever struct that impl rustls::crypto::SecureRandom, but I would suggest instead task-/thread-local storage if you need to store a non-trivial CSPRNG state.

@djc
Copy link
Member

djc commented Mar 18, 2024

I'm not sure I really understand what problem it is you're describing. None of the rustls CryptoProvider implementations rely on rustls' SecureRandom trait to do anything else -- they rely on the source of randomness provided by their backing crypto primitives. The SecureRandom interface is only used to interface between the crypto primitive implementations and other parts of rustls that use it directly to generate some randomness for the TLS handshake.

@pinkforest
Copy link
Author

pinkforest commented Mar 18, 2024

The SecureRandom interface is only used to interface between the crypto primitive implementation

Yes this is what we need for doing key exchange at CryptoProvider but it's not there sadly 😿

x25519 ECDHE key exchange requires randomness that is used by EphemeralSecret and we need to pass source of randomness to the x25519-dalek library from within the CryptoProvider which would not know where to pull that randomness e.g. in a wasm virtual machine - unless it is implemented in the primitive level itself which is not going to happen as this would require supporting all the different types of sources of randomness if every given possible platform.

Passing it down suggests it should be used in preference to something else, but I don't believe that is the case.

Could this be an optional feature ? for any primitive that supports optionally be given randomness from rustls that was passed to it by the provider believing the randomness source is the preferred one - given the provider also was responsible for selecting the used primitive in the first place ?

One option you have is having interior mutability inside whatever struct

These primitives are very low level and requiring interior mutability w/ Send + Sync as CryptoProvider is would require Mutex etc that are not available in no_std core due to this requiring the operating system - crypto primitives typically don't know much about operating systems at least in rustcrypto ecosystem.

We could implement the Mutex etc. at CryptoProvider but then again we'll bring std there when we are trying to aim the CryptoProvider for all environments which may not have operating system.

none the RSA interfaces I've seen in the ecosystem actually reflect that fact (they either use their own random source under the hood, or are non-standard with respect to FIPS186).

fips was still optional ? is the requirement for a provider to be fips compliant ? rsa crate has interface for CryptoRng.

Also if the interface uses to get the optional randomness then the provider could be marked as fips()==false marker

@newpavlov
Copy link

newpavlov commented Mar 18, 2024

Disclaimer: I am a member of the rust-random org.

@ctz

I avoided the rand_core crate because I find the whole rand ecosystem muddled and poorly designed

Please raise these issues in the rand repository. Luckily, the rand project is currently in the process of preparing next cycle of breaking releases, so it should be possible to address your concerns.

I can understand why you may feel this way about the whole rand ecosystem (admittedly, it's far from being simple), but, frankly, I find it strange that you say it about the very simple rand_core crate. Outside of the helper modules it essentially contains just 3 traits:

  • CryptoRng: marker trait to indicate cryptographically secure RNGs.
  • SeedableRng: trait for seedable PRNGs.
  • RngCore: basic RNG functionality.

In the next breaking release CryptoRng will be a super-trait of RngCore, which should simplify signatures, i.e. we will be able to write impl CryptoRng, while previously we had to use impl CryptoRng + RngCore.

Now, as for RngCore, it contains two methods relevant to cryptography: fill_bytes and try_fill_bytes. I don't think you can get simpler than those. Your SecureRandom::fill is equivalent to RngCore::try_fill_bytes. The only difference between those methods is that rand_core::Error keeps information about underlying OS error, while GetRandomFailed erases it.

Finally, SecureRandom is also bounded by Send + Sync, which I find... questionable? I am not familiar with rustls code base, but IIUC you assume that implementers of SecureRandom are usually just marker types which do not contain any state like the OsRng type, correct?

rand_core::RngCore abstracts over both fallible and infallible generators and consumers, so an fallible generator can be used with an infallible consumer

So you just want to distinguish between fallible and infallible RNGs at compile time? I think it can be easily amended by introducing a new marker trait for "infallible" RNGs.

But I am not sure how useful it would be in practice for rustls. Arguably, most users should rely on user-space RNGs like ThreadRng, which due to periodic reseeding in theory may fail, so you have to bubble potential errors.

@ctz
Copy link
Member

ctz commented Mar 18, 2024

fips was still optional ? is the requirement for a provider to be fips compliant ? rsa crate has interface for CryptoRng.

Just on this point: FIPS140 (validation and certification of cryptographic modules) is a different standard to FIPS186 (digital signatures). rustls doesn't, won't and can't require that a given RSA implementation adheres to any particular standard.

But my point wasn't to discuss issues with the rsa crate, but to illustrate that it is more flexible to require underlying libraries to depend on their RNG internally when and where needed.

@pinkforest
Copy link
Author

For somebody else finding this problem - hope this saves some research time in the future searching this issue:

There is getrandom::register_custom_getrandom macro that can be used to provide custom getrandom at bin-level

ring's less-safe-getrandom-drand with with target_os = "none" may provide some reference as well.

Supporting the trait would be nice but I'll check the testing around using the macro sans plain build-target only CI test.

Since the answer is no here I'll just close - thanks for your time in any case evaluating the proposal ❤️

@pinkforest pinkforest closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
@cpu
Copy link
Member

cpu commented Mar 18, 2024

Thanks for filing the issue and following up with the workaround you arrived at 👍

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

No branches or pull requests

5 participants