|
Jim Gallacher
jpg at jgassociates.ca
Fri May 5 09:23:39 EDT 2006
Graham Dumpleton wrote:
>
> On 05/05/2006, at 10:21 PM, Jim Gallacher wrote:
>
>> Graham Dumpleton wrote:
>>> On 24/04/2006, at 1:50 PM, Graham Dumpleton wrote:
>>>> Jim Gallacher wrote ..
>>>>> Ultimately however it seems to me there is a bug in mod_python.c
>>>>> related
>>>>> to the whole AuthAthoritative business. Consider the following code
>>>>> pulled from the python_handler function. (mod_python.c line 1412
>>>>> revision 396250):
>>>>>
>>>>> if (strcmp(phase, "PythonAuthenHandler") == 0) {
>>>>> ... snip ...
>>>>>
>>>>> if (result == HTTP_UNAUTHORIZED)
>>>>> {
>>>>> if (! conf->authoritative)
>>>>> result = DECLINED;
>>>>>
>>>>>
>>>>> conf->authoritative is initialized to 1, but we don't have an Apache
>>>>> directive to set the value. I wonder if the assumption was that
>>>>> this was
>>>>> set by AuthAuthoritative, or if it there was an oversight in not
>>>>> adding
>>>>> a new directive? Either way it's a bug. Mod_python should not concern
>>>>> itself with AuthAuthoritative, as that is for use by mod_auth, so we
>>>>> really need our on directive.
>>>>>
>>>>> As confirmation I modified python_handler to log
>>>>> conf->authoritative and
>>>>> indeed it's value is unaffected by the AuthAthoritative setting. In
>>>>> it's
>>>>> current state, PythonAuthenHander will *always* be authoritative.
>>>>>
>>>>> Other mod_auth_* modules define their own authoritative
>>>>> directives, for
>>>>> example: AuthDBMAuthoritative, AuthLDAPAuthoritative,
>>>>> AuthMySQLAuthoritative and Anonymous_Authoritative. Following the most
>>>>> common pattern I would suggest we add AuthPythonAuthoritative.
>>>>>
>>>>> This issue may also be important to
>>>>> http://issues.apache.org/jira/browse/MODPYTHON-129
>>>>
>>>> I've noted the PythonAuthenHandler code in python_handler many times
>>>> and
>>>> although I need to go back and look at it again I have been thinking
>>>> that that section of code may possibly be partly bogus and shouldn't be
>>>> in there. The warning message about req.user not being set is possibly
>>>> helpful, but why should mod_python be making a decision to change an
>>>> unauthorized response back to a declined and why should it be
>>>> generating
>>>> a WWW-Authenticate header with an assumption that Basic authorisation
>>>> is being used when it may well not be. What is going to happen if I
>>>> write
>>>> an authenhandler for Digest authentication and it returns unauthorised,
>>>> mod_python will obliterate any WWW-Authenticate header I may have
>>>> placed there specific to Digest authentication.
>>>>
>>>> Thus I don't necessarily think it is a case of amending it some way, it
>>>> may be a case of obliterating it and make people do the correct thing
>>>> in their handlers to begin with rather than providing a crutch to
>>>> fix their
>>>> omissions. This may mean adding means of calling further auth related
>>>> functions through the req object if there is something missing now,
>>>> such as access to ap_note_basic_auth_failure().
>>> FWIW, my concerns about something being a bit wrong with the code are a
>>> baseless in as much as ap_note_basic_auth_failure() only adds the header
>>> is AuthType is set to Basic. Thus it cant obliterate a Digest header.
>>
>> That is incorrect. ap_note_basic_auth_failure() checks if the auth
>> type is "Basic". If so it sets the headers, otherwise it calls
>> ap_note_auth_failure(). ap_note_auth_failure() also checks the auth
>> type and will either call ap_note_basic_auth_failure if the type is
>> "Basic", ap_note_digest_auth_failure() if the type is "Digest" or else
>> just log an error.
>>
>> ap_note_digest_auth_failure will sets the headers appropriately for
>> digest authentication.
>>
>> This doesn't mean we are correct in calling
>> ap_note_basic_auth_failure, but doing so will not cause any problems
>> as long as the AuthType is set properly.
>
> Me wrong. If this call is going to be made then, it may as well call
> ap_note_auth_failure()
> then instead of ap_note_basic_auth_failure(). At least then it is obvious.
>
> Anyway, makes sense now as I was seeing warning in error log of:
>
> need AuthType to note auth failure
>
> which is generated by ap_note_auth_failure() when AuthType not set. I
> think I
> somehow convinced myself at one point that I didn't see it and it must
> have been
> from something else I was doing.
>
> Too tired. Which makes me think I better not commit this code I was
> about to commit
> for something else. :-)
That is always prudent. There is nothing quite like committing some code
late a night and realizing you may have done something that would break
the build. The question then becomes fix or sleep...
> BTW, what do you think of:
>
> http://issues.apache.org/jira/browse/MODPYTHON-169
The short answer is I like it. I've been reading the Apache2.2
auth/authz stuff in modules/aaa with my morning coffee. :) I certainly
think we need a general review how we handle this stuff so that
mod_python can play nice with Apache 2.2.
Jim
|