server: Purge all cookies on logout, set /client path on login #4176

yadvr · 2020-06-24T17:02:34Z

This will purge all the cookies on logout including multiple sessionkey
cookies if passed. On login, this will restrict sessionkey cookie
(httponly) to the /client path like the JSESSIONID cookie.

Fixes #4136

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Needs manual testing

This will purge all the cookies on logout including multiple sessionkey cookies if passed. On login, this will restrict sessionkey cookie (httponly) to the /client path like the JSESSIONID cookie. Fixes apache#4136 Signed-off-by: Rohit Yadav <[email protected]>

yadvr · 2020-06-24T17:02:59Z

Please review and test @davidjumani @shwstppr cc @Pearl1594

yadvr · 2020-06-24T17:03:13Z

@blueorangutan package

blueorangutan · 2020-06-24T17:04:02Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

yadvr · 2020-06-24T17:04:28Z

server/src/main/java/com/cloud/api/ApiServlet.java

                        responseString = apiAuthenticator.authenticate(command, params, session, remoteAddress, responseType, auditTrailSb, req, resp);
                        if (session != null && session.getAttribute(ApiConstants.SESSIONKEY) != null) {
-                            resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY)));
+                            resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client", ApiConstants.SESSIONKEY, session.getAttribute(ApiConstants.SESSIONKEY)));


We need to check if this is necessary or not

@davidjumani please explore if this would work, without adding the /client path to the cookie; as I worry if this change can potentially break non-standard env (those which don't use the default context path of /client as defined in /etc/cloudstack/management/server.properties)

Looks like it is necessary, if not present, two sessionkeys are present
If non-standard implementations are the case, wouldn't JSESSIONID cause issues too?
Also exploring saml logins

@davidjumani in a deploy trillian env with this PR (or you may use the primate-qa server as well which has this PR) and both old UI and Primate, can you check side-effects when you change the context in /etc/cloudstack/management/server.properties from /client/ to say (a) /somenew-path and (b) / and restart management server and try log-in in both legacy UI and Primate (check network an Application/cookies)

Nevermind I tested, this will break if path is restricted to /client only; ideally shoud match the webapp context path

yadvr · 2020-06-24T17:21:22Z

@blueorangutan package

blueorangutan · 2020-06-24T17:22:02Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-24T17:31:19Z

Packaging result: ✖centos7 ✖debian. JID-1452

shwstppr · 2020-06-24T18:11:31Z

server/src/main/java/com/cloud/api/ApiServlet.java

-                        sessionKeyCookie.setMaxAge(0);
-                        resp.addCookie(sessionKeyCookie);
+                        final Cookie[] cookies = req.getCookies();
+                        if (cookies != null)


@rhtyd missing brace

yadvr · 2020-06-24T18:29:52Z

@blueorangutan package

blueorangutan · 2020-06-24T18:31:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-24T18:50:16Z

Packaging result: ✔centos7 ✔debian. JID-1453

yadvr · 2020-06-24T19:29:04Z

@blueorangutan test

blueorangutan · 2020-06-24T19:30:02Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2020-06-25T15:15:29Z

@blueorangutan package

blueorangutan · 2020-06-25T15:16:23Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-06-25T15:40:54Z

Packaging result: ✔centos7 ✖debian. JID-1462

davidjumani · 2020-06-26T04:49:07Z

Tested for local / saml users. LGTM

yadvr · 2020-06-26T05:17:44Z

@davidjumani did you also test changing the context path from server properties?

davidjumani · 2020-06-26T10:57:56Z

Works when the context path is changed to /something but fails when just set to /

Pearl1594

LGTM. This PR addresses the core issue of handling multiple session keys (closing #4166)

yadvr · 2020-07-04T05:53:20Z

@blueorangutan package

blueorangutan · 2020-07-04T05:53:55Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-07-04T07:48:45Z

Packaging result: ✔centos7 ✔debian. JID-1531

yadvr · 2020-07-04T07:53:38Z

@blueorangutan test

blueorangutan · 2020-07-04T07:55:13Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2020-07-05T10:22:01Z

@blueorangutan test

blueorangutan · 2020-07-05T10:23:22Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2020-07-06T04:32:22Z

@blueorangutan test

blueorangutan · 2020-07-06T04:33:04Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

yadvr · 2020-07-06T09:35:25Z

@davidjumani can you help review/test/fix, thnx

yadvr · 2020-07-07T13:16:04Z

@Pearl1594 @davidjumani any progress on this?

Pearl1594 · 2020-07-07T14:28:40Z

@blueorangutan package

Pearl1594 · 2020-07-07T14:29:07Z

@rhtyd looking into it

blueorangutan · 2020-07-07T14:29:10Z

@Pearl1594 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-07-07T14:49:37Z

Packaging result: ✔centos7 ✔debian. JID-1551

Pearl1594 · 2020-07-07T16:18:07Z

@rhtyd it works now - for both changing context.path to a value other that /client and for /
Have to test it for SAML login

Pearl1594 · 2020-07-07T16:18:43Z

@blueorangutan test

blueorangutan · 2020-07-07T16:19:10Z

@Pearl1594 a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-07-08T00:41:00Z

Trillian test result (tid-2026)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28097 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4176-t2026-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	176.97	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	176.08	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	245.60	test_privategw_acl.py

yadvr · 2020-07-08T02:33:00Z

Tests lgtm

…e#4176) This will purge all the cookies on logout including multiple sessionkey cookies if passed. On login, this will restrict sessionkey cookie (httponly) to the / path. Fixes apache#4136 Co-authored-by: Pearl Dsilva <[email protected]>

yadvr added type:bug component:api Severity:Critical Critical bug labels Jun 24, 2020

yadvr added this to the 4.13.2.0 milestone Jun 24, 2020

yadvr requested a review from andrijapanicsb June 24, 2020 17:02

yadvr commented Jun 24, 2020

View reviewed changes

yadvr changed the base branch from master to 4.13 June 24, 2020 17:21

apache deleted a comment from blueorangutan Jun 24, 2020

shwstppr reviewed Jun 24, 2020

View reviewed changes

Add brace

15be473

set cookie path to webapp context path

d629eb1

Pearl1594 approved these changes Jun 26, 2020

View reviewed changes

Update ApiServlet.java

b7e88bc

Change sessionkey cookie path

6605d01

apache deleted a comment from blueorangutan Jul 8, 2020

yadvr merged commit 139aa13 into apache:4.13 Jul 8, 2020

Pearl1594 mentioned this pull request Aug 5, 2020

Unable to login with latest master 2020/08/04 #4242

Closed

server: Purge all cookies on logout, set /client path on login #4176

server: Purge all cookies on logout, set /client path on login #4176

Uh oh!

Conversation

yadvr commented Jun 24, 2020

Types of changes

Uh oh!

yadvr commented Jun 24, 2020

Uh oh!

yadvr commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

yadvr Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

davidjumani Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

shwstppr Jun 24, 2020

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

yadvr commented Jun 24, 2020

Uh oh!

blueorangutan commented Jun 24, 2020

Uh oh!

yadvr commented Jun 25, 2020

Uh oh!

blueorangutan commented Jun 25, 2020

Uh oh!

blueorangutan commented Jun 25, 2020

Uh oh!

davidjumani commented Jun 26, 2020

Uh oh!

yadvr commented Jun 26, 2020

Uh oh!

davidjumani commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Jul 4, 2020

Uh oh!

blueorangutan commented Jul 4, 2020

Uh oh!

blueorangutan commented Jul 4, 2020

Uh oh!

yadvr commented Jul 4, 2020

Uh oh!

blueorangutan commented Jul 4, 2020

Uh oh!

yadvr commented Jul 5, 2020

Uh oh!

blueorangutan commented Jul 5, 2020

Uh oh!

yadvr commented Jul 6, 2020

Uh oh!

blueorangutan commented Jul 6, 2020

davidjumani commented Jun 26, 2020 •

edited

Loading