[squid-dev] [PATCH] auth_schemes directive

[Amos Jeffries]

On 30/11/2016 2:11 p.m., Alex Rousskov wrote:
> On 11/29/2016 03:50 PM, Amos Jeffries wrote:
>> On 28/11/2016 3:34 p.m., Alex Rousskov wrote:
>>> Or being able to control the order of schemes presented to the user.
> 
>> Any HTTP client implementation which was coded to be properly compliant
>> with RFC 2616 and 2617 *will not* obey any ordering presented by Squid.
>> (Yes that was relaxed in RFC723x, but only to "ought to" and most
>> clients still obey the older MUST requirement).
> 
>> IE6 was a notable exception that did follow the ordering when Basic was
>> first and why that ordering of auth_param was made significant back in
>> the early Squid-2.x sometime. It is now almost completely dead/absent as
>> a client though.
>>
>> The current worst problem is Safari - for exactly the opposite reason.
>> It demonstrably ignores the list of schemes and uses only (and forever
>> looping) its first choice of "best" scheme. No others are considered nor
>> attempted.
>>
>> I'm also aware of Firefix NTLM issues and Java, but those are not order
>> related. Are there any other client software with scheme ordering issues
>> I should be aware of?
> 
> I find it ironic that you need to ask for more issues after listing a
> well known issue and indicating that the protocol requirements have been
> relaxed. For me, your own response is a good illustration that ordering
> control is [at least marginally] useful!
> 

No irony intended. If those are the only things driving the need for
ordering then we should probably drop order considerations entirely.

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.

10-20 lines of code IIRC and about the same in docs text.

> 
>> We *already* have ordering ability in squid.conf. 
> 
> The existing ability is severely limited because it can accommodate at
> most one unusual client. It is also rather awkward to configure.
> 

Which appears to be IE and IE alone AFAIK. So nothing much gained by a
change there.

> 
>> Many users have tried
>> to make use of it. The fact that it is not solving their issues is a
>> good demonstration about why/how the above RFC requirement is a killer
>> for any attempt to use ordering to control the clients behaviour.
> 
> I am not sure how you reach that conclusion given the available
> evidence. Yes, ordering alone cannot solve all the issues, but that does
> not mean that properly supporting ACL-driven order is useless.
> 
> Said that, better ordering support is just one of several useful
> properties of the proposed approach. Even if we are 100% positive that
> nobody will ever need to control the order of schemes, the ability to
> specify a list of schemes to use is still a nice/useful feature, and
> will remain a nice/useful feature even after somebody adds support for
> multiple configurations of the same scheme.
> 
> 
>>>> To resolve (1) fully we will need to have multiple Config objects with
>>>> the same scheme name and different config details.
> 
>>> The proposed changes do not preclude
>>> such future support. Moreover, such future support will not address some
>>> of the use cases covered by the proposed patch. In other words, what has
>>> been implemented and your suggestion are complementary, not mutually
>>> exclusive.
> 
>> For the use cases this patch resolves, that is true. 
> 
> IMO, my statement is true for all use cases, both use cases covered by
> the patch and use cases not covered by the patch: The two areas are
> complementary, not mutually exclusive, regardless of the use cases.
> 
> 
>> For the other cases
>> my per-scheme ACLs proposal seeks to resolve as well, it is not.
> 
> Which part of my statement is not true for those other cases?

The part about them not being "complementary, not mutually exclusive".

They are mutually interferring on what would seem a reasonably average
config:

 auth_param basic ...
 auth_param ntlm ...
 auth_param ntlm access allow localhost

 auth_schemes ntlm,basic localnet
 auth_schemes basic,ntlm all

WTF?!! only Basic ever gets used.


Or one would think from the above that 'only Negotiate' would be the
outcome in this equivalent config:

 auth_param negotiate ...
 auth_param ntlm ...
 auth_param ntlm access allow localhost

 auth_schemes ntlm,negotiate localnet
 auth_schemes negotiate,ntlm all

Nope, NTLM gets used now. Even when its not offered (ie non-localhost
traffic).


Now some future code of an entirely different sort can fix that, but we
would still have a period of confusion to go through meanwhile.

And if we don't care about ordering (from up top reasons). Then which of
the two access lists gets implemented right away does not matter much.

> 
>> We could add both ways. But that I expect will just lead to more
>> confusion. So lets try to avoid that.
> 
> I agree that supporting two partially overlapping ways to configure used
> authentication schemes will cause some confusion in some cases, but I
> disagree that it will be the only outcome. The positive outcomes of
> supporting both explicit scheme lists and multiple scheme configurations
> outweigh the negatives IMO.

AFAICT we have pretty much narrowed the positives down to this patch
allowing multiple orderings of schemes. With accompanying evidence that
ordering is largely useless.

So no I dont agree at present that the positives outweight the
negatives. It is only outweighing the status-quo.

> 
> However, we can and should also prevent any overlaps in coverage! The
> already implemented code can be used to control scheme presence [and
> order] while the proposed future code will control which configuration
> is used for each used scheme. That divide-and-concur is the right/best
> approach here. I should have thought of that earlier, but, fortunately,
> this realization does not affect auth_schemes.
> 
>> My proposal uses that existing logic instead of adding a second config
>> setting that can contradict it and cause confusion.
> 
> If your proposal implementation is limited to scheme configuration
> selection (among multiple configurations for the same scheme), then
> there will be no contradiction and confusion. Each feature will control
> its own area.
> 
> * Want to control which schemes are used for transaction X?
>   Use the implemented auth_schemes.
> 
> * Want to control which scheme configuration is used for used scheme S?
>   Use [future feature].
> 
> 
>> For the uncommon case of needing different order of the same schemes for
>> different clients, multiple auth_param sections with different ACL lists
>> can be used. It is a little verbose, yes, but not a common need so as
>> you have argued in the past: we should not be optimizing for that type
>> of case at expense of the more common ones.
> 
> The implemented auth_schemes optimize a common case AFAICT. The future
> scheme configuration selection code can also optimize its common case.
> The features do not need to overlap, and each can focus on optimizing
> its area.
> 
> 
>>> Also, while working on the proposed feature, we have discovered why
>>> adding per-scheme ACLs (or a special NONE value to auth_schemes) will
>>> create a negative side effect that the posted implementation avoids: The
>>> implemented code does not change authentication decision; auth_schemes
>>> cannot accidentally create a situation where no schemes exist for a
>>> transaction.
> 
>> 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.


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


> 
>>>  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.
> 
> The above is still a good summary of why you should not block
> auth_schemes because you want Squid to support multiple scheme
> configurations in the future. Prior to this email, I thought that what
> you are proposing and what we have implemented can overlap. Now I am
> fairly certain that they should not overlap but should complement each
> other. In other words, they should be orthogonal questions/answers/features.
> 

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.
  The docs will need to reflect that scoping.

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

> 
>>> Please reconsider your initial evaluation.
>>
>> I have and still do not want to see this go in as one change.
>>
>> I'm happy with the structural/internal part making non-allow/deny
>> actions possible anywhere. If that were split from the auth_schemes
>> directive part it could go in immediately without another review IMO.
> 
> Sure it can, but the deadlock is not in those code polishing changes. Do
> you still block auth_schemes, even if it is committed separately from
> those polishing changes? Or does your "as one change" imply that you are
> removing your objection to auth_schems as such if it is committed
> separately from the polishing changes?
> 

I still want the split. 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.

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

 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.


Amos