[squid-dev] [PATCH] splicing resumed sessions

[Alex Rousskov]

On 03/20/2015 12:11 PM, Amos Jeffries wrote:
> On 21/03/2015 4:35 a.m., Alex Rousskov wrote:
>> On 03/20/2015 02:06 AM, Amos Jeffries wrote:
>>> On 18/03/2015 6:21 a.m., Tsantilas Christos wrote:
>>>> This patch adds the "ssl_bump_resuming_sessions" directive that controls
>>>> SslBump behavior when dealing with "resuming SSL/TLS sessions". Without
>>>> these changes, SslBump usually terminates all resuming sessions with an
>>>> error because such sessions do not include server certificates,
>>>> preventing Squid from successfully validating the server identity.


> Squid is behaving wrong right now. Fix that.

Yes, the proposed patch "fixes that".


> I dont see any need for the new directive to exist

We can remove the configuration option. The option was added to minimize
the chance that you will object to the patch because, without the new
option, the old "terminate" behavior would be lost. Clearly, my plan to
avoid that discussion has failed! Removing the configuration option is
totally fine with me because nobody has asked for it [yet]. We can
always add it later if needed.


> The validation I'm talking about is the spec requirement that we dont
> allow the following scenario:
> 
>   CONNECT example.com:443 HTTP/1.1
>   ...
>   ClientHello SNI:  example.org


I still think this part of the discussion is out of scope, but is there
really a spec requirement that talks about comparing CONNECT- and
SNI-provided server names? Where? CONNECT is about blind TCP *tunnels*.
SNI is about becoming an *end point* of a TLS connection. Those two
things seem mutually exclusive to me.


> * if the CONNECT and SNI both contain different domains the
> specification says SSL "MUST terminate" with a specified reject error.

Which specification? RFC 2818 does not mention CONNECT. It contains some
requirements for HTTP clients, but Squid is not an HTTP client in this
issue scope; Squid is a TCP client.


>   - CONNECT having a domain means we are in the RFC 2818 scenario of
> TLS-over-HTTP.

No, CONNECT means "establish a TCP tunnel by splicing client and server
TCP connections together". CONNECT does not imply SSL or TLS. When Squid
bumps a secure connection, then Squid becomes subject to various SSL and
TLS rules. Here, we are not bumping anything, just splicing at TCP level.


>   - given what Squid is doing, we are allowed to just splice always and
> let the endpoints do the error stuff. BUT, that is a waste of resources
> when we could abort early and log a nice alert.

Yes, there are cases where "doing error stuff" in Squid is useful, and
there are cases where it is harmful. There are cases where logging a
"nice alert" is useful and cases where it is at best pointless (unless
the client gets an error message as well). This patch does not try to
improve any of that "doing error stuff". I am certainly not against
adding [more] SNI checks, but not as a part of the fix for an
SNI-unrelated error.


> In all other cases we can allow the splice to resume with no loss of
> safety. The endpoints will do (or not) anything they would do anyway
> without the proxy.

I do not think the proposed changes _introduce_ a safety loss in *any*
case. They just make session resumption work in the presence of a
splicing Squid. Could Squid check more things? Yes! Are those additional
checks both trivial to add and relevant to this fix? No.


> So ...
>  if we allow an admin directive it would be to force terminate when
> resume was otherwise allowed.

Yes.


>  1) Do you have any existing need for that?

None other than avoiding this discussion, which has already failed to work.


>  2) Would not "ssl_bump terminate blah" be a better way to express the
> policy?

Probably not. We have considered that design, but most ssl_bump rules
rely on information unavailable when sessions are resumed. If we force
admins to use ssl_bump to control which resuming sessions are
terminated, the ssl_bump rules become needlessly complex and difficult
to write correctly.


> I think we should fix the currently broken Squid behaviour without a new
> directive.

Sounds good to me! If we do not hear otherwise, we will commit the
proposed patch without the new [already optional] directive.


Thank you,

Alex.