[java-plugin-shibd] branch main updated: Unit tests and fixes to cookie state manager.
Scott Cantor
cantor.2 at osu.edu
Mon Aug 19 16:21:16 UTC 2024
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=4085ea30232627bdd1faedeee685c474f3929f64
The following commit(s) were added to refs/heads/main by this push:
new 4085ea3 Unit tests and fixes to cookie state manager.
4085ea3 is described below
commit 4085ea30232627bdd1faedeee685c474f3929f64
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Mon Aug 19 12:21:12 2024 -0400
Unit tests and fixes to cookie state manager.
---
.../shibboleth/sp/AbstractStateTokenManager.java | 9 ++
.../sp/messaging/RemotedHttpServletRequest.java | 2 +-
.../sp/impl/CookieStateTokenManager.java | 83 +++++++-----
.../sp/impl/CookieStateTokenManagerTest.java | 140 +++++++++++++++++++++
4 files changed, 201 insertions(+), 33 deletions(-)
diff --git a/sp-server-api/src/main/java/net/shibboleth/sp/AbstractStateTokenManager.java b/sp-server-api/src/main/java/net/shibboleth/sp/AbstractStateTokenManager.java
index 9aca9da..2905e47 100644
--- a/sp-server-api/src/main/java/net/shibboleth/sp/AbstractStateTokenManager.java
+++ b/sp-server-api/src/main/java/net/shibboleth/sp/AbstractStateTokenManager.java
@@ -45,6 +45,15 @@ public abstract class AbstractStateTokenManager extends AbstractIdentifiableInit
expiration = Duration.ofMinutes(30);
}
+ /**
+ * Get {@link IdentifierGenerationStrategy} to use.
+ *
+ * @return identifier generator strategy
+ */
+ @NonnullAfterInit public IdentifierGenerationStrategy getIdentifierGenerationStrategy() {
+ return identifierStrategy;
+ }
+
/**
* Set {@link IdentifierGenerationStrategy} to use.
*
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 a3ea79d..4714f0c 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
@@ -591,7 +591,7 @@ public class RemotedHttpServletRequest implements HttpServletRequest {
}
/** A subclass of {@link Cookie} to enable sorting by name. */
- public class SortableCookie extends Cookie implements Comparable<Cookie> {
+ public static class SortableCookie extends Cookie implements Comparable<Cookie> {
private static final long serialVersionUID = -5930215369912605949L;
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 472e003..dcb8ef2 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
@@ -15,6 +15,8 @@
package net.shibboleth.sp.impl;
import java.io.IOException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.NoSuchAlgorithmException;
import java.time.Instant;
import java.util.Arrays;
@@ -27,6 +29,7 @@ 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;
@@ -36,6 +39,9 @@ import net.shibboleth.shared.logic.Constraint;
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;
+import net.shibboleth.shared.security.RandomIdentifierParameterSpec;
import net.shibboleth.sp.AbstractStateTokenManager;
import net.shibboleth.sp.Agent;
import net.shibboleth.sp.Application;
@@ -46,6 +52,9 @@ import net.shibboleth.sp.StateTokenManager;
*/
public class CookieStateTokenManager extends AbstractStateTokenManager {
+ /** Default cookie prefix. */
+ @Nonnull @NotEmpty public static String DEFAULT_PREFIX = "_shibsp_state";
+
/** Class logger. */
@Nonnull private Logger log = LoggerFactory.getLogger(CookieStateTokenManager.class);
@@ -56,13 +65,14 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
@NonnullAfterInit private NonnullSupplier<HttpServletResponse> responseSupplier;
/** Fixed prefix for cookie names. */
- @NonnullAfterInit private String cookiePrefix;
+ @Nonnull @NotEmpty private String cookiePrefix;
/** Limit on number of cookies to retain. */
private int cookieLimit;
/** Constructor. */
public CookieStateTokenManager() {
+ cookiePrefix = DEFAULT_PREFIX;
cookieLimit = 10;
}
@@ -91,9 +101,11 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
/**
* Set the fixed prefix to use for the cookies.
*
+ * <p>Defaults to "_shibsp_state".</p>
+ *
* @param prefix cookie prefix
*/
- public void setCookiePrefix(@Nonnull final String prefix) {
+ public void setCookiePrefix(@Nonnull @NotEmpty final String prefix) {
checkSetterPreconditions();
cookiePrefix = Constraint.isNotNull(StringSupport.trimOrNull(prefix), "Cookie prefix cannot be null or empty");
@@ -119,8 +131,15 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
if (requestSupplier == null || responseSupplier == null) {
throw new ComponentInitializationException("HttpServletRequest/Response suppliers cannot be null");
- } else if (cookiePrefix == null) {
- throw new ComponentInitializationException("Cookie prefix cannot be null");
+ }
+
+ if (getIdentifierGenerationStrategy() == null) {
+ final RandomIdentifierParameterSpec spec = new RandomIdentifierParameterSpec(null, 6, null);
+ try {
+ setIdentifierGenerationStrategy(IdentifierGenerationStrategy.getInstance(ProviderType.SECURE, spec));
+ } catch (final InvalidAlgorithmParameterException | NoSuchAlgorithmException e) {
+ throw new ComponentInitializationException(e);
+ }
}
}
@@ -134,7 +153,7 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
final Instant ts = Instant.now();
assert ts != null;
- final String key = ts.toEpochMilli() + '_' + generateToken();
+ final String key = Long.toString(ts.toEpochMilli()) + '_' + generateToken();
Cookie cookie;
try {
@@ -151,7 +170,7 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
log.trace("Created state token mapping from '{}' to value '{}'", cookie.getName(), value);
- return "cookie:" + key;
+ return key;
}
/** {@inheritDoc} */
@@ -159,12 +178,12 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
@Nullable public byte[] recoverFromStateToken(@Nonnull final Agent agent, @Nonnull final Application application,
@Nonnull final String token) throws IOException {
- if (token.length() < 8 || !token.startsWith("cookie:")) {
+ if (token.isEmpty()) {
log.warn("Invalid state token: '{}'", token);
return null;
}
- final String cookieName = getCookieName(application, token.substring(7));
+ final String cookieName = getCookieName(application, token);
final HttpServletRequest request = requestSupplier.get();
final Cookie[] cookies = request.getCookies();
@@ -197,6 +216,28 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
return null;
}
+ /**
+ * Computes the name of a new state cookie.
+ *
+ * @param application the application
+ * @param uniquePortion unique portion of name
+ *
+ * @return cookie name
+ */
+ @Nonnull public String getCookieName(@Nonnull final Application application, @Nonnull final String uniquePortion) {
+
+ // Format is prefix_appId_timestamp_random
+ // The timestamp allows them to be sorted for staleness.
+
+ final StringBuilder builder = new StringBuilder(cookiePrefix);
+ builder.append('_')
+ .append(application.getId())
+ .append('_')
+ .append(uniquePortion);
+
+ return builder.toString();
+ }
+
/**
* Scan incoming cookies for any that are over the limit.
*
@@ -216,7 +257,8 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
// Should be possible because we implement Comparable internally.
Arrays.sort(cookies);
- int maxCookies = cookieLimit;
+ // 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) {
@@ -243,27 +285,4 @@ public class CookieStateTokenManager extends AbstractStateTokenManager {
}
}
- /**
- * Computes the name of a new state cookie.
- *
- * @param application the application
- * @param uniquePortion unique portion of name
- *
- * @return cookie name
- */
- @Nonnull private String getCookieName(@Nonnull final Application application, @Nonnull final String uniquePortion) {
-
- final StringBuilder builder = new StringBuilder(cookiePrefix);
-
- // Format is prefix_appId_timestamp_random
- // The timestamp allows them to be sorted for staleness.
-
- builder.append('_')
- .append(application.getId())
- .append('_')
- .append(uniquePortion);
-
- return builder.toString();
- }
-
}
\ 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
new file mode 100644
index 0000000..8039d2d
--- /dev/null
+++ b/sp-server-impl/src/test/java/net/shibboleth/sp/impl/CookieStateTokenManagerTest.java
@@ -0,0 +1,140 @@
+/*
+ * Licensed 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 net.shibboleth.sp.impl;
+
+import java.io.IOException;
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.List;
+
+import javax.annotation.Nonnull;
+
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.testng.Assert;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+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.primitive.NonnullSupplier;
+import net.shibboleth.sp.messaging.RemotedHttpServletRequest;
+import net.shibboleth.sp.profile.impl.BaseAgplicationActionTest;
+
+/**
+ * Unit tests for {@link CookieStateTokenManager}.
+ */
+ at SuppressWarnings("javadoc")
+public class CookieStateTokenManagerTest extends BaseAgplicationActionTest {
+
+ private CookieStateTokenManager stateManager;
+
+ private MockHttpServletRequest request;
+ private MockHttpServletResponse response;
+
+ @BeforeClass
+ public void setUp() throws ComponentInitializationException {
+
+ stateManager = new CookieStateTokenManager();
+ stateManager.setId("test");
+ stateManager.setHttpServletRequestSupplier(new NonnullSupplier<HttpServletRequest>() {
+ @Nonnull public HttpServletRequest get() {
+ assert request != null;
+ return request;
+ }
+ });
+ stateManager.setHttpServletResponseSupplier(new NonnullSupplier<HttpServletResponse>() {
+ @Nonnull public HttpServletResponse get() {
+ assert response != null;
+ return response;
+ }
+ });
+ stateManager.initialize();
+ }
+
+ @AfterClass
+ public void tearDown() {
+ stateManager.destroy();
+ }
+
+ @BeforeMethod
+ public void beforeMethod() throws ComponentInitializationException {
+ super.beforeMethod();
+
+ request = new MockHttpServletRequest();
+ response = new MockHttpServletResponse();
+ }
+
+ @Test
+ public void testMissing() throws IOException {
+ request.setCookies(new RemotedHttpServletRequest.SortableCookie(getCookieName(), "foo"));
+
+ Assert.assertNull(stateManager.recoverFromStateToken(agent, application, "foo"));
+ }
+
+ @Test
+ public void testPurge() throws IOException, InterruptedException {
+
+ final List<Cookie> cookies = new ArrayList<>(12);
+ for (int i = 0; i < 12; ++i) {
+ cookies.add(new RemotedHttpServletRequest.SortableCookie(getCookieName(), "foo" + i));
+ Thread.sleep(250);
+ }
+ request.setCookies(cookies.toArray(new Cookie[12]));
+
+ final String token = stateManager.preserveToStateToken(agent, application, "foo".getBytes());
+ assert token != null;
+
+ final Cookie[] respCookies = response.getCookies();
+ Assert.assertEquals(respCookies.length, 4);
+ 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());
+ }
+
+ @Test
+ public void testMapRecover() throws IOException {
+
+ final String token = stateManager.preserveToStateToken(agent, application, "foo".getBytes());
+ assert token != null;
+
+ // Move token set on response to request.
+ request = new MockHttpServletRequest();
+ request.setCookies(response.getCookies());
+ response = new MockHttpServletResponse();
+
+ final byte[] original = stateManager.recoverFromStateToken(agent, application, token);
+ Assert.assertEquals(original, "foo".getBytes());
+
+ // Check that old token is unset.
+ final Cookie[] cookies = response.getCookies();
+ Assert.assertEquals(cookies.length, 1);
+ Assert.assertEquals(cookies[0].getName(), CookieStateTokenManager.DEFAULT_PREFIX + '_' + "test" + '_' + token);
+ Assert.assertEquals(cookies[0].getValue(), null);
+ Assert.assertEquals(cookies[0].getMaxAge(), 0);
+ }
+
+ @Nonnull private String getCookieName() {
+ final Instant now = Instant.now();
+ final String rand = stateManager.getIdentifierGenerationStrategy().generateIdentifier(false);
+ return stateManager.getCookieName(application, now.toEpochMilli() + '_' + rand);
+ }
+
+}
\ No newline at end of file
--
To stop receiving notification emails like this one, please contact
the administrator of this repository.
More information about the commits
mailing list