[PATCH] Enable multiple ssl_stapling_file configuration directives
Peter Linss
peter at linss.com
Tue Jul 4 18:51:10 UTC 2017
> On Jul 4, 2017, at 7:28 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> Hello!
>
> On Sat, Jul 01, 2017 at 06:21:03PM -0700, Peter Linss wrote:
>
>> # HG changeset patch
>> # User Peter Linss <peter at linss.com <mailto:peter at linss.com>>
>> # Date 1498957095 25200
>> # Sat Jul 01 17:58:15 2017 -0700
>> # Node ID 0580b76366e8540973e0ed884d0cec9fc4a7e488
>> # Parent a1c6685e80cba59284fc5e500818ea3b871403eb
>> Enable multiple ssl_stapling_file configuration directives
>
> There should be "SSL: " prefix and a dot after the sentence. For
> more examples, consider looking at "hg log -v”.
Ok, will do.
>
>>
>> When using OCSP stapling files with multiple certificates,
>> each certificate must have its own ssl_stapling_file.
>> Changes ssl_stapling_file to store an array, and sets individual
>> staples per certificate.
>> Generates an error if ssl_stapling_file is specified but not
>> the same number of times as certificates.
>>
>> 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
>> @@ -154,17 +154,17 @@ ngx_int_t ngx_ssl_certificate(ngx_conf_t
>> ngx_int_t ngx_ssl_ciphers(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *ciphers,
>> ngx_uint_t prefer_server_ciphers);
>> ngx_int_t ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
>> ngx_str_t *cert, ngx_int_t depth);
>> ngx_int_t ngx_ssl_trusted_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
>> ngx_str_t *cert, ngx_int_t depth);
>> ngx_int_t ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *crl);
>> ngx_int_t ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl,
>> - ngx_str_t *file, ngx_str_t *responder, ngx_uint_t verify);
>> + ngx_array_t *files, ngx_str_t *responder, ngx_uint_t verify);
>> ngx_int_t ngx_ssl_stapling_resolver(ngx_conf_t *cf, ngx_ssl_t *ssl,
>> ngx_resolver_t *resolver, ngx_msec_t resolver_timeout);
>> RSA *ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export,
>> int key_length);
>> ngx_array_t *ngx_ssl_read_password_file(ngx_conf_t *cf, ngx_str_t *file);
>> ngx_int_t ngx_ssl_dhparam(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file);
>> ngx_int_t ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *name);
>> ngx_int_t ngx_ssl_session_cache(ngx_ssl_t *ssl, ngx_str_t *sess_ctx,
>> diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c
>> --- a/src/event/ngx_event_openssl_stapling.c
>> +++ b/src/event/ngx_event_openssl_stapling.c
>> @@ -120,29 +120,47 @@ static ngx_int_t ngx_ssl_ocsp_parse_stat
>> static ngx_int_t ngx_ssl_ocsp_process_headers(ngx_ssl_ocsp_ctx_t *ctx);
>> static ngx_int_t ngx_ssl_ocsp_parse_header_line(ngx_ssl_ocsp_ctx_t *ctx);
>> static ngx_int_t ngx_ssl_ocsp_process_body(ngx_ssl_ocsp_ctx_t *ctx);
>>
>> static u_char *ngx_ssl_ocsp_log_error(ngx_log_t *log, u_char *buf, size_t len);
>>
>>
>> ngx_int_t
>> -ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file,
>> +ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *files,
>> ngx_str_t *responder, ngx_uint_t verify)
>> {
>> - X509 *cert;
>> + X509 *cert;
>> + ngx_str_t *file;
>> + ngx_uint_t i;
>>
>> - for (cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);
>> - cert;
>> - cert = X509_get_ex_data(cert, ngx_ssl_next_certificate_index))
>> + if (files == NULL)
>> {
>> - if (ngx_ssl_stapling_certificate(cf, ssl, cert, file, responder, verify)
>> - != NGX_OK)
>> + for (cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index);
>> + cert;
>> + cert = X509_get_ex_data(cert, ngx_ssl_next_certificate_index))
>> {
>> - return NGX_ERROR;
>> + if (ngx_ssl_stapling_certificate(cf, ssl, cert, NULL, responder, verify)
>> + != NGX_OK)
>> + {
>> + return NGX_ERROR;
>> + }
>> + }
>> + } else {
>> + file = files->elts;
>> +
>> + for (cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index), i = files->nelts - 1;
>> + cert;
>> + cert = X509_get_ex_data(cert, ngx_ssl_next_certificate_index), i--)
>> + {
>> + if (ngx_ssl_stapling_certificate(cf, ssl, cert, &file[i], responder, verify)
>> + != NGX_OK)
>> + {
>> + return NGX_ERROR;
>> + }
>> }
>> }
>
> This looks overcomplicated and should be rewritten. Instead,
> consider preserving a single loop over certificates available, and
> providing appropriate parameters to ngx_ssl_stapling_certificate()
> inside the loop.
Will do. I wasn’t happy this this myself, but was trying to match existing code style as much as possible in the use of the files array.
Doing this in a single loop my default approach would be to use a conditional operator to handle the file parameter, but given that files can be NULL it gets a bit ugly without the additional setup. Do you prefer this approach? (untested at this point, but will test before resubmitting, just checking style)
ngx_int_t
ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *files,
ngx_str_t *responder, ngx_uint_t verify)
{
X509 *cert;
ngx_uint_t i;
for (cert = SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_certificate_index, i = 1);
cert;
cert = X509_get_ex_data(cert, ngx_ssl_next_certificate_index), i++)
{
if (ngx_ssl_stapling_certificate(cf, ssl, cert, files ? &(files->elts[files->nelts - i]) : NULL,
responder, verify)
!= NGX_OK)
{
return NGX_ERROR;
}
}
SSL_CTX_set_tlsext_status_cb(ssl->ctx, ngx_ssl_certificate_status_callback);
return NGX_OK;
}
Also, matching the staple file to the certificate based on the certificate order feels a bit fragile to me. Have a suggestion for a more robust approach? Or is the certificate order stable enough? (Nothing obvious jumped out at me without changing a bunch of other setup logic and I wanted to keep the impact of this change minimal.)
>
>>
>> SSL_CTX_set_tlsext_status_cb(ssl->ctx, ngx_ssl_certificate_status_callback);
>>
>> return NGX_OK;
>> }
>>
>> @@ -175,17 +193,17 @@ ngx_ssl_stapling_certificate(ngx_conf_t
>>
>> staple->ssl_ctx = ssl->ctx;
>> staple->timeout = 60000;
>> staple->verify = verify;
>> staple->cert = cert;
>> staple->name = X509_get_ex_data(staple->cert,
>> ngx_ssl_certificate_name_index);
>>
>> - if (file->len) {
>> + if (file && file->len) {
>> /* use OCSP response from the file */
>>
>> if (ngx_ssl_stapling_file(cf, ssl, staple, file) != NGX_OK) {
>> return NGX_ERROR;
>> }
>>
>> return NGX_OK;
>> }
>> @@ -1866,17 +1884,17 @@ ngx_ssl_ocsp_log_error(ngx_log_t *log, u
>> return p;
>> }
>>
>>
>> #else
>>
>>
>> ngx_int_t
>> -ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file,
>> +ngx_ssl_stapling(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *files,
>> ngx_str_t *responder, ngx_uint_t verify)
>> {
>> ngx_log_error(NGX_LOG_WARN, ssl->log, 0,
>> "\"ssl_stapling\" ignored, not supported");
>>
>> return NGX_OK;
>> }
>>
>> diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
>> --- a/src/http/modules/ngx_http_ssl_module.c
>> +++ b/src/http/modules/ngx_http_ssl_module.c
>> @@ -210,19 +210,19 @@ static ngx_command_t ngx_http_ssl_comma
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG,
>> ngx_conf_set_flag_slot,
>> NGX_HTTP_SRV_CONF_OFFSET,
>> offsetof(ngx_http_ssl_srv_conf_t, stapling),
>> NULL },
>>
>> { ngx_string("ssl_stapling_file"),
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
>> - ngx_conf_set_str_slot,
>> + ngx_conf_set_str_array_slot,
>> NGX_HTTP_SRV_CONF_OFFSET,
>> - offsetof(ngx_http_ssl_srv_conf_t, stapling_file),
>> + offsetof(ngx_http_ssl_srv_conf_t, stapling_files),
>> NULL },
>>
>> { ngx_string("ssl_stapling_responder"),
>> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
>> ngx_conf_set_str_slot,
>> NGX_HTTP_SRV_CONF_OFFSET,
>> offsetof(ngx_http_ssl_srv_conf_t, stapling_responder),
>> NULL },
>> @@ -532,33 +532,33 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
>> * sscf->protocols = 0;
>> * sscf->dhparam = { 0, NULL };
>> * sscf->ecdh_curve = { 0, NULL };
>> * sscf->client_certificate = { 0, NULL };
>> * sscf->trusted_certificate = { 0, NULL };
>> * sscf->crl = { 0, NULL };
>> * sscf->ciphers = { 0, NULL };
>> * sscf->shm_zone = NULL;
>> - * sscf->stapling_file = { 0, NULL };
>> * sscf->stapling_responder = { 0, NULL };
>> */
>>
>> sscf->enable = NGX_CONF_UNSET;
>> sscf->prefer_server_ciphers = NGX_CONF_UNSET;
>> sscf->buffer_size = NGX_CONF_UNSET_SIZE;
>> sscf->verify = NGX_CONF_UNSET_UINT;
>> sscf->verify_depth = NGX_CONF_UNSET_UINT;
>> sscf->certificates = NGX_CONF_UNSET_PTR;
>> sscf->certificate_keys = NGX_CONF_UNSET_PTR;
>> sscf->passwords = NGX_CONF_UNSET_PTR;
>> sscf->builtin_session_cache = NGX_CONF_UNSET;
>> sscf->session_timeout = NGX_CONF_UNSET;
>> sscf->session_tickets = NGX_CONF_UNSET;
>> sscf->session_ticket_keys = NGX_CONF_UNSET_PTR;
>> sscf->stapling = NGX_CONF_UNSET;
>> + sscf->stapling_files = NGX_CONF_UNSET_PTR;
>> sscf->stapling_verify = NGX_CONF_UNSET;
>>
>> return sscf;
>> }
>>
>>
>> static char *
>> ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
>> @@ -611,17 +611,17 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>>
>> ngx_conf_merge_str_value(conf->ecdh_curve, prev->ecdh_curve,
>> NGX_DEFAULT_ECDH_CURVE);
>>
>> ngx_conf_merge_str_value(conf->ciphers, prev->ciphers, NGX_DEFAULT_CIPHERS);
>>
>> ngx_conf_merge_value(conf->stapling, prev->stapling, 0);
>> ngx_conf_merge_value(conf->stapling_verify, prev->stapling_verify, 0);
>> - ngx_conf_merge_str_value(conf->stapling_file, prev->stapling_file, "");
>> + ngx_conf_merge_ptr_value(conf->stapling_files, prev->stapling_files, NULL);
>> ngx_conf_merge_str_value(conf->stapling_responder,
>> prev->stapling_responder, "");
>>
>> conf->ssl.log = cf->log;
>>
>> if (conf->enable) {
>>
>> if (conf->certificates == NULL) {
>> @@ -662,16 +662,28 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>> {
>> ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> "no \"ssl_certificate_key\" is defined "
>> "for certificate \"%V\"",
>> ((ngx_str_t *) conf->certificates->elts)
>> + conf->certificates->nelts - 1);
>> return NGX_CONF_ERROR;
>> }
>> +
>> + if ((conf->stapling_files) &&
>> + (conf->stapling_files->nelts != conf->certificates->nelts))
>> + {
>> + ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
>> + "no \"ssl_stapling_file\" is defined "
>> + "for certificate \"%V\"",
>> + ((ngx_str_t *) conf->certificates->elts)
>> + + conf->certificates->nelts - 1);
>> + return NGX_CONF_ERROR;
>
> Certainly an error here is too restrictive, I would rather
> recommend preserving the current behaviour (or may be emitting a
> warning instead).
The current behavior is to serve the wrong OCSP staple for one (or more) of the certificates. I don’t think this is desirable as it can cause clients that validate OCSP staples to reject the TLS connection (especially if the certificate has Must-Staple). That seemed like a significant issue to me, but I can make it a warning if you prefer. As a server admin, I personally prefer the error if my ssl config can lead to client side issues, like not being able to reach my site, and it may not be something I see in testing my server if I don’t test against all clients (this stuff is hard enough to get right in the first place).
(FWIW, trying to set multiple stapling files currently gives an error due to a duplicate ssl_stapling_file directive.)
It was also unclear to me if the check needed to be repeated in the if (conf->enable) block as well as the else block, guidance appreciated as I don’t really get the point of that logic.
>
>> + }
>> +
>> }
>
> Style, there should be no empty line here.
Will fix all the above and resubmit shortly. Thanks for the review!
(also, I’m currently running the original patch on a production server with dual RSA/ECDSA certificates and a bot keeping the stapling files current at https://elemental.software/ if you want to test against a running instance)
Peter
>
>>
>> if (ngx_ssl_create(&conf->ssl, conf->protocols, conf) != NGX_OK) {
>> return NGX_CONF_ERROR;
>> }
>>
>> #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
>>
>> @@ -786,17 +798,17 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>> if (ngx_ssl_session_ticket_keys(cf, &conf->ssl, conf->session_ticket_keys)
>> != NGX_OK)
>> {
>> return NGX_CONF_ERROR;
>> }
>>
>> if (conf->stapling) {
>>
>> - if (ngx_ssl_stapling(cf, &conf->ssl, &conf->stapling_file,
>> + if (ngx_ssl_stapling(cf, &conf->ssl, conf->stapling_files,
>> &conf->stapling_responder, conf->stapling_verify)
>> != NGX_OK)
>> {
>> return NGX_CONF_ERROR;
>> }
>>
>> }
>>
>> diff --git a/src/http/modules/ngx_http_ssl_module.h b/src/http/modules/ngx_http_ssl_module.h
>> --- a/src/http/modules/ngx_http_ssl_module.h
>> +++ b/src/http/modules/ngx_http_ssl_module.h
>> @@ -47,17 +47,17 @@ typedef struct {
>>
>> ngx_shm_zone_t *shm_zone;
>>
>> ngx_flag_t session_tickets;
>> ngx_array_t *session_ticket_keys;
>>
>> ngx_flag_t stapling;
>> ngx_flag_t stapling_verify;
>> - ngx_str_t stapling_file;
>> + ngx_array_t *stapling_files;
>> ngx_str_t stapling_responder;
>>
>> u_char *file;
>> ngx_uint_t line;
>> } ngx_http_ssl_srv_conf_t;
>>
>>
>> extern ngx_module_t ngx_http_ssl_module;
>> _______________________________________________
>> nginx-devel mailing list
>> nginx-devel at nginx.org
>> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> --
> Maxim Dounin
> http://nginx.org/ <http://nginx.org/>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org <mailto:nginx-devel at nginx.org>
> http://mailman.nginx.org/mailman/listinfo/nginx-devel <http://mailman.nginx.org/mailman/listinfo/nginx-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20170704/f6588c4a/attachment-0001.html>
More information about the nginx-devel
mailing list