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

Replacing rand_chacha with chacha20 #1348

Closed
wants to merge 0 commits into from
Closed

Conversation

nstilt1
Copy link

@nstilt1 nstilt1 commented Oct 25, 2023

I apologize if this pull request looks a little nasty. This is for #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
Copy link
Member

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.

@dhardy dhardy mentioned this pull request Oct 31, 2023
23 tasks
@nstilt1
Copy link
Author

nstilt1 commented Nov 11, 2023

Will be rewriting this to import the RNGs from
RustCrypto/stream-ciphers#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
Copy link
Member

dhardy commented Jan 18, 2024

@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
Copy link
Author

nstilt1 commented Jan 21, 2024

@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
Copy link
Member

dhardy commented Jan 22, 2024

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.)

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.

3 participants