Re: [PATCH] ACL cleanup in 3.2

On 06/28/2012 04:59 AM, Amos Jeffries wrote:

> This is my attempted port of Alexs' ACL cleanups. They require the ACL
> extensions changes (what remained in trunk anyway), so teh differences
> there between trunk and 3.2 have been included in this patch.
>
> So far they are testing out okay on builds, but there is a bit longer to
> go on that.
>
> If there are no objections I plan to release 3.2.0.18 and apply this
> change to 3.2 branch afterward.

Hi Amos,

    Thank you for working on this! My response is not an objection to
these changes going into v3.2, but based on initial feedback from folks
running the cleaned up ACL code in trunk, there is one red flag that we
may need to discuss.

First some terminology. Any single ACL may "match", "mismatch", or
"fail" (in various ways). That result can be negated using "!acl" syntax
in the config. The negation of "match" is "mismatch". The negation of
"mismatch" is "match". The negation of "fail" depends on the code:

  In old code: a negation of a failed ACL was a match.
  In current code: a negation of a failed ACL is a failure.

This means that given a configuration like this:

  some_option allow !group

the old code "allows" or "applies" some_option if the "group" ACL fails.
 After our changes, that option will never be allowed if
the "group" ACL fails (other bugs notwithstanding).

This change breaks old configurations that relied on negated failing
ACLs to match. For example, some admins assumed that if there is no
authentication information, the negated authentication-related ACL will
match. In some cases, those configurations were just broken because they
did not cover all the cases correctly:

    http_access allow !badGuys

but there are apparently correctly-working misconfigurations that my
changes break.

I do not have enough information to post any meaningful statistics, but
I suspect that in many cases the admins were thinking that "missing
credentials" is the only ACL failure that can be negated into a match.
However, I believe the code was negating more than that, and possibly
any failure (e.g., a crashed authentication server) could be negated.

IMO, it is wrong to convert failures into matches by negating them. It
is often not logical ("this is not Bob" and "I have no idea who this is"
are often two very different things!). I also predict that this approach
will hinder ACL evolution because adding more operations and/or
extending/optimizing existing ACLs will be difficult when failures are
not propagated properly.

However, even if we agree that failures should not be converted to
matches by negation, there may be thousands of configurations out there
that use this flawed principle. Yes, all of them can be fixed/corrected,
but are we sure that forcing such corrections on users is the best way
forward?

Thank you,

Alex.
P.S. Please note that this discussion is only relevant in the negation
context (!acl) because behavior is meant to be the same in a positive
context.
Received on Thu Jun 28 2012 - 16:47:23 MDT