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

[Sam McKelvie]

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.

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

Sam

On Thu, Feb 4, 2016 at 10:29 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 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/
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel