feat(sso): report insecure SAML settings - #3224
Merged
Merged
Conversation
marevol
force-pushed
the
saml-security-warnings
branch
from
August 10, 2026 00:51
de2260c to
67d885e
Compare
marevol
force-pushed
the
saml-security-warnings
branch
from
August 10, 2026 01:40
67d885e to
2aed508
Compare
marevol
changed the title
marevol
force-pushed
the
saml-security-warnings
branch
from
August 10, 2026 01:57
2aed508 to
e6ea423
Compare
marevol
force-pushed
the
saml-security-warnings
branch
from
August 10, 2026 02:12
e6ea423 to
edcf005
Compare
java-saml exposes Saml2Settings#getSecurityWarnings(), which flags weak but non-fatal configuration. Nothing called it, so a deployment running on the permissive defaults had no way to learn about it except by reading the code. getSettings() now logs the warnings at WARN. Since the settings are rebuilt on every request, the warnings are repeated only when they change, so editing the configuration from the admin UI re-reports the new state without flooding the log in between. With the shipped defaults the message is: Insecure SAML settings: deprecated_signature_algorithms_not_rejected, assertions_and_messages_not_required_signed. See the SAML SSO documentation for the recommended values. The tomcat_config.properties note this change originally carried about SameSite and the SAML HTTP-POST binding is dropped: #3215 landed the same guidance on master, covering both SAML and Entra ID form_post.
marevol
force-pushed
the
saml-security-warnings
branch
from
August 10, 2026 02:21
edcf005 to
38d3507
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
getSecurityWarnings()had no callerjava-saml exposes
Saml2Settings#getSecurityWarnings()for configuration that is weak but not fatal — the thingscheckSettings()deliberately does not reject. Nothing in Fess called it, so an installation running on the permissive defaults had no way to learn about it short of readingSaml2Settings.getSettings()now logs them. With the shipped defaults that is:Settings are rebuilt on every request (see #3217), so logging unconditionally would put this line in front of every login. The warnings are compared with the previous set and logged only when they change, which also means editing the configuration in the admin UI re-reports the new state rather than going quiet forever after the first request.
To be explicit about what this is and is not:
want_assertions_signed=falsedoes not allow unsigned assertions.SamlResponse#isValidrejects a response with no signed element unconditionally, outside thestrictblock. The flag only relaxes which element must carry the signature, so this is a hardening prompt, not a vulnerability notice. The defaults are left as they are.Dropped: the SameSite note
This PR originally also documented in
src/main/assemblies/files/tomcat_config.propertieswhy the SAML HTTP-POST binding cannot run on the defaulttomcat.sameSiteCookies = lax. #3215 has since landed equivalent guidance onmaster, and its wording is broader — it covers Entra IDresponse_mode=form_postas well as SAML, and namessession.cookie.secure=true. The note was dropped on rebase rather than replacing the merged one; the default is still unchanged, sincenonerequires HTTPS.Test
test_getSettings_logsSecurityWarningsUntilTheyChangecaptures the logger with theLogCapturingAppenderpattern already used inLengthChunkerTest, asserts the defaults produce exactly one warning across twogetSettings()calls, then flipssaml.security.want_assertions_signedand asserts the changed set is reported again. Confirmed falsifiable: logging unconditionally makes the first assertion fail withexpected: <1> but was: <2>.