[PATCH] Enable multiple ssl_stapling_file configuration directives

[Maxim Dounin]

Hello!

On Tue, Jul 04, 2017 at 11:51:10AM -0700, Peter Linss wrote:

[...]

> > 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;
>     }

The particular example doesn't look acceptable either (and doesn't 
look working due to syntax errors), though it looks better.  Using 
a separate variable for a file might improve code readability 
here.

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

This is one of the main reasons why I generally against the 
feature in general.  Order-based matching is not perfect even for 
certificates and keys, and extending it to other directives looks 
even more fragile.  As previously suggested, it might be better 
idea to avoid trying to support multiple ssl_certificate_file 
directives, see https://trac.nginx.org/nginx/ticket/990#comment:5.

An alternative solution that will still allow to provide custom 
OCSP response would be to support OCSP responses in the 
certificate file in PEM format.  This is actually a solution I 
would prefer to see, but this will require additional support in 
OpenSSL, as currently it is not able to work with OCSP responses 
in PEM format.

Or we can instead focus on improving persistence of OCSP responses 
obtained automatically.

[...]

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

The current behaviour is much more complicated than that.  In 
particular, consider the following cases:

- RSA and ECDSA certificates are provided, ssl_stapling_file is 
  set, but ssl_stapling is switched off.

- Two RSA certificates are provided, and ssl_stapling_file 
  provided as well.

- RSA and DSA certificates are provided, ssl_stapling_file is 
  set, and DSA ciphers are disabled using ssl_ciphers.

In all of the above cases the current behaviour will be perfectly 
correct.  On the other hand, the error you've introduced will 
break such potentially existing and correctly working 
configurations.

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

The conf->enable flag is set when "ssl on" is used in the 
configuration.  Using "listen ... ssl" is recommended instead.  
Proper checking should not depend on a configuration method used, 
so adding the check to only one code path is a bad idea.  On the 
other hand, adding a check there might not be a good idea in 
general, a better place might be in ngx_ssl_stapling() or just 
before the call.

-- 
Maxim Dounin
http://nginx.org/