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

Option to use windows ca store with openssl. #4346

Closed
wants to merge 2 commits into from

Conversation

@gvollant
Copy link
Contributor

@gvollant gvollant commented Sep 13, 2019

This patch allow using Windows certificate store for Windows curl compiled with openssl

the curl command line utility with automatically uses it when there is no --cacert , no --capath option and curl-ca-bundle.crt is not specified
(before the patch, the https connexion failed)
user can also enter

https://curl.haxx.se//mail/lib-2018-09/0038.html

https://curl.haxx.se//mail/lib-2018-09/0028.html

@gvollant gvollant changed the title Option to use windows ca store with openssl Option to use windows ca store with openssl. Sep 13, 2019
@bagder
Copy link
Member

@bagder bagder commented Sep 15, 2019

The travis builds using boringssl fail, due to unrelated reasons. The rest seem green!

Copy link
Member

@bagder bagder left a comment

And perhaps consider to squash all commits into a single one and force-push, to make this patch easier to review!

// so the only possible solution is using the windows ca store
config->cacert = strdup(CURL_WINDOWS_CA_STORE);
if(!config->cacert && !config->capath) {
/* now, we are under MS-Windows.

This comment has been minimized.

@bagder

bagder Sep 15, 2019
Member

Note that this is not using a 2-space indent level, which all curl code does. You should probably make sure all your code follow that style.

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 15, 2019

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 16, 2019

I don't understand why somes test fails...

@bagder
Copy link
Member

@bagder bagder commented Sep 16, 2019

With "squash", all your changes should be made into a single commit. Right now this change is very hard to review since it is spread out over a lot of commits where most of them change things done in previous commits...

With the git command line tool, it can be made like this:

  1. update your master to be in sync with upstream
  2. check out your feature branch git checkout branch
  3. rebase your branch: git rebase -i origin/branch - the -i means interacetive. Make all commit lines except the first one say 'f' in the first column, which means fixup and that means it'll be merged into the previous commit.
  4. Now all commits should have been turned into a single one in your branch.
  5. You can't push this normally now, you need to use git push -f to force push

I can work around this, but if I provide feedback on this patch how are you going to update it? Just keep adding commits and make it harder and harder for others to read?

PS at least one of your commits changed line endings on the code which makes things complicated at least for me.

@gvanem
Copy link
Contributor

@gvanem gvanem commented Sep 16, 2019

I don't understand why somes test fails...

First failed is test 714. Running it here gives me no clues either, but the log/memdump shows a leak:

memanalyze.pl -v log\memdump
Leak detected: memory still allocated: 64 bytes
At af93ce8, there's 64 bytes.
 allocated by mprintf.c:1049
@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 38211dc to 26866fb Sep 16, 2019
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 16, 2019

I done the squash

.travis.yml Outdated
sudo: required
go:
"1.13"

This comment has been minimized.

@mback2k

mback2k Sep 16, 2019
Member

This change seems not to be related.

This comment has been minimized.

@gvollant

gvollant Sep 16, 2019
Author Contributor

It's probably a merge or rebase... I don't want change myself ...

it comes probably from ac34c70

curl compiled for MS-Windows using OpenSSL for using Windows CA Store
to verify peer
as suggested in https://curl.haxx.se/mail/lib-2018-09/0039.html */
#define CURL_WINDOWS_CA_STORE "\\\\\\WINDOWS_CA_STORE"

This comment has been minimized.

@mback2k

mback2k Sep 16, 2019
Member

The number of backslashes seems a bit weird, especially considering escaping. Maybe think of something more clear?

This comment has been minimized.

@gvollant

gvollant Sep 16, 2019
Author Contributor

there is 3*2=6. backslash
on windows a path beginning with two backslash can be a network path
three baskslash is impossible so there is no conflict
https://en.wikipedia.org/wiki/Path_(computing)#Uniform_Naming_Convention )

This comment has been minimized.

@bagder

bagder Dec 11, 2019
Member

If you'd use three forward-slashes instead, you'd avoid a lot of escaping problems and still be pretty safe, won't you? I think the main problem is here if we expect command line users to be able to set this.

This comment has been minimized.

@gvollant

gvollant Dec 11, 2019
Author Contributor

ok, I just replaced backslash by forward slash

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 17, 2019

@bagder
Copy link
Member

@bagder bagder commented Sep 17, 2019

I done the squash

You also merged two unrelated commits (from another branch), which is what broke the tests. When you've squashed and rebased, this should only be a single commit and definitely not commits that were authored by someone else than you!

I think this starts to look fine. I mostly miss the documentation of this new feature now.

To think about: Is there a way an application can know or should be able to figure out if libcurl supports this at run-time?

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 26866fb to 4d11fe5 Sep 17, 2019
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 17, 2019

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch 2 times, most recently from 138a6fa to 1058cc0 Sep 18, 2019
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 18, 2019

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 19, 2019

Sorry, I don't have idea of what need modification? Can you help me?

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch 4 times, most recently from 03b4db2 to 0311f16 Sep 20, 2019
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Sep 28, 2019

I don't understand the travis problem

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 0311f16 to 182f3c5 Oct 3, 2019
"ROOT");

if(hStore) {
while((pContext = CertEnumCertificatesInStore(hStore, pContext))

This comment has been minimized.

@jay

jay Oct 6, 2019
Member

do any attributes of the certificate have to be checked like notbefore notafter etc or does openssl do this

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 182f3c5 to df1ee46 Oct 20, 2019
@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from df1ee46 to a16cf9c Nov 10, 2019
@bagder
Copy link
Member

@bagder bagder commented Nov 10, 2019

Maybe also add a little mention in the docs for the option where this can be set?

@bagder bagder added the SSL/TLS label Nov 10, 2019
@mback2k

This comment was marked as off-topic.

@gvollant

This comment was marked as off-topic.

@michael-o

This comment was marked as off-topic.

@mback2k

This comment was marked as off-topic.

@gvollant gvollant requested a review from jay Apr 8, 2020
@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 63b05f8 to eddb82a Apr 8, 2020
@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from eddb82a to a1692ab Apr 29, 2020
@michael-o
Copy link
Member

@michael-o michael-o commented Apr 29, 2020

I ask myself whether it is a good idea to document CURLSSLOPT_NATIVE_CA explicitly with Windows. One can probably write a patch for obtain certs from macOS's Keychain which would be a native CA store too.

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Apr 29, 2020

I ask myself whether it is a good idea to document CURLSSLOPT_NATIVE_CA explicitly with Windows. One can probably write a patch for obtain certs from macOS's Keychain which would be a native CA store too.

Yes, good idea. This is probably easy

https://stackoverflow.com/questions/42432473/programmatically-read-root-ca-certificates-in-ios

https://stackoverflow.com/questions/32472337/osx-export-system-certificates-from-keychain-in-pem-format-programmatically

@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from a1692ab to e3fbb17 May 3, 2020
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented May 5, 2020

I do a small text modification

@bagder
Copy link
Member

@bagder bagder commented May 7, 2020

Lots of CI failures still. Remember to run make checksrc locally to pick most of these ones up yourself:

./vtls/openssl.c:2889:82: warning: Longer than 79 columns (LONGLINE)
         /* Continue with a warning if no certificate verification is required. */
./vtls/openssl.c:2901:82: warning: Longer than 79 columns (LONGLINE)
         /* Continue with a warning if no certificate verification is required. */
checksrc: 0 errors and 2 warnings
checksrc: 0 errors and 5 warnings suppressed
@gvollant gvollant force-pushed the gvollant:winopenssl/wincastore_openssl branch from 0350ac8 to 1f150c7 May 7, 2020
@gvollant
Copy link
Contributor Author

@gvollant gvollant commented May 7, 2020

Copy link
Member

@michael-o michael-o left a comment

There is an inconsistency throughout in the documentation and log messages. Sometimes "root certificates", "ca certificates", "CA certificates", etc. Can you apply/propose consistent naming?
Nitpicking: also note that the native CA store might not only contain root certificates, but also intermediate ones.

.IP CURLSSLOPT_NATIVE_CA
With the build against OpenSSL library, uses the native operating system
store of root certificates instead of any other CA certificates.
Currenly supported with MS-Windows.

This comment has been minimized.

@michael-o

michael-o May 7, 2020
Member

Please either write "Windows" or "Microsoft Windows" consistently.

store of root certificates instead of any other CA certificates.
Currenly supported with MS-Windows.
Caution: use only CURLOPT_CAINFO with curl 7.70.1 or later for MS-Windows
using OpenSSL library. On other configuration, curl will have no certificate.

This comment has been minimized.

@michael-o

michael-o May 7, 2020
Member

This sentence has a weird word oder.

This comment has been minimized.

@gvollant

gvollant May 7, 2020
Author Contributor

can you suggest me alternative text modified?
My english is not perfect, so I prefer you suggest a new version than ask me fix, then review and ask modify again

This comment has been minimized.

@michael-o

michael-o May 7, 2020
Member

Will do

This comment has been minimized.

@bagder

bagder May 8, 2020
Member

I don't understand the sentence at all. It describes CURLSSLOPT_NATIVE_CA and then it speaks of CURLOPT_CAINFO without explaining how they relate here. Can you elaborate what this is trying to say?

This comment has been minimized.

@bagder

bagder May 8, 2020
Member

My edited version says this now, but I'd like to understand the comment about CURLOPT_CAINFO so that we can perhaps add that:

Tell libcurl to use the operating system's native CA store for certificate
verifiction. Works when built to use OpenSSL on Windows. (Added in 7.71.0)

This comment has been minimized.

@gvollant

gvollant May 8, 2020
Author Contributor

wa must remove CURLOPT_CAINFO reference. I wrote this test before we add CURLSSLOPT_NATIVE_CA

So now, we must only say CURLOPT_CAINFO is only for new version of curl (for windows , and pehaps on other OS later).
Before new version of curl, we need provide a file of certificate

This comment has been minimized.

@bagder

bagder May 8, 2020
Member

We can possibly add that CURLSSLOPT_NATIVE_CA overrides CURLOPT_CAINFO if both are set...

@michael-o
Copy link
Member

@michael-o michael-o commented May 7, 2020

For those who'd like to play with EKUs, try our Siemens CAs:
siemens-certs.txt

@bagder bagder closed this in 148534d May 8, 2020
@bagder
Copy link
Member

@bagder bagder commented May 8, 2020

Thanks a lot @gvollant for your very hard work on this and for your patience. This has now landed, scheduled to ship in the next release; 7.71.0.

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented May 8, 2020

Thanks a lot @gvollant for your very hard work on this and for your patience. This has now landed, scheduled to ship in the next release; 7.71.0.

no problem, and thank you for help me. You learned me somes git command, the nice checksrc tool and help me with my poor english :-)

@mhils
Copy link

@mhils mhils commented May 8, 2020

Sorry for chiming in late here, but I just saw @bagder's tweet and figured this should at least be mentioned: There's been an extensive discussion among Python folks regarding OpenSSL + system trust stores in psf/requests#2966. The consensus seems to be that enumerating the Windows CA store isn't really a good idea.
First, Windows doesn't ship with all of the certificates available and downloads them on-demand when certificates are validated. cURL may not trust some legitimate CAs on fresh Windows installs. Not sure if that is a practical problem, but something to keep in mind.
Second, it's a bit of a security concern. @Lukasa sums it up nicely (psf/requests#2966 (comment)):

A side note: the wrong fix is to extract the root certificates from the system and have the Python OpenSSL validate using them as roots. The system trust store has its own validation logic on macOS and Windows, and failing to use it can lead to spurious trust failures at best and incorrect trust successes at worst. You have to entirely delegate cert verification to the system APIs to do this properly, by handing the peer cert chain over.

So yeah - I know this is merged, I really don't want to take away from @gvollant's excellent work here, but there are some security concerns so I figured I should bring it up.

@mback2k
Copy link
Member

@mback2k mback2k commented May 8, 2020

You have to entirely delegate cert verification to the system APIs to do this properly, by handing the peer cert chain over.

One alternative approach could be to instead disable the verification in OpenSSL, ask it to provide the server certificate to libcurl and then validate it similar to the code in schannel_verify.c:

CURLcode Curl_verify_certificate(struct connectdata *conn, int sockindex)

First, Windows doesn't ship with all of the certificates available and downloads them on-demand when certificates are validated. cURL may not trust some legitimate CAs on fresh Windows installs. Not sure if that is a practical problem, but something to keep in mind.

I think this applies to intermediate CAs as root CAs are kept up to date via Windows Update.

Update: I was wrong, root CAs are also fetched on demand: https://docs.microsoft.com/en-us/previous-versions//cc751157(v=technet.10)?redirectedfrom=MSDN

Starting with the release of Windows Vista, root certificates are updated on Windows automatically. When a user visits a secure Web site (by using HTTPS SSL), reads a secure email (S/MIME), or downloads an ActiveX control that is signed (code signing) and encounters a new root certificate, the Windows certificate chain verification software checks the appropriate Microsoft Update location for the root certificate. If it finds it, it downloads it to the system. To the user, the experience is seamless. The user does not see any security dialog boxes or warnings. The download happens automatically, behind the scenes.

@bagder
Copy link
Member

@bagder bagder commented May 8, 2020

Security wise, this option has many angles and aspects and it is not at all necessary worse than without it.

Many people and applications are already converting the windows CA store into a PEM to use with curl and other applications so for those users, this step just makes things easier and less error-prone.

The argument about which CA store to use and trust could even be used the other way around as using the Windows store could in some cases be easier and be more likely to be updated and accurate rather than the separate PEM file. And more in sync with other applications and subsystems that might also be used.

The PEM file itself is often extracted from Mozilla and used outside of their verification code which similar to the Windows verification APIs also does a lot of other things and checks apart from using just the certs, so "just" using the PEM is also already a shortcut and one with caveats.

I think it is important to clearly document what this option means and does and leave to the user to make an enlightened decision.

One alternative approach could be to instead disable the verification in OpenSSL, ask it to provide the server certificate to libcurl and then validate it similar to the code in schannel_verify.c

Yes, I think that would be an improvement!

@jay
Copy link
Member

@jay jay commented May 9, 2020

IIRC recent versions of Windows routinely poll for root certificates. If you have a specific "incorrect trust success" example let's hear it. Regarding verification I just don't see why we should mix it up any more. We have Schannel backend, and OpenSSL backend, and now both of them can use the OS root store or the certificate bundle. That seems like enough to me.

@mback2k
Copy link
Member

@mback2k mback2k commented May 9, 2020

I just don't see why we should mix it up any more.

I wasn't talking about mixing it up, just change the way the OpenSSL backend uses the root CA store. Instead of "mirroring" it into the OpenSSL store, it could just ask Windows to verify the certificate. That would solve any potential CA refreshing issues and could improve the overall startup performance.

Slightly off topic: I must say I only ever experienced the CA refresh with Outlook on Windows missing some intermediates while validating S/MIME e-mail signatures. This lead to the e-mail signature first showing up as invalid and a few seconds later showing as valid.

@gvollant
Copy link
Contributor Author

@gvollant gvollant commented Jul 6, 2020

I open #5657 because the feature like I expected was disabled by #5585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.