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

Rework CryptoRng #1273

Merged
merged 1 commit into from
Feb 20, 2023
Merged

Rework CryptoRng #1273

merged 1 commit into from
Feb 20, 2023

Conversation

newpavlov
Copy link
Member

@newpavlov newpavlov commented Dec 6, 2022

This PR introduces the CryptoBlockRng marker trait. It's used instead of CryptoRng on block RNGs, which allows us to mark CryptoRng as a subtrait of RngCore and makes the CryptoRngCore trait redundant.

Additionally, try_fill_bytes is moved to CryptoRng and renamed to crypto_fill_bytes. The rationale here is that error checks for potential RNG failures are practically exclusive to cryptographic code.

Unfortunately, this PR also contains a bunch of formatting changes introduced by cargo fmt. I think it could be worth to include formatting check into our CI to prevent such changes in future.

cc @tarcieri

@newpavlov newpavlov requested a review from dhardy December 6, 2022 08:08
@newpavlov newpavlov added the B-API Breakage: API label Dec 6, 2022
@dhardy
Copy link
Member

dhardy commented Dec 6, 2022

Unfortunately, this PR also contains a bunch of formatting changes introduced by cargo fmt. I think it could be worth to include formatting check into our CI to prevent such changes in future.

Er, yes. Please either remove formatting changes to old code or run rustfmt first, commit, and rebase on top of that. I.e. isolate your changes from formatting.

We should use rustfmt everywhere, but we don't. Last time I looked at this, it was rejected on grounds of too much poor formatting, and that in theory rustfmt 2.0 was coming at some point. I haven't followed everything, but I don't think 2.0 is happening any time soon, meanwhile almost every major Rust project now uses it so we should too. The main issue is that we have a lot of open PRs which would conflict.

Is it a major pain to remove formatting-only changes from this PR in the mean-time? (I usually use git checkout -p --.)


As for the actual purpose of this PR — these issues would appear related. Didn't we already discuss this?

@newpavlov
Copy link
Member Author

Please either remove formatting changes to old code or run rustfmt first, commit, and rebase on top of that. I.e. isolate your changes from formatting.

Yeah, I will do it somewhat later. I executed cargo fmt automatically and only during commit creation found the size of formatting changes. :( Before that, I hope to get initial reaction regarding the drafted approach, whether its worthwhile or not.

As for the actual purpose of this PR — these issues would appear related. Didn't we already discuss this?

I think I floated the drafted idea somewhere in issues. CryptoRngCore was designed with backward compatibility in mind, while changes in this PR are breaking ones.

@dhardy
Copy link
Member

dhardy commented Dec 7, 2022

Summary of this PR:

pub trait RngCore {
  // keeps methods next_u32, next_u64, fill_bytes
  // loses try_fill_bytes
}

// New bound on RngCore:
pub trait CryptoRng: RngCore {
  fn crypto_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> { /* default impl */ }
}

// New:
pub trait CryptoBlockRng: BlockRngCore {}

// Removed:
// pub trait CryptoRngCore: CryptoRng + RngCore { .. }

My thoughts, somewhat detached from the above...

In the long term, do we even keep BlockRng? Given the suggestions in #1261, especially yours regarding generic_const_exprs. Well, probably yes to provide a "next int please" API over a block generator.

Given that, maybe we should make larger changes:

// A revised BlockRngCore (maybe renamed or maybe not):
pub trait ByteRng {
    const LEN: usize;
    fn generate(&mut self) -> Result<[u8; Self::LEN]>;
}

// A cut-down RngCore (maybe we should keep the old name):
pub trait NumRng {
    fn next_u32(&mut self) -> u32;
    fn next_u64(&mut self) -> u64;
}

pub struct BlockRng<R: ByteRng>(..) ;
impl<R: ..> NumRng for ByteRng<R> {}

Except, it would be nice to know at compile time the generation size of NumRng, hence we could add const NATIVE_BITS: u32 or even use a type parameter NumRng<N>. Except this doesn't work for users needing an object-safe trait which means we would need a separate object-safe trait and blanket impls. Which (surprise!) means we need Rust's number one missing feature, Specialization (or maybe negative trait bounds).

Maybe we should hold off on such a re-design until rand_core 2.0 considering neither required feature is likely to make it into Rust in the near future? Or we could use a cut-down version of the above (without LEN) now, perhaps.

Either way, this would mean:

  • a ByteRng does not implement NumRng / Rng directly; so type StdRng = BlockRng<ChaCha12>;
  • NumRng does not provide for byte-filling; users must use Rng::fill
  • ... except that would be very inefficient for block-RNGs without using Specialization to impl Rng::fill, so maybe we need to keep RngCore::fill_bytes as is
  • ChaChaXRng (the RngCore implementor, not the core) has a few methods directly implemented on it so maybe we shouldn't only export the BlockRngCore implementor from the crate

But this is mostly orthogonal to your changes. So:

  • I like pub trait CryptoRng: RngCore
  • I don't really like the name crypto_fill_bytes, and do we even need it — can those who want it directly use the BlockRngCore? Maybe that doesn't work.

@newpavlov
Copy link
Member Author

See #1261 (comment) for reply to the middle part of the previous comment.

I don't really like the name crypto_fill_bytes, and do we even need it — can those who want it directly use the BlockRngCore?

I don't think the name is good either, so I am open to changing it. BlockRngCore is irrelevant here, the method is about making error handling possible for OsRng and hardware RNGs, which are usually used in cryptographic code.

But I am not sure whether it's a good idea to move try_fill_bytes to CryptoRng. While it simplifies some things, it introduces issues around error handling, e.g. in the io::Read compatibility layer. IIUC try_fill_bytes should be accessible for &mut dyn CryptoRng. So I probably will revert this change.

@dhardy
Copy link
Member

dhardy commented Dec 8, 2022

which are usually used in cryptographic code

Do these uses even want a RngCore or just a byte-generator? Of course it is useful having the buffering logic that BlockRng provides, but possibly a simpler version would suffice without the next_u* methods.

whether it's a good idea to move try_fill_bytes ... e.g. in the io::Read compatibility.

The ReadRng adapter is deprecated, so I see no worry there. However I don't see any reason for this change either.

I don't think the name is good either

Nor is the original, but it seems we don't have a good reason to change it.

If we want to simplify, we could probably remove RngCore::fill_bytes... at risk of doing more permutation than simplification (i.e. probably a needless breaking change).

@newpavlov
Copy link
Member Author

newpavlov commented Jan 6, 2023

@dhardy
Can you remind me why rand_chacha and ReseedingRng use newtype wrappers instead of type aliases? Is it only because rustdoc does not show trait impls for type aliases?

If such type aliases is the "recommended way" of doing things, then removal of the BlockRngCore trait could be warranted despite some amount of buffering boilerplate in implementation crates. But, personally, I would prefer if we simply used type aliases.

@dhardy
Copy link
Member

dhardy commented Jan 7, 2023

I think the new-type wrappers are used (a) to hide the inner type (thus not an API breaking change to replace it) and (b) to allow custom methods on that type. Without re-examining I don't know how important these are (probably only (b) is applicable to ChaCha).

I don't follow why this justifies removing BlockRngCore; that's an impl target used by BlockRng.

Thanks for cleaning up the PR.

@newpavlov
Copy link
Member Author

I don't follow why this justifies removing BlockRngCore; that's an impl target used by BlockRng.

It does not look like we have users of BlockRngCore outside of implementation crates. Since they introduce their own opaque newtypes, we can replace BlockRngCore and the wrapper types with a bunch of helper functions. I think it will result in a bit simpler rand_core and implementation crates.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we'd named RngCore as Rng instead, and what is now Rng as RngExt. Is it worth renaming now? It would make the names noted below more consistent, while also being better for common usage (where people use bounds like R: RngCore).

Besides this is the idea that maybe we should move one or some RngCore methods to a different trait. Most users don't need try_fill_bytes and next_u32. You mentioned moving try_fill_bytes to CryptoRng and we seem to have rejected that idea. If we wanted a separate ByteRng for try_fill_bytes, we'd then need pub trait CryptoByteRng: ByteRng {} too.

Anyway, I'll provisionally approve this PR. If I don't, it might just get stuck.

/// supposed to be cryptographically secure.
///
/// See [`CryptoRng`][crate::CryptoRng] docs for more information.
pub trait CryptoBlockRng: BlockRngCore { }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming seems off: CryptoRng: RngCore, CryptoBlockRng: BlockRngCore.

@tarcieri
Copy link

tarcieri commented Jan 8, 2023

I wish we'd named RngCore as Rng instead, and what is now Rng as RngExt. Is it worth renaming now?

Sounds great to me!

@dhardy
Copy link
Member

dhardy commented Feb 3, 2023

@newpavlov this is still marked as a draft, but I think it's ready to be merged? I already approved.

I'm also happy to rename RngCoreRng, RngRngExt. But that should be a new PR.

@dhardy dhardy marked this pull request as ready for review February 20, 2023 10:27
@dhardy dhardy merged commit 7c1e649 into master Feb 20, 2023
@dhardy dhardy deleted the crypto_rework branch February 20, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-API Breakage: API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants