[PATCH] Add ngx_ssl_ciphers() to set list of cipher suites in openssl module

[Maxim Dounin]

Hello!

On Fri, Jun 10, 2016 at 12:30:32PM +0200, Tim Taubert wrote:

> # HG changeset patch
> # User Tim Taubert <tim at timtaubert.de>
> # Date 1465549632 -7200
> #      Fri Jun 10 11:07:12 2016 +0200
> # Node ID d94b74c337b70087b78258d2124c49a6422190c9
> # Parent  1064ea81ed3aabb8ad422ffcc60ddcde667022ac
> Add ngx_ssl_ciphers() to set list of cipher suites in openssl module
> 
> Replace all calls to SSL_CTX_set_cipher_list() from outside the OpenSSL module
> by ngx_sll_ciphers() calls to make NGINX more crypto-library-agnostic

Style nitpicking: Please use the "SSL: " prefix for SSL-related 
commits.  Please use full sentences in the commit log, including 
dots.  Please spell "nginx" lowercase.  Please keep summary line 
under 67 symbols.

E.g.:

: SSL: ngx_ssl_ciphers() to set list of ciphers.
: 
: It replaces all direct calls to SSL_CTX_set_cipher_list() to make 
: nginx more crypto-library-agnostic.

> 
> 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
> @@ -562,16 +562,30 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_
>      }
>  
>      SSL_CTX_set_default_passwd_cb(ssl->ctx, NULL);
>  
>      return NGX_OK;
>  }
>  
>  
> +ngx_int_t
> +ngx_ssl_ciphers(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *ciphers)
> +{
> +    if (SSL_CTX_set_cipher_list(ssl->ctx, (const char *) ciphers->data) == 0) {

The "const" qualifier can be safely dropped here.  And I tend to 
think it should, as I already removed "const" from all "(const char *)" 
casts found in ngx_event_openssl.c, see rev. addd98357629.

> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                      "SSL_CTX_set_cipher_list(\"%V\") failed",
> +                      ciphers);
> +        return NGX_ERROR;
> +    }
> +
> +    return NGX_OK;
> +}
> +
> +
>  static int
>  ngx_ssl_password_callback(char *buf, int size, int rwflag, void *userdata)
>  {
>      ngx_str_t *pwd = userdata;
>  
>      if (rwflag) {
>          ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0,
>                        "ngx_ssl_password_callback() is called for encryption");

The place choosen for the ngx_ssl_ciphers() function looks wrong, 
as ngx_ssl_password_callback() is a part of ngx_ssl_certificate() 
code.  I would suggest to put it after ngx_ssl_password_callback() 
instead.

Or may be it would be even better to put it somewhere before 
ngx_ssl_dhparam() (with appropriate ngx_event_openssl.h change).

[...]

> diff --git a/src/stream/ngx_stream_ssl_module.c b/src/stream/ngx_stream_ssl_module.c
> --- a/src/stream/ngx_stream_ssl_module.c
> +++ b/src/stream/ngx_stream_ssl_module.c
> @@ -261,23 +261,17 @@ ngx_stream_ssl_merge_conf(ngx_conf_t *cf
>  
>      if (ngx_ssl_certificates(cf, &conf->ssl, conf->certificates,
>                               conf->certificate_keys, conf->passwords)
>          != NGX_OK)
>      {
>          return NGX_CONF_ERROR;
>      }
>  
> -    if (SSL_CTX_set_cipher_list(conf->ssl.ctx,
> -                                (const char *) conf->ciphers.data)
> -        == 0)
> -    {
> -        ngx_ssl_error(NGX_LOG_EMERG, cf->log, 0,
> -                      "SSL_CTX_set_cipher_list(\"%V\") failed",
> -                      &conf->ciphers);
> +    if (ngx_ssl_ciphers(cf, &conf->ssl, &conf->ciphers) != NGX_OK) {
>          return NGX_CONF_ERROR;
>      }
>  
>      if (conf->prefer_server_ciphers) {
>          SSL_CTX_set_options(conf->ssl.ctx, SSL_OP_CIPHER_SERVER_PREFERENCE);
>      }

What about moving SSL_CTX_set_options(SSL_OP_CIPHER_SERVER_PREFERENCE) 
calls to the ngx_ssl_ciphers() function as well?

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