Re: [RFC] Handle ACLs that are neither denied nor allowed

On 21.03.2012 06:33, Alex Rousskov wrote:
> On 03/20/2012 05:24 AM, Amos Jeffries wrote:
>
>> The admin whose bug report triggered this all was using a config
>> equivalent to:
>>
>> http_access allow localnet
>> ssl_bump allow login
>> ssl_bump deny all
>>
>> Do you think it reasonable to expect the LAN can do HTTP anywhere
>> but
>> only get bumped if the user is logged in?
>
> Yes, although I suspect the above example is simplified to illustrate
> the point or perhaps work around other Squid bugs. We are working on
> a
> bump-server-first sub-project to make CONNECT authentication smoother
> so
> that bumping decisions can be made based on user information. For
> example, "bump all employees but not guests".
>
> What is _not_ reasonable is to expect ssl_bump to trigger
> authentication. Same for most other options "below" http_access in
> the
> processing chain. There is only a handful of options that can trigger
> authentication.
>
>
>> Additionally we document all over the place that "slow" group ACLs
>> can
>> do helper lookups (including auth). ssl_bump was made a slow group
>> access list.
>
> This is not about slow-vs-fast ACLs, IMO.
>
> "Slow" should not imply "may trigger authentication". The fact that
> it
> sometimes does today, is kind of the core of user complaints, AFAICT.
> In
> the extreme case, we might want to convert all Squid options to
> support
> slow ACLs because slow ACLs are more powerful. However, we cannot
> teach
> all Squid options to trigger authentication because that is simply
> impossible for late-in-the-processing options.
>
> For example, ssl_bump supports slow ACLs but it cannot trigger
> authentication. Thus, it must use authentication information already
> available and must not be forced to decide whether that information
> is
> fresh or stale.

The first response byte is the boundary for "too late" on challenge
same as any other HTTP status codes.

When ssl_bump is run we have not yet sent any response bytes to the
client. Therefore it is not too late.

>
> IMHO, if authentication is involved, the only sane option is to
> follow
> this pattern:
>
> 1a. Authenticate (may require several HTTP transactions).
>
> 1b. Iff authenticated user is authorized to access Squid, continue.
> Otherwise, deny further access.
>
> 2. Use authenticated user credentials as needed.
> No authentication can be triggered at this stage.
> Credentials cannot become invalid or stale here.

They *can* become stale. But we may use them anyway when there is no
security risk. When there is security involved we must not use them
until revalidation. That is the point of EXPIRED_OK. It means we are
waiting an async helper lookup to re-validate them, but at last
validation they were okay. The access list context is where the
information about security-risk is implicit/known.

Consider: Admin closes a users account and removed them from the
system. Just before that the user opened a CONNECT and starts using it
for a week non-stop. All of the bumped requests are authorized under the
expired credentials? or credentials suddenly change to EXPIRED_BAD or
AUTH_REQUIRED and the sequence gets aborted (no challenge inside the
bumping).

This same scenario could also happen with hijacked accounts getting an
emergency change on the password. Or any routine operations doing the
same.

NOTE: I think your suggestion of passing some info to the checklist
indicating what to do about auth could be handy here to simplify things
and hide the EXPIRED_* handling inside the checklist.

>
> Http_access belongs to #1. ssl_bump belongs to #2. The high-level
> Squid
> code that initiates ACL checking should know its category and use the
> right API. The low-level ACL code cannot make this determination --
> it
> should just rely on the high-level code setting the appropriate
> options
> to allow or prohibit outcomes that trigger authentication.
>
>
> Thank you,
>
> Alex.

I'm starting to think we should clean it into two separate enums.
  * The checklist results being allow/deny/dunno/auth-required like
before but enumerated clearer.
  * a match() results enum with the full set of case states for
checklist to handle any tricky conversions internally.

Amos
Received on Wed Mar 21 2012 - 00:09:21 MDT