BREAKING CHANGE: Verify the SAMLResponse based on the raw query string #85
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
waterkip
force-pushed
the
GH-redirect-verify_fixes
branch
9 times, most recently
from
2c2d05f to
61fc4c8
Compare
waterkip
force-pushed
the
GH-redirect-verify_fixes
branch
2 times, most recently
from
540faa1 to
7aae8ce
Compare
BREAKING CHANGE: The verify function now requires the *raw* query string from
the server in order to verify the signature.
Because `URI` parses and re-encodes URI-escapes in uppercase (`%3f` becomes
`%3F`, for instance), which leads to signature verification failures if the
other party uses lower case (or mixed case). This seems to have been an issue
with the current code base as well due to the various flags that need to be
supplied to the constructor to work around it.
The problem lies with the RFC 3986[^1] and the SAML specs[^2] that both offer
different implementations of the same thing. RFC 3986 states that say the URI
must be normalized so uppercasing %2f to %2F is correct behavior. The SAML
specs states that we must operate on the original URL-encoded values. Thus no
uppercasing is allowed. It is the author's opinion that Net::SAML2 should
follow the SAML spec and and that implementations that normalize the URI should
return the original URI to the application.
This code is mostly ported from Zaaksysteem's code base, written by MSTREEK.
Users of lighttpd need to be aware that they need to configure their instance
with the following http-parseopts:
server.http-parseopts = (
"url-normalize" => "disable",
"url-normalize-unreserved" => "disable",
"url-normalize-required" => "disable"
)
You cannot change it on a URI basis because lighttpd needs a parsed URI before
it can process conditions as was mentioned on #lighttpd:
> The request must be parsed (using server.http-parseopts settings) *before* you
> can apply lighttpd.conf conditions to the parsed results.
[^1]: https://www.rfc-editor.org/rfc/rfc3986#section-6.2.2.1
[^2]: https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf
(line 620)
Signed-off-by: Wesley Schwengle <waterkip@cpan.org>
waterkip
force-pushed
the
GH-redirect-verify_fixes
branch
from
7aae8ce to
f43727d
Compare
timlegge
approved these changes
Aug 19, 2022
Contributor
timlegge
left a comment
timlegge
left a comment
There was a problem hiding this comment.
Approved - The binding redirect is only used for Logout so the change is of limited risk and impact should there be issues. May need to implement a method to grab the real Query URL similar to how onelogin does theirs in the future.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: The verify function now requires the raw query string from
the server in order to verify the signature.
Because
URIparses and re-encodes URI-escapes in uppercase (%3fbecomes%3F, for instance), which leads to signature verification failures if theother party uses lower case (or mixed case). This seems to have been an issue
with the current code base as well due to the various flags that need to be
supplied to the constructor to work around it.
This code is mostly ported from Zaaksysteem's code base, written by MSTREEK.
Signed-off-by: Wesley Schwengle waterkip@cpan.org