I recall running into approximately the same situation years and years ago, although I can’t remember if it was for portlet applications or in Geronimo for ears with multiple wars with servlets that forwarded from one war to another. I think my conclusion was that the servlet spec was just wrong, and that I implemented something similar to what Felix has here. So, I think it’s quite possible that Felix’s solution is the best possible even if there are conflicts with the servlet spec.
David Jencks > On Aug 22, 2018, at 7:28 PM, Rob Walker <[email protected]> wrote: > > Carsten > > Been pondering this one some more and thrown some notes down below. Caveat to > all of them though: I am not so familiar with the recent Felix codebase in > this area; and my experience of real-world HTTP security definitely has gaps! > > So my take is that some apps (obviously not many as we're the 1st to hit this > in 6 months since the change!) would expected a session invalidate() to > destroy every aspect of that session. All attributes definitely, and also > most would expect the ID to be destroyed and not re-used. I haven't checked > the Servlet or HTTP specs for that last part, maybe it isn’t stated in a > contract anywhere. I could certainly see apps that hold their session > information in some external cache not in attributes getting tripped up with > narrow timing windows where an old session ID comes in again before a cache > is cleared. Bad coding by that app - sure, almost certainly. But also caught > out by non-standard behaviour. > > Of course it sounds like we have just as many if not more HTTP whiteboard > users that would get caught by this changed. I'm not going to argue the > current re-use of IDs is wrong, and that we need to refactor since we risk > breaking things for these users, and that is just as bad. > > So my proposal is (hopefully) a simple one. We add a property that lets users > (like us) tripped up by the wrapper behaviour, have the raw Jetty Session > object returned rather than the wrapper. That way no Felix code gets in the > way of normal Jetty handling for those not using HTTP whiteboard. > > Looking at the code, I think there's a very small number of places this would > change. Obviously getSession() is one. And then in Dispatcher we'd need to do > an instanceof test in dispatch before calling getExpiredSessionContextNames. > And I think again something similar in ServiceController. > > We could of course add a higher level wrapper, but that seems to get > heavyweight. > > One added benefit of the above is it allows use of the normal Jetty > SecureHandler code which does an automatic session invalidate/renew as part > of the auth steps. But this gets torpedoed by the use of the Felix wrapper > due to an (admittedly also nasty) instanceof test in the Jetty code. > > Cheers > > --Rob > > -----Original Message----- > From: Rob Walker > Sent: 22 August 2018 18:49 > To: Carsten Ziegeler <[email protected]>; [email protected] > Subject: RE: Session invalidation > > I didn’t check for that, will take a look. The duplicate ID was what I > noticed, and one of the things tripping me up. > -R > > -----Original Message----- > From: Carsten Ziegeler [mailto:[email protected]] > Sent: 22 August 2018 17:59 > To: [email protected]; Rob Walker <[email protected]> > Subject: Re: Session invalidation > > The new session has the same ID (and I'm not trying to imply that this is > good), but it should be a different session object being empty > > Regards > > Carsten > > > Rob Walker wrote >> What I'm seeing is that I get the same session with the same ID back, >> and it never gets invalidated. I think it has something to do with the >> wrapper not invalidating the delegate. In our local code I've patched >> the wrapper to put the delegate.invalidate() call back, and it seems >> then to be creating a new session for me (or returning null) -R >> >> -----Original Message----- >> From: Carsten Ziegeler [mailto:[email protected]] >> Sent: 22 August 2018 17:33 >> To: [email protected]; Rob Walker <[email protected]> >> Subject: Re: Session invalidation >> >> Hmm, maybe I'm missing something here, so you call >> getSession().invalidate(). This should invalidate this session and clear all >> attributes. >> >> If another request comes in using the same session id, a >> getSession(false) should return null. >> >> A getSession() should return a new session which is empty. >> >> So this should follow the servlet API contract. >> >> Or do you experience different results here? >> >> Regards >> >> Carsten >> >> >> Rob Walker wrote >>> Well for the short term, I've just copied the source for HttpSessionWrapper >>> into our gradle re-bundling script, and just added back in the >>> delegate.invalidate() call at the end. So at least for now, we've got a >>> local solution that lets us deal with this. >>> >>> I'm afraid I'm not sure what the proper / general solution is. >>> >>> It would seem to me though that the current code doesn't honour the Servlet >>> contact i.e. getSession().invalidate() does not invalidate the underlying >>> HTTP session. >>> >>> I'm not sure I fully understood the issue with the http whiteboard as it's >>> not a service we use. But it feels like in order to allow that to work, we >>> are actually breaking an underlying fundamental operation of HTTP servlets. >>> That feels kinda wrong to me, since it could break all sorts of usage. In >>> our case, it allows session hijacking by cloning the session ID from the >>> cookie or URL parameter. That's a pretty nasty security breach, and one >>> that possibly could hit others who are relying on session invalidate to >>> clear tokens. >>> >>> Anyhow, enough from me, wanted to highlight the issue at least >>> >>> -Rob >>> >>> -----Original Message----- >>> From: Carsten Ziegeler [mailto:[email protected]] >>> Sent: 22 August 2018 17:08 >>> To: [email protected]; Rob Walker <[email protected]> >>> Subject: Re: Session invalidation >>> >>> This is a problem of the http whiteboard wrt to session handling. For each >>> context managed by the whiteboard you get a separate session. These >>> sessions are managed in the real http session. >>> >>> Now I assume you invalidate one of the per context managed sessions. >>> This does not invalidate the real http session as the http whiteboard >>> implementation does not know whether anything else still wants to use it. >>> Imagine you have another application running which happens to use the same >>> http session - and that app is not managed by the whiteboard. >>> >>> The basic idea is that the real session times out eventually. >>> >>> I'm not sure what a good way of invalidating the http session in this >>> case is :( >>> >>> Regards >>> Carsten >>> >>> Rob Walker wrote >>>> So one more from me today - I'm a little perplexed on session invalidation. >>>> >>>> >>>> >>>> In common with general security best practice on HTTP, we invalidate >>>> the session ID obtained during initial logon and create a new one >>>> for the auth'd and logged on user. This helps prevent session >>>> sniffing and spoofing because the initial session ID can become visible >>>> and disclosed. >>>> >>>> >>>> >>>> While updating to newer Felix HTTP Jetty the session ID never seems >>>> to get invalidated. We always seem to get the same ID back even >>>> after we try and invalidate >>>> >>>> >>>> >>>> Digging into the code of HttpSessionWrapper shows that the Jetty >>>> delegate invalidate never gets called. >>>> >>>> >>>> >>>> Here's where it gets weird though. It looks like a mod was committed >>>> by Carsten on 29/3/2018 to explicitly remove the delegate invalidate >>>> quiet recently >>>> >>>> >>>> >>>> SHA-1: f86428f2689e62aafe750d1905fff4f5136ab67e >>>> >>>> >>>> >>>> ** FELIX-5819 : Container session should not be invalidated* >>>> >>>> >>>> >>>> git-svn-id: https://svn.apache.org/repos/asf/felix/trunk@1827956 >>>> 13f79535-47bb-0310-9956-ffa450edef68 >>>> >>>> >>>> >>>> >>>> >>>> At which point I get thoroughly confused! Clearly there must be >>>> something I'm missing >>>> >>>> >>>> >>>> ---- >>>> >>>> Rob Walker >>>> >>>> >>>> >>>> cid:[email protected] >>>> >>>> >>>> >>>> www.ascert.com >>>> >>>> [email protected] >>>> >>>> SA +27 21 300 2028 >>>> >>>> UK +44 20 7488 3470 ext 5119 >>>> >>>> >>>> >>> -- >>> Carsten Ziegeler >>> Adobe Research Switzerland >>> [email protected] >>> >> -- >> Carsten Ziegeler >> Adobe Research Switzerland >> [email protected] >> > -- > Carsten Ziegeler > Adobe Research Switzerland > [email protected]
