-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
SAML: Don't require signed assertions #1669
Comments
I'm not familiar enough with this, but your explanation makes sense. How about we move this configuration? |
These changes make it possible to configure if the SAML based authentication requires the IDP to sign individual assertions, whole response or both. These values can be configured both globally via environmental variables or through organization specific settings from the Admin UI / API. The two environmental variables provide global defaults for these values: * REDASH_SAML_WANT_ASSERTIONS_SIGNED * REDASH_SAML_WANT_RESPONSE_SIGNED If these variables are not present, they default to the previously hardcoded values (want_signed_assertions = True and want_response_signed = False). This is done to ensure existing installations do not break on upgrade. A test has been written to ensure this behavior persists. Alternatively, these two variables can be modified on per-organization basis from the Admin UI. These configurations override the global defaults set via the environment. Special care has been taken to ensure that it's not possible to disable signing requirements completely. If the two environmental variables are both disabled while SAML is enabled, the server will instruct the user to enable at least signing requirement and refuse to start. Similarly, the UI will not let the user to disable both signing requirements and the API will return a 400 response if signing requirements are going to be disabled. Tests have also been written to test the backend functionality. Closes getredash#1669.
…Don't require signed assertions)
@susodapop I suppose this can be closed when the merged commit gets a tagged release? |
SGTM |
This is not related to the issue, but I assume that the issue author or followers might have SAML enabled for their deployment and should be aware of the following Security Advisory: #5961. This affects all Redash versions and should be patched immediately. |
Currently Redash requires signed SAML assertions. This is mostly legacy functionality, and isn't needed when the response itself is signed. Most IdPs will not sign assertions by default (requiring special configuration), but will sign the response, as that's what the vast majority of SPs expect.
In
saml_auth.py
:This should probably be reversed:
The text was updated successfully, but these errors were encountered: