You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: reimplement SAML SSO on pac4j-saml (PR 5 of jakarta migration #861) - #939
Stacked on #936 (PR 4). This is the PR that makes the stack compile again.
Why
java-saml has no usable Jakarta release: its Auth class only accepts javax.servlet types, so ACS, Logout, SLS and SSOLogin could not compile against jakarta.servlet. Per the decision recorded in #861, SSO is reimplemented on pac4j-saml 6.6 (OpenSAML 5, Jakarta-native, itself built on Spring 7, matching PR 4).
What changed
utils.SamlSso builds the SAML2Client from the existing onelogin.saml.properties keys, so deployments keep their configuration:
the SP's PEM cert/key (PKCS#8 or PKCS#1, with or without PEM armour) go into an in-memory PKCS12 keystore;
the IdP keys (entityid, SSO/SLO URLs + bindings, x509cert) are turned into an IdP metadata document;
nothing is read from or written to disk beyond the properties file.
utils.SsoUserMapper: the attribute → user/role mapping previously inline in ACS, now unit-testable.
The five servlets delegate to SamlSso. The Getter.authUserSSO(...) hand-off, session renewal, CSRF cookie and kick-list handling are unchanged.
/saml/sls returned 405 to every IdP logout message: SLS only defined handleRequest(), never doGet/doPost.
SP-initiated logout sent no NameID: Logout invalidated the session, created a new one, then read nameId/sessionIndex from the new session. ACS now stores the SAML profile in the post-login session and Logout reads it before invalidating.
ACS threw a NullPointerException (500) when a configured attribute was missing, and rendered a blank page when the assertion had no attributes; both now return to the login page with a message.
Behaviour notes
java-saml's strict=false, debug, want_*_encrypted, nameid_encrypted, logoutresponse_signed and want_xml_validation have no pac4j equivalent and are ignored (logged / documented in the sample). Encrypted assertions/NameIDs are still decrypted when received. idp.certfingerprint alone is not supported; the full IdP certificate is required.
With SSO not configured, /saml/* still returns 500, as before.
spring-core is no longer declared test-scoped: a test-scoped direct declaration would override pac4j's compile-scope copy and drop it from the WAR. A spring-framework-bom import keeps every Spring module on ${spring.version}.
commons-io 2.18 → 2.22 (what pac4j is built against).
Verification (Temurin 17, local)
mvn test: 192 run, 0 failures (2 skipped, pre-existing). 23 new tests: PEM parsing, SP metadata, AuthnRequest redirect, and signed SAML responses end-to-end: valid accepted; tampered, signed by an unknown key, and replayed all rejected; SLO request carries NameID + SessionIndex; SLS handles GET/POST. Keys are generated per test run; no private keys are committed.
Docker smoke test of the full stack (PR 8 tip) on Tomcat 11.0.27: deploys cleanly, HTTP→HTTPS redirect, TLS 1.3, admin login, /api/levels, translated JSP rendering.
Please merge with a merge commit (stack branches can't be force-pushed).
)
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 <noreply@anthropic.com>
Reimplements SAML single sign-on on Jakarta-native pac4j, addressing the remaining SAML compilation blocker in the Jakarta migration stack while retaining existing configuration keys and user authentication integration.
Changes:
Adds shared SAML handling, an in-memory SP keystore, and attribute-to-user mapping.
Migrates five servlets and updates configuration samples, including logout and missing-attribute handling.
Updates dependencies and adds tests for signed responses, replay rejection, metadata, logout requests, and user mapping.
File
Description
src/test/java/utils/SsoUserMapperTest.java
Tests attribute mapping and authorization failures.
src/test/java/utils/SamlTestSupport.java
Provides generated keys and signed SAML fixtures.
src/test/java/utils/SamlSsoTest.java
Tests SAML configuration, authentication, metadata, and logout.
src/test/java/servlets/SLSTest.java
Tests GET/POST handling and invalid-message session preservation.
Addresses review on #939.
Logout message trust:
- pac4j validates a logout message's XML signature only when one is
present, and never checks HTTP-Redirect query-string signatures (the
configured SLO binding). Unsigned or forged LogoutRequests and
LogoutResponses were therefore accepted. When want_messages_signed is
set, SamlSso now requires a valid IdP signature: XML (HTTP-POST) or
query-string (HTTP-Redirect, verified per SAML Bindings 3.4.4.1 with
the configured IdP certificate; SHA-1 algorithms are not accepted).
Session targeting:
- SLS invalidated whichever session the delivering browser had. An IdP
LogoutRequest now ends exactly the sessions created by the login it
names (NameID + SessionIndex), via a new SamlSessionRegistry that ACS
registers the post-login session in. This works whichever browser
delivers the message and with no session cookie. An unsigned request
carrying only a NameID ends nothing.
- A LogoutResponse ends nothing: Logout already ended the local session
before sending the user to the IdP.
Also:
- Send NameQualifier / SPNameQualifier back in LogoutRequests
(useNameQualifier), so IdPs issuing qualified NameIDs can match them.
- Default requested_authncontextcomparison to "exact" (pac4j drops
RequestedAuthnContext entirely without one), and still read the key as
misspelled in the old sample file.
- @OverRide on SLS doGet/doPost.
Tests: SamlLogoutTest covers signed/unsigned/unknown-key/tampered
LogoutResponses and LogoutRequests in both bindings, session targeting
across browsers and without a cookie, and SessionIndex mismatch; plus
NameID qualifier and comparison-default tests. Each fix was checked to
fail its tests when switched off.
Refs #861
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ery SessionIndex
Addresses review on #939.
- The HTTP-Redirect signature check matched query parameters by their
raw names, while the servlet container (and pac4j) URL-decodes names.
A captured signed query with an unsigned message prepended as
%53AMLRequest=... passed the check on the signed copy while pac4j
processed the unsigned one. The check now accepts only a GET, rejects
percent-encoded aliases and repeated signed parameters, and requires
each signed value to equal the single value the container returns.
- An IdP LogoutRequest naming several SessionIndexes ended only the
first; it now ends each one. A NameID-wide logout (no SessionIndex)
still requires a signed request.
Regression tests (the slsGet fixture now URL-decodes parameter names
like a container): encoded alias, repeated parameter, redirect
signature on a POST, and two SessionIndexes for one NameID. Three of
the four fail against the previous SamlSso; the repeated-parameter
case already failed safe and is kept as a guard.
Refs #861
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Spring is no longer test-only: pac4j-saml needs it at runtime and SamlSso
uses spring-core's ByteArrayResource (review on #942).
Refs #861
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Convert XMLDSIG ECDSA signatures to DER before verification
src/main/java/utils/SamlSso.java:419
[P2] Convert ECDSA signatures from XMLDSIG format before verification. SAML's ecdsa-sha* signatures contain fixed-width r || s bytes, but the configured SHA*withECDSA JCA algorithms expect ASN.1 DER. A standards-compliant EC IdP's redirect signature therefore fails verification, rejecting its logout messages when want_messages_signed=true. Convert only ECDSA signatures to DER before calling verify, leaving RSA signatures unchanged. Add an EC-signing regression alongside the RSA tests.
…A 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 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Valid ECDSA-signed logout redirects are rejected because the verifier expects the wrong signature encoding, leaving targeted sessions active.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Use P1363 ECDSA verification for SAML redirect signatures
src/main/java/utils/SamlSso.java:137
[P2] Use the SAML wire format when verifying ECDSA redirect signatures. SAML encodes these signatures as fixed-width r || s, but the mapped SHA*withECDSA verifiers expect ASN.1 DER. A valid EC-signed IdP LogoutRequest therefore fails verification when want_messages_signed=true, leaving its targeted sessions authenticated. Map these algorithms to the P1363-format verifiers supported by the project's Java 17 runtime, and add an EC-key redirect logout regression test; the current fixtures only generate RSA signatures.
… java-saml)
Every PR in the Jakarta migration stack must pass CI by itself. This one
didn't compile: ACS, Logout, SLS and SSOLogin passed jakarta servlet
types to java-saml 2.9.0, which only accepts javax.servlet and has no
Jakarta release. Its Docker build would also have failed: the Tomcat 9
era server.xml patch doesn't apply to Tomcat 10.1.
- Remove java-saml. The SSO endpoints (ACS, SSOLogin, SLS, SSOMetadata)
answer 503 "SAML SSO is unavailable during migration", and Logout
drops its java-saml branch. SSO is off unless sso.enabled=true, and
is reimplemented on pac4j-saml in #939 further up the stack, so the
final code on dev is unchanged.
- Regenerate serverxml.patch / webxml.patch against Tomcat 10.1.61's
conf files (HTTPS via <SSLHostConfig>/<Certificate>), with the
matching Dockerfile substitutions. Both apply exactly with the image's
GNU patch.
Verified locally: spotless, 169 unit tests, 312 integration tests,
mvn -Pdocker install, docker compose build, and a Tomcat 10.1.61 smoke
test (deploys, HTTP->HTTPS, TLS 1.3, login page, SSO endpoints 503).
Refs #861
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ture state
Addresses review on #939.
- The binding cookie had Path=<ACS 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 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Overlapping login attempts overwrite the browser-binding cookie, causing valid earlier authentication responses to be rejected.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Preserve SAML binding across overlapping login attempts
src/main/java/utils/SamlRequestStore.java:77
[P2] Preserve the browser binding across overlapping login attempts. With the configured /saml/acs cookie path, the browser never sends this cookie to /saml/dologin, so each login start generates a new token and overwrites the previous cookie. If a second tab starts login before the first returns, the first valid, unexpired response carries the second token and is rejected. Use a stable binding available to both endpoints, refreshing its lifetime when starting a login, or use separate per-request cookies. Preserve application isolation and the existing security flags, update the documented cookie scope, and add a two-pending-logins regression that applies browser cookie-path rules.
[P2] Redirect outside the SAML subdirectory. This servlet is mapped to /saml/dologin, so index.jsp resolves to /saml/index.jsp, which does not exist. A POST therefore redirects to a 404 instead of the application's index page. Use ../index.jsp, as ACS does, to preserve the deployment context and reach the existing page.
Redirect metadata POST to application index
src/main/java/servlets/SSOMetadata.java:41
[P2] Redirect to the application's index page rather than /saml/index.jsp. This servlet is mapped to /saml/metadata.xml, so the relative target index.jsp stays under /saml/, where no such page exists. A POST therefore redirects to a 404. Use ../index.jsp, as ACS does, to reach the application root while preserving its deployment context.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Concurrent initial logins can lose browser binding, and unsupported callback bindings can be configured without a working authentication handler.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Prevent concurrent logins from overwriting binding cookies
src/main/java/utils/SamlRequestStore.java:158
[P2] Concurrent initial logins still overwrite each other's binding cookie. If two tabs request /saml/dologin before either receives the cookie, each request generates a different token here. Both responses set the same cookie name and path, so the last cookie replaces the other flow's token and that flow's valid callback is rejected. The overlapping-login test applies the first response's cookie before starting the second request, so it does not cover this race. Establish shared browser binding before parallel login starts, or atomically reuse a token from an existing shared pre-login session while retaining the dedicated callback cookie. Add a regression where both login requests arrive without the binding cookie before either response is applied.
Reject unsupported ACS bindings
src/main/java/utils/SamlSso.java:679
[P2] Reject ACS bindings that the servlet cannot consume. This passes the configured binding to pac4j, but ACS.doGet() redirects to index.jsp without authenticating. Configuring a GET-based response binding therefore advertises a login flow whose callback is discarded. Restrict this setting to HTTP-POST until other callback bindings are implemented, and add a test that unsupported bindings fail configuration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
PR 5 of the Jakarta migration stack (#861)
Stacked on #936 (PR 4). This is the PR that makes the stack compile again.
Why
java-samlhas no usable Jakarta release: itsAuthclass only acceptsjavax.servlettypes, soACS,Logout,SLSandSSOLogincould not compile againstjakarta.servlet. Per the decision recorded in #861, SSO is reimplemented on pac4j-saml 6.6 (OpenSAML 5, Jakarta-native, itself built on Spring 7, matching PR 4).What changed
utils.SamlSsobuilds theSAML2Clientfrom the existingonelogin.saml.propertieskeys, so deployments keep their configuration:entityid, SSO/SLO URLs + bindings,x509cert) are turned into an IdP metadata document;utils.SsoUserMapper: the attribute → user/role mapping previously inline inACS, now unit-testable.SamlSso. TheGetter.authUserSSO(...)hand-off, session renewal, CSRF cookie and kick-list handling are unchanged.onelogin.saml.properties.sample/sso.properties.sampleupdated (correct/saml/*URLs, a typo'd key fixed, unsupported java-saml options listed).Bugs fixed along the way (each has a test)
/saml/slsreturned 405 to every IdP logout message:SLSonly definedhandleRequest(), neverdoGet/doPost.Logoutinvalidated the session, created a new one, then readnameId/sessionIndexfrom the new session.ACSnow stores the SAML profile in the post-login session andLogoutreads it before invalidating.ACSthrew aNullPointerException(500) when a configured attribute was missing, and rendered a blank page when the assertion had no attributes; both now return to the login page with a message.Behaviour notes
strict=false,debug,want_*_encrypted,nameid_encrypted,logoutresponse_signedandwant_xml_validationhave no pac4j equivalent and are ignored (logged / documented in the sample). Encrypted assertions/NameIDs are still decrypted when received.idp.certfingerprintalone is not supported; the full IdP certificate is required./saml/*still returns 500, as before.Build
java-saml→pac4j-saml+pac4j-jakartaee6.6.0;log4j-slf4j2-implroutes pac4j/OpenSAML logging into log4j2.spring-coreis no longer declared test-scoped: a test-scoped direct declaration would override pac4j's compile-scope copy and drop it from the WAR. Aspring-framework-bomimport keeps every Spring module on${spring.version}.commons-io2.18 → 2.22 (what pac4j is built against).Verification (Temurin 17, local)
mvn test: 192 run, 0 failures (2 skipped, pre-existing). 23 new tests: PEM parsing, SP metadata, AuthnRequest redirect, and signed SAML responses end-to-end: valid accepted; tampered, signed by an unknown key, and replayed all rejected; SLO request carries NameID + SessionIndex; SLS handles GET/POST. Keys are generated per test run; no private keys are committed.mvn verify -DskipUTs=true -DmongoDockeragainst MariaDB 10.6 + Mongo: 312 run, 0 failures (5 skipped, pre-existing), incl.LogoutIT./api/levels, translated JSP rendering.Please merge with a merge commit (stack branches can't be force-pushed).
Refs #861
🤖 Generated with Claude Code