Re: [PATCH] Bug 3342: username token for external ACL without triggering auth validation

On 12/16/2011 09:54 PM, Amos Jeffries wrote:
> Original design by "arronax28".
>
> This adds a %un token (different to %LOGIN and %EXT_USER) which passes
> any pre-known username details to the external ACL helper. But does not
> trigger or require authentication verifications.
>
> This will not process auth headers if presented but not yet
> authenticated. But it will allow IDENT and external ACL out-of-band
> authorization usernames to be sent to the helper.
>
> On the upside it will solve some of the cases where people want to
> process usernames without accidental auth challenges.
>
> On the downside I am expecting some small amount of confusion as admin
> send HTTP auth headers and expect Squid to magically understand them
> without doing any auth processing.

I think this is a useful feature but I find the proposed squid.conf
documentation inadequate. We should list the exact sources where the
"user name" will come from, in the right order, similar to how it is
documented in the code. I am not an authentication expert of course, but
the current "any name available" definition seems unnecessary vague to me.

> + case _external_acl_format::EXT_ACL_USERNAME:
> + // find any name from: auth, ext ACL, ssl cert, and rfc931 usernames; in that order of preference.
> + if (!str && ch->auth_user_request != NULL)

The leading !str condition implies that the name may be already set
before we get to this case. Please remove unless that is indeed
possible. Other !str conditions that follow are necessary, of course.

> + if(!str && strcmp(request->extacl_user.termedBuf(), "-") != 0)
> + str = request->extacl_user.termedBuf();

No objection, but it would be a lot faster to compare using size()==1
and [0]=='-' conditions than to terminate the buffer and do strcmp() on
that.

> - %LOGIN Authenticated user login name
> + %LOGIN Authenticated user login name. Will perform authenticateion
> + challenges if no valid credentials are present.
> + %un A user name. Pulls any name available from both
> + authenticated and non-authenticated sources.
> %EXT_USER Username from previous external acl
> %EXT_LOG Log details from previous external acl

%un seems inconsistent in spelling/capitalization with all other format
tags. Please consider using %USER_NAME or similar instead. BTW, have you
considered adding an optional "no-auth" parameter to the existing %LOGIN
instead?

"authenticateion" is misspelled.

Should explicitly say that %un will not trigger authentication, to
contrast with %LOGIN. In general, keep the description symmetric to
%LOGIN to the extent possible, to emphasize both the similarities and
the differences.

Should we document what the %un value is when no information is
available (a dash?) or do we already document that elsewhere?

Thank you,

Alex.
Received on Wed Dec 21 2011 - 17:00:02 MST