From 6ecd1a3f3c669df48946fd08f5cbc9131c952692 Mon Sep 17 00:00:00 2001 From: Chris Beck Date: Tue, 13 Sep 2022 13:46:17 -0600 Subject: [PATCH] delete some terrible code that was written to work around cargo bugs (#2503) * delete some terrible code that was written to work around cargo bugs many years ago, we encountered this bug: https://github.com/rust-lang/cargo/issues/6571 This bug meant that if we wanted to have a single Rng type that worked on all platforms, including SGX, then it could not use the standard library on any platform, because cargo would evaluate dependencies without respect to what platform you are on. However, it was really important for our code that we had such an abstraction layer. This was important for writing enclave impl code that could run in SGX and also in unit tests for instance. The way we solved this issue was that, we took the current version of `ThreadRng` which is the generically recommendable cryptographic RNG type from `rand` crate, and made the smallest possible changes to it until it would build without the standard library. The main change was replacing `std::thread_local!` with the `#[thread_local]` attribute, which turned out to be straightforward. However, it creates an annoying maintenance burden on us, and there has been a lot of churn in the rand crate since then. Since the `resolver = 2` stuff was stabilized, the original problem is no longer the case. It's fine to have crates that pull in `std` in one platform but not another. So we can now just use `ThreadRng` as the no-RDRAND fallback, like we wanted all along. * fixup from review comments * remove unnecessary dependencies --- Cargo.lock | 2 - consensus/enclave/trusted/Cargo.lock | 2 - crypto/rand/Cargo.toml | 15 ++--- crypto/rand/README.md | 16 +++-- crypto/rand/src/fallback.rs | 96 +-------------------------- crypto/rand/src/lib.rs | 4 +- fog/ingest/enclave/trusted/Cargo.lock | 2 - fog/ledger/enclave/trusted/Cargo.lock | 2 - fog/view/enclave/trusted/Cargo.lock | 2 - 9 files changed, 19 insertions(+), 122 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 52197ac571..a576840605 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3099,10 +3099,8 @@ name = "mc-crypto-rand" version = "2.0.0" dependencies = [ "cfg-if 1.0.0", - "getrandom 0.2.7", "rand 0.8.5", "rand_core 0.6.3", - "rand_hc 0.3.1", ] [[package]] diff --git a/consensus/enclave/trusted/Cargo.lock b/consensus/enclave/trusted/Cargo.lock index 2a7ff1fe0c..47c9874803 100644 --- a/consensus/enclave/trusted/Cargo.lock +++ b/consensus/enclave/trusted/Cargo.lock @@ -1134,10 +1134,8 @@ name = "mc-crypto-rand" version = "2.0.0" dependencies = [ "cfg-if 1.0.0", - "getrandom", "rand", "rand_core", - "rand_hc", ] [[package]] diff --git a/crypto/rand/Cargo.toml b/crypto/rand/Cargo.toml index 6791152b9b..f7a0bf73a8 100644 --- a/crypto/rand/Cargo.toml +++ b/crypto/rand/Cargo.toml @@ -4,16 +4,17 @@ version = "2.0.0" authors = ["MobileCoin"] edition = "2021" description = ''' -This crate provides a no-std compatible rng called `RdRandRng`. +This crate provides a no-std compatible rng called `McRng`. On targets with +rdrand target feature, it uses the intel RDRAND instruction to get randomness directly from the CPU, bypassing a dependency on the OS, libc, -etc. For servers using SGX, this rng works in trusted and untrusted code equally -well without changes, which is convenient. +etc. For servers using SGX, this rng works in no_std trusted and untrusted code +equally well without changes, which is convenient. -For targets without rdrand, we provide a fallback implementation similar to the -standard rust `rand` crate's `thread_rng`, using the `getrandom` crate OsRng to -seed a thread-local rng. `RdRandRng` is in all configurations a zero-width type. +For targets without rdrand, `McRng` is `ThreadRng`. +On wasm, `ThreadRng` is not available, so `McRng` is `OsRng`. + +`McRng` is in all configurations a zero-width type. ''' [features] @@ -22,7 +23,5 @@ std = ["rand_core/std", "rand/std", "rand/std_rng"] [dependencies] cfg-if = "1.0" -getrandom = { version = "0.2", default_features = false } rand = { version = "0.8", default-features = false } rand_core = { version = "0.6", default-features = false } -rand_hc = "0.3" diff --git a/crypto/rand/README.md b/crypto/rand/README.md index de35656533..46d62b19d5 100644 --- a/crypto/rand/README.md +++ b/crypto/rand/README.md @@ -1,28 +1,30 @@ mc-crypto-rand ====== -`mcrand` crate provides an rng type which is: +`mc-crypto-rand` crate provides an rng type which is: (1) no_std compatible (2) uses RDRAND when available, in and out of the enclave on sgx servers, (3) uses something like rand::ThreadRng on any other platforms `mc-crypto-rand` does not require any cargo feature configuration to be used correctly, -which simplifies the build +which simplifies the build. When `+rdrand` rustc target feature is enabled, we use x86 intrinsics directly to do rdrand correctly, and do not pull in getrandom or any other library. -when this target feature is not enabled, we do something that is almost the same as -`rand::ThreadRng`, but we use the nightly `#[thread_local]` api instead of `std::thread_local!` -macro. In fact we rely on rand for the implementation details, but we turn of `std` feature of rand. +When this target feature is not enabled, we use `rand::ThreadRng`, except on WASM where +we use `rand::OsRng`. + +Note that these fallbacks require the standard library. We don't currently +have any targets that don't have RDRAND and also can't use the standard library. Example usage: ``` -use mc_crypto_rand::{RdRandRng, RngCore} +use mc_crypto_rand::{McRng, RngCore} pub fn my_func() -> (u64, u64) { - let mut rng = RdRandRng{}; + let mut rng = McRng{}; let k0 = rng.next_u64(); let k1 = rng.next_u64(); (k0, k1) diff --git a/crypto/rand/src/fallback.rs b/crypto/rand/src/fallback.rs index f3143e6be2..a848ba21f6 100644 --- a/crypto/rand/src/fallback.rs +++ b/crypto/rand/src/fallback.rs @@ -1,97 +1,5 @@ // Copyright (c) 2018-2022 The MobileCoin Foundation -use core::option::Option; -use rand::rngs::adapter::ReseedingRng; -use rand_core::{impls, CryptoRng, Error, RngCore, SeedableRng}; +use rand::rngs::ThreadRng; -// Using Hc128Rng because it was (at one point) the StdRng of rand crate, up to -// version 0.6 We don't want to depend on rand because it depends on std, and -// specifically, its implementation of ThreadRng depends on rust stdlib -// thread_local! macro -use rand_hc::Hc128Core as Core; - -// Number of generated bytes after which to reseed `ThreadRng`. -// This constant is taken from rand and is not public. -const THREAD_RNG_RESEED_THRESHOLD: u64 = 1024 * 64; - -type InnerRng = ReseedingRng; - -// Compare this impl with rand::thread_rng -#[thread_local] -static mut THREAD_LOCAL_RNG: Option = None; - -// Get the thread local instance, initializing it if that hasn't happened yet in -// this thread -fn get_thread_local_rng() -> &'static mut InnerRng { - // unsafe is required because we are accessing a static mut, - // but it is thread_local so this is not actually a problem. - unsafe { - THREAD_LOCAL_RNG.get_or_insert_with(|| { - let r = Core::from_rng(OsRng) - .unwrap_or_else(|err| panic!("could not initialize thread_rng: {}", err)); - ReseedingRng::new(r, THREAD_RNG_RESEED_THRESHOLD, OsRng) - }) - } -} - -// The ZWT we give to users -// Similar to rand::ThreadRng -#[derive(Default)] -pub struct McRng; - -// Forward implementation from InnerRng -impl RngCore for McRng { - #[inline] - fn next_u32(&mut self) -> u32 { - get_thread_local_rng().next_u32() - } - fn next_u64(&mut self) -> u64 { - get_thread_local_rng().next_u64() - } - fn fill_bytes(&mut self, dest: &mut [u8]) { - get_thread_local_rng().fill_bytes(dest) - } - fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> { - get_thread_local_rng().try_fill_bytes(dest) - } -} - -impl CryptoRng for McRng {} - -/// OsRng -/// -/// We have to copy OsRng from rand_core unfortunately, because cargo: -/// To get rand_core::OsRng, we must turn on `rand_core/getrandom` cargo -/// feature, but this appears to turns on `libc/std`. -/// There is therefore no way to get rand_core::OsRng without turning on the -/// standard library. Fortunately this body of code is trivial -/// -/// This may just be a versioning issue? it may be that if rand_core upgrades -/// the version of getrandom that they rely on, we will be able to use -/// rand_core/getrandom without getting std, and then wouldn't have to carry -/// this. -#[derive(Clone, Copy, Debug, Default)] -struct OsRng; - -impl CryptoRng for OsRng {} - -impl RngCore for OsRng { - fn next_u32(&mut self) -> u32 { - impls::next_u32_via_fill(self) - } - - fn next_u64(&mut self) -> u64 { - impls::next_u64_via_fill(self) - } - - fn fill_bytes(&mut self, dest: &mut [u8]) { - if let Err(e) = self.try_fill_bytes(dest) { - panic!("Error: {}", e); - } - } - - fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> { - getrandom::getrandom(dest).map_err(|e| e.code())?; - Ok(()) - } -} +pub type McRng = ThreadRng; diff --git a/crypto/rand/src/lib.rs b/crypto/rand/src/lib.rs index 294c43d9ef..08bfd3a75a 100644 --- a/crypto/rand/src/lib.rs +++ b/crypto/rand/src/lib.rs @@ -1,8 +1,6 @@ // Copyright (c) 2018-2022 The MobileCoin Foundation #![no_std] -// The fallback code needs the unstable [thread_local] attribute -#![cfg_attr(not(target_feature = "rdrand"), feature(thread_local))] pub extern crate rand_core; @@ -10,7 +8,7 @@ pub use rand_core::{CryptoRng, RngCore}; use cfg_if::cfg_if; -// Not using cfg_attr( ..., path = fallback.rs) because it appears to confused +// Not using cfg_attr( ..., path = fallback.rs) because it appears to confuse // rustfmt cfg_if! { if #[cfg(target_feature = "rdrand")] { diff --git a/fog/ingest/enclave/trusted/Cargo.lock b/fog/ingest/enclave/trusted/Cargo.lock index 89ee676c81..7932d8c5c5 100644 --- a/fog/ingest/enclave/trusted/Cargo.lock +++ b/fog/ingest/enclave/trusted/Cargo.lock @@ -1048,10 +1048,8 @@ name = "mc-crypto-rand" version = "2.0.0" dependencies = [ "cfg-if 1.0.0", - "getrandom", "rand", "rand_core", - "rand_hc", ] [[package]] diff --git a/fog/ledger/enclave/trusted/Cargo.lock b/fog/ledger/enclave/trusted/Cargo.lock index 9320889900..da78fe33ff 100644 --- a/fog/ledger/enclave/trusted/Cargo.lock +++ b/fog/ledger/enclave/trusted/Cargo.lock @@ -1017,10 +1017,8 @@ name = "mc-crypto-rand" version = "2.0.0" dependencies = [ "cfg-if", - "getrandom", "rand", "rand_core", - "rand_hc", ] [[package]] diff --git a/fog/view/enclave/trusted/Cargo.lock b/fog/view/enclave/trusted/Cargo.lock index a21dce40bf..967f941d3d 100644 --- a/fog/view/enclave/trusted/Cargo.lock +++ b/fog/view/enclave/trusted/Cargo.lock @@ -1058,10 +1058,8 @@ name = "mc-crypto-rand" version = "2.0.0" dependencies = [ "cfg-if 1.0.0", - "getrandom", "rand", "rand_core", - "rand_hc", ] [[package]]