[squid-dev] [PATCH] Fix 'miss_access' and 'cache' checks when no ACL rules matched

[Amos Jeffries]

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