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

Fix issue where SAML assertion validation fails if namespace is not defined on Signature node. #20

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

Conversation

cmordue
Copy link

@cmordue cmordue commented Dec 13, 2017

Remove namespace-breaking reserialization of signature which used to be in the documented example from xml-crypto but was removed due to this bug

See: node-saml/xml-crypto#105

This is a fix for the example signature validation documentation; the toString() call on the signature node turns out to be harmful, as it removes namespace metadata which would otherwise propagate from the parent document. In situations where the namespace is ds, this works out ok (as ds is provided as a default) but when using other strings for this namespace the example boilerplate fails.

In the example below, the definition of dsig would be undefined in a toString-orphaned signature, and the canonicalization algorithm would resolve the url as an empty string -- changing the underlying canonical text and causing the SignatureValue verification to fail.

PR fixes this in the example, which realistically will get used as boilerplate in many integrations of this library. Having just spent three days chasing down the cause of one of our SAML integration failures, I think its a useful change.

…efined on Signature node.

Remove namespace-breaking reserialization of signature which used to be in the documented example from xml-crypto but was removed due to this bug
See: node-saml/xml-crypto#105
@chrisgarber
Copy link

Can we get this PR merged? This is causing errors for some saml implementations at our org

chrisgarber added a commit to abacuslabs/saml20 that referenced this pull request Jul 13, 2020
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.

3 participants