Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAML 4.3.0 addition persmission #2987

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

willyborankin
Copy link
Collaborator

@willyborankin willyborankin commented Jul 11, 2023

Description

Added addition permissions for new version of SAML in the policy file.

  permission java.util.PropertyPermission "*" "read,write";
  permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread";

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

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.

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #2987 (87e5e2d) into main (e5348eb) will decrease coverage by 2.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2987      +/-   ##
============================================
- Coverage     62.47%   60.45%   -2.02%     
+ Complexity     3380     3294      -86     
============================================
  Files           267      267              
  Lines         19772    19772              
  Branches       3356     3355       -1     
============================================
- Hits          12352    11954     -398     
- Misses         5780     6188     +408     
+ Partials       1640     1630      -10     
Impacted Files Coverage Δ
...ch/security/securityconf/DynamicConfigModelV7.java 65.14% <100.00%> (ø)

... and 25 files with indirect coverage changes

@@ -74,6 +73,9 @@ grant {

//Enable this permission to debug unauthorized de-serialization attempt
//permission java.io.SerializablePermission "enableSubstitution";

//SAML policy
permission java.util.PropertyPermission "*", "read,write";
Copy link
Collaborator

@reta reta Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm ... could we enumerate the properties that needs to be accessed instead? And why the write is needed?

Copy link
Collaborator Author

@willyborankin willyborankin Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmm no. It is up to the library which permissions it wants to use and how AFAIU :-( this permission is about access to OpenSAML XML mapping objects... I took a look in the code of OpenSAML so I do not think that this is critical.

Added addition permissions for new version of SAML.

Signed-off-by: Andrey Pleskach <[email protected]>
cwperks
cwperks previously approved these changes Jul 11, 2023
@cwperks
Copy link
Member

cwperks commented Jul 11, 2023

@willyborankin I approved this PR to fix the CI issues in security-dashboards-plugin, but I agree that it would be nice to have a test to prove that this change fixes the issue in this repo. Should we wait to backport the SAML bump to 2.x until automated tests to verify the upgrade are developed?

@cwperks
Copy link
Member

cwperks commented Jul 11, 2023

FYI The issue is reproducible by trying to call on the API to update the config directly and trying to add a SAML Authenticator: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/test/jest_integration/saml_auth.test.ts#L159-L175

@willyborankin
Copy link
Collaborator Author

willyborankin commented Jul 11, 2023

@willyborankin I approved this PR to fix the CI issues in security-dashboards-plugin, but I agree that it would be nice to have a test to prove that this change fixes the issue in this repo. Should we wait to backport the SAML bump to 2.x until automated tests to verify the upgrade are developed?

This is good question. I'm thinking about https://testcontainers.com/ so we can test both SAML and real OpenSearch together just to verify that simple integration works but it will take sometime. Alternative is to to enable SecurityManager for our local/integration tests but it needs investigation as well.

@willyborankin
Copy link
Collaborator Author

@cwperks nd @reta btw CodeQL complains abut log4j2 version 2.20 vs 2.17. I will fix it in this PR as well in four of OpenSearch versions.*.

@reta
Copy link
Collaborator

reta commented Jul 11, 2023

@cwperks nd @reta btw CodeQL complains abut log4j2 version 2.20 vs 2.17. I will fix it in this PR as well in four of OpenSearch versions.*.

The change change went into main


//SAML policy
permission java.util.PropertyPermission "*", "read,write";
permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    permission java.lang.RuntimePermission "modifyThread";

Should be sufficient

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this out to confirm

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created an issue for investigation #2989

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reta That permission did not work for me testing this locally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tried with

permission java.lang.RuntimePermission "modifyThread";
permission java.lang.RuntimePermission "modifyThreadGroup";

but that also doesn't work

Copy link
Collaborator

@reta reta Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that also doesn't work

Thanks @cwperks could we have a test for it that manifests the issue? Could you please share stack traces as well?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We think we identified the problem with @willyborankin , that is related to Cleaners however the worrying part is that this is not really specific to OpenSAML or security plugin and could manifest anywhere. That would mean granting that permission to possibly every module and plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full stack trace is:

ttp.saml.HTTPSamlAuthenticator_2: Error occurred while attempting to refresh metadata from 'http://localhost:7000/metadata'
java.lang.NoClassDefFoundError: Could not initialize class org.opensaml.xmlsec.signature.impl.X509CertificateImpl
	at org.opensaml.xmlsec.signature.impl.X509CertificateBuilder.buildObject(X509CertificateBuilder.java:40) ~[opensaml-xmlsec-impl-4.3.0.jar:?]
	at org.opensaml.xmlsec.signature.impl.X509CertificateBuilder.buildObject(X509CertificateBuilder.java:28) ~[opensaml-xmlsec-impl-4.3.0.jar:?]
	at org.opensaml.core.xml.AbstractXMLObjectBuilder.buildObject(AbstractXMLObjectBuilder.java:58) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.AbstractXMLObjectBuilder.buildObject(AbstractXMLObjectBuilder.java:73) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.buildXMLObject(AbstractXMLObjectUnmarshaller.java:182) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:104) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshallChildElement(AbstractXMLObjectUnmarshaller.java:337) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:128) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshallChildElement(AbstractXMLObjectUnmarshaller.java:337) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:128) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshallChildElement(AbstractXMLObjectUnmarshaller.java:337) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:128) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshallChildElement(AbstractXMLObjectUnmarshaller.java:337) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:128) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshallChildElement(AbstractXMLObjectUnmarshaller.java:337) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.core.xml.io.AbstractXMLObjectUnmarshaller.unmarshall(AbstractXMLObjectUnmarshaller.java:128) ~[opensaml-core-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractMetadataResolver.unmarshallMetadata(AbstractMetadataResolver.java:353) ~[opensaml-saml-impl-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractReloadingMetadataResolver.unmarshallMetadata(AbstractReloadingMetadataResolver.java:458) ~[opensaml-saml-impl-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractReloadingMetadataResolver.processNewMetadata(AbstractReloadingMetadataResolver.java:499) ~[opensaml-saml-impl-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractReloadingMetadataResolver.refresh(AbstractReloadingMetadataResolver.java:370) [opensaml-saml-impl-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractReloadingMetadataResolver.initMetadataResolver(AbstractReloadingMetadataResolver.java:325) [opensaml-saml-impl-4.3.0.jar:?]
	at org.opensaml.saml.metadata.resolver.impl.AbstractMetadataResolver.doInitialize(AbstractMetadataResolver.java:289) [opensaml-saml-impl-4.3.0.jar:?]
	at net.shibboleth.utilities.java.support.component.AbstractInitializableComponent.initialize(AbstractInitializableComponent.java:65) [java-support-8.4.0.jar:?]
	at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator$2.run(HTTPSamlAuthenticator.java:340) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator$2.run(HTTPSamlAuthenticator.java:337) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at java.security.AccessController.doPrivileged(AccessController.java:569) [?:?]
	at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.createMetadataResolver(HTTPSamlAuthenticator.java:337) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at com.amazon.dlic.auth.http.saml.HTTPSamlAuthenticator.<init>(HTTPSamlAuthenticator.java:126) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:67) ~[?:?]
	at java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500) ~[?:?]
	at java.lang.reflect.Constructor.newInstance(Constructor.java:484) ~[?:?]
	at org.opensearch.security.support.ReflectionHelper.instantiateAAA(ReflectionHelper.java:62) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.securityconf.DynamicConfigModelV7$1.run(DynamicConfigModelV7.java:408) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at java.security.AccessController.doPrivileged(AccessController.java:318) [?:?]
	at org.opensearch.security.securityconf.DynamicConfigModelV7.newInstance(DynamicConfigModelV7.java:405) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.securityconf.DynamicConfigModelV7.buildAAA(DynamicConfigModelV7.java:314) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.securityconf.DynamicConfigModelV7.<init>(DynamicConfigModelV7.java:91) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.securityconf.DynamicConfigFactory.onChange(DynamicConfigFactory.java:274) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.configuration.ConfigurationRepository.notifyAboutChanges(ConfigurationRepository.java:406) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.configuration.ConfigurationRepository.reloadConfiguration0(ConfigurationRepository.java:395) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.configuration.ConfigurationRepository.reloadConfiguration(ConfigurationRepository.java:379) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.action.configupdate.TransportConfigUpdateAction.nodeOperation(TransportConfigUpdateAction.java:128) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.action.configupdate.TransportConfigUpdateAction.nodeOperation(TransportConfigUpdateAction.java:52) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.action.support.nodes.TransportNodesAction.nodeOperation(TransportNodesAction.java:200) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.action.support.nodes.TransportNodesAction$NodeTransportHandler.messageReceived(TransportNodesAction.java:328) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.security.ssl.transport.SecuritySSLRequestHandler.messageReceivedDecorate(SecuritySSLRequestHandler.java:215) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.transport.SecurityRequestHandler.messageReceivedDecorate(SecurityRequestHandler.java:208) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.ssl.transport.SecuritySSLRequestHandler.messageReceived(SecuritySSLRequestHandler.java:100) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.security.OpenSearchSecurityPlugin$7$1.messageReceived(OpenSearchSecurityPlugin.java:760) [opensearch-security-3.0.0.0-SNAPSHOT.jar:3.0.0.0-SNAPSHOT]
	at org.opensearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:106) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.transport.TransportService$8.doRun(TransportService.java:1063) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:908) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52) [opensearch-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) [?:?]
	at java.lang.Thread.run(Thread.java:1589) [?:?]
Caused by: java.lang.ExceptionInInitializerError: Exception java.security.AccessControlException: access denied ("org.opensearch.secure_sm.ThreadPermission" "modifyArbitraryThread") [in thread "opensearch[smoketestnode][management][T#1]"]
	at java.security.AccessControlContext.checkPermission(AccessControlContext.java:485) ~[?:?]
	at java.security.AccessController.checkPermission(AccessController.java:1068) ~[?:?]
	at java.lang.SecurityManager.checkPermission(SecurityManager.java:411) ~[?:?]
	at org.opensearch.secure_sm.SecureSM.checkThreadAccess(SecureSM.java:228) ~[opensearch-secure-sm-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at org.opensearch.secure_sm.SecureSM.checkAccess(SecureSM.java:179) ~[opensearch-secure-sm-3.0.0-SNAPSHOT.jar:3.0.0-SNAPSHOT]
	at java.lang.Thread.checkAccess(Thread.java:2360) ~[?:?]
	at java.lang.Thread.setDaemon(Thread.java:2308) ~[?:?]
	at jdk.internal.ref.CleanerImpl.start(CleanerImpl.java:111) ~[?:?]
	at java.lang.ref.Cleaner.create(Cleaner.java:179) ~[?:?]
	at net.shibboleth.utilities.java.support.primitive.CleanerSupport.getInstance(CleanerSupport.java:49) ~[?:?]
	at org.opensaml.xmlsec.signature.impl.X509CertificateImpl.<clinit>(X509CertificateImpl.java:42) ~[?:?]
	... 56 more

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am manually running the frontend tests in saml_auth.test.ts and it fails when calling the endpoint to update the security configuration and to add a SAML Authenticator. These lines in particular: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/test/jest_integration/saml_auth.test.ts#L159-L175

Signed-off-by: Andrey Pleskach <[email protected]>
Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock ci

@stephen-crawford stephen-crawford added the v2.9.0 v2.9.0 label Jul 11, 2023
@cwperks
Copy link
Member

cwperks commented Jul 11, 2023

@scrawfor99 This will not be for 2.9.0. This unblocks the CI for main for the security-dashboards-plugin, but we should aim to get an automated test in for verifying the SAML authenticator initialization before backporting to 2.x

@willyborankin
Copy link
Collaborator Author

@scrawfor99 This will not be for 2.9.0. This unblocks the CI for main for the security-dashboards-plugin, but we should aim to get an automated test in for verifying the SAML authenticator initialization before backporting to 2.x

And @reta raised a valid question about the internal permission. I crated an issue #2989

@stephen-crawford stephen-crawford removed the v2.9.0 v2.9.0 label Jul 11, 2023
@stephen-crawford stephen-crawford merged commit df07bea into opensearch-project:main Jul 11, 2023
peternied added a commit to peternied/security that referenced this pull request Aug 3, 2023
This change combines the many updates from the following commits:
* 5f62e8a dependabot: bump commons-io:commons-io from 2.11.0 to 2.13.0 (opensearch-project#3074)
* 2f69a10 bump com.github.wnameless.json:json-base from 2.4.0 to 2.4.1 (opensearch-project#3062)
* c0e50da dependabot: bump org.cryptacular:cryptacular from 1.2.4 to 1.2.5 (opensearch-project#3071)
* d3488e8 dependabot: bump kafka_version from 3.5.0 to 3.5.1 (opensearch-project#3041)
* ab6778d Update ospackage, checker-qual, zcxvbn and error_prone_annotations, camel-xmlsecurity (opensearch-project#3023)
* 0e6608d Bump JSON libs (opensearch-project#2926)
* df07bea SAML 4.3.0 addition persmission (opensearch-project#2987)
* e5348eb Change maven repo location for compatibility check (opensearch-project#2980)
* 4a1ec53 Bump jaxb to 2.3.8 (opensearch-project#2977)
* 9599155 Bump guava to 32.1.1-jre (opensearch-project#2976)
* 06eed60 dependabot: bump org.glassfish.jaxb:jaxb-runtime from 2.3.4 to 4.0.3 (opensearch-project#2970)
* 1113244 Bump eventbus to 3.3.1 (opensearch-project#2965)
* 99ff7b3 dependabot: bump org.apache.bcel:bcel from 6.6.0 to 6.7.0 (opensearch-project#2969)
* 0794c3f dependabot: bump jakarta.xml.bind:jakarta.xml.bind-api (opensearch-project#2968)
* 9e6aab3 dependabot: bump com.google.j2objc:j2objc-annotations from 1.3 to 2.8 (opensearch-project#2963)
* 8227f64 dependabot: bump com.sun.istack:istack-commons-runtime (opensearch-project#2960)
* 8e044a6 dependabot: bump org.apiguardian:apiguardian-api from 1.0.0 to 1.1.2 (opensearch-project#2964)
* 49cbf52 Remove commons-collections 3.2.2 (opensearch-project#2924)
* 092e8f5 Bump SAML libs (opensearch-project#2927)
* 8ab7cb4 Resolve CVE-2023-2976 by forcing use of Guava 32.0.1 (opensearch-project#2937)
* 4eef662 Clean up and bump Apache libs (opensearch-project#2925)
* 9a72355 Bump BouncyCastle from jdk15on to jdk15to18 (opensearch-project#2901)
* e4f4817 [Enhancement] Parallel test jobs for CI (opensearch-project#2861)
* d871af3 Update snappy to 1.1.10.1 and guava to 32.0.1-jre (opensearch-project#2886)
* c808692 Format everything (opensearch-project#2866)

Signed-off-by: Peter Nied <[email protected]>
@reta
Copy link
Collaborator

reta commented Aug 23, 2023

To clarify the problem, it comes from the way X509CertificateImpl is initializing Cleaner instance:

public class X509CertificateImpl extends AbstractXMLObject implements X509Certificate {
    /** The {@link Cleaner} instance to use. */
    private static final Cleaner CLEANER = CleanerSupport.getInstance(X509CertificateImpl.class);

The JDK's cleaners are managed from the dedicated threads and the standard library allows to provide the thread factory to be used, in case of OpenSearch:

return Cleaner.create(OpenSearchExecutors.daemonThreadFactory("cleaners"));

But the class CleanerSupport does not use this method (it uses default Cleaner.create()) and that violates the OpenSearch SecurityManager checks.

@reta
Copy link
Collaborator

reta commented Aug 23, 2023

@peternied do you know if AWS is a member of Shibboleth Consortium (https://www.shibboleth.net/) so we could submit an issue against JavaSupport project.

@peternied
Copy link
Member

@reta No membership AFAIK, I've created a couple of outreach threads including (opensearch#dev) that link out to this issue. Maybe we will get lucky?

@peternied
Copy link
Member

Circling back, I haven't gotten any leads. If something comes my way latter, I'll update on the tracking issue [1]

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-2987-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 df07bea02f529cf498c7a360c30784a863523a8c
# Push it to GitHub
git push --set-upstream origin backport/backport-2987-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2987-to-2.x.

@DarshitChanpura
Copy link
Member

@willyborankin @reta I believe this change should be backported to 2.x? Please correct me if I'm wrong

@willyborankin
Copy link
Collaborator Author

@DarshitChanpura so far no sorry. The problem is permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread"; which we can not use. OpenSAML 5.0 was released Im preparing PR with it. Lets see additionla permissions the library need.

willyborankin added a commit to willyborankin/security that referenced this pull request Nov 8, 2023
Manually backported PRs:
 - opensearch-project#2987
 - opensearch-project#2927

Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin added a commit to willyborankin/security that referenced this pull request Nov 13, 2023
Manually backported PRs:
 - opensearch-project#2987
 - opensearch-project#2927

Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin added a commit to willyborankin/security that referenced this pull request Nov 20, 2023
Manually backported PRs:
 - opensearch-project#2987
 - opensearch-project#2927

Signed-off-by: Andrey Pleskach <[email protected]>
willyborankin added a commit to willyborankin/security that referenced this pull request Nov 30, 2023
Manually backported PRs:
 - opensearch-project#2987
 - opensearch-project#2927

Signed-off-by: Andrey Pleskach <[email protected]>
cwperks pushed a commit that referenced this pull request Dec 6, 2023
Backported PRs:
- #2987
- #2927
- #3651
- #3690 
into 2.x

---------

Signed-off-by: Andrey Pleskach <[email protected]>
cwperks added a commit that referenced this pull request Dec 18, 2023
### 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants