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

improve handling of #WithComments #72

Merged

Conversation

toby-jn
Copy link

@toby-jn toby-jn commented May 26, 2021

I had some issues with gosaml2 when the IdP was using the http://www.w3.org/2001/10/xml-exc-c14n#WithComments canonicalization algorithm. According to the SAML 2.0 spec:

SAML implementations SHOULD use Exclusive Canonicalization, with or without comments

So these should be handled by goxmldsig in order to support SAML correctly.

I also noticed that the existing canonicalization algorithms were not removing comments when transforming the XML in the cases that didn't have #WithComments.

This change extends the existing canonicalizers to include a comments flag, which will strip comments if not set, and adds additional AlgorithmIDs for each of the #WithComments variants.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@toby-jn
Copy link
Author

toby-jn commented Aug 18, 2021

Hi @russellhaering any chance this could be considered for merge?

@gerritmaritz
Copy link

+1 Also facing issues with an IdP that is using the WithComments variant. I see there is another PR open so this is probably affecting several consumers: #74

@sahnib
Copy link

sahnib commented Sep 8, 2021

+1. Facing similar issues for IdP using WithComments variant.

@simonbrynych
Copy link

@russellhaering this PR seems good to me and it's definitely worth merging it. I'm struggling withComments too.

@tomstaijen
Copy link

+1

@lejeuneretif
Copy link

+1 That would help a ton of people.

@CoryBond
Copy link

+1

@alolita
Copy link

alolita commented Dec 8, 2021

@russellhaering please let us know how we can help you merge this PR? We are blocked on using this library and would greatly appreciate a response from you. Thanks in advance.

@russellhaering
Copy link
Owner

Thanks for your work on this, sorry it has taken me so (very) long to get around to looking at it!

@russellhaering russellhaering merged commit 9a278d1 into russellhaering:main Mar 2, 2022
@toby-jn
Copy link
Author

toby-jn commented Mar 3, 2022

no problem, thanks for taking the time to look 🙏

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.

None yet

9 participants