[java-plugin-shibd] branch main updated: Retrofit cookie-based state manager to use standard cookie manager.
Scott Cantor
cantor.2 at osu.edu
Thu Apr 10 15:10:01 UTC 2025
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository java-plugin-shibd.
View the commit online:
http://git.shibboleth.net/view/?p=java-plugin-shibd.git;a=commit;h=ac3d95afcc722954ec8fd68b647fdeeb08377d5d
The following commit(s) were added to refs/heads/main by this push:
new ac3d95a Retrofit cookie-based state manager to use standard cookie manager.
ac3d95a is described below
commit ac3d95afcc722954ec8fd68b647fdeeb08377d5d
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Thu Apr 10 11:09:59 2025 -0400
Retrofit cookie-based state manager to use standard cookie manager.
---
.../META-INF/net.shibboleth.idp/postconfig.xml | 13 ++
.../shibboleth/idp/module/conf/sp/sp.properties | 11 +-
.../sp/messaging/RemotedHttpServletRequest.java | 27 +---
.../sp/impl/CookieStateTokenManager.java | 156 ++++-----------------
.../sp/impl/CookieStateTokenManagerTest.java | 31 ++--
.../profile/impl/MapResourceToStateTokenTest.java | 9 +-
6 files changed, 74 insertions(+), 173 deletions(-)
diff --git a/sp-conf-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml b/sp-conf-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
index 889aaee..4dfe682 100644
--- a/sp-conf-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
+++ b/sp-conf-impl/src/main/resources/META-INF/net.shibboleth.idp/postconfig.xml
@@ -46,4 +46,17 @@
class="net.shibboleth.sp.profile.impl.TokenConsumerFlowDescriptorManager"
p:components="#{getObject('shibboleth.AvailableTokenConsumerFlows')}" />
+ <bean id="shibboleth.SP.CookieManager" class="net.shibboleth.shared.net.CookieManager" lazy-init="true"
+ p:httpServletRequestSupplier-ref="shibboleth.RemotedHttpServletRequestSupplier"
+ p:httpServletResponseSupplier-ref="shibboleth.RemotedHttpServletResponseSupplier"
+ p:cookieLimit="%{sp.cookie.limit:10}"
+ p:secure="%{sp.cookie.secure:true}"
+ p:httpOnly="%{sp.cookie.httpOnly:true}"
+ p:cookieDomain="%{sp.cookie.domain:}"
+ p:cookiePath="%{sp.cookie.path:/}"
+ p:sameSite="%{sp.cookie.sameSite:None}"
+ p:sameSiteCondition-ref="#{'%{sp.cookie.sameSiteCondition:shibboleth.Conditions.TRUE}'.trim()}"
+ p:sameSiteCookies="#{getObject('shibboleth.SameSiteCookieMap')}"
+ p:maxAge="%{sp.cookie.maxAge:-1}" />
+
</beans>
\ No newline at end of file
diff --git a/sp-conf-impl/src/main/resources/net/shibboleth/idp/module/conf/sp/sp.properties b/sp-conf-impl/src/main/resources/net/shibboleth/idp/module/conf/sp/sp.properties
index 2c488c4..a2cfc72 100644
--- a/sp-conf-impl/src/main/resources/net/shibboleth/idp/module/conf/sp/sp.properties
+++ b/sp-conf-impl/src/main/resources/net/shibboleth/idp/module/conf/sp/sp.properties
@@ -19,12 +19,17 @@ sp.service.agents.checkInterval = PT5M
#sp.application.sessionInitiators =
#sp.application.tokenConsumers =
-# General SP cookie properties (maxAge only applies to persistent cookies)
+# General SP cookie properties
#sp.cookie.secure = true
#sp.cookie.httpOnly = true
#sp.cookie.domain =
-#sp.cookie.path =
-#sp.cookie.maxAge = 31536000
+#sp.cookie.path = /
+#sp.cookie.maxAge = -1
+#sp.cookie.sameSite = None
+#sp.cookie.sameSiteCondition = shibboleth.Conditions.TRUE
+# Controls how many cookies for a given use case are allowed before purging
+#sp.cookie.limit = 10
+
# Default state token management (SAML RelayState, etc.)
# Set to shibboleth.CookieStateTokenManager to switch to cookie-based mechanism
diff --git a/sp-server-api/src/main/java/net/shibboleth/sp/messaging/RemotedHttpServletRequest.java b/sp-server-api/src/main/java/net/shibboleth/sp/messaging/RemotedHttpServletRequest.java
index 3a45746..b6e31e0 100644
--- a/sp-server-api/src/main/java/net/shibboleth/sp/messaging/RemotedHttpServletRequest.java
+++ b/sp-server-api/src/main/java/net/shibboleth/sp/messaging/RemotedHttpServletRequest.java
@@ -410,12 +410,8 @@ public class RemotedHttpServletRequest implements HttpServletRequest {
final String[] nvpair = c.split("=", -1);
if (nvpair.length == 2) {
final String name = nvpair[0].trim();
- // This is a fallback cookie used for Safari to work around SameSite bugs.
- if (name.endsWith("_fgwars")) {
- name.substring(0, name.length() - 7);
- }
assert cookies != null;
- cookies.add(new SortableCookie(name, nvpair[1]));
+ cookies.add(new Cookie(name, nvpair[1]));
}
}
} else {
@@ -621,27 +617,6 @@ public class RemotedHttpServletRequest implements HttpServletRequest {
throw new UnsupportedOperationException();
}
- /** A subclass of {@link Cookie} to enable sorting by name. */
- public static class SortableCookie extends Cookie implements Comparable<Cookie> {
-
- private static final long serialVersionUID = -5930215369912605949L;
-
- /**
- * Constructor.
- *
- * @param name cookie name
- * @param value cookie value
- */
- public SortableCookie(@Nonnull final String name, @Nullable final String value) {
- super(name, value);
- }
-
- /** {@inheritDoc} */
- public int compareTo(final Cookie c) {
- return getName().compareTo(c.getName());
- }
- }
-
/**
* Helper method to decode a byte buffer into either UTF-8 or ISO-8859-1.
*
diff --git a/sp-server-impl/src/main/java/net/shibboleth/sp/impl/CookieStateTokenManager.java b/sp-server-impl/src/main/java/net/shibboleth/sp/impl/CookieStateTokenManager.java
index dcb8ef2..26e936d 100644
--- a/sp-server-impl/src/main/java/net/shibboleth/sp/impl/CookieStateTokenManager.java
+++ b/sp-server-impl/src/main/java/net/shibboleth/sp/impl/CookieStateTokenManager.java
@@ -18,26 +18,21 @@ import java.io.IOException;
import java.security.InvalidAlgorithmParameterException;
import java.security.NoSuchAlgorithmException;
import java.time.Instant;
-import java.util.Arrays;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.slf4j.Logger;
-import jakarta.servlet.http.Cookie;
-import jakarta.servlet.http.HttpServletRequest;
-import jakarta.servlet.http.HttpServletResponse;
import net.shibboleth.shared.annotation.constraint.NonnullAfterInit;
import net.shibboleth.shared.annotation.constraint.NotEmpty;
-import net.shibboleth.shared.annotation.constraint.Positive;
import net.shibboleth.shared.codec.Base64Support;
import net.shibboleth.shared.codec.DecodingException;
import net.shibboleth.shared.codec.EncodingException;
import net.shibboleth.shared.component.ComponentInitializationException;
import net.shibboleth.shared.logic.Constraint;
+import net.shibboleth.shared.net.CookieManager;
import net.shibboleth.shared.primitive.LoggerFactory;
-import net.shibboleth.shared.primitive.NonnullSupplier;
import net.shibboleth.shared.primitive.StringSupport;
import net.shibboleth.shared.security.IdentifierGenerationStrategy;
import net.shibboleth.shared.security.IdentifierGenerationStrategy.ProviderType;
@@ -57,45 +52,16 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
/** Class logger. */
@Nonnull private Logger log = LoggerFactory.getLogger(CookieStateTokenManager.class);
-
- /** Servlet request supplier. */
- @NonnullAfterInit private NonnullSupplier<HttpServletRequest> requestSupplier;
-
- /** Servlet response supplier. */
- @NonnullAfterInit private NonnullSupplier<HttpServletResponse> responseSupplier;
+ /** Cookie manager. */
+ @NonnullAfterInit private CookieManager cookieManager;
+
/** Fixed prefix for cookie names. */
@Nonnull @NotEmpty private String cookiePrefix;
- /** Limit on number of cookies to retain. */
- private int cookieLimit;
-
/** Constructor. */
public CookieStateTokenManager() {
cookiePrefix = DEFAULT_PREFIX;
- cookieLimit = 10;
- }
-
- /**
- * Set {@link HttpServletRequest} supplier.
- *
- * @param supplier request supplier
- */
- public void setHttpServletRequestSupplier(@Nonnull final NonnullSupplier<HttpServletRequest> supplier) {
- checkSetterPreconditions();
-
- requestSupplier = Constraint.isNotNull(supplier, "HttpServletRequest supplier cannot be null");
- }
-
- /**
- * Set {@link HttpServletRequest} supplier.
- *
- * @param supplier request supplier
- */
- public void setHttpServletResponseSupplier(@Nonnull final NonnullSupplier<HttpServletResponse> supplier) {
- checkSetterPreconditions();
-
- responseSupplier = Constraint.isNotNull(supplier, "HttpServletResponse supplier cannot be null");
}
/**
@@ -112,16 +78,14 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
}
/**
- * Set limit on the number of state cookies to permit.
- *
- * <p>Defaults to 10.</p>
+ * Set the {@link CookieManager} to use.
*
- * @param limit limit to set
+ * @param manager instance to use
*/
- public void setCookieLimit(@Positive final int limit) {
+ public void setCookieManager(@Nonnull final CookieManager manager) {
checkSetterPreconditions();
- cookieLimit = Constraint.isGreaterThan(0, limit, "Cookie limit must be positive");
+ cookieManager = Constraint.isNotNull(manager, "CookieManager cannot be null");
}
/** {@inheritDoc} */
@@ -129,8 +93,8 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
protected void doInitialize() throws ComponentInitializationException {
super.doInitialize();
- if (requestSupplier == null || responseSupplier == null) {
- throw new ComponentInitializationException("HttpServletRequest/Response suppliers cannot be null");
+ if (cookieManager == null) {
+ throw new ComponentInitializationException("CookieManager cannot be null");
}
if (getIdentifierGenerationStrategy() == null) {
@@ -144,37 +108,28 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
}
/** {@inheritDoc} */
- @Override
@Nonnull public String preserveToStateToken(@Nonnull final Agent agent, @Nonnull final Application application,
@Nonnull final byte[] value) throws IOException {
- purgeStaleCookies(application);
+ cookieManager.purgeStaleCookies(cookiePrefix);
final Instant ts = Instant.now();
assert ts != null;
final String key = Long.toString(ts.toEpochMilli()) + '_' + generateToken();
-
- Cookie cookie;
+ final String name = getCookieName(application, key);
try {
- cookie = new Cookie(getCookieName(application, key), Base64Support.encodeURLSafe(value));
+ cookieManager.addCookie(name, Base64Support.encodeURLSafe(value), (int) getExpiration().toSeconds());
} catch (final EncodingException e) {
throw new IOException(e);
}
-
- cookie.setMaxAge((int) getExpiration().toSeconds());
- cookie.setAttribute("SameSite", "none");
- // TODO: handle other cookie attributes
- responseSupplier.get().addCookie(cookie);
-
- log.trace("Created state token mapping from '{}' to value '{}'", cookie.getName(), value);
+ log.trace("Created state token mapping from '{}' to value '{}'", name, value);
return key;
}
/** {@inheritDoc} */
- @Override
@Nullable public byte[] recoverFromStateToken(@Nonnull final Agent agent, @Nonnull final Application application,
@Nonnull final String token) throws IOException {
@@ -184,34 +139,18 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
}
final String cookieName = getCookieName(application, token);
-
- final HttpServletRequest request = requestSupplier.get();
- final Cookie[] cookies = request.getCookies();
- if (cookies == null) {
- log.warn("No cookies in request");
- return null;
- }
-
- for (final Cookie c : cookies) {
- if (cookieName.equals(c.getName())) {
- log.trace("Recovered state token mapping from '{}' to value '{}'", token, c.getValue());
- final Cookie unsetCookie = new Cookie(c.getName(), null);
- unsetCookie.setMaxAge(0);
- unsetCookie.setAttribute("SameSite", "none");
- // TODO: handle other cookie attributes
- responseSupplier.get().addCookie(unsetCookie);
- try {
- if (c.getValue() != null) {
- return Base64Support.decodeURLSafe(c.getValue());
- } else {
- return null;
- }
- } catch (final DecodingException e) {
- throw new IOException(e);
- }
+ final String cookieValue = cookieManager.getCookieValue(cookieName, null);
+
+ if (cookieValue != null) {
+ log.trace("Recovered state token mapping from '{}' to value '{}'", token, cookieValue);
+ cookieManager.unsetCookie(cookieName);
+ try {
+ return Base64Support.decodeURLSafe(cookieValue);
+ } catch (final DecodingException e) {
+ throw new IOException(e);
}
}
-
+
log.warn("No cookie found matching state token: '{}'", token);
return null;
}
@@ -238,51 +177,4 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
return builder.toString();
}
- /**
- * Scan incoming cookies for any that are over the limit.
- *
- * @param application the application
- */
- private void purgeStaleCookies(@Nonnull final Application application) {
-
- final HttpServletRequest request = requestSupplier.get();
-
- final Cookie[] cookies = request.getCookies();
- if (cookies == null) {
- return;
- }
-
- final HttpServletResponse response = responseSupplier.get();
-
- // Should be possible because we implement Comparable internally.
- Arrays.sort(cookies);
-
- // This is off by one because we're about to set one.
- int maxCookies = cookieLimit - 1;
- int purgedCookies = 0;
-
- for (int i = cookies.length - 1; i >= 0; --i) {
- if (!cookies[0].getName().startsWith(cookiePrefix)) {
- continue;
- }
-
- if (maxCookies > 0) {
- // Keep it but count against limit.
- --maxCookies;
- } else {
- // We're over the limit, so everything here and older gets cleaned up.
- final Cookie unsetCookie = new Cookie(cookies[0].getName(), null);
- unsetCookie.setMaxAge(0);
- unsetCookie.setAttribute("SameSite", "none");
- // TODO: handle other cookie attributes
- response.addCookie(unsetCookie);
- ++purgedCookies;
- }
- }
-
- if (purgedCookies > 0) {
- log.debug("Purged {} stale state token cookie(s)", purgedCookies);
- }
- }
-
}
\ No newline at end of file
diff --git a/sp-server-impl/src/test/java/net/shibboleth/sp/impl/CookieStateTokenManagerTest.java b/sp-server-impl/src/test/java/net/shibboleth/sp/impl/CookieStateTokenManagerTest.java
index 8039d2d..a7015cb 100644
--- a/sp-server-impl/src/test/java/net/shibboleth/sp/impl/CookieStateTokenManagerTest.java
+++ b/sp-server-impl/src/test/java/net/shibboleth/sp/impl/CookieStateTokenManagerTest.java
@@ -33,8 +33,9 @@ import jakarta.servlet.http.Cookie;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import net.shibboleth.shared.component.ComponentInitializationException;
+import net.shibboleth.shared.net.CookieManager;
+import net.shibboleth.shared.net.CookieManager.SameSiteValue;
import net.shibboleth.shared.primitive.NonnullSupplier;
-import net.shibboleth.sp.messaging.RemotedHttpServletRequest;
import net.shibboleth.sp.profile.impl.BaseAgplicationActionTest;
/**
@@ -43,6 +44,7 @@ import net.shibboleth.sp.profile.impl.BaseAgplicationActionTest;
@SuppressWarnings("javadoc")
public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
+ private CookieManager cookieManager;
private CookieStateTokenManager stateManager;
private MockHttpServletRequest request;
@@ -51,26 +53,35 @@ public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
@BeforeClass
public void setUp() throws ComponentInitializationException {
- stateManager = new CookieStateTokenManager();
- stateManager.setId("test");
- stateManager.setHttpServletRequestSupplier(new NonnullSupplier<HttpServletRequest>() {
+ cookieManager = new CookieManager();
+ cookieManager.setCookiePath("/");
+ cookieManager.setSameSite(SameSiteValue.None);
+ cookieManager.setCookieLimit(10);
+ cookieManager.setHttpServletRequestSupplier(new NonnullSupplier<HttpServletRequest>() {
@Nonnull public HttpServletRequest get() {
assert request != null;
return request;
}
});
- stateManager.setHttpServletResponseSupplier(new NonnullSupplier<HttpServletResponse>() {
+ cookieManager.setHttpServletResponseSupplier(new NonnullSupplier<HttpServletResponse>() {
@Nonnull public HttpServletResponse get() {
assert response != null;
return response;
}
});
+ cookieManager.initialize();
+
+ stateManager = new CookieStateTokenManager();
+ stateManager.setId("test");
+ stateManager.setCookieManager(cookieManager);
+
stateManager.initialize();
}
@AfterClass
public void tearDown() {
stateManager.destroy();
+ cookieManager.destroy();
}
@BeforeMethod
@@ -83,7 +94,7 @@ public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
@Test
public void testMissing() throws IOException {
- request.setCookies(new RemotedHttpServletRequest.SortableCookie(getCookieName(), "foo"));
+ request.setCookies(new Cookie(getCookieName(), "foo"));
Assert.assertNull(stateManager.recoverFromStateToken(agent, application, "foo"));
}
@@ -93,7 +104,7 @@ public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
final List<Cookie> cookies = new ArrayList<>(12);
for (int i = 0; i < 12; ++i) {
- cookies.add(new RemotedHttpServletRequest.SortableCookie(getCookieName(), "foo" + i));
+ cookies.add(new Cookie(getCookieName(), "foo" + i));
Thread.sleep(250);
}
request.setCookies(cookies.toArray(new Cookie[12]));
@@ -102,11 +113,10 @@ public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
assert token != null;
final Cookie[] respCookies = response.getCookies();
- Assert.assertEquals(respCookies.length, 4);
+ Assert.assertEquals(respCookies.length, 3);
Assert.assertEquals(respCookies[0].getMaxAge(), 0);
Assert.assertEquals(respCookies[1].getMaxAge(), 0);
- Assert.assertEquals(respCookies[2].getMaxAge(), 0);
- Assert.assertEquals(respCookies[3].getMaxAge(), stateManager.getExpiration().toSeconds());
+ Assert.assertEquals(respCookies[2].getMaxAge(), stateManager.getExpiration().toSeconds());
}
@Test
@@ -129,6 +139,7 @@ public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
Assert.assertEquals(cookies[0].getName(), CookieStateTokenManager.DEFAULT_PREFIX + '_' + "test" + '_' + token);
Assert.assertEquals(cookies[0].getValue(), null);
Assert.assertEquals(cookies[0].getMaxAge(), 0);
+ Assert.assertEquals(cookies[0].getAttribute("SameSite"), SameSiteValue.None.getValue());
}
@Nonnull private String getCookieName() {
diff --git a/sp-server-impl/src/test/java/net/shibboleth/sp/profile/impl/MapResourceToStateTokenTest.java b/sp-server-impl/src/test/java/net/shibboleth/sp/profile/impl/MapResourceToStateTokenTest.java
index 7b297cc..c8ea8ed 100644
--- a/sp-server-impl/src/test/java/net/shibboleth/sp/profile/impl/MapResourceToStateTokenTest.java
+++ b/sp-server-impl/src/test/java/net/shibboleth/sp/profile/impl/MapResourceToStateTokenTest.java
@@ -34,6 +34,7 @@ import net.shibboleth.shared.annotation.constraint.NotEmpty;
import net.shibboleth.shared.codec.Base64Support;
import net.shibboleth.shared.codec.DecodingException;
import net.shibboleth.shared.component.ComponentInitializationException;
+import net.shibboleth.shared.net.CookieManager;
import net.shibboleth.shared.primitive.NonnullSupplier;
import net.shibboleth.sp.ddf.DDF;
import net.shibboleth.sp.impl.CookieStateTokenManager;
@@ -181,10 +182,14 @@ public class MapResourceToStateTokenTest extends BaseAgplicationActionTest {
final MockHttpServletRequest request = new MockHttpServletRequest();
final MockHttpServletResponse response = new MockHttpServletResponse();
+ final CookieManager cookieManager = new CookieManager();
+ cookieManager.setHttpServletRequestSupplier(NonnullSupplier.of(request));
+ cookieManager.setHttpServletResponseSupplier(NonnullSupplier.of(response));
+ cookieManager.initialize();
+
final CookieStateTokenManager manager = new CookieStateTokenManager();
manager.setId("test");
- manager.setHttpServletRequestSupplier(NonnullSupplier.of(request));
- manager.setHttpServletResponseSupplier(NonnullSupplier.of(response));
+ manager.setCookieManager(cookieManager);
manager.initialize();
application.setStateTokenManager(manager);
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list