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

Never modify original document #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tvdijen
Copy link
Contributor

@tvdijen tvdijen commented Jan 12, 2021

HI @robrichards !

I noticed that this method manipulates the original document that holds the sigNode and removes the entire signature from the bigger XML document.. This PR should fix it!

@tvdijen tvdijen changed the title Never modify original input Never modify original document Jan 12, 2021
@tvdijen
Copy link
Contributor Author

tvdijen commented Jan 12, 2021

Since Travis has cut us all off: The tests still pass!

[saml@webapp-10 xmlseclibs]$ vendor/bin/phpunit tests
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

............................ 28 / 28 (100%)

Time: 1.06 seconds, Memory: 4.00 MB

@robrichards
Copy link
Owner

@tvdijen Was about to review but saw its now closed. Is the original work still in progress (which I would try to monitor progress so speed up future review) or is it abandoned?

@tvdijen
Copy link
Contributor Author

tvdijen commented Jan 19, 2021

@robrichards It didn't really solve my problem in the end, so I figured I'd close it..

Perhaps you can explain to me why and under what circumstances it is necessary to remove the signature in the XMLSecuritySDig::validateReference-method?
My issue is that I have a signed DOMElement, I run validateReference() on it, and after that my original DOMElement has lost it's ds:Signature structure.

I may have referred to an irrelevant PR.. The more relevant part of the code is here;
https://github.com/simplesamlphp/xml-security/blob/master/src/XML/ds/Signature.php#L144-L178

It would definitely not hurt to merge this because it makes things a bit more readable and PSR-comliant, so I guess I can reopen it! Would be great if you can give your thought on the issue described

@tvdijen tvdijen reopened this Jan 19, 2021
@robrichards
Copy link
Owner

@tvdijen I can't recall the exact reason but vaguely remember some issue I was having early on which is why the sig got removed. Possibly being so early on it was only supporting some specific cases at the time. I will need to dig into it a bit more to see if I can jog my memory

@tvdijen
Copy link
Contributor Author

tvdijen commented Mar 24, 2021

Well, at some point you have to figure out the document that the signature was calculated over... With an enveloped signature, it makes sense to extract the sig, recalculate the sig on the refnode and compare the two.. It should probably be just like that, but it would be great if it didn't modify the original document, because I need it at a later point with the signature intact.

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.

2 participants