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

Fixed a bug in XML canonicalisation causing a digest mismatch on Okta… #51

Merged
merged 1 commit into from
Jun 25, 2023

Conversation

fumieval
Copy link
Contributor

… when attributes are present

When parsing a SAML response, it has been inappropriately stripping xmlns:xs="http://www.w3.org/2001/XMLSchema" attribute in saml2:Assertion. This was causing a discrepancy between Okta's digest and our digest (but only when AttributeStatement is present).

This change fixes the problem by setting psRetainNamespaces = True and adding "xs" to the list of allowed prefixes for c14n.

Special thanks to @hiroqn for figuring this out

Summary

Checklist

  • All definitions are documented with Haddock-style comments.
  • All exported definitions have @since annotations.
  • Code is formatted in line with the existing code.
  • The changelog has been updated.

Copy link
Owner

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for figuring this out! Looks good to me 🚀, just a couple of bits of documentation that can be improved.

src/Network/Wai/SAML2/Response.hs Show resolved Hide resolved
src/Network/Wai/SAML2/C14N.hs Outdated Show resolved Hide resolved
… when attributes are present

When parsing a SAML response, it has been inappropriately stripping `xmlns:xs="http://www.w3.org/2001/XMLSchema"` attribute in saml2:Assertion. This was causing a discrepancy between Okta's digest and our digest (but only when AttributeStatement is present).

This change fixes the problem by setting `psRetainNamespaces = True` and adding "xs" to the list of allowed prefixes for c14n.

Special thanks to @hiroqn for figuring this out
@fumieval
Copy link
Contributor Author

Thank you for reviewing. I updated documentation of those functions (also rebased the branch in order to fix a conflict).

@mbg mbg merged commit 88fdad9 into mbg:master Jun 25, 2023
6 checks passed
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

2 participants