fix session sharing and simplify logout #1032

ahgittin · 2019-01-27T02:37:18Z

have a shared session available for the various bundles

mainly to know if each other have authorized. see 1a15c36 for the main new code, in MultiSessionAttributeAdapter.
note this has some historic aborted stuff, in case we need to revisit. that can be ignored, best to evaluate this not commit-by-commit!

also...

this makes /logout just log out and simplifies it

previously it redirected to the user-specific logout; but some clients didn't respect that, and assumed the logout had happened.
not convinced there is a good reason for /logout/{user} ... and if there is, you can now use /logout/redirect to get to it.

calls SecurityProvider.logout and correctly invalildates everything on logout

previously that was neglected

CSRF fail returns `403 FORBIDDEN` not `401 UNAUTHORIZED`

it's more accurate, and allows clients to distinguish problems from auth fails

previously it redirected to the user-specific logout; but some clients didn't respect that, and assumed the logout had happened. not convinced there is a good reason for /logout/{user} ... and if there is, you can now use /logout/redirect to get to it.

this takes the first encountered session handler and caches it, also setting the sessonPath on the cookies so cookies are valid for all bundles. some low-level hacking to work around pain that CXF doesn't let you configure session handling; if there is a nicer way i'd love to see it, but i think it would require rewriting chunks of CXF

ahgittin · 2019-01-28T02:05:13Z

experimenting to see what makes sense

the browser seems to keep the local cached creds, even on a 401 unauthorized, so not sure if there's much point in returning that

fixes tests which cycle the server

…thorization, we need the client to correctly encode the CSRF header.

ahgittin · 2019-01-29T01:18:17Z

mostly fixes problem at #1033 and makes it nicer to work with from the uI, including invalidating locally-stored creds

still to do:

push the corresponding logout UI fixes
figure out whether the Authorization header being kept on the looping /extended call can be made to be dropped after the first call, so it doesn't cache the login, and confirm this means when you've logged out you are properly logged out
determine what code is creating a session when the user isn't authenticated
check if time-based invalidation (without manual cache deletion) causes the server to crash
invoke security provider's logout method

see MultiSessionAttributeAdapter. this replaces the dodgy brittle request.setSession(...) that we had tried previously.

ahgittin · 2019-01-29T19:36:40Z

done most the list in previous comment. confirmed logout works nicely in browser (unless the /extended call is done in another tab), invalidation just works without problems using new method, and no extraneous sessions are created after logout.

one more issue i've noticed: unauthorized requests create a session which is kept; this is a memory leak and needless. i'll have a quick look to see if that can be undone.

remaining changes will be in PR(s) in other project (UI):

push corresponding logout UI fixes
figure out whether the Authorization header being kept on the looping /extended call can be made to be dropped after the first call, so it doesn't cache the creds in the app and re-log you in
check the ui-module bundles (e.g. module registry) have the same auth/filter and session management configuration

…Provider API without this DOS attacks could hurt quite quickly note that this requires further changes to the SecurityProvider implementations (in addition to the requireUserPass() addition), giving them finer-grained access to the request and the session

ahgittin · 2019-01-30T00:01:40Z

looking good. have updated all other items in the list above at:

tidy logout and other misc minor changes for new server security model brooklyn-ui#120

(i can't make basic auth fully always disabled, but that's a well-known problem; i've done what i could and added some info about it)

also see minor change needed to brooklyn-dist itests:

update for security provider API change brooklyn-dist#139

these two PRs should be reviewed along with this one

all ready for review and (i hope) merge

tbouron

Thank you @ahgittin, it looks great and clean, knowing the fact that cxf doesn't let you share sessions.

I'm going to test it locally with the other PRs to make sure it is working as expected but didn't want to hold my review in the meantime. I just a very small comments but the rest is brilliant

tbouron · 2019-01-30T10:48:02Z

karaf/jetty-config/src/main/resources/jetty.xml

@@ -20,47 +20,4 @@
 <!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_0.dtd">

 <Configure id="Server" class="org.eclipse.jetty.server.Server">


Do we even need this file now that we don't configure Jetty with these beans?

I'm not sure; I think it might break if we remove it. Note that you can't configure the ServletContext / WebAppContext (where SessionHandler is set) at this jetty server-wide level.

tbouron · 2019-01-30T10:52:38Z

rest/rest-api/src/main/java/org/apache/brooklyn/rest/api/LogoutApi.java

+    @ApiOperation(value = "Logout and clean session")
+    Response logout(
+            @ApiParam(value = "Instead of 200 (the default) to indicate successful logout, "
+                + "return a 401 with this value in a message key in the body (a 401 will cause browsers to clear some locally cached credentials)", 


I'm not too sure about returning a 401 for a successful operation, that is counter-intuitive. I got you on the argument that a 401 will make the browser clear the cache but should it be the responsibility of the REST API to take care of that? I don't think so IMO.

Cannot we return a 200 and handle the credential cache in the UI directly?

No, if we return 200 browsers keep the credentials. But note that the API does return 200 unless this unauthorize parameter is supplied. This parameter really exists to help UI clients encourage the browser clear. Pretty sure that's the nicest pattern.

Have updated one comment in LogoutResource to explain better; in short there is no way that either the server or the client can 100% clear creds at present, but this appears to be the best we can do.

tbouron · 2019-01-30T11:02:12Z

rest/rest-resources/src/main/java/org/apache/brooklyn/rest/resources/LogoutResource.java

+//            ((Request)jreq).setSessionHandler(new TrackingSessionHandler(((Request)jreq).getSessionHandler()));
+//        } else {
+//            log.warn("UNABLE to swap request"+MultiSessionAttributeAdapter.info(jreq));
+//        }


Can be removed

I'd like to keep for the reasons noted in the comment in this section

// if we need to intercept session creation then can use this // create TrackingSessionHandler which delegates (no-op if delegate==null esp in setSessionTrackingMode) // and log with stack trace in newHttpSession

I know we could restore it from history but unless/until we are really happy with this behaviour I think it is worth making it a little more prominent.

Added

// can remove this after we've been running happily with new session management model for a while

tbouron · 2019-01-30T11:19:47Z

...rest-resources/src/main/java/org/apache/brooklyn/rest/util/MultiSessionAttributeAdapter.java

+            if (handlers != null) {
+                for (Handler handler: handlers) {
+                    SessionHandler sh = (SessionHandler) handler;
+                    ContextHandler.Context ctx = getContext(sh);


I'm a bit concerned performance wise about this call, as getContext(sh) uses refection. This is done within a loop of handler which can be potentially big.

If I read this code correctly, this will be executed only during the session creation, if there is no other sessions marked as preferred, so it is somewhat mitigated. But still, I'm wondering if it could be a bottleneck

There is no other way I don't think. Reflection isn't that slow so I wouldn't expect a problem (CXF does it in a few places too, where the server version might be different). If performance analysis shows it as a bottleneck let's reconsider. But we don't do this often (wouldn't expect more than one per second, probably one per ten minutes in a normal-to-heavy server) on the UI. We do it more often with the CLI (every request) so it might be worth adding a header which says X-No-Session-Needed to suppress session creation (I'm 95% sure other aspects of session creation and management are more significant than this reflection call).

ahgittin · 2019-01-31T10:26:55Z

Note as per #1034 I'v done a full rebuild including dist and it seems to be working great now.

tbouron

LGTM, thanks @ahgittin.

Merging this now as the previous commit passed, and the new changes include only comments

ahgittin added 5 commits January 27, 2019 02:27

make /logout just log out

b1d1b11

previously it redirected to the user-specific logout; but some clients didn't respect that, and assumed the logout had happened. not convinced there is a good reason for /logout/{user} ... and if there is, you can now use /logout/redirect to get to it.

fix security provider delegation

c21bdc8

Merge branch 'make-logout-just-logout' into misc8

c30d4c9

simplify logout api

39cd453

ahgittin added 5 commits January 28, 2019 09:52

reinstall shared session handler if it has stopped

6417a79

fixes tests which cycle the server

reinstall shared session handler if it has stopped

bb9a38f

fixes tests which cycle the server

CSRF should return "forbidden" not "unauthorized" as we don't need au…

cb576a8

…thorization, we need the client to correctly encode the CSRF header.

lots of debugging for case where session invalidation hangs on logout

7cbf124

tidy, removing debug code, just having a clean-ish solution

b356e52

ahgittin mentioned this pull request Jan 29, 2019

Share sessions among all bundles so we can share auth #1033

Closed

ahgittin added 3 commits January 29, 2019 01:11

make double-check/initialized field volatile

1518c1c

make double-check/initialized field volatile

3be6eac

Merge branch 'fix-sessions-4' into make-logout-just-logout

c4e245b

ahgittin force-pushed the make-logout-just-logout branch from 02696b1 to c4e245b Compare January 29, 2019 01:13

ahgittin added 2 commits January 29, 2019 11:32

fix test for csrf mismatch now returns forbidden

11d6f14

use adapter to access a shared session when required

1a15c36

see MultiSessionAttributeAdapter. this replaces the dodgy brittle request.setSession(...) that we had tried previously.

ahgittin changed the title ~~make /logout just log out~~ fix session sharing and simplify logout Jan 29, 2019

ahgittin force-pushed the make-logout-just-logout branch from 55ef6ef to 1d6f63a Compare January 29, 2019 20:06

ahgittin mentioned this pull request Jan 29, 2019

update for security provider API change apache/brooklyn-dist#139

Merged

better error-handling for bad auth strings

f6df273

ahgittin mentioned this pull request Jan 29, 2019

tidy logout and other misc minor changes for new server security model apache/brooklyn-ui#120

Merged

tbouron reviewed Jan 30, 2019

View reviewed changes

ahgittin mentioned this pull request Jan 31, 2019

Fix dependency versions #1034

Merged

comments addressing PR questions

dc30572

tbouron approved these changes Jan 31, 2019

View reviewed changes

tbouron merged commit f54c1e2 into apache:master Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix session sharing and simplify logout #1032

fix session sharing and simplify logout #1032

ahgittin commented Jan 27, 2019 •

edited

ahgittin commented Jan 28, 2019

ahgittin commented Jan 29, 2019 •

edited

ahgittin commented Jan 29, 2019

ahgittin commented Jan 30, 2019

tbouron left a comment •

edited

tbouron Jan 30, 2019

ahgittin Jan 31, 2019

tbouron Jan 30, 2019

ahgittin Jan 31, 2019

tbouron Jan 30, 2019

ahgittin Jan 31, 2019

tbouron Jan 30, 2019

ahgittin Jan 31, 2019

ahgittin commented Jan 31, 2019

tbouron left a comment

		@@ -20,47 +20,4 @@
		<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_0.dtd">

		<Configure id="Server" class="org.eclipse.jetty.server.Server">

fix session sharing and simplify logout #1032

fix session sharing and simplify logout #1032

Conversation

ahgittin commented Jan 27, 2019 • edited

have a shared session available for the various bundles

this makes /logout just log out and simplifies it

calls SecurityProvider.logout and correctly invalildates everything on logout

CSRF fail returns 403 FORBIDDEN not 401 UNAUTHORIZED

ahgittin commented Jan 28, 2019

ahgittin commented Jan 29, 2019 • edited

ahgittin commented Jan 29, 2019

ahgittin commented Jan 30, 2019

tbouron left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahgittin commented Jan 31, 2019

tbouron left a comment

Choose a reason for hiding this comment

ahgittin commented Jan 27, 2019 •

edited

CSRF fail returns `403 FORBIDDEN` not `401 UNAUTHORIZED`

ahgittin commented Jan 29, 2019 •

edited

tbouron left a comment •

edited