schannel: add support for CURLOPT_CAINFO. #1325

mcnulty · 2017-03-10T22:59:57Z

Modifications:

make verify_certificate functionality in curl_schannel.c
available on all versions of Windows instead of just
Windows CE. verify_certificate will only be invoked on
Windows CE or when the user specifies CURLOPT_CAINFO and
CURLOPT_SSL_VERIFYPEER
In verify_certificate, create a custom certificate chain
engine that exclusively trusts the certificate store backed
by the CURLOPT_CAINFO file
HAVE_EXCLUSIVE_TRUST_MODE macro defined in config-win32.h for
Windows 7+ to control enabling this feature that relies on
Windows 7+ functionality
doc updates of --cacert/CAINFO support for schannel
Use CERT_NAME_SEARCH_ALL_NAMES_FLAG when invoking CertGetNameString
when available. This implements a TODO in schannel.c to improve
handling of multiple SANs in a certificate. In particular, all SANs
will now be searched instead of just the first name.
Add new test cases 314 and 315. These test cases check that
the first and last SAN, respectively, matches the connection
hostname. New test certificates have been added for these
cases. For 314, the certificate prefix is Server-localhost-firstSAN
and for 315, the certificate prefix is Server-localhost-secondSAN
Remove TODO 15.2 (Add support for custom server certificate
validation), this commit addresses it.

Testing

I didn't have any luck running the test suite for this particular platform/lib configuration so I validated the following test cases by hand:

test305
test310
test311
test312
test314
test315
test2043

All the tests I validated (except for test305) required specifying --ssl-no-revoke because the test CA (EdelCurlRoot-ca) does not have any revocation checking data in its cert and SChannel requires revocation checking by default.

I also ran the testsuite (for good measure) on Ubuntu 16.04.2/x86_64, using ./configure without args (built against OpenSSL) and didn't see any test failures.

mention-bot · 2017-03-10T22:59:58Z

@mcnulty, thanks for your PR! By analyzing the history of the files in this pull request, we identified @masali-hp, @mback2k and @bagder to be potential reviewers.

bagder · 2017-03-16T17:01:52Z

The failed CI test is test 2033, which is yet another pipelining test we probably need to disable due to its flakiness.

mback2k

Thanks for your hard work on this feature and pull-request, I really appreciate it since I never had the time to implement this myself. I hope that you will find the time to address my review comments, thanks!

lib/vtls/schannel.c

mcnulty · 2017-04-26T05:17:39Z

Thanks for your comments @mback2k! I hope to be able to address them within a week or so.

mcnulty · 2017-07-05T21:43:39Z

Just checking in on this PR. I think I addressed all of @mback2k's comments. Is there anything more I can do to help shepherd this change along?

bagder · 2017-07-05T22:29:41Z

Can you please squash the commits (and rebase them) so that they are the minimum set of logical changes required to provide this feature? It makes them easier for a final review and the rebase/ṕush will make them run through the (now) updated CI tests. (and I too would like @mback2k's ack on this)

coveralls · 2017-07-07T19:38:13Z

Coverage increased (+0.01%) to 76.01% when pulling 966583e on mcnulty:schannel-cainfo into ca3545d on curl:master.

coveralls · 2017-07-07T23:49:26Z

Coverage increased (+0.05%) to 76.022% when pulling 9cb1985 on mcnulty:schannel-cainfo into a548183 on curl:master.

dscho · 2017-08-28T22:24:49Z

@mcnulty the conflicts are most likely due to my recent changes to support switching the SSL backend at runtime. Would you like me to help you resolving them?

mcnulty · 2017-10-15T17:09:27Z

This PR has been rebased on a master.

@dscho: Thanks for the offer to help resolve the conflicts. I think I was successfully able to resolve the conflicts. It appears all that needed to be done was to define backend-specific data in the ssl_backend_data struct instead of in the ssl_connect_data struct and then utilize the BACKEND macro in schannel.c to access the backend-specific data. Does that align with your understanding?

dscho · 2017-10-16T12:01:33Z

It appears all that needed to be done was to define backend-specific data in the ssl_backend_data struct instead of in the ssl_connect_data struct and then utilize the BACKEND macro in schannel.c to access the backend-specific data. Does that align with your understanding?

Yes, it does! Thanks!

mcnulty · 2017-10-16T14:59:25Z

Looks like the Travis T=normal C=--enable-ares build failed on test 1208:

test 1208...[FTP PORT download, no data conn and no transient negative reply]
1208: protocol FAILED:

- make verify_certificate functionality in curl_schannel.c available on all versions of Windows instead of just Windows CE. verify_certificate will only be invoked on Windows CE or when the user specifies CURLOPT_CAINFO and CURLOPT_SSL_VERIFYPEER. - In verify_certificate, create a custom certificate chain engine that exclusively trusts the certificate store backed by the CURLOPT_CAINFO file. - HAVE_EXCLUSIVE_TRUST_MODE macro defined in config-win32.h for Windows 7+ to control enabling this feature that relies on Windows 7+ functionality. - doc updates of --cacert/CAINFO support for schannel - Use CERT_NAME_SEARCH_ALL_NAMES_FLAG when invoking CertGetNameString when available. This implements a TODO in schannel.c to improve handling of multiple SANs in a certificate. In particular, all SANs will now be searched instead of just the first name. - Add new test cases 316 and 317. These test cases check that the first and last SAN, respectively, matches the connection hostname. New test certificates have been added for these cases. For 316, the certificate prefix is Server-localhost-firstSAN and for 317, the certificate prefix is Server-localhost-secondSAN. - Remove TODO 15.2 (Add support for custom server certificate validation), this commit addresses it. Closes curl#1325

jay

In addition to the comments I made in the review one more thing concerns me, the behavior of the curl tool currently is set to auto-detect certificate bundles on windows and prior to this change when curl w/WinSSL was used nothing would happen, the ~~default certificate bundle would still be used~~ Edit: The default certificate store in Windows would still be used is more correct to say. With this change however the curl tool would prefer the bundle instead. I think we should consider making it so that the curl tool only passes the CAINFO option in the case of schannel when the user has explicitly requested it, and not use autodetect, that way we could still preserve backwards compatibility. Thoughts?

jay · 2017-11-09T23:57:40Z

lib/config-win32.h

+ * option for Schannel.
+ */
+#if defined(_WIN32_WINNT) && (_WIN32_WINNT >= 0x0601)
+#define HAVE_EXCLUSIVE_TRUST_MODE 1


If possible I would like for us to handle this at runtime, it seems that in CERT_CHAIN_ENGINE_CONFIG the flag is available in the struct even in earlier versions though it doesn't do anything. Can't we do it like Curl_verify_windows_version(6, 1, PLATFORM_WINNT, VERSION_GREATER_THAN_EQUAL);

Interesting. Perhaps the CERT_CHAIN_ENGINE_CONFIG definition has changed in more recent versions of the Windows SDK?

Here's what the definition looks like in the 8.0 SDK:

typedef struct _CERT_CHAIN_ENGINE_CONFIG { DWORD cbSize; HCERTSTORE hRestrictedRoot; HCERTSTORE hRestrictedTrust; HCERTSTORE hRestrictedOther; DWORD cAdditionalStore; HCERTSTORE* rghAdditionalStore; DWORD dwFlags; DWORD dwUrlRetrievalTimeout; // milliseconds DWORD MaximumCachedCertificates; DWORD CycleDetectionModulus; #if (NTDDI_VERSION >= NTDDI_WIN7) HCERTSTORE hExclusiveRoot; HCERTSTORE hExclusiveTrustedPeople; #endif #if (NTDDI_VERSION >= NTDDI_WIN8) DWORD dwExclusiveFlags; #endif } CERT_CHAIN_ENGINE_CONFIG, *PCERT_CHAIN_ENGINE_CONFIG;

When I explicitly set _WIN32_WINNT to 0x0600 (Windows Server 2008), the hExclusiveRoot field is not present in the structure. Given this, I don't think this check can be done at runtime.

But, I do notice that other ifdef checks specific to Schannel are in the schannel.c file, for example, HAS_ALPN. Given that, I can move this check into schannel.c (or schannel_verify.c) instead of having it live in config-win32.h.

jay · 2017-11-09T23:59:02Z

lib/vtls/schannel.c

 };

 #define BACKEND connssl->backend

 static Curl_recv schannel_recv;
 static Curl_send schannel_send;

-#ifdef _WIN32_WCE
+#define MAX_CAFILE_SIZE 67108864 /* 64 MB */


this seems like a large size to read into memory for a certificate bundle. I have a full mozilla certificate bundle that's like 256k

Good point. Would a smaller value like 512k or 1M work? I'll go with 1M unless there are objections to that.

jay · 2017-11-10T00:00:21Z

lib/vtls/schannel.c

+      if(BACKEND->use_manual_cred_validation) {
+        schannel_cred.dwFlags = SCH_CRED_MANUAL_CRED_VALIDATION |
+                                SCH_CRED_IGNORE_NO_REVOCATION_CHECK |
+                                SCH_CRED_IGNORE_REVOCATION_OFFLINE;


this I'm not fond of, windows users have the expectation that revocation checks are enabled by default. wouldn't we want to do the same for the certificate bundle they supply for consistency? (I'm assuming here that is what will happen since manual cred is now used for the bundle)

Great catch! Looks like revocation checks were being unconditionally disabled for CE, regardless of the setting of the no_revoke flag.

Interestingly enough, it looks like the call to CertGetCertificateChain overrides these flags by setting the CERT_CHAIN_REVOCATION_CHECK_CHAIN flag in the flags for that function if the no_revoke flag is set.

That said, I'll still update this code to set dwFlags based on the no_revoke flag.

lib/vtls/schannel.c

jay · 2017-11-10T00:03:25Z

tests/data/Makefile.inc

@@ -55,7 +55,7 @@ test280 test281 test282 test283 test284 test285 test286 test287 test288 \
 test289 test290 test291 test292 test293 test294 test295 test296 test297 \
 test298 test299 test300 test301 test302 test303 test304 test305 test306 \
 test307 test308 test309 test310 test311 test312 test313 test314 test315 \
-                                test320 test321 test322 test323 test324 \
+test316 test317                 test320 test321 test322 test323 test324 \


i rebased this and renamed the tests to avoid conflicts 314 => 316 and 315 => 317

Cool, thanks!

- Move verify_certificate functionality in schannel.c into a new file called schannel_verify.c. Additionally, some structure defintions from schannel.c have been moved to schannel.h to allow them to be used in schannel_verify.c. - Make verify_certificate functionality for Schannel available on all versions of Windows instead of just Windows CE. verify_certificate will be invoked on Windows CE or when the user specifies CURLOPT_CAINFO and CURLOPT_SSL_VERIFYPEER. - In verify_certificate, create a custom certificate chain engine that exclusively trusts the certificate store backed by the CURLOPT_CAINFO file. - HAVE_EXCLUSIVE_TRUST_MODE macro defined in schannel.h for Windows 7+ to control enabling this feature that relies on Windows 7+ functionality. - doc updates of --cacert/CAINFO support for schannel - Use CERT_NAME_SEARCH_ALL_NAMES_FLAG when invoking CertGetNameString when available. This implements a TODO in schannel.c to improve handling of multiple SANs in a certificate. In particular, all SANs will now be searched instead of just the first name. - Update tool_operate.c to not search for the curl-ca-bundle.crt file when using Schannel to maintain backward compatibility. Previously, any curl-ca-bundle.crt file found in that search would have been ignored by Schannel. But, with CAINFO support, the file found by that search would have been used as the certificate store and could cause issues for any users that have curl-ca-bundle.crt in the search path. - Add new test cases 317 and 318. These test cases check that the first and last SAN, respectively, matches the connection hostname. New test certificates have been added for these cases. For 317, the certificate prefix is Server-localhost-firstSAN and for 318, the certificate prefix is Server-localhost-secondSAN. - Remove TODO 15.2 (Add support for custom server certificate validation), this commit addresses it. Closes curl#1325

mcnulty · 2017-12-29T21:05:43Z

Thanks for your review @jay! I think I've addressed all of your comments with my most recent push.

I think we should consider making it so that the curl tool only passes the CAINFO option in the case of schannel when the user has explicitly requested it, and not use autodetect, that way we could still preserve backwards compatibility. Thoughts?

I added some code to tool_operate.c in the operate_do function to skip the autodetect search if the SSL backend is Schannel. I used the CURLINFO_TLS_SSL_PTR option for curl_easy_getinfo to determine the SSL backend in use. I'm not sure if this was the best way to determine the SSL backend, but it seemed to do the trick.

- Move verify_certificate functionality in schannel.c into a new file called schannel_verify.c. Additionally, some structure defintions from schannel.c have been moved to schannel.h to allow them to be used in schannel_verify.c. - Make verify_certificate functionality for Schannel available on all versions of Windows instead of just Windows CE. verify_certificate will be invoked on Windows CE or when the user specifies CURLOPT_CAINFO and CURLOPT_SSL_VERIFYPEER. - In verify_certificate, create a custom certificate chain engine that exclusively trusts the certificate store backed by the CURLOPT_CAINFO file. - HAVE_EXCLUSIVE_TRUST_MODE macro defined in schannel.h for Windows 7+ to control enabling this feature that relies on Windows 7+ functionality. - doc updates of --cacert/CAINFO support for schannel - Use CERT_NAME_SEARCH_ALL_NAMES_FLAG when invoking CertGetNameString when available. This implements a TODO in schannel.c to improve handling of multiple SANs in a certificate. In particular, all SANs will now be searched instead of just the first name. - Update tool_operate.c to not search for the curl-ca-bundle.crt file when using Schannel to maintain backward compatibility. Previously, any curl-ca-bundle.crt file found in that search would have been ignored by Schannel. But, with CAINFO support, the file found by that search would have been used as the certificate store and could cause issues for any users that have curl-ca-bundle.crt in the search path. - Add new test cases 3000 and 3001. These test cases check that the first and last SAN, respectively, matches the connection hostname. New test certificates have been added for these cases. For 3000, the certificate prefix is Server-localhost-firstSAN and for 3001, the certificate prefix is Server-localhost-secondSAN. - Remove TODO 15.2 (Add support for custom server certificate validation), this commit addresses it. Closes curl#1325

mcnulty · 2018-03-29T14:24:29Z

I went ahead and rebased this PR on master. Any chance these changes could be considered for inclusion in 7.60.0?

- Move verify_certificate functionality in schannel.c into a new file called schannel_verify.c. Additionally, some structure defintions from schannel.c have been moved to schannel.h to allow them to be used in schannel_verify.c. - Make verify_certificate functionality for Schannel available on all versions of Windows instead of just Windows CE. verify_certificate will be invoked on Windows CE or when the user specifies CURLOPT_CAINFO and CURLOPT_SSL_VERIFYPEER. - In verify_certificate, create a custom certificate chain engine that exclusively trusts the certificate store backed by the CURLOPT_CAINFO file. - HAVE_EXCLUSIVE_TRUST_MODE macro defined in schannel.h for Windows 7+ to control enabling this feature that relies on Windows 7+ functionality. - doc updates of --cacert/CAINFO support for schannel - Use CERT_NAME_SEARCH_ALL_NAMES_FLAG when invoking CertGetNameString when available. This implements a TODO in schannel.c to improve handling of multiple SANs in a certificate. In particular, all SANs will now be searched instead of just the first name. - Update tool_operate.c to not search for the curl-ca-bundle.crt file when using Schannel to maintain backward compatibility. Previously, any curl-ca-bundle.crt file found in that search would have been ignored by Schannel. But, with CAINFO support, the file found by that search would have been used as the certificate store and could cause issues for any users that have curl-ca-bundle.crt in the search path. - Add new test cases 3000 and 3001. These test cases check that the first and last SAN, respectively, matches the connection hostname. New test certificates have been added for these cases. For 3000, the certificate prefix is Server-localhost-firstSAN and for 3001, the certificate prefix is Server-localhost-secondSAN. - Remove TODO 15.2 (Add support for custom server certificate validation), this commit addresses it. Closes curl#1325

jay · 2018-03-30T07:14:45Z

Thanks Dan I will review again when I can.

edit 4-16 I will review this week before the feature window closes,

jay · 2018-04-18T08:13:23Z

Thanks. I changed the OS check from a build time check to a runtime check since it's common to build libcurl targeting an OS earlier than Windows 7, and yet we would still like this ability on Windows 7+ for those builds.

bagder requested review from mback2k and jay April 4, 2017 21:59

mback2k requested changes Apr 23, 2017

View reviewed changes

mback2k mentioned this pull request Apr 23, 2017

SChannel/WinSSL: Implement public key pinning #1429

Closed

mcnulty force-pushed the schannel-cainfo branch from 219aa61 to 966583e Compare July 7, 2017 19:05

mcnulty force-pushed the schannel-cainfo branch from 966583e to 9cb1985 Compare July 7, 2017 23:16

bagder self-requested a review August 27, 2017 18:17

mcnulty force-pushed the schannel-cainfo branch 2 times, most recently from 84ba86f to 884c1cf Compare October 15, 2017 00:49

jay force-pushed the schannel-cainfo branch from 884c1cf to 228f50c Compare November 9, 2017 23:53

jay requested changes Nov 10, 2017

View reviewed changes

mcnulty force-pushed the schannel-cainfo branch from 228f50c to e083939 Compare December 29, 2017 17:19

mcnulty force-pushed the schannel-cainfo branch from e083939 to a8a2c14 Compare December 29, 2017 18:59

mback2k approved these changes Dec 30, 2017

View reviewed changes

mcnulty force-pushed the schannel-cainfo branch from a8a2c14 to 46831c5 Compare March 28, 2018 15:19

mcnulty force-pushed the schannel-cainfo branch from 46831c5 to 53b61cb Compare March 29, 2018 14:28

jay closed this in 8996300 Apr 18, 2018

mcnulty deleted the schannel-cainfo branch April 18, 2018 14:59

niik mentioned this pull request May 30, 2018

After restarting my client following a desktop client update prompt, I can no longer Fetch Origin desktop/desktop#4817

Closed

whoisj mentioned this pull request Jun 8, 2018

New curl behaviour favours http.sslCAInfo config (if set) when using SChannel backend git-for-windows/git#1698

Closed

1 task

lock bot locked as resolved and limited conversation to collaborators Jul 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schannel: add support for CURLOPT_CAINFO. #1325

schannel: add support for CURLOPT_CAINFO. #1325

mcnulty commented Mar 10, 2017

mention-bot commented Mar 10, 2017

bagder commented Mar 16, 2017

mback2k left a comment •

edited

mcnulty commented Apr 26, 2017

mcnulty commented Jul 5, 2017

bagder commented Jul 5, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

dscho commented Aug 28, 2017

mcnulty commented Oct 15, 2017

dscho commented Oct 16, 2017

mcnulty commented Oct 16, 2017

jay left a comment •

edited

jay Nov 9, 2017

mcnulty Dec 28, 2017

jay Nov 9, 2017

mcnulty Dec 28, 2017

jay Nov 10, 2017

mcnulty Dec 28, 2017

jay Nov 10, 2017

mcnulty Dec 28, 2017

mcnulty commented Dec 29, 2017

mcnulty commented Mar 29, 2018

jay commented Mar 30, 2018 •

edited

jay commented Apr 18, 2018

schannel: add support for CURLOPT_CAINFO. #1325

schannel: add support for CURLOPT_CAINFO. #1325

Conversation

mcnulty commented Mar 10, 2017

mention-bot commented Mar 10, 2017

bagder commented Mar 16, 2017

mback2k left a comment • edited

Choose a reason for hiding this comment

mcnulty commented Apr 26, 2017

mcnulty commented Jul 5, 2017

bagder commented Jul 5, 2017

coveralls commented Jul 7, 2017

coveralls commented Jul 7, 2017

dscho commented Aug 28, 2017

mcnulty commented Oct 15, 2017

dscho commented Oct 16, 2017

mcnulty commented Oct 16, 2017

jay left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcnulty commented Dec 29, 2017

mcnulty commented Mar 29, 2018

jay commented Mar 30, 2018 • edited

jay commented Apr 18, 2018

mback2k left a comment •

edited

jay left a comment •

edited

jay commented Mar 30, 2018 •

edited