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

[Simo Sorce]

On Fri, 2012-12-07 at 16:21 -0500, John Dennis wrote:
> 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.

I think that the algorithm fails to follow the RFC when you do:
          elif url_path.endswith('/'):
              request_path = url_path[:-1]

Point 4 of the RFC doesn't say the path needs to end with a / it says
you need to take everything before the last / wherever it is.

Ie if the patch is /ipa/ui/foo then the path for the cookie is /ipa/ui
Conversely if the path is /ipa/ui/foo/ the path is /ipa/ui/foo

Basically these rules threat the last 'leaf' component as not part of
the path and are meant to remove it.

If I read it right instead your code fails the first example ie,
both /ipa/ui/foo and /ipa/ui/foo/ give out a path of /ipa/ui/foo

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York