This PR introduces new audit checks for SAML configurations.#192
This PR introduces new audit checks for SAML configurations.#192FeSuert wants to merge 12 commits intoiteratec:mainfrom
Conversation
malexmave
left a comment
malexmave
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review this. Here's some feedback on the pull request.
Overall: Very solid work. There are a few issues with the code style that we would like to see changed to make it consistent with the rest of the codebase and maintainable long term, see the inline comments below.
Also: after making the changes, please run the ruff formatter as described in the docs. Otherwise, the lint pipeline will fail (as it did for the current commit).
| ## SamlIdpValidateSignatureCheck | ||
|
|
||
| This auditor warns about SAML Identity Providers configured with `Validate Signature` set to `false`. | ||
| When disabled, Keycloak accepts SAML responses without verifying the digital signature of the upstream Identity Provider. | ||
|
|
||
| This is a critical security risk. | ||
| Without signature verification, an attacker can forge a completely fabricated SAML response or inject a malicious assertion into a valid response (known as XML Signature Wrapping or XSW). | ||
| This effectively allows an attacker to log in as any user, including administrators, without a valid password. | ||
| We strongly recommend ensuring that `Validate Signature` is enabled for all SAML providers. |
There was a problem hiding this comment.
As I understand it:
- SAML allows signing either the entire message or individual assertions (or nothing at all).
- As long as critical assertions are signed, not signing the entire payload is not a critical problem.
Is that correct? If so, we may have to rethink how we structure these checks, as "no signatures at all" would be critical, while "only signature A" or "only signature B" would be less bad (but still a finding). Or am I misunderstanding something?
There was a problem hiding this comment.
Ah, I see that further down below, this question is partially answered. But still - do you think that it would be worth differentiating between "no signatures" and "some signatures" in more ways than we are currently doing with this implementation?
| for uri in uris: | ||
| # Check for trailing wildcard | ||
| if uri and uri.strip().endswith("*"): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
For OIDC clients, the logic of how redirect URIs are assembled is non-trivial. We replicated it in the function get_resolved_redirect_uris. Unless you have reason to believe that SAML clients use a different logic, I would recommend using that function instead of the naive get_redirect_uris.
There was a problem hiding this comment.
Note to self: replicate the auditor for wildcard without preceding slash for SAML clients as well.
| if self.is_vulnerable(client): | ||
| # Re-fetch URIs for the report detail | ||
| uris = client.get_redirect_uris() | ||
| bad_uris = [u for u in uris if u.endswith("*")] |
There was a problem hiding this comment.
Same here. Or consider changing the signature of the method to return a list of affected redirect URIs and check if the length of the list is larger than zero to see if the client is vulnerable.
Adds checks for signature validation, encryption enforcement, weak algorithms, and wildcard redirects. Registers the new module in configuration/auditors.py.
…s and making their implementation easier
Co-authored-by: Max Maass <1688580+malexmave@users.noreply.github.com>
Signed-off-by: Max Maass <max.maass@iteratec.com>
Signed-off-by: Max Maass <max.maass@iteratec.com>
3 participants
No description provided.