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

[Rob Crittenden]

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