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

[Maxim Dounin]

Hello!

On Tue, Jul 22, 2014 at 03:16:35PM +0400, Dmitrii Pichulin wrote:

> # HG changeset patch
> # User Dmitrii Pichulin <pdn at cryptopro.ru>
> # Date 1406021876 -14400
> #      Tue Jul 22 13:37:56 2014 +0400
> # Node ID 638389b21e0e1522ed8b8205012f5af562dc50c7
> # Parent  63d7d69d0fe48e030ff9fc520c7036dbd1ebc13f
> allow to use engine keyform for server private key

The patch is built against an old version, and there are 
conflicting changes in nginx 1.7.3.

> 
> diff -r 63d7d69d0fe4 -r 638389b21e0e src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c	Fri Jun 20 12:55:41 2014 +0400
> +++ b/src/event/ngx_event_openssl.c	Tue Jul 22 13:37:56 2014 +0400
> @@ -257,11 +257,31 @@
>  
>  ngx_int_t
>  ngx_ssl_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
> -    ngx_str_t *key)
> +    ngx_str_t *key, ngx_str_t *keyform, ngx_str_t *engine)
>  {
> -    BIO     *bio;
> -    X509    *x509;
> -    u_long   n;
> +    BIO        *bio;
> +    X509       *x509;
> +    u_long      n;
> +    ngx_uint_t  ssl_use_engine_keyform = 0;
> +
> +    if (keyform->len) {
> +
> +        if (ngx_strcmp(keyform->data, "ENGINE") == 0) {
> +            ssl_use_engine_keyform = 1;
> +
> +        } else if (ngx_strcmp(keyform->data, "PEM") != 0) {
> +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                               "invalid parameter: %V", keyform);
> +            return NGX_ERROR;
> +        }
> +    }
> +
> +    if (ssl_use_engine_keyform && engine->len == 0) {
> +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                             "no \"ssl_certificate_engine\" is defined "
> +                             "while \"ssl_certificate_keyform\" is \"ENGINE\"");
> +            return NGX_ERROR;
> +    }
>  
>      if (ngx_conf_full_name(cf->cycle, cert, 1) != NGX_OK) {
>          return NGX_ERROR;

As previously suggested, it should be "engine=" parameter of the 
ssl_certificate_key directive and/or some specific path prefix to 
load a key from an engine (like "engine:<foo>:<keyid>" instead of 
a file name), see the thread here:

http://mailman.nginx.org/pipermail/nginx-devel/2014-March/005114.html

There is no need to introduce such a number of directives - there 
is no real need for this, and it's confusing for users.

> @@ -344,17 +364,51 @@
>  
>      BIO_free(bio);
>  
> -    if (ngx_conf_full_name(cf->cycle, key, 1) != NGX_OK) {
> -        return NGX_ERROR;
> -    }
> -
> -    if (SSL_CTX_use_PrivateKey_file(ssl->ctx, (char *) key->data,
> -                                    SSL_FILETYPE_PEM)
> -        == 0)
> -    {
> -        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> -                      "SSL_CTX_use_PrivateKey_file(\"%s\") failed", key->data);
> -        return NGX_ERROR;
> +    if (ssl_use_engine_keyform) {
> +        EVP_PKEY   *pkey;
> +        ENGINE     *e;
> +

Please define all variables at function start.

> +        e = ENGINE_by_id((const char *) engine->data);

There is no need to use "const" in the cast.

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

Style: there should be a space between "(const char *)" and 
"key->data".  (Also, there is no need to use "const" in the cast.)

> +
> +        ENGINE_free(e);
> +
> +        if (!pkey) {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                          "ENGINE_load_private_key(\"%s\") failed", key->data);
> +            return NGX_ERROR;
> +        }

Error stack may be changed by the ENGINE_free().

Additinally, it looks like ENGINE_free() can return errors, and it 
may be a good idea to check them.

> +
> +        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", key->data);

Style: it may be a good idea to break the line into two here.

> +            EVP_PKEY_free(pkey);
> +            return NGX_ERROR;
> +        }
> +
> +        EVP_PKEY_free(pkey);
> +
> +    } else {

It should be better to just return here and hence avoid 
indentation changes on the default code path.

> +
> +        if (ngx_conf_full_name(cf->cycle, key, 1) != NGX_OK) {
> +            return NGX_ERROR;
> +        }
> +
> +        if (SSL_CTX_use_PrivateKey_file(ssl->ctx, 
> +                                        (char *) key->data, SSL_FILETYPE_PEM)
> +            == 0)
> +        {
> +            ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                       "SSL_CTX_use_PrivateKey_file(\"%s\") failed", key->data);
> +            return NGX_ERROR;
> +        }
>      }
>  
>      return NGX_OK;

[...]

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