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

Windows support #14

Merged
merged 1 commit into from Mar 5, 2021
Merged

Windows support #14

merged 1 commit into from Mar 5, 2021

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Mar 1, 2021

Closes #14

On Windows, the root certificates are available in the system store named "ROOT". The corresponding certificate contexts hold a DER encoding of the certificates, which is exposed through a C external.

See https://docs.microsoft.com/en-us/windows/win32/seccrypto/example-c-program-listing-the-certificates-in-a-store

@emillon emillon force-pushed the windows branch 4 times, most recently from 4555030 to fcb0f4f Compare March 2, 2021 09:01
Copy link

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cons cells aren't tagged correctly, but for lists I'd use the low-level allocation functions

lib/ca_certs_stubs.c Outdated Show resolved Hide resolved
lib/ca_certs_stubs.c Outdated Show resolved Hide resolved
lib/ca_certs_stubs.c Outdated Show resolved Hide resolved
lib/ca_certs_stubs.c Outdated Show resolved Hide resolved
lib/ca_certs_stubs.c Show resolved Hide resolved
@emillon emillon force-pushed the windows branch 2 times, most recently from aea23cf to 07eef05 Compare March 2, 2021 12:56
@hannesm
Copy link
Member

hannesm commented Mar 3, 2021

Thanks for your work @emillon. It would be nice if you could add a GitHub action CI that executes the build and test on Windows. (an example yaml file is available at https://github.com/mirage/mirage-crypto/blob/main/.github/workflows/windows.yml).

@hannesm
Copy link
Member

hannesm commented Mar 3, 2021

FWIW, I don't think that we need the PEM formatted certificates (getting DER and decoding is fine) -- as indicated by the open tasks. I'm in favor to setup CI and merge and release this PR.

@emillon
Copy link
Collaborator Author

emillon commented Mar 4, 2021

OK! I haven't thought everything fully, this is why I marked this as WIP. I'll let you know when that can be reviewed.

For the github action, it won't work well if I'm adding it in this PR (when the PR is from a fork, it's the action from main that's used to test the PR) so I'll do this first in another PR and rebase this one.

@emillon emillon force-pushed the windows branch 2 times, most recently from 5efc686 to ce9010f Compare March 4, 2021 11:19
emillon added a commit to emillon/ca-certs that referenced this pull request Mar 4, 2021
Closes mirage#14

On Windows, the root certificates are available in the system store
named "ROOT". The corresponding certificate contexts hold a DER encoding
of the certificates, which is exposed through a C external.

See <https://docs.microsoft.com/en-us/windows/win32/seccrypto/example-c-program-listing-the-certificates-in-a-store>
Closes mirage#14

On Windows, the root certificates are available in the system store
named "ROOT". The corresponding certificate contexts hold a DER encoding
of the certificates, which is exposed through a C external.

See <https://docs.microsoft.com/en-us/windows/win32/seccrypto/example-c-program-listing-the-certificates-in-a-store>
@emillon emillon changed the title WIP: Windows support Windows support Mar 5, 2021
@emillon
Copy link
Collaborator Author

emillon commented Mar 5, 2021

This is now ready for review.

The list of certificates that are returned correspond to the ones in the list of Trusted Root Certificate Authorities:

image

About encoding: certificate contexts have an implicit certificate encoding type. At the moment, the only one is X509_ASN_ENCODING, but I still added a check to make sure we're not trying to decode as DER something that's not supposed to be. An alternative would be to call a C function to encode directly as PEM (ignoring how the context is encoded).

@emillon emillon marked this pull request as ready for review March 5, 2021 10:43
@hannesm
Copy link
Member

hannesm commented Mar 5, 2021

Thanks @emillon, to me this looks fine. The only question I have whether the flags.sexp dance is required to pass ldflags depending on the os (in other words: is there a way to do this without a temporary file?).

@emillon
Copy link
Collaborator Author

emillon commented Mar 5, 2021

Thanks @emillon, to me this looks fine. The only question I have whether the flags.sexp dance is required to pass ldflags depending on the os (in other words: is there a way to do this without a temporary file?).

There are some alternatives (reading from flags.%{os_type} and adding rules to support all os types; writing the flags file from an executable instead of dune rules), but nothing that skips the temporary file.

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.

looks fine (thanks for taking your time to answer my questions), feel free to merge and release to opam-repository.

@emillon
Copy link
Collaborator Author

emillon commented Mar 5, 2021

Thanks!

@emillon emillon merged commit a7e1e2f into mirage:main Mar 5, 2021
@emillon emillon deleted the windows branch March 5, 2021 13:24
emillon added a commit to emillon/opam-repository that referenced this pull request Mar 5, 2021
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

4 participants