[squid-dev] [RFC] Where do ACL flags belong?

[Amos Jeffries]

Okay, I see I have very misunderstood your proposals.

So snipping away my side-trails...

On 4/12/2015 7:51 p.m., Alex Rousskov wrote:
> On 12/03/2015 10:33 PM, Amos Jeffries wrote:
>> On 4/12/2015 4:03 p.m., Alex Rousskov wrote:
>>> On 12/03/2015 06:10 PM, Amos Jeffries wrote:
>>>> On 4/12/2015 1:02 p.m., Alex Rousskov wrote:
>>>>> In summary, this is a tough choice between:
>
>>>>> #1: Supporting what the admins thought they are configuring.
>>>>>     Better configuration interface?
>>>>>     More similar to how -i is handled today.
>
>>>>> #2: Backward compatible (but with "Is your config broken?" warnings).
>>>>>     A lot less development work.
>

>>>>> How do you think this should be fixed?
> 
>>>> I prefer #1, because it is already familiar to admins and does not force
>>>> us all to do a non-backward-compatible manual config update (those as
>>>> you may see below that is not mandatory). We have enough config updates
>>>> happening without this.
> 
> 
>>> If we do #1, do you think we should ignore the possibility that some
>>> Squids will be broken because Squid interpretation of the configuration
>>> file will change? We can add warnings, but then removing those warnings
>>> will require changing the config...
> 
> 
> This is the last open question on this thread AFAICT: What to do about
> backward compatibility in approach #1 that you favor?
> 

#2 does sound like an attractve short-term fix rather than a good
long-term solution.

But as you state in that one-liner summary of #2 later it would also
involve warnings. And ones we cant get rid of while the -n behaviour
remains unchanged.

We would need to add a completely different flag for the no-rDNS and
"burn" -n for a very long time. Which is somewhat worse.

> 
>>>> I know that has issues of its own as side effects on *access complexity.
>>>> Allowing one "name" to be shared between the dstdomain, dstdom_regex,
>>>> and dstdom_str types would simplify those.
>>>>  OR, using a namespace sub-type ('_' maybe?) that implies the flagged
>>>> behaviour difference. A renaming dstdom_regex to dstdomain_regex etc.
>>>> under the new model.
> 
>>> Sorry, you lost me here completely. Can you give an example?
> 
> 
>>   acl foo dstdomain .example.com
>>   acl foo dstdomain 192.0.2.1
>>
>>   acl foo dstdomain_str 192.0.2.3
>>
>>   http_access deny foo
>>
>> when looking up http://192.0.2.3/ the ACL only performs an exact match.
>> when looking up http://192.0.2.1/ the ACL performs exact-match, then
>> rDNS lookup and match.
> 
> AFAICT, the above will perform an rDNS lookup (to match against
> .example.com) in both cases. We are still matching each acl data entry
> in order, right? Did you mean to place the dstdomain_str line first?
> 
> 
>> Same as the below would do today:
>>
>>   acl foo dstdomain .example.com
>>   acl foo dstdomain 192.0.2.1
>>
>>   acl bar dstdomain -n 192.0.2.3
>>
>>   http_access deny foo
>>   http_access deny bar
> 
> 
>> (bar currently needing to exist due to -n's per-ACL not per-line nature).
> 
> 
> The -n approach is overall better than _str suffix approach because the
> former makes it easy to mix-and-match multiple flags and to support
> flags with values such as -m=delimiters.
> 

Good point.

> 
>>>> AFAICS it should not be much more difficult to
>>>> re-implement -n using #1 than to re-implement regex parsing and -i to
>>>> implement #2.
>>>
>>> True, but I do not think we should touch the -i interface. It works as
>>> expected.
>>>
>>
>> Then I must have completely misunderstood what your #2 proposal was about.
> 
> #2 issues a warning when -n is used on some but not all lines of a given
> ACL foo. No other changes in #2.
> 

Which makes that a permanent warning, and a permanent annoyance that
only forces admin to expand the number of named ACLs they use. Which
kind of defeats the purpose of having the flag vs a different ACL type name.


> 
>>  -i flag operates per-line, and resets at the EOL. 
> 
> Yes (which, BTW, also breaks the "multiple lines are the same as one
> combined line" design principle).
> 

Nod.

> 
>> If we remove per-line/value flags then a lot of configs are broken.
> 
> Neither #1 nor #2 change how -i works.
> 
> #1 makes -n work per line, as intended.
> 
> #2 does not change how -n works, but issues a warning if -n is used
> inconsistently across multiple lines.
> 
> 
> 
>> I am proposing option #3 be that we have the flag (or ACL type _suffix)
>> cause parser to generate a similar sub-tree of nodes to what anyof would
>> generate. BUT using only the specific named type of ACL that the line
>> was given.
>>
>>
>> So this:
>>  acl foo dstdomain 192.0.2.1
>>  acl foo dstdomain -n 192.0.2.3
>>
>> or this:
>>  acl foo dstdomain 192.0.2.1
>>  acl foo dstdomain_str 192.0.2.3
>>
>>
>> both generates internally a tree identical to:
>>
>>   acl foo:1 dstdomain 192.0.2.1
>>   acl foo:2 dstdomain -n 192.0.2.3
>>   acl foo anyof foo:1 foo:2
> 
> 
> AFAICT this is the same as optimized #1 I have mentioned earlier (except
> for the _suffix part which I think we should avoid as discussed above).
> To illustrate this even better, you need to add more lines and values:
> 
>   acl foo dstdomain v1
>   acl foo dstdomain -n v2a v2b
>   acl foo dstdomain v3
> 
> is interpreted after #1 changes as
> 
>   acl foo1  dstdomain v1
>   acl foo2a dstdomain -n v2a
>   acl foo2b dstdomain -n v2b
>   acl foo3  dstdomain v3
>   acl foo anyof foo1 foo2a foo2b foo3
> 
> Does this match what you think we should do?

Yes.

> If yes, what to do about
> backward compatibility? Break it because -n is not that widely used?

Yes. With a +n flag (akin to the +i, re-enabling what -n disabled) we
can follow -i/+i pattern with an auto-reset at the end of each line but
add a warning if the +n is omitted on the next one.

The internal implementation of that could be a node listing values to
check pre-rDNS and a node listing values only checked post-rDNS.



The message I have been getting about back-compat seems to be that
admins are relatively okay with pushing out config changes after the
fact, so long as headless upgrades dont actually self-abort during the
upgrade process itself and leave the proxy shutdown.
Much as I hate having a proxy suddenly start behaving differently than
expected it seems to be what the vocal others are saying they prefer (ew).

>From a security standpoint changing the behaviour of -n on line endings
is borderline. Depending on whether the ACL is used as whitelist or
blacklist it might have CVE-serious implications. But we avoid CVE
assignment by documenting the change as intentional.


Could this be something completed quickly enough for Squid-4?

Amos