REF-33: Read IdP metadata from a deployed file and accept every signing certificate in it - #100
Merged
thomasnymand merged 6 commits intoAug 26, 2026
Conversation
…ng certificate in it The SP could fetch IdP metadata over HTTP, and did so on a refresh timer, which made the IdP signing certificate replaceable at runtime by whoever answered the metadata URL. NemLog-in does not sign its metadata, so there is nothing in the document itself to verify: trust can only come from the deployment of the file. oiosaml.servlet.trust.selfsigned.certs made this worse by turning off both certificate and hostname validation for that fetch. HTTPMetadataResolver and the HTTP client are removed, oiosaml.servlet.idp .metadata.file is now mandatory, and the resolver requires metadata to be within its validUntil. The file is still re-read while the SP runs, so publishing new keys remains a matter of replacing it. DispatcherServlet refuses to start when oiosaml.servlet.idp.metadata.url or trust.selfsigned.certs is set, naming the replacement, rather than starting with a trust model the deployment does not expect. Without an auto refreshing resolver the SP has to ride a signing key rotation on the metadata it was given, and an IdP publishes both the outgoing and the incoming certificate while rotating. getValidX509Certificate returned only the first one, so half of such a rotation would have failed validation. It is replaced by getValidX509Certificates, and assertion signatures are accepted when they validate against any of the published certificates. Tests deploy metadata as a temporary file rather than serving it from MockServer, and a new test signs an assertion with a certificate that is not first in metadata.
…ttings The keystore, IdP metadata and configuration file settings are all resolved from the classpath first and then as a filesystem path, so they were not classpath-only as documented. Also note that metadata packed inside a war or jar is copied to a temporary file at startup, which the periodic refresh then re-reads, and that the configuration file is merged on top of the init-params rather than replacing them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The certificate the valid-certificate revocation tests used expired on 2026-08-17, so the OCSP test started failing: the PKIX validation rejects it on the validity period, which is not the revoked status the test is about. The DevTest4 IdP signing certificate is issued by the same test CA and runs until 2028. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mthiim
reviewed
Aug 26, 2026
|
|
||
| /** | ||
| * Refuse to start on configuration that is no longer supported, rather than starting with the setting | ||
| * silently ignored and a different trust model than the deployment expects. |
Collaborator
There was a problem hiding this comment.
It could be a bit more clear what it means a configuration is no longer supported and what it is we intent to reject
Collaborator
Author
There was a problem hiding this comment.
Agreed. I have updated the comment.
…signing-certificates # Conflicts: # oiosaml/src/main/java/dk/gov/oio/saml/service/validation/AssertionValidationService.java # oiosaml/src/test/java/dk/gov/oio/saml/service/validation/AssertionValidationServiceTest.java # oiosaml/src/test/java/dk/gov/oio/saml/util/TestConstants.java
…rrect in LogoutRequestService.
…s DispatcherServlet to initialize.
mthiim
approved these changes
Aug 26, 2026
thomasnymand
deleted the
feature/REF-33-file-metadata-and-multiple-signing-certificates
branch
August 26, 2026 20:25
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
2 participants
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.
Makes deployed IdP metadata the only source of trust, and accepts every signing certificate published in it.
Problem
The SP could fetch IdP metadata over HTTP, on a refresh timer, which made the IdP signing certificate — the anchor for assertion, logout and revocation validation — replaceable at runtime by whoever answered the metadata URL. NemLog-in does not sign its metadata, so there is nothing in the document to verify against a pinned key: trust can only come from deploying the file.
oiosaml.servlet.trust.selfsigned.certsmade it worse by disabling both certificate and hostname validation for that fetch.Changes
Metadata is read from a deployed file
HTTPMetadataResolverand the HTTP client are removed.oiosaml.servlet.idp.metadata.fileis mandatory;idpMetadataUrlandsupportSelfSignedare gone fromConfiguration.validUntil.DispatcherServletrefuses to start whenoiosaml.servlet.idp.metadata.urlis configured without a file, naming the replacement property and why. If both are set, the URL is ignored with a warning.trust.selfsigned.certs=trueis a hard failure. The two keys remain inConstantsasREMOVED_*only so the check can name them.Every signing certificate in metadata is accepted
Without a refreshing resolver the SP has to ride a signing key rotation on the metadata it was given, and an IdP publishes both the outgoing and the incoming certificate while rotating.
getValidX509Certificatereturned only the first, so half of such a rotation would have failed validation. It is replaced bygetValidX509Certificates(UsageType), and an assertion signature is accepted when it validates against any published certificate.Verification
mvn -pl oiosaml test→ 113 tests, 1 failure:OIOBPPUtilTest, which fails onmasteron this JDK and is fixed by #99 on its own branch.The new test builds metadata with a decoy signing certificate ahead of the real one and asserts the assertion still validates; restricting the loop to the first certificate makes it fail. Tests now deploy metadata as a temporary file instead of serving it from MockServer. The library and the demo war build clean.
Breaking changes
oiosaml.servlet.idp.metadata.urlandoiosaml.servlet.trust.selfsigned.certsare no longer supported; deployments using them fail at startup with a message naming the replacement.Configuration.Builder.setIdpMetadataUrl,Configuration.getIdpMetadataUrl/setIdpMetadataUrlandisSupportSelfSigned/setSupportSelfSignedare removed.IdPMetadata.getValidX509Certificate(UsageType)is replaced bygetValidX509Certificates(UsageType), and theIdPMetadataconstructor no longer takes a metadata URL.Worth an entry in
RELEASE_NOTES.mdalongside the other breaking changes in this round.Merge note
#95 (REF-28) calls
getValidX509Certificate(UsageType.SIGNING)inLogoutRequestService. Whichever of the two merges second needs that call moved to the plural form, and the logout path should try all published certificates for the same rotation reason.