Skip to content

feat: Support formatted SignatureValue and lenient verification #507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jlucaso1
Copy link

@jlucaso1 jlucaso1 commented Jul 8, 2025

Closes #486

This PR resolves an interoperability issue with XML signatures generated by some Java libraries (like javax.xml.crypto.dsig). These libraries often format the <ds:SignatureValue> with line breaks and carriage returns (&#xD;), which caused validation failures when checked with the current implementation.

This is addressed with two key changes:

  1. Flexible Signature Formatting: A new signatureValueFormatting option has been added to SignedXmlOptions. This allows developers to configure the line length and line endings to match the output of other systems, ensuring compatibility.

    const sig = new SignedXml({
      signatureValueFormatting: {
        lineLength: 76, // Or another desired length
        carriageReturn: true // to use \r\n line endings
      }
    });
  2. Resilient Signature Verification: The signature loading logic has been updated to strip all whitespace from the <ds:SignatureValue> during parsing. This ensures that validation succeeds regardless of how the signature is formatted (single-line vs. multi-line).

jlucaso1 added 2 commits July 8, 2025 11:19
Introduces options to format the SignatureValue and makes the
verification process more resilient to whitespace differences.

Some XML signature implementations, particularly in Java, format the
base64-encoded SignatureValue with line breaks and carriage returns
(e.g., at every 76th character). The previous implementation produced a
single-line value and expected the same during verification, leading to
interoperability failures.

This commit addresses the issue in two ways:
1. A new `signatureValueFormatting` option is added, allowing the
   caller to specify a line length and choose between LF and CRLF line
   endings for the generated signature.

2. The `loadSignature` method now strips all whitespace characters
   from the SignatureValue content, making verification resilient to
   any formatting differences.

Closes node-saml#486
@srd90
Copy link

srd90 commented Jul 8, 2025

This PR resolves an interoperability issue with XML signatures generated by some Java libraries (like javax.xml.crypto.dsig).

Out of curiosity: which other java libraries (other than javax.xml.crypto.dsig)?

The way I read (*) content of links (and transitive links) provided at #486 (comment) is that it was java side mistake/regression which is now taken care of. Affected versions listed at https://bugs.openjdk.org/browse/JDK-8264194 are 8u231, 11.0.4-oracle. E.g. Oracle java8 and java11 has reached end of public support ( https://endoflife.date/oracle-jdk ) and e.g. Corretto and Temurin provide only security updates to those ( https://endoflife.date/amazon-corretto and https://endoflife.date/eclipse-temurin ). Furthermore all of those are way past versions 8u231 and 11.0.4

At the time latest java8 seems to be 8u451 and latest java11 11.0.27

So, (if) things have settled down at java side why introduce workaround to xml-crypto side (i.e. more code to be maintained) especially if workaround is for something that is fixed/fixable at java side if those systems are kept running on top of up to date java8/java11/java... (if those systems provide security related services those should be running on top of latest supported 8/11/...)

(*) I am NOT saying that I am 100% sure that I read those correctly.

@srd90
Copy link

srd90 commented Jul 8, 2025

FWIW, seems that e.g. Keycloak fixed behavioral change of javax.xml.crypto.dsig's internals (Santuario's behavioral change) with this commit: keycloak/keycloak@977cc47 (and backport to 20.x: keycloak/keycloak@8438281 ). Read also commit message.

Have you asked from your counterpart whether it can set com.sun.org.apache.xml.internal.security.ignoreLineBreaks system property to true?

Interesting note at SANTUARIO-482
https://issues.apache.org/jira/browse/SANTUARIO-482?focusedCommentId=16381230&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16381230

@jlucaso1
Copy link
Author

jlucaso1 commented Jul 8, 2025

Thank you for the research.
Makes sense, adding signatureValueFormatting will increase complexity. What do you think about proceeding with just that focused change to the verification logic (loadSignature to strip all whitespace (/\s/g))? I think this change will be a small risk.

@cjbarth
Copy link
Contributor

cjbarth commented Jul 8, 2025

@jlucaso1 , I'm with @srd90 on this one. That is a lot of code to deal with what appears to be a straight-forward issue. Your proposal to simply remove whitespace seems a little more reasonable, but one has to wonder, why would there be whitespace in a signature that we trust? Is some library generating signatures with whitespace in them?

@srd90
Copy link

srd90 commented Jul 12, 2025

@jlucaso1 asked:

What do you think about proceeding with just that focused change to the verification logic (loadSignature to strip all whitespace (/\s/g))?

AFAIK xml-crypto hasn't had any issues with SignatureValues that are splitted to multiple lines as long as extra escaping has not been applied (AFAIK2: \s matched also linebreaks) so why would you add code to force those to oneliner (whats the "business case")?

E.g. xmlsec1 produces multiline SignatureValue (see example e.g. from this unrelated issue's comment: #212 (comment) ) and xml-crypto at least has been able to handle such SignatureValues (see this another comment from same unrelated issue #212 (comment) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difference from javax.xml.crypto.dsig and xml-crypto signature value
3 participants