Re: [PATCH] Add auth_param request_format, request_realm to proxy authentication schemes

On 30/10/2013 5:13 a.m., Tsantilas Christos wrote:
> Hi all,
>
> The attached patch add the "auth_param request_format" and "auth_param
> request_realm" to proxy authentication schemes.
>
> The request_format value used to define the format of the helper request
> line. It is a "quoted string" with logformat %macro support. A new
> %credentials macro can be used to supply user password and other
> scheme-dependent information to the helper.
>
> The request_realm is an authenticated users cache key format, needed
> when request_format feature is used to authenticate different users with
> identical user names (e.g., when user authentication depends on http_port).
>

I dont think the idea made it out of the IRC planning discussion properly.

 We need only _one_ format called realm_format. The helper lookup can be
extended "$credentials $realm_format" and the cache key made "user
$realm_format". Where $credentials is the data already sent to the helper.

 The parameters sent to the helpers today are essentially mandatory for
those schemes validation process to operate correctly. The auth helpers
are semantically a validation API. The possibility of sending other
details to widen the authentication space around that validation is an
optional extra, not a replacement for any one of the mandatory parts or
the validation itself.
 So we can add, but not allow subtraction from the details arleady sent
an dhave to be very particular about making the extensions identical in
both lookup line and cache key.

For schemes like Digest doing it any other way compromises the message
user+realm fields which are critical for correct nonce hash generation
by exposing them unnecessarily to the admin. Similar nasty things happen
in the other schemes.

 In Basic auth there is a lot more flexible about what the helper can do
with the credentials, but that is no reason to elide them and make it an
exception for the one helper type. The helper can simply ignore
user+pass fields and use only the realm extensions details. Even in
Basic allowing two distinct formats for lookup and cache key enables the
dangerous configurations such as lookup "%credentials %>lp" mapped to
cache key "%>a %>lp".

> The Format::Format class used to produce the helper request line. This
> class requires the AccessLogEntry, so passing this class to various
> squid subsystems required.
>
> Also this patch try to fill AccessLogEntry::cache::caddr (%>A) and
> AccessLogEntry::cache::port (%la, %lp) members at the time the
> AccessLogEntry build, instead of computing them just before the request
> logged, to make them available for Format::Format class.
>
> Regards,
> Christos
>

in src/adaptation/AccessCheck.cc
* please use const AccessLogEntry::Pointer & instead of
AccessLogEntry::Pointer & for the al parameter of
Adaptation::AccessCheck::Start().
 - same in many other method parameter lists.

in src/auth/Config.cc:
* for Auth::Config::dump can you avoid use of the String::defined method
please? I belive these make the same sense using size()>0 since we
definitely do not want 0-length strings to be used.

* for Auth::Config::done() we do not have to wrap delete in an
if(exists) pattern. Just delete and set to NULL afterwards.

in src/auth/User.cci:
* please remove the useless new whitespace after
xstrdup(aString);_________\n

in src/auth/User.h:
* please use SBuf for new variables instead of String.

in src/auth/UserRequest.cc:
* in Auth::UserRequest::helperFormatedRequest s/Assebled/Assembled/

* I can almost guarantee that somebody is going to want to send Cookie
or other large details to the helper. Helpers are already required to
handle up to 32KB lines anyway.
 - So the MemBuf might as well be initialized with the defaults with
mb.init();
 - OR, since its content is being printed to buf. Use size parameter as
the upper limit for: mb.init(1024, size-1);

in src/auth/basic/auth_basic.cc:
* in authBasicAuthUserFindUsername() please remove HERE from debugs when
altering the line.

in src/auth/ntlm/UserRequest.cc:

* the YR and KK are lookups codes, not part of the credentials. They
must be first on the helper query line and not manipulable by the admin.
 - same problem in Negotiate as well.

Amos
Received on Fri Nov 15 2013 - 15:11:27 MST