Update SAML2 auth sessionkey cookie path by kohrar · Pull Request #6149 · apache/cloudstack

kohrar · 2022-03-21T16:52:13Z

Description

This change will set the sessionkey under the /client/api path. This commit should prevent duplicate sessionkey cookies from being set on both /client (incorrect) and /client/api (correct). Prior to this commit, the /client version was being set while the /client/api version remained unchanged with an invalid sessionkey. As a result, subsequent requests after the SAML2 authentication would immediately fail with an invalid session and results in the user being logged out.

The sessionkey is now set explicitly for the /client/api path which should fix this issue, regardless of the SSO URL and path that's being used.

Fixes: #6127

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

I've built 4.16.1.0 with this change and SAML2 auth is now working as expected again.

This change will set the sessionkey under the /client/api path. This commit should prevent duplicate sessionkey cookies from being set on both /client (incorrect) and /client/api (correct). Prior to this commit, the /client version was being set while the /client/api version remained unchanged with an invalid sessionkey. As a result, subsequent requests after the SAML2 authentication would immediately fail with an invalid session and results in the user being logged out. The sessionkey is now set explicitly for the /client/api path which should fix this issue, regardless of the SSO URL and path that's being used.

yadvr · 2022-03-21T17:11:42Z

 }
 resp.addCookie(new Cookie("userfullname", URLEncoder.encode(loginResponse.getFirstName() + " " + loginResponse.getLastName(), HttpUtils.UTF_8).replace("+", "%20")));
- resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", ApiConstants.SESSIONKEY, loginResponse.getSessionKey()));
+ resp.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly;Path=/client/api", ApiConstants.SESSIONKEY, loginResponse.getSessionKey()));


we might want to take the path from the configurable setting? cc @Pearl1594 to check if it makes sense to read the variable defined in server.properties

LGTM otherwise

Nevermind, my comment- Based on https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/api/ApiServlet.java#L224 your changes LGTM (the Path /client/api is more restrictive than /client but should work and ensure all APIs function. The only issue is when the path isn't /client/api which is not a case I think think of as all APIs calls are handled at /client/api)

nvazquez · 2022-04-08T00:49:21Z

Hi @kohrar is this PR still valid after the UI fixes?

kohrar · 2022-04-08T01:11:47Z

I would think so. The UI issue is a separate issue. The original issue was that the session key cookie was set with a different path by the browser, resulting in the valid session key being ignored in favor for the invalid key set previously.

The UI issue is merely an interface issue with the login page which can be worked around by refreshing the login page. The cookie issue here can't be worked around unless cookies are cleared completely.

nvazquez · 2022-04-11T14:46:30Z

@blueorangutan package

blueorangutan · 2022-04-11T14:47:02Z

@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-04-11T15:34:25Z

Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3124

nvazquez · 2022-04-12T02:31:33Z

@blueorangutan package

blueorangutan · 2022-04-12T02:33:02Z

@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-04-12T03:24:05Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3131

nvazquez · 2022-04-12T10:15:11Z

@blueorangutan test

blueorangutan · 2022-04-12T10:16:03Z

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

blueorangutan · 2022-04-12T20:37:38Z

Trillian test result (tid-3868)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35798 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6149-t3868-kvm-centos7.zip
Smoke tests completed. 88 look OK, 5 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.57	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.11	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.19	test_primary_storage.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	34.14	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	685.81	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	62.51	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	36.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	33.86	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	130.03	test_kubernetes_clusters.py
test_01_secure_vm_migration	`Error`	155.55	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	270.22	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	144.95	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	43.71	test_vm_life_cycle.py
test_02_list_snapshots_with_removed_data_store	`Error`	8.45	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	`Error`	8.45	test_snapshots.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	307.04	test_hostha_kvm.py

yadvr · 2022-04-13T11:03:04Z

@blueorangutan package

blueorangutan · 2022-04-13T11:04:03Z

@rohityadavcloud a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2022-04-13T11:58:24Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3158

nvazquez · 2022-04-14T14:14:37Z

@blueorangutan test

yadvr · 2022-04-15T15:00:56Z

@blueorangutan test

blueorangutan · 2022-04-15T15:02:03Z

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

blueorangutan · 2022-04-16T00:11:00Z

Trillian test result (tid-3920)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31620 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6149-t3920-kvm-centos7.zip
Smoke tests completed. 93 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

boring-cyborg · 2022-04-18T11:46:23Z

Awesome work, congrats on your first merged pull request!

yadvr reviewed Mar 21, 2022
View reviewed changes

nvazquez requested a review from weizhouapache March 22, 2022 14:42

weizhouapache added this to the 4.17.0.0 milestone Mar 23, 2022

nvazquez mentioned this pull request Mar 23, 2022
SSO fails with error "Expired session, missing signature, or missing apiKey" #6127
Closed

yadvr closed this Apr 6, 2022

yadvr reopened this Apr 6, 2022

boring-cyborgBot added the component:saml label Apr 6, 2022

nvazquez approved these changes Apr 18, 2022
View reviewed changes

yadvr approved these changes Apr 18, 2022
View reviewed changes

yadvr merged commit 7012200 into apache:mainApr 18, 2022

Uh oh!

Conversation

kohrar commented Mar 21, 2022

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

yadvrMar 21, 2022

Choose a reason for hiding this comment

Uh oh!

yadvrMar 21, 2022

Choose a reason for hiding this comment

Uh oh!

yadvrApr 18, 2022

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Apr 8, 2022

Uh oh!

kohrar commented Apr 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented Apr 11, 2022

Uh oh!

blueorangutan commented Apr 11, 2022

Uh oh!

blueorangutan commented Apr 11, 2022

Uh oh!

nvazquez commented Apr 12, 2022

Uh oh!

blueorangutan commented Apr 12, 2022

Uh oh!

blueorangutan commented Apr 12, 2022

Uh oh!

nvazquez commented Apr 12, 2022

Uh oh!

blueorangutan commented Apr 12, 2022

Uh oh!

blueorangutan commented Apr 12, 2022

Uh oh!

yadvr commented Apr 13, 2022

Uh oh!

blueorangutan commented Apr 13, 2022

Uh oh!

blueorangutan commented Apr 13, 2022

Uh oh!

nvazquez commented Apr 14, 2022

Uh oh!

yadvr commented Apr 15, 2022

Uh oh!

blueorangutan commented Apr 15, 2022

Uh oh!

blueorangutan commented Apr 16, 2022

Uh oh!

boring-cyborgBot commented Apr 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kohrar commented Apr 8, 2022•
edited
Loading