Windows fixes #162
Windows fixes #162
Conversation
|
Thanks for this! The PR that this conflicts with is #158, which switches to Dune (and should make Windows significantly easier). I didn't quite understand the reason for having a new Windows module for the entropy -- what didn't work with the existing Lwt one? A lot of the code looks shared. |
|
I copied the old code but as there is no If #158 is going to be merged soon I would love to implement this against that branch instead as that would likely be easier/cleaner. |
| BCryptCloseAlgorithmProvider(phAlgorithm, 0); | ||
|
|
||
| return Val_int(32); | ||
| } |
There was a problem hiding this comment.
I'm curious about the following points:
- why not pass the
32as input toget_random_bytes(get_random_bytes : int -> buffer -> unit)? - according to https://docs.microsoft.com/en-us/windows/desktop/api/Bcrypt/nf-bcrypt-bcryptgenrandom the
BCryptGenRandom(and also theOpenAlgorithmProvider) returns aNTSTATUS, which should be checked forSTATUS_SUCCESS(as it is now, an error will be silently discarded and the buffer will be used as if it contained random bytes). - what is the reason for
BCRYPT_RSA_ALGORITHM? I'd have expectedBCRYPT_RNG_ALGORITHMhere (from https://docs.microsoft.com/en-us/windows/desktop/SecCNG/cng-algorithm-identifiers)
There was a problem hiding this comment.
To be honest this is more of a proof of concept, but I'm happy to clean it up to try to make a proper implementation.
I would love more pointers on how to do it as I'm still pretty new to this.
I tried the signature you suggest but had some issues with it, but that might be because I called it with the wrong data.
Is it be possible to pass a Cstruct.t directly and set the bytes in there?
There was a problem hiding this comment.
NB: I'm neither author nor maintainer of ocaml-nocrypto.
not sure what your goal is, and what "proof of concept" means to you. if your goal is only "nocrypto compiles on windows", you don't even have to mess around with C stubs (but can just use 0s). if you want to provide entropy for nocrypto's rng, you should ensure to retrieve random numbers from windows. in case you want others to use your code, you should be very careful to get proper entropy (i.e. check result codes from the windows API, run some tests to check that the call really modifies the buffer passed).
I have not programmed against windows APIs for years - the above comment is from searching for "/dev/urandom on windows" and following the API reference/guidelines from MSDN.
Is it be possible to pass a Cstruct.t directly and set the bytes in there?
I'm not entirely sure what this means, but you could read how other C functions are used in this library to figure out how it is done -- take a look into the native.ml module. Cstruct.t is just a wrapper (length, offset, bigarray), and to me it looks like the bigarray is passed to C code directly.
I've not looked into anything apart from the get_random_bytes implementation of this PR, and don't have a windows installation to test it.
There was a problem hiding this comment.
Proof of concept as in I wanted to be able to build and run a httpaf/H2 based webserver on Windows (which I now can). The initial goal of this PR was to see if there is any interest in this and to prove that it's possible.
As there seems to be a positive response to this I'll happily keep working on it. But since this is a lot lower level than I'm used to I appreciate the feedback.
As I see it what needs to be done in this part is:
Handle the possible errors, either by using a result or a option depending on what's possible with FFI.
Change to use BCRYPT_RNG_ALGORITHM.
Make the signature bytes -> int -> int, but with error handling.
If there is a possibility to not go via bytes but just use the Cstruct.t directly that would be preferable to the current implementation.
|
This is a PR that implements missing things for Windows.
I'm under no illusion that the code is good or that the linking is done in a idiomatic way but this made it work for me.
It adds a endian.h that is cross-platform compatible. Same as #161.
And a entropy implementation that works on Windows. I decided to try to just use the Windows entropy instead of the unix-specific one without the user choosing.
I'm opening this up for discussion around how to best support Windows going forward.