[java-identity-provider] 09/28: IDP-2163 - Warning interceptor flow could exploit StorageService
Scott Cantor
cantor.2 at osu.edu
Wed Jan 31 14:51:55 UTC 2024
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch dev/thymeleaf
in repository java-identity-provider.
View the commit online:
http://git.shibboleth.net/view/?p=java-identity-provider.git;a=commit;h=94718eda6e5043d4f8497b07220c69c1eed70221
commit 94718eda6e5043d4f8497b07220c69c1eed70221
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Thu Dec 14 11:23:27 2023 -0500
IDP-2163 - Warning interceptor flow could exploit StorageService
https://shibboleth.atlassian.net/browse/IDP-2163
Storage-backed cookie manager added to IdP.
Wired into warning and exp-password flows.
Property added to toggle in a StorageService to use.
---
.../net/shibboleth/idp/conf/global-system.xml | 16 ++
.../flows/intercept/expiring-password-beans.xml | 2 +-
.../idp/flows/intercept/warning-beans.xml | 2 +-
.../profile/support/StorageAwareCookieManager.java | 213 +++++++++++++++++++++
.../support/StorageAwareCookieManagerTest.java | 152 +++++++++++++++
5 files changed, 383 insertions(+), 2 deletions(-)
diff --git a/idp-conf-impl/src/main/resources/net/shibboleth/idp/conf/global-system.xml b/idp-conf-impl/src/main/resources/net/shibboleth/idp/conf/global-system.xml
index a45ddc1fa..3d7d25874 100644
--- a/idp-conf-impl/src/main/resources/net/shibboleth/idp/conf/global-system.xml
+++ b/idp-conf-impl/src/main/resources/net/shibboleth/idp/conf/global-system.xml
@@ -338,6 +338,22 @@
p:cookiePath="%{idp.cookie.path:/}"
p:maxAge="%{idp.cookie.maxAge:31536000}" />
+ <bean id="shibboleth.StorageAwarePersistentCookieManager" class="net.shibboleth.idp.profile.support.StorageAwareCookieManager" lazy-init="true"
+ p:httpServletRequestSupplier-ref="shibboleth.HttpServletRequestSupplier"
+ p:httpServletResponseSupplier-ref="shibboleth.HttpServletResponseSupplier"
+ p:storageService="#{getObject('%{idp.cookie.storageService:}'.trim())}"
+ p:secure="%{idp.cookie.secure:true}"
+ p:httpOnly="%{idp.cookie.httpOnly:true}"
+ p:cookieDomain="%{idp.cookie.domain:}"
+ p:cookiePath="%{idp.cookie.path:/}"
+ p:maxAge="%{idp.cookie.maxAge:31536000}">
+ <property name="usernameLookupStrategy">
+ <bean parent="shibboleth.Functions.Compose"
+ c:g-ref="shibboleth.PrincipalNameLookup.Subject"
+ c:f-ref="shibboleth.ChildLookup.SubjectContext" />
+ </property>
+ </bean>
+
<bean id="shibboleth.UserPrefsCookieManager" class="net.shibboleth.shared.net.CookieManager" lazy-init="true"
p:httpServletRequestSupplier-ref="shibboleth.HttpServletRequestSupplier"
p:httpServletResponseSupplier-ref="shibboleth.HttpServletResponseSupplier"
diff --git a/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/expiring-password-beans.xml b/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/expiring-password-beans.xml
index 431a117f9..5efb1363e 100644
--- a/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/expiring-password-beans.xml
+++ b/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/expiring-password-beans.xml
@@ -25,6 +25,6 @@
<alias name="shibboleth.expiring-password.Condition" alias="ExpiringPasswordPredicate"/>
<alias name="shibboleth.expiring-password.NotifyCookieName" alias="ExpiringPasswordCookieName" />
<alias name="shibboleth.expiring-password.NotifyInterval" alias="ExpiringPasswordNotifyInterval" />
- <alias name="shibboleth.PersistentCookieManager" alias="ExpiringPasswordCookieManager" />
+ <alias name="shibboleth.StorageAwarePersistentCookieManager" alias="ExpiringPasswordCookieManager" />
</beans>
diff --git a/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/warning-beans.xml b/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/warning-beans.xml
index 1dfbe6334..80b0bc7dc 100644
--- a/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/warning-beans.xml
+++ b/idp-conf-impl/src/main/resources/net/shibboleth/idp/flows/intercept/warning-beans.xml
@@ -28,6 +28,6 @@
<!-- Simplifies flow definition expressions. -->
<alias name="shibboleth.warning.ConditionMap" alias="WarningConditionMap" />
<alias name="shibboleth.warning.CookieCondition" alias="WarningCookieCondition" />
- <alias name="shibboleth.PersistentCookieManager" alias="WarningCookieManager" />
+ <alias name="shibboleth.StorageAwarePersistentCookieManager" alias="WarningCookieManager" />
</beans>
diff --git a/idp-profile-impl/src/main/java/net/shibboleth/idp/profile/support/StorageAwareCookieManager.java b/idp-profile-impl/src/main/java/net/shibboleth/idp/profile/support/StorageAwareCookieManager.java
new file mode 100644
index 000000000..d555ce187
--- /dev/null
+++ b/idp-profile-impl/src/main/java/net/shibboleth/idp/profile/support/StorageAwareCookieManager.java
@@ -0,0 +1,213 @@
+/*
+ * 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.idp.profile.support;
+
+import java.io.IOException;
+import java.time.Instant;
+import java.util.function.Function;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+import org.opensaml.profile.context.ProfileRequestContext;
+import org.opensaml.storage.StorageRecord;
+import org.opensaml.storage.StorageService;
+import org.slf4j.Logger;
+
+import net.shibboleth.shared.annotation.constraint.NonnullAfterInit;
+import net.shibboleth.shared.component.ComponentInitializationException;
+import net.shibboleth.shared.net.CookieManager;
+import net.shibboleth.shared.primitive.LoggerFactory;
+
+/**
+ * An extended {@link CookieManager} that allows use of a {@link StorageService}.
+ *
+ * <p>Reads are backed up by a read into the storage service, while writes are passed
+ * through to it.</p>
+ *
+ * <p>The cookie path and domain, and the username (as obtained by a lookup function)
+ * are used to construct the storage context and key to maintain the expected isolation.
+ * Notably, the function operates by obtaining the {@link ProfileRequestContext} from the
+ * bound request attribute to address the fact that the API does not directly include it.</p>
+ *
+ * <p>This is NOT suitable for use cases in which consistency of data is critical, as
+ * there are few if any storage options (other than the client itself) that will provide
+ * sufficient reliability and locking to avoid problems. It's best used for advisory cookies
+ * whose absence does not create issues with security or expected behavior.</p>
+ *
+ * @since 5.1.0
+ */
+public class StorageAwareCookieManager extends CookieManager {
+
+ /** Class logger. */
+ @Nonnull private Logger log = LoggerFactory.getLogger(StorageAwareCookieManager.class);
+
+ /** Optional storage service to backstop the cookie. */
+ @Nullable private StorageService storageService;
+
+ /** Lookup strategy for username. */
+ @Nullable private Function<ProfileRequestContext,String> usernameLookupStrategy;
+
+ /** Storage context based on fixed value and cookie attributes. */
+ @NonnullAfterInit private String storageContext;
+
+ /**
+ * Sets the {@link StorageService} to read/write.
+ *
+ * @param ss storage service
+ */
+ public void setStorageService(@Nullable final StorageService ss) {
+ checkSetterPreconditions();
+
+ storageService = ss;
+ }
+
+ /**
+ * Get the storage context used to hold the cookies.
+ *
+ * @return storage context
+ */
+ @NonnullAfterInit public String getStorageContext() {
+ return storageContext;
+ }
+
+ /**
+ * Sets the lookup strategy to obtain the username for cookie partitioning.
+ *
+ * @param strategy lookup strategy
+ */
+ public void setUsernameLookupStrategy(@Nullable final Function<ProfileRequestContext,String> strategy) {
+ checkSetterPreconditions();
+
+ usernameLookupStrategy = strategy;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ protected void doInitialize() throws ComponentInitializationException {
+ super.doInitialize();
+
+ if (storageService != null) {
+ if (getMaxAge() == -1) {
+ throw new ComponentInitializationException(
+ "StorageService cannot be used for per-session cookie manager.");
+ } else if (usernameLookupStrategy == null) {
+ throw new ComponentInitializationException("StorageService use requires a username lookup strategy.");
+ }
+ }
+
+ final StringBuilder contextBuilder = new StringBuilder(getClass().getName());
+ contextBuilder.append('!');
+ if (getCookieDomain() != null) {
+ contextBuilder.append(getCookieDomain());
+ }
+ contextBuilder.append('!');
+ if (getCookiePath() != null) {
+ contextBuilder.append(getCookiePath());
+ }
+
+ storageContext = contextBuilder.toString();
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void addCookie(@Nonnull final String name, @Nonnull final String value) {
+ super.addCookie(name, value);
+
+ final Long exp = Instant.now().plusSeconds(getMaxAge()).toEpochMilli();
+
+ final StorageService ss = storageService;
+ if (ss != null) {
+ try {
+ final String cookieName = getPartitionedCookieName(name);
+ if (ss.create(storageContext, cookieName, value, exp)) {
+ log.trace("Created new cookie record {}", cookieName);
+ } else if (ss.update(storageContext, cookieName, value, exp)) {
+ log.trace("Updated cookie record {}", cookieName);
+ }
+ } catch (final IOException e) {
+ log.warn("Error creating/updating cookie record in storage service", e);
+ }
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void unsetCookie(@Nonnull final String name) {
+ super.unsetCookie(name);
+
+ final StorageService ss = storageService;
+ if (ss != null) {
+ try {
+ final String cookieName = getPartitionedCookieName(name);
+ ss.delete(storageContext, cookieName);
+ log.trace("Deleted cookie record {}", cookieName);
+ } catch (final IOException e) {
+ log.warn("Error deleting cookie record from storage service", e);
+ }
+ }
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ @Nullable public String getCookieValue(@Nonnull final String name, @Nullable final String defValue) {
+
+ final StorageService ss = storageService;
+ if (ss != null) {
+ try {
+ final String val = super.getCookieValue(name, null);
+ if (val != null) {
+ return val;
+ }
+
+ final String cookieName = getPartitionedCookieName(name);
+ final StorageRecord<String> record = ss.read(storageContext, cookieName);
+ if (record != null) {
+ log.debug("Backfilling/setting missing cookie {} based on stored record", cookieName);
+ final Long exp = record.getExpiration();
+ if (exp != null) {
+ // Uses protected hook to override max-age to backdate it.
+ super.addCookie(name, record.getValue(), (int) (exp - Instant.now().toEpochMilli()) / 1000);
+ } else {
+ // Won't ever happen, per init checking.
+ super.addCookie(name, record.getValue(), -1);
+ }
+ return record.getValue();
+ }
+ } catch (final IOException e) {
+ log.warn("Error reading cookie record from storage service", e);
+ }
+ } else {
+ return super.getCookieValue(name, defValue);
+ }
+
+ return defValue;
+ }
+
+ @Nonnull protected String getPartitionedCookieName(@Nonnull final String cookieName) throws IOException {
+
+ final Object obj = getHttpServletRequest().getAttribute(ProfileRequestContext.BINDING_KEY);
+ if (obj instanceof ProfileRequestContext prc) {
+ final String username = usernameLookupStrategy != null ? usernameLookupStrategy.apply(prc) : null;
+ if (username != null) {
+ return cookieName + '!' + username;
+ }
+ throw new IOException("Username was unavailable");
+ }
+
+ throw new IOException("ProfileRequestContext was unavailable");
+ }
+
+}
\ No newline at end of file
diff --git a/idp-profile-impl/src/test/java/net/shibboleth/idp/profile/support/StorageAwareCookieManagerTest.java b/idp-profile-impl/src/test/java/net/shibboleth/idp/profile/support/StorageAwareCookieManagerTest.java
new file mode 100644
index 000000000..2f87d14e5
--- /dev/null
+++ b/idp-profile-impl/src/test/java/net/shibboleth/idp/profile/support/StorageAwareCookieManagerTest.java
@@ -0,0 +1,152 @@
+/*
+ * 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.idp.profile.support;
+
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+import javax.annotation.Nonnull;
+
+import org.opensaml.profile.context.ProfileRequestContext;
+import org.opensaml.storage.StorageRecord;
+import org.opensaml.storage.impl.MemoryStorageService;
+import org.springframework.mock.web.MockHttpServletRequest;
+import org.springframework.mock.web.MockHttpServletResponse;
+import org.testng.Assert;
+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.idp.profile.testing.RequestContextBuilder;
+import net.shibboleth.shared.component.ComponentInitializationException;
+import net.shibboleth.shared.logic.FunctionSupport;
+import net.shibboleth.shared.primitive.NonnullSupplier;
+
+/** {@link CookieManager} unit test. */
+ at SuppressWarnings("javadoc")
+public class StorageAwareCookieManagerTest {
+
+ private MockHttpServletRequest request;
+ private MockHttpServletResponse response;
+
+ private MemoryStorageService storage;
+
+ private StorageAwareCookieManager cm;
+
+ @BeforeMethod
+ public void setUp() throws ComponentInitializationException {
+ request = new MockHttpServletRequest();
+ request.setAttribute(ProfileRequestContext.BINDING_KEY, new RequestContextBuilder().buildProfileRequestContext());
+ response = new MockHttpServletResponse();
+
+ storage = new MemoryStorageService();
+ storage.setId("test");
+ storage.setCleanupInterval(Duration.ZERO);
+ storage.initialize();
+
+ cm = new StorageAwareCookieManager();
+ cm.setHttpServletRequestSupplier(new NonnullSupplier<>() { @Nonnull public HttpServletRequest get() {return request;}});
+ cm.setHttpServletResponseSupplier(new NonnullSupplier<>() { @Nonnull public HttpServletResponse get() {return response;}});
+ cm.setCookiePath("/idp");
+ cm.setStorageService(storage);
+ cm.setUsernameLookupStrategy(FunctionSupport.constant("jdoe"));
+ cm.setMaxAge(600);
+ cm.initialize();
+ }
+
+ @Test public void testInitFailure() {
+ final StorageAwareCookieManager cm = new StorageAwareCookieManager();
+ try {
+ cm.initialize();
+ Assert.fail();
+ } catch (final ComponentInitializationException e) {
+
+ }
+ }
+ @Test public void testCookieWithPath() throws ComponentInitializationException, IOException {
+
+ cm.addCookie("foo", "bar");
+
+ final Cookie cookie = response.getCookie("foo");
+ assert(cookie != null);
+ Assert.assertEquals(cookie.getValue(), "bar");
+ Assert.assertEquals(cookie.getPath(), "/idp");
+ Assert.assertNull(cookie.getDomain());
+ Assert.assertTrue(cookie.getSecure());
+ Assert.assertEquals(cookie.getMaxAge(), 600);
+
+ final StorageRecord<String> record = storage.read(cm.getStorageContext(), "foo!jdoe");
+ assert record != null;
+ Assert.assertEquals(record.getVersion(), 1);
+ Assert.assertEquals(record.getValue(), "bar");
+ }
+
+ @Test public void testCookieNoPath() throws ComponentInitializationException, IOException {
+ request.setContextPath("/idp");
+
+ cm.addCookie("foo", "bar");
+
+ final Cookie cookie = response.getCookie("foo");
+ assert(cookie != null);
+ Assert.assertEquals(cookie.getValue(), "bar");
+ Assert.assertEquals(cookie.getPath(), "/idp");
+ Assert.assertNull(cookie.getDomain());
+ Assert.assertTrue(cookie.getSecure());
+ Assert.assertEquals(cookie.getMaxAge(), 600);
+
+ final StorageRecord<String> record = storage.read(cm.getStorageContext(), "foo!jdoe");
+ assert record != null;
+ Assert.assertEquals(record.getVersion(), 1);
+ Assert.assertEquals(record.getValue(), "bar");
+ }
+
+ @Test public void testCookieUnset() throws ComponentInitializationException, IOException {
+ request.setContextPath("/idp");
+ request.setCookies(new Cookie("foo", "bar"));
+
+ cm.unsetCookie("foo");
+
+ final Cookie cookie = response.getCookie("foo");
+ assert(cookie != null);
+ Assert.assertNull(cookie.getValue());
+ Assert.assertEquals(cookie.getPath(), "/idp");
+ Assert.assertNull(cookie.getDomain());
+ Assert.assertTrue(cookie.getSecure());
+ Assert.assertEquals(cookie.getMaxAge(), 0);
+
+ final StorageRecord<String> record = storage.read(cm.getStorageContext(), "foo!jdoe");
+ Assert.assertNull(record);
+ }
+
+ @Test public void testCookieRestore() throws ComponentInitializationException, IOException {
+ request.setContextPath("/idp");
+
+ storage.create(cm.getStorageContext(), "foo!jdoe", "bar", Instant.now().plusSeconds(600).toEpochMilli());
+
+ Assert.assertEquals(cm.getCookieValue("foo", "baz"), "bar");
+
+ final Cookie cookie = response.getCookie("foo");
+ assert(cookie != null);
+ Assert.assertEquals(cookie.getValue(), "bar");
+ Assert.assertEquals(cookie.getPath(), "/idp");
+ Assert.assertNull(cookie.getDomain());
+ Assert.assertTrue(cookie.getSecure());
+ Assert.assertTrue(cookie.getMaxAge() <= 600);
+ }
+
+}
\ 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