On Thu, 2008-04-10 at 00:56 +1200, Amos Jeffries wrote:
> Alex Rousskov wrote:
> > On Tue, 2008-04-08 at 17:41 +0900, S.KOBAYASHI wrote:
> >
> >> I'm handling ipv6 functions on ICAP, so I found a curious point.
> >> ICAP capable is ON and use ipv6 URI with SSL from the browser such as
> >> https://[ 2001:5c0:9a37:0:21d:9ff:fe68:333a]/.
> >> CONNECT header in the ICAP request-line which was sent from squid to ICAP
> >> server is "0.0.7.209:443 HTTP1.1".
> >> I think it should be included ipv6 address, not "0.0.7.209".
> >> I'm using squid version Squid-3.HEAD-20080320.
> >
> > Please file a "IPv6 IPs in ICAP URLs" bug report with bugzilla,
> > especially if nobody sends you a fix for this bug here.
> >
> > Thank you,
> >
> > Alex.
> >
>
> (Moving to Squid-dev as this should be discussed there.)
>
> Alex, is ICAP not using the built-in squid URL Parser and IPAddress to
> handle URLs?
It does, but I believe this is about request generation, not parsing.
The generation code uses "built-in" HttpMsg::packInto:
> void ICAPModXact::packHead(MemBuf &httpBuf, const HttpMsg *head)
> {
> Packer p;
> packerToMemInit(&p, &httpBuf);
> head->packInto(&p, true);
> packerClean(&p);
> }
So, either the bug is in core packInto or ICAP encapsulation code that
calls it. The latter is not trivial because the core does not support
true HTTP message header cloning. We have to do it ourselves to remove
hop-by-hop headers from the virgin HTTP header:
> HttpRequest* new_request = new HttpRequest;
> urlParse(old_request->method, old_request->canonical, new_request);
> new_request->http_ver = old_request->http_ver;
> inheritVirginProperties(*new_request, *old_request);
> headClone = new_request;
>
> ... clone all header fields using HttpHeaderEntry::clone()
>
> headClone->header.removeHopByHopEntries();
> packHead(httpBuf, headClone);
So, if packInto is not buggy, then either urlParse is or the above
cloning code is missing something. FWIW, the HTTP Request line is
produced by HttpRequest::packableURI(true), which calls urlCanonical(),
which does this for CONNECT requests:
> case METHOD_CONNECT:
> snprintf(urlbuf, MAX_URL, "%s:%d", request->GetHost(), request->port);
> break;
So, GetHost() apparently returns garbage. As far as I can tell, the host
member returned by GetHost is set by urlParse(). I would start by adding
a Must() around urlParse call above and then figuring out why it does
not set host properly for CONNECT.
HTH,
Alex.
