Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove namespace-breaking reserialization of signature from example in README #105

Merged
merged 1 commit into from
Apr 11, 2016
Merged

Remove namespace-breaking reserialization of signature from example in README #105

merged 1 commit into from
Apr 11, 2016

Conversation

brownstein
Copy link
Contributor

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.

<samlp:Response ... xmlns:dsig="http://www.w3.org/2000/09/xmldsig#">
  ...
    <dsig:Signature>
      <dsig:SignedInfo>
        <dsig:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
        <dsig:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
        <dsig:Reference URI="#id-2f7VgU-0XBjCqhLN2NWJyb-e5F2gWKOV9KT-c1nD">
          <dsig:Transforms>
            <dsig:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
            <dsig:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
          </dsig:Transforms>
          <dsig:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
          <dsig:DigestValue>...</dsig:DigestValue>
        </dsig:Reference>
      </dsig:SignedInfo>
     <dsig:SignatureValue>...</dsig:SignatureValue>
    </dsig:Signature>
  ...
</samlp:Response>

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.

@bjrmatos bjrmatos merged commit 3142b39 into node-saml:master Apr 11, 2016
@bjrmatos
Copy link
Contributor

thnk you! you're right toString() is considered harmful.. if you find other examples using it please send a PR 😄

@yaronn
Copy link
Contributor

yaronn commented Apr 11, 2016

@brownstein - sorry to hear it wasted you a few days to chase this (been there...) but so great that you came back with this PR!

cmordue added a commit to cmordue/saml20 that referenced this pull request Dec 13, 2017
…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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants