rust-random / rand

A Rust library for random number generation.
https://crates.io/crates/rand
Other
1.68k stars 430 forks source link

Replacing rand_chacha with chacha20 #1348

Closed nstilt1 closed 10 months ago

nstilt1 commented 1 year ago

I apologize if this pull request looks a little nasty. This is for https://github.com/rust-random/rand/issues/934

I was able to copy in the code from chacha20, as well as some code from 0.8.1 of chacha20 which included a formerly valid RNG implementation. I was able to adjust it so that it compiles and passes some tests. It should be able to process 4 blocks at a time on AVX2 and NEON. I will bench it tomorrow with soft, avx2, and neon.

The main thing that probably needs to change is that it currently takes a [u8; 12] for the stream_id, which used to use a u64 before it switched to 96 bits. We could use a u128 and discard the upper 32 bits, but I haven't done that yet. Also, I used a little bit of unsafe code for converting [u8] to [u32] and back. It isn't that unsafe, but as long as the input len is what it is supposed to be (a multiple of 4), then it should be okay.

/// A trait for altering the state of ChaChaCore<R>
trait AlteredState {
    /// Set the stream identifier
    fn set_stream(&mut self, stream: [u8; 12]);
    /// Get the stream identifier
    fn get_stream(&self) -> [u8; 12];
}

impl<R: Unsigned> AlteredState for ChaChaCore<R> {
    fn set_stream(&mut self, stream: [u8; 12]) {
        let (_prefix, result, _suffix) = unsafe {stream.align_to::<u32>()};
        for (val, chunk) in self.state[13..16].iter_mut().zip(result) {
            *val = *chunk;
        }
    }
    fn get_stream(&self) -> [u8; 12] {
        let (_prefix, result_slice, _suffix) = unsafe {self.state[13..16].align_to::<u8>()};
        let mut result = [0u8; 12];
        result.copy_from_slice(result_slice);
        result
    }
}

There might be a more concise way to do it though.

There's also a potential issue with copies being made in fn generate() in rng.rs. Maybe the buffer should be of type Self::Results. I honestly don't like the necessity of the GenericArray there.

newpavlov commented 1 year ago

I think it will be better to use chacha20 directly. This way any future improvement on the RustCrypto side will be automatically pulled by rand and it will reduce amount of code duplication across projects.

Previously, we had feature-gated implementation of the rand_core traits in the chacha20 crate. I think we can return them, so rand would be able to use chacha20 directly without going through rand_chacha.

nstilt1 commented 1 year ago

Will be rewriting this to import the RNGs from https://github.com/RustCrypto/stream-ciphers/pull/333

Regarding the re-exporting of rand_core, should this crate re-export it from chacha20 since the rand_core version in this library hasn't been released, ensuring that the same version is exported that is being used by the actual RNG, in case a trait were to get renamed between rand_core versions or something?

dhardy commented 10 months ago

@nstilt1 I'm not certain what your plan is here — to implement RngCore / BlockRng directly in chacha20 and then deprecate rand_chacha? That should be acceptable.

I'm also not sure what the status of that change is (haven't been following too closely), but you do have a lot of code in one PR...

Regarding the re-exporting of rand_core, should this crate re-export it from chacha20 since the rand_core version in this library hasn't been released

I find such re-exports a good policy. In this particular case it may not be all that useful given that rand_core is not a user-facing crate — one usually needs to import rand too anyway. So whichever.

nstilt1 commented 10 months ago

@nstilt1 I'm not certain what your plan is here — to implement RngCore / BlockRng directly in chacha20 and then deprecate rand_chacha? That should be acceptable.

That is the current plan.

I'm also not sure what the status of that change is (haven't been following too closely), but you do have a lot of code in one PR...

I believe it is getting closer to being done. The main things remaining are zeroizing the SIMD backends, and 2 additional API methods, get_block_pos() and set_block_pos(). Those API methods seem to belong on the Rng Core, but if they were to go on the core, then the index would not be able to be reset, resulting in the following values to be different:

rng.get_core_mut().set_block_pos(5);
let a = rng.next_u32();
rng.get_core_mut().set_block_pos(5);
let b = rng.next_u32();

If it is preferred for that method to behave as expected, then those methods might need to be implemented for the Rng instead of the core.

dhardy commented 10 months ago

Thanks for the clarification.

If it is preferred for that method to behave as expected, then those methods might need to be implemented for the Rng instead of the core.

Agreed. These methods are in any case not part of a standard trait like SeedableRng so can be specific to the RNG (like the current set_word_pos). (I suppose we could introduce a new trait for this, but I've yet to see a request for that.)