Re: [PATCH] AND and OR ACLs v15

On 15/05/2013 4:10 a.m., Alex Rousskov wrote:
> On 05/14/2013 01:34 AM, Amos Jeffries wrote:
>
>> I'm still stuck on muliple all-of lines ORing together for the AND acl
>> type. It really is much simpler to think of and describe "acl X foo ..."
>> as X being a test of a set of values with operation foo applied to the
>> whole set.
> Yes, but ANDing all-of lines leads to serious inconsistency because all
> other ACL types OR their lines.

Those other types are also OR'ing the values along their lines. I see a
bigger inconsistency by changing the behaviour of one ACL depending on
whether we spread a set over multiple lines or single lines.
   ** note that some config generators may combine multiple lines into a
single line or unpack single lines into lists of lines. For ease of
editing, or simply becasue teh editing GUI box used does not allow long
lines (like squid.conf line limits BTW). This will seriously screw up
all_of behaviour if we toggle AND/OR between lines.

> We would have to explain that
> inconsistency too, which would be even harder and which would confuse
> some admins just as much.

I disagree it is simpler to explain:
  "acl directive lists a set of values which the current transaction is
to be tested against depending on the acl type.

... The all-of ACL type acts like AND on its set of values and all of
them must match for it to match."

versus trying to explain why "all-of" performs OR operation when you
spread its arguments across multiple lines.

We already document:
    "Every access list definition must begin with an aclname and acltype,
     followed by either type-specific arguments or a quoted filename that
     they are read from."

I note that if one is to put the all-of ACL names into a file (like
webmin and chef do). That you only get to use one all-of line per ACL
name and also that they cannot include several user-specific files into
the one named ACL anyway and have to fallbak on the second syntax
outlined below...

> Besides being more consistent, the proposed
> implementation results in more succinct rules because it does not
> require an extra any-of "aggregation" line in many cases.
>
> Does anybody else have an opinion? The choice is between:
>
> # Implemented in the proposed patch:
> # What should we deny? One "bad" acl line per condition:
> acl bad all-of !authorized access_to_sensitive_area
> acl bad all-of !userA access_to_userA_area
> acl bad all-of !userB access_to_userB_area
> http_access deny bad
>
> versus
>
> # Suggested as an easier-to-grok alternative:
> # What should we deny? One unique acl line per condition:
> acl bad1 all-of !authenticated access_to_users_area
> acl bad2 all-of !userA access_to_userA_area
> acl bad3 all-of !userB access_to_userB_area
> acl bad any-of bad1 bad2 bad3
> http_access deny bad
>
> Both constructs result in essentially the same ACL expression tree. The
> first (proposed) approach is consistent with how all other ACLs work and
> allows for simple aggregation of rules, including rules loaded from
> files using "quoted file name" syntax.

IMHO, the second is clearer despite the extra line of config needed for
this example. It is explicitly clear to see what the decision tree is
going to be doing without actually needing to know how the ACLs operate.

> Note that if we AND all-of lines, loading all-of rules from an ACL file
> would become impossible in most interesting cases because no natural
> condition can be described as a long list of simple ANDed expressions.
> By the very nature of the AND operation, AND lists tend to be short. OR
> lists tend to be long. Current all-of implementation allows for loading
> of long all-of lists from a file without creating a large number of
> otherwise not needed unique ACLs.
>
>
> If there is no consensus, I think we should remain consistent while
> acknowledging the problem.
>
>
>> In abstract I think we should define ACL types as each performing a
>> *single* operation on a set of values.
> That is not how ACL types are defined today. They all perform two
> operations: a type-specific match followed by ORing of the type-specific
> match results. This two-operation logic is often hidden behind splay
> trees and other data structures, but it is there.
>
> In most cases, the type-specific match has only one configurable
> parameter, but there are ACLs that take more than one parameter per
> type-specific match, including:
>
> acl aclname time [day-abbrevs] [h1:m1-h2:m2]
> acl aclname req_header header-name any\.regex\.here
>
> The all-of ACL is similar, but has a variable number of parameters for a
> type-specific match:
>
> acl aclname all-of acl1 [acl2 ...]
>
>
>> With this patch all-of is
>> applying a much more complex multi-type calculation which is not safe to
>> describe as an AND test. I can't put my finger on a good example case
>> but I'm sure this will come back to bite us (or a client) when we get to
>> adding tests like this with large sets of values:
>>
>> acl magic all-of A
>> acl magic all-of B
>> acl voodoo all-of magic !B
>>
>> when A=true, B=false ==> voodoo is true.
> I do not understand the point behind the above example, but I agree that
> some admins will misinterpret and misapply new ACLs types. However, I
> think that will happen no matter how we structure them.
>
> If we want to play it safe at the expense of folks who know what they
> are doing, we can prohibit multiple lines for all-of ACLs on the grounds
> that they are "too confusing for admins". I do not think we should do
> that though. It feels very wrong to cripple a powerful optional tool
> just because somebody will misuse it. However, we may want to do that
> when we add an "=" acl:
>
> acl good = (a or b) and !c

Indeed, it would a huge effort to alter every single config out there
and all the documentation ... Safe_ports ACL has been documented for
over 20 years as multi-line single *set* of port numbers.

Taking a step aside; what use would an "=" ACL be?
   the other conditional operators convert into tests or tree structure
layout. assignment builds a new name or alters the content of an
existing name - neither of which would seem to be good things to do to
the ACL tree structure shared by all requests in the running Squid.

If we did implement it, there is a strong case for making *it alone* a
single-line ACL type. OR maybe for making each line of it *replace*
(re-assignment) the existing ACL tree inserted wherever the name "good"
was used (yuck).

>
>> I don't think we should be forcing this level of mathematical knowledge
>> on people just to write ACLs.
> We are not forcing the use of these new ACLs on anybody, but yes, basic
> boolean arithmetic knowledge is required to write correct ACLs and yes,
> folks will screw up more with more powerful tools.
>
> I hope others will chime in on this discussion.

Anyone?

Looks like we need a tie-breaker.

Amos
Received on Tue May 21 2013 - 06:45:30 MDT