Added option to exclude c14n tranformation from Transform XML node

[vo-va]

Some software can uses hard restrictions on Transform XML node, that prohibit include c14n transformation into Transform XML node.

Pull request to resolve issue #43

[codecov-io]

Codecov Report

Merging #104 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #104      +/-   ##
==========================================
+ Coverage   94.89%   94.91%   +0.01%     
==========================================
  Files           3        3              
  Lines         588      590       +2     
==========================================
+ Hits          558      560       +2     
  Misses         30       30

Impacted Files	Coverage Δ
signxml/__init__.py	94.65% <100%> (+0.02%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e5bf2c...1bdef2d. Read the comment docs.

[kislyuk]

Thank you for your contribution. Generally, I am unwilling to add options to the main codebase that result in signatures that don't pass validation by other XML Signature implementations (such as https://github.com/lsh123/xmlsec and https://github.com/yaronn/xml-crypto). In this case I don't have enough information to gather whether or not that is the case, so I have to ask you to give me some details about which XML signature implementations are able to verify the resulting signature. Similarly, I'd like to understand whether setting this option to False results in a signature that complies with the XML Signature standard and passes validation with the XML Signature schema (not Microsoft's custom schema). Can you provide information on both of these questions?

Aside from that, there are a couple of practical issues with the PR:

• Tests are required that exercise the option that you introduced.
• The docstring contains a few grammatical errors, it would be great to correct them.

This library is built to be extensible. Even if this option is not introduced into the library, you have the option of subclassing the XMLSigner class and overloading the _build_sig method.

[vo-va]

There is a project https://github.com/vo-va/signxml-test/tree/master with Docker container that verify signed document with xmlsec and xml-crypto. I used xmlsec1 util and example from xml-crypto docs to verify signature.

Similarly, I'd like to understand whether setting this option to False results in a signature that complies with the XML Signature standard and passes validation with the XML Signature schema (not Microsoft's custom schema).

If I understand correctly xmlsign tests verify signed document with this scheme https://github.com/XML-Security/signxml/blob/master/signxml/schemas/xmldsig-core-schema.xsd
I changed tests code to run test for cases with option in both states, True or False. Therefore I think signature without c14n transformation complies with standard xsd.

Also standard scheme is not limit amount of transformation https://github.com/XML-Security/signxml/blob/master/signxml/schemas/xmldsig-core-schema.xsd#L117

I am not sure what my changes will test all cases. Can you look at my changes and say is it enough or not?

[vo-va]

Hi @kislyuk, I do not want to bother you, but can you review pull request and say me your verdict? Reject/Accept? Or maybe I should change something?

[kislyuk]

Thank you for your patience. I want you to know that I haven't forgotten about this PR, but currently have no time to review it (and the nature of the changes is such that this requires a substantial review). I'll get back to this ASAP, hopefully sometime around next weekend.

[bobdoah]

Any chance of getting this merged? I too am hitting the same issue.

[kislyuk]

Thank you for your work on this PR so far, @vo-va, and sorry about the delays in handling this.

I don't currently use software that is incompatible with the way SignXML produces signatures. It's clear that there is demand for fixing the underlying issue here, which as far as I know is that SignXML only produces "Compatibility Mode" signatures, and not "2.0" signatures (thank you @tjeb for pointing toward that fact). I have described the issue in further detail in #142.

With apologies for not having had time to look at this issue in this level of detail sooner, I must ask that someone familiar with these applications write a principled solution for #142, with a separate test case, if you want this issue to be resolved.

I'm going to leave this PR open as a reference, but it has major issues that prevent it from being merged. The PR:

• does not document the underlying incompatibility in sufficient detail
• does not reference software or test cases that demonstrate the incompatibility
• is too vague in the documentation that it does provide
• solves the problem in too narrow a way
• disrupts the existing test cases unnecessarily (the test for this should ideally be a new, separate test case)

A solution to this issue should address these problems.

Again, apologies for the delays in handling this, but I am one person maintaining this library in my spare time outside my line of work, and it carries serious security requirements. Handling issues in this library requires substantial time, focus and clarity in understanding the underlying problems, given the issues in the underlying standard and the diversity of software interfacing with it. If you want to help, please contribute clear, maintainer-friendly solutions.

[davidcolclazier]

Might I inquire as to the current progress for this PR? Are you still looking for a more robust PR, or have you already implemented this?

[kislyuk]

@davidcolclazier please see #104 (comment) - PRs that address the points that I made in that comment are welcome.