From 4ee2c7e13018da1f0c339ceaa38a95bb19d4f761 Mon Sep 17 00:00:00 2001 From: Shreemaan Abhishek Date: Tue, 4 Aug 2026 11:04:43 +0000 Subject: [PATCH 1/3] fix: stop rescanning the CA directory on every TLS handshake ngx_http_ffi_client_ssl_ctx() called SSL_CTX_set_default_verify_paths() unconditionally. That registers the hashed CA directory as X509_FILETYPE_DEFAULT, and with DEFAULT OpenSSL enumerates the whole directory on *each* handshake instead of opening the single .N file the lookup needs. On a connection-per-request TLS workload the target worker spent 11.8% of its CPU in ext4 directory code against lua-resty-http's 0.4%, and 36.7% in the kernel against 17.9%. Under load that was one openat plus two getdents64 per request, sweeping 245 entries each time; lua-resty-http issued none. Reproduced independently of this module, against the benchmark upstream: default_verify_paths getdents64=4 explicit -CAfile bundle getdents64=0 explicit -CApath dir getdents64=0 Explicit CApath still verifies correctly when the issuer is present, so the filetype is what costs, not hash-dir lookup and not lookup failure. Two changes. The trust store is now installed by hand as X509_FILETYPE_PEM, honouring SSL_CERT_FILE/SSL_CERT_DIR the way OpenSSL does, so a verifying caller no longer pays the sweep either. And it is not installed at all unless the caller asked to verify: OpenSSL checks the chain during every handshake regardless of verify mode, but the handshake handler only reads the result when ssl_verify is set, so for everyone else the whole store was wasted work. The context cache is keyed on the verify flag as well as the CA path, since a verify-off context carries no store and would fail every chain a verify-on request handed it. ffi.tlsshort goes from 932 to 1257 req/s, median of 5 saturated repeats, which turns a 0.81x result against lua-resty-http into 1.10x. Pooled TLS is unchanged at 1.53x. The benchmark harness these numbers come from, and the tlsverify and tlsverifyshort cases that cover the verifying path, are in a companion PR; this commit touches only src/. --- src/ngx_http_ffi_client.h | 8 +- src/ngx_http_ffi_client_request.c | 137 ++++++++++++++++++++++++------ 2 files changed, 120 insertions(+), 25 deletions(-) diff --git a/src/ngx_http_ffi_client.h b/src/ngx_http_ffi_client.h index dc4e256..c62e48a 100644 --- a/src/ngx_http_ffi_client.h +++ b/src/ngx_http_ffi_client.h @@ -199,12 +199,18 @@ typedef struct ngx_http_ffi_client_keepalive_item_s #if (NGX_SSL) -/* one cached client SSL_CTX per distinct trust config (trusted CA path) */ +/* + * One cached client SSL_CTX per distinct trust config. The key is the trusted + * CA path *and* whether verification is on: a verify-off context carries no + * trust store at all, so reusing one for a verify-on request would fail every + * chain it was asked to check. + */ typedef struct ngx_http_ffi_client_ssl_ctx_s ngx_http_ffi_client_ssl_ctx_t; struct ngx_http_ffi_client_ssl_ctx_s { ngx_http_ffi_client_ssl_ctx_t *next; ngx_str_t ca; + unsigned verify:1; ngx_ssl_t *ssl; }; #endif diff --git a/src/ngx_http_ffi_client_request.c b/src/ngx_http_ffi_client_request.c index 0ee73ab..7088b4f 100644 --- a/src/ngx_http_ffi_client_request.c +++ b/src/ngx_http_ffi_client_request.c @@ -60,7 +60,10 @@ static ngx_int_t ngx_http_ffi_client_handle_read_wait( ngx_http_ffi_client_op_t *op); #if (NGX_SSL) static ngx_ssl_t *ngx_http_ffi_client_ssl_ctx( - ngx_http_ffi_client_main_conf_t *fmcf, ngx_str_t *ca, ngx_log_t *log); + ngx_http_ffi_client_main_conf_t *fmcf, ngx_str_t *ca, ngx_uint_t verify, + ngx_log_t *log); +static ngx_int_t ngx_http_ffi_client_ssl_default_store(X509_STORE *store, + ngx_log_t *log); static ngx_int_t ngx_http_ffi_client_ssl_init_connection( ngx_http_ffi_client_op_t *op); static ngx_uint_t ngx_http_ffi_client_name_is_ip(ngx_str_t *name); @@ -1537,14 +1540,91 @@ ngx_http_ffi_client_handle_read_wait(ngx_http_ffi_client_op_t *op) #if (NGX_SSL) +/* + * Load the platform trust store into `store`. + * + * This is what SSL_CTX_set_default_verify_paths() would do, except that it + * registers both locations as X509_FILETYPE_PEM rather than + * X509_FILETYPE_DEFAULT. The filetype is not cosmetic: with DEFAULT, OpenSSL + * enumerates the whole hashed CA directory (a getdents64 sweep of every entry + * in /etc/ssl/certs) on *each* handshake, instead of opening the single + * .N file the lookup actually needs. On a fresh-connection-per-request + * workload that directory walk cost ~11% of the worker's CPU. + * + * The SSL_CERT_FILE / SSL_CERT_DIR environment overrides are honoured the same + * way OpenSSL honours them, so behaviour is unchanged apart from the cost. + * + * Returns NGX_OK if either location loaded; a trust store with neither is + * unusable and would fail every verify. + */ +static ngx_int_t +ngx_http_ffi_client_ssl_default_store(X509_STORE *store, ngx_log_t *log) +{ + const char *file, *dir; + ngx_uint_t loaded; + X509_LOOKUP *lookup; + + loaded = 0; + + file = getenv(X509_get_default_cert_file_env()); + if (file == NULL) { + file = X509_get_default_cert_file(); + } + + dir = getenv(X509_get_default_cert_dir_env()); + if (dir == NULL) { + dir = X509_get_default_cert_dir(); + } + + lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()); + if (lookup == NULL) { + return NGX_ERROR; + } + + if (X509_LOOKUP_load_file(lookup, file, X509_FILETYPE_PEM) == 1) { + loaded = 1; + + } else { + /* not fatal on its own: the hashed directory may still cover it */ + ERR_clear_error(); + } + + lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir()); + if (lookup == NULL) { + return NGX_ERROR; + } + + if (X509_LOOKUP_add_dir(lookup, dir, X509_FILETYPE_PEM) == 1) { + loaded = 1; + + } else { + ERR_clear_error(); + } + + if (!loaded) { + ngx_ssl_error(NGX_LOG_ALERT, log, 0, + "failed to load the default trust store from \"%s\"" + " or \"%s\"", file, dir); + return NGX_ERROR; + } + + return NGX_OK; +} + + /* * Lazily build one client SSL_CTX per distinct trust config, cached on * main_conf. `ca` is an optional trusted-CA file path (empty = system store - * only). Returns NULL on failure without caching a half-built context. + * only). `verify` says whether this context will ever be asked to check a + * chain; when it is off no trust store is loaded at all, because OpenSSL + * verifies the chain during every handshake regardless of the verify mode and + * ngx_http_ffi_client_ssl_handshake_handler() only reads the result when the + * caller asked for it. Returns NULL on failure without caching a half-built + * context. */ static ngx_ssl_t * ngx_http_ffi_client_ssl_ctx(ngx_http_ffi_client_main_conf_t *fmcf, - ngx_str_t *ca, ngx_log_t *log) + ngx_str_t *ca, ngx_uint_t verify, ngx_log_t *log) { u_char *path; ngx_ssl_t *ssl; @@ -1553,7 +1633,8 @@ ngx_http_ffi_client_ssl_ctx(ngx_http_ffi_client_main_conf_t *fmcf, for (node = fmcf->ssl_ctxs; node != NULL; node = node->next) { /* guard memcmp: NULL pointers with len 0 are UB */ - if (node->ca.len == ca->len + if (node->verify == (verify ? 1u : 0u) + && node->ca.len == ca->len && (ca->len == 0 || ngx_memcmp(node->ca.data, ca->data, ca->len) == 0)) { @@ -1575,32 +1656,39 @@ ngx_http_ffi_client_ssl_ctx(ngx_http_ffi_client_main_conf_t *fmcf, /* verify mode stays NONE so the handshake completes and we inspect the * result ourselves. An unusable trust store is a hard init failure: don't * cache a context that would fail every verify-on request. */ - if (SSL_CTX_set_default_verify_paths(ssl->ctx) == 0) { - ngx_ssl_error(NGX_LOG_ALERT, log, 0, - "SSL_CTX_set_default_verify_paths() failed"); - ngx_ssl_cleanup_ctx(ssl); - return NULL; - } - - if (ca->len) { - path = ngx_pnalloc(ngx_cycle->pool, ca->len + 1); - if (path == NULL) { + if (verify) { + if (ngx_http_ffi_client_ssl_default_store( + SSL_CTX_get_cert_store(ssl->ctx), log) != NGX_OK) + { ngx_ssl_cleanup_ctx(ssl); return NULL; } - ngx_memcpy(path, ca->data, ca->len); - path[ca->len] = '\0'; + if (ca->len) { + path = ngx_pnalloc(ngx_cycle->pool, ca->len + 1); + if (path == NULL) { + ngx_ssl_cleanup_ctx(ssl); + return NULL; + } - if (SSL_CTX_load_verify_locations(ssl->ctx, (char *) path, NULL) == 0) { - ngx_ssl_error(NGX_LOG_ALERT, log, 0, - "SSL_CTX_load_verify_locations(\"%s\") failed", path); - ngx_ssl_cleanup_ctx(ssl); - return NULL; + ngx_memcpy(path, ca->data, ca->len); + path[ca->len] = '\0'; + + /* load_verify_locations registers PEM, not DEFAULT, so it does not + * pay the per-handshake directory walk described above */ + if (SSL_CTX_load_verify_locations(ssl->ctx, (char *) path, NULL) + == 0) + { + ngx_ssl_error(NGX_LOG_ALERT, log, 0, + "SSL_CTX_load_verify_locations(\"%s\") failed", + path); + ngx_ssl_cleanup_ctx(ssl); + return NULL; + } } - } - SSL_CTX_set_verify_depth(ssl->ctx, 100); + SSL_CTX_set_verify_depth(ssl->ctx, 100); + } /* build the cache node before registering cleanup so an alloc failure * frees the context instead of orphaning it until worker exit */ @@ -1611,6 +1699,7 @@ ngx_http_ffi_client_ssl_ctx(ngx_http_ffi_client_main_conf_t *fmcf, } node->ssl = ssl; + node->verify = verify ? 1 : 0; node->ca.len = ca->len; if (ca->len) { node->ca.data = ngx_pnalloc(ngx_cycle->pool, ca->len); @@ -1740,7 +1829,7 @@ ngx_http_ffi_client_ssl_init_connection(ngx_http_ffi_client_op_t *op) fmcf = ngx_http_get_module_main_conf(op->r, ngx_http_ffi_client_module); ssl = ngx_http_ffi_client_ssl_ctx(fmcf, &op->ssl_trusted_certificate, - c->log); + op->ssl_verify, c->log); if (ssl == NULL) { ngx_http_ffi_client_finalize(op, NGX_ERROR, "failed to init TLS"); return NGX_ERROR; From f1f5d49a6f9d4aaf8361a0d1226d3216598db436 Mon Sep 17 00:00:00 2001 From: Shreemaan Abhishek Date: Wed, 5 Aug 2026 13:29:52 +0545 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/ngx_http_ffi_client_request.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/ngx_http_ffi_client_request.c b/src/ngx_http_ffi_client_request.c index 7088b4f..224376a 100644 --- a/src/ngx_http_ffi_client_request.c +++ b/src/ngx_http_ffi_client_request.c @@ -1578,6 +1578,8 @@ ngx_http_ffi_client_ssl_default_store(X509_STORE *store, ngx_log_t *log) lookup = X509_STORE_add_lookup(store, X509_LOOKUP_file()); if (lookup == NULL) { + ngx_ssl_error(NGX_LOG_ALERT, log, 0, + "X509_STORE_add_lookup(X509_LOOKUP_file) failed"); return NGX_ERROR; } @@ -1591,6 +1593,8 @@ ngx_http_ffi_client_ssl_default_store(X509_STORE *store, ngx_log_t *log) lookup = X509_STORE_add_lookup(store, X509_LOOKUP_hash_dir()); if (lookup == NULL) { + ngx_ssl_error(NGX_LOG_ALERT, log, 0, + "X509_STORE_add_lookup(X509_LOOKUP_hash_dir) failed"); return NGX_ERROR; } From e32ae46d606140f1cdc7abf2f396d61d7aaec7c3 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 5 Aug 2026 13:39:42 +0545 Subject: [PATCH 3/3] fix: share one SSL_CTX across all verify-off callers Nothing in ngx_http_ffi_client_ssl_ctx() reads `ca` unless `verify` is set, so every verify-off context is byte-identical whatever CA the caller named. The cache still keyed on `ca`, so a caller passing ssl_trusted_certificate with ssl_verify off retained one storeless SSL_CTX per distinct path for the life of the worker. Fold `ca` away at function entry, ahead of both the lookup and the cache node, so the two can never disagree: normalising only the lookup would miss every time and allocate a context per request. --- src/ngx_http_ffi_client.h | 3 ++- src/ngx_http_ffi_client_request.c | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/ngx_http_ffi_client.h b/src/ngx_http_ffi_client.h index c62e48a..5030c09 100644 --- a/src/ngx_http_ffi_client.h +++ b/src/ngx_http_ffi_client.h @@ -203,7 +203,8 @@ typedef struct ngx_http_ffi_client_keepalive_item_s * One cached client SSL_CTX per distinct trust config. The key is the trusted * CA path *and* whether verification is on: a verify-off context carries no * trust store at all, so reusing one for a verify-on request would fail every - * chain it was asked to check. + * chain it was asked to check. The CA is only part of the key when verify is + * on; with it off every caller shares the one storeless context. */ typedef struct ngx_http_ffi_client_ssl_ctx_s ngx_http_ffi_client_ssl_ctx_t; diff --git a/src/ngx_http_ffi_client_request.c b/src/ngx_http_ffi_client_request.c index 224376a..4bb0292 100644 --- a/src/ngx_http_ffi_client_request.c +++ b/src/ngx_http_ffi_client_request.c @@ -1623,18 +1623,29 @@ ngx_http_ffi_client_ssl_default_store(X509_STORE *store, ngx_log_t *log) * chain; when it is off no trust store is loaded at all, because OpenSSL * verifies the chain during every handshake regardless of the verify mode and * ngx_http_ffi_client_ssl_handshake_handler() only reads the result when the - * caller asked for it. Returns NULL on failure without caching a half-built - * context. + * caller asked for it, so with verify off `ca` is folded away and every such + * caller shares one context. Returns NULL on failure without caching a + * half-built context. */ static ngx_ssl_t * ngx_http_ffi_client_ssl_ctx(ngx_http_ffi_client_main_conf_t *fmcf, ngx_str_t *ca, ngx_uint_t verify, ngx_log_t *log) { u_char *path; + ngx_str_t none = ngx_null_string; ngx_ssl_t *ssl; ngx_pool_cleanup_t *cln; ngx_http_ffi_client_ssl_ctx_t *node; + /* Nothing below reads `ca` unless `verify` is set, so a verify-off context + * is the same whatever CA the caller named. Fold it here, ahead of both the + * lookup and the cache node, so the two always agree: normalising only one + * of them would miss every time and leak a context per request. This must + * move if `ca` ever gains meaning with verify off. */ + if (!verify) { + ca = &none; + } + for (node = fmcf->ssl_ctxs; node != NULL; node = node->next) { /* guard memcmp: NULL pointers with len 0 are UB */ if (node->verify == (verify ? 1u : 0u)