Add EncryptedElementType key resolver to SAML plugin by mlsorensen · Pull Request #7268 · apache/cloudstack

mlsorensen · 2023-02-21T01:03:20Z

Description

There are multiple ways in which a SAML response can be formatted, especially when encryption is enabled. This PR removes the hardcoding of EncryptedKeyResolver= InlineEncryptedKeyResolver in favor of using a ChainingEncryptedKeyResolver which will try multiple resolvers. It preserves the InlineEncryptedKeyResolver as the first option but adds EncryptedElementTypeEncryptedKeyResolver to the chain of resolvers to try.

ChainingEncryptedKeyResolver is a bit finicky in that you can't provide it a list of resolvers, you can only fetch its internal list and add to it.

Theoretically we could add all of the resolver types to the chain, but for now just preserving the ones known to be in use.

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

How Has This Been Tested?

Tested locally against an installed IDP that provides the EncryptedElementType of XML response.

codecov · 2023-02-21T04:16:05Z

Codecov Report

Merging #7268 (bf7b205) into main (dd51534) will increase coverage by 2.29%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #7268      +/-   ##
============================================
+ Coverage     10.38%   12.67%   +2.29%     
- Complexity     6651     8641    +1990     
============================================
  Files          2453     2716     +263     
  Lines        242444   256114   +13670     
  Branches      37941    39926    +1985     
============================================
+ Hits          25168    32461    +7293     
- Misses       214159   219524    +5365     
- Partials       3117     4129    +1012

Impacted Files	Coverage Δ
.../cloudstack/api/response/ApiDiscoveryResponse.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../cloudstack/api/response/ApiParameterResponse.java	`0.00% <0.00%> (-100.00%)`	⬇️
...va/com/cloud/upgrade/dao/DatabaseAccessObject.java	`0.00% <0.00%> (-97.50%)`	⬇️
...ain/java/com/cloud/upgrade/dao/DbUpgradeUtils.java	`0.00% <0.00%> (-92.31%)`	⬇️
...e/cloudstack/api/response/ApiResponseResponse.java	`0.00% <0.00%> (-80.00%)`	⬇️
.../cloudstack/discovery/ApiDiscoveryServiceImpl.java	`13.93% <0.00%> (-55.25%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade444to450.java	`0.00% <0.00%> (-50.00%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade453to460.java	`0.00% <0.00%> (-50.00%)`	⬇️
...java/com/cloud/upgrade/DatabaseUpgradeChecker.java	`0.00% <0.00%> (-46.67%)`	⬇️
...in/java/com/cloud/upgrade/dao/Upgrade431to440.java	`0.00% <0.00%> (-40.00%)`	⬇️
... and 753 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sonarqubecloud · 2023-02-21T12:56:46Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

mlsorensen · 2023-02-21T21:07:52Z

I'm not sure what's going on with that Jenkins build. I can see that it is looking for org.opensaml:opensaml:pom:2.6.7-SNAPSHOT and not finding it. However I haven't changed the version (it is 2.6.6 in the pom), and it builds fine locally, so I'm not sure where it's coming up with 2.6.7-SNAPSHOT.

[WARNING] Unable to create Maven project for org.opensaml:opensaml:pom:2.6.7-SNAPSHOT from repository.
org.apache.maven.project.ProjectBuildingException: Error resolving project artifact: org.opensaml:opensaml:pom:2.6.7-SNAPSHOT was not found in https://repo.jenkins-ci.org/public/ during a previous attempt. This failure was cached in the local repository and resolution is not reattempted until the update interval of repo.jenkins-ci.org.public has elapsed or updates are forced for project org.opensaml:opensaml:pom:2.6.7-SNAPSHOT

mlsorensen · 2023-02-21T21:25:03Z

Ok looks like I'm not the only PR failing in the same way...

yadvr · 2023-02-24T07:41:48Z

That looks like a warning as it's trying to download/fetch opensaml dependency from a maven repo which doesn't have it. Ignorable as build works.

@blueorangutan package

blueorangutan · 2023-02-24T07:42: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.

yadvr

code lgtm, this would need manual regression testing with a known SAML provide such as https://samltest.id etc.

blueorangutan · 2023-02-24T08:39:31Z

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

DaanHoogland · 2023-02-24T13:58:43Z

@blueorangutan test

blueorangutan · 2023-02-24T13:59:03Z

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

blueorangutan · 2023-02-25T01:44:52Z

Trillian test result (tid-6232)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40912 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7268-t6232-kvm-centos7.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland

clgtm

yadvr · 2023-03-17T05:29:03Z

@blueorangutan package

blueorangutan · 2023-03-17T05:30: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 · 2023-03-17T06:22:39Z

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

yadvr · 2023-03-31T09:33:16Z

@blueorangutan package

blueorangutan · 2023-03-31T09:47:07Z

@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.

borisstoyanov · 2023-03-31T09:50:27Z

@blueorangutan test keepEnv

blueorangutan · 2023-03-31T09:51:05Z

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

blueorangutan · 2023-03-31T10:52:07Z

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

borisstoyanov · 2023-03-31T12:24:59Z

@blueorangutan test keepEnv

blueorangutan · 2023-03-31T12:26:03Z

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

blueorangutan · 2023-04-01T00:30:24Z

Trillian test result (tid-6353)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41964 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7268-t6353-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors, 16 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
all_test_accounts	`Skipped`	---	test_accounts.py
all_test_over_provisioning	`Skipped`	---	test_over_provisioning.py
all_test_affinity_groups	`Skipped`	---	test_affinity_groups.py
all_test_ipv6_infra	`Skipped`	---	test_ipv6_infra.py
all_test_usage	`Skipped`	---	test_usage.py
all_test_affinity_groups_projects	`Skipped`	---	test_affinity_groups_projects.py
all_test_resource_detail	`Skipped`	---	test_resource_detail.py
all_test_annotations	`Skipped`	---	test_annotations.py
all_test_router_dhcphosts	`Skipped`	---	test_router_dhcphosts.py
all_test_iso	`Skipped`	---	test_iso.py
all_test_staticroles	`Skipped`	---	test_staticroles.py
all_test_attach_multiple_volumes	`Skipped`	---	test_attach_multiple_volumes.py
all_test_kubernetes_clusters	`Skipped`	---	test_kubernetes_clusters.py
all_test_backup_recovery_dummy	`Skipped`	---	test_backup_recovery_dummy.py
all_test_password_server	`Skipped`	---	test_password_server.py
all_test_certauthority_root	`Skipped`	---	test_certauthority_root.py

borisstoyanov

LGTM,

Managed to manually regression test it with a simple saml server, didn't had any issues.

yadvr · 2023-04-03T09:45:44Z

Considering smoketests from #7268 (comment) and manual QA, LGTM, merging.

There are multiple ways in which a SAML response can be formatted, especially when encryption is enabled. This PR removes the hardcoding of EncryptedKeyResolver= InlineEncryptedKeyResolver in favor of using a ChainingEncryptedKeyResolver which will try multiple resolvers. It preserves the InlineEncryptedKeyResolver as the first option but adds EncryptedElementTypeEncryptedKeyResolver to the chain of resolvers to try. ChainingEncryptedKeyResolver is a bit finicky in that you can't provide it a list of resolvers, you can only fetch its internal list and add to it. Theoretically we could add all of the resolver types to the chain, but for now just preserving the ones known to be in use. Co-authored-by: Marcus Sorensen <mls@apple.com>

Add EncryptedElementType key resolver to SAML plugin

bf7b205

boring-cyborg bot added the component:saml label Feb 21, 2023

DaanHoogland closed this Feb 21, 2023

DaanHoogland reopened this Feb 21, 2023

yadvr approved these changes Feb 24, 2023

View reviewed changes

yadvr added this to the 4.18.1.0 milestone Feb 24, 2023

borisstoyanov added the status:needs-testing label Mar 1, 2023

yadvr requested review from DaanHoogland and borisstoyanov March 2, 2023 06:50

DaanHoogland approved these changes Mar 2, 2023

View reviewed changes

DaanHoogland modified the milestones: 4.18.1.0, 4.18.0.0 Mar 2, 2023

borisstoyanov modified the milestones: 4.18.0.0, 4.18.1.0 Mar 2, 2023

yadvr changed the base branch from main to 4.18 March 17, 2023 05:28

borisstoyanov approved these changes Apr 3, 2023

View reviewed changes

yadvr merged commit 5d5fa04 into apache:4.18 Apr 3, 2023

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

mlsorensen commented Feb 21, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

codecov bot commented Feb 21, 2023

Codecov Report

Uh oh!

sonarqubecloud bot commented Feb 21, 2023

Uh oh!

mlsorensen commented Feb 21, 2023

Uh oh!

mlsorensen commented Feb 21, 2023

Uh oh!

yadvr commented Feb 24, 2023

Uh oh!

blueorangutan commented Feb 24, 2023

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Feb 24, 2023

Uh oh!

DaanHoogland commented Feb 24, 2023

Uh oh!

blueorangutan commented Feb 24, 2023

Uh oh!

blueorangutan commented Feb 25, 2023

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Mar 17, 2023

Uh oh!

blueorangutan commented Mar 17, 2023

Uh oh!

blueorangutan commented Mar 17, 2023

Uh oh!

yadvr commented Mar 31, 2023

Uh oh!

blueorangutan commented Mar 31, 2023

Uh oh!

borisstoyanov commented Mar 31, 2023

Uh oh!

blueorangutan commented Mar 31, 2023

Uh oh!

blueorangutan commented Mar 31, 2023

Uh oh!

borisstoyanov commented Mar 31, 2023

Uh oh!

blueorangutan commented Mar 31, 2023

Uh oh!

blueorangutan commented Apr 1, 2023

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Apr 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants