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

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 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

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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 commented Sep 15, 2019 via email

@gvollant
Copy link
Contributor Author

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

@bagder
Copy link
Member

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 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
Copy link
Contributor Author

I done the squash

.travis.yml Outdated
sudo: required
go:
"1.13"
Copy link
Member

Choose a reason for hiding this comment

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

This change seems not to be related.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 )

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I just replaced backslash by forward slash

@gvollant
Copy link
Contributor Author

gvollant commented Sep 17, 2019 via email

@bagder
Copy link
Member

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
Copy link
Contributor Author

gvollant commented Sep 17, 2019 via email

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

gvollant commented Sep 18, 2019 via email

@gvollant
Copy link
Contributor Author

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

@gvollant gvollant force-pushed the winopenssl/wincastore_openssl branch 4 times, most recently from 03b4db2 to 0311f16 Compare September 27, 2019 06:45
@gvollant
Copy link
Contributor Author

I don't understand the travis problem

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

if(hStore) {
while((pContext = CertEnumCertificatesInStore(hStore, pContext))
Copy link
Member

Choose a reason for hiding this comment

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

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

@bagder
Copy link
Member

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 TLS label Nov 10, 2019
@gvollant

This comment has been minimized.

@michael-o

This comment has been minimized.

@mback2k

This comment has been minimized.

@gvollant gvollant requested a review from jay April 8, 2020 12:37
@gvollant gvollant force-pushed the winopenssl/wincastore_openssl branch from 63b05f8 to eddb82a Compare April 8, 2020 14:43
@gvollant gvollant force-pushed the winopenssl/wincastore_openssl branch from eddb82a to a1692ab Compare April 29, 2020 09:50
@michael-o
Copy link
Contributor

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

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 winopenssl/wincastore_openssl branch from a1692ab to e3fbb17 Compare May 3, 2020 08:42
@gvollant
Copy link
Contributor Author

gvollant commented May 5, 2020

I do a small text modification

@bagder
Copy link
Member

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 winopenssl/wincastore_openssl branch from 0350ac8 to 1f150c7 Compare May 7, 2020 09:28
@gvollant
Copy link
Contributor Author

gvollant commented May 7, 2020 via email

Copy link
Contributor

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence has a weird word oder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@bagder bagder May 8, 2020

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@michael-o
Copy link
Contributor

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 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 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 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 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 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 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 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
Copy link
Contributor Author

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
Labels
feature-window A merge of this requires an open feature window TLS Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

7 participants