Modified loadSignature to only check local name and not namespace name

[brianhartsock]

Loading the signature for XML documents that specify the signature namespace 'http://www.w3.org/2000/09/xmldsig#' on the root element of the document doesn't work because loadSignature only has the signature node (as XML), and therefore doesn't have reference to it's namespace.

For example:

<root xmlns:ns1="http://www.w3.org/2000/09/xmldsig#">
...
  <ns1:Signature>

This will effectively find the Signature element, but then break finding any Reference elements inside of the loadSignature function.

The other way to handle this is to change loadSignature to accept an XML DOM Node that has reference to the document instead of just a string, however this is obviously a big breaking change.

I'm no XML expert so I could be missing something, but the scenario where the NS for signature is defined on the root element is happening to me when working with a few SAML implementations.

[bjrmatos]

i think loadSignature should accept a xml DOM Node, or at least let specify the namespace of the signature as another parameter (or maybe a options object?).

@yaronn could you explain why loadSignature can not take the whole xml and search for the signature node?

[brianhartsock]

@bjrmatos I think you are probably right. Take the following example:

sig.loadSignature(xmlString)
//at this point, sig.signatureXmlDoc doesn't have reference to the namespaces declared on the root element
sig.checkSignature(xmlString)
//now we will see a failure in validateSignatureValue because getCanonXml can't find the root namespace.

[brianhartsock]

See f0d4b45 for an implementation that passes a node object (and still accepts string).

[yaronn]

Thanks @brianhartsock, I've pushed it to npm.

@bjrmatos - initially the concept was that xml-crypto should be xml library agnostic, so I did not want to rely on any specific xmldom objects. Maybe it was a mistake since it cause a performance hit and also some semantic issues like this.

[bjrmatos]

maybe we can make this more fun :)

this is my proposal:

maybe we can get rid of sig.loadSignature (at least from the public API) and only use sig.checkSignature -> sig.checkSignature(xmlDocument, [externalSignatureXmlDocument])

we will always search for an enveloped signature in xmlDocument, with this we can bring it back the condition: //*[local-name(.)='SignedInfo']/*[local-name(.)='Reference' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#'] for ensure a well formed xml document, and it will be ok since we have all the information that we need (namespaces) because we have the whole xml document.

we will only use the last parameter (externalSignatureXmlDocument) when the xmlDocument does not have the signature inside it (external xml signature), i think both xmlDocument and externalSignatureXmlDocument can accept a xml string or a xml node.

what do you think? i can send a PR :)

[bjrmatos]

obviously, this will be a breaking change

[yaronn]

it's a good idea though will break a lot of code, tests and documentation. if you have time you can try it and see if it is risky or not.

[brianhartsock]

@bjrmatos I am obviously a fan of sending objects that somehow have reference to the entire XML document, otherwise it is easy to accidentally lose XML NS info as this PR solved.

The issue I have is that it is unclear clear what is signed when passing the xml document in. An XML document can have numerous signatures on different nodes. If that's the case, what does checkSignature actually check? Which parts of the document are signed and can be trusted, what parts aren't?

[brianhartsock]

@yaronn I can open a PR for f0d4b45 if you think that approach is right.

[bjrmatos]

right now, i think checkSignature is designed to handle only one signature, via loadSignature.

the API is not designed for a document with multilpe <Signature /> nodes, we should fix that :(

with my proposal sig.checkSignature(xmlDocument, [externalSignatureXmlDocument]) i think this will be easy to resolve.. first we need to update checkSignature to find all instances of <Signature />, by default it will search inside the document -> xmlDocument param, if externalSignatureXmlDocument is specified it will search inside the external document.

For all instances of <Signature /> we'll call the internal process of loadSignature and validate all the instances accordingly.

obviously we must refactor the function a little bit to handle the case, but this is the general idea :)

[yaronn]

@brianhartsock @bjrmatos

if I understand correctly you are raising two issues:

1. checkSignature receives a string rather than xml, which is a problem when there are ambient namespaces (and possibly performance and aesthetic problem as well.)
2. checkSignature will not work when there are multiple signatures

currently issue 1 is solved with this PR (with a risk that some unrelated Signature node will be captured) and issue 2 requires end user to iterate over the signatures and validate each one.

I'm ok with adding checkSignature support for xmldom inputs as long as it does not break current api (e.g. string inputs should also be accepted).

I agree the current API does not fit multiple signatures since you need to explicitly load a single signature and validate it. Changing this behavior for checkSignature might cause unknown regressions. But we can add a new API function checkAllSignatures to do it.

[bjrmatos]

i really think loadSignature must be deprecated in favor of a new checkSignature behaviour. This will be a breaking change but with a more compact API and new features.

Maybe this is a good excuse for a 1.0 version, i think we should define milestones and see how it goes.

New Proposal:

• deprecate loadSignature, it will require some work but i think it will be for a good reason.
• letting the user to specify the signature manually is risky (possibly lost namespaces, it requires the user to parse the xml document and search for the signature which affects performance), maybe we can deprecate that and let xml-crypto to do this work.
• checkSignature will search and validate only the first signature found (i dont know if you would like to support letting the user to validate a specific signature, we can do it vía a xmlPath expression).
• checkAllSignatures will validate all signatures found, the behavior would be the same as described in my previous comment. (by default it will search inside the document for all instances of <Signature /> and validate all instances accordingly. if externalSignatureXmlDocument is specified it will search inside the external document).
• both checkSignature and checkAllSignatures must support the externalSignatureXmlDocument param for external signatures. -> checkSignature(xmlDocument,[externalSignatureXmlDocument]) and checkAllSignatures(xmlDocument,[externalSignatureXmlDocument])

the user wont need to specify the signature manually, xml-crypto will do it and will support multiple signature validation.

[yaronn]

@bjrmatos - I like it. this sounds like a great a candidate for version 1.0.0. Is this something you have the time to start implementing?

the only concern is about exposing xmldom as a direct api type (rather than string). maybe if someone is using other xml parsers it would seem strange we force them to use our parser. but maybe in the future we can be parser agnostic or support a string overload, so not a big issue.

[bjrmatos]

yes i can start it :)

maybe i was not clear enough, but my idea is that both checkSignature and checkAllSignatures only accept strings representing the xml documents, checkSignature(String: xmlDocument, [String: externalSignatureXmlDocument]), since xml-crypto will do the work of search for the signatures, it won't require the user to pass a DOM node, the user only has to pass the xml document and optionally the signature xml document for external signatures if is the case.

Also, do you like the idea to letting the user to validate a specific signature?, as i mentioned we can do it vía a xmlPath expression so it won't require the user to parse anything :)

[yaronn]

got it, sounds good.

will loadSignature still work and output a deprecation message or will it throw an exception and not work at all?

letting validate a specific signature is nice to have but not a must for the first release. users that wish that can strip out manually the other signatures.

[bjrmatos]

since the target of these changes is a 1.0 version i think we should just deprecate the method and remove it from the public API (it will be used internally).

is a major version change, i think we won't break anyone's codebase.. only people installing the module with xml-crypto: *, which is a bad practice at all in the node.js comunity.

i will work on this very soon :)

thnks for your time!

[yaronn]

👍 sounds good!

[brianhartsock]

@bjrmatos how would you know what parts of the xml document are signed? That's my concern. Take the following:

<root>
  <node1>
     <Signature>...</Signature
     <data>...</data>
  </node1>
  <node2>
    <data>...</data>
  </node2>
</root>

In this scenario, the caller would know what parts of the document are signed and should be trusted vs. what parts should be ignored. In the case of SAML, this could be incredibly dangerous, as a signed assertion should be trusted and an unsigned one shouldn't be. A caller could easily call checkSignature thinking it validates the whole document, which isn't necessarily true.

Maybe I don't understand your plans for externalSignatureDocument

[bjrmatos]

how would you know what parts of the xml document are signed? That's my concern. Take the following

A <Signature> contains <Reference> node(s) which points to the parts of the document that was signed via its URI attribute -> <Reference URI="#myInfo">. As the digital signature specification says, if a URI with an empty value is found -> URI="" it means that the whole document was signed.

externalSignatureDocument is there to support external signatures (when the signature is not present in the original xml document that was signed), when externalSignatureDocument is specified xml-crypto will search <Signature> nodes inside the external document. checkSignature will validate only the first signature found (maybe with the possibility to pass a xpath expression referencing to a specific signature to validate), and checkAllSignatures will validate all signatures found.