> On 30 Aug 2024, at 03:28, Aleksei Bavshin <a.bavs...@nginx.com> wrote:
>
> On 8/21/2024 3:04 PM, Sergey Kandaurov wrote:
>> # HG changeset patch
>> # User Sergey Kandaurov <pluk...@nginx.com>
>> # Date 1721762857 0
>> # Tue Jul 23 19:27:37 2024 +0000
>> # Node ID 0d87e1495981ca541d8cdb947d94f20a686545a3
>> # Parent 6fbe0bcb81696bba12d186e5c15323046bcac2d9
>> SSL: caching certificates.
>> Certificate chains are now loaded once.
>> The certificate cache provides each chain as a unique stack of referenced
>> counted elements. This shallow copy is required because OpenSSL's stacks
>> aren't reference counted.
>> Based on previous work by Mini Hawthorne.
>> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
>> --- a/src/event/ngx_event_openssl.c
>> +++ b/src/event/ngx_event_openssl.c
>> @@ -18,8 +18,6 @@ typedef struct {
>> } ngx_openssl_conf_t;
>> -static X509 *ngx_ssl_load_certificate(ngx_pool_t *pool, char **err,
>> - ngx_str_t *cert, STACK_OF(X509) **chain);
>> static EVP_PKEY *ngx_ssl_load_certificate_key(ngx_pool_t *pool, char **err,
>> ngx_str_t *key, ngx_array_t *passwords);
>> static int ngx_ssl_password_callback(char *buf, int size, int rwflag,
>> @@ -443,8 +441,9 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
>> STACK_OF(X509) *chain;
>> ngx_ssl_name_t *name;
>> - x509 = ngx_ssl_load_certificate(cf->pool, &err, cert, &chain);
>> - if (x509 == NULL) {
>> + chain = ngx_ssl_cache_fetch(cf, NGX_SSL_CACHE_CERT, &err, cert, NULL);
>> +
>> + if (chain == NULL) {
>> if (err != NULL) {
>> ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
>> "cannot load certificate \"%s\": %s",
>> @@ -454,6 +453,8 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
>> return NGX_ERROR;
>> }
>> + x509 = sk_X509_shift(chain);
>> +
>> if (SSL_CTX_use_certificate(ssl->ctx, x509) == 0) {
>> ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
>> "SSL_CTX_use_certificate(\"%s\") failed", cert->data);
>> @@ -568,8 +569,10 @@ ngx_ssl_connection_certificate(ngx_conne
>> EVP_PKEY *pkey;
>> STACK_OF(X509) *chain;
>> - x509 = ngx_ssl_load_certificate(pool, &err, cert, &chain);
>> - if (x509 == NULL) {
>> + chain = ngx_ssl_cache_connection_fetch(NGX_SSL_CACHE_CERT, &err, cert,
>> + NULL);
>> +
>> + if (chain == NULL) {
>> if (err != NULL) {
>> ngx_ssl_error(NGX_LOG_ERR, c->log, 0,
>> "cannot load certificate \"%s\": %s",
>> @@ -579,6 +582,8 @@ ngx_ssl_connection_certificate(ngx_conne
>> return NGX_ERROR;
>> }
>> + x509 = sk_X509_shift(chain);
>> +
>> if (SSL_use_certificate(c->ssl->connection, x509) == 0) {
>> ngx_ssl_error(NGX_LOG_ERR, c->log, 0,
>> "SSL_use_certificate(\"%s\") failed", cert->data);
>> @@ -630,96 +635,6 @@ ngx_ssl_connection_certificate(ngx_conne
>> }
>> -static X509 *
>> -ngx_ssl_load_certificate(ngx_pool_t *pool, char **err, ngx_str_t *cert,
>> - STACK_OF(X509) **chain)
>> -{
>> - BIO *bio;
>> - X509 *x509, *temp;
>> - u_long n;
>> -
>> - if (ngx_strncmp(cert->data, "data:", sizeof("data:") - 1) == 0) {
>> -
>> - bio = BIO_new_mem_buf(cert->data + sizeof("data:") - 1,
>> - cert->len - (sizeof("data:") - 1));
>> - if (bio == NULL) {
>> - *err = "BIO_new_mem_buf() failed";
>> - return NULL;
>> - }
>> -
>> - } else {
>> -
>> - if (ngx_get_full_name(pool, (ngx_str_t *) &ngx_cycle->conf_prefix,
>> cert)
>> - != NGX_OK)
>> - {
>> - *err = NULL;
>> - return NULL;
>> - }
>> -
>> - bio = BIO_new_file((char *) cert->data, "r");
>> - if (bio == NULL) {
>> - *err = "BIO_new_file() failed";
>> - return NULL;
>> - }
>> - }
>> -
>> - /* certificate itself */
>> -
>> - x509 = PEM_read_bio_X509_AUX(bio, NULL, NULL, NULL);
>> - if (x509 == NULL) {
>> - *err = "PEM_read_bio_X509_AUX() failed";
>> - BIO_free(bio);
>> - return NULL;
>> - }
>> -
>> - /* rest of the chain */
>> -
>> - *chain = sk_X509_new_null();
>> - if (*chain == NULL) {
>> - *err = "sk_X509_new_null() failed";
>> - BIO_free(bio);
>> - X509_free(x509);
>> - return NULL;
>> - }
>> -
>> - for ( ;; ) {
>> -
>> - temp = PEM_read_bio_X509(bio, NULL, NULL, NULL);
>> - if (temp == NULL) {
>> - n = ERR_peek_last_error();
>> -
>> - if (ERR_GET_LIB(n) == ERR_LIB_PEM
>> - && ERR_GET_REASON(n) == PEM_R_NO_START_LINE)
>> - {
>> - /* end of file */
>> - ERR_clear_error();
>> - break;
>> - }
>> -
>> - /* some real error */
>> -
>> - *err = "PEM_read_bio_X509() failed";
>> - BIO_free(bio);
>> - X509_free(x509);
>> - sk_X509_pop_free(*chain, X509_free);
>> - return NULL;
>> - }
>> -
>> - if (sk_X509_push(*chain, temp) == 0) {
>> - *err = "sk_X509_push() failed";
>> - BIO_free(bio);
>> - X509_free(x509);
>> - sk_X509_pop_free(*chain, X509_free);
>> - return NULL;
>> - }
>> - }
>> -
>> - BIO_free(bio);
>> -
>> - return x509;
>> -}
>> -
>> -
>> static EVP_PKEY *
>> ngx_ssl_load_certificate_key(ngx_pool_t *pool, char **err,
>> ngx_str_t *key, ngx_array_t *passwords)
>> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
>> --- a/src/event/ngx_event_openssl.h
>> +++ b/src/event/ngx_event_openssl.h
>> @@ -202,6 +202,9 @@ typedef struct {
>> #define NGX_SSL_BUFSIZE 16384
>> +#define NGX_SSL_CACHE_CERT 0
>> +
>> +
>> ngx_int_t ngx_ssl_init(ngx_log_t *log);
>> ngx_int_t ngx_ssl_create(ngx_ssl_t *ssl, ngx_uint_t protocols, void *data);
>> diff --git a/src/event/ngx_event_openssl_cache.c
>> b/src/event/ngx_event_openssl_cache.c
>> --- a/src/event/ngx_event_openssl_cache.c
>> +++ b/src/event/ngx_event_openssl_cache.c
>> @@ -39,6 +39,12 @@ typedef struct {
>> static ngx_ssl_cache_node_t *ngx_ssl_cache_lookup(ngx_ssl_cache_t *cache,
>> ngx_ssl_cache_type_t *type, ngx_str_t *id, uint32_t hash);
>> +static void *ngx_ssl_cache_cert_create(ngx_str_t *id, char **err, void
>> *data);
>> +static void ngx_ssl_cache_cert_free(void *data);
>> +static void *ngx_ssl_cache_cert_ref(char **err, void *data);
>> +
>> +static BIO *ngx_ssl_cache_create_bio(ngx_str_t *id, char **err);
>> +
>> static void *ngx_openssl_cache_create_conf(ngx_cycle_t *cycle);
>> static void ngx_ssl_cache_cleanup(void *data);
>> static void ngx_ssl_cache_node_insert(ngx_rbtree_node_t *temp,
>> @@ -70,6 +76,10 @@ ngx_module_t ngx_openssl_cache_module =
>> static ngx_ssl_cache_type_t ngx_ssl_cache_types[] = {
>> + /* NGX_SSL_CACHE_CERT */
>> + { ngx_ssl_cache_cert_create,
>> + ngx_ssl_cache_cert_free,
>> + ngx_ssl_cache_cert_ref },
>> };
>> @@ -191,6 +201,166 @@ ngx_ssl_cache_lookup(ngx_ssl_cache_t *ca
>> static void *
>> +ngx_ssl_cache_cert_create(ngx_str_t *id, char **err, void *data)
>> +{
>> + BIO *bio;
>> + X509 *x509;
>> + u_long n;
>> + STACK_OF(X509) *chain;
>> +
>> + chain = sk_X509_new_null();
>> + if (chain == NULL) {
>> + *err = "sk_X509_new_null() failed";
>> + return NULL;
>> + }
>> +
>> + bio = ngx_ssl_cache_create_bio(id, err);
>> + if (bio == NULL) {
>> + sk_X509_pop_free(chain, X509_free);
>> + return NULL;
>> + }
>> +
>> + /* certificate itself */
>> +
>> + x509 = PEM_read_bio_X509_AUX(bio, NULL, NULL, NULL);
>> + if (x509 == NULL) {
>> + *err = "PEM_read_bio_X509_AUX() failed";
>> + BIO_free(bio);
>> + sk_X509_pop_free(chain, X509_free);
>> + return NULL;
>> + }
>> +
>> + if (sk_X509_push(chain, x509) == 0) {
>> + *err = "sk_X509_push() failed";
>> + BIO_free(bio);
>> + X509_free(x509);
>> + sk_X509_pop_free(chain, X509_free);
>> + return NULL;
>> + }
>> +
>> + /* rest of the chain */
>> +
>> + for ( ;; ) {
>> +
>> + x509 = PEM_read_bio_X509(bio, NULL, NULL, NULL);
>> + if (x509 == NULL) {
>> + n = ERR_peek_last_error();
>> +
>> + if (ERR_GET_LIB(n) == ERR_LIB_PEM
>> + && ERR_GET_REASON(n) == PEM_R_NO_START_LINE)
>> + {
>> + /* end of file */
>> + ERR_clear_error();
>> + break;
>> + }
>> +
>> + /* some real error */
>> +
>> + *err = "PEM_read_bio_X509() failed";
>> + BIO_free(bio);
>> + sk_X509_pop_free(chain, X509_free);
>> + return NULL;
>> + }
>> +
>> + if (sk_X509_push(chain, x509) == 0) {
>> + *err = "sk_X509_push() failed";
>> + BIO_free(bio);
>> + X509_free(x509);
>> + sk_X509_pop_free(chain, X509_free);
>> + return NULL;
>> + }
>> + }
>> +
>> + BIO_free(bio);
>> +
>> + return chain;
>> +}
>> +
>> +
>> +static void
>> +ngx_ssl_cache_cert_free(void *data)
>> +{
>> + sk_X509_pop_free(data, X509_free);
>> +}
>> +
>> +
>> +static void *
>> +ngx_ssl_cache_cert_ref(char **err, void *data)
>> +{
>> + int n, i;
>> + X509 *x509;
>> + STACK_OF(X509) *chain;
>> +
>> + chain = sk_X509_dup(data);
>> + if (chain == NULL) {
>> + *err = "sk_X509_dup() failed";
>> + return NULL;
>> + }
>> +
>> + n = sk_X509_num(chain);
>> +
>> + for (i = 0; i < n; i++) {
>> + x509 = sk_X509_value(chain, i);
>> +
>> +#if (OPENSSL_VERSION_NUMBER >= 0x10100000L)
>> + X509_up_ref(x509);
>> +#else
>> + CRYPTO_add(&x509->references, 1, CRYPTO_LOCK_X509);
>> +#endif
>> + }
>> +
>> + return chain;
>> +}
>> +
>> +
>> +static BIO *
>> +ngx_ssl_cache_create_bio(ngx_str_t *id, char **err)
>> +{
>> + BIO *bio;
>> + ngx_str_t path;
>> + ngx_pool_t *temp_pool;
>> +
>> + if (ngx_strncmp(id->data, "data:", sizeof("data:") - 1) == 0) {
>> +
>> + bio = BIO_new_mem_buf(id->data + sizeof("data:") - 1,
>> + id->len - (sizeof("data:") - 1));
>> + if (bio == NULL) {
>> + *err = "BIO_new_mem_buf() failed";
>> + }
>> +
>> + return bio;
>> + }
>> +
>> + temp_pool = ngx_create_pool(NGX_MAX_PATH, ngx_cycle->log);
>> + if (temp_pool == NULL) {
>> + *err = NULL;
>> + return NULL;
>> + }
>> +
>> + ngx_memcpy(&path, id, sizeof(ngx_str_t));
>> +
>> + if (ngx_get_full_name(temp_pool,
>> + (ngx_str_t *) &ngx_cycle->conf_prefix,
>> + &path)
>> + != NGX_OK)
>> + {
>> + *err = NULL;
>> + ngx_destroy_pool(temp_pool);
>> + return NULL;
>> + }
>> +
>> + bio = BIO_new_file((char *) path.data, "r");
>> + if (bio == NULL) {
>> + *err = "BIO_new_file() failed";
>> + }
>> +
>> + ngx_destroy_pool(temp_pool);
>> +
>> + return bio;
>> +}
>> +
>> +
>> +static void *
>> ngx_openssl_cache_create_conf(ngx_cycle_t *cycle)
>> {
>> ngx_ssl_cache_t *cache;
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel@nginx.org
>> https://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> The patch introduces a small difference in the behavior: we no longer resolve
> the full name preemptively and modify the paths in place.
> This results in:
>
> * possible duplication if the same object is referenced both by absolute and
> relative paths
> * relative certificate paths in the name_rbtree and in the OCSP stapling log
Agree, these side-effects should be fixed.
>
> Below is the patch addressing that and a test update. Note that a few lines
> dealing with NGX_SSL_CACHE_KEY should be applied to the patch 4.
>
Applied, with some larger rewrite.
> Otherwise, the series looks good to me.
>
> (There's one more difference in behavior; we started accepting "data:" in the
> trusted CA and CRL configuration directives. I've been reviewing with
> assumption that it is an intended consequence.)
>
This is also an unintentional change.
Fixed as well with some modifications to your patch.
Since we've moved to GitHub, see the updated patch series at
https://github.com/nginx/nginx/pull/140
Specifically, force-pushes from c61f1a1 to c51af6b.
[..]
> diff --git a/ssl_cache.t b/ssl_cache.t
> index 96bd6a83ef..44935c3386 100644
> --- a/ssl_cache.t
> +++ b/ssl_cache.t
> @@ -29,8 +29,10 @@ my $t = Test::Nginx->new();
>
> plan(skip_all => "not yet") unless $t->has_version('1.27.2');
>
> +my $d = $t->testdir();
> +
> $t->has(qw/http http_ssl socket_ssl/)->has_daemon('openssl')
> - ->write_file_expand('nginx.conf', <<'EOF');
> + ->write_file_expand('nginx.conf', << "EOF");
>
> %%TEST_GLOBALS%%
>
> @@ -64,12 +66,22 @@ http {
> ssl_client_certificate root.crt.fifo;
> ssl_crl root.crl.fifo;
> }
> +
> + server {
> + listen 127.0.0.1:8445 ssl;
> + server_name localhost;
> +
> + ssl_certificate $d/localhost.crt.fifo;
A %%TESTDIR%% template is used for such expansion.
Applied locally, tnx.
> + ssl_certificate_key $d/localhost.key.fifo;
> +
> + ssl_verify_client on;
> + ssl_client_certificate $d/root.crt.fifo;
> + ssl_crl $d/root.crl.fifo;
> + }
> }
>
> EOF
>
> -my $d = $t->testdir();
> -
> $t->write_file('openssl.conf', <<EOF);
> [ req ]
> default_bits = 2048
> @@ -135,12 +147,13 @@ foreach my $name ('root.crt', 'root.crl',
> 'localhost.crt', 'localhost.key') {
>
> $t->write_file('t', '');
>
> -$t->plan(2)->run();
> +$t->plan(3)->run();
>
> ###############################################################################
>
> like(get(8443), qr/200 OK/, 'cached certificate');
> like(get(8444, 'client'), qr/200 OK/, 'cached CA and CRL');
> +like(get(8445, 'client'), qr/200 OK/, 'cached objects with absolute path');
>
> ###############################################################################
--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
