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

Mark the initialize functions as deprecated. #254

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Jan 8, 2025

Instead, advertise the use of the default Unix generator based on getrandom/getentropy. This is thread-safe and allows to remove the mirage-crypto-rng-{lwt,miou-unix,eio,async} packages in the future.

The downside is that fortuna / seeding is only exercised on MirageOS. The upside is that this means we've less struggle and ease the maintenance. WDYT? (cc @reynir @dinosaure)

@dinosaure
Copy link
Member

I think I need a time to look on the Windows issue about Miou. Also, I don't think that the Miou port should have a deprecation where Pfortuna is domain-safe - if I remember, it was the initial issue.

@hannesm hannesm force-pushed the deprecate-rng-initialize branch from a2e976d to ef34dc8 Compare January 8, 2025 13:20
@hannesm
Copy link
Member Author

hannesm commented Jan 8, 2025

@dinosaure sure, we can retain the miou-unix code, and not deprecate it. Done in a subsequent commit. I was maybe too eager, or thought why not simply have a uniform interface, and use getrandom/getentropy/BCryptRandom on miou-unix as well?

Instead, advertise the use of the default Unix generator based on
getrandom/getentropy. This is thread-safe and allows to remove the
mirage-crypto-rng-{lwt,miou-unix,eio,async} packages in the future.
@hannesm hannesm force-pushed the deprecate-rng-initialize branch from 688cc6d to 698033f Compare January 8, 2025 13:29
@dinosaure
Copy link
Member

thought why not simply have a uniform interface, and use getrandom/getentropy/BCryptRandom on miou-unix as well?

Because of the bug on Windows :), I still don't know what it's related to and why it's only appearing now 😕.

@hannesm
Copy link
Member Author

hannesm commented Jan 8, 2025

Maybe @talex5 @bikallem have an opinion about the mirage-crypto-rng-eio module - if only getrandom/getentropy are used, do you see a reason to keep this run stuff around?

Maybe @seliopou has an opinion on the mirage-crypto-rng-async module -- if we deprecate initialize and later remove the package, would that be fine? Random would be provided via getrandom/getentropy.

Maybe @cfcs has an opinion (who was involved in some of the mirage-crypto-rng-lwt work)?

Also maybe @edwintorok?

Also @emillon who if I remember correctly was concerned back in the days about thread safety of the RNG -- this PR would bring this straightforward to us.

@edwintorok
Copy link
Contributor

Can (P)Fortuna be exercised in the CI even when not a unikernel? Just because it isn't the default, and isn't thread safe it doesn't mean we can't explicitly instantiate that RNG in order to test it.

@hannesm
Copy link
Member Author

hannesm commented Jan 8, 2025

Can (P)Fortuna be exercised in the CI even when not a unikernel? Just because it isn't the default, and isn't thread safe it doesn't mean we can't explicitly instantiate that RNG in order to test it.

Sure. This can always be done. The lwt/eio/async code mainly consists of hooking some RDTSC in the main event loop to gather some further entropy for the fortuna pools. And a periodic task to rdseed/rdrand.

For the CI and testing, I don't think it is worth to test the different schedulers. Of course the rdrand/rdseed and sanety of entropy feeding should be exercised (as is via test_entropy / test_entropy_collection -- maybe even better using some statistical tests). Also, the fortuna implementation itself should be tested in respect to the literature and test vectors (given deterministic seed, there should be deterministic output). We can even use the mirage-crypto-rng-mirage opam package (even on Unix without a unikernel).

@talex5
Copy link
Contributor

talex5 commented Jan 8, 2025

Maybe @talex5 @bikallem have an opinion about the mirage-crypto-rng-eio module - if only getrandom/getentropy are used, do you see a reason to keep this run stuff around?

I'm very happy to get rid of this!

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.

4 participants