From f7ce2cf44cefe17c022e2ffb99d5ff4fe3c2efc8 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Thu, 22 Oct 2026 20:41:51 +0100 Subject: [PATCH 01/19] feat: reimplement SAML SSO on pac4j-saml (PR 5 of jakarta migration #861) java-saml has no usable Jakarta release (its Auth class only accepts javax.servlet types), which left ACS, Logout, SLS and SSOLogin unable to compile. SSO now runs on pac4j-saml 6.6 (OpenSAML 5, Jakarta-native, itself built on Spring 7). - utils.SamlSso builds the SAML2Client from the existing onelogin.saml.properties keys, so deployments keep their config: the SP's PEM cert/key go into an in-memory keystore (PKCS#8 or PKCS#1), and the IdP settings are turned into an IdP metadata document. Nothing is written to disk. - utils.SsoUserMapper holds the attribute -> user/role mapping that used to be inline in ACS, now unit-testable. - The five servlets delegate to SamlSso; the Getter.authUserSSO hand-off and session setup are unchanged. Bugs fixed along the way (each covered by a test): - SLS only defined handleRequest(), so /saml/sls answered 405 to every IdP logout message. It now handles GET and POST. - Logout read the NameID/session index from the freshly created session after invalidating the old one, so SLO requests carried no NameID. ACS now stores the SAML profile in the post-login session and Logout reads it before invalidating. - ACS hit a NullPointerException when a configured attribute was missing, and left a blank page when the assertion had no attributes; both now return to the login page with a message. Build: spring-core is no longer declared test-scoped (that would strip pac4j's compile-scope copy from the WAR); a spring-framework-bom import keeps every Spring module on ${spring.version}. commons-io 2.18 -> 2.22 to match what pac4j is built against. log4j-slf4j2-impl routes pac4j / OpenSAML logging into log4j2. Refs #861 Co-Authored-By: Claude Opus 5.5 --- pom.xml | 43 +- src/main/java/servlets/ACS.java | 325 ++++-------- src/main/java/servlets/Logout.java | 56 +-- src/main/java/servlets/SLS.java | 95 ++-- src/main/java/servlets/SSOLogin.java | 12 +- src/main/java/servlets/SSOMetadata.java | 28 +- src/main/java/utils/SamlSso.java | 472 ++++++++++++++++++ src/main/java/utils/SsoUserMapper.java | 124 +++++ .../resources/onelogin.saml.properties.sample | 65 +-- src/main/resources/sso.properties.sample | 14 + src/test/java/servlets/SLSTest.java | 58 +++ src/test/java/utils/SamlSsoTest.java | 242 +++++++++ src/test/java/utils/SamlTestSupport.java | 298 +++++++++++ src/test/java/utils/SsoUserMapperTest.java | 106 ++++ 14 files changed, 1513 insertions(+), 425 deletions(-) create mode 100644 src/main/java/utils/SamlSso.java create mode 100644 src/main/java/utils/SsoUserMapper.java create mode 100644 src/test/java/servlets/SLSTest.java create mode 100644 src/test/java/utils/SamlSsoTest.java create mode 100644 src/test/java/utils/SamlTestSupport.java create mode 100644 src/test/java/utils/SsoUserMapperTest.java diff --git a/pom.xml b/pom.xml index c58143380..4eb3f3d69 100644 --- a/pom.xml +++ b/pom.xml @@ -16,13 +16,41 @@ 7.0.10 + 6.6.0 + + + + + org.springframework + spring-framework-bom + ${spring.version} + pom + import + + + + + + + org.pac4j + pac4j-saml + ${pac4j.version} + - com.onelogin - java-saml - 2.9.0 + org.pac4j + pac4j-jakartaee + ${pac4j.version} + + + + org.apache.logging.log4j + log4j-slf4j2-impl + 2.25.4 de.mkammerer @@ -50,7 +78,7 @@ commons-io commons-io - 2.18.0 + 2.22.0 @@ -157,13 +185,6 @@ test - - - org.springframework - spring-core - ${spring.version} - test - + 7.0.10 6.6.0 From a7aa59c24a68e3f4b632a4d6b605248f75095dcf Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Thu, 22 Oct 2026 22:39:03 +0100 Subject: [PATCH 05/19] fix: check SAML LogoutRequest destination, freshness and replay; ECDSA format Addresses review on #939. pac4j 6.6's LogoutRequest validation checks signature (if present) and issuer only, so before a LogoutRequest may end sessions SamlSso now checks: - Destination: a signed request must be addressed to this SP's logout endpoint (SAML Bindings 3.4.5.2 / 3.5.5.2), and any Destination given must match. A request signed for another SP that shares NameIDs can no longer end Shepherd sessions. - Freshness: IssueInstant within 5 minutes (plus 5 minutes clock skew), and NotOnOrAfter not passed. - Replay: request IDs (per issuer) are recorded atomically in a bounded cache that outlives the acceptance window, so a captured request can't be replayed to end a later login. Also: XML-DSig ecdsa-sha* redirect signatures are raw r || s (IEEE P1363), not DER; verify them with the SHA*withECDSAinP1363Format JCA algorithms so EC-signing IdPs work. Regression tests: wrong / missing Destination, stale, future-dated and expired requests, replay after a fresh login, and an EC IdP's ecdsa-sha256 redirect signature. All seven fail against the previous SamlSso. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlSso.java | 88 +++++++++++++++++++++-- src/test/java/utils/SamlLogoutTest.java | 92 ++++++++++++++++++++++++ src/test/java/utils/SamlTestSupport.java | 29 ++++++-- 3 files changed, 197 insertions(+), 12 deletions(-) diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index 5b3d061a8..3c565021b 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -18,6 +18,8 @@ import java.security.cert.X509Certificate; import java.security.spec.InvalidKeySpecException; import java.security.spec.PKCS8EncodedKeySpec; +import java.time.Duration; +import java.time.Instant; import java.util.ArrayList; import java.util.Base64; import java.util.HashMap; @@ -27,6 +29,7 @@ import java.util.Properties; import java.util.Set; import java.util.UUID; +import java.util.concurrent.ConcurrentHashMap; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.bouncycastle.asn1.DERNull; @@ -119,7 +122,10 @@ public class SamlSso { private static final String HTTP_POST_BINDING = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"; private static final String SP_KEY_ALIAS = "shepherd-sp"; - /** HTTP-Redirect binding signature algorithms accepted on logout messages (no SHA-1) */ + /** + * HTTP-Redirect binding signature algorithms accepted on logout messages (no SHA-1). XML-DSig + * ECDSA signatures are the raw r || s concatenation (IEEE P1363), not ASN.1 DER. + */ private static final Map REDIRECT_SIGNATURE_ALGORITHMS = new HashMap<>(); static { @@ -130,16 +136,32 @@ public class SamlSso { REDIRECT_SIGNATURE_ALGORITHMS.put( "http://www.w3.org/2001/04/xmldsig-more#rsa-sha512", "SHA512withRSA"); REDIRECT_SIGNATURE_ALGORITHMS.put( - "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256", "SHA256withECDSA"); + "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256", "SHA256withECDSAinP1363Format"); REDIRECT_SIGNATURE_ALGORITHMS.put( - "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha384", "SHA384withECDSA"); + "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha384", "SHA384withECDSAinP1363Format"); REDIRECT_SIGNATURE_ALGORITHMS.put( - "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512", "SHA512withECDSA"); + "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha512", "SHA512withECDSAinP1363Format"); } private static final Set REDIRECT_SIGNED_PARAMETERS = Set.of("SAMLRequest", "SAMLResponse", "RelayState", "SigAlg", "Signature"); + /** How long after its IssueInstant an IdP LogoutRequest is accepted (plus clock skew) */ + static final Duration LOGOUT_REQUEST_LIFETIME = Duration.ofMinutes(5); + + /** Allowed clock difference between the IdP and this server */ + static final Duration CLOCK_SKEW = Duration.ofMinutes(5); + + /** Upper bound on remembered LogoutRequest IDs; beyond it new requests are refused */ + private static final int MAX_REMEMBERED_LOGOUT_REQUESTS = 10000; + + /** + * IDs of LogoutRequests already acted on, with when they can be forgotten. An entry outlives the + * window in which its request would be accepted, so a replay is always caught here or by the age + * check. + */ + private static final Map seenLogoutRequests = new ConcurrentHashMap<>(); + /** The configured SAML client and the settings SamlSso enforces itself */ static final class Sso { final SAML2Client client; @@ -148,10 +170,18 @@ static final class Sso { /** want_messages_signed: logout messages must carry a valid IdP signature */ final boolean requireSignedLogout; - Sso(SAML2Client client, X509Certificate idpCertificate, boolean requireSignedLogout) { + /** This SP's single logout endpoint, which IdP LogoutRequests must be addressed to */ + final String singleLogoutUrl; + + Sso( + SAML2Client client, + X509Certificate idpCertificate, + boolean requireSignedLogout, + String singleLogoutUrl) { this.client = client; this.idpCertificate = idpCertificate; this.requireSignedLogout = requireSignedLogout; + this.singleLogoutUrl = singleLogoutUrl; } } @@ -303,6 +333,12 @@ public static LogoutMessage processLogoutMessage( return LogoutMessage.LOGOUT_RESPONSE; } LogoutRequest logoutRequest = (LogoutRequest) message; + // pac4j checks none of these for LogoutRequests + String rejection = checkLogoutRequest(logoutRequest, signed, config.singleLogoutUrl); + if (rejection != null) { + log.error("SAML LogoutRequest rejected: " + rejection); + return LogoutMessage.INVALID; + } NameID nameId = logoutRequest.getNameID(); if (nameId == null && logoutRequest.getEncryptedID() != null) { nameId = (NameID) saml.getDecrypter().decrypt(logoutRequest.getEncryptedID()); @@ -336,6 +372,44 @@ public static LogoutMessage processLogoutMessage( } } + /** + * Checks an IdP LogoutRequest's addressing, freshness and uniqueness before it may end sessions, + * and records its ID. A signed request must name this SP's logout endpoint as its Destination + * (SAML Bindings 3.4.5.2 / 3.5.5.2); any Destination given must match. + * + * @return why the request is rejected, or null if it may be acted on + */ + static String checkLogoutRequest(LogoutRequest request, boolean signed, String singleLogoutUrl) { + String destination = request.getDestination(); + if (destination == null ? signed : !destination.equals(singleLogoutUrl)) { + return "Destination " + destination + " is not " + singleLogoutUrl; + } + Instant now = Instant.now(); + Instant issued = request.getIssueInstant(); + if (issued == null + || issued.isAfter(now.plus(CLOCK_SKEW)) + || issued.isBefore(now.minus(LOGOUT_REQUEST_LIFETIME).minus(CLOCK_SKEW))) { + return "IssueInstant " + issued + " is outside the accepted window"; + } + Instant notOnOrAfter = request.getNotOnOrAfter(); + if (notOnOrAfter != null && !now.minus(CLOCK_SKEW).isBefore(notOnOrAfter)) { + return "expired at " + notOnOrAfter; + } + if (request.getID() == null) { + return "no ID"; + } + String issuer = request.getIssuer() == null ? "" : request.getIssuer().getValue(); + seenLogoutRequests.values().removeIf(forgetAt -> forgetAt.isBefore(now)); + if (seenLogoutRequests.size() >= MAX_REMEMBERED_LOGOUT_REQUESTS) { + return "too many recent logout requests to check for replay"; + } + Instant forgetAt = now.plus(LOGOUT_REQUEST_LIFETIME).plus(CLOCK_SKEW.multipliedBy(2)); + if (seenLogoutRequests.putIfAbsent(issuer + "|" + request.getID(), forgetAt) != null) { + return "replay of " + request.getID(); + } + return null; + } + /** * Verifies an HTTP-Redirect binding signature (SAML Bindings 3.4.4.1): the signature covers * SAMLRequest/SAMLResponse, RelayState and SigAlg exactly as they appear, URL-encoded, in the @@ -511,7 +585,9 @@ static Sso build(Properties props) { } catch (GeneralSecurityException | IllegalArgumentException e) { throw new IllegalStateException("Cannot load " + IDP_CERT + ": " + e.getMessage(), e); } - return new Sso(saml, idpCertificate, flag(props, WANT_MESSAGES_SIGNED, false)); + // pac4j advertises the ACS URL as the logout endpoint when no SLS URL is configured + String singleLogoutUrl = optional(props, SP_SLS_URL, required(props, SP_ACS_URL)); + return new Sso(saml, idpCertificate, flag(props, WANT_MESSAGES_SIGNED, false), singleLogoutUrl); } /** diff --git a/src/test/java/utils/SamlLogoutTest.java b/src/test/java/utils/SamlLogoutTest.java index 696cee825..27eb7d769 100644 --- a/src/test/java/utils/SamlLogoutTest.java +++ b/src/test/java/utils/SamlLogoutTest.java @@ -9,12 +9,15 @@ import java.security.KeyPair; import java.security.cert.X509Certificate; +import java.time.Duration; +import java.time.Instant; import java.util.Arrays; import java.util.Properties; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.opensaml.saml.saml2.core.LogoutRequest; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpSession; @@ -289,6 +292,95 @@ public void redirectSignatureOnPostRequest_IsNotAccepted() throws Exception { assertFalse(loggedIn.isInvalid()); } + private static String signedRedirect(LogoutRequest request) throws Exception { + return SamlTestSupport.redirectQuery( + "SAMLRequest", SamlTestSupport.toXml(request, null, null), saml.idpKeys); + } + + @Test + public void signedLogoutRequest_ForAnotherSp_EndsNothing() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setDestination("https://other-sp.test/saml/sls"); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + + @Test + public void signedLogoutRequest_WithoutDestination_EndsNothing() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setDestination(null); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + + @Test + public void staleLogoutRequest_EndsNothing() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setIssueInstant(Instant.now().minus(Duration.ofMinutes(20))); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + + @Test + public void futureDatedLogoutRequest_EndsNothing() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setIssueInstant(Instant.now().plus(Duration.ofMinutes(20))); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + + @Test + public void expiredLogoutRequest_EndsNothing() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setNotOnOrAfter(Instant.now().minus(Duration.ofMinutes(10))); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + + @Test + public void replayedLogoutRequest_DoesNotEndALaterLogin() throws Exception { + String query = signedRedirect(saml.logoutRequest("user-123", (String) null)); + assertEquals(SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST, process(SamlTestSupport.slsGet(query))); + assertTrue(loggedIn.isInvalid()); + // The user logs in again + MockHttpSession newLogin = new MockHttpSession(); + SamlSessionRegistry.register(newLogin, "user-123", "_session-new"); + + assertEquals(SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(query))); + assertFalse(newLogin.isInvalid()); + newLogin.invalidate(); + } + + /** XML-DSig ecdsa-sha256 signatures are raw r || s, not the DER that SHA256withECDSA expects */ + @Test + public void ecdsaSignedRedirectLogoutResponse_FromEcIdp_IsAccepted() throws Exception { + KeyPair ecKeys = SamlTestSupport.newEcKeyPair(); + Properties props = signedLogoutProperties(true); + props.setProperty( + SamlSso.IDP_CERT, + SamlTestSupport.certificatePem(SamlTestSupport.selfSigned(ecKeys, "idp.test"))); + SamlTestSupport.install(props); + String query = + SamlTestSupport.redirectQuery( + "SAMLResponse", + logoutResponseXml(), + ecKeys, + "http://www.w3.org/2001/04/xmldsig-more#ecdsa-sha256", + "SHA256withECDSAinP1363Format"); + + assertEquals(SamlSso.LogoutMessage.LOGOUT_RESPONSE, process(SamlTestSupport.slsGet(query))); + } + @Test public void endedSession_IsRemovedFromRegistry() { int before = SamlSessionRegistry.size(); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index f8ed6c54a..fe7b60593 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -315,6 +315,14 @@ public static String toXml( */ public static String redirectQuery(String param, String xml, KeyPair signingKeys) throws Exception { + return redirectQuery( + param, xml, signingKeys, SignatureConstants.ALGO_ID_SIGNATURE_RSA_SHA256, "SHA256withRSA"); + } + + /** As {@link #redirectQuery(String, String, KeyPair)}, with a chosen signature algorithm */ + public static String redirectQuery( + String param, String xml, KeyPair signingKeys, String sigAlgUri, String jcaAlgorithm) + throws Exception { Deflater deflater = new Deflater(Deflater.DEFLATED, true); deflater.setInput(xml.getBytes(StandardCharsets.UTF_8)); deflater.finish(); @@ -331,11 +339,8 @@ public static String redirectQuery(String param, String xml, KeyPair signingKeys if (signingKeys == null) { return query; } - query += - "&SigAlg=" - + URLEncoder.encode( - SignatureConstants.ALGO_ID_SIGNATURE_RSA_SHA256, StandardCharsets.UTF_8); - java.security.Signature signer = java.security.Signature.getInstance("SHA256withRSA"); + query += "&SigAlg=" + URLEncoder.encode(sigAlgUri, StandardCharsets.UTF_8); + java.security.Signature signer = java.security.Signature.getInstance(jcaAlgorithm); signer.initSign(signingKeys.getPrivate()); signer.update(query.getBytes(StandardCharsets.UTF_8)); return query @@ -398,6 +403,13 @@ public static String decodeRedirectRequest(String location) throws Exception { throw new IllegalArgumentException("No SAMLRequest in " + location); } + /** An EC P-256 key pair, as used by IdPs that sign with ecdsa-sha256 */ + public static KeyPair newEcKeyPair() throws Exception { + KeyPairGenerator generator = KeyPairGenerator.getInstance("EC"); + generator.initialize(256); + return generator.generateKeyPair(); + } + public static KeyPair newKeyPair() throws Exception { KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); generator.initialize(2048); @@ -417,7 +429,12 @@ public static X509Certificate selfSigned(KeyPair keys, String commonName) throws keys.getPublic()); return new JcaX509CertificateConverter() .getCertificate( - builder.build(new JcaContentSignerBuilder("SHA256withRSA").build(keys.getPrivate()))); + builder.build( + new JcaContentSignerBuilder( + "EC".equals(keys.getPrivate().getAlgorithm()) + ? "SHA256withECDSA" + : "SHA256withRSA") + .build(keys.getPrivate()))); } public static String certificatePem(X509Certificate cert) throws Exception { From e22d1b039dd93b6d06bf98df2b3c1f9a944cead5 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Thu, 22 Oct 2026 22:53:22 +0100 Subject: [PATCH 06/19] fix: match complete SAML NameIDs, advertise verifiable SLO bindings, optional SP key Addresses review on #939. - Sessions are registered and ended by the complete NameID: value, Format, NameQualifier and SPNameQualifier, with omitted fields taking their SAML defaults (unspecified format; IdP / SP entity IDs as qualifiers, SAML Core 8.3.7). A NameID-only logout can no longer end sessions of a different principal that happens to share the value. - pac4j's SP metadata advertised HTTP-POST-SimpleSign and SOAP logout endpoints, which SamlSso can't verify signatures for; an IdP choosing them would have its signed logouts rejected. The metadata now lists only HTTP-POST and HTTP-Redirect (filtered before metadata signing). - Like java-saml, the SP certificate/key may be omitted when the SP signs nothing (authnrequest_signed, logoutrequest_signed and sign_metadata all false); a temporary in-memory key pair satisfies pac4j. Setting only one of the two, or omitting them while a signing option is on, fails with a message naming the settings. Regression tests for each; each fails with its fix switched off. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/servlets/ACS.java | 3 +- src/main/java/utils/SamlSessionRegistry.java | 5 +- src/main/java/utils/SamlSso.java | 193 ++++++++++++++++-- .../resources/onelogin.saml.properties.sample | 5 +- src/test/java/utils/SamlLogoutTest.java | 51 ++++- src/test/java/utils/SamlSsoTest.java | 50 +++++ 6 files changed, 284 insertions(+), 23 deletions(-) diff --git a/src/main/java/servlets/ACS.java b/src/main/java/servlets/ACS.java index 2dc3b47fe..1770f2bcf 100644 --- a/src/main/java/servlets/ACS.java +++ b/src/main/java/servlets/ACS.java @@ -15,7 +15,6 @@ import org.apache.logging.log4j.Logger; import org.pac4j.saml.profile.SAML2Profile; import utils.Hash; -import utils.SamlSessionRegistry; import utils.SamlSso; import utils.ShepherdLogManager; import utils.SsoUserMapper; @@ -102,7 +101,7 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) // Kept for single logout, which needs the NameID and session index ses.setAttribute(SamlSso.PROFILE_SESSION_ATTRIBUTE, profile.get()); // Lets an IdP-initiated logout end this session - SamlSessionRegistry.register(ses, profile.get().getId(), profile.get().getSessionIndex()); + SamlSso.registerLoginSession(ses, profile.get()); log.debug("userClassId = " + user[4]); ses.setAttribute("userClass", user[4]); diff --git a/src/main/java/utils/SamlSessionRegistry.java b/src/main/java/utils/SamlSessionRegistry.java index 5046f66ec..db8675a9f 100644 --- a/src/main/java/utils/SamlSessionRegistry.java +++ b/src/main/java/utils/SamlSessionRegistry.java @@ -68,7 +68,8 @@ public void valueUnbound(HttpSessionBindingEvent event) { * Records that a session belongs to a SAML login. * * @param session The post-login session - * @param nameId The NameID from the assertion + * @param nameId The complete NameID identity from the assertion (see + * SamlSso.registerLoginSession) * @param sessionIndex The SessionIndex from the assertion, if any */ public static void register(HttpSession session, String nameId, String sessionIndex) { @@ -80,7 +81,7 @@ public static void register(HttpSession session, String nameId, String sessionIn /** * Invalidates the sessions belonging to a SAML login. * - * @param nameId The NameID named in the logout request + * @param nameId The complete NameID identity named in the logout request * @param sessionIndex The SessionIndex named in the logout request, or null to end every session * for the NameID * @return How many sessions were invalidated diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index 3c565021b..b66ca46d8 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -2,14 +2,18 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpSession; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.math.BigInteger; import java.net.URLDecoder; import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.security.KeyFactory; +import java.security.KeyPair; +import java.security.KeyPairGenerator; import java.security.KeyStore; import java.security.PrivateKey; import java.security.Signature; @@ -22,6 +26,7 @@ import java.time.Instant; import java.util.ArrayList; import java.util.Base64; +import java.util.Date; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -36,10 +41,17 @@ import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers; import org.bouncycastle.asn1.pkcs.PrivateKeyInfo; import org.bouncycastle.asn1.pkcs.RSAPrivateKey; +import org.bouncycastle.asn1.x500.X500Name; import org.bouncycastle.asn1.x509.AlgorithmIdentifier; +import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter; +import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder; +import org.bouncycastle.operator.OperatorCreationException; +import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; import org.opensaml.saml.saml2.core.LogoutRequest; import org.opensaml.saml.saml2.core.NameID; +import org.opensaml.saml.saml2.core.NameIDType; import org.opensaml.saml.saml2.core.SessionIndex; +import org.opensaml.saml.saml2.metadata.SPSSODescriptor; import org.opensaml.xmlsec.encryption.support.DecryptionException; import org.owasp.encoder.Encode; import org.pac4j.core.context.CallContext; @@ -55,6 +67,7 @@ import org.pac4j.saml.config.SAML2Configuration; import org.pac4j.saml.context.SAML2MessageContext; import org.pac4j.saml.credentials.SAML2Credentials; +import org.pac4j.saml.metadata.SAML2FileSystemMetadataGenerator; import org.pac4j.saml.profile.SAML2Profile; import org.springframework.core.io.ByteArrayResource; @@ -173,15 +186,24 @@ static final class Sso { /** This SP's single logout endpoint, which IdP LogoutRequests must be addressed to */ final String singleLogoutUrl; + /** Defaults for omitted NameQualifier / SPNameQualifier (SAML Core 8.3.7) */ + final String idpEntityId; + + final String spEntityId; + Sso( SAML2Client client, X509Certificate idpCertificate, boolean requireSignedLogout, - String singleLogoutUrl) { + String singleLogoutUrl, + String idpEntityId, + String spEntityId) { this.client = client; this.idpCertificate = idpCertificate; this.requireSignedLogout = requireSignedLogout; this.singleLogoutUrl = singleLogoutUrl; + this.idpEntityId = idpEntityId; + this.spEntityId = spEntityId; } } @@ -278,6 +300,49 @@ public static boolean logout( return true; } + /** + * Records a post-login session against the SAML login that created it, so an IdP-initiated logout + * naming that login can end it. + * + * @param session The post-login session + * @param profile The profile from the validated assertion + */ + public static void registerLoginSession(HttpSession session, SAML2Profile profile) { + SamlSessionRegistry.register( + session, + nameIdKey( + get(), + profile.getId(), + profile.getSamlNameIdFormat(), + profile.getSamlNameIdNameQualifier(), + profile.getSamlNameIdSpNameQualifier()), + profile.getSessionIndex()); + } + + /** The complete identity of a NameID, as {@link SamlSessionRegistry} matches it */ + static String nameIdKey( + String value, String format, String nameQualifier, String spNameQualifier) { + return nameIdKey(get(), value, format, nameQualifier, spNameQualifier); + } + + /** + * Two NameIDs identify the same principal only if value, Format, NameQualifier and + * SPNameQualifier all match. Omitted fields take their SAML defaults: unspecified format, and the + * IdP / SP entity IDs as qualifiers (SAML Core 8.3.7). + */ + private static String nameIdKey( + Sso config, String value, String format, String nameQualifier, String spNameQualifier) { + if (value == null) { + return null; + } + return String.join( + "\u0000", + value, + format == null || format.isEmpty() ? NameIDType.UNSPECIFIED : format, + nameQualifier == null || nameQualifier.isEmpty() ? config.idpEntityId : nameQualifier, + spNameQualifier == null || spNameQualifier.isEmpty() ? config.spEntityId : spNameQualifier); + } + /** Outcome of a message received on the single logout service */ public enum LogoutMessage { /** @@ -343,7 +408,15 @@ public static LogoutMessage processLogoutMessage( if (nameId == null && logoutRequest.getEncryptedID() != null) { nameId = (NameID) saml.getDecrypter().decrypt(logoutRequest.getEncryptedID()); } - String nameIdValue = nameId == null ? null : nameId.getValue(); + String nameIdKey = + nameId == null + ? null + : nameIdKey( + config, + nameId.getValue(), + nameId.getFormat(), + nameId.getNameQualifier(), + nameId.getSPNameQualifier()); List sessionIndexes = new ArrayList<>(); for (SessionIndex index : logoutRequest.getSessionIndexes()) { if (index != null && index.getValue() != null) { @@ -353,10 +426,10 @@ public static LogoutMessage processLogoutMessage( int ended = 0; if (!sessionIndexes.isEmpty()) { for (String sessionIndex : sessionIndexes) { - ended += SamlSessionRegistry.invalidate(nameIdValue, sessionIndex); + ended += SamlSessionRegistry.invalidate(nameIdKey, sessionIndex); } } else if (signed) { - ended = SamlSessionRegistry.invalidate(nameIdValue, null); + ended = SamlSessionRegistry.invalidate(nameIdKey, null); } else { // A NameID alone is guessable, so only a signed request may end all of a user's sessions log.error("Unsigned SAML LogoutRequest without a SessionIndex; no sessions ended"); @@ -568,7 +641,8 @@ static Sso build(Properties props) { config.setSignatureAlgorithms(List.of(signatureAlgorithm)); } - configureKeystore(config, required(props, SP_CERT), required(props, SP_KEY)); + configureKeystore(config, spCredentials(props)); + config.setMetadataGenerator(new SupportedBindingsMetadataGenerator()); config.setIdentityProviderMetadataResource( new ByteArrayResource( buildIdentityProviderMetadata(props).getBytes(StandardCharsets.UTF_8), @@ -587,14 +661,108 @@ static Sso build(Properties props) { } // pac4j advertises the ACS URL as the logout endpoint when no SLS URL is configured String singleLogoutUrl = optional(props, SP_SLS_URL, required(props, SP_ACS_URL)); - return new Sso(saml, idpCertificate, flag(props, WANT_MESSAGES_SIGNED, false), singleLogoutUrl); + return new Sso( + saml, + idpCertificate, + flag(props, WANT_MESSAGES_SIGNED, false), + singleLogoutUrl, + required(props, IDP_ENTITY_ID), + required(props, SP_ENTITY_ID)); } /** - * Loads the SP's PEM certificate and private key into an in-memory keystore, so pac4j neither - * reads nor generates a keystore file. + * pac4j advertises HTTP-POST, HTTP-POST-SimpleSign, HTTP-Redirect and SOAP single logout + * endpoints. SamlSso can only verify signatures for HTTP-POST (XML signature) and HTTP-Redirect + * (query-string signature), so advertise just those; the descriptor is signed afterwards. */ - private static void configureKeystore(SAML2Configuration config, String certPem, String keyPem) { + private static final class SupportedBindingsMetadataGenerator + extends SAML2FileSystemMetadataGenerator { + SupportedBindingsMetadataGenerator() { + super(null); + } + + @Override + protected SPSSODescriptor buildSPSSODescriptor() { + SPSSODescriptor descriptor = super.buildSPSSODescriptor(); + descriptor + .getSingleLogoutServices() + .removeIf( + service -> + !HTTP_POST_BINDING.equals(service.getBinding()) + && !HTTP_REDIRECT_BINDING.equals(service.getBinding())); + return descriptor; + } + } + + /** + * The SP's signing/decryption key and certificate. Like java-saml, both settings may be left out + * when the SP signs nothing (authnrequest_signed, logoutrequest_signed and sign_metadata all + * false): pac4j still needs a key pair, so a temporary one is generated in memory. Encrypted + * assertions then can't be used, since the key changes on every restart. + */ + private static KeyStore.PrivateKeyEntry spCredentials(Properties props) { + String certPem = optional(props, SP_CERT); + String keyPem = optional(props, SP_KEY); + try { + if (certPem != null && keyPem != null) { + return new KeyStore.PrivateKeyEntry( + parsePrivateKey(keyPem), new Certificate[] {parseCertificate(certPem)}); + } + if (certPem != null || keyPem != null) { + throw new IllegalStateException("Set both " + SP_CERT + " and " + SP_KEY + ", or neither"); + } + List signingSettings = new ArrayList<>(); + for (String setting : List.of(AUTHN_REQUEST_SIGNED, LOGOUT_REQUEST_SIGNED, SIGN_METADATA)) { + if (flag(props, setting, false)) { + signingSettings.add(setting); + } + } + if (!signingSettings.isEmpty()) { + throw new IllegalStateException( + "Missing SAML setting " + + SP_CERT + + " / " + + SP_KEY + + ", required by " + + String.join(", ", signingSettings)); + } + log.info( + "No SP certificate configured: using a temporary in-memory key pair. SP signing and" + + " encrypted assertions are unavailable."); + return temporaryCredentials(); + } catch (GeneralSecurityException | IOException | OperatorCreationException e) { + throw new IllegalStateException( + "Cannot load " + SP_CERT + " / " + SP_KEY + ": " + e.getMessage(), e); + } + } + + private static KeyStore.PrivateKeyEntry temporaryCredentials() + throws GeneralSecurityException, OperatorCreationException { + KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); + generator.initialize(2048); + KeyPair keys = generator.generateKeyPair(); + X500Name name = new X500Name("CN=Security Shepherd SAML SP (temporary)"); + Instant now = Instant.now(); + X509Certificate cert = + new JcaX509CertificateConverter() + .getCertificate( + new JcaX509v3CertificateBuilder( + name, + BigInteger.valueOf(now.toEpochMilli()), + Date.from(now.minus(Duration.ofDays(1))), + Date.from(now.plus(Duration.ofDays(3650))), + name, + keys.getPublic()) + .build(new JcaContentSignerBuilder("SHA256withRSA").build(keys.getPrivate()))); + return new KeyStore.PrivateKeyEntry(keys.getPrivate(), new Certificate[] {cert}); + } + + /** + * Loads the SP's private key and certificate into an in-memory keystore, so pac4j neither reads + * nor generates a keystore file. + */ + private static void configureKeystore( + SAML2Configuration config, KeyStore.PrivateKeyEntry credentials) { String password = UUID.randomUUID().toString(); byte[] keystore; try { @@ -602,15 +770,14 @@ private static void configureKeystore(SAML2Configuration config, String certPem, ks.load(null, null); ks.setKeyEntry( SP_KEY_ALIAS, - parsePrivateKey(keyPem), + credentials.getPrivateKey(), password.toCharArray(), - new Certificate[] {parseCertificate(certPem)}); + credentials.getCertificateChain()); ByteArrayOutputStream out = new ByteArrayOutputStream(); ks.store(out, password.toCharArray()); keystore = out.toByteArray(); } catch (GeneralSecurityException | IOException e) { - throw new IllegalStateException( - "Cannot load " + SP_CERT + " / " + SP_KEY + ": " + e.getMessage(), e); + throw new IllegalStateException("Cannot build the SP keystore: " + e.getMessage(), e); } config.getKeystore().setKeystoreResource(new ByteArrayResource(keystore, "SP keystore")); diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index 000313fc0..3cf618e4d 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -43,8 +43,9 @@ onelogin.saml2.sp.single_logout_service.binding = urn:oasis:names:tc:SAML:2.0:bi # Take a look on lib/Saml2/Constants.php to see the NameIdFormat supported onelogin.saml2.sp.nameidformat = urn:oasis:names:tc:SAML:2.0:nameid-format:persistent -# Usually x509cert and privateKey of the SP are provided by files placed at -# the certs folder. But we can also provide them with the following parameters +# The SP's certificate and private key. Both may be left empty if the SP signs +# nothing (authnrequest_signed, logoutrequest_signed and sign_metadata all false): +# a temporary in-memory key is then used, and encrypted assertions can't be used. onelogin.saml2.sp.x509cert = diff --git a/src/test/java/utils/SamlLogoutTest.java b/src/test/java/utils/SamlLogoutTest.java index 27eb7d769..16d7b4bb5 100644 --- a/src/test/java/utils/SamlLogoutTest.java +++ b/src/test/java/utils/SamlLogoutTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.opensaml.saml.saml2.core.LogoutRequest; +import org.opensaml.saml.saml2.core.NameIDType; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.mock.web.MockHttpSession; @@ -44,9 +45,9 @@ public static void generateKeys() throws Exception { public void setUp() throws Exception { SamlTestSupport.install(signedLogoutProperties(true)); loggedIn = new MockHttpSession(); - SamlSessionRegistry.register(loggedIn, "user-123", "_session-1"); + register(loggedIn, "user-123", "_session-1"); otherUser = new MockHttpSession(); - SamlSessionRegistry.register(otherUser, "user-456", "_session-2"); + register(otherUser, "user-456", "_session-2"); } @AfterEach @@ -59,6 +60,12 @@ public void tearDown() { SamlTestSupport.uninstallClient(); } + /** Registers a session for a persistent NameID with default qualifiers, as ACS would */ + private static void register(MockHttpSession session, String nameId, String sessionIndex) { + SamlSessionRegistry.register( + session, SamlSso.nameIdKey(nameId, NameIDType.PERSISTENT, null, null), sessionIndex); + } + private static Properties signedLogoutProperties(boolean wantMessagesSigned) throws Exception { Properties props = saml.properties(); props.setProperty(SamlSso.WANT_MESSAGES_SIGNED, Boolean.toString(wantMessagesSigned)); @@ -242,7 +249,7 @@ public void unsignedIdpLogoutRequestWithoutSessionIndex_EndsNothing() throws Exc @Test public void idpLogoutRequest_EndsEverySessionIndexItNames() throws Exception { MockHttpSession secondLogin = new MockHttpSession(); - SamlSessionRegistry.register(secondLogin, "user-123", "_session-3"); + register(secondLogin, "user-123", "_session-3"); String xml = SamlTestSupport.toXml( saml.logoutRequest("user-123", Arrays.asList("_session-1", "_session-3")), null, null); @@ -354,7 +361,7 @@ public void replayedLogoutRequest_DoesNotEndALaterLogin() throws Exception { assertTrue(loggedIn.isInvalid()); // The user logs in again MockHttpSession newLogin = new MockHttpSession(); - SamlSessionRegistry.register(newLogin, "user-123", "_session-new"); + register(newLogin, "user-123", "_session-new"); assertEquals(SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(query))); assertFalse(newLogin.isInvalid()); @@ -381,6 +388,42 @@ public void ecdsaSignedRedirectLogoutResponse_FromEcIdp_IsAccepted() throws Exce assertEquals(SamlSso.LogoutMessage.LOGOUT_RESPONSE, process(SamlTestSupport.slsGet(query))); } + @Test + public void nameIdOnlyLogout_DoesNotEndSameValueWithDifferentFormatOrQualifier() + throws Exception { + MockHttpSession emailFormat = new MockHttpSession(); + SamlSessionRegistry.register( + emailFormat, SamlSso.nameIdKey("user-123", NameIDType.EMAIL, null, null), "_e"); + MockHttpSession otherIdp = new MockHttpSession(); + SamlSessionRegistry.register( + otherIdp, + SamlSso.nameIdKey("user-123", NameIDType.PERSISTENT, "https://other-idp.test", null), + "_o"); + + assertEquals( + SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST, + process( + SamlTestSupport.slsGet(signedRedirect(saml.logoutRequest("user-123", (String) null))))); + + assertTrue(loggedIn.isInvalid()); + assertFalse(emailFormat.isInvalid()); + assertFalse(otherIdp.isInvalid()); + emailFormat.invalidate(); + otherIdp.invalidate(); + } + + @Test + public void explicitDefaultQualifiers_MatchOmittedOnes() throws Exception { + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.getNameID().setNameQualifier(SamlTestSupport.IDP_ENTITY_ID); + request.getNameID().setSPNameQualifier(SamlTestSupport.SP_ENTITY_ID); + + assertEquals( + SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST, + process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertTrue(loggedIn.isInvalid()); + } + @Test public void endedSession_IsRemovedFromRegistry() { int before = SamlSessionRegistry.size(); diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 31189c8ef..9c620b08e 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -274,6 +274,56 @@ public void login_LegacyComparisonKey_IsRead() throws Exception { assertThat(authnRequest, containsString("Comparison=\"minimum\"")); } + @Test + public void getServiceProviderMetadata_AdvertisesOnlyVerifiableLogoutBindings() { + String metadata = SamlSso.getServiceProviderMetadata(); + + assertThat(metadata, containsString("bindings:HTTP-POST\"")); + assertThat(metadata, containsString("bindings:HTTP-Redirect\"")); + assertFalse(metadata.contains("HTTP-POST-SimpleSign")); + assertFalse(metadata.contains("bindings:SOAP")); + } + + @Test + public void build_WithoutSpKeyPair_AuthenticatesSignedAssertions() throws Exception { + Properties props = saml.properties(); + props.remove(SamlSso.SP_CERT); + props.remove(SamlSso.SP_KEY); + props.setProperty(SamlSso.AUTHN_REQUEST_SIGNED, "false"); + props.setProperty(SamlSso.LOGOUT_REQUEST_SIGNED, "false"); + SamlTestSupport.install(props); + String samlResponse = + saml.signedResponse("user-123", studentAttributes(), saml.idpKeys, saml.idpCert); + + Optional profile = + SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()); + + assertTrue(profile.isPresent()); + assertEquals("user-123", profile.get().getId()); + } + + @Test + public void build_WithoutSpKeyPair_WhenSigningRequired_NamesTheSettings() throws Exception { + Properties props = saml.properties(); + props.remove(SamlSso.SP_CERT); + props.remove(SamlSso.SP_KEY); + + IllegalStateException e = assertThrows(IllegalStateException.class, () -> SamlSso.build(props)); + + assertThat(e.getMessage(), containsString(SamlSso.SP_CERT)); + assertThat(e.getMessage(), containsString(SamlSso.AUTHN_REQUEST_SIGNED)); + } + + @Test + public void build_OnlyOneOfSpCertAndKey_IsRejected() throws Exception { + Properties props = saml.properties(); + props.remove(SamlSso.SP_KEY); + + IllegalStateException e = assertThrows(IllegalStateException.class, () -> SamlSso.build(props)); + + assertThat(e.getMessage(), containsString("or neither")); + } + @Test public void processLogoutMessage_NoMessage_IsInvalid() { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/sls"); From 998b2664f839f210638a90ea7c59869e851178ce Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Thu, 22 Oct 2026 23:03:24 +0100 Subject: [PATCH 07/19] fix: treat requested SAML authentication contexts as alternatives Addresses review on #939. pac4j 6.6 rejects a response unless every requested AuthnContextClassRef appears in it, but SAML treats the requested classes as alternatives (SAML Core 3.3.2.2.1). With two classes configured, a valid signed response carrying one of them was rejected and ACS reported "Not authenticated". SamlSso now uses a SAML2Client whose response validator requires, for "exact" comparison (the default), at least one returned class to be one of those requested. For minimum / better / maximum the IdP judges relative strength, which an SP can't check in general (a stronger class may be one that wasn't listed), so the returned class is accepted, as java-saml did. Tests: one-of-several accepted, unrequested class rejected under exact, stronger unlisted class accepted under minimum, any class accepted when none is requested. The two acceptance cases fail with pac4j's validator. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlSso.java | 49 ++++++++++++++++++++- src/test/java/utils/SamlSsoTest.java | 56 ++++++++++++++++++++++++ src/test/java/utils/SamlTestSupport.java | 5 ++- 3 files changed, 108 insertions(+), 2 deletions(-) diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index b66ca46d8..f56ff88b6 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -67,8 +67,10 @@ import org.pac4j.saml.config.SAML2Configuration; import org.pac4j.saml.context.SAML2MessageContext; import org.pac4j.saml.credentials.SAML2Credentials; +import org.pac4j.saml.exceptions.SAMLAuthnContextClassRefException; import org.pac4j.saml.metadata.SAML2FileSystemMetadataGenerator; import org.pac4j.saml.profile.SAML2Profile; +import org.pac4j.saml.sso.impl.SAML2AuthnResponseValidator; import org.springframework.core.io.ByteArrayResource; /** @@ -648,7 +650,7 @@ static Sso build(Properties props) { buildIdentityProviderMetadata(props).getBytes(StandardCharsets.UTF_8), "identity provider metadata from " + PROPERTIES_FILE)); - SAML2Client saml = new SAML2Client(config); + SAML2Client saml = new AlternativeAuthnContextClient(config); // The ACS URL is registered with the IdP as-is, so don't append ?client_name=... saml.setCallbackUrlResolver(new NoParameterCallbackUrlResolver()); saml.setCallbackUrl(required(props, SP_ACS_URL)); @@ -670,6 +672,51 @@ static Sso build(Properties props) { required(props, SP_ENTITY_ID)); } + /** + * pac4j requires every requested AuthnContextClassRef to appear in the response, but SAML treats + * the requested classes as alternatives (SAML Core 3.3.2.2.1). With "exact" comparison, at least + * one returned class must be one of those requested. With minimum / better / maximum the IdP + * judges relative strength, which an SP can't check in general (a stronger class may be one that + * wasn't listed), so the returned class is accepted, as java-saml did. + */ + private static final class AlternativeAuthnContextClient extends SAML2Client { + AlternativeAuthnContextClient(SAML2Configuration configuration) { + super(configuration); + } + + @Override + protected void initSAMLResponseValidator(boolean forceReinit) { + if (authnResponseValidator == null || forceReinit) { + String comparison = configuration.getComparisonType(); + authnResponseValidator = + new SAML2AuthnResponseValidator( + signatureTrustEngineProvider, + decrypter, + replayCache, + configuration, + findSessionLogoutHandler()) { + @Override + protected void validateAuthnContextClassRefs( + SAML2MessageContext context, List provided) { + List requested = configuration.getAuthnContextClassRefs(); + if (requested.isEmpty() + || (comparison != null && !"exact".equalsIgnoreCase(comparison))) { + return; + } + if (provided.stream().noneMatch(requested::contains)) { + throw new SAMLAuthnContextClassRefException( + "None of the requested authentication context classes " + + requested + + " is in the IdP's authentication statements " + + provided); + } + } + }; + authnResponseValidator.setAcceptedSkew(configuration.getAcceptedSkew()); + } + } + } + /** * pac4j advertises HTTP-POST, HTTP-POST-SimpleSign, HTTP-Redirect and SOAP single logout * endpoints. SamlSso can only verify signatures for HTTP-POST (XML signature) and HTTP-Redirect diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 9c620b08e..bc45e30cc 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -23,6 +23,7 @@ import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.opensaml.saml.saml2.core.AuthnContext; import org.pac4j.saml.profile.SAML2Profile; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -324,6 +325,61 @@ public void build_OnlyOneOfSpCertAndKey_IsRejected() throws Exception { assertThat(e.getMessage(), containsString("or neither")); } + private Optional authenticateWithRequestedContexts( + String requested, String comparison, String returned) throws Exception { + Properties props = saml.properties(); + props.setProperty(SamlSso.REQUESTED_AUTHN_CONTEXT, requested); + if (comparison != null) { + props.setProperty(SamlSso.REQUESTED_AUTHN_CONTEXT_COMPARISON, comparison); + } + SamlTestSupport.install(props); + saml.authnContextClassRef = returned; + try { + String samlResponse = + saml.signedResponse("user-123", studentAttributes(), saml.idpKeys, saml.idpCert); + return SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()); + } finally { + saml.authnContextClassRef = AuthnContext.PPT_AUTHN_CTX; + } + } + + @Test + public void authenticate_OneOfSeveralRequestedContexts_IsAccepted() throws Exception { + assertTrue( + authenticateWithRequestedContexts( + AuthnContext.PPT_AUTHN_CTX + ", " + AuthnContext.X509_AUTHN_CTX, + null, + AuthnContext.X509_AUTHN_CTX) + .isPresent()); + } + + @Test + public void authenticate_ExactComparison_UnrequestedContext_IsRejected() throws Exception { + assertFalse( + authenticateWithRequestedContexts( + AuthnContext.PPT_AUTHN_CTX + ", " + AuthnContext.X509_AUTHN_CTX, + "exact", + AuthnContext.KERBEROS_AUTHN_CTX) + .isPresent()); + } + + @Test + public void authenticate_MinimumComparison_StrongerUnlistedContext_IsAccepted() throws Exception { + assertTrue( + authenticateWithRequestedContexts( + AuthnContext.PPT_AUTHN_CTX, "minimum", AuthnContext.X509_AUTHN_CTX) + .isPresent()); + } + + @Test + public void authenticate_NoRequestedContext_AcceptsAnyContext() throws Exception { + String samlResponse = + saml.signedResponse("user-123", studentAttributes(), saml.idpKeys, saml.idpCert); + + assertTrue( + SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()).isPresent()); + } + @Test public void processLogoutMessage_NoMessage_IsInvalid() { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/sls"); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index fe7b60593..d50d0b7cb 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -72,6 +72,9 @@ public class SamlTestSupport { public static final String IDP_SSO_URL = "https://idp.test/sso"; public static final String IDP_SLO_URL = "https://idp.test/slo"; + /** The AuthnContextClassRef that {@link #signedResponse} reports */ + public String authnContextClassRef = AuthnContext.PPT_AUTHN_CTX; + public final KeyPair spKeys; public final X509Certificate spCert; public final KeyPair idpKeys; @@ -202,7 +205,7 @@ public String signedResponse( authn.setSessionIndex("_session-" + nameId); AuthnContext authnContext = build(AuthnContext.DEFAULT_ELEMENT_NAME); AuthnContextClassRef classRef = build(AuthnContextClassRef.DEFAULT_ELEMENT_NAME); - classRef.setURI(AuthnContext.PPT_AUTHN_CTX); + classRef.setURI(authnContextClassRef); authnContext.setAuthnContextClassRef(classRef); authn.setAuthnContext(authnContext); assertion.getAuthnStatements().add(authn); From f589e478bf37e078b67b8bfe0f3249cd43923953 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Thu, 22 Oct 2026 23:12:54 +0100 Subject: [PATCH 08/19] fix: advertise no SAML logout endpoint when no SLS URL is configured Addresses review on #939. With onelogin.saml2.sp.single_logout_service.url empty, pac4j advertised the ACS URL as the single logout endpoint, but ACS doesn't process logout messages, so an IdP following the metadata would send LogoutRequests that left sessions active. The SP metadata now lists no SingleLogoutService in that case, and SamlSso refuses IdP LogoutRequests rather than handling them against the ACS URL. The sample config documents leaving the setting empty to disable IdP-initiated logout. Tests: metadata without the SLS setting has no logout endpoint, and an IdP LogoutRequest is refused without ending sessions. Both fail under the previous behaviour. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlSso.java | 30 ++++++++++++++----- .../resources/onelogin.saml.properties.sample | 3 +- src/test/java/utils/SamlLogoutTest.java | 13 ++++++++ src/test/java/utils/SamlSsoTest.java | 13 ++++++++ 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index f56ff88b6..476a14f72 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -185,7 +185,10 @@ static final class Sso { /** want_messages_signed: logout messages must carry a valid IdP signature */ final boolean requireSignedLogout; - /** This SP's single logout endpoint, which IdP LogoutRequests must be addressed to */ + /** + * This SP's single logout endpoint, which IdP LogoutRequests must be addressed to; null when + * single logout isn't configured + */ final String singleLogoutUrl; /** Defaults for omitted NameQualifier / SPNameQualifier (SAML Core 8.3.7) */ @@ -455,6 +458,9 @@ public static LogoutMessage processLogoutMessage( * @return why the request is rejected, or null if it may be acted on */ static String checkLogoutRequest(LogoutRequest request, boolean signed, String singleLogoutUrl) { + if (singleLogoutUrl == null) { + return "no single logout endpoint is configured (" + SP_SLS_URL + ")"; + } String destination = request.getDestination(); if (destination == null ? signed : !destination.equals(singleLogoutUrl)) { return "Destination " + destination + " is not " + singleLogoutUrl; @@ -644,7 +650,8 @@ static Sso build(Properties props) { } configureKeystore(config, spCredentials(props)); - config.setMetadataGenerator(new SupportedBindingsMetadataGenerator()); + config.setMetadataGenerator( + new SupportedBindingsMetadataGenerator(optional(props, SP_SLS_URL) != null)); config.setIdentityProviderMetadataResource( new ByteArrayResource( buildIdentityProviderMetadata(props).getBytes(StandardCharsets.UTF_8), @@ -661,8 +668,9 @@ static Sso build(Properties props) { } catch (GeneralSecurityException | IllegalArgumentException e) { throw new IllegalStateException("Cannot load " + IDP_CERT + ": " + e.getMessage(), e); } - // pac4j advertises the ACS URL as the logout endpoint when no SLS URL is configured - String singleLogoutUrl = optional(props, SP_SLS_URL, required(props, SP_ACS_URL)); + // Null when no SLS URL is configured: no logout endpoint is advertised, and IdP + // LogoutRequests are refused + String singleLogoutUrl = optional(props, SP_SLS_URL); return new Sso( saml, idpCertificate, @@ -720,12 +728,17 @@ protected void validateAuthnContextClassRefs( /** * pac4j advertises HTTP-POST, HTTP-POST-SimpleSign, HTTP-Redirect and SOAP single logout * endpoints. SamlSso can only verify signatures for HTTP-POST (XML signature) and HTTP-Redirect - * (query-string signature), so advertise just those; the descriptor is signed afterwards. + * (query-string signature), so advertise just those. With no SLS URL configured pac4j would + * advertise the ACS URL, which doesn't process logout messages, so advertise none. The descriptor + * is signed afterwards. */ private static final class SupportedBindingsMetadataGenerator extends SAML2FileSystemMetadataGenerator { - SupportedBindingsMetadataGenerator() { + private final boolean advertiseSingleLogout; + + SupportedBindingsMetadataGenerator(boolean advertiseSingleLogout) { super(null); + this.advertiseSingleLogout = advertiseSingleLogout; } @Override @@ -735,8 +748,9 @@ protected SPSSODescriptor buildSPSSODescriptor() { .getSingleLogoutServices() .removeIf( service -> - !HTTP_POST_BINDING.equals(service.getBinding()) - && !HTTP_REDIRECT_BINDING.equals(service.getBinding())); + !advertiseSingleLogout + || (!HTTP_POST_BINDING.equals(service.getBinding()) + && !HTTP_REDIRECT_BINDING.equals(service.getBinding()))); return descriptor; } } diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index 3cf618e4d..18ff6856e 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -30,7 +30,8 @@ onelogin.saml2.sp.assertion_consumer_service.url = https://localhost/saml/acs onelogin.saml2.sp.assertion_consumer_service.binding = urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST # Specifies info about where and how the message MUST be -# returned to the requester, in this case our SP. +# returned to the requester, in this case our SP. Leave empty to disable +# IdP-initiated single logout: no logout endpoint is then advertised. onelogin.saml2.sp.single_logout_service.url = https://localhost/saml/sls # SAML protocol binding to be used when returning the or sending the diff --git a/src/test/java/utils/SamlLogoutTest.java b/src/test/java/utils/SamlLogoutTest.java index 16d7b4bb5..ee4a5e144 100644 --- a/src/test/java/utils/SamlLogoutTest.java +++ b/src/test/java/utils/SamlLogoutTest.java @@ -424,6 +424,19 @@ public void explicitDefaultQualifiers_MatchOmittedOnes() throws Exception { assertTrue(loggedIn.isInvalid()); } + @Test + public void withoutSlsUrl_IdpLogoutRequestIsRefusedAndEndsNothing() throws Exception { + Properties props = signedLogoutProperties(true); + props.remove(SamlSso.SP_SLS_URL); + SamlTestSupport.install(props); + LogoutRequest request = saml.logoutRequest("user-123", "_session-1"); + request.setDestination(SamlTestSupport.ACS_URL); + + assertEquals( + SamlSso.LogoutMessage.INVALID, process(SamlTestSupport.slsGet(signedRedirect(request)))); + assertFalse(loggedIn.isInvalid()); + } + @Test public void endedSession_IsRemovedFromRegistry() { int before = SamlSessionRegistry.size(); diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index bc45e30cc..c0b4d1d02 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -380,6 +380,19 @@ public void authenticate_NoRequestedContext_AcceptsAnyContext() throws Exception SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()).isPresent()); } + @Test + public void getServiceProviderMetadata_WithoutSlsUrl_AdvertisesNoLogoutEndpoint() + throws Exception { + Properties props = saml.properties(); + props.remove(SamlSso.SP_SLS_URL); + SamlTestSupport.install(props); + + String metadata = SamlSso.getServiceProviderMetadata(); + + assertFalse(metadata.contains("SingleLogoutService")); + assertThat(metadata, containsString("Location=\"" + SamlTestSupport.ACS_URL + "\"")); + } + @Test public void processLogoutMessage_NoMessage_IsInvalid() { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/sls"); From 01002903921a32e74ffce86976327ef030232481 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 07:58:34 +0100 Subject: [PATCH 09/19] build: restrict and PGP-verify the Shibboleth repository (OpenSAML) pac4j-saml depends on OpenSAML 5, which Shibboleth publishes only to its own repository (build.shibboleth.net), not Maven Central. That repository was trusted implicitly via pac4j-saml's POM, for any artifact, with checksum mismatches only warned about, and nothing checked signatures. - pom.xml declares shibboleth-releases with checksumPolicy=fail. - Maven remote repository filtering (.mvn/maven.config, .mvn/rrf/groupId-shibboleth-releases.txt) lets that repository serve only org.opensaml and net.shibboleth; everything else comes from Central only. - pgpverify-maven-plugin checks every dependency's PGP signature on every build (disableChecksum=true, so a jar tampered with in the local repository after a successful build is still caught). .mvn/pgp-keys.map pins org.opensaml and net.shibboleth to the Shibboleth release key (B5B5DD332142AD657E8D87AC7D27E610B8A3DC52, as published at shibboleth.net/downloads/PGP_KEYS); other groupIds are listed by name and need a valid signature. There is deliberately no wildcard rule: any matching rule lets an artifact pass, so a catch-all would undo the pin (caught by the test below). The one unsigned legacy artifact, hamcrest-core 1.1 via json-simple (#944), is allowed by exact version. - src/test/scripts/pgp-verification-test.sh, run in the CI build job: genuine artifacts pass; OpenSAML pinned to a different key fails ("Not allowed artifact org.opensaml..."); a tampered opensaml-core-api jar fails ("PGP Signature INVALID"). - CI's change filter now includes .mvn/**. Refs #861 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/test.yml | 3 + .mvn/maven.config | 2 + .mvn/pgp-keys.map | 74 +++++++++++++++++++++++ .mvn/rrf/groupId-shibboleth-releases.txt | 2 + pom.xml | 41 +++++++++++++ src/test/scripts/pgp-verification-test.sh | 44 ++++++++++++++ 6 files changed, 166 insertions(+) create mode 100644 .mvn/maven.config create mode 100644 .mvn/pgp-keys.map create mode 100644 .mvn/rrf/groupId-shibboleth-releases.txt create mode 100755 src/test/scripts/pgp-verification-test.sh diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5fbcc85cd..077b4e16f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -35,6 +35,7 @@ jobs: src: - 'src/**' - 'pom.xml' + - '.mvn/**' - 'docker-compose.yml' - 'docker/**' - '.env' @@ -75,6 +76,8 @@ jobs: cache: maven - name: Build Maven with Docker Profile run: mvn clean install -Pdocker -DskipTests -B + - name: Check PGP verification rejects tampered and wrongly-signed OpenSAML + run: bash src/test/scripts/pgp-verification-test.sh - name: Docker Compose Build run: docker compose build diff --git a/.mvn/maven.config b/.mvn/maven.config new file mode 100644 index 000000000..eb0f85399 --- /dev/null +++ b/.mvn/maven.config @@ -0,0 +1,2 @@ +-Daether.remoteRepositoryFilter.groupId=true +-Daether.remoteRepositoryFilter.groupId.basedir=${maven.multiModuleProjectDirectory}/.mvn/rrf diff --git a/.mvn/pgp-keys.map b/.mvn/pgp-keys.map new file mode 100644 index 000000000..ef7086dec --- /dev/null +++ b/.mvn/pgp-keys.map @@ -0,0 +1,74 @@ +# PGP keys map for pgpverify-maven-plugin (see pom.xml). Every dependency must match a rule here. +# +# OpenSAML / Shibboleth come from Shibboleth's own repository (not Maven Central), so they are +# pinned to the Shibboleth release key: Philip David Smart (Jisc), fingerprint +# B5B5 DD33 2142 AD65 7E8D 87AC 7D27 E610 B8A3 DC52, as published at +# https://shibboleth.net/downloads/PGP_KEYS. Never add a wildcard that could also match these +# groups: any matching rule is enough for an artifact to pass, which would undo the pin. +org.opensaml = 0xB5B5DD332142AD657E8D87AC7D27E610B8A3DC52 +net.shibboleth = 0xB5B5DD332142AD657E8D87AC7D27E610B8A3DC52 + +# Everything else comes from Maven Central only (.mvn/rrf), and must carry a valid signature by +# any key. A new dependency from an unlisted groupId fails the build ("Not allowed artifact"): +# review it, then add its groupId here. +com.fasterxml.jackson.core = any +com.fasterxml.jackson.datatype = any +com.github.ben-manes.caffeine = any +com.github.waffle = any +com.google.code.findbugs = any +com.google.errorprone = any +com.google.guava = any +com.google.j2objc = any +com.google.protobuf = any +com.googlecode.json-simple = any +com.mysql = any +com.zaxxer = any +commons-codec = any +commons-io = any +commons-logging = any +de.mkammerer = any +io.dropwizard.metrics = any +io.github.cdimascio = any +io.micrometer = any +jakarta.activation = any +jakarta.el = any +jakarta.mail = any +jakarta.servlet.jsp.jstl = any +junit = any +net.bytebuddy = any +net.java.dev.jna = any +net.sf.jtidy = any +org.apache.commons = any +org.apache.httpcomponents.client5 = any +org.apache.httpcomponents.core5 = any +org.apache.logging.log4j = any +org.apache.santuario = any +org.apache.velocity = any +org.apiguardian = any +org.bouncycastle = any +org.checkerframework = any +org.cryptacular = any +org.eclipse.angus = any +org.glassfish = any +org.glassfish.web = any +org.hamcrest = any +org.jetbrains = any +org.jetbrains.kotlin = any +org.json = any +org.jsoup = any +org.jspecify = any +org.junit.jupiter = any +org.junit.platform = any +org.mariadb.jdbc = any +org.mockito = any +org.mongodb = any +org.objenesis = any +org.opentest4j = any +org.owasp.encoder = any +org.pac4j = any +org.slf4j = any +org.springframework = any + +# Unsigned legacy artifact pulled in via json-simple -> junit 4.10 (to be removed, see #944). +# Exact version only. +org.hamcrest:hamcrest-core:1.1 = noSig diff --git a/.mvn/rrf/groupId-shibboleth-releases.txt b/.mvn/rrf/groupId-shibboleth-releases.txt new file mode 100644 index 000000000..5f1727b3a --- /dev/null +++ b/.mvn/rrf/groupId-shibboleth-releases.txt @@ -0,0 +1,2 @@ +net.shibboleth +org.opensaml diff --git a/pom.xml b/pom.xml index fb22efceb..8a16c0220 100644 --- a/pom.xml +++ b/pom.xml @@ -18,8 +18,28 @@ servlet mocks. --> 7.0.10 6.6.0 + + ${project.basedir}/.mvn/pgp-keys.map + + + + shibboleth-releases + Shibboleth Releases Repository + https://build.shibboleth.net/maven/releases/ + + true + fail + + + false + + + + + true + hkps://keys.openpgp.org,hkps://keyserver.ubuntu.com + + org.codehaus.mojo properties-maven-plugin diff --git a/src/test/scripts/pgp-verification-test.sh b/src/test/scripts/pgp-verification-test.sh new file mode 100755 index 000000000..0ab9b0848 --- /dev/null +++ b/src/test/scripts/pgp-verification-test.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# Checks that the build's PGP verification (pgpverify-maven-plugin, .mvn/pgp-keys.map) +# rejects a tampered OpenSAML artifact and an OpenSAML artifact signed by a key other than +# the pinned Shibboleth release key. Uses a private local repository, so it never touches +# ~/.m2. Run from the project root: bash src/test/scripts/pgp-verification-test.sh +set -euo pipefail + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT +repo="$work/repository" +mvn_validate() { + mvn -B validate -Dmaven.repo.local="$repo" "$@" >"$work/build.log" 2>&1 +} +fail() { + echo "FAIL: $1" + tail -40 "$work/build.log" + exit 1 +} + +echo "1. Untampered dependencies verify" +mvn_validate || fail "verification failed on genuine artifacts" + +jar=$(find "$repo/org/opensaml/opensaml-core-api" -name 'opensaml-core-api-*.jar' | head -1) +[ -n "$jar" ] || fail "opensaml-core-api jar not found in $repo" + +echo "2. OpenSAML pinned to a different key is rejected" +sed 's/^org\.opensaml = .*/org.opensaml = 0x0000000000000000000000000000000000000001/' \ + .mvn/pgp-keys.map >"$work/wrong-key.map" +if mvn_validate -o -Dpgp.keysMap="$work/wrong-key.map"; then + fail "build passed although OpenSAML is not signed by the pinned key" +fi +grep -E "^\[ERROR\] Not allowed artifact org\.opensaml" "$work/build.log" | head -1 || + fail "failure was not the OpenSAML key check" + +echo "3. A tampered OpenSAML jar is rejected" +cp "$jar" "$work/original.jar" +printf 'tampered' >>"$jar" +if mvn_validate -o; then + fail "build passed with a tampered $(basename "$jar")" +fi +grep -E "^\[ERROR\].*opensaml-core-api" "$work/build.log" | head -1 || + fail "failure was not the opensaml-core-api signature check" + +echo "PASS: PGP verification rejects tampered and wrongly-signed OpenSAML artifacts" From ab0b672ad5e43c467fe991584875cae0b4fae9ce Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 08:09:45 +0100 Subject: [PATCH 10/19] build: require Maven 3.9.2+ so the Shibboleth repository filter applies Addresses review on #881. Maven before 3.9.2 does not expand ${maven.multiModuleProjectDirectory} in .mvn/maven.config, so the remote-repository filter's allow-list is never found and the filter fails open. Checked against real releases: 3.9.0 and 3.9.1 fetched OpenSAML from Shibboleth with org.opensaml removed from the allow-list; 3.9.2, 3.9.4, 3.9.8, 3.9.16 and 3.10.0 refused it. A relative path is resolved against the local repository and fails open on every version. - maven-enforcer-plugin (first plugin in the build) requires [3.9.2,), so older Maven stops with an explicit message. - pgp-verification-test.sh gains step 0: with org.opensaml removed from the allow-list, resolution must fail and nothing may be downloaded from Shibboleth for org.opensaml. Run in CI, this proves the filter is enforced on the runner's own Maven. Refs #861 Co-Authored-By: Claude Opus 5.5 --- pom.xml | 24 +++++++++++++++++++++ src/test/scripts/pgp-verification-test.sh | 26 ++++++++++++++++++++--- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 8a16c0220..9339704d2 100644 --- a/pom.xml +++ b/pom.xml @@ -445,6 +445,30 @@ + + + org.apache.maven.plugins + maven-enforcer-plugin + 3.6.3 + + + require-maven-for-repository-filter + + enforce + + + + + + [3.9.2,) + Maven 3.9.2 or newer is required: older versions ignore the Shibboleth repository restriction in .mvn/maven.config. + + + + + + org.simplify4u.plugins pgpverify-maven-plugin diff --git a/src/test/scripts/pgp-verification-test.sh b/src/test/scripts/pgp-verification-test.sh index 0ab9b0848..2fe6b9fc7 100755 --- a/src/test/scripts/pgp-verification-test.sh +++ b/src/test/scripts/pgp-verification-test.sh @@ -1,7 +1,8 @@ #!/usr/bin/env bash -# Checks that the build's PGP verification (pgpverify-maven-plugin, .mvn/pgp-keys.map) -# rejects a tampered OpenSAML artifact and an OpenSAML artifact signed by a key other than -# the pinned Shibboleth release key. Uses a private local repository, so it never touches +# Checks that the Shibboleth repository filter (.mvn/maven.config) is enforced, and that the +# build's PGP verification (pgpverify-maven-plugin, .mvn/pgp-keys.map) rejects a tampered +# OpenSAML artifact and an OpenSAML artifact signed by a key other than the pinned Shibboleth +# release key. Uses a private local repository, so it never touches # ~/.m2. Run from the project root: bash src/test/scripts/pgp-verification-test.sh set -euo pipefail @@ -17,6 +18,25 @@ fail() { exit 1 } +echo "0. The Shibboleth repository may serve only allow-listed groupIds" +# With org.opensaml dropped from the allow-list, OpenSAML must not be fetched from Shibboleth's +# repository (and is not on Central), so resolution has to fail. If it succeeds, the repository +# filter in .mvn/maven.config is not being applied by this Maven. +allowlist=.mvn/rrf/groupId-shibboleth-releases.txt +cp "$allowlist" "$work/allowlist.orig" +grep -v '^org\.opensaml$' "$work/allowlist.orig" >"$allowlist" +restore_allowlist() { cp "$work/allowlist.orig" "$allowlist"; } +trap 'restore_allowlist; rm -rf "$work"' EXIT +if mvn_validate -Dpgpverify.skip -Dmaven.repo.local="$work/filter-repo" dependency:resolve; then + fail "OpenSAML was fetched although org.opensaml is not allow-listed: repository filter not applied" +fi +if grep -q "Downloaded from shibboleth-releases.*org/opensaml" "$work/build.log"; then + fail "OpenSAML was downloaded from Shibboleth although not allow-listed" +fi +grep -E "org\.opensaml:opensaml-core-api:jar:[^ ]* (is missing|was not found)|Could not find artifact org\.opensaml" \ + "$work/build.log" | head -1 || fail "resolution failed for an unrelated reason" +restore_allowlist + echo "1. Untampered dependencies verify" mvn_validate || fail "verification failed on genuine artifacts" From c0bb9fe050b460d818c3c8b6033b68ecbd6b00c8 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 08:23:37 +0100 Subject: [PATCH 11/19] fix: keep no-op logouts out of the replay cache; omit temporary SP keys Addresses review on #939. - With want_messages_signed=false, an unsigned LogoutRequest that ends nothing (NameID only, or a SessionIndex we don't track) was still recorded in the bounded replay cache, so a flood of them could fill it and make genuine signed logouts be refused. Replay recording now happens only for requests that will act: signed requests, or unsigned ones naming a tracked login (SamlSessionRegistry.isRegistered). The destination/freshness checks still apply to every request. - With no SP certificate configured, the temporary in-memory key was still published in SP metadata; an IdP encrypting to it would break after a restart. Metadata now has no KeyDescriptor in that case; configured certificates are still published. Tests: a flood of unsigned no-op logouts against a small cache no longer blocks a signed logout; an unsigned logout naming a tracked login still ends it; keyless metadata publishes no key, configured metadata does. The flood and keyless cases fail with their fixes switched off. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlSessionRegistry.java | 19 ++++++ src/main/java/utils/SamlSso.java | 71 +++++++++++++++----- src/test/java/utils/SamlLogoutTest.java | 33 +++++++++ src/test/java/utils/SamlSsoTest.java | 20 ++++++ 4 files changed, 127 insertions(+), 16 deletions(-) diff --git a/src/main/java/utils/SamlSessionRegistry.java b/src/main/java/utils/SamlSessionRegistry.java index db8675a9f..1c84ee00b 100644 --- a/src/main/java/utils/SamlSessionRegistry.java +++ b/src/main/java/utils/SamlSessionRegistry.java @@ -108,6 +108,25 @@ public static int invalidate(String nameId, String sessionIndex) { return invalidated; } + /** + * Whether a session is tracked for this login. + * + * @param nameId The complete NameID identity + * @param sessionIndex The SessionIndex + * @return true if a live session was registered for exactly this NameID and SessionIndex + */ + public static boolean isRegistered(String nameId, String sessionIndex) { + if (nameId == null || sessionIndex == null) { + return false; + } + for (Entry entry : sessions.values()) { + if (nameId.equals(entry.nameId) && sessionIndex.equals(entry.sessionIndex)) { + return true; + } + } + return false; + } + /** Number of tracked sessions; for tests */ static int size() { return sessions.size(); diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index 476a14f72..4181a81cd 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -168,7 +168,7 @@ public class SamlSso { static final Duration CLOCK_SKEW = Duration.ofMinutes(5); /** Upper bound on remembered LogoutRequest IDs; beyond it new requests are refused */ - private static final int MAX_REMEMBERED_LOGOUT_REQUESTS = 10000; + static int maxRememberedLogoutRequests = 10000; /** * IDs of LogoutRequests already acted on, with when they can be forgotten. An entry outlives the @@ -428,16 +428,30 @@ public static LogoutMessage processLogoutMessage( sessionIndexes.add(index.getValue()); } } + // Only a request that will end something takes a slot in the replay cache: a signed request, + // or an unsigned one naming a login we track. Otherwise unauthenticated no-op requests could + // fill the cache and lock out genuine signed logouts. (A NameID alone is guessable, so only + // a signed request may end all of a user's sessions.) + boolean acts = + signed + || sessionIndexes.stream() + .anyMatch(index -> SamlSessionRegistry.isRegistered(nameIdKey, index)); int ended = 0; - if (!sessionIndexes.isEmpty()) { - for (String sessionIndex : sessionIndexes) { - ended += SamlSessionRegistry.invalidate(nameIdKey, sessionIndex); + if (acts) { + String replay = recordLogoutRequest(logoutRequest); + if (replay != null) { + log.error("SAML LogoutRequest rejected: " + replay); + return LogoutMessage.INVALID; + } + if (!sessionIndexes.isEmpty()) { + for (String sessionIndex : sessionIndexes) { + ended += SamlSessionRegistry.invalidate(nameIdKey, sessionIndex); + } + } else { + ended = SamlSessionRegistry.invalidate(nameIdKey, null); } - } else if (signed) { - ended = SamlSessionRegistry.invalidate(nameIdKey, null); } else { - // A NameID alone is guessable, so only a signed request may end all of a user's sessions - log.error("Unsigned SAML LogoutRequest without a SessionIndex; no sessions ended"); + log.error("Unsigned SAML LogoutRequest names no tracked login; no sessions ended"); } log.debug("IdP-initiated SAML logout ended " + ended + " session(s)"); HttpAction action = saml.processLogout(ctx, credentials.get()); @@ -451,9 +465,10 @@ public static LogoutMessage processLogoutMessage( } /** - * Checks an IdP LogoutRequest's addressing, freshness and uniqueness before it may end sessions, - * and records its ID. A signed request must name this SP's logout endpoint as its Destination - * (SAML Bindings 3.4.5.2 / 3.5.5.2); any Destination given must match. + * Checks an IdP LogoutRequest's addressing and freshness before it may end sessions. A signed + * request must name this SP's logout endpoint as its Destination (SAML Bindings 3.4.5.2 / + * 3.5.5.2); any Destination given must match. Replay is checked by {@link #recordLogoutRequest}, + * only for requests that will act. * * @return why the request is rejected, or null if it may be acted on */ @@ -479,9 +494,20 @@ static String checkLogoutRequest(LogoutRequest request, boolean signed, String s if (request.getID() == null) { return "no ID"; } + return null; + } + + /** + * Records a LogoutRequest's ID (per issuer) before it ends sessions, rejecting a replay. Fails + * closed when the cache is full. + * + * @return why the request is rejected, or null if it was recorded + */ + static String recordLogoutRequest(LogoutRequest request) { + Instant now = Instant.now(); String issuer = request.getIssuer() == null ? "" : request.getIssuer().getValue(); seenLogoutRequests.values().removeIf(forgetAt -> forgetAt.isBefore(now)); - if (seenLogoutRequests.size() >= MAX_REMEMBERED_LOGOUT_REQUESTS) { + if (seenLogoutRequests.size() >= maxRememberedLogoutRequests) { return "too many recent logout requests to check for replay"; } Instant forgetAt = now.plus(LOGOUT_REQUEST_LIFETIME).plus(CLOCK_SKEW.multipliedBy(2)); @@ -491,6 +517,11 @@ static String checkLogoutRequest(LogoutRequest request, boolean signed, String s return null; } + /** Empties the replay cache; for tests. */ + static void clearRememberedLogoutRequests() { + seenLogoutRequests.clear(); + } + /** * Verifies an HTTP-Redirect binding signature (SAML Bindings 3.4.4.1): the signature covers * SAMLRequest/SAMLResponse, RelayState and SigAlg exactly as they appear, URL-encoded, in the @@ -651,7 +682,8 @@ static Sso build(Properties props) { configureKeystore(config, spCredentials(props)); config.setMetadataGenerator( - new SupportedBindingsMetadataGenerator(optional(props, SP_SLS_URL) != null)); + new SupportedBindingsMetadataGenerator( + optional(props, SP_SLS_URL) != null, optional(props, SP_CERT) != null)); config.setIdentityProviderMetadataResource( new ByteArrayResource( buildIdentityProviderMetadata(props).getBytes(StandardCharsets.UTF_8), @@ -729,16 +761,19 @@ protected void validateAuthnContextClassRefs( * pac4j advertises HTTP-POST, HTTP-POST-SimpleSign, HTTP-Redirect and SOAP single logout * endpoints. SamlSso can only verify signatures for HTTP-POST (XML signature) and HTTP-Redirect * (query-string signature), so advertise just those. With no SLS URL configured pac4j would - * advertise the ACS URL, which doesn't process logout messages, so advertise none. The descriptor - * is signed afterwards. + * advertise the ACS URL, which doesn't process logout messages, so advertise none. Without a + * configured SP certificate the key is temporary, so publish no KeyDescriptor. The descriptor is + * signed afterwards. */ private static final class SupportedBindingsMetadataGenerator extends SAML2FileSystemMetadataGenerator { private final boolean advertiseSingleLogout; + private final boolean publishKeys; - SupportedBindingsMetadataGenerator(boolean advertiseSingleLogout) { + SupportedBindingsMetadataGenerator(boolean advertiseSingleLogout, boolean publishKeys) { super(null); this.advertiseSingleLogout = advertiseSingleLogout; + this.publishKeys = publishKeys; } @Override @@ -751,6 +786,10 @@ protected SPSSODescriptor buildSPSSODescriptor() { !advertiseSingleLogout || (!HTTP_POST_BINDING.equals(service.getBinding()) && !HTTP_REDIRECT_BINDING.equals(service.getBinding()))); + if (!publishKeys) { + // A temporary key changes on every restart, so an IdP must not sign-verify or encrypt to it + descriptor.getKeyDescriptors().clear(); + } return descriptor; } } diff --git a/src/test/java/utils/SamlLogoutTest.java b/src/test/java/utils/SamlLogoutTest.java index ee4a5e144..224cbf86d 100644 --- a/src/test/java/utils/SamlLogoutTest.java +++ b/src/test/java/utils/SamlLogoutTest.java @@ -43,6 +43,7 @@ public static void generateKeys() throws Exception { /** Installs a client that requires signed logout messages, as the sample configuration does */ @BeforeEach public void setUp() throws Exception { + SamlSso.clearRememberedLogoutRequests(); SamlTestSupport.install(signedLogoutProperties(true)); loggedIn = new MockHttpSession(); register(loggedIn, "user-123", "_session-1"); @@ -58,6 +59,8 @@ public void tearDown() { } } SamlTestSupport.uninstallClient(); + SamlSso.maxRememberedLogoutRequests = 10000; + SamlSso.clearRememberedLogoutRequests(); } /** Registers a session for a persistent NameID with default qualifiers, as ACS would */ @@ -437,6 +440,36 @@ public void withoutSlsUrl_IdpLogoutRequestIsRefusedAndEndsNothing() throws Excep assertFalse(loggedIn.isInvalid()); } + /** + * Unsigned requests that end nothing must not take replay-cache slots; otherwise flooding them + * would make genuine signed logouts be refused as "too many". + */ + @Test + public void floodOfUnsignedNoOpLogouts_DoesNotBlockSignedLogout() throws Exception { + SamlSso.maxRememberedLogoutRequests = 5; + SamlTestSupport.install(signedLogoutProperties(false)); + for (int i = 0; i < 10; i++) { + process(SamlTestSupport.slsGet(logoutRequestQuery("user-" + i, null, null))); + } + + MockHttpServletRequest signed = + SamlTestSupport.slsGet(logoutRequestQuery("user-123", "_session-1", saml.idpKeys)); + + assertEquals(SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST, process(signed)); + assertTrue(loggedIn.isInvalid()); + } + + @Test + public void unsignedLogoutNamingATrackedLogin_StillEndsIt() throws Exception { + SamlTestSupport.install(signedLogoutProperties(false)); + + assertEquals( + SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST, + process(SamlTestSupport.slsGet(logoutRequestQuery("user-123", "_session-1", null)))); + assertTrue(loggedIn.isInvalid()); + assertFalse(otherUser.isInvalid()); + } + @Test public void endedSession_IsRemovedFromRegistry() { int before = SamlSessionRegistry.size(); diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index c0b4d1d02..6a0c491d8 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -303,6 +303,26 @@ public void build_WithoutSpKeyPair_AuthenticatesSignedAssertions() throws Except assertEquals("user-123", profile.get().getId()); } + @Test + public void getServiceProviderMetadata_WithoutSpKeyPair_PublishesNoKey() throws Exception { + Properties props = saml.properties(); + props.remove(SamlSso.SP_CERT); + props.remove(SamlSso.SP_KEY); + props.setProperty(SamlSso.AUTHN_REQUEST_SIGNED, "false"); + props.setProperty(SamlSso.LOGOUT_REQUEST_SIGNED, "false"); + SamlTestSupport.install(props); + + String metadata = SamlSso.getServiceProviderMetadata(); + + assertFalse(metadata.contains("KeyDescriptor")); + assertFalse(metadata.contains("X509Certificate")); + } + + @Test + public void getServiceProviderMetadata_WithSpKeyPair_PublishesIt() { + assertThat(SamlSso.getServiceProviderMetadata(), containsString("KeyDescriptor")); + } + @Test public void build_WithoutSpKeyPair_WhenSigningRequired_NamesTheSettings() throws Exception { Properties props = saml.properties(); From 3fdd95c6614e2cbc09ac52926e8e64be40117b5a Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 08:32:45 +0100 Subject: [PATCH 12/19] fix: SLS must not log users out by redirecting to login.jsp Addresses review on #939. login.jsp invalidates the current session when no login/registration message is pending, so SLS redirecting there after an empty or rejected logout message let any page log a user out by sending their browser to /saml/sls. A LogoutResponse delivered through a different, still-logged-in browser did the same. - Invalid or missing logout message: 400 Bad Request, no redirect. - LogoutResponse: redirect to index.jsp if the browser's session is still logged in, else login.jsp. Tests (SLSTest): GET/POST without a message return 400 with no redirect; an invalid message keeps the session and does not redirect; a LogoutResponse keeps a logged-in session (index.jsp) and sends a logged-out browser to login.jsp. Four of the five fail against the previous SLS. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/servlets/SLS.java | 18 ++++++-- src/test/java/servlets/SLSTest.java | 72 +++++++++++++++++++++++------ 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/src/main/java/servlets/SLS.java b/src/main/java/servlets/SLS.java index 27037de09..19189be84 100644 --- a/src/main/java/servlets/SLS.java +++ b/src/main/java/servlets/SLS.java @@ -4,6 +4,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import jakarta.servlet.http.HttpSession; import java.io.IOException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -55,7 +56,8 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) * *

Sessions are ended by SamlSso for the login an IdP LogoutRequest names, not by whichever * session the delivering browser happens to have. A LogoutResponse ends nothing: Logout already - * ended the local session before sending the user to the IdP. + * ended the local session before sending the user to the IdP. An invalid message gets a 400, + * never a redirect to login.jsp (which would end the browser's session). */ public void handleRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { @@ -69,10 +71,18 @@ public void handleRequest(HttpServletRequest request, HttpServletResponse respon SamlSso.LogoutMessage message = SamlSso.processLogoutMessage(request, response); log.debug("SAML logout message: " + message); - // For an IdP LogoutRequest the LogoutResponse back to the IdP has already been written - if (message != SamlSso.LogoutMessage.IDP_LOGOUT_REQUEST) { - response.sendRedirect("../login.jsp"); + if (message == SamlSso.LogoutMessage.INVALID) { + // No redirect: login.jsp invalidates the current session, so redirecting there would let + // any page log a user out by sending their browser here with a bogus message + response.sendError(HttpServletResponse.SC_BAD_REQUEST); + } else if (message == SamlSso.LogoutMessage.LOGOUT_RESPONSE) { + // Logout already ended this browser's session; if it still has one (e.g. the response was + // delivered to a different browser), don't end it via login.jsp + HttpSession ses = request.getSession(false); + boolean loggedIn = ses != null && ses.getAttribute("userRole") != null; + response.sendRedirect(loggedIn ? "../index.jsp" : "../login.jsp"); } + // For an IdP LogoutRequest the LogoutResponse back to the IdP has already been written log.debug("*** END SLS ***"); } } diff --git a/src/test/java/servlets/SLSTest.java b/src/test/java/servlets/SLSTest.java index e99bacd0f..f1c9e2a41 100644 --- a/src/test/java/servlets/SLSTest.java +++ b/src/test/java/servlets/SLSTest.java @@ -1,7 +1,8 @@ package servlets; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -13,9 +14,12 @@ public class SLSTest { + private static SamlTestSupport saml; + @BeforeAll public static void installClient() throws Exception { - new SamlTestSupport().installClient(); + saml = new SamlTestSupport(); + saml.installClient(); } @AfterAll @@ -23,36 +27,78 @@ public static void uninstallClient() { SamlTestSupport.uninstallClient(); } + private static MockHttpSession loggedInSession() { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("userName", "jdoe"); + session.setAttribute("userRole", "player"); + return session; + } + + private static MockHttpServletRequest logoutResponse() throws Exception { + return SamlTestSupport.slsGet( + SamlTestSupport.redirectQuery( + "SAMLResponse", SamlTestSupport.toXml(saml.logoutResponse(), null, null), null)); + } + /** The SLS used to only define handleRequest, so the IdP's redirect got a 405 */ @Test - public void doGet_IsHandled() throws Exception { + public void doGet_WithoutSamlMessage_IsBadRequest() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); new SLS().doGet(new MockHttpServletRequest("GET", "/saml/sls"), response); - assertEquals(302, response.getStatus()); - assertEquals("../login.jsp", response.getRedirectedUrl()); + assertEquals(400, response.getStatus()); + assertNull(response.getRedirectedUrl()); } @Test - public void doPost_IsHandled() throws Exception { + public void doPost_WithoutSamlMessage_IsBadRequest() throws Exception { MockHttpServletResponse response = new MockHttpServletResponse(); new SLS().doPost(new MockHttpServletRequest("POST", "/saml/sls"), response); - assertEquals(302, response.getStatus()); - assertEquals("../login.jsp", response.getRedirectedUrl()); + assertEquals(400, response.getStatus()); + assertNull(response.getRedirectedUrl()); } + /** + * A bogus message must not end the session, directly or by redirecting to login.jsp, which + * invalidates the session it is loaded in + */ @Test - public void invalidLogoutMessage_KeepsSession() throws Exception { + public void invalidLogoutMessage_KeepsSessionAndDoesNotRedirect() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/sls"); - MockHttpSession session = new MockHttpSession(); - session.setAttribute("userName", "jdoe"); + MockHttpSession session = loggedInSession(); + request.setSession(session); + MockHttpServletResponse response = new MockHttpServletResponse(); + + new SLS().doGet(request, response); + + assertFalse(session.isInvalid()); + assertEquals(400, response.getStatus()); + assertNull(response.getRedirectedUrl()); + } + + @Test + public void logoutResponse_WithoutSession_GoesToLogin() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + + new SLS().doGet(logoutResponse(), response); + + assertEquals("../login.jsp", response.getRedirectedUrl()); + } + + /** A LogoutResponse delivered to a still-logged-in browser must not end that session */ + @Test + public void logoutResponse_InLoggedInBrowser_KeepsSession() throws Exception { + MockHttpServletRequest request = logoutResponse(); + MockHttpSession session = loggedInSession(); request.setSession(session); + MockHttpServletResponse response = new MockHttpServletResponse(); - new SLS().doGet(request, new MockHttpServletResponse()); + new SLS().doGet(request, response); - assertNotNull(session.getAttribute("userName")); + assertFalse(session.isInvalid()); + assertEquals("../index.jsp", response.getRedirectedUrl()); } } From 2a6d4b386c7be74f9e586b57678ff50acbb1ccba Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 17:32:27 +0100 Subject: [PATCH 13/19] fix: bind SAML logins to the browser, gate SSO on sso.enabled, reject blank IDs Addresses review on #939. Login CSRF (P1): pac4j's default message store keeps nothing and it accepts responses without InResponseTo, so nothing tied a login response to the browser that started the login. A valid response for an attacker's account could be posted through a victim's browser and sign it into that account. - SamlRequestStore records each sent AuthnRequest bound to a random token in a dedicated cookie (HttpOnly, Secure, SameSite=None so it survives the IdP's cross-site POST, scoped to the ACS path; the session cookie is untouched). pac4j only accepts a Response whose InResponseTo names a pending request bound to the delivering browser; each request is used once and expires after 10 minutes. - Unsolicited (IdP-initiated) responses, which carry no InResponseTo and can't be bound to a browser, are rejected. Also: - ACS, SSOLogin, SLS and SSOMetadata answer 404 unless SSO is enabled (LoginMethod.isSaml, as Login already checks), so a retained SAML configuration can't authenticate anyone while sso.enabled is off. - SsoUserMapper treats blank (empty / whitespace-only) identifiers and display names as missing, so distinct principals can't share an account through a blank ssoName; non-blank values are kept as sent. Tests: the fixture now runs a real SP-initiated login (startLogin) and answers it, so the existing authentication tests exercise the full round trip; no assertion was weakened. New: unsolicited response rejected; response delivered through another browser (no cookie, or its own) rejected; response answering another browser's login rejected; all four SSO endpoints 404 with SSO disabled, and ACS doesn't authenticate; blank ssoName/userName rejected, non-blank kept as sent. Each fix fails its tests when switched off. 251 unit and 312 integration tests pass. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/servlets/ACS.java | 7 + src/main/java/servlets/SLS.java | 7 + src/main/java/servlets/SSOLogin.java | 7 + src/main/java/servlets/SSOMetadata.java | 7 + src/main/java/utils/LoginMethod.java | 6 + src/main/java/utils/SamlRequestStore.java | 169 ++++++++++++++++++++ src/main/java/utils/SamlSso.java | 42 ++++- src/main/java/utils/SsoUserMapper.java | 18 ++- src/test/java/servlets/SsoDisabledTest.java | 80 +++++++++ src/test/java/utils/SamlSsoTest.java | 49 ++++++ src/test/java/utils/SamlTestSupport.java | 56 +++++++ src/test/java/utils/SsoUserMapperTest.java | 29 ++++ 12 files changed, 467 insertions(+), 10 deletions(-) create mode 100644 src/main/java/utils/SamlRequestStore.java create mode 100644 src/test/java/servlets/SsoDisabledTest.java diff --git a/src/main/java/servlets/ACS.java b/src/main/java/servlets/ACS.java index 1770f2bcf..9a8d7237a 100644 --- a/src/main/java/servlets/ACS.java +++ b/src/main/java/servlets/ACS.java @@ -15,6 +15,7 @@ import org.apache.logging.log4j.Logger; import org.pac4j.saml.profile.SAML2Profile; import utils.Hash; +import utils.LoginMethod; import utils.SamlSso; import utils.ShepherdLogManager; import utils.SsoUserMapper; @@ -45,6 +46,12 @@ public class ACS extends HttpServlet { public void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + if (!LoginMethod.isSaml()) { + // SSO is off (sso.enabled in sso.properties): this endpoint must not authenticate anyone, + // even if a SAML configuration is present + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } ShepherdLogManager.setRequestIp(request.getRemoteAddr(), request.getHeader("X-Forwarded-For")); log.debug("**** servlets.ACS ***"); diff --git a/src/main/java/servlets/SLS.java b/src/main/java/servlets/SLS.java index 19189be84..601396eae 100644 --- a/src/main/java/servlets/SLS.java +++ b/src/main/java/servlets/SLS.java @@ -8,6 +8,7 @@ import java.io.IOException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import utils.LoginMethod; import utils.SamlSso; import utils.ShepherdLogManager; @@ -61,6 +62,12 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) */ public void handleRequest(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + if (!LoginMethod.isSaml()) { + // SSO is off (sso.enabled in sso.properties): this endpoint must not authenticate anyone, + // even if a SAML configuration is present + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } // Setting IpAddress To Log and taking header for original IP if forwarded from // proxy ShepherdLogManager.setRequestIp(request.getRemoteAddr(), request.getHeader("X-Forwarded-For")); diff --git a/src/main/java/servlets/SSOLogin.java b/src/main/java/servlets/SSOLogin.java index b07e8ec1a..232658650 100644 --- a/src/main/java/servlets/SSOLogin.java +++ b/src/main/java/servlets/SSOLogin.java @@ -7,6 +7,7 @@ import java.io.IOException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import utils.LoginMethod; import utils.SamlSso; import utils.ShepherdLogManager; @@ -42,6 +43,12 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + if (!LoginMethod.isSaml()) { + // SSO is off (sso.enabled in sso.properties): this endpoint must not authenticate anyone, + // even if a SAML configuration is present + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } ShepherdLogManager.setRequestIp(request.getRemoteAddr(), request.getHeader("X-Forwarded-For")); log.debug("**** servlets.SSOLogin ***"); request.setCharacterEncoding("UTF-8"); diff --git a/src/main/java/servlets/SSOMetadata.java b/src/main/java/servlets/SSOMetadata.java index 6e510d46f..4db1a786f 100644 --- a/src/main/java/servlets/SSOMetadata.java +++ b/src/main/java/servlets/SSOMetadata.java @@ -7,6 +7,7 @@ import java.io.IOException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import utils.LoginMethod; import utils.SamlSso; import utils.ShepherdLogManager; @@ -42,6 +43,12 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + if (!LoginMethod.isSaml()) { + // SSO is off (sso.enabled in sso.properties): this endpoint must not authenticate anyone, + // even if a SAML configuration is present + response.sendError(HttpServletResponse.SC_NOT_FOUND); + return; + } ShepherdLogManager.setRequestIp(request.getRemoteAddr(), request.getHeader("X-Forwarded-For")); try { diff --git a/src/main/java/utils/LoginMethod.java b/src/main/java/utils/LoginMethod.java index 833df8dfb..9ef1a3965 100644 --- a/src/main/java/utils/LoginMethod.java +++ b/src/main/java/utils/LoginMethod.java @@ -73,6 +73,12 @@ public static boolean isSaml() { } } + /** Overrides sso.properties; for tests. Null goes back to reading the file. */ + static void setSaml(Boolean saml) { + isSet = saml != null; + isSaml = saml != null && saml; + } + public static Boolean isLogin() { return (!isSaml()); } diff --git a/src/main/java/utils/SamlRequestStore.java b/src/main/java/utils/SamlRequestStore.java new file mode 100644 index 000000000..6abcee70e --- /dev/null +++ b/src/main/java/utils/SamlRequestStore.java @@ -0,0 +1,169 @@ +package utils; + +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.net.URI; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.security.SecureRandom; +import java.time.Duration; +import java.time.Instant; +import java.util.Base64; +import java.util.Comparator; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import org.opensaml.core.xml.XMLObject; +import org.pac4j.core.context.WebContext; +import org.pac4j.core.context.session.SessionStore; +import org.pac4j.jee.context.JEEContext; +import org.pac4j.saml.store.SAMLMessageStore; +import org.pac4j.saml.store.SAMLMessageStoreFactory; + +/** + * Remembers the SAML requests this SP sends, each bound to the browser that started it, so pac4j + * only accepts a Response whose InResponseTo names a request started in the same browser. This + * stops login CSRF: a valid response for one account can't be replayed through another browser's + * assertion consumer service.
+ * The binding is a random token in a dedicated cookie ({@value #COOKIE}), HttpOnly, Secure and + * SameSite=None so it survives the IdP's cross-site POST back, and scoped to the ACS path only. The + * session cookie's policy is left alone.
+ * This file is part of the Security Shepherd Project. + * + *

The Security Shepherd project is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software Foundation, either + * version 3 of the License, or (at your option) any later version.
+ * + *

The Security Shepherd project is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR + * PURPOSE. See the GNU General Public License for more details.
+ * + *

You should have received a copy of the GNU General Public License along with the Security + * Shepherd project. If not, see . + */ +public class SamlRequestStore implements SAMLMessageStoreFactory { + + static final String COOKIE = "shepherd_saml_request"; + + /** How long a started login may take at the IdP */ + static final Duration LIFETIME = Duration.ofMinutes(10); + + /** Upper bound on pending requests; the oldest are dropped beyond it */ + static int maxPending = 20000; + + private static final SecureRandom random = new SecureRandom(); + + private static final class Pending { + final String token; + final XMLObject message; + final Instant expires; + + Pending(String token, XMLObject message, Instant expires) { + this.token = token; + this.message = message; + this.expires = expires; + } + } + + private final Map pending = new ConcurrentHashMap<>(); + private final String cookiePath; + + /** + * @param assertionConsumerServiceUrl The ACS URL, whose path scopes the binding cookie + */ + public SamlRequestStore(String assertionConsumerServiceUrl) { + String path = URI.create(assertionConsumerServiceUrl).getPath(); + this.cookiePath = path == null || path.isEmpty() ? "/" : path; + } + + @Override + public SAMLMessageStore getMessageStore(WebContext context, SessionStore sessionStore) { + JEEContext jee = (JEEContext) context; + return new BoundStore(jee.getNativeRequest(), jee.getNativeResponse()); + } + + /** Forgets a request once its response has been accepted, so each request is used once */ + void forget(String requestId) { + if (requestId != null) { + pending.remove(requestId); + } + } + + private final class BoundStore implements SAMLMessageStore { + private final HttpServletRequest request; + private final HttpServletResponse response; + + BoundStore(HttpServletRequest request, HttpServletResponse response) { + this.request = request; + this.response = response; + } + + @Override + public void set(String id, XMLObject message) { + Instant now = Instant.now(); + pending.values().removeIf(p -> p.expires.isBefore(now)); + while (pending.size() >= maxPending) { + pending.entrySet().stream() + .min(Comparator.comparing(e -> e.getValue().expires)) + .ifPresent(oldest -> pending.remove(oldest.getKey())); + } + pending.put(id, new Pending(bindingToken(), message, now.plus(LIFETIME))); + } + + @Override + public Optional get(String id) { + Pending entry = id == null ? null : pending.get(id); + String token = cookieToken(); + if (entry == null || token == null || entry.expires.isBefore(Instant.now())) { + return Optional.empty(); + } + boolean sameBrowser = + MessageDigest.isEqual( + entry.token.getBytes(StandardCharsets.US_ASCII), + token.getBytes(StandardCharsets.US_ASCII)); + return sameBrowser ? Optional.of(entry.message) : Optional.empty(); + } + + @Override + public void remove(String id) { + forget(id); + } + + /** This browser's binding token, issuing a new cookie if it has none yet */ + private String bindingToken() { + Object issued = request.getAttribute(COOKIE); + if (issued instanceof String) { + return (String) issued; + } + String token = cookieToken(); + if (token == null) { + byte[] bytes = new byte[32]; + random.nextBytes(bytes); + token = Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + Cookie cookie = new Cookie(COOKIE, token); + cookie.setPath(cookiePath); + cookie.setHttpOnly(true); + cookie.setSecure(true); + cookie.setMaxAge((int) LIFETIME.getSeconds()); + cookie.setAttribute("SameSite", "None"); + response.addCookie(cookie); + } + request.setAttribute(COOKIE, token); + return token; + } + + private String cookieToken() { + Cookie[] cookies = request.getCookies(); + if (cookies == null) { + return null; + } + for (Cookie cookie : cookies) { + if (COOKIE.equals(cookie.getName()) && !cookie.getValue().isEmpty()) { + return cookie.getValue(); + } + } + return null; + } + } +} diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index 4181a81cd..68bf3c887 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -50,6 +50,7 @@ import org.opensaml.saml.saml2.core.LogoutRequest; import org.opensaml.saml.saml2.core.NameID; import org.opensaml.saml.saml2.core.NameIDType; +import org.opensaml.saml.saml2.core.Response; import org.opensaml.saml.saml2.core.SessionIndex; import org.opensaml.saml.saml2.metadata.SPSSODescriptor; import org.opensaml.xmlsec.encryption.support.DecryptionException; @@ -196,19 +197,24 @@ static final class Sso { final String spEntityId; + /** Binds each sent AuthnRequest to the browser that started the login */ + final SamlRequestStore requestStore; + Sso( SAML2Client client, X509Certificate idpCertificate, boolean requireSignedLogout, String singleLogoutUrl, String idpEntityId, - String spEntityId) { + String spEntityId, + SamlRequestStore requestStore) { this.client = client; this.idpCertificate = idpCertificate; this.requireSignedLogout = requireSignedLogout; this.singleLogoutUrl = singleLogoutUrl; this.idpEntityId = idpEntityId; this.spEntityId = spEntityId; + this.requestStore = requestStore; } } @@ -266,14 +272,27 @@ public static void login(HttpServletRequest request, HttpServletResponse respons */ public static Optional authenticate( HttpServletRequest request, HttpServletResponse response) { - SAML2Client saml = getClient(); + Sso config = get(); + SAML2Client saml = config.client; CallContext ctx = callContext(request, response); try { - Optional credentials = - saml.getCredentials(ctx).flatMap(c -> saml.validateCredentials(ctx, c)); + Optional extracted = saml.getCredentials(ctx); + if (extracted.isEmpty()) { + return Optional.empty(); + } + // Only answers to a login started in this browser are accepted (login CSRF): an unsolicited + // (IdP-initiated) Response has no InResponseTo to bind it to a browser, so it is refused; + // otherwise pac4j checks InResponseTo against this browser's pending requests + String inResponseTo = inResponseTo(extracted.get()); + if (inResponseTo == null) { + log.error("SAML response rejected: unsolicited (no InResponseTo)"); + return Optional.empty(); + } + Optional credentials = saml.validateCredentials(ctx, extracted.get()); if (credentials.isEmpty()) { return Optional.empty(); } + config.requestStore.forget(inResponseTo); Optional profile = saml.getUserProfile(ctx, credentials.get()); return profile.filter(SAML2Profile.class::isInstance).map(SAML2Profile.class::cast); } catch (RuntimeException e) { @@ -284,6 +303,14 @@ public static Optional authenticate( } } + private static String inResponseTo(Credentials credentials) { + if (!(credentials instanceof SAML2Credentials)) { + return null; + } + Object message = ((SAML2Credentials) credentials).getContext().getMessageContext().getMessage(); + return message instanceof Response ? ((Response) message).getInResponseTo() : null; + } + /** * Starts SP-initiated single logout for the given profile. * @@ -689,6 +716,10 @@ static Sso build(Properties props) { buildIdentityProviderMetadata(props).getBytes(StandardCharsets.UTF_8), "identity provider metadata from " + PROPERTIES_FILE)); + // pac4j's default store keeps nothing, so a Response's InResponseTo was never checked + SamlRequestStore requestStore = new SamlRequestStore(required(props, SP_ACS_URL)); + config.setSamlMessageStoreFactory(requestStore); + SAML2Client saml = new AlternativeAuthnContextClient(config); // The ACS URL is registered with the IdP as-is, so don't append ?client_name=... saml.setCallbackUrlResolver(new NoParameterCallbackUrlResolver()); @@ -709,7 +740,8 @@ static Sso build(Properties props) { flag(props, WANT_MESSAGES_SIGNED, false), singleLogoutUrl, required(props, IDP_ENTITY_ID), - required(props, SP_ENTITY_ID)); + required(props, SP_ENTITY_ID), + requestStore); } /** diff --git a/src/main/java/utils/SsoUserMapper.java b/src/main/java/utils/SsoUserMapper.java index dc296faf1..11d80d113 100644 --- a/src/main/java/utils/SsoUserMapper.java +++ b/src/main/java/utils/SsoUserMapper.java @@ -105,16 +105,24 @@ private static List values(Map attributes, String key) { Object value = key == null ? null : attributes.get(key); if (value instanceof Collection) { for (Object item : (Collection) value) { - if (item != null) { - values.add(item.toString()); - } + addIfNotBlank(values, item); } - } else if (value != null) { - values.add(value.toString()); + } else { + addIfNotBlank(values, value); } return values; } + /** + * Blank values count as missing: a blank ssoName would let different principals share one + * account, since accounts are looked up by it. + */ + private static void addIfNotBlank(List values, Object value) { + if (value != null && !value.toString().trim().isEmpty()) { + values.add(value.toString()); + } + } + private static List splitList(String value) { if (value == null || value.trim().isEmpty()) { return Collections.emptyList(); diff --git a/src/test/java/servlets/SsoDisabledTest.java b/src/test/java/servlets/SsoDisabledTest.java new file mode 100644 index 000000000..d0f9f5303 --- /dev/null +++ b/src/test/java/servlets/SsoDisabledTest.java @@ -0,0 +1,80 @@ +package servlets; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; +import org.springframework.mock.web.MockHttpSession; +import utils.SamlTestSupport; + +/** With sso.enabled off, a retained SAML configuration must not make the SSO endpoints usable */ +public class SsoDisabledTest { + + private SamlTestSupport saml; + + @BeforeEach + public void installClientWithSsoDisabled() throws Exception { + saml = new SamlTestSupport(); + saml.installClient(); + SamlTestSupport.setSsoEnabled(false); + } + + @AfterEach + public void uninstallClient() { + SamlTestSupport.uninstallClient(); + } + + @Test + public void acs_ValidAssertion_DoesNotAuthenticate() throws Exception { + Map> attributes = new LinkedHashMap<>(); + attributes.put("uid", Collections.singletonList("jdoe")); + attributes.put("displayName", Collections.singletonList("Jane Doe")); + attributes.put("groups", Arrays.asList("staff")); + String samlResponse = saml.signedResponse("user-123", attributes, saml.idpKeys, saml.idpCert); + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/saml/acs"); + request.setScheme("https"); + request.setSecure(true); + request.setServerName("shepherd.test"); + request.setServerPort(443); + request.addParameter("SAMLResponse", samlResponse); + request.setCookies(saml.browserCookie()); + MockHttpSession session = new MockHttpSession(); + request.setSession(session); + MockHttpServletResponse response = new MockHttpServletResponse(); + + new ACS().doPost(request, response); + + assertEquals(404, response.getStatus()); + assertNull(session.getAttribute("userName")); + } + + @Test + public void ssoLogin_IsNotFound() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + new SSOLogin().doGet(new MockHttpServletRequest("GET", "/saml/dologin"), response); + assertEquals(404, response.getStatus()); + } + + @Test + public void sls_IsNotFound() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + new SLS().doGet(new MockHttpServletRequest("GET", "/saml/sls"), response); + assertEquals(404, response.getStatus()); + } + + @Test + public void ssoMetadata_IsNotFound() throws Exception { + MockHttpServletResponse response = new MockHttpServletResponse(); + new SSOMetadata().doGet(new MockHttpServletRequest("GET", "/saml/metadata.xml"), response); + assertEquals(404, response.getStatus()); + } +} diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 6a0c491d8..5f487793d 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -62,6 +62,10 @@ private static MockHttpServletRequest acsPost(String samlResponse) { request.setServerName("shepherd.test"); request.setServerPort(443); request.addParameter("SAMLResponse", samlResponse); + // Delivered by the browser that started the login (see SamlTestSupport#startLogin) + if (saml.browserCookie() != null) { + request.setCookies(saml.browserCookie()); + } return request; } @@ -413,6 +417,51 @@ public void getServiceProviderMetadata_WithoutSlsUrl_AdvertisesNoLogoutEndpoint( assertThat(metadata, containsString("Location=\"" + SamlTestSupport.ACS_URL + "\"")); } + @Test + public void authenticate_UnsolicitedResponse_IsRejected() throws Exception { + saml.unsolicited = true; + try { + String samlResponse = + saml.signedResponse("user-123", studentAttributes(), saml.idpKeys, saml.idpCert); + + assertFalse( + SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()).isPresent()); + } finally { + saml.unsolicited = false; + } + } + + /** Login CSRF: a valid response must not log in a browser other than the one that asked */ + @Test + public void authenticate_ResponseDeliveredThroughAnotherBrowser_IsRejected() throws Exception { + saml.startLogin(); + String samlResponse = + saml.signedResponse("attacker", studentAttributes(), saml.idpKeys, saml.idpCert); + MockHttpServletRequest victimWithoutCookie = acsPost(samlResponse); + victimWithoutCookie.setCookies(); + MockHttpServletRequest victimWithOwnCookie = acsPost(samlResponse); + victimWithOwnCookie.setCookies( + new jakarta.servlet.http.Cookie(SamlRequestStore.COOKIE, "victims-own-token")); + + assertFalse( + SamlSso.authenticate(victimWithoutCookie, new MockHttpServletResponse()).isPresent()); + assertFalse( + SamlSso.authenticate(victimWithOwnCookie, new MockHttpServletResponse()).isPresent()); + } + + @Test + public void authenticate_ResponseToAnotherBrowsersLogin_IsRejected() throws Exception { + saml.startLogin(); + String samlResponse = + saml.signedResponse("attacker", studentAttributes(), saml.idpKeys, saml.idpCert); + // The victim's browser has started a login of its own, with its own binding cookie + jakarta.servlet.http.Cookie victimCookie = saml.startLogin(); + MockHttpServletRequest victim = acsPost(samlResponse); + victim.setCookies(victimCookie); + + assertFalse(SamlSso.authenticate(victim, new MockHttpServletResponse()).isPresent()); + } + @Test public void processLogoutMessage_NoMessage_IsInvalid() { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/sls"); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index d50d0b7cb..1f66f100c 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -1,5 +1,6 @@ package utils; +import jakarta.servlet.http.Cookie; import java.io.ByteArrayOutputStream; import java.math.BigInteger; import java.net.URLDecoder; @@ -16,6 +17,8 @@ import java.util.Map; import java.util.Properties; import java.util.UUID; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.zip.Deflater; import java.util.zip.Inflater; import javax.xml.namespace.QName; @@ -58,6 +61,7 @@ import org.opensaml.xmlsec.signature.support.Signer; import org.pac4j.saml.client.SAML2Client; import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; /** * Test fixtures for SAML SSO: generated SP/IdP key pairs, onelogin.saml.properties contents for @@ -124,12 +128,54 @@ public SAML2Client installClient() throws Exception { public static SAML2Client install(Properties props) { SamlSso.Sso sso = SamlSso.build(props); SamlSso.set(sso); + LoginMethod.setSaml(true); return sso.client; } /** Clears the client installed by {@link #installClient()} */ public static void uninstallClient() { SamlSso.set(null); + LoginMethod.setSaml(null); + } + + /** Switches SSO (sso.enabled) on or off without touching the installed client */ + public static void setSsoEnabled(boolean enabled) { + LoginMethod.setSaml(enabled); + } + + /** When true, {@link #signedResponse} builds an unsolicited response (no InResponseTo) */ + public boolean unsolicited; + + private String pendingRequestId; + private Cookie browserCookie; + + /** + * Starts an SP-initiated login as a browser would. The next {@link #signedResponse} answers it. + * + * @return the browser's request-binding cookie, which the ACS request must carry + */ + public Cookie startLogin() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/dologin"); + request.setScheme("https"); + request.setSecure(true); + request.setServerName("shepherd.test"); + request.setServerPort(443); + MockHttpServletResponse response = new MockHttpServletResponse(); + SamlSso.login(request, response); + Matcher id = + Pattern.compile(" ID=\"([^\"]+)\"") + .matcher(decodeRedirectRequest(response.getRedirectedUrl())); + if (!id.find()) { + throw new IllegalStateException("No ID in AuthnRequest"); + } + pendingRequestId = id.group(1); + browserCookie = response.getCookie(SamlRequestStore.COOKIE); + return browserCookie; + } + + /** The binding cookie of the browser that started the most recent login, if any */ + public Cookie browserCookie() { + return browserCookie; } /** @@ -161,6 +207,15 @@ public String signedResponse( response.setIssueInstant(now); response.setVersion(SAMLVersion.VERSION_20); response.setDestination(ACS_URL); + String inResponseTo = null; + if (!unsolicited) { + if (pendingRequestId == null) { + startLogin(); + } + inResponseTo = pendingRequestId; + pendingRequestId = null; + response.setInResponseTo(inResponseTo); + } response.setIssuer(issuer()); Status status = build(Status.DEFAULT_ELEMENT_NAME); StatusCode statusCode = build(StatusCode.DEFAULT_ELEMENT_NAME); @@ -185,6 +240,7 @@ public String signedResponse( confirmation.setMethod(SubjectConfirmation.METHOD_BEARER); SubjectConfirmationData confirmationData = build(SubjectConfirmationData.DEFAULT_ELEMENT_NAME); confirmationData.setRecipient(ACS_URL); + confirmationData.setInResponseTo(inResponseTo); confirmationData.setNotOnOrAfter(now.plus(Duration.ofMinutes(5))); confirmation.setSubjectConfirmationData(confirmationData); subject.getSubjectConfirmations().add(confirmation); diff --git a/src/test/java/utils/SsoUserMapperTest.java b/src/test/java/utils/SsoUserMapperTest.java index 3dbb860ab..32267ec27 100644 --- a/src/test/java/utils/SsoUserMapperTest.java +++ b/src/test/java/utils/SsoUserMapperTest.java @@ -96,6 +96,35 @@ public void map_MissingUserName_IsRejected() { assertEquals("Required SSO attributes are missing", result.error); } + @Test + public void map_BlankSsoName_IsRejected() { + for (String blank : new String[] {"", " "}) { + Map attributes = attributes(Collections.singletonList("staff")); + attributes.put("uid", Collections.singletonList(blank)); + + SsoUserMapper.Result result = SsoUserMapper.map(attributes, ssoProperties()); + + assertFalse(result.isValid()); + assertEquals("Required SSO attributes are missing", result.error); + } + } + + @Test + public void map_BlankUserName_IsRejected() { + Map attributes = attributes(Collections.singletonList("staff")); + attributes.put("displayName", " "); + + assertFalse(SsoUserMapper.map(attributes, ssoProperties()).isValid()); + } + + @Test + public void map_NonBlankValue_IsKeptAsSent() { + Map attributes = attributes(Collections.singletonList("staff")); + attributes.put("uid", Collections.singletonList(" jdoe ")); + + assertEquals(" jdoe ", SsoUserMapper.map(attributes, ssoProperties()).ssoName); + } + @Test public void map_NoAttributes_IsRejected() { SsoUserMapper.Result result = SsoUserMapper.map(Collections.emptyMap(), ssoProperties()); From bad7b288ea9e935116f394ec2cd178172812c75f Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 17:37:13 +0100 Subject: [PATCH 14/19] docs: SAML sample notes SP-initiated-only login and the binding cookie IdP-initiated (unsolicited) responses are now refused and logins are bound to the browser by the shepherd_saml_request cookie; say so where operators configure SSO. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/resources/onelogin.saml.properties.sample | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index 18ff6856e..e7cc6fd0b 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -11,6 +11,12 @@ # (encrypted assertions and NameIDs are still decrypted when received) # onelogin.saml2.idp.certfingerprint alone is not supported: set # onelogin.saml2.idp.x509cert to the IdP's full signing certificate. +# +# Only SP-initiated login is supported (users start at /saml/dologin). +# IdP-initiated (unsolicited) responses are refused: they can't be tied to the +# browser that is logging in, which would allow login CSRF. Each login is bound +# to the browser by the shepherd_saml_request cookie (HttpOnly, Secure, +# SameSite=None, path of the ACS URL), so the ACS must be served over HTTPS. onelogin.saml2.strict = true From fe7a470b07317e4d7f8226d8067ca57c9f767dc9 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 17:41:43 +0100 Subject: [PATCH 15/19] fix: scope the SAML binding cookie to the SAML directory; isolate fixture state Addresses review on #939. - The binding cookie had Path= (/saml/acs), so /saml/dologin never received it: every new login issued a fresh token that replaced the previous one, and with two tabs logging in at once the first callback was rejected. The cookie is now scoped to the ACS URL's directory (e.g. /saml/, keeping any context prefix), so a second login reuses the browser's token. - Test fixture: the simulated browser state (pending AuthnRequest ID, binding cookie) is now cleared whenever a client is installed, so it can't leak between tests; and the browser sends its cookie only where the cookie's Path matches, instead of tests copying it by hand. authenticate_ResponseToAnotherBrowsersLogin_IsRejected now switches to a separate browser explicitly (newBrowser()) before the victim's login; its assertion is unchanged. Tests: two overlapping logins in one browser both succeed; the cookie is Path=/saml/, HttpOnly, Secure, SameSite=None. Both fail with the old ACS-only path. SamlSsoTest passes in three random method orders. Refs #861 Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlRequestStore.java | 18 +++++-- .../resources/onelogin.saml.properties.sample | 3 +- src/test/java/utils/SamlSsoTest.java | 41 +++++++++++++-- src/test/java/utils/SamlTestSupport.java | 50 +++++++++++++++++-- 4 files changed, 99 insertions(+), 13 deletions(-) diff --git a/src/main/java/utils/SamlRequestStore.java b/src/main/java/utils/SamlRequestStore.java index 6abcee70e..0a9307f3b 100644 --- a/src/main/java/utils/SamlRequestStore.java +++ b/src/main/java/utils/SamlRequestStore.java @@ -27,8 +27,9 @@ * stops login CSRF: a valid response for one account can't be replayed through another browser's * assertion consumer service.
* The binding is a random token in a dedicated cookie ({@value #COOKIE}), HttpOnly, Secure and - * SameSite=None so it survives the IdP's cross-site POST back, and scoped to the ACS path only. The - * session cookie's policy is left alone.
+ * SameSite=None so it survives the IdP's cross-site POST back, and scoped to the SAML endpoint + * directory (the ACS URL's directory, e.g. /saml/), so a second login in the same browser (another + * tab) reuses the token instead of replacing it. The session cookie's policy is left alone.
* This file is part of the Security Shepherd Project. * *

The Security Shepherd project is free software: you can redistribute it and/or modify it under @@ -70,11 +71,20 @@ private static final class Pending { private final String cookiePath; /** - * @param assertionConsumerServiceUrl The ACS URL, whose path scopes the binding cookie + * @param assertionConsumerServiceUrl The ACS URL, whose directory (including any context prefix) + * scopes the binding cookie, so both the login start and the ACS receive it */ public SamlRequestStore(String assertionConsumerServiceUrl) { String path = URI.create(assertionConsumerServiceUrl).getPath(); - this.cookiePath = path == null || path.isEmpty() ? "/" : path; + this.cookiePath = + path == null || path.lastIndexOf('/') < 0 + ? "/" + : path.substring(0, path.lastIndexOf('/') + 1); + } + + /** The binding cookie's Path */ + String cookiePath() { + return cookiePath; } @Override diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index e7cc6fd0b..f3bf77828 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -16,7 +16,8 @@ # IdP-initiated (unsolicited) responses are refused: they can't be tied to the # browser that is logging in, which would allow login CSRF. Each login is bound # to the browser by the shepherd_saml_request cookie (HttpOnly, Secure, -# SameSite=None, path of the ACS URL), so the ACS must be served over HTTPS. +# SameSite=None, path of the ACS URL's directory, e.g. /saml/), so the ACS must +# be served over HTTPS. Logins started in several tabs at once all work. onelogin.saml2.strict = true diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 5f487793d..260840df3 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import jakarta.servlet.http.Cookie; import java.security.KeyPair; import java.security.PrivateKey; import java.security.cert.X509Certificate; @@ -62,9 +63,10 @@ private static MockHttpServletRequest acsPost(String samlResponse) { request.setServerName("shepherd.test"); request.setServerPort(443); request.addParameter("SAMLResponse", samlResponse); - // Delivered by the browser that started the login (see SamlTestSupport#startLogin) - if (saml.browserCookie() != null) { - request.setCookies(saml.browserCookie()); + // Delivered by the browser that started the login, with the cookies it sends to the ACS + Cookie[] cookies = SamlTestSupport.cookiesFor("/saml/acs"); + if (cookies.length > 0) { + request.setCookies(cookies); } return request; } @@ -417,6 +419,36 @@ public void getServiceProviderMetadata_WithoutSlsUrl_AdvertisesNoLogoutEndpoint( assertThat(metadata, containsString("Location=\"" + SamlTestSupport.ACS_URL + "\"")); } + /** Two tabs start a login before either finishes: both callbacks must succeed */ + @Test + public void overlappingLogins_InOneBrowser_BothSucceed() throws Exception { + saml.startLogin(); + String first = SamlTestSupport.lastRequestId(); + saml.startLogin(); + String second = SamlTestSupport.lastRequestId(); + + assertTrue( + SamlSso.authenticate( + acsPost(saml.signedResponseTo(first, "user-123", studentAttributes())), + new MockHttpServletResponse()) + .isPresent()); + assertTrue( + SamlSso.authenticate( + acsPost(saml.signedResponseTo(second, "user-123", studentAttributes())), + new MockHttpServletResponse()) + .isPresent()); + } + + @Test + public void bindingCookie_IsScopedToTheSamlDirectory() throws Exception { + Cookie cookie = saml.startLogin(); + + assertEquals("/saml/", cookie.getPath()); + assertTrue(cookie.isHttpOnly()); + assertTrue(cookie.getSecure()); + assertEquals("None", cookie.getAttribute("SameSite")); + } + @Test public void authenticate_UnsolicitedResponse_IsRejected() throws Exception { saml.unsolicited = true; @@ -454,7 +486,8 @@ public void authenticate_ResponseToAnotherBrowsersLogin_IsRejected() throws Exce saml.startLogin(); String samlResponse = saml.signedResponse("attacker", studentAttributes(), saml.idpKeys, saml.idpCert); - // The victim's browser has started a login of its own, with its own binding cookie + // The victim's (different) browser has started a login of its own, with its own cookie + SamlTestSupport.newBrowser(); jakarta.servlet.http.Cookie victimCookie = saml.startLogin(); MockHttpServletRequest victim = acsPost(samlResponse); victim.setCookies(victimCookie); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index 1f66f100c..e4d470ee7 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -128,6 +128,9 @@ public SAML2Client installClient() throws Exception { public static SAML2Client install(Properties props) { SamlSso.Sso sso = SamlSso.build(props); SamlSso.set(sso); + // A fresh client has an empty request store: forget the previous browser's login state + pendingRequestId = null; + browserCookie = null; LoginMethod.setSaml(true); return sso.client; } @@ -146,11 +149,16 @@ public static void setSsoEnabled(boolean enabled) { /** When true, {@link #signedResponse} builds an unsolicited response (no InResponseTo) */ public boolean unsolicited; - private String pendingRequestId; - private Cookie browserCookie; + /* + * One simulated browser, matching the single installed client: the AuthnRequest ID of its most + * recent login, and its binding cookie. Reset by install(). + */ + private static String pendingRequestId; + private static Cookie browserCookie; /** - * Starts an SP-initiated login as a browser would. The next {@link #signedResponse} answers it. + * Starts an SP-initiated login as a browser would, sending its binding cookie if the cookie's + * Path covers the login URL. The next {@link #signedResponse} answers it. * * @return the browser's request-binding cookie, which the ACS request must carry */ @@ -160,6 +168,10 @@ public Cookie startLogin() throws Exception { request.setSecure(true); request.setServerName("shepherd.test"); request.setServerPort(443); + Cookie[] sent = cookiesFor("/saml/dologin"); + if (sent.length > 0) { + request.setCookies(sent); + } MockHttpServletResponse response = new MockHttpServletResponse(); SamlSso.login(request, response); Matcher id = @@ -169,15 +181,45 @@ public Cookie startLogin() throws Exception { throw new IllegalStateException("No ID in AuthnRequest"); } pendingRequestId = id.group(1); - browserCookie = response.getCookie(SamlRequestStore.COOKIE); + Cookie issued = response.getCookie(SamlRequestStore.COOKIE); + if (issued != null) { + browserCookie = issued; + } return browserCookie; } + /** Switches to a different browser, with no binding cookie and no login in progress */ + public static void newBrowser() { + pendingRequestId = null; + browserCookie = null; + } + + /** The AuthnRequest ID of the most recently started login */ + public static String lastRequestId() { + return pendingRequestId; + } + /** The binding cookie of the browser that started the most recent login, if any */ public Cookie browserCookie() { return browserCookie; } + /** The cookies the browser sends to a URL path, following the cookie Path rule */ + public static Cookie[] cookiesFor(String requestPath) { + if (browserCookie == null) { + return new Cookie[0]; + } + String path = browserCookie.getPath() == null ? "/" : browserCookie.getPath(); + return requestPath.startsWith(path) ? new Cookie[] {browserCookie} : new Cookie[0]; + } + + /** As {@link #signedResponse}, answering the given AuthnRequest ID */ + public String signedResponseTo( + String requestId, String nameId, Map> attributes) throws Exception { + pendingRequestId = requestId; + return signedResponse(nameId, attributes, idpKeys, idpCert); + } + /** * Builds a base64 SAMLResponse for the HTTP-POST binding, with the assertion signed by the given * key pair. From 4ea607855958205a3bae421cdb94dbf3cb4694f5 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 17:53:02 +0100 Subject: [PATCH 16/19] fix: refresh the SAML binding cookie's Max-Age on every login start A reused binding token was not re-sent, so the cookie kept the expiry of the first login. A second login started near that expiry had its callback arrive without the cookie and was rejected. The cookie is now re-sent on every login with a full lifetime. The test fixture models the browser dropping cookies by Max-Age to cover this. Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlRequestStore.java | 20 +++++++++------- src/test/java/utils/SamlSsoTest.java | 20 ++++++++++++++++ src/test/java/utils/SamlTestSupport.java | 28 +++++++++++++++++++---- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/main/java/utils/SamlRequestStore.java b/src/main/java/utils/SamlRequestStore.java index 0a9307f3b..1015d1c0e 100644 --- a/src/main/java/utils/SamlRequestStore.java +++ b/src/main/java/utils/SamlRequestStore.java @@ -140,7 +140,11 @@ public void remove(String id) { forget(id); } - /** This browser's binding token, issuing a new cookie if it has none yet */ + /** + * This browser's binding token, creating one if it has none yet. The cookie is (re)sent either + * way, so it lasts as long as the newest pending request and an earlier one's expiry can't drop + * it while a later login is still in progress. + */ private String bindingToken() { Object issued = request.getAttribute(COOKIE); if (issued instanceof String) { @@ -151,14 +155,14 @@ private String bindingToken() { byte[] bytes = new byte[32]; random.nextBytes(bytes); token = Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); - Cookie cookie = new Cookie(COOKIE, token); - cookie.setPath(cookiePath); - cookie.setHttpOnly(true); - cookie.setSecure(true); - cookie.setMaxAge((int) LIFETIME.getSeconds()); - cookie.setAttribute("SameSite", "None"); - response.addCookie(cookie); } + Cookie cookie = new Cookie(COOKIE, token); + cookie.setPath(cookiePath); + cookie.setHttpOnly(true); + cookie.setSecure(true); + cookie.setMaxAge((int) LIFETIME.getSeconds()); + cookie.setAttribute("SameSite", "None"); + response.addCookie(cookie); request.setAttribute(COOKIE, token); return token; } diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 260840df3..33ab37c61 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -12,6 +12,7 @@ import java.security.KeyPair; import java.security.PrivateKey; import java.security.cert.X509Certificate; +import java.time.Duration; import java.util.Arrays; import java.util.Base64; import java.util.Collections; @@ -439,6 +440,25 @@ public void overlappingLogins_InOneBrowser_BothSucceed() throws Exception { .isPresent()); } + /** + * A second tab starts a login just before the first tab's cookie would expire: the cookie must + * last as long as the newest login, or that login's callback arrives without it + */ + @Test + public void overlappingLogins_LaterLoginKeepsTheCookieAlive() throws Exception { + saml.startLogin(); + SamlTestSupport.advanceBrowserClock(Duration.ofMinutes(9)); + saml.startLogin(); + String second = SamlTestSupport.lastRequestId(); + SamlTestSupport.advanceBrowserClock(Duration.ofMinutes(2)); + + assertTrue( + SamlSso.authenticate( + acsPost(saml.signedResponseTo(second, "user-123", studentAttributes())), + new MockHttpServletResponse()) + .isPresent()); + } + @Test public void bindingCookie_IsScopedToTheSamlDirectory() throws Exception { Cookie cookie = saml.startLogin(); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index e4d470ee7..fe658788c 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -129,8 +129,7 @@ public static SAML2Client install(Properties props) { SamlSso.Sso sso = SamlSso.build(props); SamlSso.set(sso); // A fresh client has an empty request store: forget the previous browser's login state - pendingRequestId = null; - browserCookie = null; + newBrowser(); LoginMethod.setSaml(true); return sso.client; } @@ -151,10 +150,13 @@ public static void setSsoEnabled(boolean enabled) { /* * One simulated browser, matching the single installed client: the AuthnRequest ID of its most - * recent login, and its binding cookie. Reset by install(). + * recent login, its binding cookie and when the browser drops that cookie (by its Max-Age), + * measured on a browser clock that tests can move forward. Reset by install(). */ private static String pendingRequestId; private static Cookie browserCookie; + private static Instant browserCookieExpires; + private static Duration browserClock = Duration.ZERO; /** * Starts an SP-initiated login as a browser would, sending its binding cookie if the cookie's @@ -184,6 +186,7 @@ public Cookie startLogin() throws Exception { Cookie issued = response.getCookie(SamlRequestStore.COOKIE); if (issued != null) { browserCookie = issued; + browserCookieExpires = browserNow().plusSeconds(issued.getMaxAge()); } return browserCookie; } @@ -192,6 +195,20 @@ public Cookie startLogin() throws Exception { public static void newBrowser() { pendingRequestId = null; browserCookie = null; + browserCookieExpires = null; + browserClock = Duration.ZERO; + } + + /** + * Moves the browser's clock forward, so its binding cookie expires as its Max-Age says. The + * server's own clock is not moved. + */ + public static void advanceBrowserClock(Duration elapsed) { + browserClock = browserClock.plus(elapsed); + } + + private static Instant browserNow() { + return Instant.now().plus(browserClock); } /** The AuthnRequest ID of the most recently started login */ @@ -204,8 +221,11 @@ public Cookie browserCookie() { return browserCookie; } - /** The cookies the browser sends to a URL path, following the cookie Path rule */ + /** The cookies the browser sends to a URL path, following the cookie Path and Max-Age rules */ public static Cookie[] cookiesFor(String requestPath) { + if (browserCookie != null && !browserNow().isBefore(browserCookieExpires)) { + browserCookie = null; + } if (browserCookie == null) { return new Cookie[0]; } From 95dff1d588a7ea6b24111b7e41913c2dbf4cc5d9 Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 18:07:56 +0100 Subject: [PATCH 17/19] fix: require the bearer confirmation to answer the correlated request pac4j binds a SAML Response to the browser's pending AuthnRequest only through the Response's InResponseTo, which is unsigned unless the whole message is signed (want_messages_signed defaults to false), and never checks the InResponseTo of the bearer SubjectConfirmationData. A valid assertion for one account could therefore be placed in an envelope naming another browser's pending request and log that browser in. The bearer confirmation, inside the signed assertion, must now name the same request as the Response (SAML Profiles 4.1.4.3). A confirmation with no InResponseTo is refused too, since only SP-initiated login is supported. Decrypted assertions go through the same check. Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlSso.java | 27 +++++++++++++++ src/test/java/utils/SamlSsoTest.java | 43 ++++++++++++++++++++++++ src/test/java/utils/SamlTestSupport.java | 17 ++++++++-- 3 files changed, 85 insertions(+), 2 deletions(-) diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index 68bf3c887..bb7971b09 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -52,6 +52,7 @@ import org.opensaml.saml.saml2.core.NameIDType; import org.opensaml.saml.saml2.core.Response; import org.opensaml.saml.saml2.core.SessionIndex; +import org.opensaml.saml.saml2.core.SubjectConfirmationData; import org.opensaml.saml.saml2.metadata.SPSSODescriptor; import org.opensaml.xmlsec.encryption.support.DecryptionException; import org.owasp.encoder.Encode; @@ -783,6 +784,32 @@ protected void validateAuthnContextClassRefs( + provided); } } + + /** + * pac4j binds a response to this browser's request only through the Response's + * InResponseTo, which isn't signed unless the whole message is. The bearer + * confirmation sits inside the signed assertion and must name the same request (SAML + * Profiles 4.1.4.3), so an assertion can't be moved into an envelope naming another + * request. Only SP-initiated login is supported, so it must name one. This runs on + * decrypted assertions too. + */ + @Override + protected boolean isValidBearerSubjectConfirmationData( + SubjectConfirmationData data, SAML2MessageContext context) { + if (!super.isValidBearerSubjectConfirmationData(data, context)) { + return false; + } + Object message = context.getMessageContext().getMessage(); + String requestId = + message instanceof Response ? ((Response) message).getInResponseTo() : null; + if (requestId == null || !requestId.equals(data.getInResponseTo())) { + log.error( + "SAML response rejected: the assertion's bearer confirmation doesn't answer" + + " the request named by the response"); + return false; + } + return true; + } }; authnResponseValidator.setAcceptedSkew(configuration.getAcceptedSkew()); } diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index 33ab37c61..b08ec9c9c 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -483,6 +483,49 @@ public void authenticate_UnsolicitedResponse_IsRejected() throws Exception { } } + /** + * Login CSRF: the attacker's own valid assertion, with only the unsigned envelope's InResponseTo + * changed to the victim browser's pending request, must not log the victim's browser in + */ + @Test + public void authenticate_EnvelopeRetargetedToAnotherBrowsersRequest_IsRejected() + throws Exception { + saml.startLogin(); + String attackersRequest = SamlTestSupport.lastRequestId(); + SamlTestSupport.newBrowser(); + saml.startLogin(); + String victimsRequest = SamlTestSupport.lastRequestId(); + + saml.envelopeInResponseTo = victimsRequest; + try { + String forged = saml.signedResponseTo(attackersRequest, "attacker", studentAttributes()); + + assertFalse(SamlSso.authenticate(acsPost(forged), new MockHttpServletResponse()).isPresent()); + } finally { + saml.envelopeInResponseTo = null; + } + } + + /** + * An assertion whose bearer confirmation names no request (as an IdP-initiated one) can't be + * bound to a login, even inside an envelope that names this browser's pending request + */ + @Test + public void authenticate_ConfirmationWithoutInResponseTo_IsRejected() throws Exception { + saml.startLogin(); + String request = SamlTestSupport.lastRequestId(); + + saml.omitConfirmationInResponseTo = true; + try { + String samlResponse = saml.signedResponseTo(request, "user-123", studentAttributes()); + + assertFalse( + SamlSso.authenticate(acsPost(samlResponse), new MockHttpServletResponse()).isPresent()); + } finally { + saml.omitConfirmationInResponseTo = false; + } + } + /** Login CSRF: a valid response must not log in a browser other than the one that asked */ @Test public void authenticate_ResponseDeliveredThroughAnotherBrowser_IsRejected() throws Exception { diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index fe658788c..0ed7a04fe 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -148,6 +148,19 @@ public static void setSsoEnabled(boolean enabled) { /** When true, {@link #signedResponse} builds an unsolicited response (no InResponseTo) */ public boolean unsolicited; + /** + * When set, {@link #signedResponse} puts this request ID in the Response envelope's InResponseTo + * (which isn't signed unless the whole message is) while the signed assertion's bearer + * confirmation keeps the request it answers: an attacker editing the envelope + */ + public String envelopeInResponseTo; + + /** + * When true, {@link #signedResponse} leaves InResponseTo out of the bearer confirmation, as in an + * assertion issued without a request (IdP-initiated) + */ + public boolean omitConfirmationInResponseTo; + /* * One simulated browser, matching the single installed client: the AuthnRequest ID of its most * recent login, its binding cookie and when the browser drops that cookie (by its Max-Age), @@ -276,7 +289,7 @@ public String signedResponse( } inResponseTo = pendingRequestId; pendingRequestId = null; - response.setInResponseTo(inResponseTo); + response.setInResponseTo(envelopeInResponseTo != null ? envelopeInResponseTo : inResponseTo); } response.setIssuer(issuer()); Status status = build(Status.DEFAULT_ELEMENT_NAME); @@ -302,7 +315,7 @@ public String signedResponse( confirmation.setMethod(SubjectConfirmation.METHOD_BEARER); SubjectConfirmationData confirmationData = build(SubjectConfirmationData.DEFAULT_ELEMENT_NAME); confirmationData.setRecipient(ACS_URL); - confirmationData.setInResponseTo(inResponseTo); + confirmationData.setInResponseTo(omitConfirmationInResponseTo ? null : inResponseTo); confirmationData.setNotOnOrAfter(now.plus(Duration.ofMinutes(5))); confirmation.setSubjectConfirmationData(confirmationData); subject.getSubjectConfirmations().add(confirmation); From b1086b6f33f87f037009fa7359a3bc47544810de Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 18:08:23 +0100 Subject: [PATCH 18/19] docs: note the bearer confirmation must name the AuthnRequest Co-Authored-By: Claude Opus 5.5 --- src/main/resources/onelogin.saml.properties.sample | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index f3bf77828..44ac89e4a 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -18,6 +18,9 @@ # to the browser by the shepherd_saml_request cookie (HttpOnly, Secure, # SameSite=None, path of the ACS URL's directory, e.g. /saml/), so the ACS must # be served over HTTPS. Logins started in several tabs at once all work. +# The assertion's bearer SubjectConfirmationData must carry the AuthnRequest's +# ID in InResponseTo, as the SAML profile requires (standard IdPs do this), so +# the binding holds even when the Response itself is unsigned. onelogin.saml2.strict = true From d1a75e1fe65268488077d6098b2bf22c98019b3b Mon Sep 17 00:00:00 2001 From: ismisepaul Date: Fri, 23 Oct 2026 18:36:27 +0100 Subject: [PATCH 19/19] fix: give each SAML login its own binding cookie All logins shared one binding cookie. Two tabs that opened the login page before either had the cookie each got a different token, and the second Set-Cookie replaced the first, so the first tab's login was rejected. Each AuthnRequest now gets its own cookie, named after a hash of its ID, which lives as long as the request and is deleted once the login completes. That also replaces the Max-Age refresh needed by the shared cookie. The test fixture now keeps a cookie jar and can start logins in parallel tabs. The cross-browser rejection test uses the per-request cookie name, so it still presents a cookie of the right name with the wrong token. Co-Authored-By: Claude Opus 5.5 --- src/main/java/utils/SamlRequestStore.java | 82 +++++++----- src/main/java/utils/SamlSso.java | 2 +- .../resources/onelogin.saml.properties.sample | 7 +- src/test/java/utils/SamlSsoTest.java | 37 +++++- src/test/java/utils/SamlTestSupport.java | 121 +++++++++++------- 5 files changed, 166 insertions(+), 83 deletions(-) diff --git a/src/main/java/utils/SamlRequestStore.java b/src/main/java/utils/SamlRequestStore.java index 1015d1c0e..002f27080 100644 --- a/src/main/java/utils/SamlRequestStore.java +++ b/src/main/java/utils/SamlRequestStore.java @@ -6,6 +6,7 @@ import java.net.URI; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.time.Duration; import java.time.Instant; @@ -26,10 +27,12 @@ * only accepts a Response whose InResponseTo names a request started in the same browser. This * stops login CSRF: a valid response for one account can't be replayed through another browser's * assertion consumer service.
- * The binding is a random token in a dedicated cookie ({@value #COOKIE}), HttpOnly, Secure and - * SameSite=None so it survives the IdP's cross-site POST back, and scoped to the SAML endpoint - * directory (the ACS URL's directory, e.g. /saml/), so a second login in the same browser (another - * tab) reuses the token instead of replacing it. The session cookie's policy is left alone.
+ * Each request gets its own random token, in its own cookie (named {@value #COOKIE} plus a hash of + * the request ID), so logins started at once in several tabs can't overwrite each other's binding. + * The cookie is HttpOnly, Secure and SameSite=None so it survives the IdP's cross-site POST back, + * scoped to the SAML endpoint directory (the ACS URL's directory, e.g. /saml/), lives as long as + * its request and is deleted once the login completes. The session cookie's policy is left alone. + *
* This file is part of the Security Shepherd Project. * *

The Security Shepherd project is free software: you can redistribute it and/or modify it under @@ -45,6 +48,7 @@ */ public class SamlRequestStore implements SAMLMessageStoreFactory { + /** Prefix of the binding cookies' names */ static final String COOKIE = "shepherd_saml_request"; /** How long a started login may take at the IdP */ @@ -93,6 +97,19 @@ public SAMLMessageStore getMessageStore(WebContext context, SessionStore session return new BoundStore(jee.getNativeRequest(), jee.getNativeResponse()); } + /** The name of the cookie binding a request to its browser */ + static String cookieName(String requestId) { + try { + byte[] digest = + MessageDigest.getInstance("SHA-256").digest(requestId.getBytes(StandardCharsets.UTF_8)); + return COOKIE + + "_" + + Base64.getUrlEncoder().withoutPadding().encodeToString(digest).substring(0, 22); + } catch (NoSuchAlgorithmException e) { + throw new IllegalStateException("SHA-256 is unavailable", e); + } + } + /** Forgets a request once its response has been accepted, so each request is used once */ void forget(String requestId) { if (requestId != null) { @@ -100,6 +117,24 @@ void forget(String requestId) { } } + /** As {@link #forget(String)}, also deleting the request's binding cookie from the browser */ + void complete(String requestId, HttpServletResponse response) { + forget(requestId); + if (requestId != null) { + response.addCookie(bindingCookie(requestId, "", 0)); + } + } + + private Cookie bindingCookie(String requestId, String token, int maxAge) { + Cookie cookie = new Cookie(cookieName(requestId), token); + cookie.setPath(cookiePath); + cookie.setHttpOnly(true); + cookie.setSecure(true); + cookie.setMaxAge(maxAge); + cookie.setAttribute("SameSite", "None"); + return cookie; + } + private final class BoundStore implements SAMLMessageStore { private final HttpServletRequest request; private final HttpServletResponse response; @@ -118,13 +153,17 @@ public void set(String id, XMLObject message) { .min(Comparator.comparing(e -> e.getValue().expires)) .ifPresent(oldest -> pending.remove(oldest.getKey())); } - pending.put(id, new Pending(bindingToken(), message, now.plus(LIFETIME))); + byte[] bytes = new byte[32]; + random.nextBytes(bytes); + String token = Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); + response.addCookie(bindingCookie(id, token, (int) LIFETIME.getSeconds())); + pending.put(id, new Pending(token, message, now.plus(LIFETIME))); } @Override public Optional get(String id) { Pending entry = id == null ? null : pending.get(id); - String token = cookieToken(); + String token = id == null ? null : cookieToken(cookieName(id)); if (entry == null || token == null || entry.expires.isBefore(Instant.now())) { return Optional.empty(); } @@ -140,40 +179,13 @@ public void remove(String id) { forget(id); } - /** - * This browser's binding token, creating one if it has none yet. The cookie is (re)sent either - * way, so it lasts as long as the newest pending request and an earlier one's expiry can't drop - * it while a later login is still in progress. - */ - private String bindingToken() { - Object issued = request.getAttribute(COOKIE); - if (issued instanceof String) { - return (String) issued; - } - String token = cookieToken(); - if (token == null) { - byte[] bytes = new byte[32]; - random.nextBytes(bytes); - token = Base64.getUrlEncoder().withoutPadding().encodeToString(bytes); - } - Cookie cookie = new Cookie(COOKIE, token); - cookie.setPath(cookiePath); - cookie.setHttpOnly(true); - cookie.setSecure(true); - cookie.setMaxAge((int) LIFETIME.getSeconds()); - cookie.setAttribute("SameSite", "None"); - response.addCookie(cookie); - request.setAttribute(COOKIE, token); - return token; - } - - private String cookieToken() { + private String cookieToken(String name) { Cookie[] cookies = request.getCookies(); if (cookies == null) { return null; } for (Cookie cookie : cookies) { - if (COOKIE.equals(cookie.getName()) && !cookie.getValue().isEmpty()) { + if (name.equals(cookie.getName()) && !cookie.getValue().isEmpty()) { return cookie.getValue(); } } diff --git a/src/main/java/utils/SamlSso.java b/src/main/java/utils/SamlSso.java index bb7971b09..824d550e4 100644 --- a/src/main/java/utils/SamlSso.java +++ b/src/main/java/utils/SamlSso.java @@ -293,7 +293,7 @@ public static Optional authenticate( if (credentials.isEmpty()) { return Optional.empty(); } - config.requestStore.forget(inResponseTo); + config.requestStore.complete(inResponseTo, response); Optional profile = saml.getUserProfile(ctx, credentials.get()); return profile.filter(SAML2Profile.class::isInstance).map(SAML2Profile.class::cast); } catch (RuntimeException e) { diff --git a/src/main/resources/onelogin.saml.properties.sample b/src/main/resources/onelogin.saml.properties.sample index 44ac89e4a..d7e417526 100644 --- a/src/main/resources/onelogin.saml.properties.sample +++ b/src/main/resources/onelogin.saml.properties.sample @@ -15,9 +15,10 @@ # Only SP-initiated login is supported (users start at /saml/dologin). # IdP-initiated (unsolicited) responses are refused: they can't be tied to the # browser that is logging in, which would allow login CSRF. Each login is bound -# to the browser by the shepherd_saml_request cookie (HttpOnly, Secure, -# SameSite=None, path of the ACS URL's directory, e.g. /saml/), so the ACS must -# be served over HTTPS. Logins started in several tabs at once all work. +# to the browser by a cookie of its own, shepherd_saml_request_ (HttpOnly, +# Secure, SameSite=None, path of the ACS URL's directory, e.g. /saml/), deleted +# once the login completes, so the ACS must be served over HTTPS. Logins started +# in several tabs at once all work. # The assertion's bearer SubjectConfirmationData must carry the AuthnRequest's # ID in InResponseTo, as the SAML profile requires (standard IdPs do this), so # the binding holds even when the Response itself is unsigned. diff --git a/src/test/java/utils/SamlSsoTest.java b/src/test/java/utils/SamlSsoTest.java index b08ec9c9c..79c1de579 100644 --- a/src/test/java/utils/SamlSsoTest.java +++ b/src/test/java/utils/SamlSsoTest.java @@ -5,6 +5,7 @@ import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -459,6 +460,39 @@ public void overlappingLogins_LaterLoginKeepsTheCookieAlive() throws Exception { .isPresent()); } + /** Two tabs open the login page before the browser holds any binding cookie: both must work */ + @Test + public void concurrentFirstLogins_InOneBrowser_BothSucceed() throws Exception { + List requests = SamlTestSupport.startConcurrentLogins(2); + + assertTrue( + SamlSso.authenticate( + acsPost(saml.signedResponseTo(requests.get(0), "user-123", studentAttributes())), + new MockHttpServletResponse()) + .isPresent()); + assertTrue( + SamlSso.authenticate( + acsPost(saml.signedResponseTo(requests.get(1), "user-123", studentAttributes())), + new MockHttpServletResponse()) + .isPresent()); + } + + /** A completed login deletes its binding cookie, so cookies don't pile up in the browser */ + @Test + public void authenticate_Success_DeletesThatLoginsCookie() throws Exception { + Cookie cookie = saml.startLogin(); + String samlResponse = + saml.signedResponse("user-123", studentAttributes(), saml.idpKeys, saml.idpCert); + MockHttpServletResponse response = new MockHttpServletResponse(); + + assertTrue(SamlSso.authenticate(acsPost(samlResponse), response).isPresent()); + + Cookie deleted = response.getCookie(cookie.getName()); + assertNotNull(deleted); + assertEquals(0, deleted.getMaxAge()); + assertEquals(cookie.getPath(), deleted.getPath()); + } + @Test public void bindingCookie_IsScopedToTheSamlDirectory() throws Exception { Cookie cookie = saml.startLogin(); @@ -530,13 +564,14 @@ public void authenticate_ConfirmationWithoutInResponseTo_IsRejected() throws Exc @Test public void authenticate_ResponseDeliveredThroughAnotherBrowser_IsRejected() throws Exception { saml.startLogin(); + String request = SamlTestSupport.lastRequestId(); String samlResponse = saml.signedResponse("attacker", studentAttributes(), saml.idpKeys, saml.idpCert); MockHttpServletRequest victimWithoutCookie = acsPost(samlResponse); victimWithoutCookie.setCookies(); MockHttpServletRequest victimWithOwnCookie = acsPost(samlResponse); victimWithOwnCookie.setCookies( - new jakarta.servlet.http.Cookie(SamlRequestStore.COOKIE, "victims-own-token")); + new jakarta.servlet.http.Cookie(SamlRequestStore.cookieName(request), "victims-own-token")); assertFalse( SamlSso.authenticate(victimWithoutCookie, new MockHttpServletResponse()).isPresent()); diff --git a/src/test/java/utils/SamlTestSupport.java b/src/test/java/utils/SamlTestSupport.java index 0ed7a04fe..ae31cce5f 100644 --- a/src/test/java/utils/SamlTestSupport.java +++ b/src/test/java/utils/SamlTestSupport.java @@ -11,8 +11,11 @@ import java.security.cert.X509Certificate; import java.time.Duration; import java.time.Instant; +import java.util.ArrayList; import java.util.Base64; import java.util.Date; +import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Properties; @@ -163,58 +166,94 @@ public static void setSsoEnabled(boolean enabled) { /* * One simulated browser, matching the single installed client: the AuthnRequest ID of its most - * recent login, its binding cookie and when the browser drops that cookie (by its Max-Age), - * measured on a browser clock that tests can move forward. Reset by install(). + * recent login, and its cookie jar, where each cookie is dropped when its Max-Age runs out on a + * browser clock that tests can move forward. Reset by install(). */ private static String pendingRequestId; - private static Cookie browserCookie; - private static Instant browserCookieExpires; + private static final Map cookieJar = new LinkedHashMap<>(); + private static final Map cookieExpires = new HashMap<>(); + private static Cookie lastLoginCookie; private static Duration browserClock = Duration.ZERO; /** - * Starts an SP-initiated login as a browser would, sending its binding cookie if the cookie's - * Path covers the login URL. The next {@link #signedResponse} answers it. + * Starts an SP-initiated login as a browser would, sending the cookies whose Path covers the + * login URL. The next {@link #signedResponse} answers it. * - * @return the browser's request-binding cookie, which the ACS request must carry + * @return the request-binding cookie this login set, which the ACS request must carry */ public Cookie startLogin() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/dologin"); - request.setScheme("https"); - request.setSecure(true); - request.setServerName("shepherd.test"); - request.setServerPort(443); - Cookie[] sent = cookiesFor("/saml/dologin"); - if (sent.length > 0) { - request.setCookies(sent); + startConcurrentLogins(1); + return lastLoginCookie; + } + + /** + * Starts logins in several tabs at once: every request leaves with the cookies the browser held + * before any of them, and only then are the responses' cookies stored, in order. The next {@link + * #signedResponse} answers the last one. + * + * @return the AuthnRequest IDs, in the order the logins were started + */ + public static List startConcurrentLogins(int tabs) throws Exception { + List ids = new ArrayList<>(); + List responses = new ArrayList<>(); + for (int i = 0; i < tabs; i++) { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/saml/dologin"); + request.setScheme("https"); + request.setSecure(true); + request.setServerName("shepherd.test"); + request.setServerPort(443); + Cookie[] sent = cookiesFor("/saml/dologin"); + if (sent.length > 0) { + request.setCookies(sent); + } + MockHttpServletResponse response = new MockHttpServletResponse(); + SamlSso.login(request, response); + Matcher id = + Pattern.compile(" ID=\"([^\"]+)\"") + .matcher(decodeRedirectRequest(response.getRedirectedUrl())); + if (!id.find()) { + throw new IllegalStateException("No ID in AuthnRequest"); + } + ids.add(id.group(1)); + responses.add(response); } - MockHttpServletResponse response = new MockHttpServletResponse(); - SamlSso.login(request, response); - Matcher id = - Pattern.compile(" ID=\"([^\"]+)\"") - .matcher(decodeRedirectRequest(response.getRedirectedUrl())); - if (!id.find()) { - throw new IllegalStateException("No ID in AuthnRequest"); + for (MockHttpServletResponse response : responses) { + for (Cookie cookie : response.getCookies()) { + storeCookie(cookie); + if (cookie.getName().startsWith(SamlRequestStore.COOKIE) && cookie.getMaxAge() != 0) { + lastLoginCookie = cookie; + } + } } - pendingRequestId = id.group(1); - Cookie issued = response.getCookie(SamlRequestStore.COOKIE); - if (issued != null) { - browserCookie = issued; - browserCookieExpires = browserNow().plusSeconds(issued.getMaxAge()); + pendingRequestId = ids.get(ids.size() - 1); + return ids; + } + + /** Stores a Set-Cookie as a browser does: replacing any cookie of that name, or deleting it */ + private static void storeCookie(Cookie cookie) { + if (cookie.getMaxAge() == 0) { + cookieJar.remove(cookie.getName()); + cookieExpires.remove(cookie.getName()); + return; } - return browserCookie; + cookieJar.put(cookie.getName(), cookie); + cookieExpires.put( + cookie.getName(), + cookie.getMaxAge() < 0 ? Instant.MAX : browserNow().plusSeconds(cookie.getMaxAge())); } - /** Switches to a different browser, with no binding cookie and no login in progress */ + /** Switches to a different browser, with no cookies and no login in progress */ public static void newBrowser() { pendingRequestId = null; - browserCookie = null; - browserCookieExpires = null; + cookieJar.clear(); + cookieExpires.clear(); + lastLoginCookie = null; browserClock = Duration.ZERO; } /** - * Moves the browser's clock forward, so its binding cookie expires as its Max-Age says. The - * server's own clock is not moved. + * Moves the browser's clock forward, so its cookies expire as their Max-Age says. The server's + * own clock is not moved. */ public static void advanceBrowserClock(Duration elapsed) { browserClock = browserClock.plus(elapsed); @@ -229,21 +268,17 @@ public static String lastRequestId() { return pendingRequestId; } - /** The binding cookie of the browser that started the most recent login, if any */ + /** The binding cookie set by the most recently started login, if any */ public Cookie browserCookie() { - return browserCookie; + return lastLoginCookie; } /** The cookies the browser sends to a URL path, following the cookie Path and Max-Age rules */ public static Cookie[] cookiesFor(String requestPath) { - if (browserCookie != null && !browserNow().isBefore(browserCookieExpires)) { - browserCookie = null; - } - if (browserCookie == null) { - return new Cookie[0]; - } - String path = browserCookie.getPath() == null ? "/" : browserCookie.getPath(); - return requestPath.startsWith(path) ? new Cookie[] {browserCookie} : new Cookie[0]; + cookieJar.keySet().removeIf(name -> !browserNow().isBefore(cookieExpires.get(name))); + return cookieJar.values().stream() + .filter(c -> requestPath.startsWith(c.getPath() == null ? "/" : c.getPath())) + .toArray(Cookie[]::new); } /** As {@link #signedResponse}, answering the given AuthnRequest ID */