Skip to content
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

Include InclusiveNamespaces specified in CanonicalizationMethod when canonicalizing SignedInfo #178

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yamitenshi
Copy link

The current implementation ignores inclusive namespaces when canonicalizing the SignedInfo element. This pull request ensures that an InclusiveNamespaces element within the topmost CanonicalizationMethod element is also parsed and included in the SignedInfo canonicalization, preventing signature verification issues when the canonicalized XML would differ as a result.

This may fix #98, but I'm not 100% sure of that.

@robrichards
Copy link
Owner

@yamitenshi I am probably going to refactor this to a private method that processTransforms() can also call

@yamitenshi
Copy link
Author

I could add that to this PR - it's a good refactor, since my changes do introduce quite a bit of duplicate code.
What do you have in mind? Maybe a method that returns a list of inclusive namespace prefixes given a root node to search from?

@wildspitze
Copy link

wildspitze commented May 14, 2019

I had the same requirement and needed to consider the InclusiveNamespaces in the CanonicalizationMethod.
So I use @yamitenshi function canonicalizeSignedInfo(), which works well and verifies the signature successfully.
Would appreciate if the adjustments would be merged into the master-branch.

@yamitenshi
Copy link
Author

Oh <bad word I probably shouldn't use in a semi-professional setting>, I'd completely forgotten about this one...

I'd love to propose a refactor but I'm afraid I no longer have an XML file that requires this... I'm afraid the XML files I did have were for a previous client of mine and - even if I did still have them - were quite confidential. If you (or someone else, I suppose) could supply an example XML that fails to validate without my fix, I'd be happy to propose a better-refactored solution.

I'll try to come up with something myself, of course, but if there's an example ready, that'd simplify the whole thing tremendously.

@yamitenshi
Copy link
Author

Hang on, the issue I linked (#98) probably has one... Will likely be proposing a refactor in the coming days

evperedadiaz added a commit to evperedadiaz/XmlSecurity that referenced this pull request Mar 19, 2020
Include InclusiveNamespaces specified in CanonicalizationMethod when canonicalizing SignedInfo
ass/xmlsecurity: aschamberger#23
robrichards/xmlseclibs: robrichards/xmlseclibs#178
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.

Unable to validate Signature
3 participants