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

Remove rand from public api. #167

Closed
wants to merge 1 commit into from

Conversation

dvc94ch
Copy link

@dvc94ch dvc94ch commented Apr 12, 2021

I expect for most users this is fine, and if some user would like to use a different Rng they can use the from_bytes call.

Since rand has a history of causing breakage in ed25519-dalek I think this change could be justified as a bug fix.

Also updates the dependencies to new versions where appropriate.

Closes #160
Closes #159
Closes #162

src/keypair.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Contributor

...if some user would like to use a different Rng they can use the from_bytes call.

This has a corollary: if you'd like to avoid rand-related breakages, use from_bytes

@dvc94ch
Copy link
Author

dvc94ch commented Apr 12, 2021

Well, the downside is that many users would have to wrap all of the ed25519-dalek api to get a simple generate function. It also precludes me from being able to use ed25519-dalek in my public apis. Also I'd assume it is more advanced users that want to customize the rng, since zeroizing the original bytes can be a bit tricky. Every time the bytes are moved on the stack they need to be zeroized. But this is probably a problem even in ed25519-dalek (when you pass a keypair by value to a function) I guess?

@dvc94ch
Copy link
Author

dvc94ch commented Apr 12, 2021

Would adding a generate_with_rng method work in addition to a generate that just works? Although that is probably hard to sell as a bug fix :)

@tarcieri
Copy link
Contributor

I'd assume it is more advanced users that want to customize the rng

It's needed by any user who is using a platform which isn't supported by OsRng:

https://docs.rs/getrandom/0.2.2/getrandom/#supported-targets

There is a new register_custom_getrandom function in getrandom, but it is by design stateless and thus somewhat unhelpful for cases where a stateful, seeded RNG (or RNG peripheral) is needed.

@dvc94ch
Copy link
Author

dvc94ch commented Apr 13, 2021

Interesting, so maybe generate_with_rng should take a FnMut(&mut [u8]) -> Result<(), E> and generate would call generate_with_rng(|dest: &mut [u8]| getrandom(dest)). For key generation that's the paranoid thing to do anyway as opposed to seeding a csprng right?

@dvc94ch dvc94ch force-pushed the fix-rand-breakage branch from 7ded890 to 3036007 Compare April 13, 2021 17:28
@dvc94ch
Copy link
Author

dvc94ch commented Apr 13, 2021

So tried the FnMut closure approach. One added benefit is that we don't have to depend on rand for default builds at all anymore.

@dvc94ch dvc94ch force-pushed the fix-rand-breakage branch from 3036007 to 7744afb Compare April 13, 2021 17:37
@dvc94ch
Copy link
Author

dvc94ch commented Apr 15, 2021

Are there any objections? Other than the marker trait for csprng I don't think we lose anything with this approach.

@tarcieri
Copy link
Contributor

This is definitely a breaking change and as such should be part of a major release.

It would also be good to look into the roadmap for rand/rand_core and whether they plan on putting out more breaking releases or if rand v0.8 / rand_core v0.6 are to be the last before rand_core is incorporated into core.

If the API is almost stable, I'm not sure it's worth dropping support now.

@dvc94ch
Copy link
Author

dvc94ch commented Apr 15, 2021

I didn't find a roadmap, could you point me to it?

If the API is almost stable, I'm not sure it's worth dropping support now.

Well, it's not dropping support, they're still interoperable. |bytes] OsRng {}.fill_bytes(bytes) can hardly be considered "dropping support". It's just not clear what the advantage is of pulling in rand for OsRng which just calls out to getrandom and unwraps the result. In any case having a generate that doesn't require passing in a rng would be great.

@tarcieri
Copy link
Contributor

tarcieri commented Apr 15, 2021

It's just not clear what the advantage is of pulling in rand for OsRng which just calls out to getrandom and unwraps the result.

The use cases where CryptoRng + RngCore are interesting are ones where OsRng isn't being used, such as embedded devices supplying their own RNG, or using something like rand::thread_rng().

I'm not sure the bounds you've supplied for the F generic parameter for generate_with_rng cover both of these cases and they can have conflicting requirements. For example, some RNGs may be Send + Sync, but an embedded RNG might explicitly be !Send. These issues don't come up in a purely trait-based API since there is no closure involved.

Especially in embedded use cases, the RNGs are owned and stateful, and don't fit into a getrandom-style API.

@dvc94ch
Copy link
Author

dvc94ch commented Apr 15, 2021

it's a FnMut so you can have arbitrary mutable state in it, and &mut FnMut also implements FnMut. And if you have something like this:

static mut RNG: HardwareRng = HardwareRng {};

you should be able to call generate_with_rng(|bytes| RNG.fill_bytes(bytes))

I think the Send/Sync concern doesn't apply as there is no bound for Send or Sync, meaning you can supply any FnMut not just a Send or a Sync one. I think Sized is applied automatically to generic arguments.

@tarcieri
Copy link
Contributor

You can't just stick hardware RNGs (or SeedableRngs seeded from them) in statics without resorting to convoluted mechanisms, and generally in the Rust Embedded ecosystem global state like that is frowned upon.

Hardware peripheral access in the Rust Embedded ecosystem is generally managed explicitly via owned values which are passed as parameters to functions after device initialization. This avoids any sort of racy "life before main"-style initialization problems.

@tarcieri
Copy link
Contributor

Let me put it this way:

  • A convenience function which uses OsRng seems OK. You don't need to explicitly pull in getrandom though, or even rand to achieve that. If the std feature of rand_core is enabled, rand_core::OsRng will be available, wrapping getrandom
  • I don't think it makes sense to define a generate_with_rng function that accepts a parameter other than something like impl CryptoRng + RngCore

@dvc94ch dvc94ch force-pushed the fix-rand-breakage branch from 7744afb to 3a54d0e Compare April 15, 2021 16:15
@dvc94ch
Copy link
Author

dvc94ch commented Apr 15, 2021

third time is the charm

@dvc94ch
Copy link
Author

dvc94ch commented Apr 27, 2021

ping @isislovecruft

@dvc94ch
Copy link
Author

dvc94ch commented Apr 29, 2021

Closing for now as there doesn't seem to be any interest. Feel free to reopen if things change.

@dvc94ch dvc94ch closed this Apr 29, 2021
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.

rand update to v0.8
2 participants