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

3.10.2 changes for master #1219

Merged
merged 6 commits into from Apr 4, 2021
Merged

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Mar 30, 2021

This embeds the changes of 3.10.2 for master, namely #1218 and #1209. Superseeds #1216.

@hannesm hannesm changed the title mirage-crypto-rng will soon have 0.9.0 release without API changes (i… 3.10.2 changes for master Mar 30, 2021
@hannesm
Copy link
Member Author

hannesm commented Mar 30, 2021

The CI faliure shows that some generated main.ml now initialize the random device multiple times (there are multiple random devices) -- this is not good. This needs to be investigated and fixed before merge!

@TheLortex
Copy link
Member

For information, the random devices involved are:

  • in Mirage_impl_stack.direct_stackv4: by default it's rng ~time ~mclock ()
  • in Mirage_impl_conduit and the test config: default_random

@hannesm
Copy link
Member Author

hannesm commented Mar 31, 2021

Thanks for looking into this @TheLortex. My intuition is that we should use default_random (to avoid multiple RNG devices), as done in 4102b23.

There's still a question about flexibility and what the default should be, in case a user wants a custom "default time" / "default clock" / "default random", how would they pass it through -- I'm not sure where (and when) to discuss this. OTOH from a security perspective, since entropy is (with mirage-crypto-rng) only fed to a single RNG, we should ensure that Mirage only ever generates a single "real RNG".

@hannesm
Copy link
Member Author

hannesm commented Mar 31, 2021

IMHO good to be merged.

@TheLortex
Copy link
Member

CI-wise, this is looking good. The mirage-skeleton PR mirage/mirage-skeleton#299 fixes the remaining errors.

@hannesm
Copy link
Member Author

hannesm commented Apr 2, 2021

@TheLortex great. Feel free to merge then. For mirage-skeleton, I used to rebase & force-push the dev branch (to have the few commits ahead of master clearly visible, and easily merge them to master one day mirage@master is out).

@dinosaure dinosaure merged commit ce38817 into mirage:master Apr 4, 2021
@dinosaure
Copy link
Member

Thanks both!

@hannesm hannesm deleted the mirage-crypto-rng-master branch April 4, 2021 16:54
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.

None yet

3 participants