-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Remove signature nodes appropriately #46
Conversation
…le signatures in XML. Removal is also done so that any text after the signature is preserved
…gger the appropriate transformation paths
@@ -588,11 +612,14 @@ def verify(self, require_x509=True, x509_cert=None, ca_pem_file=None, ca_path=No | |||
root = fromstring(self.data, parser=parser) | |||
else: | |||
root = self.data | |||
c14n_root = fromstring(etree.tostring(root)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't truly understand why this is necessary. Can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we may need to apply transformation modifying the XML tree during the canonicalization process we need to make a copy of it to prevent modifying the original tree.
Originally I used copy.deepcopy but with lxml it failed at preserving the namespaces on the root node which broke some cases, thus I instead copy the tree in this way.
Thanks. This is outstanding work. I'm merging, but I'd appreciate it if you could answer the question I left inline. |
@kislyuk Answered, it's mostly a hack around copy.deepcopy not preserving unused namespaces on the root node. |
Released in v0.6.0. |
This solves the issue with double signatures in XML. Removal is also done so that any text after the signature is preserved.
I found this issue when trying to validate SAML 2.0 responses with a signature on both the assertion and the node signed with the library.
This should at least solve #40 but it may solve others like #44