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 ...
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
