[Freeipa-devel] [PATCH 82] Compliant client side session cookie behavior

[John Dennis]

On 12/07/2012 03:44 PM, Rob Crittenden wrote:
> John Dennis wrote:
>> Revised patch attached.
>>
>
> Why catch exceptions from client_session_keyring_keyname() when it
> doesn't raise any?

It may not explicitly raise an exception but one can still be raised if 
either KEYRING_COOKIE_NAME or principal is invalid. It's not likely that 
KEYRING_COOKIE_NAME would be invalid but the principal might be due to 
logic failures earlier.


> In store_session_cookie() shouldn't we log an error if a cookie can't be
> parsed, not a debug?

Good point. Actually there is another problem there, if None is returned 
we need to stop processing and return. I've fixed both.

>
> In apply_session_cookie() I think we should log Cookie.URLMismatch and
> Exception at the error level instead of debug.

Good point, changed that to an error message as well as the catch-all 
handler immediately below it.


> My knowledge of cookies is rusty, but I don't understand this bit in
> path_valid()
>
> +            if not url_path or not url_path.startswith('/'):
> +                request_path = '/'
> +            elif url_path.count('/') <= 1:
> +                request_path = '/'
> +            elif url_path.endswith('/'):
> +                request_path = url_path[:-1]
> +            else:
> +                request_path = url_path
>
> If my url_path cis /ipa isn't this going to treat it as "/"? That seems
> wrong.

I agree, that's confusing, it confused me too, but that's exactly what's 
in the RFC (http://tools.ietf.org/html/rfc6265#page-16). I stared at 
that a long time myself with exactly the same concerns.

I'd appreciate it you or someone else would look at the RFC because I 
wonder if I'm not reading it correctly. I tend to agree that check is wrong.

I'll send a revised patch with the above mentioned fixes once someone 
else puts their eyeballs on the RFC, or maybe we should just remove the 
check for the time being.

> Functionally the patch appears to be fine.
>
> rob
>


-- 
John Dennis <jdennis at redhat.com>

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