On 06/05/17 02:18, Eduard Bagdasaryan wrote:
Hello,
This patch fixes "miss_access" and "cache" checks when no ACL rules
matched.
The miss_access code allowed transactions to reach origin server when no
"allow" rules matched (and no "deny" rule matched either). This could
happen when a miss_access directive ACL could not make a "match" or
"mismatch" decision (e.g., the admin used a slow ACL or an ACL that
required authentication when the user was not authenticated).
Similarly, the "cache" directive code allowed requests to be served from
the cache (and responses to be stored in the cache) when no "allow" (and
"deny") rules matched. This behavior (established in v5 r14984) does not
contradict "Requests denied by this directive will not be..."
documentation, though.
I believe that both "miss_access" and "cache" directives should behave
like all other directives in similar contexts, i.e., "deny" in such
indeterminate cases because:
Unlike most other directives these two the "deny" action is their
primary purpose. Where most actions are about allowing things which are
otherwise not done, these two are about denying things which normally
are done. The allow action is merely a way to avoid following deny lines.
These both have a long history, and the cache directive in particular is
kind of twisted and non-intuitive since its current behaviour was
created to be backwards consistent with "no_cache allow" implying the
Cache-Control:no-cache header existence.
* It avoids problems with configurations like:
# broken if badGuys fails to match or mismatch (allows bad guys)
acl_driven_option allow !badGuys
That is what trunk r12176 has partially fixed.
* It makes ACL checking rules consistent with each other, especially if
there is no good reason for making exception for these two cases.
The other access lists which obviously treat non-allowed as denied are
very recent additions. So using them as a template to re-write existing
and widely used directives behaviour is not great.
It is very likely that this change will cause installations previously
"working" to have large amounts of traffic suddenly stop caching, or
start getting 403 denials. That violates our intentions of not doing
things to surprise admin if we can avoid it.
If we make this not-allowed == denied behaviour formal for how
allow/deny/dunno tristate directives map to boolean. We should first
make -k parse perform the "a fast-only directive uses a slow ACL!"
check, and make the existing one an ERROR for a few releases.
Also going through the process to deprecate cache directive formally
might be worth beginning - most old uses should be a straight rename to
store_miss, but some will be send_hit or a mix of the two. Just adding a
debugs statement to that effect on startup, reconfigure, and -k parse
should do for a while.
To help avoid similar problems in the future, and to improve code
consistency, I added an API that eliminates the need for direct
comparison with ACCESS_ALLOWED and ACCESS_DENIED.
These parts I like. The allowed() method addition bits would be nice to
apply however the behaviour change issues turn out.
Amos
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
