[squid-dev] [RFC] annotate_transaction ACL

[Amos Jeffries]

On 11/07/2016 5:27 p.m., Alex Rousskov wrote:
> On 07/09/2016 05:47 PM, Amos Jeffries wrote:
>> On 10/07/2016 7:14 a.m., Alex Rousskov wrote:
>>>
>>> B. Add general ACL options to be able to force any existing ACL to add
>>> an annotation:
>>>
>>>     acl myOldAcl dst --annotate foo=bar 127.0.0.1/32
>>>
>>> Please let me know if you consider any of the above alternatives more
>>> attractive (than adding new ACLs) or need me to detail their drawbacks.
> 
> 
>> I do like the below better. But for the record what was the drawback on
>> (B) that you saw as serious?
> 
> (B) will screw up many squid.conf parsers and ACL generator scripts that
> cannot handle complex ACL parameters like
> 
>    --annotate foo="bar baz"
> 
> This is especially true for ACLs that normally take no parameters at
> all. There is already a big problem regarding ACL parameter scope in
> multi-ACL lines, and it may be best to stay away from that.
> 
> 
> (B) cannot add annotations to predefined/hard-coded ACLs like "all" and
> also to frequently used ACLs like, say, "trustedNetworks". As a
> workaround, it would be possible to use named all-of or any-of ACLs
> containing nothing but a hard-coded or frequently used ACL, but then we
> may be better off with always naming marking ACLs to start with:
> 
>    # create a named alias for "all" so that we can annotate
>    acl markCase123 any-of --annotate foo=bar all
>    ...
>    http_access deny markCase123
> 
> (B) is less explicit so there is more risk that annotations will be
> accidentally added in lots of places they should not be.
> 

Thanks. Yes the second point is a total killer.

> 
>>> * acl aclname annotate_client key value [fast]
>>>
>>> Always matches. Used for its side effect: This ACL immediately adds
>>> a key=value annotation to the current client-to-Squid connection.
>>> Connection annotations are propagated to the current and all future
>>> master transactions on the annotated connection. All caveats for the
>>> annotate_transaction ACL apply to this ACL.
>>
>> I think the format should use explicitly the ' key="value" ' syntax to
>> make it simpler to identify where the value ends and ACL sub-tree begin.
>>
>> That will also allow us to define the above format as adding a new key
>> only if not already present. We could use key+="value" syntax for
>> appending a value to an possibly existing key (or adding if none).
> 
> Yes, I was thinking about similar tricks as well. The key=value syntax
> will tempt us to add support for setting several annotations with one
> ACL, but perhaps that is OK.
> 
> 
>> "Always matches" seems incorrect definition for this ACL. When 'fast' is
>> provided it should pass-thru the match or non-match result of the [fast]
>> ACL sub-tree, and only annotate when that result is a match.
>>  I assume [fast] default would be 'all' to always annotate and be true
>> if there was no sub-tree.
> 
> Err... The letters "[fast]" is just how we mark non-blocking ACLs in
> squid.conf. That is, the proposed ACL syntax is just
> 
>     acl aclname annotate_client key value
> or
>     acl aclname annotate_client key=value
> 

Sorry. Braindead moment there. I'm used to only seeing that at the end
of the textual description. Not the ABNF syntax.


> 
> "Always matches" is exactly what we plan for this ACL. If you think it
> should not match in some cases, please discuss which
> transactions/connections it should not match.
> 
> One [documented] problem is that !foo will still annotate, which is a
> little counter-intuitive, but I cannot think of a better approach.
> 

Well, that issue is partially resolved in the design I was mistakenly
thinking you were suggesting.

If this ACL is implmented as a Node with a sub-tree, like any-of/all-of.
Which annotates whenever the sub-tree produces a match. Then the '!' on
it wont matter so much.

I was thinking to use it like this to annotate:

 # without annotation
 http_access allow localhost

 # with annotation "localhost" when its allowed:
 acl foo annotate_client key="localhost" localhost
 http_access allow foo

 # with annotation "localhost" when its denied
 http_access deny foo


Or both of these being equivalent to "deny !localhost" with different
annotation when it denies:

 # annotates localhost, denies non-localhost stuff
 http_access deny !foo

 # annotates non-localhost and denies non-localhost stuff
 acl foo2 annotate_client key="non-localhost" !localhost
 http_access deny foo2


> 
>> We will also have to be careful to blacklist the reserved key names that
>> require special processing to be correct:
>>  user, group, password, ttl, url, url-rewrite, etc.
> 
> Are those already reserved for annotations set by the external ACL? I
> was hoping the new ACLs can reuse at least some external ACL code
> related to annotations. No need to answer this one. We will research it
> as needed.

Not just by external ACL. They are used in various ways by the helper
protocols to trigger things. Their existence usually signals that thing
has happened. But if its just an annotation without actually going
through the helper handlers to do that thing - they could badly confuse
people about whats happening.
 The user/password/group ones are most dangerous since they can be used
as input to external ACL with dangerous results to authorization.

Amos