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.

Sign up for GitHub

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

Jump to bottom

SAML 4.3.0 addition persmission #2987

Merged
merged 2 commits into from
Jul 11, 2023
Merged

SAML 4.3.0 addition persmission #2987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7765f28
SAML 4.3.0 addition persmission
willyborankin Jul 11, 2023
87e5e2d
Fix log4j version
willyborankin Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ dependencies {
runtimeOnly 'org.lz4:lz4-java:1.8.0'
runtimeOnly 'io.dropwizard.metrics:metrics-core:3.1.2'
runtimeOnly 'org.slf4j:slf4j-api:1.7.30'
runtimeOnly 'org.apache.logging.log4j:log4j-slf4j-impl:2.17.1'
runtimeOnly "org.apache.logging.log4j:log4j-slf4j-impl:${versions.log4j}"
runtimeOnly 'org.xerial.snappy:snappy-java:1.1.10.1'
runtimeOnly 'org.codehaus.woodstox:stax2-api:4.2.1'
runtimeOnly "org.glassfish.jaxb:txw2:${jaxb_version}"
Expand All @@ -570,7 +570,7 @@ dependencies {
testImplementation "org.opensearch.plugin:lang-mustache-client:${opensearch_version}"
testImplementation "org.opensearch.plugin:parent-join-client:${opensearch_version}"
testImplementation "org.opensearch.plugin:aggs-matrix-stats-client:${opensearch_version}"
testImplementation 'org.apache.logging.log4j:log4j-core:2.17.1'
testImplementation "org.apache.logging.log4j:log4j-core:${versions.log4j}"
testImplementation 'javax.servlet:servlet-api:2.5'
testImplementation 'com.unboundid:unboundid-ldapsdk:4.0.9'
testImplementation 'com.github.stephenc.jcip:jcip-annotations:1.0-1'
Expand Down Expand Up @@ -618,8 +618,8 @@ dependencies {
integrationTestImplementation "org.opensearch.plugin:reindex-client:${opensearch_version}"
integrationTestImplementation "org.opensearch.plugin:percolator-client:${opensearch_version}"
integrationTestImplementation 'commons-io:commons-io:2.11.0'
integrationTestImplementation 'org.apache.logging.log4j:log4j-core:2.17.1'
integrationTestImplementation 'org.apache.logging.log4j:log4j-jul:2.17.1'
integrationTestImplementation "org.apache.logging.log4j:log4j-core:${versions.log4j}"
integrationTestImplementation "org.apache.logging.log4j:log4j-jul:${versions.log4j}"
integrationTestImplementation 'org.hamcrest:hamcrest:2.2'
integrationTestImplementation "org.bouncycastle:bcpkix-jdk15to18:${versions.bouncycastle}"
integrationTestImplementation "org.bouncycastle:bcutil-jdk15to18:${versions.bouncycastle}"
Expand Down
5 changes: 4 additions & 1 deletion plugin-security.policy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ grant {
permission java.security.SecurityPermission "putProviderProperty.BC";
permission java.security.SecurityPermission "insertProvider.BC";
permission java.security.SecurityPermission "removeProviderProperty.BC";
permission java.util.PropertyPermission "jdk.tls.rejectClientInitiatedRenegotiation", "write";

permission java.lang.RuntimePermission "accessUserInformation";

Expand All @@ -74,6 +73,10 @@ 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 •
edited
Loading

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 •
edited
Loading

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.

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 •
edited
Loading

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

};

grant codeBase "${codebase.netty-common}" {
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

import java.net.InetAddress;
import java.nio.file.Path;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
Expand All @@ -44,6 +46,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;

import org.opensearch.SpecialPermission;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.security.auth.AuthDomain;
Expand Down Expand Up @@ -396,14 +399,11 @@ private void destroyDestroyables(List<Destroyable> destroyableComponents) {
}

private <T> T newInstance(final String clazzOrShortcut, String type, final Settings settings, final Path configPath) {

String clazz = clazzOrShortcut;

if (authImplMap.containsKey(clazz + "_" + type)) {
clazz = authImplMap.get(clazz + "_" + type);
}

return ReflectionHelper.instantiateAAA(clazz, settings, configPath);
final String clazz = authImplMap.computeIfAbsent(clazzOrShortcut + "_" + type, k -> clazzOrShortcut);
return AccessController.doPrivileged((PrivilegedAction<T>) () -> {
SpecialPermission.check();
return ReflectionHelper.instantiateAAA(clazz, settings, configPath);
});
}

private String translateShortcutToClassName(final String clazzOrShortcut, final String type) {
Expand Down
Loading