Re: [RFC] CONNECT and use_ssl cache_peer

On 09/07/2012 10:38 PM, Amos Jeffries wrote:
> On 8/09/2012 3:54 p.m., Alex Rousskov wrote:
>> 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?
>
> My vote is for #2 if you can do it. That needs to be done long-term
> anyways and will result in less time trying to upkeep separate pathways
> in tunnel.cc and forward.cc. The tunnel.cc pathway is already way behind
> on features like 1xx status, chunking, keep-alive and pipelined
> single-packet requests.

Hi Amos,

    Can you clarify how tunneling is related to chunking, keep-alive and
pipelined single-packet requests? It seems to me that all those HTTP
features are not compatible with CONNECT tunnels because once the tunnel
is established, the connection stops having any HTTP semantics. Are you
thinking about error responses to CONNECT requests? A successful CONNECT
response cannot be chunked (no body), kept alive (no way to determine
the end of the tunnel other than by closing the connection), or handle
more requests (same reason).

As for 1xx messages and error responses, I agree that we should handle
them better in response to Squid's CONNECT request, but we do not handle
them well now, and option #3 will share any future handling improvements
with tunnel.cc and forward.cc code.

Thank you,

Alex.

> After #2 is done ssl-bump should be able to become something like just
> another BodyPipe component in the chain handling the CONNECT request.
>
> Amos
Received on Sat Sep 08 2012 - 15:54:51 MDT