Re: [PATCH] Squid host rewrite for intercepted https requests

[Amos Jeffries]

On 8/06/2012 9:23 p.m., Alexander Komyagin wrote:
Hello Alex!
On Thu, 2012-06-07 at 17:22 -0600, Alex Rousskov wrote:
On 06/07/2012 10:02 AM, Alexander Komyagin wrote:
Hi! I've found that Squid (as of 3.2.0.16) is still overwriting clients
requests to HTTP if they are intercepted:

src/client_side.cc:prepareTransparentURL():

snprintf(http->uri, url_sz, "http://%s%s";, /*conn->port->protocol,*/
host, url);

When I want to intercept https traffic this one really breaks the things
down. I have not deepened into the details, but it seems that everything
works fine (simultaneous http/https) when I use conn->port->protocol and
force protocol to http if it is NULL.

Patch is attached.
Hello Alexander,

     It looks like your patch is changing more than just the lines that
format the URL. We made a similar but more compact change for the
bump-server-first project:

  static void
  prepareTransparentURL(ConnStateData * conn, ClientHttpRequest *http, char 
*url,
  const char *req_hdr)
  {
      char *host;
      char ipbuf[MAX_IPSTRLEN];

      if (*url != '/')
          return; /* already in good shape */

      /* BUG: Squid cannot deal with '*' URLs (RFC2616 5.1.2) */
      // BUG 2976: Squid only accepts intercepted HTTP.

      if ((host = mime_get_header(req_hdr, "Host")) != NULL) {
          int url_sz = strlen(url) + 32 + Config.appendDomainLen +
                       strlen(host);
          http->uri = (char *)xcalloc(url_sz, 1);
-        snprintf(http->uri, url_sz, "http://%s%s";, /*conn->port->protocol,*/ 
host, url);
+        snprintf(http->uri, url_sz, "%s://%s%s", conn->port->protocol, host, 
url);
          debugs(33, 5, "TRANSPARENT HOST REWRITE: '"<<  http->uri<<"'");
      } else {
          /* Put the local socket IP address as the hostname.  */
          int url_sz = strlen(url) + 32 + Config.appendDomainLen;
          http->uri = (char *)xcalloc(url_sz, 1);
          
http->getConn()->clientConnection->local.ToHostname(ipbuf,MAX_IPSTRLEN),
-        snprintf(http->uri, url_sz, "http://%s:%d%s";,
-                 // http->getConn()->port->protocol,
+        snprintf(http->uri, url_sz, "%s://%s:%d%s",
+                 http->getConn()->port->protocol,
                   ipbuf, http->getConn()->clientConnection->local.GetPort(), 
url);
          debugs(33, 5, "TRANSPARENT REWRITE: '"<<  http->uri<<  "'");
      }
  }

Have you seen the port protocol being NULL? It feels like that should
not be possible. If it is never NULL, I think the above changes are
preferred.
Nope. Never seen. But the guy who reported *BUG 2976* certainly did. See
http://www.squid-cache.org/mail-archive/squid-dev/201103/0020.html

As did several other people around that period. Enough to make me add 
that workaround to stable releases. It happens whenever Squid is 
reconfigured. The "conn->port" is a pointer to the SquidConfig 
http(s)_port state which gets erasesd and rebuilt, problem is the object 
is reallocated in a different location and the old object being pointed 
to by this fixed pointer has its string unset. NOTE: when MemPools is 
disabled this is an invalid pointer dereference crash rather than a 
silent "NULL".

The easiest proper fix is to copy the http_port protocol= string into 
ConnStateData member for use. ie conn->transportProtocol.

The harder one I was working towards is to RefCount the new 
AnyP::PortCfg objects at which point they will persist as long as that 
ConnStateData needs to use them. Which is tricky since they are 
apparently CBDATA which is not compatible with RefCounting.

Amos