[java-opensaml] branch main updated: IDP-2163 - Warning interceptor could exploit StorageService
Scott Cantor
cantor.2 at osu.edu
Wed Dec 13 18:19:50 UTC 2023
This is an automated email from the git hooks/post-receive script.
scantor pushed a commit to branch main
in repository java-opensaml.
View the commit online:
http://git.shibboleth.net/view/?p=java-opensaml.git;a=commit;h=64ae5871f2ad34bd59604e0e0505fa4a93f6e0d4
The following commit(s) were added to refs/heads/main by this push:
new 64ae5871f IDP-2163 - Warning interceptor could exploit StorageService
64ae5871f is described below
commit 64ae5871f2ad34bd59604e0e0505fa4a93f6e0d4
Author: Scott Cantor <cantor.2 at osu.edu>
AuthorDate: Wed Dec 13 13:19:47 2023 -0500
IDP-2163 - Warning interceptor could exploit StorageService
https://shibboleth.atlassian.net/browse/IDP-2163
Add extended CookieManager allowing backstop by StorageService.
---
opensaml-storage-api/pom.xml | 5 +
.../storage/StorageAwareCookieManager.java | 162 +++++++++++++++++++++
.../impl/StorageAwareCookieManagerTest.java | 147 +++++++++++++++++++
3 files changed, 314 insertions(+)
diff --git a/opensaml-storage-api/pom.xml b/opensaml-storage-api/pom.xml
index 527f962ce..5789804ea 100644
--- a/opensaml-storage-api/pom.xml
+++ b/opensaml-storage-api/pom.xml
@@ -27,6 +27,11 @@
<version>${project.version}</version>
</dependency>
+ <dependency>
+ <groupId>${shib-shared.groupId}</groupId>
+ <artifactId>shib-networking</artifactId>
+ </dependency>
+
<!-- Provided Dependencies -->
<!-- Runtime Dependencies -->
diff --git a/opensaml-storage-api/src/main/java/org/opensaml/storage/StorageAwareCookieManager.java b/opensaml-storage-api/src/main/java/org/opensaml/storage/StorageAwareCookieManager.java
new file mode 100644
index 000000000..860a15bf4
--- /dev/null
+++ b/opensaml-storage-api/src/main/java/org/opensaml/storage/StorageAwareCookieManager.java
@@ -0,0 +1,162 @@
+/*
+ * 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 org.opensaml.storage;
+
+import java.io.IOException;
+import java.time.Instant;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+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>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.</p>
+ *
+ * @since 5.1.0
+ */
+public class StorageAwareCookieManager extends CookieManager {
+
+ /** Class logger. */
+ @Nonnull private Logger log = LoggerFactory.getLogger(org.opensaml.storage.StorageAwareCookieManager.class);
+
+ /** Optional storage service to backstop the cookie. */
+ @Nullable private StorageService storageService;
+
+ /** 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;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ protected void doInitialize() throws ComponentInitializationException {
+ super.doInitialize();
+
+ if (getMaxAge() == -1 && storageService != null) {
+ log.warn("Unsetting StorageService due to per-session max-age setting");
+ storageService = null;
+ }
+
+ 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 {
+ if (ss.create(storageContext, name, value, exp)) {
+ log.trace("Created new cookie record {}", name);
+ } else if (ss.update(storageContext, name, value, exp)) {
+ log.trace("Updated cookie record {}", name);
+ }
+ } 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);
+
+ if (storageService != null) {
+ try {
+ storageService.delete(storageContext, name);
+ } 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 String val = super.getCookieValue(name, defValue);
+ if (val != null) {
+ return val;
+ }
+
+ if (storageService != null) {
+ try {
+ final StorageRecord<String> record = storageService.read(storageContext, name);
+ if (record != null) {
+ log.debug("Backfilling/setting missing cookie {} based on stored record", name);
+ 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);
+ }
+ }
+
+ return defValue;
+ }
+
+}
\ No newline at end of file
diff --git a/opensaml-storage-impl/src/test/java/org/opensaml/storage/impl/StorageAwareCookieManagerTest.java b/opensaml-storage-impl/src/test/java/org/opensaml/storage/impl/StorageAwareCookieManagerTest.java
new file mode 100644
index 000000000..c546ff0c8
--- /dev/null
+++ b/opensaml-storage-impl/src/test/java/org/opensaml/storage/impl/StorageAwareCookieManagerTest.java
@@ -0,0 +1,147 @@
+/*
+ * 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 org.opensaml.storage.impl;
+
+import java.io.IOException;
+import java.time.Duration;
+import java.time.Instant;
+
+import javax.annotation.Nonnull;
+
+import org.opensaml.storage.StorageAwareCookieManager;
+import org.opensaml.storage.StorageRecord;
+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.shared.component.ComponentInitializationException;
+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();
+ 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.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");
+ 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");
+ 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");
+ Assert.assertNull(record);
+ }
+
+ @Test public void testCookieRestore() throws ComponentInitializationException, IOException {
+ request.setContextPath("/idp");
+
+ storage.create(cm.getStorageContext(), "foo", "bar", Instant.now().plusSeconds(600).toEpochMilli());
+
+ Assert.assertEquals(cm.getCookieValue("foo", null), "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