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.

Sign up for GitHub

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

Jump to bottom

Add support for Windows CA certificate store #18070

Closed
wants to merge 8 commits into from
Closed

Add support for Windows CA certificate store #18070

wants to merge 8 commits into from

Conversation

hlandau
Copy link
Member

@hlandau hlandau commented Apr 8, 2022

This PR adds support for the Windows CA certificate store. It does this by introducing a new store in the default provider which recognises the capi:// URI scheme. The URI used is just capi://, there are no arguments. This new store also becomes the default store when capistore is enabled at compile time (i.e., on Windows).

A build option is added (capistore) which can be disabled if this functionality is not desired.

This fixes #18020 but also implements my proposal in #18068 and is built on top of that fix. I can split these out into separate PRs if desired, but the former would preferably be merged before the latter.

@hlandau hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels Apr 8, 2022
@hlandau hlandau requested review from levitte and mattcaswell April 8, 2022 12:46
@hlandau hlandau self-assigned this Apr 8, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Apr 8, 2022
@hlandau
Copy link
Member Author

hlandau commented Apr 8, 2022

Manpage.

@petrovr
Copy link

petrovr commented Apr 8, 2022

I'm not convinced that proposed solution should change existing methods.
Implementation of Store2 API must be enough.

@petrovr
Copy link

petrovr commented Apr 8, 2022

About implementation - it is good to see a provider based sample. I have a number of engine based implementation including own by_store x509 lookup for openssl before 3.
Also I expect not so generic function names, i.e. with store as part of name. Perhaps current names are acceptable as static functions.
I could not see "error", "expect" and "find" methods. May be "expect" is superseded by "set...param...". If think that other two are required.

@petrovr
Copy link

petrovr commented Apr 8, 2022

P.S.I think that store name could be extracted from URI.

t8m
t8m reviewed Apr 11, 2022
View reviewed changes
include/internal/common.h Outdated Show resolved Hide resolved
@kaduk
Copy link
Contributor

kaduk commented Apr 11, 2022

Please register any new URI schemes with IANA: https://www.iana.org/assignments/uri-schemes/uri-schemes.xhtml -- provisional registrations are given on a "first come first served" policy.

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

@beldmit
Copy link
Member

beldmit commented May 12, 2022

I strongly support this idea. If I remember correctly, windows has more than one storage (at least, system and user's). Will this code work with it?

@hlandau
Copy link
Member Author

hlandau commented May 13, 2022

@beldmit It uses the system root CA store by default. Since this code uses an URI to specify the Windows store there is the possibility we could allow other stores to be specified.

@beldmit
Copy link
Member

beldmit commented May 13, 2022

How do we override this default URI in run-time?

@hlandau
Copy link
Member Author

hlandau commented May 13, 2022

The same way the URI is currently specified, e.g. via an environment variable, see this PR. I could add support for specifying different stores.

@beldmit
Copy link
Member

beldmit commented May 13, 2022

It looks better to me if it is overrideable via API.

@hlandau
Copy link
Member Author

hlandau commented May 13, 2022

That is also feasible.

@levitte
Copy link
Member

levitte commented May 19, 2022

As mentioned here, capi: isn't a good scheme name: #18068 (comment)

t8m
t8m requested changes May 19, 2022
View reviewed changes
crypto/x509/by_store.c Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented May 23, 2022

As mentioned here, capi: isn't a good scheme name: #18068 (comment)

Suggestion regarding the name: winstore:...
non-registration alternative: org.openssl.winstore: (same as we did with engines, in #13570)

@hlandau hlandau mentioned this pull request Jun 14, 2022
Closed
@levitte
Copy link
Member

levitte commented Jun 16, 2022

As mentioned here, capi: isn't a good scheme name: #18068 (comment)

Suggestion regarding the name: winstore:... non-registration alternative: org.openssl.winstore: (same as we did with engines, in #13570)

Fair warning: I'm going to be quite insistent on this

@hlandau
Copy link
Member Author

hlandau commented Jun 29, 2022

Rebased. Updated to implement option C. URI scheme renamed to winstore://.

levitte
levitte approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@levitte levitte left a comment

Choose a reason for hiding this comment

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

Looking good at this point.

@hlandau
Copy link
Member Author

hlandau commented Sep 13, 2022

Updated.

@t8m t8m added approval: done This pull request has the required number of approvals and removed approval: done This pull request has the required number of approvals labels Sep 13, 2022
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 14, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member Author

hlandau commented Sep 14, 2022

Merged to master.

@hlandau hlandau closed this Sep 14, 2022
openssl-machine pushed a commit that referenced this pull request Sep 14, 2022
@hlandau
Fix URI handling in SSL_CERT_DIR/introduce SSL_CERT_URI env
021859b 
Fixes #18068.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18070)
openssl-machine pushed a commit that referenced this pull request Sep 14, 2022
@hlandau
Add support for loading root CAs from Windows crypto API
606e042 
Fixes #18020.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #18070)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@hlandau @sftcd
Fix URI handling in SSL_CERT_DIR/introduce SSL_CERT_URI env
b90fca5 
Fixes openssl#18068.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18070)
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@hlandau @sftcd
Add support for loading root CAs from Windows crypto API
94c9bba 
Fixes openssl#18020.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18070)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@hlandau @beldmit
Fix URI handling in SSL_CERT_DIR/introduce SSL_CERT_URI env
9917dac 
Fixes openssl#18068.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18070)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
@hlandau @beldmit
Add support for loading root CAs from Windows crypto API
444d9ef 
Fixes openssl#18020.

Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#18070)
@vszakats vszakats mentioned this pull request Apr 28, 2023
Closed
This was referenced May 26, 2023
Closed
Closed
hlandau added a commit to hlandau/openssl that referenced this pull request Jun 13, 2023
@hlandau
Partially revert openssl#18070 (Add support for Windows CA certificat…
47b06fd 
…e store)
hlandau added a commit to hlandau/openssl that referenced this pull request Jun 14, 2023
@hlandau
fixup! Partially revert openssl#18070 (Add support for Windows CA cer…
aa40a4f 
…tificate store)
openssl-machine pushed a commit that referenced this pull request Jun 15, 2023
@hlandau @paulidale
Partially revert #18070 (Add support for Windows CA certificate store)
dfdbc11 
Reviewed-by: Richard Levitte <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
Reviewed-by: Matt Caswell <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #21190)
@mattcaswell mattcaswell mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levitte levitte levitte approved these changes

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell Awaiting requested review from mattcaswell

Assignees

@hlandau hlandau

Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows-ROOT as truststore
8 participants
@hlandau @petrovr @kaduk @openssl-machine @beldmit @levitte @t8m @mattcaswell