[PATCH] allow to use engine keyform for server private key

[Maxim Dounin]

Hello!

On Wed, Jul 23, 2014 at 06:58:23PM +0400, Dmitrii Pichulin wrote:

> # HG changeset patch
> # User Dmitrii Pichulin
> # Date 1406127158 -14400
> #      Wed Jul 23 18:52:38 2014 +0400
> # Node ID fec1d814c8f363976a1217c81faec3d80e6c718f
> # Parent  9de5820bb3e04d7e21727b472a15831ec0b2be1d
> allow to use engine keyform for server private key
> 
> diff -r 9de5820bb3e0 -r fec1d814c8f3 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c	Fri Jul 18 20:11:40 2014 +0400
> +++ b/src/event/ngx_event_openssl.c	Wed Jul 23 18:52:38 2014 +0400
> @@ -11,6 +11,7 @@
>  
>  
>  #define NGX_SSL_PASSWORD_BUFFER_SIZE  4096
> +#define NGX_SSL_MAX_ENGINE_NAME_LEN   260
>  
>  
>  typedef struct {
> @@ -270,6 +271,10 @@
>      u_long       n;
>      ngx_str_t   *pwd;
>      ngx_uint_t   tries;
> +    EVP_PKEY    *pkey;
> +    ENGINE      *e;
> +    char        *p, *last;
> +    char         e_name[NGX_SSL_MAX_ENGINE_NAME_LEN + 1];

This doesn't match style used in nginx code.
Both declaration order and variable names should be different.

>  
>      if (ngx_conf_full_name(cf->cycle, cert, 1) != NGX_OK) {
>          return NGX_ERROR;
> @@ -352,6 +357,61 @@
>  
>      BIO_free(bio);
>  
> +    if (ngx_strncmp(key->data, "engine:", sizeof("engine:") - 1) == 0) {
> +
> +        p = (char *) key->data + sizeof("engine:") - 1;
> +        last = ngx_strchr(p, ':');
> +
> +        if (last == NULL || ngx_strchr(last + 1, ':') != NULL) {

Why key id shouldn't contain colons?  This restriction on key id 
looks strange.

> +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "invalid syntax: %V", key);
> +            return NGX_ERROR;
> +        }
> +
> +        if (last - p > NGX_SSL_MAX_ENGINE_NAME_LEN) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                          "too long engine name in \"ssl_certificate_key\"");
> +            return NGX_ERROR;
> +        }
> +
> +        ngx_memcpy(e_name, p, last - p);
> +        e_name[last - p] = 0;

It should be fine to assume we can modify the key name passed, and 
just set ':' to '\0' instead of using a fixed-size buffer.

> +
> +        e = ENGINE_by_id((char *) e_name);
> +
> +        if (e == NULL) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                          "ENGINE_by_id(\"%s\") failed", e_name);
> +            return NGX_ERROR;
> +        }
> +
> +        pkey = ENGINE_load_private_key(e, (char *) last + 1, 0, 0);

It looks like ENGINE_load_private_key() may need to ask user for a 
password.  The ssl_password_file should be respected here as well.

> +
> +        if (!pkey) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                          "ENGINE_load_private_key(\"%s\") failed", last + 1);
> +            ENGINE_free(e);
> +            return NGX_ERROR;
> +        }
> +
> +        if (SSL_CTX_use_PrivateKey(ssl->ctx, pkey) == 0) {
> +
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                       "SSL_CTX_use_PrivateKey_file(\"%s\") failed", last + 1);

I believe I've already pointed out in the previous review that 
it's better to split the line instead of shifting it to the left.

> +            EVP_PKEY_free(pkey);
> +            ENGINE_free(e);
> +            return NGX_ERROR;
> +        }
> +
> +        EVP_PKEY_free(pkey);
> +
> +        if (ENGINE_free(e) == 0) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0, "ENGINE_free() failed");
> +            return NGX_ERROR;
> +        }

Checking ENGINE_free(e) errors in one place but not in others is 
really bad idea.

> +
> +        return NGX_OK;
> +    }
> +
>      if (ngx_conf_full_name(cf->cycle, key, 1) != NGX_OK) {
>          return NGX_ERROR;
>      }
> 
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

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