xml-crypto
xml-crypto copied to clipboard
New line and whitespace normalisation in canonicalisation operation
I have and error related to unmatching digests. After debugging some signature validation errors on SAML responses, I would like to discuss with you a couple of questions.
The SAML response we have is like this:
<bar>\r\n
<foo>Something Here</foo>\r\n
</bar>\r\n
In the Canonical process the \r
chars are replaced by 
( see this function: https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L66)
This code will modify the SAML response generating this output:
<bar>
\n
<foo>Something Here</foo>
\n
</bar>
\n
The digest is calculated on this modified XML which generates a digest different from the one in the original SAML response. The consequence is the validation process fails.
I have a couple of questions:
- This reference https://www.w3.org/TR/xml-c14n/#ProcessingModel says “all
#xD
characters are replaced by
”. So why you are replacing\r
with
in this line https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L53 - This code
.replace(/\r\n?/g, '\n')
was removed in this PR https://github.com/yaronn/xml-crypto/pull/196/files - but it looks to me it’s the right code in order to prevent the issue I have being having and it will make the operation to calculate the digest be successful. Can this code be re-introduced? Otherwise what should be the right approach to follow?
- This reference https://www.w3.org/TR/xml-c14n/#ProcessingModel says “all
#xD
characters are replaced by
”. So why you are replacing\r
with
in this line https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L53
Unless I'm missing something, that replacement is only for Attribute Nodes.
- Otherwise what should be the right approach to follow?
Have you looked at #195 for thoughts?
We'd welcome a PR that includes some tests of what the problem is. It would then be easier to determine what the appropriate fix would be. Having said that, you might also want to look at #286 where we have several broken tests. Perhaps one of those addresses your issue and you could submit a PR to fix the test.