Re: NTLM auth issue due to patch introduced in squid-3.1.7

On 30/06/2012 2:15 a.m., Jiri Skala wrote:
> Hi all,
> I've reproduced an issue that is caused due to following squid-3.1.7
> patch
>
> http://www.squid-cache.org/Versions/v3/3.1/changesets/squid-3.1-10067.patch
>
>
> The reproducer contains a web site in IIS 7.5 that requires NTLM
> authentication. IE8 on Windows isn't able to authenticate when the
> traffic is handled by squid-3.1.7+. I have an info the issue depends on
> the Windows system (win7 is problematic but winXP works fine, i've used
> winServer).
>
> Reverting 'Connection' header back to 'Proxy-Connection' header fixes
> the issue (see patch below). I see the 'Connection' header is RFC
> compliant unfortunately no every software manufacturer is aware of
> necessity to be RFC compliant.

There is no reversion here. Squid-3.1-10067 is not the beginning of the
problem, it was a regression fix.

Squid prior to 3.1 never emitted Proxy-Connection. Only accepted it. I
made the mistake of making 3.1 emit it and was soundly corrected by a
bunch of people having problems. r10067 was the outcome.

I think you will find the issue your reporter is observing is from a
build of Squid with http-violations disabled. Such builds will obey the
specification as strictly as possibly, even at cost of breaking some
non-compliant software (which explains why --enable-http-violations is
default for general use). For Proxy-Connection that means the build will
ignore incoming Proxy-Connection, just erasing it on the relayed request
due to the safety issues of ignoring it completely. The overall result
is the browsers which wrongly depend on it alone to keep-alive an
HTTP/1.0 connection will recieve back Connection:close from Squid.

The second portion of your patch makes Squid do
Proxy-Connection:keep-alive even if --disable-http-violations has been
configured. Squid prior to 3.1.7 used to do that, but later releases
have it hidden behind HTTP-violations. Despite that there are hacks in
Squid to force emit Connection:keep-alive when NTLM of Negotiate is
used, so there should really be no reason to emit it anyway. BUT, we can
echo useless Proxy-Connection:keep-alive back at IE as a hack if that helps.

>
> I see the first element of the patch as the most controversial. What do
> you think about the patch bellow? Any other tips, comments?

The first part of the patch creates a bit of quite dangrous behaviour.
IIS has no reason to handle Squid traffic any different to any other
RFC2616 compliant agent. So there is good reason for it to accept
Connection:keep-alive, even assuming it considers them "broken" clients.
The older Squid versions never emitted Proxy-Connection anyway, so
unless you have evidence to show this is required, myself and a bunch of
people in the IETF (including at least one senior IE developer) will be
vetoing that particular change.

Amos

>
> Thank you for your answer in advance.
>
> Best regards
>
> Jiri
>
>
> =====================================================
> diff -up squid-3.1.10/src/client_side_reply.cc.http10
> squid-3.1.10/src/client_side_reply.cc
> --- squid-3.1.10/src/client_side_reply.cc.http10 2010-12-22
> 06:46:56.000000000 +0100
> +++ squid-3.1.10/src/client_side_reply.cc 2012-06-29
> 13:05:50.535114802 +0200
> @@ -1447,7 +1447,10 @@ clientReplyContext::buildReplyHeader()
> hdr->delById(HDR_VIA);
> hdr->putStr(HDR_VIA, strVia.termedBuf());
> }
> - /* Signal keep-alive or close explicitly */
> + /* Signal keep-alive if needed */
> + if (!http->flags.accel && !http->flags.intercepted)
> + hdr->putStr(HDR_PROXY_CONNECTION,
> request->flags.proxy_keepalive ? "keep-alive" : "close");
> +
> hdr->putStr(HDR_CONNECTION, request->flags.proxy_keepalive ?
> "keep-alive" : "close");
>
> #if ADD_X_REQUEST_URI
> diff -up squid-3.1.10/src/http.cc.http10 squid-3.1.10/src/http.cc
> --- squid-3.1.10/src/http.cc.http10 2010-12-22 06:46:56.000000000
> +0100
> +++ squid-3.1.10/src/http.cc 2012-06-29 10:09:41.856239753 +0200
> @@ -1729,7 +1729,11 @@ HttpStateData::httpBuildRequestHeader(Ht
>
> /* maybe append Connection: keep-alive */
> if (flags.keepalive) {
> - hdr_out->putStr(HDR_CONNECTION, "keep-alive");
> + if (hdr_in->has(HDR_PROXY_CONNECTION)) {
> + hdr_out->putStr(HDR_PROXY_CONNECTION, "keep-alive");
> + } else {
> + hdr_out->putStr(HDR_CONNECTION, "keep-alive");
> + }
> }
>
> /* append Front-End-Https */
> diff -up squid-3.1.10/src/HttpHeaderTools.cc.http10
> squid-3.1.10/src/HttpHeaderTools.cc
> --- squid-3.1.10/src/HttpHeaderTools.cc.http10 2010-12-22
> 06:46:56.000000000 +0100
> +++ squid-3.1.10/src/HttpHeaderTools.cc 2012-06-29 10:09:41.857250002
> +0200
> @@ -148,15 +148,12 @@ httpHeaderHasConnDir(const HttpHeader *
> int res;
> /* what type of header do we have? */
>
> -#if HTTP_VIOLATIONS
> if (hdr->has(HDR_PROXY_CONNECTION))
> list = hdr->getList(HDR_PROXY_CONNECTION);
> + else if (hdr->has(HDR_CONNECTION))
> + list = hdr->getList(HDR_CONNECTION);
> else
> -#endif
> - if (hdr->has(HDR_CONNECTION))
> - list = hdr->getList(HDR_CONNECTION);
> - else
> - return 0;
> + return 0;
>
> res = strListIsMember(&list, directive, ',');
>
>
Received on Mon Jul 02 2012 - 11:26:42 MDT