schannel: add support for CURLOPT_CAINFO. #1325

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@mcnulty
Contributor

mcnulty commented Mar 10, 2017

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

This comment has been minimized.

Show comment Hide comment
@mention-bot

mention-bot Mar 10, 2017

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

@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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Mar 16, 2017

Member

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

Member

bagder commented Mar 16, 2017

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

@bagder bagder requested review from mback2k and jay Apr 4, 2017

@mback2k

mback2k requested changes Apr 23, 2017 edited

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
-static CURLcode verify_certificate(struct connectdata *conn, int sockindex);
+/* certificate validation on CE doesn't seem to work right; we'll
+ * do it following a more manual process. */
+static const int DEFAULT_MANUAL_CRED_VALIDATION = 1;

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Please use a #define instead of a static const int variable that is not really needed in my opinion.

@mback2k

mback2k Apr 23, 2017

Member

Please use a #define instead of a static const int variable that is not really needed in my opinion.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Per your comment to use a variable to store the result of checking the conditions that enable manual certificate validation, I went ahead and removed this variable since it was not needed anymore.

@mcnulty

mcnulty May 1, 2017

Contributor

Per your comment to use a variable to store the result of checking the conditions that enable manual certificate validation, I went ahead and removed this variable since it was not needed anymore.

lib/vtls/schannel.c
+ * do it following a more manual process. */
+static const int DEFAULT_MANUAL_CRED_VALIDATION = 1;
+#else
+static const int DEFAULT_MANUAL_CRED_VALIDATION = 0;

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

lib/vtls/schannel.c
+#endif
+
+#ifdef HAVE_EXCLUSIVE_TRUST_MODE
+static const int CAFILE_SUPPORTED = 1;

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

lib/vtls/schannel.c
+#ifdef HAVE_EXCLUSIVE_TRUST_MODE
+static const int CAFILE_SUPPORTED = 1;
+#else
+static const int CAFILE_SUPPORTED = 0;

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #define instead.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

@mcnulty

mcnulty May 1, 2017

Contributor

Per reply above, this variable has been removed.

lib/vtls/schannel.c
#endif
+static const int MAX_CAFILE_SIZE = 67108864; /* 64 MB */
+static const char * const BEGIN_CERT = "-----BEGIN CERTIFICATE-----\n";
+static const char * const END_CERT = "\n-----END CERTIFICATE-----";

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #defines instead.

@mback2k

mback2k Apr 23, 2017

Member

Same as above, please use a compile-time #defines instead.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

lib/vtls/schannel.c
+ else if(result != CURLE_OK) {
+ failf(data, "schannel: server certificate name verification failed");
+ }
+ }while(0);

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Same as before. I think you are doing this to break out and go to the cleanup part of the function. But in this case I would actually recommend to use goto, since this is used in libcurl for such purposes and actually describes what you are doing.

@mback2k

mback2k Apr 23, 2017

Member

Same as before. I think you are doing this to break out and go to the cleanup part of the function. But in this case I would actually recommend to use goto, since this is used in libcurl for such purposes and actually describes what you are doing.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

This has been changed to use a goto instead.

@mcnulty

mcnulty May 1, 2017

Contributor

This has been changed to use a goto instead.

lib/vtls/schannel.c
+ 0);
+ if(len == 0) {
+ failf(data,
+ "schannel: CertGetNameString() returned no "

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Please fix the indentation here, thanks.

@mback2k

mback2k Apr 23, 2017

Member

Please fix the indentation here, thanks.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

lib/vtls/schannel.c
+ match_result = Curl_cert_hostcheck(cert_hostname, conn->host.name);
+ if(match_result == CURL_HOST_MATCH) {
+ infof(data,
+ "schannel: connection hostname (%s) validated "

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Please fix the indentation here, thanks.

@mback2k

mback2k Apr 23, 2017

Member

Please fix the indentation here, thanks.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

lib/vtls/schannel.c
+ cert_hostname);
+ result = CURLE_OK;
+ }
+ else{

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

As before, add a space please.

@mback2k

mback2k Apr 23, 2017

Member

As before, add a space please.

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

@mcnulty

mcnulty May 1, 2017

Contributor

Fixed.

lib/vtls/schannel.c
- if((status != SEC_E_OK) || (pCertContextServer == NULL)) {
- failf(data, "schannel: Failed to read remote certificate context: %s",
+ if((status != SEC_E_OK) || !pCertContextServer) {
+ failf(data, "schannel: failed to read remote certificate context: %s",

This comment has been minimized.

Show comment Hide comment
@mback2k

mback2k Apr 23, 2017

Member

Please keep changes irrelevant for this new feature in a separate commit. As far as I can tell this is just a code-style change, right?

@mback2k

mback2k Apr 23, 2017

Member

Please keep changes irrelevant for this new feature in a separate commit. As far as I can tell this is just a code-style change, right?

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty May 1, 2017

Contributor

Good catch, this change has been reverted.

@mcnulty

mcnulty May 1, 2017

Contributor

Good catch, this change has been reverted.

@mcnulty

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Apr 26, 2017

Contributor

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

Contributor

mcnulty commented Apr 26, 2017

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

@mcnulty

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Jul 5, 2017

Contributor

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?

Contributor

mcnulty commented Jul 5, 2017

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

This comment has been minimized.

Show comment Hide comment
@bagder

bagder Jul 5, 2017

Member

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)

Member

bagder commented Jul 5, 2017

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

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Jul 7, 2017

Coverage Status

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

Coverage Status

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

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Jul 7, 2017

Coverage Status

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

Coverage Status

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

@bagder bagder self-requested a review Aug 27, 2017

@dscho

This comment has been minimized.

Show comment Hide comment
@dscho

dscho Aug 28, 2017

Contributor

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

Contributor

dscho commented Aug 28, 2017

@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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Oct 15, 2017

Contributor

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?

Contributor

mcnulty commented Oct 15, 2017

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

This comment has been minimized.

Show comment Hide comment
@dscho

dscho Oct 16, 2017

Contributor

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!

Contributor

dscho commented Oct 16, 2017

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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Oct 16, 2017

Contributor

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:

Contributor

mcnulty commented Oct 16, 2017

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:

jay added a commit to mcnulty/curl that referenced this pull request Nov 9, 2017

schannel: add support for CURLOPT_CAINFO
- 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

jay requested changes Nov 10, 2017 edited

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?

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

This comment has been minimized.

Show comment Hide comment
@jay

jay Nov 10, 2017

Member

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

@jay

jay Nov 10, 2017

Member

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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Dec 28, 2017

Contributor

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.

@mcnulty

mcnulty Dec 28, 2017

Contributor

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.

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 comment has been minimized.

Show comment Hide comment
@jay

jay Nov 10, 2017

Member

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

@jay

jay Nov 10, 2017

Member

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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Dec 28, 2017

Contributor

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

@mcnulty

mcnulty Dec 28, 2017

Contributor

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

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 comment has been minimized.

Show comment Hide comment
@jay

jay Nov 10, 2017

Member

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)

@jay

jay Nov 10, 2017

Member

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)

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Dec 28, 2017

Contributor

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.

@mcnulty

mcnulty Dec 28, 2017

Contributor

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
@@ -1574,7 +1594,332 @@ CURLcode Curl_schannel_random(unsigned char *entropy, size_t length)
return CURLE_OK;
}
-#ifdef _WIN32_WCE
+#ifdef HAVE_EXCLUSIVE_TRUST_MODE
+static CURLcode add_certs_to_store(HCERTSTORE trust_store,

This comment has been minimized.

Show comment Hide comment
@jay

jay Nov 10, 2017

Member

I will echo this , I think the verify stuff could be broken away schannel_verify/cert/whatever.c like how polarssl threadlock stuff went in polarssl_threadlock.c instead

@jay

jay Nov 10, 2017

Member

I will echo this , I think the verify stuff could be broken away schannel_verify/cert/whatever.c like how polarssl threadlock stuff went in polarssl_threadlock.c instead

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 \

This comment has been minimized.

Show comment Hide comment
@jay

jay Nov 10, 2017

Member

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

@jay

jay Nov 10, 2017

Member

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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Dec 28, 2017

Contributor

Cool, thanks!

@mcnulty

mcnulty Dec 28, 2017

Contributor

Cool, thanks!

mcnulty added a commit to mcnulty/curl that referenced this pull request Dec 29, 2017

schannel: add support for CURLOPT_CAINFO
- 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 added a commit to mcnulty/curl that referenced this pull request Dec 29, 2017

schannel: add support for CURLOPT_CAINFO
- 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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Dec 29, 2017

Contributor

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.

Contributor

mcnulty commented Dec 29, 2017

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.

mcnulty added a commit to mcnulty/curl that referenced this pull request Mar 28, 2018

schannel: add support for CURLOPT_CAINFO
- 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

This comment has been minimized.

Show comment Hide comment
@mcnulty

mcnulty Mar 29, 2018

Contributor

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

Contributor

mcnulty commented Mar 29, 2018

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

schannel: add support for CURLOPT_CAINFO
- 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 #1325
@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Mar 30, 2018

Member

Thanks Dan I will review again when I can.

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

Member

jay commented Mar 30, 2018

Thanks Dan I will review again when I can.

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

@jay jay closed this in 8996300 Apr 18, 2018

@jay

This comment has been minimized.

Show comment Hide comment
@jay

jay Apr 18, 2018

Member

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.

Member

jay commented Apr 18, 2018

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.

@mcnulty mcnulty deleted the mcnulty:schannel-cainfo branch Apr 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment