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

[Maxim Dounin]

Hello!

On Thu, Feb 04, 2016 at 09:17:31AM -0800, Sam McKelvie wrote:

> > Hello!
> >
> > On Wed, Feb 03, 2016 at 03:38:13PM -0800, Sam McKelvie wrote:
> >
> >> The ngx_ssl_get_certificate() changes ?\n? to ?\n\t? in the returned PEM string in an effort to make
> >> the string usable as an HTTP header value with $ssl_client_cert. However, bare ?\n? (without a preceding ?\r?) is passed
> >> along as ?\n\t". This causes some HTTP servers (including node/express) to hang up. This changeset
> >> fixes the problem by replacing occurrences of ?\n? that have no preceding ?\r? with "\r\n\t".
> >>
> >> Tested with node.js/express and nginx-tests.
> >>
> >> I should note that a similar solution was proposed at https://forum.nginx.org/read.php?29,249804,249833 <https://forum.nginx.org/read.php?29,249804,249833>, but the thread never went anywhere.
> >> This solution is slightly more paranoid with edge cases and does not insert extra ?\r? characters if they are already present.
> >
> > IMHO, header line folding is wrong enough to don't bother with
> > trying to fix this.  It doesn't work in way too many cases
> > including with nginx itself, and it is deprecated by RFC7230.
> >
> > Much better approach would be to switch to something different -
> > may be just properly urlencoded $ssl_client_raw_cert, or plain
> > base64 without any newlines, or whatever.
> >
> I tend to agree. However, nix_ssl_get_certificate() is already folding
> the certificate for HTTP headers by inserting '\t' at the beginning of
> each line, so that seems to be its primary purpose. I can’t change
> that function to return anything that is not itself a valid PEM
> certificate, or it would certainly break any existing applications
> that parse the results. So unless you think it is OK to tweak it to
> insert '\r' as well,  I think you are saying this function is just
> permanently unusable for the purposes of passing a certificate in a
> header.

Current implementation is usable where it is currently used.  Most 
servers happen to parse "\n" without "\r" just fine.  RFC2616 
explicitly recommends applications to accept bare LF,
http://tools.ietf.org/html/rfc2616#section-19.3:

   The line terminator for message-header fields is the sequence CRLF.
   However, we recommend that applications, when parsing such headers,
   recognize a single LF as a line terminator and ignore the leading CR.

On the other hand, any changes, even minor ones like adding "\r", 
may break existing applications who use $ssl_client_cert.  And if 
we are goint to introduce changes - it's better to change it 
something more useable than header folding.

> Would you be more amenable to defining a new "$ssl_base64_client_cert"
> or “$ssl_escaped_client_cert” variable that is a properly encoded PEM
> string? Would you prefer base64 (which would greatly increase the
> size) or a quoted-string for with backslashes to escape control
> characters? I would be willing to submit such a changeset…

For sure base64-encoded PEM is not something we want, that would 
be double base64 encoding and makes no sense.  Base64-encoded DER 
(== PEM without header/footer/newlines) may be an option.

I tend to think that we want to use URI escaping here.  We already 
do this in mail for Auth-SSL-Cert header, with minimal escaping.  
As we don't know context for the variable - it probably would be a 
better option to escape it as an URI component.

Tricky part here is to understand what we are going to do with the 
$ssl_client_cert variable.  I don't like the idea of preserving it 
in "PEM with tab added for header folding" forever.

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