schannel: CA file and memory blob cache #12261

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

lib/vtls/schannel.c

+                                          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

lib/vtls/schannel.c

+  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

lib/vtls/schannel.c

@@ -2754,6 +2754,148 @@ static void *schannel_get_internals(struct ssl_connect_data *connssl,
  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

lib/vtls/schannel.c

+    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

lib/vtls/schannel.c

+    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

lib/vtls/schannel.c

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

lib/vtls/schannel.c

+    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

lib/vtls/schannel.c

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

lib/vtls/schannel.c

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

lib/vtls/schannel.c

+  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

lib/vtls/schannel_verify.c

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

lib/vtls/schannel.c

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

lib/vtls/schannel.c Outdated Show resolved Hide resolved

jay reviewed Nov 5, 2023

View reviewed changes

consolidate conditions

1fc861f

jay reviewed Nov 5, 2023

View reviewed changes

lib/vtls/schannel.c Show resolved Hide resolved

obey zero cache timeout rule

28746bc

jay closed this in 1af46f2 Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schannel: CA file and memory blob cache #12261

schannel: CA file and memory blob cache #12261

ajax16384 commented Nov 3, 2023

bagder Nov 3, 2023 •

edited

ajax16384 Nov 3, 2023

bagder Nov 4, 2023

ajax16384 Nov 5, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder Nov 3, 2023

ajax16384 Nov 3, 2023

bagder commented Nov 4, 2023

ajax16384 commented Nov 5, 2023

jay Nov 5, 2023

ajax16384 Nov 5, 2023

jay Nov 6, 2023

jay Nov 5, 2023

ajax16384 Nov 5, 2023

jay Nov 5, 2023

jay Nov 5, 2023 •

edited

ajax16384 Nov 6, 2023

jay commented 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);
	}
	}

schannel: CA file and memory blob cache #12261

schannel: CA file and memory blob cache #12261

Conversation

ajax16384 commented Nov 3, 2023

bagder Nov 3, 2023 • 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

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

bagder commented Nov 4, 2023

ajax16384 commented Nov 5, 2023

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

jay Nov 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jay commented Nov 11, 2023

bagder Nov 3, 2023 •

edited

jay Nov 5, 2023 •

edited