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

Sam McKelvie sam at surround.io
Wed Feb 10 22:38:43 UTC 2016


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?

~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.

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)
+{
+    size_t       len;
+    ngx_str_t    cert;
+    uintptr_t    escape;
+
+    s->data = NULL;
+    s->len = 0;
+
+    if (ngx_ssl_get_raw_certificate(c, pool, &cert) != NGX_OK) {
+        return NGX_ERROR;
+    }
+
+    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;
+
+    s->data = ngx_pnalloc(pool, len);
+    if (s->data == NULL) {
+        ngx_pfree(pool, cert.data);
+        return NGX_ERROR;
+    }
+    s->len = len;
+
+    ngx_escape_uri(s->data, cert.data, cert.len, NGX_ESCAPE_URI_COMPONENT);
+
+    ngx_pfree(pool, cert.data);
+
+    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);
 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 },
+
     { ngx_string("ssl_client_s_dn"), NULL, ngx_http_ssl_variable,
       (uintptr_t) ngx_ssl_get_subject_dn, NGX_HTTP_VAR_CHANGEABLE, 0 },


On Fri, Feb 5, 2016 at 7:58 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
>
> On Thu, Feb 04, 2016 at 01:38:48PM -0800, Sam McKelvie wrote:
>
>> I think it is your call if you want to make a breaking change to
>> $ssl_client_cert to URL encode it; I'd be happy to submit the
>> changeset if you would approve that, but I personally don't feel
>> comfortable breaking any existing applications that parse/decode the
>> certificate.
>
> No, certainly not something I want to be done.
>
>> So my suggestion now is to define a new $ssl_client_cert_url_encoded
>> variable that is the URL-encoded form of the raw PEM certificate. With
>> your approval I will submit a changeset for that...
>
> Yes, adding a variable with an URL-escaped versions looks like a
> way to go.  I disagree with the name you suggest though, I think
> that something like $ssl_client_escaped_cert would be more in line
> with $ssl_client_cert and $ssl_client_raw_cert variables we
> currently have and the ngx_escape_uri() function nginx uses
> internally.
>
> Some more background.  As of now we have:
>
> - $ssl_client_raw_cert - client cert in PEM format
> - $ssl_client_cert - client cert in PEM format with \t added
>
> At some distant point in the future we probably want to have:
>
> - $ssl_client_cert - client cert in PEM format
> - a way to urlescape() things, see
>   https://trac.nginx.org/nginx/ticket/52
>
> At this point, an escaped version of the client cert will be
> available as something like ${urlescape($ssl_client_cert)}.  All
> uses of client cert with tabs are expected to disappear.  There
> are a couple of problems though:
>
> - there are existing uses of $ssl_client_cert and
>   $ssl_client_raw_cert, breaking them would be bad;
>
> - we don't have urlescape() function in configs, and probably
>   won't have it in a near future.
>
> So we have to figure out some migration plan, e.g.:
>
> - introduce $ssl_client_escaped_cert, with urlescaped PEM cert;
>
> - introduce $ssl_client_tabbed_cert as an alias to
>   $ssl_client_cert (with PEM cert with tabs);
>
> - change $ssl_client_cert back to be raw cert (preserving
>   $ssl_client_raw_cert as a deprecated alias for some time);
>
> - do something with $ssl_client_tabbed_cert at some point, not
>   sure;
>
> - once urlescape() functionality is added, deprecate
>   $ssl_client_escaped_cert, suggesting to use
>   urlescape($ssl_client_cert) instead.
>
> Not sure if it's an optimal plan and if we are actually going to
> follow it, but introducing $ssl_client_escaped_cert looks like a
> more or less obvious 1st step, at least if we don't expect
> urlescape() to appear in the near future.
>
> --
> Maxim Dounin
> http://nginx.org/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel



More information about the nginx-devel mailing list