This is an automated email from the ASF dual-hosted git repository. robertlazarski pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/axis-axis2-java-rampart.git
commit 6d19d990b76b19076d1e31ed680e280b4d367aeb Author: Robert Lazarski <[email protected]> AuthorDate: Thu Oct 8 17:38:31 2026 -1000 RAMPART-459: initialise OpenSAML once instead of on every message InitializationService.initialize() has no idempotency guard and holds a class monitor, so calling it per message serialised all inbound traffic. The new OpenSAMLInitializer runs initSamlEngine() then initialize() exactly once; that order matters because initSamlEngine() replaces the global Configuration. Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../org/apache/rampart/RampartMessageData.java | 44 +++-------- .../rahas/impl/util/OpenSAMLInitializer.java | 73 +++++++++++++++++ .../org/apache/rahas/impl/util/SAML2Utils.java | 4 +- .../rahas/impl/util/OpenSAMLInitializerTest.java | 91 ++++++++++++++++++++++ 4 files changed, 174 insertions(+), 38 deletions(-) diff --git a/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java b/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java index 66ea13ac..97e6af78 100644 --- a/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java +++ b/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java @@ -35,6 +35,7 @@ import org.apache.neethi.PolicyEngine; import org.apache.rahas.RahasConstants; import org.apache.rahas.SimpleTokenStore; import org.apache.rahas.TokenStorage; +import org.apache.rahas.impl.util.OpenSAMLInitializer; import org.apache.rampart.handler.RampartUsernameTokenValidator; import org.apache.rampart.handler.WSSHandlerConstants; import org.apache.rampart.policy.RampartPolicyBuilder; @@ -236,44 +237,17 @@ public class RampartMessageData { try { - // PERFORMANCE vs CORRECTNESS: - // The WSS4J / Santuario / OpenSAML providers below are one-time, process-wide - // initialisers. Invoking them in this per-message constructor is intentional but - // is a correctness-over-performance trade-off: - // * Correctness: it guarantees the SAML stack is initialised before any - // assertion is processed. Without it OpenSAMLUtil.unmarshallerFactory could be - // null (an initialisation-ordering problem surfaced during the OpenSAML 5 / - // Jakarta migration), causing SAML processing to fail intermittently. - // * Performance: all of these calls are idempotent guards, so the steady-state - // cost is only the guard checks rather than real re-initialisation - but they - // still run on every message, which is wasteful under high throughput. - // The proper fix is to run this once per application lifecycle (e.g. in the - // Rampart/Rahas module init), which is tracked separately; do not move it without - // re-verifying the unmarshallerFactory ordering issue does not return. - if (log.isDebugEnabled()) { - log.debug("WSS4J initialization starting"); - } + // The WSS4J / Santuario / OpenSAML initialisers below are process-wide. They + // stay on the message path so the SAML stack is ready before any assertion is + // processed (without it OpenSAMLUtil.unmarshallerFactory could be null), but + // every call here must be cheap once initialised: WSSConfig.init() and + // Init.init() guard themselves, and OpenSAML goes through OpenSAMLInitializer + // because InitializationService.initialize() does not (RAMPART-459). Never + // call InitializationService.initialize() directly from this constructor. WSSConfig.init(); org.apache.xml.security.Init.init(); - if (log.isDebugEnabled()) { - log.debug("Basic WSS4J initialization complete"); - } - - // Initialize WSS4J's OpenSAML integration specifically try { - if (log.isDebugEnabled()) { - log.debug("Starting OpenSAML initialization"); - } - org.opensaml.core.config.InitializationService.initialize(); - - // Call WSS4J's OpenSAMLUtil initialization method - Class<?> openSAMLUtilClass = Class.forName("org.apache.wss4j.common.saml.OpenSAMLUtil"); - java.lang.reflect.Method initMethod = openSAMLUtilClass.getDeclaredMethod("initSamlEngine"); - initMethod.setAccessible(true); - initMethod.invoke(null); - if (log.isDebugEnabled()) { - log.debug("OpenSAMLUtil.initSamlEngine() called successfully"); - } + OpenSAMLInitializer.ensureInitialized(); } catch (Exception e) { log.warn("WSS4J OpenSAML initialization failed: " + e.getMessage(), e); } diff --git a/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java b/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java new file mode 100644 index 00000000..2b30ed79 --- /dev/null +++ b/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.rahas.impl.util; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.wss4j.common.saml.OpenSAMLUtil; +import org.opensaml.core.config.InitializationException; +import org.opensaml.core.config.InitializationService; + +/** + * One-time, process-wide initialisation of the OpenSAML stack used by WSS4J and Rahas. + * + * <p>RAMPART-459: OpenSAML's {@link InitializationService#initialize()} is + * {@code static synchronized} and has no idempotency guard - every call reloads every + * registered module initialiser. Calling it per message serialised all inbound + * traffic on that class monitor, SAML policy or not. Callers on the message path must + * use {@link #ensureInitialized()} instead of calling it directly.</p> + * + * <p>Ordering matters and must not be swapped: {@link OpenSAMLUtil#initSamlEngine()} + * replaces OpenSAML's global {@code Configuration} with a fresh one holding only + * WSS4J's provider registry, so anything initialised before it is discarded. + * {@code InitializationService.initialize()} therefore runs second, populating that + * configuration with the remaining OpenSAML modules (global security configuration, + * xmlsec, parser pool). This is the state the per-message calls used to converge on + * from the second message onwards; it is now reached on the first and left alone.</p> + */ +public final class OpenSAMLInitializer { + + private static final Log log = LogFactory.getLog(OpenSAMLInitializer.class); + + private static volatile boolean initialized; + + private OpenSAMLInitializer() { + } + + /** + * Initialises OpenSAML once. After the first successful call this is a single + * volatile read. A failed attempt is not recorded, so the next call retries. + */ + public static void ensureInitialized() throws InitializationException { + if (initialized) { + return; + } + synchronized (OpenSAMLInitializer.class) { + if (initialized) { + return; + } + OpenSAMLUtil.initSamlEngine(); + InitializationService.initialize(); + initialized = true; + if (log.isDebugEnabled()) { + log.debug("OpenSAML initialization complete"); + } + } + } +} diff --git a/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/SAML2Utils.java b/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/SAML2Utils.java index 9015ae59..aad9736b 100644 --- a/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/SAML2Utils.java +++ b/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/SAML2Utils.java @@ -51,7 +51,6 @@ import org.opensaml.saml.saml2.core.Subject; import org.opensaml.saml.saml2.core.SubjectConfirmation; import org.opensaml.saml.saml2.core.SubjectConfirmationData; import org.opensaml.saml.common.SAMLVersion; -import org.opensaml.core.config.InitializationService; import org.opensaml.core.config.InitializationException; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -109,7 +108,7 @@ public class SAML2Utils { //build the assertion by unmarhalling the DOM element. try { - InitializationService.initialize(); + OpenSAMLInitializer.ensureInitialized(); String keyInfoElementString = elem.toString(); DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance(); @@ -124,7 +123,6 @@ public class SAML2Utils { .unmarshall(element); } catch (InitializationException e) { -//[ERROR] /home/rlapache/axis-axis2-java-rampart/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/SAML2Utils.java:[123,60] incompatible types: java.lang.String cannot be converted to java.lang.Exception throw new WSSecurityException( WSSecurityException.ErrorCode.FAILURE, e, "Failure in bootstrapping"); } catch (UnmarshallingException e) { diff --git a/modules/rampart-trust/src/test/java/org/apache/rahas/impl/util/OpenSAMLInitializerTest.java b/modules/rampart-trust/src/test/java/org/apache/rahas/impl/util/OpenSAMLInitializerTest.java new file mode 100644 index 00000000..7ae2e12c --- /dev/null +++ b/modules/rampart-trust/src/test/java/org/apache/rahas/impl/util/OpenSAMLInitializerTest.java @@ -0,0 +1,91 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.rahas.impl.util; + +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; + +import junit.framework.TestCase; + +import org.opensaml.core.xml.config.XMLObjectProviderRegistrySupport; +import org.opensaml.xmlsec.SecurityConfigurationSupport; +import org.opensaml.xmlsec.SignatureSigningConfiguration; + +/** + * RAMPART-459: OpenSAML must be initialised once, not on every message, and must end + * up in the same state the per-message calls used to converge on. + */ +public class OpenSAMLInitializerTest extends TestCase { + + public void testInitialisesWss4jAndOpenSAMLModules() throws Exception { + OpenSAMLInitializer.ensureInitialized(); + + assertNotNull("unmarshaller factory must be available", + XMLObjectProviderRegistrySupport.getUnmarshallerFactory()); + // Registered by an OpenSAML module initialiser, not by WSS4J; present only if + // InitializationService ran after initSamlEngine replaced the configuration. + assertNotNull("OpenSAML global security configuration must be registered", + SecurityConfigurationSupport.getGlobalSignatureSigningConfiguration()); + } + + /** + * Each InitializationService run registers fresh global configuration objects, so + * an unchanged instance across calls shows the module initialisers did not re-run. + */ + public void testRepeatedCallsDoNotReinitialise() throws Exception { + OpenSAMLInitializer.ensureInitialized(); + SignatureSigningConfiguration before = + SecurityConfigurationSupport.getGlobalSignatureSigningConfiguration(); + + OpenSAMLInitializer.ensureInitialized(); + OpenSAMLInitializer.ensureInitialized(); + + assertSame("OpenSAML must not be re-initialised on later calls", before, + SecurityConfigurationSupport.getGlobalSignatureSigningConfiguration()); + } + + public void testConcurrentCallsSucceed() throws Exception { + final int threadCount = 16; + final CountDownLatch startGate = new CountDownLatch(1); + final CountDownLatch doneGate = new CountDownLatch(threadCount); + final List<Throwable> failures = new CopyOnWriteArrayList<Throwable>(); + + for (int i = 0; i < threadCount; i++) { + new Thread(new Runnable() { + public void run() { + try { + startGate.await(); + OpenSAMLInitializer.ensureInitialized(); + } catch (Throwable t) { + failures.add(t); + } finally { + doneGate.countDown(); + } + } + }).start(); + } + + startGate.countDown(); + doneGate.await(); + + assertTrue("concurrent initialisation must not fail: " + failures, failures.isEmpty()); + assertNotNull(XMLObjectProviderRegistrySupport.getUnmarshallerFactory()); + } +}
