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