Fix XMLSignatureUtil to omit KeyInfo when includeKeyInfoInSignature is false#48219
Open
ozimakov wants to merge 3 commits into
Open
Fix XMLSignatureUtil to omit KeyInfo when includeKeyInfoInSignature is false#48219ozimakov wants to merge 3 commits into
ozimakov wants to merge 3 commits into
Conversation
…s false When setIncludeKeyInfoInSignature(false) was called, signImpl() still invoked createKeyInfo(keyName, null, null), which produced a stub <KeyInfo><KeyName/></KeyInfo> element when keyName was non-null, and threw IllegalArgumentException when keyName was null (empty items list is rejected by the XML crypto API). The fix passes null directly to XMLSignatureFactory.newXMLSignature(), which is the correct way to produce a signature without a KeyInfo element per the javax.xml.crypto.dsig API contract. Closes keycloak#46302 Signed-off-by: Oleg Zimakov <oleg@zimakov.net>
2 tasks
Author
|
@stianst May I get a review for this one? Thx. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes XML signature generation so disabling KeyInfo produces a valid signature without a <KeyInfo> element.
Changes:
- Passes
nullKeyInfo when inclusion is disabled. - Adds regression tests for enabled, disabled, and null-key-name cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
XMLSignatureUtil.java |
Omits KeyInfo when configured. |
XMLSignatureUtilTest.java |
Covers KeyInfo inclusion and omission. |
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
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.
Summary
XMLSignatureUtil.signImpl()was not respecting theincludeKeyInfoInSignature=falseflag:keyNameis non-null: a stub<KeyInfo><KeyName/></KeyInfo>was still included in the signaturekeyNameis null: anIllegalArgumentExceptionwas thrown (content cannot be empty) because the XML crypto API rejects an emptyKeyInfoitems listThe fix passes
nulldirectly toXMLSignatureFactory.newXMLSignature(), which is the correct way to produce a signature without a<KeyInfo>element per thejavax.xml.crypto.dsigAPI contract and SAML spec section 4.5 (KeyInfo is optional).A new unit test (
XMLSignatureUtilTest) covers all three cases:includeKeyInfoInSignature=true) — KeyInfo is presentincludeKeyInfoInSignature=falsewith non-null keyName — KeyInfo is absentincludeKeyInfoInSignature=falsewith null keyName — no exception thrown, KeyInfo is absentCloses #46302