From 44a03c5a5929357a2e936b45dd754085fd282f0f Mon Sep 17 00:00:00 2001 From: Andrey Pleskach Date: Tue, 7 Nov 2023 16:17:03 +0100 Subject: [PATCH] Integrates OpenSAML 4.3 with SAML authenticator (#3651) 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 --- plugin-security.policy | 1 - .../auth/http/saml/HTTPSamlAuthenticator.java | 68 ++++++++------- .../http/saml/SamlHTTPMetadataResolver.java | 7 +- .../opensaml/integration/CleanerFactory.java | 42 +++++++++ .../integration/SecurityX509CRLBuilder.java | 23 +++++ .../integration/SecurityX509CRLImpl.java | 86 ++++++++++++++++++ .../SecurityX509CertificateBuilder.java | 24 +++++ .../SecurityX509CertificateImpl.java | 86 ++++++++++++++++++ .../SecurityXMLObjectProviderInitializer.java | 87 +++++++++++++++++++ 9 files changed, 386 insertions(+), 38 deletions(-) create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/CleanerFactory.java create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLBuilder.java create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateBuilder.java create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateImpl.java create mode 100644 src/main/java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java diff --git a/plugin-security.policy b/plugin-security.policy index a4e4a66c73..65b6b22fee 100644 --- a/plugin-security.policy +++ b/plugin-security.policy @@ -76,7 +76,6 @@ grant { //SAML policy permission java.util.PropertyPermission "*", "read,write"; - permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread"; }; grant codeBase "${codebase.netty-common}" { diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java b/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java index 918e3be5ab..fa8db83814 100644 --- a/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java +++ b/src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java @@ -20,6 +20,7 @@ import java.security.PrivilegedExceptionAction; import java.util.Map; import java.util.Optional; +import java.util.ServiceLoader; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -42,9 +43,11 @@ import org.apache.logging.log4j.Logger; import org.opensaml.core.config.InitializationException; import org.opensaml.core.config.InitializationService; +import org.opensaml.core.config.Initializer; import org.opensaml.saml.metadata.resolver.MetadataResolver; import org.opensaml.saml.metadata.resolver.impl.AbstractMetadataResolver; import org.opensaml.saml.metadata.resolver.impl.DOMMetadataResolver; +import org.opensaml.xmlsec.config.impl.XMLObjectProviderInitializer; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.xml.sax.SAXException; @@ -61,10 +64,11 @@ import org.opensearch.rest.RestRequest; import org.opensearch.security.auth.Destroyable; import org.opensearch.security.auth.HTTPAuthenticator; +import org.opensearch.security.filter.OpenSearchRequest; import org.opensearch.security.filter.SecurityRequest; import org.opensearch.security.filter.SecurityRequestChannelUnsupported; import org.opensearch.security.filter.SecurityResponse; -import org.opensearch.security.filter.OpenSearchRequest; +import org.opensearch.security.opensaml.integration.SecurityXMLObjectProviderInitializer; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.support.PemKeyReader; import org.opensearch.security.user.AuthCredentials; @@ -112,12 +116,12 @@ public HTTPSamlAuthenticator(final Settings settings, final Path configPath) { spSignaturePrivateKey = getSpSignaturePrivateKey(settings, configPath); useForceAuthn = settings.getAsBoolean("sp.forceAuthn", null); - if (rolesKey == null || rolesKey.length() == 0) { + if (rolesKey == null || rolesKey.isEmpty()) { log.warn("roles_key is not configured, will only extract subject from SAML"); rolesKey = null; } - if (subjectKey == null || subjectKey.length() == 0) { + if (subjectKey == null || subjectKey.isEmpty()) { // If subjectKey == null, get subject from the NameID element. // Thus, this is a valid configuration. subjectKey = null; @@ -288,35 +292,40 @@ static void ensureOpenSamlInitialization() { } try { - AccessController.doPrivileged(new PrivilegedExceptionAction() { - @Override - public Void run() throws InitializationException { - - Thread thread = Thread.currentThread(); - ClassLoader originalClassLoader = thread.getContextClassLoader(); - - try { - - thread.setContextClassLoader(InitializationService.class.getClassLoader()); - - InitializationService.initialize(); - - new org.opensaml.saml.config.impl.XMLObjectProviderInitializer().init(); - new org.opensaml.saml.config.impl.SAMLConfigurationInitializer().init(); - new org.opensaml.xmlsec.config.impl.XMLObjectProviderInitializer().init(); - } finally { - thread.setContextClassLoader(originalClassLoader); - } - - openSamlInitialized = true; - return null; + AccessController.doPrivileged((PrivilegedExceptionAction) () -> { + Thread thread = Thread.currentThread(); + ClassLoader originalClassLoader = thread.getContextClassLoader(); + try { + thread.setContextClassLoader(InitializationService.class.getClassLoader()); + initializeOpenSAMLConfiguration(); + } catch (InitializationException e) { + throw new RuntimeException(e.getCause()); + } finally { + thread.setContextClassLoader(originalClassLoader); } + + openSamlInitialized = true; + return null; }); } catch (PrivilegedActionException e) { throw new RuntimeException(e.getCause()); } } + private static void initializeOpenSAMLConfiguration() throws InitializationException { + log.info("Initializing OpenSAML using the Java Services API"); + + final ServiceLoader serviceLoader = ServiceLoader.load(Initializer.class); + for (Initializer initializer : serviceLoader) { + if (initializer instanceof XMLObjectProviderInitializer) { + // replace initialization of X509 builders which support Cleaner with our own solution + new SecurityXMLObjectProviderInitializer().init(); + } else { + initializer.init(); + } + } + } + @SuppressWarnings("removal") private MetadataResolver createMetadataResolver(final Settings settings, final Path configPath) throws Exception { final AbstractMetadataResolver metadataResolver; @@ -350,12 +359,9 @@ private MetadataResolver createMetadataResolver(final Settings settings, final P } try { - AccessController.doPrivileged(new PrivilegedExceptionAction() { - @Override - public Void run() throws ComponentInitializationException { - metadataResolver.initialize(); - return null; - } + AccessController.doPrivileged((PrivilegedExceptionAction) () -> { + metadataResolver.initialize(); + return null; }); } catch (PrivilegedActionException e) { if (e.getCause() instanceof ComponentInitializationException) { diff --git a/src/main/java/com/amazon/dlic/auth/http/saml/SamlHTTPMetadataResolver.java b/src/main/java/com/amazon/dlic/auth/http/saml/SamlHTTPMetadataResolver.java index 2a380539e6..d68905fe51 100644 --- a/src/main/java/com/amazon/dlic/auth/http/saml/SamlHTTPMetadataResolver.java +++ b/src/main/java/com/amazon/dlic/auth/http/saml/SamlHTTPMetadataResolver.java @@ -41,12 +41,7 @@ public class SamlHTTPMetadataResolver extends HTTPMetadataResolver { @SuppressWarnings("removal") protected byte[] fetchMetadata() throws ResolverException { try { - return AccessController.doPrivileged(new PrivilegedExceptionAction() { - @Override - public byte[] run() throws ResolverException { - return SamlHTTPMetadataResolver.super.fetchMetadata(); - } - }); + return AccessController.doPrivileged((PrivilegedExceptionAction) () -> SamlHTTPMetadataResolver.super.fetchMetadata()); } catch (PrivilegedActionException e) { if (e.getCause() instanceof ResolverException) { diff --git a/src/main/java/org/opensearch/security/opensaml/integration/CleanerFactory.java b/src/main/java/org/opensearch/security/opensaml/integration/CleanerFactory.java new file mode 100644 index 0000000000..7b0d968c57 --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/CleanerFactory.java @@ -0,0 +1,42 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import org.opensearch.common.util.concurrent.OpenSearchExecutors; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.ref.Cleaner; +import java.util.concurrent.ThreadFactory; + +/** + * The class was adapted from {@link net.shibboleth.utilities.java.support.primitive.CleanerSupport}. + * The main reason is that it is only one way to set Cleaner.create() + * together with cleaners daemon thread factory which is required for OpenSearch + */ +public class CleanerFactory { + + private static final Logger LOG = LoggerFactory.getLogger(CleanerFactory.class); + + private static final ThreadFactory cleanersThreadFactory = OpenSearchExecutors.daemonThreadFactory("cleaners"); + + /** Constructor. */ + private CleanerFactory() {} + + public static Cleaner create(final Class requester) { + // Current approach here is to create a new Cleaner on each call. A given class requester/owner + // is assumed to call only once and store in static storage. + LOG.debug("Creating new java.lang.ref.Cleaner instance requested by class: {}", requester.getName()); + return Cleaner.create(cleanersThreadFactory); + } + +} diff --git a/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLBuilder.java b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLBuilder.java new file mode 100644 index 0000000000..6df3ea8969 --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLBuilder.java @@ -0,0 +1,23 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import org.opensaml.xmlsec.signature.X509CRL; +import org.opensaml.xmlsec.signature.impl.X509CRLBuilder; + +public class SecurityX509CRLBuilder extends X509CRLBuilder { + + public X509CRL buildObject(final String namespaceURI, final String localName, final String namespacePrefix) { + return new SecurityX509CRLImpl(namespaceURI, localName, namespacePrefix); + } + +} diff --git a/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java new file mode 100644 index 0000000000..716826e742 --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java @@ -0,0 +1,86 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import net.shibboleth.utilities.java.support.collection.IndexingObjectStore; +import org.opensaml.core.xml.AbstractXMLObject; +import org.opensaml.core.xml.XMLObject; +import org.opensaml.xmlsec.signature.X509CRL; + +import javax.annotation.Nonnull; +import java.lang.ref.Cleaner; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * The class was adapted from {@link org.opensaml.xmlsec.signature.impl.X509CRLImpl}. + * The main reason is that it is only one way to set up {@link CleanerFactory} + * together with cleaners daemon thread factory which is required for OpenSearch + */ +public class SecurityX509CRLImpl extends AbstractXMLObject implements X509CRL { + + private static final IndexingObjectStore B64_CRL_STORE = new IndexingObjectStore<>(); + + private static final Cleaner CLEANER = CleanerFactory.create(SecurityX509CRLImpl.class); + + private Cleaner.Cleanable cleanable; + + private String b64CRLIndex; + + protected SecurityX509CRLImpl(final String namespaceURI, final String elementLocalName, final String namespacePrefix) { + super(namespaceURI, elementLocalName, namespacePrefix); + } + + public String getValue() { + return B64_CRL_STORE.get(b64CRLIndex); + } + + public void setValue(final String newValue) { + // Dump our cached DOM if the new value really is new + final String currentCRL = B64_CRL_STORE.get(b64CRLIndex); + final String newCRL = prepareForAssignment(currentCRL, newValue); + + // This is a new value, remove the old one, add the new one + if (!Objects.equals(currentCRL, newCRL)) { + if (cleanable != null) { + cleanable.clean(); + cleanable = null; + } + b64CRLIndex = B64_CRL_STORE.put(newCRL); + if (b64CRLIndex != null) { + cleanable = CLEANER.register(this, new SecurityX509CRLImpl.CleanerState(b64CRLIndex)); + } + } + } + + @Override + public List getOrderedChildren() { + return Collections.emptyList(); + } + + static class CleanerState implements Runnable { + + /** The index to remove from the store. */ + private String index; + + public CleanerState(@Nonnull final String idx) { + index = idx; + } + + /** {@inheritDoc} */ + public void run() { + SecurityX509CRLImpl.B64_CRL_STORE.remove(index); + } + + } +} diff --git a/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateBuilder.java b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateBuilder.java new file mode 100644 index 0000000000..f8fd664830 --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateBuilder.java @@ -0,0 +1,24 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import org.opensaml.xmlsec.signature.X509Certificate; +import org.opensaml.xmlsec.signature.impl.X509CertificateBuilder; + +public class SecurityX509CertificateBuilder extends X509CertificateBuilder { + + @Override + public X509Certificate buildObject(final String namespaceURI, final String localName, final String namespacePrefix) { + return new SecurityX509CertificateImpl(namespaceURI, localName, namespacePrefix); + } + +} diff --git a/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateImpl.java b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateImpl.java new file mode 100644 index 0000000000..32013ab727 --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CertificateImpl.java @@ -0,0 +1,86 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import net.shibboleth.utilities.java.support.collection.IndexingObjectStore; +import org.opensaml.core.xml.AbstractXMLObject; +import org.opensaml.core.xml.XMLObject; +import org.opensaml.xmlsec.signature.X509Certificate; + +import javax.annotation.Nonnull; +import java.lang.ref.Cleaner; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * The class was adapted from {@link org.opensaml.xmlsec.signature.impl.X509CertificateBuilder}. + * The main reason is that it is only one way to set up {@link CleanerFactory} + * together with cleaners daemon thread factory which is required for OpenSearch + */ +public class SecurityX509CertificateImpl extends AbstractXMLObject implements X509Certificate { + + private static final IndexingObjectStore B64_CERT_STORE = new IndexingObjectStore<>(); + + private static final Cleaner CLEANER = CleanerFactory.create(SecurityX509CertificateImpl.class); + + private Cleaner.Cleanable cleanable; + + private String b64CertIndex; + + protected SecurityX509CertificateImpl(final String namespaceURI, final String elementLocalName, final String namespacePrefix) { + super(namespaceURI, elementLocalName, namespacePrefix); + } + + @Override + public String getValue() { + return B64_CERT_STORE.get(b64CertIndex); + } + + @Override + public void setValue(final String newValue) { + // Dump our cached DOM if the new value really is new + final String currentCert = B64_CERT_STORE.get(b64CertIndex); + final String newCert = prepareForAssignment(currentCert, newValue); + + // This is a new value, remove the old one, add the new one + if (!Objects.equals(currentCert, newCert)) { + if (cleanable != null) { + cleanable.clean(); + cleanable = null; + } + b64CertIndex = B64_CERT_STORE.put(newCert); + if (b64CertIndex != null) { + cleanable = CLEANER.register(this, new SecurityX509CertificateImpl.CleanerState(b64CertIndex)); + } + } + } + + @Override + public List getOrderedChildren() { + return Collections.emptyList(); + } + + static class CleanerState implements Runnable { + + private String index; + + public CleanerState(@Nonnull final String idx) { + index = idx; + } + + public void run() { + SecurityX509CertificateImpl.B64_CERT_STORE.remove(index); + } + + } +} diff --git a/src/main/java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java b/src/main/java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java new file mode 100644 index 0000000000..bf87514bff --- /dev/null +++ b/src/main/java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java @@ -0,0 +1,87 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.opensaml.integration; + +import net.shibboleth.utilities.java.support.primitive.StringSupport; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensaml.core.config.InitializationException; +import org.opensaml.core.xml.config.XMLConfigurationException; +import org.opensaml.core.xml.config.XMLConfigurator; +import org.opensaml.xmlsec.config.impl.XMLObjectProviderInitializer; +import org.opensaml.xmlsec.signature.impl.X509CRLBuilder; +import org.opensaml.xmlsec.signature.impl.X509CertificateBuilder; +import org.w3c.dom.Element; + +import java.io.IOException; +import java.io.InputStream; + +/** + * The class extends {@link org.opensaml.xmlsec.config.impl.XMLObjectProviderInitializer} + * which is responsible to map signature configuration from SAML + * .well-known XML to the OpenSAML object model + */ +public class SecurityXMLObjectProviderInitializer extends XMLObjectProviderInitializer { + + protected final static Logger log = LogManager.getLogger(SecurityXMLObjectProviderInitializer.class); + + static final class SecurityObjectProviderXMLConfigurator extends XMLConfigurator { + + public static final String X509_CERTIFICATE_BUILDER_CLASS_NAME = X509CertificateBuilder.class.getCanonicalName(); + + public static final String X509_CRL_BUILDER_CLASS_NAME = X509CRLBuilder.class.getCanonicalName(); + + public SecurityObjectProviderXMLConfigurator() throws XMLConfigurationException { + super(); + } + + @Override + protected Object createClassInstance(Element configuration) throws XMLConfigurationException { + final String className = StringSupport.trimOrNull(configuration.getAttributeNS(null, "className")); + if (X509_CERTIFICATE_BUILDER_CLASS_NAME.equals(className)) { + log.trace("Replace instance of {} with {}", className, SecurityX509CertificateBuilder.class.getCanonicalName()); + return new SecurityX509CertificateBuilder(); + } else if (X509_CRL_BUILDER_CLASS_NAME.equals(className)) { + log.trace("Replace instance of {} with {}", className, SecurityX509CRLBuilder.class.getCanonicalName()); + return new SecurityX509CRLBuilder(); + } else { + return super.createClassInstance(configuration); + } + } + + } + + @Override + public void init() throws InitializationException { + try { + final XMLConfigurator configurator = new SecurityObjectProviderXMLConfigurator(); + for (String resource : getConfigResources()) { + if (resource.startsWith("/")) { + resource = resource.substring(1); + } + log.debug("Loading XMLObject provider configuration from resource '{}'", resource); + try (final InputStream is = Thread.currentThread().getContextClassLoader().getResourceAsStream(resource)) { + if (is != null) { + configurator.load(is); + } else { + throw new XMLConfigurationException("Resource not found: " + resource); + } + } catch (final IOException e) { + throw new XMLConfigurationException("Error loading resource: " + resource, e); + } + } + } catch (final XMLConfigurationException e) { + log.error("Problem loading configuration resource: {}", e.getMessage()); + throw new InitializationException("Problem loading configuration resource", e); + } + } +}