[Freeipa-devel] [PATCH 64] Implement password based session login

[Rob Crittenden]

John Dennis wrote:
> On 02/27/2012 05:53 PM, Rob Crittenden wrote:
>> John Dennis wrote:
>>> On 02/27/2012 01:50 PM, Rob Crittenden wrote:
>>>> John Dennis wrote:
>>>>> Attached is a revised patch, it addresses the following concerns
>>>>> raised
>>>>> during review:
>>>>>
>>>>> * The version in ipa.conf has been bumped.
>>>>>
>>>>> * Rob reported duplicate session cookies being returned. As far as
>>>>> I can
>>>>> tell this was due to a Python bug where it reused the value of a
>>>>> default
>>>>> keyword parameter from a previous invocation rather than
>>>>> re-initializing
>>>>> it. Workaround is to change the default value from [] to the value to
>>>>> None and create an empty list if the arg is None.
>>>>>
>>>>> * Rob reported two test failures, one for ERRNO (e.g. **1234**) not
>>>>> being present in the docstring for an error I added and the other was
>>>>> for a change in the wsgi dispatch route() method that showed up in
>>>>> test_rpcserver.py
>>>>
>>>> The Requires on krb5-workstation is not required. The server requires
>>>> the client which requires it.
>>>
>>> Yeah, I wasn't sure about that, now I know.
>>>
>>> OK, fixed
>>>
>>>> I think you need a more unique way of generating the ccache name when
>>>> doing the kinit (I'd use tempfile.mkstemp).
>>>
>>> Why? The session code is already set up to use a "temporary" ccache file
>>> for each request it processes (your suggestion). The file is created
>>> when the request comes, exists for the duration of the request, and is
>>> deleted when the request completes. Is there a compelling reason to
>>> treat initializing a ccache in a request from using a ccache in a
>>> request?
>>
>> Ok, I think this is fine. I thought you were storing the name of the
>> file and since there is no way to predict which Apache subprocess would
>> handle a given request there would have been a chance of collision.
>> Since you always use the current pid as the name it is fine (as long as
>> we never use the worker model).
>>
>>>
>>>> There is an incorrect comment in internal_error()
>>>
>>> Good catch, OK fixed.
>>>>
>>>> You want to return 401 Unauthorized and not 403 Forbidden on password
>>>> failures.
>>>
>>> That wasn't an accident, I read the RFC for 401 and 403. The RFC for 401
>>> states "The response MUST include a WWW-Authenticate header field
>>> (section 14.47) containing a challenge applicable to the requested
>>> resource." But we're not doing Basic Auth here and we're not returning
>>> challenges as spec'ed out in the other RFC's, this is something a bit
>>> different so I concluded 401 was not appropriate. But I also see your
>>> point about 403 not being quite right either.
>>>
>>> I'm happy to change it to 401 though, your point is well taken, it's
>>> probably closer to being correct
>>>
>>> OK, fixed
>>
>> Hmm, yeah. I think we'll need to see what the browsers do when they get
>> a 401 and no WWW-authenticate back.
>>
>>>>
>>>> We shouldn't support the GET method as the password will appear in the
>>>> logs:
>>>>
>>>> 192.168.0.1 - - [27/Feb/2012:13:46:31 -0500] "GET
>>>> /ipa/session/login_password?user=admin&password=password HTTP/1.1"
>>>> 200 -
>>>
>>> Good point, OK, fixed
>>>
>>> revised patch coming soon ...
>
> revised patch has arrived ... see attachement
>
>


ACK, pushed to master and ipa-2-2

rob