schannel: CA file and memory blob cache by ajax16384 · Pull Request #12261 · curl/curl

ajax16384 · 2023-11-03T15:29:25Z

add schannel CA file and memory blob cache support
add schannel CURLOPT_CA_CACHE_TIMEOUT support

add schannel CURLOPT_CA_CACHE_TIMEOUT support

bagder · 2023-11-03T15:56:49Z

+                                          const struct Curl_easy *data)
+{
+  struct Curl_multi *multi = data->multi_easy ? data->multi_easy : data->multi;
+


Suggested change

DEBUGASSERT(multi);

DEBUGASSERT(multi->ssl_backend_data);

As from what I understand something is terribly wrong if either one is NULL here.

used same pattern from

curl/lib/vtls/openssl.c

Line 3377 in 3e6254f

if(multi &&

as it checked multi for null without assert.

bagder · 2023-11-03T16:00:31Z

+  return mbackend->cert_store;
+}
+
+bool schannel_set_cached_cert_store(struct Curl_cfilter *cf,


This is not a static function, then it needs to be prefixed with Curl_.

bagder · 2023-11-03T16:00:57Z

  return &backend->ctxt->ctxt_handle;
 }

+HCERTSTORE schannel_get_cached_cert_store(struct Curl_cfilter *cf,


As this is not a static function, then it needs to be prefixed with Curl_.

bagder · 2023-11-03T16:02:04Z

+    return NULL;
+  }
+
+  struct schannel_multi_ssl_backend_data *mbackend;


We write C89 code, so we prefer the variable declarations at the top of every code block. Even for this code, I presume there is no compiler that is going to warn about it in this windows specific section.

bagder · 2023-11-03T16:03:50Z

+    if(memcmp(mbackend->CAinfo_blob_digest,
+              info_blob_digest,
+              CURL_SHA256_DIGEST_LENGTH)) {
+        return NULL;


What is the purpose of this check?

memcmp compares hash of previous mem block with current, if it's not equal then cert_store is invalid

bagder · 2023-11-03T16:04:39Z

+{
+  struct Curl_multi *multi = data->multi_easy ? data->multi_easy : data->multi;
+
+  if(!multi) {


I propose a DEBUGASSERT() here as well for the same reason. This should not be possible to happen.

bagder · 2023-11-03T16:05:58Z

+    if(conn_config->CAfile) {
+      CAfile = strdup(conn_config->CAfile);
+      if(!CAfile) {
+        return false;


In this and the other early returns, do they not leak memory allocated previously in this function?

all possible allocations will be freed at backend free method (free_multi_ssl_backend_data)

consolidate var declaration at block start add DEBUGASSERT

... so add the asserts now and consider removing the dynamic checks in a future. Ref: #12261

... so add the asserts now and consider removing the dynamic checks in a future. Ref: #12261 Closes #12264

bagder · 2023-11-04T22:24:43Z

warnings

C:\projects\curl\lib\vtls\schannel.c(2802,50): warning C4057: 'function': 'const unsigned char *' differs in indirection to slightly different base types from 'const char *' [C:\projects\curl\lib\libcurl_object.vcxproj]
C:\projects\curl\lib\vtls\schannel.c(2861,50): warning C4057: 'function': 'const unsigned char *' differs in indirection to slightly different base types from 'const char *' [C:\projects\curl\lib\libcurl_object.vcxproj]

ajax16384 · 2023-11-05T14:23:25Z

warnings

C:\projects\curl\lib\vtls\schannel.c(2802,50): warning C4057: 'function': 'const unsigned char *' differs in indirection to slightly different base types from 'const char *' [C:\projects\curl\lib\libcurl_object.vcxproj]
C:\projects\curl\lib\vtls\schannel.c(2861,50): warning C4057: 'function': 'const unsigned char *' differs in indirection to slightly different base types from 'const char *' [C:\projects\curl\lib\libcurl_object.vcxproj]

thanks, fixed

jay · 2023-11-05T19:54:12Z

+    schannel_sha256sum((const unsigned char *)ca_info_blob->data,
+                       ca_info_blob->len,
+                       CAinfo_blob_digest,
+                       CURL_SHA256_DIGEST_LENGTH);


is this really faster than storing the original blob and comparing?

blob might be memory expensive
(like https://curl.se/docs/caextract.html - 200KB)

In a release configuration in an average of over 100 runs a memcmp of the 200k memory takes 24us and a sha256sum comparison takes 3200us (1600us x 2). That is basically 3 ms difference between the two. I did not account for the memdup time in the first case though I forgot about that. So first case might actually be slower because of crt locks needed to copy the memory. My conclusion is it's not worth copying the entire blob.

jay · 2023-11-05T21:02:26Z

+  free(mbackend->CAfile);
+
+  mbackend->time = Curl_now();
+  mbackend->cert_store = cert_store;


I notice that unlike openssl code there's no increase to a reference count when this is added

Here(schannnel*.c) HCERTSTORE resource use non refcount contructor/destrctor pattern. Just after successfull Curl_schannel_set_cached_cert_store call from Curl_verify_certificate - cache record became this resource owner. Lifetime of cert_store do not exceed get/store* methods.

@tlsa do you happen to remember why you used ref counting for the openssl store

jay · 2023-11-05T22:50:59Z

-              Curl_winapi_strerror(GetLastError(), buffer, sizeof(buffer)));
-        result = CURLE_SSL_CACERT_BADFILE;
+      /* try cache */
+      trust_store = Curl_schannel_get_cached_cert_store(cf, data);


openssl get/set of cert store has more conditionals, see cache_criteria_met. it looks like most? of those don't apply here but what about data->set.general_ssl.ca_cache_timeout

curl/lib/vtls/openssl.c

Lines 3439 to 3460 in 3e6254f

/* Consider the X509 store cacheable if it comes exclusively from a CAfile,

or no source is provided and we are falling back to openssl's built-in

default. */

cache_criteria_met = (data->set.general_ssl.ca_cache_timeout != 0) &&

conn_config->verifypeer &&

!conn_config->CApath &&

!conn_config->ca_info_blob &&

!ssl_config->primary.CRLfile &&

!ssl_config->native_ca_store;

cached_store = get_cached_x509_store(cf, data);

if(cached_store && cache_criteria_met && X509_STORE_up_ref(cached_store)) {

SSL_CTX_set_cert_store(ssl_ctx, cached_store);

}

else {

X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx);

result = populate_x509_store(cf, data, store);

if(result == CURLE_OK && cache_criteria_met) {

set_cached_x509_store(cf, data, store);

}

}

Thanks. Zero cache timeout should skip cache usage at all. Will fix with next commit.

jay · 2023-11-11T09:15:47Z

Thanks

add schannel CA file and memory blob cache support

98b5ade

add schannel CURLOPT_CA_CACHE_TIMEOUT support

github-actions Bot added TLS Windows Windows-specific labels Nov 3, 2023

ajax16384 added 4 commits November 3, 2023 18:36

fix comments style

48fa0da

fix code style

6c22c9e

fix code style

6a6ffe9

fix code style

cf1e13d

bagder reviewed Nov 3, 2023

View reviewed changes

add Curl_ prefix for non static function

6dbb5ab

consolidate var declaration at block start add DEBUGASSERT

bagder added a commit that referenced this pull request Nov 3, 2023

openssl: two multi pointer checks should probably rather be asserts

30abe31

... so add the asserts now and consider removing the dynamic checks in a future. Ref: #12261

bagder mentioned this pull request Nov 3, 2023

openssl: two multi pointer checks should probably rather be asserts #12264

Closed

bagder added a commit that referenced this pull request Nov 4, 2023

openssl: two multi pointer checks should probably rather be asserts

b3c4bbf

... so add the asserts now and consider removing the dynamic checks in a future. Ref: #12261 Closes #12264

fix cast warnings

bb92931

fix ISO C90 error

088613c

jay reviewed Nov 5, 2023

View reviewed changes

Comment thread lib/vtls/schannel.c Outdated

jay reviewed Nov 5, 2023

View reviewed changes

consolidate conditions

1fc861f

jay reviewed Nov 5, 2023

View reviewed changes

Comment thread lib/vtls/schannel.c

obey zero cache timeout rule

28746bc

jay closed this in 1af46f2 Nov 11, 2023

	/* Consider the X509 store cacheable if it comes exclusively from a CAfile,
	or no source is provided and we are falling back to openssl's built-in
	default. */
	cache_criteria_met = (data->set.general_ssl.ca_cache_timeout != 0) &&
	conn_config->verifypeer &&
	!conn_config->CApath &&
	!conn_config->ca_info_blob &&
	!ssl_config->primary.CRLfile &&
	!ssl_config->native_ca_store;

	cached_store = get_cached_x509_store(cf, data);
	if(cached_store && cache_criteria_met && X509_STORE_up_ref(cached_store)) {
	SSL_CTX_set_cert_store(ssl_ctx, cached_store);
	}
	else {
	X509_STORE *store = SSL_CTX_get_cert_store(ssl_ctx);

	result = populate_x509_store(cf, data, store);
	if(result == CURLE_OK && cache_criteria_met) {
	set_cached_x509_store(cf, data, store);
	}
	}

Uh oh!

Conversation

ajax16384 commented Nov 3, 2023

Uh oh!

bagder Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagder commented Nov 4, 2023

Uh oh!

ajax16384 commented Nov 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jay Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jay commented Nov 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

bagder Nov 3, 2023 •

edited

Loading

jay Nov 5, 2023 •

edited

Loading