This is an automated email from the ASF dual-hosted git repository.
coheigea pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cxf.git
The following commit(s) were added to refs/heads/main by this push:
new d27cfb07780 make the SAMLSSOResponseValidator replay check atomic
(#3625)
d27cfb07780 is described below
commit d27cfb07780e19478887d7ebbac4fddb9f346814
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
---
.../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 cc53375e241..2a814646382 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
@@ -139,6 +139,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
