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

[John Dennis]

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?

> 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
>
> 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 ...


-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/