Skip to content

Added option to exclude c14n tranformation from Transform XML node #104

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

vo-va
Copy link

@vo-va vo-va commented Feb 2, 2018

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

vo-va added 2 commits February 2, 2018 22:31
Some software can uses hard restrictions on Transform XML node, that prohibit include c14n transformation into Transform XML node.
@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

@vo-va vo-va force-pushed the master branch 7 times, most recently from a8dc0e5 to 02e538c Compare March 17, 2018 17:56
@vo-va vo-va force-pushed the master branch 4 times, most recently from 4a7ead8 to 8e2e234 Compare March 17, 2018 18:21
brew return error if package already installed - changed scripts to check package installation.
For python3 use venv module instead virtualenv
@kislyuk
Copy link
Member

kislyuk commented Mar 18, 2018

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
Copy link
Author

vo-va commented Mar 21, 2018

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
Copy link
Author

vo-va commented Mar 29, 2018

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
Copy link
Member

kislyuk commented Apr 2, 2018

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
Copy link
Contributor

bobdoah commented May 30, 2018

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

@kislyuk kislyuk force-pushed the master branch 4 times, most recently from 04b5d0e to c6b7dec Compare December 2, 2019 05:19
@kislyuk kislyuk force-pushed the master branch 2 times, most recently from 8f30f82 to 06e5f93 Compare June 8, 2020 02:15
@kislyuk kislyuk closed this Sep 12, 2020
@kislyuk kislyuk reopened this Sep 12, 2020
@kislyuk kislyuk changed the base branch from master to develop September 12, 2020 02:47
@kislyuk kislyuk force-pushed the develop branch 4 times, most recently from 7a5c538 to 6baf513 Compare August 20, 2022 06:52
@kislyuk kislyuk force-pushed the develop branch 6 times, most recently from 6b21a86 to 6879c98 Compare November 14, 2022 00:32
@kislyuk kislyuk force-pushed the develop branch 2 times, most recently from 9717621 to 82ae152 Compare November 27, 2022 22:11
@davidcolclazier
Copy link

davidcolclazier commented Oct 23, 2023

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
Copy link
Member

kislyuk commented Oct 25, 2023

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

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

Successfully merging this pull request may close these issues.

6 participants