[squid-dev] [RFC] simplifying ssl_bump complexity

[Amos Jeffries]

On 20/11/2016 12:08 p.m., Marcus Kool wrote:
> 
> 
> On 11/19/2016 08:07 AM, Amos Jeffries wrote:
>> Since ssl_bump directive went in my original opinion of it as being too
>> complicated and confusing has pretty much been demonstrated as correct
>> by the vast amount of misconfigurations and failed attempts of people to
>> use it without direct assistance from those of us involved with its
>> design.
>>
>> Since we are also transitioning to a world where 'SSL' does not exist
>> any longer I think v5 is a good time to rename and redesign the
>> directive a bit.
>>
>> I propose going back to the older config style where each step has its
>> own directive name which self-documents what it does. That will reduce
>> the confusion about what is going on at each 'step', and allow us a
>> chance to have clearly documented default actions for each step.
>>
>> For example:
>>  tls_new_connection
>>   - default: peek all
>>   - or run ssl_bump check if that directive exists
>>
>>  tls_client_hello
>>   - default: splice all
>>   - or run ssl_bump check if that directive exists
>>
>>  tls_server_hello
>>   - default: terminate all
>>   - or run ssl_bump check if that directive exists
> 
> I like the intent of the proposal and the new directives tls_*.
> What currently makes configuration in Squid 3/4 difficult is
> the logic of 'define in step x what to do in the next step' and
> IMO this logic is the main cause of misunderstandings and
> incorrect configurations.  Also the terms 'bump' and 'splice'
> do not help ease of understanding.  Since Squid evolved and
> bumping changed from 3.3 - 3.4 - 3.5 to 4.x, and likely will
> change again in 5.x, there is an opportunity to improve
> things more than is proposed.
> There is also a difference in dealing with transparent intercepted
> connections and direct connections (browsers doing a CONNECT)
> which also causes some misunderstandings.
> The current ssl bump steps allow problematic configs where Squid
> bumps or stares in one step and to splice in an other step,
> which can be resolved (made impossible) in a new configuration syntax.
> 
> I propose to use a new logic for the configuration directives
> where 'define in step x what to do in the next step' is replaced
> with a new logic 'define in step x what to do _now_'.

>From reading the below I think you are mistaking what "now" means to
Squid. Input access control directives in squid.conf make a decision
about what action to do based on some state that just arrived.

For example:
 HTTP message just finished parsing -> check http_access what to do with it.
 HTTP reply message just arrived -> check http_reply_access what to do
with it.

Thus my proposal was along the lines of:
  client hello recieved -> check tls_client_hello what to do with it.
  server hello recieved -> check tls_server_hello what to do with it.


> 
> Below is a new proposal to attempt to make the configuration
> more intuitive and less prone for admin misunderstandings.
> 
> First the admin must define if there is any bumping at all.
> This could be done with
> https_decryption on|off
> This is similar to tls_new_connection peek|splice but much
> more intuitive.
> 
> Iff https_decryption is on:
> 
> 1) the "connection" step:
> When a browser uses "CONNECT <FQDN>" Squid does not need to make
> peek or splice decisions.
> When Squid intercepts a connection to "port 443 of <IP>" no peek
> or splice decision is made here any more.
> This step becomes obsolete in the proposed configuration.

I am still hoping that will happen. But also still getting pushback that
people want to terminate or splice without even looking at the
clear-text hello details.

> 
> 2) the "TLS client hello" step:
> When a browser uses CONNECT, Squid has a FQDN and does not need
> peeking a TLS client hello message. It can use the tls_client_hello
> directives given below.

Sadly this is not correct. Squid still needs to get the client hello
details at this point. They are needed to perform bump before the server
hello is received, and to "terminate with an error message" without
contacting a server.


> When Squid intercepts a connection, Squid always peeks to retrieve
> the SNI which is the equivalent of the FQDN used by a CONNECT.
> In this step admins may want to define what Squid must do, e.g.
> tls_client_hello passthrough aclfoo
> Note that the acl 'aclfoo' can use tls::client_servername and
> tls::client_servername should always have a FQDN if the connection
> is https.  tls::client_servername expands to the IP address if
> the SNI of an intercepted connection could not be retrieved.

What if the SNI contradicts the CONNECT message FQDN ?
What if a raw-IP in the CONNECT message (or TCP SYN) does not belong to
the server named in SNI ?

Squid would now be diverting the client transparently to a server other
than the one it expects and caching under that FQDN. But the server cert
would still authenticate as being the SNI host, so TLS cannot detect the
diversion.

The fake CONNECT's are a bit messy but IMHO we can only get rid of the
first one done for intercepted connections. Although that alone would
make both cases handle the same way.

> 
> For https connections with a client hello without the SNI extension:
> tls_client_hello passthrough|terminate aclbar
> where aclbar can contain tls::client_hello_missing_sni
> 
> For connections that do not use TLS (i.e. not a valid
> TLS client hello message was seen):
> tls_client_hello passthrough|terminate aclbar2
> where aclbar2 may contain tls::handshake_failure
> 
> To define that the TLS handshake continues, the config can contain
> tls_client_hello continue
> This is a basically a no-op and not required but enhances readability
> of a configuration.

This is getting complicated again. Avoiding the first (TCP SYN) access
control cycles will make ssl::server_name ACL contain either FQDN/SNI or
the actual server cert name. A default "none" or "-" value when no
FQDN/SNI is available should be sufficient for matching these clients.
And we should have that already I think, if not its a missing feature bug.

Also even "no-op" ACLs still require CPU time to assemble data for ACL
checking. So avoiding that kind of thing in configs is good.

> 
> 3) the "TLS server hello" step:
> Usually no directives are needed since rarely actions are taken
> based on the server hello message, so the default is
> tls_server_hello continue

Once Squid gets to this stage of processing "continue" could mean either
splice or bump, but only one of the two is possible.

Bump is not legal in a lot of situations but splice may not be possible.
So terminate is the only realistic default we can set for general use.
As you say, its uncommon to need it, and people who do tend to know what
they want done. So will configure that.


The handshake containing error signals or other protocols are different
decisions points. Remember Squid is deciding what to do with some data
that just arrived. If its going to tunnel/splice the server hello, there
is no point even evaluating what the protocol inside is, and error
alerts get delivered to the client as-is.

So the existing on_unsupported_protocol and sslproxy_cert_error to make
those handling decisions with 'ssl_error' type ACL to match the specific
problem are fine IMO.


> The tls_server_hello can be used to terminate specific connections.
> In this step many types of certificate errors can be detected
> and in the Squid configuration there must be a way to define
> what to do for specific errors and optionally for which FQDN.
> E.g. allow to define that connections with self-signed certificates
> are terminates but the self-signed cert for domain foo.example.com
> is allowed.  See also the example config below and the use of
> tls::server_servername.
> 
> What is left, is a configuration directive for connections
> that use TLS as an encryption wrapper but do not use HTTP
> inside the TLS wrapper:
> tls_no_http passthrough|terminate   # similar to on_unsupported_protocol
> 
> An example configuration looks like this:
> https_decryption on
> acl banks tls::client_servername .bank1.example.org
> acl no_sni tls::client_hello_missing_sni
> acl no_handshake tls::handshake_failure
> acl hacked_server tls::server_servername evil.example.com
> tls_client_hello passthrough banks
> tls_client_hello terminate no_sni
> tls_client_hello passthrough no_handshake
> tls_client_hello continue
> tls_server_hello terminate hacked_server
> tls_server_hello continue
> tls_no_http passthrough
> 
> The configuration directives as I proposed are IMO intuitive and
> leave very little room for misunderstandings.

Looks just like what I'm proposing except your config above lets clients
connect to servers with invalid TLS credentials (maybe not so intuitive?).

 acl banks ssl::server_name .bank1.example.org
 acl no_sni ssl::server_name "-"
 acl hacked_server ssl::server_name evil.example.com

 tls_client_hello splice banks
 tls_client_hello terminate no_sni
 tls_client_hello terminate hacked_server
 tls_client_hello peek all

 tls_server_hello terminate hacked_server
 tls_server_hello splice all

 # NP: "sslproxy_cert_error deny all" is default

 on_unsupported_protocol tunnel all


Amos