Discussion:
[squid-dev] [RFC] annotate_transaction ACL
Alex Rousskov
2016-07-09 19:14:16 UTC
Permalink
Hello,

I propose adding two new ACLs: annotate_transaction and
annotate_client[_connection]. Their draft documentation and usage
examples are provided at the end of the email.

The motivation for adding these ACLs is a persistent stream of requests
from admins that want to know whether Squid has made some decision (or
reached some important decision point). For example, an admin may need
to know whether a particular http_access rule has matched or whether the
request was received on a bumped connection.

All these complex decisions live inside various ACL-driven squid.conf
directives, especially directives that take many allow/deny/etc rules.
In theory, it is possible to re-compute those ACLs to determine whether
a certain condition was met. However, the following complications often
make that "just recompute the same ACLs" solution impractical:

0. Directives like ssl_bump are evaluated several times. Each time, the
ACL decision tree changes (and so does Squid state). It is usually
impractical to reconstruct the exact replica of the ACL decision tree as
it existed at the time a particular decision was reached. This problem
often be worked around by adding (at the decision point) a special
external ACL that always matches and adds annotations, but each extra
external ACL adds significant overheads and may increase complexity,
especially in setups that do not use external ACLs.

1. The ACLs involved may have side effects. This is typical for external
ACLs. For external ACLs, the problem can sometimes be worked around if
the external ACL itself returns an annotation (which can later be tested
instead of re-evaluating that external ACL), but since the same external
ACL can be used in different unknown-to-the-ACL contexts, the
same-for-all annotation may not be enough. Another [poor] solution
mentioned in #0 applies to this problem as well, with similar limitations.

2. The ACLs involved may be unstable -- re-evaluating them is not
guaranteed to produce the same result. This problem is typical for
complex ACLs such as server_name and external, but also affects such
simple ACLs such as dst. A [poor] solution mentioned in #0 applies to
this problem as well, with similar limitations.

3. The ACLs involved may be slow/asynchronous but the directive where
the decision needs to be reproduced only supports fast ACLs. Again, a
[poor] solution mentioned in #0 applies to this problem as well, with
similar limitations.

4. The directives involved only support fast ACLs while adding
annotation (as described in #0) requires "slow" [external] ACL support.

The proposed new ACLs solve these problems nicely, with low overhead,
and without adding any overheads to deployments that do not need them.


I have also considered and rejected the following two alternative
solutions to the problem because they had [fairly obvious?] serious
drawbacks:

A. Extend rule syntax to be able to mark reached decision points without
creating an ACL:

ssl_bump bump acl1 acl2 annotate(foo=bar) acl3

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.


Please note that the documentation below does not currently detail what
"adding" a foo=bar annotation means when there is already an annotation
named "foo". During implementation, we will decide whether the new ACLs
should always append to the existing matching annotation and/or
overwrite it. The documentation will be adjusted accordingly, of course.


* acl aclname annotate_transaction key value [fast]

Always matches. Used for its side effect: This ACL immediately adds
a key=value annotation to the current master transaction. The added
annotation can then be tested using note ACL and logged (or sent to
helpers) using %note format code. This ACL is especially useful for
recording complex multi-step ACL-driven decisions. For example:

# Do not log transaction accepted after aclX matched

# First, mark transactions accepted after aclX matched
acl markSpecial annotate_transaction special true
http_access allow acl001
...
http_access deny acl100
http_access allow aclX markSpecial

# Second, do not log marked transactions:
acl markedSpecial note special true
access_log ... deny markedSpecial

# Note that the following would not have worked because aclX
# alone does not determine whether the transaction was allowed:
# access_log ... deny markedSpecial # Wrong


Warning: This ACL annotates the transaction even when negated and
even if subsequent ACLs fail to match. For example, the following
three rules will have exactly the same effect as far as annotations
set by the "mark" ACL are concerned:

some_directive acl1 acl2 ... mark # rule matches if mark is reached
some_directive acl1 acl2 ... !mark # rule never matches
some_directive acl1 acl2 ... mark !all # rule never matches


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

Example:

# Do not rewrite bumped transactions

# First, mark bumped connections:
acl markBumped annotate_client bumped true
ssl_bump peek acl1
ssl_bump stare acl2
ssl_bump bump acl3 markBumped
ssl_bump splice all

# Second, do not send marked transactions to the redirector:
acl markedBumped note bumped true
url_rewrite_access deny markedBumped

# Note that the following would not have worked because acl3 alone
# does not determine whether the connection is going to be bumped:
# url_rewrite_access deny acl3 # Wrong



Please let me know if you object to adding these new ACLs or have any
improvement suggestions.


Thank you,

Alex.
Amos Jeffries
2016-07-09 23:47:11 UTC
Permalink
Post by Alex Rousskov
B. Add general ACL options to be able to force any existing ACL to add
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?
This used to be what we were planning to add one day.
Post by Alex Rousskov
Please note that the documentation below does not currently detail what
"adding" a foo=bar annotation means when there is already an annotation
named "foo". During implementation, we will decide whether the new ACLs
should always append to the existing matching annotation and/or
overwrite it. The documentation will be adjusted accordingly, of course.
* acl aclname annotate_transaction key value [fast]
Always matches. Used for its side effect: This ACL immediately adds
a key=value annotation to the current master transaction. The added
annotation can then be tested using note ACL and logged (or sent to
helpers) using %note format code. This ACL is especially useful for
# Do not log transaction accepted after aclX matched
# First, mark transactions accepted after aclX matched
acl markSpecial annotate_transaction special true
http_access allow acl001
...
http_access deny acl100
http_access allow aclX markSpecial
acl markedSpecial note special true
access_log ... deny markedSpecial
# Note that the following would not have worked because aclX
# access_log ... deny markedSpecial # Wrong
Warning: This ACL annotates the transaction even when negated and
even if subsequent ACLs fail to match. For example, the following
three rules will have exactly the same effect as far as annotations
some_directive acl1 acl2 ... mark # rule matches if mark is reached
some_directive acl1 acl2 ... !mark # rule never matches
some_directive acl1 acl2 ... mark !all # rule never matches
* 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).

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

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.

Otherwise it seems good. Thanks.

Amos
Kinkie
2016-07-10 11:29:41 UTC
Permalink
Same thoughts as Amos: a common ACL switch to annotate results of the
matching seems more natural from the user's perspective (although probably
requires more code to implement).
Post by Amos Jeffries
Post by Alex Rousskov
B. Add general ACL options to be able to force any existing ACL to add
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?
This used to be what we were planning to add one day.
Post by Alex Rousskov
Please note that the documentation below does not currently detail what
"adding" a foo=bar annotation means when there is already an annotation
named "foo". During implementation, we will decide whether the new ACLs
should always append to the existing matching annotation and/or
overwrite it. The documentation will be adjusted accordingly, of course.
* acl aclname annotate_transaction key value [fast]
Always matches. Used for its side effect: This ACL immediately adds
a key=value annotation to the current master transaction. The added
annotation can then be tested using note ACL and logged (or sent to
helpers) using %note format code. This ACL is especially useful for
# Do not log transaction accepted after aclX matched
# First, mark transactions accepted after aclX matched
acl markSpecial annotate_transaction special true
http_access allow acl001
...
http_access deny acl100
http_access allow aclX markSpecial
acl markedSpecial note special true
access_log ... deny markedSpecial
# Note that the following would not have worked because aclX
# access_log ... deny markedSpecial # Wrong
Warning: This ACL annotates the transaction even when negated and
even if subsequent ACLs fail to match. For example, the following
three rules will have exactly the same effect as far as annotations
some_directive acl1 acl2 ... mark # rule matches if mark is reached
some_directive acl1 acl2 ... !mark # rule never matches
some_directive acl1 acl2 ... mark !all # rule never matches
* 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).
"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.
We will also have to be careful to blacklist the reserved key names that
user, group, password, ttl, url, url-rewrite, etc.
Otherwise it seems good. Thanks.
Amos
_______________________________________________
squid-dev mailing list
http://lists.squid-cache.org/listinfo/squid-dev
--
Francesco
Alex Rousskov
2016-07-11 05:31:08 UTC
Permalink
Post by Kinkie
Same thoughts as Amos: a common ACL switch to annotate results of the
matching seems more natural from the user's perspective (although
probably requires more code to implement).
AFAICT, Amos actually said the opposite -- he liked the proposed ACLs
"better" than the currently rejected alternative (B). Please see my
response for a list of (B) drawbacks (that Amos has requested). Do you
still favor (B)?

Alex.
Post by Kinkie
Post by Alex Rousskov
B. Add general ACL options to be able to force any existing ACL to add
acl myOldAcl dst --annotate foo=bar 127.0.0.1/32 <http://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?
This used to be what we were planning to add one day.
Post by Alex Rousskov
* acl aclname annotate_transaction key value [fast]
...
Alex Rousskov
2016-07-11 05:27:38 UTC
Permalink
Post by Amos Jeffries
Post by Alex Rousskov
B. Add general ACL options to be able to force any existing ACL to add
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.
Post by Amos Jeffries
Post by Alex Rousskov
* 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.
Post by Amos Jeffries
"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.
Post by Amos Jeffries
We will also have to be careful to blacklist the reserved key names that
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.
Amos Jeffries
2016-07-12 06:59:32 UTC
Permalink
Post by Alex Rousskov
Post by Amos Jeffries
Post by Alex Rousskov
B. Add general ACL options to be able to force any existing ACL to add
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
# 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.
Post by Alex Rousskov
Post by Amos Jeffries
Post by Alex Rousskov
* 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.
Post by Amos Jeffries
"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.
Post by Alex Rousskov
"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
Post by Alex Rousskov
Post by Amos Jeffries
We will also have to be careful to blacklist the reserved key names that
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
Alex Rousskov
2016-07-12 15:00:24 UTC
Permalink
Post by Amos Jeffries
Post by Alex Rousskov
Post by Alex Rousskov
* acl aclname annotate_client key value [fast]
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.
I do not think your sub-tree design solves that problem, even partially,
because !foo annotates in your design as well (assuming foo itself
annotates). Moreover, I doubt this is a problem that can be solved! When
foo annotates,

* if !foo annotates, some admins will be confused
* if !foo does not annotate, some admins will be confused

My always-annotates design is the simplest among the various
confusing-for-some options: If Squid reaches your annotating ACL, it
_will_ annotate, no matter what happens afterwards.
Post by Amos Jeffries
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 do not think it would make a difference because the "!" is applied
_after_ the new ACL, regardless of whether the new ACL is a leaf or a
sub-tree. In other words, the ACL itself does not know about its
subsequent negation. We used to _implement_ negation as an ACL flag, but
that [inferior for other reasons] design is long gone IIRC.

* In your proposal, the new ACL X matches/annotates when both the
previous ACLs in the same rule and X subtree match.

* In my proposal, the new ACL X matches when the previous ACLs in the
same rule match.

The latter is simpler.

Please note that one can still achieve your functionality with the
simpler approach by using an all-of ACL:

acl foo_ annotate_client key=value
acl foo all-of sub-tree foo_

Also, if we add a sub-tree to the new ACL, then there is a question of
whether that sub-tree should be an "all-of" or "any-of" tree. Folks will
want both, and some will be confused about the actual support,
regardless of which one we actually support. In the simpler design,
there is no such question/confusion (but folks can still create all-of
and any-of subtrees if needed as the example above illustrates).
Post by Amos Jeffries
# without annotation
http_access allow localhost
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
# 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
Yes, that would work, but I do not see any _advantages_ this added
complexity buys us, and I see some disadvantages (besides complexity).
One of the disadvantages is that to clearly see what traffic is denied
while reading http_access rules, one would have to use long ACL names,
re-telling the annotating ACL construction:

# sub-tree approach where it is unclear what is denied
http_access deny mark

# sub-tree approach where it is unclear that denied traffic is marked
http_access deny myLocalhost

# clear sub-tree approach that duplicates information
http_access deny localhost_and_mark

# clear leaf approach
http_access deny localhost mark


I suggest that we "divide and conquer" by using simplest,
dedicated-to-one-task ACLs. If my arguments above did not convince you,
please detail the advantages of the more complex sub-tree scheme.


Thank you,

Alex.
Amos Jeffries
2016-07-13 11:39:06 UTC
Permalink
Okay, as the author you get to choose.

Lets see how your approach works in reality (as opposed to our differing
ideas of reality) and we can change later if need be.

Amos

Loading...