[squid-dev] [RFC] annotate_transaction ACL

[Alex Rousskov]

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.


>> * 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


"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.


> 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.


Thank you,

Alex.