[squid-dev] [RFC] simplifying ssl_bump complexity

[Alex Rousskov]

On 11/19/2016 07:06 PM, Amos Jeffries wrote:
> On 20/11/2016 12:08 p.m., Marcus Kool wrote:
>> 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.

It would be nice to prohibit truly impossible actions at the syntax
level, but I suspect that the only way to make that possible is to focus
on final actions [instead of steps] and require at *most* one ssl_bump
rule for each of the supported final actions:

  ssl_bump splice    ...rules that define when to splice...
  ssl_bump bump      ...rules that define when to bump...
  ssl_bump terminate ...rules that define when to terminate...
  # no other ssl_bump lines allowed!

The current intermediate actions (peek and stare) would have to go into
the ACLs. There will be no ssl_bump rules for them at all. In other
words, the admin would be required to _always_ write an equivalent of

  if (a1() && a2() && ...)
  then
      splice
  elsif (b1() && b2() && ...)
  then
      bump
  elsif (c1() && c2() && ...)
  then
      terminate
  else
      splice or bump, depending on state
      (or some other default; this decision is secondary)
  endif

where a1(), b2(), and other functions/ACLs may peek or stare as needed
to get the required information.

I am not sure such a change is desirable, but it is worth considering, I
guess.

Please note that I am ignoring the directives/actions naming issue for now.


AFAICT, the syntax proposed by Amos (i.e., making stepN mandatory) does
not solve this particular problem at all:

  # Syntactically valid nonsense:
  ssl_bump_step1 splice all
  ssl_bump_step2 bump all


and neither is yours:

  # Syntactically valid nonsense?
  tls_server_hello passthrough all
  tls_client_hello terminate all


>> 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.

I do not see why

  https_decryption off

is more intuitive (or more precise) than

  ssl_bump splice all

especially after you consider the order of directives.

Again, I am ignoring the naming issue for now. You may assume any name
you want for any directive or ACL.


>> 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.

I suspect that not looking at some SSL Hellos will always be needed
because some of those Hellos are not Hellos at all and it takes too much
time/resources for the SSL Hellos parser to detect some non-SSL Hellos.
Besides that, it is always nice to be able to selectively bypass the
complex Hello parser code in emergencies.


>> 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.

Just like naming, the defaults are probably not that important yet: If
we can agree on the new configuration approach, then we probably will be
able to agree on the defaults.

However, all proposals have to decide how to distinguish stare from peek
during step2. Those two intermediate actions result in different bytes
on the wire so they cannot be mapped to a single verb like "continue".


>> 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.

... if you use them correctly. However, the same is true for the current
directives so this is not really an illustration of improvement AFAICT.
For example, what does the following [mis]configuration do?

  https_decryption off
  tls_server_hello continue
  tls_server_hello terminate hacked_server
  https_decryption on
  tls_client_hello continue
  tls_client_hello passthrough banks

If I understand the proposals correctly, the above will probably splice
or perhaps bump everything but I am obviously not sure. I do not see an
an improvement as far as possible confusion/misunderstanding is concerned.

Again, I am ignoring the naming issue. If, for example, there is
consensus that "passthrough" is better than "splice" or "continue2" is
better than "stare" then we can, of course, add aliases and eventually
completely get rid of the old names. Let's focus on the functionality first.


Thank you,

Alex.