[squid-dev] [PATCH] auth_schemes directive

[Alex Rousskov]

On 11/29/2016 08:35 PM, Amos Jeffries wrote:
> Considering just the allow/deny decision about whether any particular
> scheme configuration should be used on a given request my proposal would
> be the simplest implementation;
> 
>  one checklist object created outside the fixHeader() loop,
>  fastCheck() the relevant access list for active schemes,
>    continue the loop on denials,
>  if no auth headers added when the loop ends,
>    change status code to 403.

If we

* want to add 403 support as a side effect of running out of
authentication schemes (rather difficult to configure explicitly because
the decision to send 403 has to be made repeatedly for each scheme!),

* want to make configuring simple scheme lists more difficult, and

* do not want to improve scheme order support,

then the per-scheme access list is indeed a better approach! You are
essentially trading admin convenience/sanity for ease of implementation,
which is a strange trade since the implementation is already available.

The per-scheme access lists do have one advantage: They allow for simple
configuration of a "do not send scheme S to user U" exception when
multiple schemes are enabled. However, this advantage turns into a
problem in many cases (because an admin may accidentally run out of
schemes, because it is going to be difficult for the admin to visualize
the final list after many individual rules, and because it is a bad way
to control 403 responses explicitly).

Perhaps there is a way to keep the per-scheme access list advantage
without opening the 403 Pandora box and preserving the whole-list
visualization provided by auth_schemes?

For example, we could support something like this:

  auth_schemes "ALL except S1"    acl1 ...
  auth_schemes "ALL except S1,S2" acl2 ...
  auth_schemes S1,S2              acl3 ...
  auth_schemes ALL                acl4 ...

but I do not like how this syntax essentially moves operators inside
quoted strings.

Another alternative is:

  auth_schemes S1    deny  acl1 ... # ALL except S1
  auth_schemes S1,S2 deny  acl2 ... # ALL except S1 and S2
  auth_schemes S1,S2 allow acl3 ... # just S1 and S2
  auth_schemes ALL   allow acl4 ... # ALL

(with the configuration implementation similar to the existing
request_header_access rules).

Unfortunately, in all these cases, we would have to special-case
denying/excepting all schemes to avoid opening the 403 Pandora box. Only
the current auth_schemes implementation avoids that 403 problem (because
an empty list is a syntax-level/configure-time violation).


> The part about them not being "complementary, not mutually exclusive".
> They are mutually interferring on what would seem a reasonably average
> config:

You are probably interpreting "complementary" as non-intersecting. I did
not mean it that way. I agree that if per-scheme access lists are
allowed to select which schemes are used (as opposed to which scheme
configurations are used), then there will be interference. I think we
agree that such interference is undesirable and can be avoided.


> AFAICT we have pretty much narrowed the positives down to this patch
> allowing multiple orderings of schemes.

We have not. I doubt introducing implicitly controlled 403s is a good
idea, and I am sure some admins will appreciate the ability to specify
the whole list of schemes as a single parameter rather than trying to
construct it on the fly from individual pieces using properly ordered ACLs.


>>> That is exactly the case in HTTP Authentication where 403 status code is
>>> supposed to be used. It is explicitly one of the permitted auth states.
>>
>> Explicitly forbidding access and wanting to authenticate but implicitly
>> running out of authentication schemes are very different cases. We
> 
> Running out of usable schemes is just non-credential criteria and may be
> based on other header values ACL matching, not very different case at all.

I disagree: Configuring ways to authenticate and denying access by
prohibiting authentication are different cases IMHO. Both may be based
on various header values, of course.


>> should not duplicate http_access functionality in auth_schemes or the
> 
> This is not http_access functaionality. This is the 403 response from
> authentication attempt. With auth-failed status codes being 401 (www
> credentials), 407 (proxy credentials) and 403 (other auth criteria).

It is probably pointless to argue without enough use cases, but I
suspect that all three kinds of access denials should be controlled by
http_access, not schemes configuration/selection code.


>>>>  The
>>>> "Should I use this configuration for scheme X?" and "What schemes should
>>>> I use for this transaction?" are two different questions, each deserving
>>>> an answer in various real-world situations.


> I've taken another read of the patch and am leaning towards agreeing to
> the above separation despite what I think is a very limited gain.

Why? We do not seem to agree on any rationale items that back up one
approach versus the other. In other words, all your previous responses
seem to claim that auth_schemes offers no gain at all, but now you see
some. I am curious which agreement I missed; or perhaps you see some
auth_schemes advantage that we have not discussed?


>   The docs will need to reflect that scoping.

Sure. The implemented auth_scheme docs should not invade per-scheme
configuration selection territory, but we should double check.


> Though I do think my proposal would prove to be more efficient if we can
> put together some tests for comparing the two.

The performance difference would depend on the configuration: If there
are fewer scheme lists than explicit per-scheme rules, then auth_schemes
win. Otherwise, per-scheme ACLs win.


> I still want the split.

Yes, that is understood.


> If it helps clarify my reasons are basically;
> 
> The new directive is UI change so is a v5 feature, excluded from v4 due
> to where we are at in the release cycle.

This is a bogus reason IMO: Adding an optional configuration option is
not the kind of UI change that should matter for a v4/v5 boundary.


> But the structural changes should probably still end up in v4 to
> minimize future portage pain around all the other access controls touched.

Sure.


> Also, each part is likely to result in (or allow fixing) different
> types of bugs. The new directive in specifically auth related issues,
> and the structural change more widespread ACL processing issues.

Yes, but if both end up in v4 and v5, then the time spent separating
them would be better spent elsewhere IMO.


Let's do another round in case we manage to improve the overall design
somehow (as discussed above). If that fails, we will start working on
separating auth_schemes from action code polishing so that each can be
committed separately per your request.


Thank you,

Alex.