Proposed changeset to fix client cert from ngx_ssl_get_certificate passed as HTTP header value

[Maxim Dounin]

Hello!

On Wed, Feb 10, 2016 at 02:38:43PM -0800, Sam McKelvie wrote:

> OK, sorry for the delay.
> 
> Here is a changeset that implements $ssl_client_escaped_certificate,
> following your latest comments. It works well with node/express. OK
> with this?

See review below.

> 
> ~Sam
> 
> 
> # HG changeset patch
> # User Sam McKelvie <sam at surround.io>
> # Date 1455143619 28800
> #      Wed Feb 10 14:33:39 2016 -0800
> # Node ID f3f2dbd8340f04adccd805ff5a8547e10e6fa874
> # Parent  e0d7c2f718515d4c48e5ed2a0643294da60cf2ba
> Define a new $ssl_client_escaped_certificate variable that is the
> URL-encoded form of the raw PEM client certificate. This variable
> is always appropriate for inclusion in proxy_set_header values,
> unlike the $ssl_client_cert variable, which includes '\n'
> characters that are not prefixed by '\r', breaking some http
> servers (e.g., node/express).
> 
> Per Maxim, $ssl_client_cert should eventually be deprecated. Users
> of $ssl_client_cert should move to ssl_client_scaped_certificate.
> 
> Tested with node/express and nginx-tests.

This should be rewritten to contain less typos and better match 
recommended format, see some hints here:

http://nginx.org/en/docs/contributing_changes.html

Additionally, instead of focusing on badly-written servers which 
don't handle "\n" you may want to focus on header folding being 
deprecated in general.

> 
> diff -r e0d7c2f71851 -r f3f2dbd8340f src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c Fri Feb 05 14:02:54 2016 +0300
> +++ b/src/event/ngx_event_openssl.c Wed Feb 10 14:33:39 2016 -0800
> @@ -3273,6 +3273,42 @@
> 
> 
>  ngx_int_t
> +ngx_ssl_get_escaped_certificate(ngx_connection_t *c, ngx_pool_t *pool,
> +                                ngx_str_t *s)

Style: function definition/declaration arguments are wrapped with 
4-space indent.

> +{
> +    size_t       len;
> +    ngx_str_t    cert;
> +    uintptr_t    escape;
> +
> +    s->data = NULL;
> +    s->len = 0;

These initializations look unneeded.
Both s->data and s->len are always set if NGX_OK is returned.

> +
> +    if (ngx_ssl_get_raw_certificate(c, pool, &cert) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +

You may want to add (cert.len == 0) special case here, much like 
in ngx_ssl_get_certificate().

> +    escape = ngx_escape_uri(NULL, cert.data, cert.len,
> +                            NGX_ESCAPE_URI_COMPONENT);
> +
> +    /* Each escaped character adds 2 characters to result */
> +    len = cert.len + 2 * escape;

Just using

    escape = 2 * ngx_escape_uri(...);

or even

    len = cert.len + 2 * ngx_escape_uri(...);

as in other places is much better idea than adding an obvious 
comment.

> +
> +    s->data = ngx_pnalloc(pool, len);
> +    if (s->data == NULL) {
> +        ngx_pfree(pool, cert.data);

There is no need to free small/non-recurring allocations from 
pool.  Especially when returning errors - as this usually means 
that nginx will just terminate the request and will free the pool.

> +        return NGX_ERROR;
> +    }
> +    s->len = len;

Something like:

    s->len = len;
    s->data = ngx_pnalloc(pool, len);
    if (s->data == NULL) {
        return NGX_ERROR;
    }

as in ngx_ssl_get_certificate() should be a bit more readable.

> +
> +    ngx_escape_uri(s->data, cert.data, cert.len, NGX_ESCAPE_URI_COMPONENT);
> +
> +    ngx_pfree(pool, cert.data);

There is no need to free small/non-recurring allocations from 
pool.

> +
> +    return NGX_OK;
> +}
> +
> +
> +ngx_int_t
>  ngx_ssl_get_subject_dn(ngx_connection_t *c, ngx_pool_t *pool, ngx_str_t *s)
>  {
>      char       *p;
> diff -r e0d7c2f71851 -r f3f2dbd8340f src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h Fri Feb 05 14:02:54 2016 +0300
> +++ b/src/event/ngx_event_openssl.h Wed Feb 10 14:33:39 2016 -0800
> @@ -180,6 +180,8 @@
>      ngx_str_t *s);
>  ngx_int_t ngx_ssl_get_certificate(ngx_connection_t *c, ngx_pool_t *pool,
>      ngx_str_t *s);
> +ngx_int_t ngx_ssl_get_escaped_certificate(ngx_connection_t *c,
> ngx_pool_t *pool,
> +    ngx_str_t *s);

The patch seems to be currupted by your mail client here.

>  ngx_int_t ngx_ssl_get_subject_dn(ngx_connection_t *c, ngx_pool_t *pool,
>      ngx_str_t *s);
>  ngx_int_t ngx_ssl_get_issuer_dn(ngx_connection_t *c, ngx_pool_t *pool,
> diff -r e0d7c2f71851 -r f3f2dbd8340f src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c Fri Feb 05 14:02:54 2016 +0300
> +++ b/src/http/modules/ngx_http_ssl_module.c Wed Feb 10 14:33:39 2016 -0800
> @@ -292,6 +292,9 @@
>        (uintptr_t) ngx_ssl_get_raw_certificate,
>        NGX_HTTP_VAR_CHANGEABLE, 0 },
> 
> +    { ngx_string("ssl_client_escaped_cert"), NULL, ngx_http_ssl_variable,
> +      (uintptr_t) ngx_ssl_get_escaped_certificate,
> NGX_HTTP_VAR_CHANGEABLE, 0 },
> +

And here.

>      { ngx_string("ssl_client_s_dn"), NULL, ngx_http_ssl_variable,
>        (uintptr_t) ngx_ssl_get_subject_dn, NGX_HTTP_VAR_CHANGEABLE, 0 },
> 

[...]

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