[RFC] CONNECT and use_ssl cache_peer

Hello,

    I came upon a Squid proxy configured with "cache_peer ssl" (i.e.,
peer->use_ssl is true). The proxy forwards GETs to the peer using an SSL
connection because forward.cc code knows that "SSL peers" need an SSL
connection and establishes such a connection. This works well.

However, when the proxy receives a CONNECT request, it diverts the
request to tunnel.cc. Tunnel.cc is aware of peer selection logic but
lacks the code that establishes an SSL connection to SSL peers. Squid
establishes a regular TCP connection to the SSL peer. The SSL peer
receives a plain CONNECT request from Squid instead of the expected SSL
handshake and closes the connection with an error.

Here are my top three choices for the fix:

1) Teach tunnel.cc code to establish SSL connections to SSL peers, like
forward.cc does. This design is straightforward, but duplicates
significant (and sometimes rather complex) portions of forward.cc code.
For example, FwdState::initiateSSL() and FwdState::negotiateSSL() would
need to be duplicated (with the exception of SslBump code).

2) Remove peer selection (and SSL peer connection) code from tunnel.cc.
Pass CONNECT requests to forward.cc and allow forward.cc to call
tunnelStart() after selecting (and connecting to) the peer as usual.
This probably requires rewriting tunnel.cc to work with
clientStreams/storeEntries because forward.cc kind of relies on that
API. While I would like to see that kind of design eventually, I do not
want to rewrite so much just to fix this bug. And these more complex
APIs are likely to also make tunneling less efficient.

3) Extract [SSL] peer connection establishment code from tunnel.cc and
forward.cc into a new class. Allow tunnel.cc and forward.cc code to use
that new class to establish the SSL connection as needed, returning
control back to the caller (either tunnel.cc or forward.cc). This avoids
code duplication but SslBump and other code specific to forward.cc will
complicate a clean extraction of the forward.cc code into a stand-alone
peer-connection-establishing class.

My current plan is to try #3 to avoid code duplication but if it becomes
too complex, go for #1 and duplicate what is necessary. What do you
think? Any better ideas?

Thank you,

Alex.
P.S. Currently, ClientHttpRequest::processRequest() calls tunnelStart().
This is where request forwarding splits between tunnel.cc and forward.cc:

> /*
> * Identify requests that do not go through the store and client side stream
> * and forward them to the appropriate location. All other requests, request
> * them.
> */
> void
> ClientHttpRequest::processRequest()
> {
> if (request->method == METHOD_CONNECT && !redirect.status) {
> logType = LOG_TCP_MISS;
> getConn()->stopReading(); // tunnels read for themselves
> tunnelStart(this, &out.size, &al->http.code);
> return;
> }
>
> httpStart(); // calls forward.cc
> }
Received on Sat Sep 08 2012 - 03:54:28 MDT