Proposed changeset to fix client cert from ngx_ssl_get_certificate passed as HTTP header value
mdounin at mdounin.ru
Thu Feb 4 18:29:28 UTC 2016
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
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,
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.
More information about the nginx-devel