Fix/49019 dsig to ds prefix by JoWe112 · Pull Request #49033 · keycloak/keycloak

JoWe112 · 2026-05-16T12:46:32Z

Keycloak uses dsig: as the XML Digital Signature namespace prefix in SAML metadata and signed documents. While technically valid (XML namespaces are URI-based, not prefix-based), the conventional prefix used by virtually all SAML implementations is ds:.

This PR changes the hardcoded "dsig" string in XMLSignatureUtil.signImpl() to use the existing WSTrustConstants.XMLDSig.DSIG_PREFIX constant (which equals "ds").

Changes
XMLSignatureUtil.java — use WSTrustConstants.XMLDSig.DSIG_PREFIX instead of hardcoded "dsig"
Updated three integration tests that matched against the literal prefix in generated XML output
Notes
The namespace URI (http://www.w3.org/2000/09/xmldsig#) is unchanged
All namespace-URI-based processing (parsing, validation) is unaffected
XML test fixtures in saml-core/src/test/resources/ intentionally keep the dsig: prefix to verify the parser remains prefix-agnostic
Closes #49019

The XML Digital Signature namespace prefix is changed from the non-standard "dsig" to the conventional "ds" used by virtually all SAML implementations. Uses the existing WSTrustConstants constant instead of a hardcoded string. Closes keycloak#49019 Signed-off-by: JW <joakim.westlund@id-north.com>

mposolda

@pskopek Do you please have a chance to review?

For me, it seems that the file saml-dsig-to-ds-prefix.patch should be removed from this patch.

pskopek

@JoWe112 thanks for the PR. See the comment for the patch file.

I would like to see the same change for SAML Adapter (SamlDescriptorIDPKeysExtractor) and SAML IdP (SAMLIdentityProviderFactory).
Can you update your PR, please?

Update SamlDescriptorIDPKeysExtractor to use "ds" as the XPath namespace context prefix instead of "dsig". Fix SAMLIdentityProviderFactory to use the proper namespace URI via JBossSAMLURIConstants.XMLDSIG_NSURI instead of incorrectly passing "dsig" as the namespace URI in the QName constructor. Signed-off-by: JW <joakim.westlund@id-north.com>

Copilot

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Updates Keycloak’s generated XML Digital Signature prefix from the non-standard dsig: to the conventional ds: to improve interoperability and align with common SAML ecosystem expectations.

Changes:

Switch XML signature generation to use WSTrustConstants.XMLDSig.DSIG_PREFIX ("ds") instead of hardcoded "dsig".
Update integration tests that asserted the literal dsig: prefix to now expect ds:.
Align some XML parsing/manipulation code to the new prefix usage.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/SamlConsentTest.java	Updates string assertion to expect `<ds:Signature` in generated SAML response XML.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java	Updates DOM element lookup to `ds:SignatureValue`.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/adapter/servlet/SAMLServletAdapterTest.java	Updates element-removal path to use `ds:` segments.
services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java	Changes child element lookup to use an XMLDSIG namespace-aware `QName` with `ds` prefix.
saml-core/src/main/java/org/keycloak/saml/processing/core/util/XMLSignatureUtil.java	Switches default signature namespace prefix to `WSTrustConstants.XMLDSig.DSIG_PREFIX`.
adapters/saml/core/src/main/java/org/keycloak/adapters/saml/descriptor/parsers/SamlDescriptorIDPKeysExtractor.java	Updates namespace context mapping key from `dsig` to `ds`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

                    for (KeyDescriptorType keyDescriptorType : keyDescriptor) {
                        Element keyInfo = keyDescriptorType.getKeyInfo();
-                        Element x509KeyInfo = DocumentUtil.getChildElement(keyInfo, new QName("dsig", "X509Certificate"));
+                        Element x509KeyInfo = DocumentUtil.getChildElement(keyInfo, new QName(JBossSAMLURIConstants.XMLDSIG_NSURI.get(), "X509Certificate", "ds"));


+              .getElementsByTagName("ds:SignatureValue")
              .item(0).getTextContent();


        log.debug("Removing KeyInfo from Keycloak response");
        Document responseDoc = documentHolder.getSamlDocument();
-        IOUtil.removeElementFromDoc(responseDoc, "samlp:Response/dsig:Signature/dsig:KeyInfo");
+        IOUtil.removeElementFromDoc(responseDoc, "samlp:Response/ds:Signature/ds:KeyInfo");


    static {
        NS_CONTEXT.addNsUriPair("m", JBossSAMLURIConstants.METADATA_NSURI.get());
-        NS_CONTEXT.addNsUriPair("dsig", JBossSAMLURIConstants.XMLDSIG_NSURI.get());
+        NS_CONTEXT.addNsUriPair("ds", JBossSAMLURIConstants.XMLDSIG_NSURI.get());


Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/saml/BasicSamlTest.java:347

getElementsByTagName("ds:SignatureValue") is prefix-sensitive and can be brittle; this test is validating allowed characters in the SignatureValue content and shouldn’t depend on the chosen namespace prefix. Prefer a namespace-aware lookup (e.g., getElementsByTagNameNS with the XMLDSIG namespace URI and local name SignatureValue) so the test remains correct even if the prefix changes again.

            final String signature = documentHolder.getSamlDocument()
              .getElementsByTagName("ds:SignatureValue")
              .item(0).getTextContent();

services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProviderFactory.java:135

The QName passed here hardcodes the "ds" prefix even though prefix is not semantically meaningful and can vary across documents. If DocumentUtil.getChildElement matches by namespace URI + local name (typical behavior), the prefix is unnecessary; using the 2-arg QName(namespaceURI, localPart) (or otherwise avoiding a fixed prefix) keeps parsing strictly namespace-based and reduces confusion.

                        Element x509KeyInfo = DocumentUtil.getChildElement(keyInfo, new QName(JBossSAMLURIConstants.XMLDSIG_NSURI.get(), "X509Certificate", "ds"));

adapters/saml/core/src/main/java/org/keycloak/adapters/saml/descriptor/parsers/SamlDescriptorIDPKeysExtractor.java:56

Consider also keeping an alias mapping for "dsig" (in addition to "ds") to avoid churn/breakage if any existing XPath expressions (or future maintenance) still reference the old prefix within this class/module. Adding both mappings is low-cost and makes the namespace context more tolerant without affecting namespace-URI correctness.

        NS_CONTEXT.addNsUriPair("m", JBossSAMLURIConstants.METADATA_NSURI.get());
        NS_CONTEXT.addNsUriPair("ds", JBossSAMLURIConstants.XMLDSIG_NSURI.get());

JoWe112 requested review from a team as code owners May 16, 2026 12:46

keycloak-github-bot Bot added team/cloud-native team/core-clients team/core-iam labels May 16, 2026

stianst added team/core-protocols and removed team/cloud-native team/core-clients team/core-iam labels May 21, 2026

mposolda assigned pskopek Jun 5, 2026

mposolda reviewed Jun 5, 2026

View reviewed changes

pskopek requested changes Jun 5, 2026

View reviewed changes

Comment thread saml-dsig-to-ds-prefix.patch Outdated

JoWe112 force-pushed the fix/49019-dsig-to-ds-prefix branch from a86863a to 1c085f3 Compare June 5, 2026 13:41

Merge branch 'main' into fix/49019-dsig-to-ds-prefix

7a67f3c

Copilot AI balanced review requested due to automatic review settings August 11, 2026 17:14

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Merge branch 'main' into fix/49019-dsig-to-ds-prefix

4f21d46

JoWe112 requested review from Copilot and pskopek August 11, 2026 17:39

Copilot started reviewing on behalf of JoWe112 August 11, 2026 17:54 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

Copilot started reviewing on behalf of JoWe112 August 11, 2026 18:11 View session

Navigation Menu

Uh oh!

Fix/49019 dsig to ds prefix - #49033

Fix/49019 dsig to ds prefix#49033
JoWe112 wants to merge 4 commits into
keycloak:mainfrom
JoWe112:fix/49019-dsig-to-ds-prefix

JoWe112 commented May 16, 2026

Uh oh!

mposolda left a comment

Uh oh!

pskopek left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		.getElementsByTagName("ds:SignatureValue")
		.item(0).getTextContent();

Uh oh!

Conversation

JoWe112 commented May 16, 2026

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

pskopek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants