Skip to content

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

Merged
merged 7 commits into from
Mar 15, 2020
Merged

Add Windows support #39

merged 7 commits into from
Mar 15, 2020

Conversation

avsm
Copy link
Member

@avsm avsm commented Mar 15, 2020

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.

@avsm avsm requested a review from hannesm March 15, 2020 00:11
Copy link
Member

@hannesm hannesm left a 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?

@avsm
Copy link
Member Author

avsm commented Mar 15, 2020

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.

@hannesm
Copy link
Member

hannesm commented Mar 15, 2020

@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

Windows 8: Beginning with Windows 8, the EC RNG algorithm supports FIPS 186-3. Keys less than or equal to 1024 bits adhere to FIPS 186-2 and keys greater than 1024 to FIPS 186-3.

Windows 10: Beginning with Windows 10, the dual elliptic curve random number generator algorithm has been removed. Existing uses of this algorithm will continue to work; however, the random number generator is based on the AES counter mode specified in the NIST SP 800-90 standard. New code should use BCRYPT_RNG_ALGORITHM, and it is recommended that existing code be changed to use BCRYPT_RNG_ALGORITHM. 

-> I'd be happy with a code comment (why this is fine) and a test (if achievable) that at least windows 10 is used

@avsm
Copy link
Member Author

avsm commented Mar 15, 2020

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.

avsm and others added 2 commits March 15, 2020 17:06
Move the doc for `initialize` to the function itself.
@avsm
Copy link
Member Author

avsm commented Mar 15, 2020

This should be ready for merge now. I rearranged the doc slightly in 2260884 @hannesm, if you want to glance over that.

@hannesm
Copy link
Member

hannesm commented Mar 15, 2020

ok, that looks good. could you add a dune-configurator {build} dependency to mirage-crypto-rng.opam, please?

@avsm
Copy link
Member Author

avsm commented Mar 15, 2020

Good catch! Done and will merge when CI passes

@avsm avsm merged commit 77df68f into mirage:master Mar 15, 2020
@avsm avsm deleted the win-port branch March 15, 2020 18:11
@hannesm hannesm mentioned this pull request Mar 15, 2020
avsm added a commit to avsm/opam-repository that referenced this pull request Mar 15, 2020
…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)
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.

3 participants