This is an automated email from the ASF dual-hosted git repository. coheigea pushed a commit to branch 4.1.x-fixes in repository https://gitbox.apache.org/repos/asf/cxf.git
commit 59f732882585c2377fcb5d5760b447e1a8181214 Author: Naveed Khan <[email protected]> AuthorDate: Mon Oct 26 13:33:54 2026 +0530 make the SAMLSSOResponseValidator replay check atomic (#3625) * make the SAMLSSOResponseValidator replay check atomic SAMLSSOResponseValidator.validateSubjectConfirmation guarded a POST binding assertion against replay with contains() followed by putId() on the TokenReplayCache, so concurrent submissions of the same captured response all passed the lookup before any of them was cached, and were all accepted. Add TokenReplayCache.putIdIfAbsent, backed by the atomic putIfAbsent of Ehcache and JCache (with a synchronized default for custom caches), and use it in the validator. This is the JAX-RS SAML SSO counterpart of the WS-Security JCacheReplayCache change in #3622. * reject null and empty ids in the default putIdIfAbsent, add timeouts to the cache concurrency test (cherry picked from commit d27cfb07780e19478887d7ebbac4fddb9f346814) --- .../security/saml/sso/EHCacheTokenReplayCache.java | 16 +++ .../saml/sso/SAMLSSOResponseValidator.java | 6 +- .../cxf/rs/security/saml/sso/TokenReplayCache.java | 25 ++++ .../saml/sso/jcache/JCacheTokenReplayCache.java | 16 +++ .../saml/sso/SAMLSSOResponseValidatorTest.java | 144 +++++++++++++++++++++ .../rs/security/saml/sso/TokenReplayCacheTest.java | 76 +++++++++++ 6 files changed, 279 insertions(+), 4 deletions(-) diff --git a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/EHCacheTokenReplayCache.java b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/EHCacheTokenReplayCache.java index 43a7ac44b17..c7a51872f14 100644 --- a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/EHCacheTokenReplayCache.java +++ b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/EHCacheTokenReplayCache.java @@ -134,6 +134,22 @@ public class EHCacheTokenReplayCache implements TokenReplayCache<String> { cache.put(identifier, new EHCacheValue(identifier, expiry)); } + /** + * Add the given identifier to the cache if, and only if, it is not already contained in it, as a + * single atomic operation. + * @param identifier The identifier to be added + * @param expiry A custom expiry time for the identifier. Can be null in which case, the default expiry is used. + * @return true if the identifier was added, false if it was already contained in the cache or is + * null or empty + */ + @Override + public boolean putIdIfAbsent(String identifier, Instant expiry) { + if (identifier == null || "".equals(identifier)) { + return false; + } + return cache.putIfAbsent(identifier, new EHCacheValue(identifier, expiry)) == null; + } + /** * Return true if the given identifier is contained in the cache * @param identifier The identifier to check diff --git a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidator.java b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidator.java index 62da52cfe95..598f29f04ff 100644 --- a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidator.java +++ b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidator.java @@ -300,10 +300,8 @@ public class SAMLSSOResponseValidator { // Need to keep bearer assertion IDs based on NotOnOrAfter to detect replay attacks if (postBinding && replayCache != null) { - if (!replayCache.contains(id)) { - Instant expires = subjectConfData.getNotOnOrAfter(); - replayCache.putId(id, expires); - } else { + Instant expires = subjectConfData.getNotOnOrAfter(); + if (!replayCache.putIdIfAbsent(id, expires)) { LOG.warning("Replay attack with token id: " + id); throw new WSSecurityException(WSSecurityException.ErrorCode.FAILURE, "invalidSAMLsecurity"); } diff --git a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCache.java b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCache.java index 6175e6d42bf..51cb3dd0510 100644 --- a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCache.java +++ b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCache.java @@ -31,5 +31,30 @@ public interface TokenReplayCache<T> extends Closeable { void putId(T id, Instant expiry); + /** + * Add the given identifier to the cache if, and only if, it is not already contained in it, as a + * single atomic operation, so that concurrent requests carrying the same identifier cannot all + * pass a replay check before any of them has cached it. This default implementation only + * serializes calls within a single JVM; an implementation backed by a distributed cache should + * override it with the atomic put-if-absent operation of the underlying cache. + * @param id The identifier to be added + * @param expiry A custom expiry time for the identifier. Can be null in which case, the default + * expiry is used. + * @return true if the identifier was added, false if it was already contained in the cache or is + * null or empty + */ + default boolean putIdIfAbsent(T id, Instant expiry) { + if (id == null || "".equals(id)) { + return false; + } + synchronized (this) { + if (contains(id)) { + return false; + } + putId(id, expiry); + return true; + } + } + void close() throws IOException; } \ No newline at end of file diff --git a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/jcache/JCacheTokenReplayCache.java b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/jcache/JCacheTokenReplayCache.java index e944c36227c..853bb45c6ad 100644 --- a/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/jcache/JCacheTokenReplayCache.java +++ b/rt/rs/security/sso/saml/src/main/java/org/apache/cxf/rs/security/saml/sso/jcache/JCacheTokenReplayCache.java @@ -139,6 +139,22 @@ public class JCacheTokenReplayCache implements TokenReplayCache<String> { cache.put(identifier, new EHCacheValue(identifier, expiry)); } + /** + * Add the given identifier to the cache if, and only if, it is not already contained in it, as a + * single atomic operation. JCache's putIfAbsent is atomic for a distributed provider as well. + * @param identifier The identifier to be added + * @param expiry A custom expiry time for the identifier. Can be null in which case, the default expiry is used. + * @return true if the identifier was added, false if it was already contained in the cache or is + * null or empty + */ + @Override + public boolean putIdIfAbsent(String identifier, Instant expiry) { + if (identifier == null || "".equals(identifier)) { + return false; + } + return cache.putIfAbsent(identifier, new EHCacheValue(identifier, expiry)); + } + /** * Return true if the given identifier is contained in the cache * @param identifier The identifier to check diff --git a/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidatorTest.java b/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidatorTest.java index 9bba9c4e189..447c7459edc 100644 --- a/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidatorTest.java +++ b/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/SAMLSSOResponseValidatorTest.java @@ -28,6 +28,12 @@ import java.time.Instant; import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -666,6 +672,144 @@ public class SAMLSSOResponseValidatorTest { } } + @org.junit.Test + public void testReplayedResponseIsRejected() throws Exception { + SubjectConfirmationDataBean subjectConfirmationData = new SubjectConfirmationDataBean(); + subjectConfirmationData.setAddress("http://apache.org";); + subjectConfirmationData.setInResponseTo("12345"); + subjectConfirmationData.setNotAfter(Instant.now().plus(Duration.ofMinutes(5))); + subjectConfirmationData.setRecipient("http://recipient.apache.org";); + + Response response = createResponse(subjectConfirmationData); + + TokenReplayCache<String> replayCache = new EHCacheTokenReplayCache(); + try { + SAMLSSOResponseValidator validator = createReplayValidator(replayCache); + + SSOValidatorResponse validateSamlResponse = validator.validateSamlResponse(response, true); + assertEquals(response.getID(), validateSamlResponse.getResponseId()); + + // The same (POST binding) response is replayed + try { + validator.validateSamlResponse(copyResponse(response), true); + fail("Expected failure on replayed response"); + } catch (WSSecurityException ex) { + // expected + } + } finally { + replayCache.close(); + } + } + + // Two copies of the same response are validated concurrently, with the replay cache arranged so + // that both threads look the assertion id up before either of them has stored it. Only one of + // them may be accepted. + @org.junit.Test + public void testConcurrentlyReplayedResponseIsAcceptedOnce() throws Exception { + SubjectConfirmationDataBean subjectConfirmationData = new SubjectConfirmationDataBean(); + subjectConfirmationData.setAddress("http://apache.org";); + subjectConfirmationData.setInResponseTo("12345"); + subjectConfirmationData.setNotAfter(Instant.now().plus(Duration.ofMinutes(5))); + subjectConfirmationData.setRecipient("http://recipient.apache.org";); + + Response response = createResponse(subjectConfirmationData); + + final int threads = 2; + TokenReplayCache<String> replayCache = new InterleavingTokenReplayCache(threads); + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + SAMLSSOResponseValidator validator = createReplayValidator(replayCache); + + AtomicInteger accepted = new AtomicInteger(); + List<Future<?>> futures = new ArrayList<>(threads); + for (int t = 0; t < threads; t++) { + final Response replayedResponse = copyResponse(response); + futures.add(executor.submit(() -> { + try { + validator.validateSamlResponse(replayedResponse, true); + accepted.incrementAndGet(); + } catch (WSSecurityException ex) { + // expected for all but one of the submissions + } + return null; + })); + } + for (Future<?> future : futures) { + future.get(30, TimeUnit.SECONDS); + } + assertEquals("The replayed response was accepted more than once", 1, accepted.get()); + } finally { + executor.shutdownNow(); + replayCache.close(); + } + } + + private SAMLSSOResponseValidator createReplayValidator(TokenReplayCache<String> replayCache) { + SAMLSSOResponseValidator validator = new SAMLSSOResponseValidator(); + validator.setEnforceAssertionsSigned(false); + validator.setIssuerIDP("http://cxf.apache.org/issuer";); + validator.setAssertionConsumerURL("http://recipient.apache.org";); + validator.setClientAddress("http://apache.org";); + validator.setRequestId("12345"); + validator.setSpIdentifier("http://service.apache.org";); + validator.setReplayCache(replayCache); + return validator; + } + + // Creates an independent copy of the response (with the same assertion id), as received by + // another concurrent request + private Response copyResponse(Response response) throws Exception { + Document doc = DOMUtils.createDocument(); + Element responseElement = (Element)doc.importNode(response.getDOM(), true); + doc.appendChild(responseElement); + return (Response)OpenSAMLUtil.fromDom(responseElement); + } + + /** + * A replay cache whose contains() call only returns once the expected number of threads have + * all called it, so that a non-atomic contains-then-putId replay check lets every one of them + * through, whereas putIdIfAbsent accepts exactly one. + */ + private static final class InterleavingTokenReplayCache implements TokenReplayCache<String> { + private final TokenReplayCache<String> delegate; + private final CyclicBarrier barrier; + + InterleavingTokenReplayCache(int threads) throws Exception { + this.delegate = new EHCacheTokenReplayCache(); + this.barrier = new CyclicBarrier(threads); + } + + @Override + public boolean contains(String id) { + try { + barrier.await(10, TimeUnit.SECONDS); + } catch (Exception ex) { + throw new IllegalStateException(ex); + } + return delegate.contains(id); + } + + @Override + public void putId(String id) { + delegate.putId(id); + } + + @Override + public void putId(String id, Instant expiry) { + delegate.putId(id, expiry); + } + + @Override + public boolean putIdIfAbsent(String id, Instant expiry) { + return delegate.putIdIfAbsent(id, expiry); + } + + @Override + public void close() throws java.io.IOException { + delegate.close(); + } + } + private Response createResponse( SubjectConfirmationDataBean subjectConfirmationData ) throws Exception { diff --git a/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCacheTest.java b/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCacheTest.java index 0b3731366b8..8850fe5a803 100644 --- a/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCacheTest.java +++ b/rt/rs/security/sso/saml/src/test/java/org/apache/cxf/rs/security/saml/sso/TokenReplayCacheTest.java @@ -21,9 +21,17 @@ package org.apache.cxf.rs.security.saml.sso; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.List; import java.util.UUID; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicIntegerArray; import org.apache.cxf.rs.security.saml.sso.jcache.JCacheTokenReplayCache; @@ -31,6 +39,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -103,6 +112,73 @@ public class TokenReplayCacheTest { replayCache.close(); } + @Test + public void testTokenReplayCachePutIdIfAbsent() throws Exception { + TokenReplayCache<String> replayCache = createCache(); + try { + String id = UUID.randomUUID().toString(); + assertTrue(replayCache.putIdIfAbsent(id, null)); + assertTrue(replayCache.contains(id)); + assertFalse(replayCache.putIdIfAbsent(id, null)); + assertFalse(replayCache.putIdIfAbsent(id, Instant.now().plusSeconds(100L))); + + // An identifier added with putId() is also seen + id = UUID.randomUUID().toString(); + replayCache.putId(id); + assertFalse(replayCache.putIdIfAbsent(id, null)); + + // An empty identifier cannot be cached, so is never reported as added + assertFalse(replayCache.putIdIfAbsent(null, null)); + assertFalse(replayCache.putIdIfAbsent("", null)); + assertFalse(replayCache.contains("")); + } finally { + replayCache.close(); + } + } + + // Concurrent requests carrying the same identifier must not all be accepted: exactly one + // putIdIfAbsent call per identifier may succeed. + @Test + public void testTokenReplayCacheConcurrentPutIdIfAbsent() throws Exception { + TokenReplayCache<String> replayCache = createCache(); + + final int threads = 8; + final int identifiers = 500; + List<String> ids = new ArrayList<>(identifiers); + for (int i = 0; i < identifiers; i++) { + ids.add(UUID.randomUUID().toString()); + } + AtomicIntegerArray accepted = new AtomicIntegerArray(identifiers); + CyclicBarrier barrier = new CyclicBarrier(threads); + Instant expiry = Instant.now().plusSeconds(100L); + + ExecutorService executor = Executors.newFixedThreadPool(threads); + try { + List<Future<?>> futures = new ArrayList<>(threads); + for (int t = 0; t < threads; t++) { + futures.add(executor.submit(() -> { + barrier.await(10, TimeUnit.SECONDS); + for (int i = 0; i < identifiers; i++) { + if (replayCache.putIdIfAbsent(ids.get(i), expiry)) { + accepted.incrementAndGet(i); + } + } + return null; + })); + } + for (Future<?> future : futures) { + future.get(30, TimeUnit.SECONDS); + } + } finally { + executor.shutdownNow(); + replayCache.close(); + } + + for (int i = 0; i < identifiers; i++) { + assertEquals("Identifier " + i + " was accepted more than once", 1, accepted.get(i)); + } + } + private void testTokenReplayCacheInstance(TokenReplayCache<String> replayCache) throws InterruptedException { // Test default TTL caches OK
