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 <[email protected]> # 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 <[email protected]> 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 > [email protected] > http://mailman.nginx.org/mailman/listinfo/nginx-devel _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
