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

shouldn't the XML be canonicalized before calculating the digest/hash value? #21

Open
dcu opened this issue Aug 25, 2021 · 8 comments
Open
Labels

Comments

@dcu
Copy link

dcu commented Aug 25, 2021

from what I see in the code it is only canonicalized when calculating the signature

@ma314smith
Copy link
Collaborator

What you're saying makes sense, but its been quite awhile since I've committed to or used this library. So, I can't recall all the details. Its possible that the tests and my use cases at the time didn't catch a potential issue here.

Its unlikely that I'll be able to spend much time looking into this, but I'd be happy to accept a PR that addresses your use cases/concerns.

@dcu
Copy link
Author

dcu commented Aug 27, 2021

I've been looking at this but I still don't have a solution, the transform block is supposed not canonicalize the block but apparently it isn't doing it.
I'll try to continue investigating on weekend

@dcu
Copy link
Author

dcu commented Aug 29, 2021

so after commenting out this if https://github.com/ma314smith/signedxml/blob/master/exclusivecanonicalization.go#L199 requests to SOAP servers work, at least in my case.
but that condition should be there for a reason so I'm not sure how to proceed

I'm comparing the signature procedure against:

xmlsec1 --sign --store-signatures --store-references --print-debug --privkey-pem priv.pem --id-attr:Id Body example3.xml

actually xmlsec creates an attribute with the namespace, signedxml do not creates it and if you add it to the original document it is removed

Another issue I had was the id attribute when finding a referenced section is case sensitive, must be ID

@ma314smith
Copy link
Collaborator

That might be an issue with your InclusiveNamespaces or it might be a bug. Its hard to say without the xml.
https://www.w3.org/TR/xml-exc-c14n/

namespace nodes that are not on the InclusiveNamespaces PrefixList are expressed only in start tags where they are visible and if they are not in effect from an output ancestor of that tag.

This was contributed awhile back which may help with your reference issue:
https://github.com/ma314smith/signedxml#using-a-custom-reference-id-attribute

@dcu
Copy link
Author

dcu commented Sep 3, 2021

I tried adding inclusive namespaces but it didn't work and xmlsec which is what everybody uses doesn't need it, it automatically adds the missing namespace

@adamdecaf
Copy link
Member

@dcu Are you still seeing this issue with the v1.0.0 release of signedxml?

@dcu
Copy link
Author

dcu commented Apr 21, 2023

I haven't tested but my forked repo works at least for my case dcu/signedxml

@adamdecaf
Copy link
Member

adamdecaf commented May 8, 2023

@dcu I see a small change as the only difference between moov-io/signedxml and dcu/signedxml. Could you try moov-io and see if we've fixed the issue? It may be a small change/fix needed otherwise.

Diff: master...dcu:signedxml:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants