Skip to content

fix: improve C14N handling for inline and transformed references#98

Closed
leifj wants to merge 3 commits intomoov-io:masterfrom
sirosfoundation:pr/c14n-fixes
Closed

fix: improve C14N handling for inline and transformed references#98
leifj wants to merge 3 commits intomoov-io:masterfrom
sirosfoundation:pr/c14n-fixes

Conversation

@leifj
Copy link
Copy Markdown

@leifj leifj commented Mar 24, 2026

Summary

Fix XML Canonicalization (C14N) handling for both inline and transformed references, and pass InclusiveNamespaces from CanonicalizationMethod to ProcessElement.

Problem

XML signature validation was failing for various EU Trust Service Lists (TSLs) due to incorrect handling of namespace contexts during canonicalization:

  1. XAdES SignedProperties (no transforms): Elements were being extracted from their namespace context, losing inherited namespace declarations needed for proper C14N.
  2. References with C14N transforms (e.g., exclusive C14N): The C14N transform was not being applied to the element in its original document context, causing incorrect handling of inherited namespace declarations (e.g., Sweden TSL).
  3. References with non-C14N transforms only (e.g., enveloped-signature): Elements were not being properly extracted, incorrectly including inherited namespaces (e.g., root-level signature test case).
  4. InclusiveNamespaces in CanonicalizationMethod: When signing/validating with exc-c14n and InclusiveNamespaces PrefixList in the CanonicalizationMethod element, the transform content was not being passed to ProcessElement, causing signature verification mismatches.

Changes

  • Refactor validateReferences() and setDigest() to handle three cases: no transforms, C14N transforms (element-in-context), and non-C14N-only transforms (extract-then-hash)
  • Add ProcessElement method to ExclusiveCanonicalization for canonicalizing an element within its document context
  • Add calculateHashFromElement and calculateHashFromString helper functions
  • Add findReferencedElement method to locate elements without extraction
  • Extract and pass canonTransform (InclusiveNamespaces content) from CanonicalizationMethod to ProcessElement in both Signer and Validator
  • Preserve existing attributes when prepending namespace attributes in parseSignedInfo

Testing

Validated with:

  • Liechtenstein TSL (XAdES with no transforms on SignedProperties)
  • Sweden TSL (exclusive C14N transform)
  • France TSL (enveloped-signature + C14N)
  • Root-level signature test case (enveloped-signature only)
  • All existing tests continue to pass

Leif Johansson added 2 commits March 24, 2026 10:26
This commit addresses XML signature validation for various EU Trust Service Lists (TSLs):

1. XAdES SignedProperties (no transforms): Keep element in original namespace
   context for proper C14N, which ensures inherited namespaces are included.

2. References WITH c14n transforms (e.g., exclusive c14n): Apply the c14n
   transform to the element IN ITS CONTEXT using ProcessElement, which properly
   handles inherited namespace declarations (needed for Sweden TSL).

3. References with non-c14n transforms but NO explicit c14n transform (e.g.,
   only enveloped-signature): Extract the element to a new document before
   hashing, which correctly excludes inherited namespaces (needed for root-level
   signature test case).

Tested with:
- Liechtenstein TSL (XAdES with no transforms on SignedProperties)
- Sweden TSL (exclusive c14n transform)
- France TSL (enveloped-signature + c14n)
- Root-level signature test case (enveloped-signature only)
…lement

When signing/validating XML with exc-c14n and InclusiveNamespaces in the
CanonicalizationMethod element, the transform content (containing the
PrefixList) was not being passed to ProcessElement. This caused signature
verification to fail on the receiving end because the canonical form used
for signing differed from what the verifier computed.

Fix by:
1. Adding canonTransform field to signatureData struct
2. Extracting InclusiveNamespaces content in parseCanonAlgorithm()
3. Passing canonTransform to ProcessElement in both Signer and Validator
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses critical issues in XML signature validation related to incorrect handling of namespace contexts during canonicalization. By refining how XML elements are processed and hashed, especially concerning inherited namespaces and various transform types, it ensures robust and accurate validation for diverse XML digital signatures, including those found in EU Trust Service Lists (TSLs). The changes improve the library's compliance with XML Signature specifications, resolving validation failures for complex signature structures.

Highlights

  • Improved XML Canonicalization (C14N) Handling: Refactored validateReferences() and setDigest() to correctly handle three distinct cases for XML references: those with no transforms, those with C14N transforms, and those with only non-C14N transforms. This ensures proper namespace context is maintained during canonicalization.
  • New ProcessElement for ExclusiveCanonicalization: Added a ProcessElement method to the ExclusiveCanonicalization algorithm, allowing an element to be canonicalized within its original document context, which is crucial for preserving inherited namespace declarations.
  • Specialized Hash Calculation Functions: Introduced calculateHashFromElement and calculateHashFromString helper functions to address different scenarios for digest calculation, ensuring the correct canonical form is hashed based on whether the element is in context or if a canonical string has already been produced by transforms.
  • Element Location in Original Context: Added a findReferencedElement method to locate referenced elements without extracting them, which is vital for C14N transforms that require the element's original namespace context.
  • InclusiveNamespaces Support: Implemented extraction and passing of canonTransform (e.g., InclusiveNamespaces content) from CanonicalizationMethod to ProcessElement in both the Signer and Validator, enabling correct exclusive C14N with prefix lists.
  • Preservation of Attributes in parseSignedInfo: Modified parseSignedInfo to preserve existing attributes when prepending namespace attributes, preventing unintended data loss or modification.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request provides a significant and necessary fix for XML Canonicalization (C14N) handling, addressing several subtle but critical issues with namespace contexts during signature validation. The refactoring of validateReferences to handle different transform scenarios (no transforms, C14N transforms, and non-C14N transforms) is well-executed and correctly addresses the problems outlined in the description. The introduction of findReferencedElement and the changes to process elements within their original context are key improvements. My review includes a few suggestions to enhance robustness by handling potential errors and to improve maintainability by reducing code duplication.

Comment thread signedxml.go
Comment thread validator.go
Comment thread exclusivecanonicalization.go Outdated
Comment thread signedxml.go
leifj pushed a commit to sirosfoundation/signedxml that referenced this pull request Mar 24, 2026
Incorporates all review feedback from upstream PRs:
- PR moov-io#98 (c14n-fixes): WriteToString error handling, redundant assignment removal
- PR moov-io#99 (rsa-pss): getDigestAlgorithm helper, rsaPSSHashAlgorithms shared map
- PR moov-io#100 (ecdsa-fix): hash messages before ECDSA sign/verify in tests, nolint:exhaustive
- PR moov-io#101 (ed25519-xmlenc-cryptosigner): error handling in xmlenc parse functions,
  gosec nolint for safe integer conversions, NewSigner error check in tests,
  .gitleaks.toml for W3C test vectors

Also updates golang.org/x/crypto to v0.49.0.
@adamdecaf
Copy link
Copy Markdown
Member

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request significantly refactors the XML canonicalization logic to correctly handle namespace declarations, especially when elements are extracted or processed in different contexts. It introduces new functions like collectAncestorNamespaces, processDocumentWithAncestorNS, findReferencedElement, and several specialized calculateHash functions to ensure proper C14N canonicalization, particularly for inherited namespaces and XAdES signatures. The signatureData struct now includes canonTransform to pass specific canonicalization instructions. The validateReferences and setSignature methods were updated to utilize these new mechanisms. Additionally, a minor improvement was made to an error message in getCertFromPEMString to be more accurate.

Comment thread signedxml.go
@adamdecaf
Copy link
Copy Markdown
Member

These commits have been included in master. Thanks! I'll keep looking at your other PRs.

@adamdecaf adamdecaf closed this Mar 26, 2026
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.

2 participants