-
Notifications
You must be signed in to change notification settings - Fork 44
Add Windows support #39
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this code path. Why did you choose BCryptGenRandom with BCRYPT_USE_SYSTEM_PREFERRED_RNG? According to what I found on the Internet, this may use the rather weak DUAL_EC generator? And is only available since Windows 8 (fine with me).
but more concerning, looking into other people's code (here libsodium):
/* `RtlGenRandom` is used over `CryptGenRandom` on Microsoft Windows based systems:
* - `CryptGenRandom` requires pulling in `CryptoAPI` which causes unnecessary
* memory overhead if this API is not being used for other purposes
* - `RtlGenRandom` is thus called directly instead. A detailed explanation
* can be found here: https://blogs.msdn.microsoft.com/michael_howard/2005/01/14/cryptographically-secure-random-number-on-windows-without-using-cryptoapi/
*
* In spite of the disclaimer on the `RtlGenRandom` documentation page that was
* written back in the Windows XP days, this function is here to stay. The CRT
* function `rand_s()` directly depends on it, so touching it would break many
* applications released since Windows XP.
*
* Also note that Rust, Firefox and BoringSSL (thus, Google Chrome and everything
* based on Chromium) also depend on it, and that libsodium allows the RNG to be
* replaced without patching nor recompiling the library.
*/
from that, I'd suggest to use RtlGenRandom. What do you think?
I'm in two minds about which one to use. It's a tossup between a complex linkage (for rtlgenrandom and advapi32.dll) and unclear semantics (for BCryptGenRandom, see here for the variations). I picked the Microsoft-recommended version, but I'll give the RtlGenRandom one a shot now to see how that diff looks. I'm only testing on Windows 10 / Server 2016. |
@avsm thanks for your work and comments. I'm not a windows expert. I suspect if we check for a recent enough windows version, we can as well avoid using DUAL_EC_DRBG, from https://docs.microsoft.com/en-us/windows/win32/seccng/cng-algorithm-identifiers?redirectedfrom=MSDN
-> I'd be happy with a code comment (why this is fine) and a test (if achievable) that at least windows 10 is used |
Decision with @hannesm: we will use bcrypt as its more modern, and Windows 10 is our base of support anyway. I'll push a README and code comment note to this effect. |
Co-Authored-By: Hannes Mehnert <[email protected]>
Move the doc for `initialize` to the function itself.
ok, that looks good. could you add a |
Good catch! Done and will merge when CI passes |
…mirage-crypto-entropy (0.6.1) CHANGES: * Add Windows 10+ support (mirage/mirage-crypto#39 @avsm, review by @hannesm @dinosaure) * Fix unix stubs to be allocating in case of exception (mirage/mirage-crypto#39 @avsm review by @hannesm @dinosaure) * Add GitHub Actions CI (mirage/mirage-crypto#39 @avsm) * Rebuild source if the `MIRAGE_CRYPTO_ACCELERATE` environment variable changes value (mirage/mirage-crypto#40 @avsm)
Need this to release conduit (which needs Windows support as well). It also fixes a bug in the C stubs, and adds GH Actions CI so that we can test Windows in CI as well.