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

[Maxim Dounin]

Hello!

On Wed, Jun 15, 2016 at 06:51:25PM +0100, Tim Taubert wrote:

> # HG changeset patch
> # User Tim Taubert <tim at timtaubert.de>
> # Date 1466012923 -3600
> #      Wed Jun 15 18:48:43 2016 +0100
> # Node ID 2a1e1a1d88c225770bdac64b59688a5129ce8cba
> # Parent  1064ea81ed3aabb8ad422ffcc60ddcde667022ac
> SSL: ngx_ssl_ciphers() to set list of ciphers.
> 
> This patch replaces all calls to SSL_CTX_set_cipher_list() with
> ngx_ssl_ciphers() 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
> @@ -18,16 +18,18 @@ typedef struct {
>  } ngx_openssl_conf_t;
>  
>  
>  static int ngx_ssl_password_callback(char *buf, int size, int rwflag,
>      void *userdata);
>  static int ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
>  static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
>      int ret);
> +RSA *ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export,
> +    int key_length);

Moving the function here will need "static".

You may also want to preserve it as is in the ngx_event_openssl.h 
for now, for compatibility with 3rd party modules using it, if 
any.  Though I've failed to find any module potentially affected 
by the change, so it's up to you.

[...]

> @@ -587,16 +589,40 @@ ngx_ssl_password_callback(char *buf, int
>  
>      ngx_memcpy(buf, pwd->data, size);
>  
>      return size;
>  }
>  
>  
>  ngx_int_t
> +ngx_ssl_ciphers(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *ciphers,
> +    ngx_flag_t prefer_server_ciphers)

Please use "ngx_uint_t" here instead.  The "ngx_flag_t" type 
is used during configuration parsing, and not expected to have any 
ngx_flag_t-specific values at the point where ngx_ssl_ciphers() is 
called.

[...]

> diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -4318,23 +4318,20 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, n
>          if (ngx_ssl_certificate(cf, plcf->upstream.ssl, &plcf->ssl_certificate,
>                                  &plcf->ssl_certificate_key, plcf->ssl_passwords)
>              != NGX_OK)
>          {
>              return NGX_ERROR;
>          }
>      }
>  
> -    if (SSL_CTX_set_cipher_list(plcf->upstream.ssl->ctx,
> -                                (const char *) plcf->ssl_ciphers.data)
> -        == 0)
> +    if (ngx_ssl_ciphers(cf, plcf->upstream.ssl, &plcf->ssl_ciphers,
> +                        NGX_CONF_UNSET)

The NGX_CONF_UNSET here looks wrong and will quite unexpectedly 
try to set SSL_OP_CIPHER_SERVER_PREFERENCE (it means nothing on 
client side, but nevertheless somewhat confusing).  Just using 0 
will be a better idea.

[...]

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