Hello!

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

> # HG changeset patch
> # User Dmitrii Pichulin <pdn@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/

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel