Add an option to use query string for validation #256
Merged
Conversation
This file contains 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
eltoder
force-pushed
the
feature/validate-signature-query-string
branch
from
4e6a8e8
to
d32d525
Compare
eltoder
changed the title
|
Can we add a new flag 'validate_signature_from_qs' by default to False to keep backward compatibility and in case of enabling it |
eltoder
force-pushed
the
feature/validate-signature-query-string
branch
from
d32d525
to
19e9d32
Compare
|
@pitbulk Added the flag, though note that you can get the old behavior simply by not passing query_string. AFAICT there's no reason to pass query_string for SLO requests otherwise. |
|
Can you document this new behavior on the Readme? |
When validating request or response signature in process_slo() we currently rebuild query string from 'get_data' elements. This requires URL encoding components of the string. Unfortunately, some IdPs (Azure AD, ADFS) use lower-case encoding. To handle this, one needs to pass lowercase_urlencoding=True. This complicates code that needs to support different IdPs. Instead, if 'query_string' is passed, take parts from it directly. This avoids the need to URL encode them. This is similar to the `retrieveParametersFromServer` argument in the PHP version. This feature is disabled by default. Pass validate_signature_from_qs=True to enable it.
eltoder
force-pushed
the
feature/validate-signature-query-string
branch
from
19e9d32
to
293bcde
Compare
|
@pitbulk done |
pitbulk
approved these changes
May 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When validating request or response signature in process_slo() we
currently rebuild query string from 'get_data' elements. This requires
URL encoding components of the string. Unfortunately, some IdPs (Azure
AD, ADFS) use lower-case encoding. To handle this, one needs to pass
lowercase_urlencoding=True. This complicates code that needs to support
different IdPs.
Instead, if 'query_string' is passed, take parts from it directly. This
avoids the need to URL encode them. This is similar to the
retrieveParametersFromServerargument in the PHP version.