[Backport 2.x] Backport OpenSAML 4.3.0 to 2.x by willyborankin · Pull Request #3671 · opensearch-project/security

willyborankin · 2023-11-08T21:19:39Z

Backported PRs:

codecov · 2023-11-08T21:23:03Z

Codecov Report

Merging #3671 (773dcef) into 2.x (9110114) will decrease coverage by 0.04%.
The diff coverage is 63.88%.

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #3671      +/-   ##
============================================
- Coverage     64.90%   64.87%   -0.04%     
- Complexity     3638     3656      +18     
============================================
  Files           285      291       +6     
  Lines         20535    20622      +87     
  Branches       3386     3399      +13     
============================================
+ Hits          13329    13378      +49     
- Misses         5528     5562      +34     
- Partials       1678     1682       +4

Files	Coverage Δ
...zon/dlic/auth/http/saml/Saml2SettingsProvider.java	`60.18% <100.00%> (ø)`
.../dlic/auth/http/saml/SamlHTTPMetadataResolver.java	`61.53% <100.00%> (-1.43%)`	⬇️
.../security/opensaml/integration/CleanerFactory.java	`100.00% <100.00%> (ø)`
...ml/integration/SecurityX509CertificateBuilder.java	`100.00% <100.00%> (ø)`
...y/opensaml/integration/SecurityX509CRLBuilder.java	`50.00% <50.00%> (ø)`
...zon/dlic/auth/http/saml/HTTPSamlAuthenticator.java	`67.51% <82.60%> (+0.14%)`	⬆️
...nsaml/integration/SecurityX509CertificateImpl.java	`76.19% <76.19%> (ø)`
...egration/SecurityXMLObjectProviderInitializer.java	`74.19% <74.19%> (ø)`
...rity/opensaml/integration/SecurityX509CRLImpl.java	`0.00% <0.00%> (ø)`

... and 5 files with indirect coverage changes

plugin-security.policy

cwperks · 2023-11-13T15:13:51Z

Great work @willyborankin ! I was out of the office last week and didn't get a chance to properly review, but I'm back this week.

FYI the cypress tests in the security-dashboards-plugin are currently disabled and that repo has helped us identify issues around upgrades of OpenSAML in the past. I would like to merge this PR and also ensure that the dashboards-plugin CI passes with this change included.

willyborankin · 2023-11-13T18:42:23Z

Great work @willyborankin ! I was out of the office last week and didn't get a chance to properly review, but I'm back this week.

FYI the cypress tests in the security-dashboards-plugin are currently disabled and that repo has helped us identify issues around upgrades of OpenSAML in the past. I would like to merge this PR and also ensure that the dashboards-plugin CI passes with this change included.

The code is in main so we can test. I tried to config on my local machine OSD sec plugin test but failed. Im terrible bad in JS infrastructure :-(

### Description Permission: `permission java.util.PropertyPermission "*", "read,write";` was declared twice. Observed here: #3671 I will backport it in my PR. ### Testing [Please provide details of testing done: unit testing, integration testing and manual testing] ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Andrey Pleskach <ples@aiven.io>

Manually backported PRs: - opensearch-project#2987 - opensearch-project#2927 Signed-off-by: Andrey Pleskach <ples@aiven.io>

) In order to exclude `permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread";` we must use the dedicated `cleaners` thread factory in `OpenSearch`. Since `OpenSAML` packages are sealed it it impossible to replace `CleanerSupport` class with our own solution. Instead we have to change configuration of how such objects should be parsed. There are only 2 classes in the library which use `CleanerSupport`: - `X509CertificateImpl` - `X509CRLImpl` This fix uses the same solution as in `OpenSAML` and replaces `CleanerSupport` with our custom implementation `CleanerFactory`. Signed-off-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <ples@aiven.io>

Signed-off-by: Andrey Pleskach <ples@aiven.io>

DarshitChanpura · 2023-12-05T17:20:54Z

@willyborankin Is this ready to be re-reviewed ?

willyborankin · 2023-12-05T17:30:46Z

@willyborankin Is this ready to be re-reviewed ?

Sure 😁

### Description Resolves an issue with permissions in the SAML auth flow after merge of #3671 This backports code from #2987 that wraps the call to `instantiateAAA` in `AccessController.doPrivileged`. Expand the section below to see the stack trace with permissions error. <details> <summary>Permission issue stack trace</summary> [2023-12-15T23:45:14,828][WARN ][o.o.s.s.ReflectionHelper ] [6c7e67b914eb] Unable to enable 'com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator' due to java.lang.reflect.InvocationTargetException [2023-12-15T23:45:14,832][ERROR][o.o.s.s.DynamicConfigModelV7] [6c7e67b914eb] Unable to initialize auth domain saml_auth_domain=AuthcDomain [http_enabled=true, transport_enabled=false, order=5, http_authenticator=HttpAuthenticator [challenge=true, type=saml, config={idp={metadata_url=http://localhost:7000/metadata, entity_id=urn:example:idp}, sp={entity_id=https://localhost:9200/}, kibana_url=http://localhost:5601/, exchange_key=6aff3042-1327-4f3d-82f0-40a157ac4464}], authentication_backend=AuthcBackend [type=noop, config={}], description=null] due to OpenSearchException[java.lang.reflect.InvocationTargetException]; nested: InvocationTargetException; nested: RuntimeException[java.security.AccessControlException: access denied ("java.util.PropertyPermission" "*" "read,write")]; nested: AccessControlException[access denied ("java.util.PropertyPermission" "*" "read,write")]; org.opensearch.OpenSearchException: java.lang.reflect.InvocationTargetException at org.opensearch.security.support.ReflectionHelper.instantiateAAA(ReflectionHelper.java:73) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.securityconf.DynamicConfigModelV7.newInstance(DynamicConfigModelV7.java:443) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.securityconf.DynamicConfigModelV7.buildAAA(DynamicConfigModelV7.java:330) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.securityconf.DynamicConfigModelV7.<init>(DynamicConfigModelV7.java:100) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.securityconf.DynamicConfigFactory.onChange(DynamicConfigFactory.java:285) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.configuration.ConfigurationRepository.notifyAboutChanges(ConfigurationRepository.java:406) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.configuration.ConfigurationRepository.reloadConfiguration0(ConfigurationRepository.java:395) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.configuration.ConfigurationRepository.reloadConfiguration(ConfigurationRepository.java:379) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.action.configupdate.TransportConfigUpdateAction.nodeOperation(TransportConfigUpdateAction.java:128) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.action.configupdate.TransportConfigUpdateAction.nodeOperation(TransportConfigUpdateAction.java:52) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.action.support.nodes.TransportNodesAction.nodeOperation(TransportNodesAction.java:200) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.action.support.nodes.TransportNodesAction$NodeTransportHandler.messageReceived(TransportNodesAction.java:328) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.action.support.nodes.TransportNodesAction$NodeTransportHandler.messageReceived(TransportNodesAction.java:324) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.security.ssl.transport.SecuritySSLRequestHandler.messageReceivedDecorate(SecuritySSLRequestHandler.java:224) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.transport.SecurityRequestHandler.messageReceivedDecorate(SecurityRequestHandler.java:211) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.ssl.transport.SecuritySSLRequestHandler.messageReceived(SecuritySSLRequestHandler.java:109) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.security.OpenSearchSecurityPlugin$6$1.messageReceived(OpenSearchSecurityPlugin.java:794) [opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at org.opensearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:106) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.transport.TransportService$7.doRun(TransportService.java:1067) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:911) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52) [opensearch-2.12.0-SNAPSHOT.jar:2.12.0-SNAPSHOT] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?] at java.base/java.lang.Thread.run(Thread.java:829) [?:?] Caused by: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?] at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?] at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?] at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?] at org.opensearch.security.support.ReflectionHelper.instantiateAAA(ReflectionHelper.java:62) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] ... 23 more Caused by: java.lang.RuntimeException: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "*" "read,write") at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.<init>(HTTPSamlAuthenticator.java:154) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?] at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?] at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?] at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?] at org.opensearch.security.support.ReflectionHelper.instantiateAAA(ReflectionHelper.java:62) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] ... 23 more Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "*" "read,write") at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472) ~[?:?] at java.base/java.security.AccessController.checkPermission(AccessController.java:897) ~[?:?] at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322) ~[?:?] at java.base/java.lang.SecurityManager.checkPropertiesAccess(SecurityManager.java:1034) ~[?:?] at java.base/java.lang.System.getProperties(System.java:731) ~[?:?] at org.opensaml.core.config.provider.SystemPropertyConfigurationPropertiesSource.getProperties(SystemPropertyConfigurationPropertiesSource.java:31) ~[opensaml-core-4.3.0.jar:?] at org.opensaml.core.config.ConfigurationService.getConfigurationProperties(ConfigurationService.java:148) ~[opensaml-core-4.3.0.jar:?] at org.opensaml.core.config.ConfigurationService.getPartitionName(ConfigurationService.java:192) ~[opensaml-core-4.3.0.jar:?] at org.opensaml.core.config.ConfigurationService.get(ConfigurationService.java:90) ~[opensaml-core-4.3.0.jar:?] at org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport.getUnmarshallerFactory(XMLObjectProviderRegistrySupport.java:126) ~[opensaml-core-4.3.0.jar:?] at org.opensaml.saml.metadata.resolver.impl.AbstractMetadataResolver.<init>(AbstractMetadataResolver.java:118) ~[opensaml-saml-impl-4.3.0.jar:?] at org.opensaml.saml.metadata.resolver.impl.AbstractBatchMetadataResolver.<init>(AbstractBatchMetadataResolver.java:75) ~[opensaml-saml-impl-4.3.0.jar:?] at org.opensaml.saml.metadata.resolver.impl.AbstractReloadingMetadataResolver.<init>(AbstractReloadingMetadataResolver.java:128) ~[opensaml-saml-impl-4.3.0.jar:?] at org.opensaml.saml.metadata.resolver.impl.HTTPMetadataResolver.<init>(HTTPMetadataResolver.java:103) ~[opensaml-saml-impl-4.3.0.jar:?] at org.opensaml.saml.metadata.resolver.impl.HTTPMetadataResolver.<init>(HTTPMetadataResolver.java:89) ~[opensaml-saml-impl-4.3.0.jar:?] at com.amazon.dlic.auth.http.saml.SamlHTTPMetadataResolver.<init>(SamlHTTPMetadataResolver.java:34) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.createMetadataResolver(HTTPSamlAuthenticator.java:336) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.<init>(HTTPSamlAuthenticator.java:133) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?] at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?] at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?] at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?] at org.opensearch.security.support.ReflectionHelper.instantiateAAA(ReflectionHelper.java:62) ~[opensearch-security-2.12.0.0-SNAPSHOT.jar:2.12.0.0-SNAPSHOT] ... 23 more </details> * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Bug fix ### Issues Resolved Fixes CI issues seen in: - opensearch-project/security-dashboards-plugin#1691 - opensearch-project/security-dashboards-plugin#1686 ### Testing Tested by creating a local distribution and running the security-dashboards-plugin integration tests before and after this change to show that it resolves the issue. ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [ ] Commits are signed per the DCO using --signoff By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Craig Perkins <cwperx@amazon.com>

willyborankin marked this pull request as ready for review November 9, 2023 21:09

willyborankin requested review from DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied, reta and stephen-crawford as code owners November 9, 2023 21:09

reta previously approved these changes Nov 9, 2023

View reviewed changes

cwperks reviewed Nov 13, 2023

View reviewed changes

plugin-security.policy Outdated Show resolved Hide resolved

willyborankin mentioned this pull request Nov 13, 2023

Remove duplicate permissions #3690

Merged

3 tasks

willyborankin dismissed reta’s stale review via 0f6e54f November 13, 2023 20:44

willyborankin force-pushed the backport-3651-to-2.x branch from 305c731 to 0f6e54f Compare November 13, 2023 20:44

DarshitChanpura approved these changes Nov 14, 2023

View reviewed changes

willyborankin force-pushed the backport-3651-to-2.x branch from 0f6e54f to 2f141a3 Compare November 20, 2023 12:53

willyborankin added 3 commits November 30, 2023 21:45

Bump OpenSAML to 4.3.0

61ca2ac

Manually backported PRs: - opensearch-project#2987 - opensearch-project#2927 Signed-off-by: Andrey Pleskach <ples@aiven.io>

Remove duplicate permissions

773dcef

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the backport-3651-to-2.x branch from 2f141a3 to 773dcef Compare November 30, 2023 20:45

RyanL1997 approved these changes Dec 6, 2023

View reviewed changes

cwperks merged commit d7b10bb into opensearch-project:2.x Dec 6, 2023

This was referenced Dec 16, 2023

[Backport 2.x] Cypress13 testing frame work for OIDC and SAML opensearch-project/security-dashboards-plugin#1691

Merged

[2.x] Backport SAML permissions changes in DynamicConfigModelV7 #3853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backport 2.x] Backport OpenSAML 4.3.0 to 2.x#3671

[Backport 2.x] Backport OpenSAML 4.3.0 to 2.x#3671
cwperks merged 3 commits intoopensearch-project:2.xfrom
willyborankin:backport-3651-to-2.x

willyborankin commented Nov 8, 2023 •

edited

Loading

Uh oh!

codecov bot commented Nov 8, 2023 •

edited

Loading

Uh oh!

Uh oh!

cwperks commented Nov 13, 2023

Uh oh!

willyborankin commented Nov 13, 2023

Uh oh!

DarshitChanpura commented Dec 5, 2023

Uh oh!

willyborankin commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

willyborankin commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

cwperks commented Nov 13, 2023

Uh oh!

willyborankin commented Nov 13, 2023

Uh oh!

DarshitChanpura commented Dec 5, 2023

Uh oh!

willyborankin commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

willyborankin commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading