ACL cleanup and failure propagation

Hello,

    While running more ACL tests with trunk, I discovered another angle
where old and trunk ACLs differ. The difference stems from the same
"failure propagation" effects we have discussed before, but I did not
realize that the effects go beyond a single rule: If one ACL fails,
Squid stops checking all ACL rules for the given option because Squid
cannot tell whether it should proceed (as if there was no match) or stop
(as if there was a match).

Here are a few examples to illustrate all known differences.

In all these examples, lets assume that all "user*" ACLs fail for one
reason or another (e.g., authentication information was required to
identify a specific user but was not available because the user has not
authenticated yet or maybe the DNS server crashed).

The title of each example tries to capture the admin intent.

ex1: Allow all except Alice (compact version)

  option allow !userAlice

  old ACL check code will result in: allow
  new ACL check code will result in: failure

ex2: Allow all except Alice (explicit version)

  option deny userAlice
  option allow all

  old code: allow
  new code: failure

ex3: Deny all except Alice (compact version)

  option deny !userAlice

  old code: deny
  new code: failure

ex4: Deny all except Alice (explicit version)

  option allow userAlice
  option deny all

  old code: deny
  new code: failure

ex5: Allow all except Greens (but do allow Bob Green)

  option allow userBobGreen
  option deny userGreen
  option allow all

  old code: allow
  new code: failure

In all of the above, strictly speaking, the failure is the right outcome
IMO because Squid cannot tell whether allowing or denying the
transaction or option would match the admin intent (i.e., whether it
should be allowed or denied based on available information).

However, the current trunk behavior makes it impossible to support at
least one of the following two usage cases, depending on what Squid does
when the ACL check fails.

ex6: Deny Bob. Allow all others. Allow if you cannot tell who this is.
ex7: Allow Bob. Deny all others. Deny if you cannot tell who this is.

As soon as my squid.conf mentions userBob in the ACLs, the trunk code
will fail the check. There is no way to express the "if you cannot tell
who this is" condition in ACLs. If the default Squid action on check
failures does not correspond to what the admin wants, the admin cannot
configure Squid correctly.

If we want to allow for such "if something failed" rules (and I think we
must!), I see a few basic options:

1) Go back to how the old code used to work. Yes, many Squids will
remain misconfigured and the admins will not even know that. Yes, it may
become increasingly difficult to add new ACL operations/features
correctly. Yes, this still requires complex code changes. But there will
be no upgrade headaches as all old use cases will be supported.

2) Add explicit "preconditions are met" ACLs that will not fail but will
mismatch when things go wrong. For example:

ex6: Deny Bob. Allow all others. Allow if you cannot tell who this is.

    option deny userKnown userBob
    option allow all

    future code: matches the intent because userBob is not evaluated
        and, hence, does not fail if the user is unknown. The userKnown
        ACL mismatches and Squid proceeds to the next rule.

This approach works, but it would be very difficult to implement ACLs
that cover all "preconditions are met" conditions _and_ it will be a
significant performance penalty too because many conditions would have
to be evaluated more often (e.g., once for userKnown and once for userBob).

3) Make individual ACLs configurable via a -x "throw on failures" ACL
parameter:

    # userAlice will throw on failures (the check stops on failures)
    acl userAlice acl_foo -x ...

ex6: Deny Bob. Allow all others. Allow if you cannot tell who this is.

    # userBob will mismatch on failures (old behavior by default)
    acl userBob acl_foo ...
    option deny userBob
    option allow all

while other examples can be made to work as intended if one adds -x
options to all user* ACLs, covering new use cases that old code (and
option #1) cannot currently cover.

Please note that (3) requires (1) as the first step. Thus, it looks like
we should do (1) and then someday extend it to (3).

Any better ideas?

Thank you,

Alex.
Received on Fri Jun 29 2012 - 23:22:36 MDT