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

[John Dennis]

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


-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jdennis-0064-2-Implement-password-based-session-login.patch
Type: text/x-patch
Size: 26856 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120227/a21dafdd/attachment.bin>