This PR introduces new audit checks for SAML configurations. #192
Conversation
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
Audits for SAML misconfigurations
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?
| REFERENCE = "" | ||
|
|
||
| def should_consider_client(self, client) -> bool: | ||
| return self.is_not_ignored(client) and client.get_protocol() == "saml" |
There was a problem hiding this comment.
Flagging something here once, but it covers all auditors: Please put logic like "is this a SAML client" into the Client dataclass, similar to the Client.is_oidc_client() method, and use that everywhere.
| attributes = client.get_attributes() | ||
| val = attributes.get("saml.assertion.signature", "false") |
There was a problem hiding this comment.
Flagging something here once, but it covers all auditors: Please try to encapsulate this kind of logic into the Client dataclass (or, for the IDP auditors, into that respective class), and then use a function call here to get the boolean. This also allows you to return it as a "native" boolean instead of a string, as a bonus. :)
The rationale for why this is important: We would like to keep the implementation details of the keycloak config format out of the auditors as far as possible, as that allows us to make changes in a single location if the format ever changes. It also makes it possible to, in principle, add other sources for config data down the line, like IaC tool formats.
This will also mean that you will have to update the unit tests as well, I'm afraid.
| def is_vulnerable(client) -> bool: | ||
| attributes = client.get_attributes() | ||
| algo = attributes.get("saml.signature.algorithm", "") | ||
| weak_algos = ["RSA_SHA1", "DSA_SHA1"] |
There was a problem hiding this comment.
Code style: consider pulling the list of weak algorithms out of this function and define it further up in the class. That makes it more prominent and easier to change in the future.
| attributes = getattr(client, "attributes", client.get("attributes", {})) | ||
| algo = attributes.get("saml.signature.algorithm", "Unknown") |
There was a problem hiding this comment.
Try to avoid getattr and stick to more pythonic solutions like get, otherwise @twwd will cry ;)
| 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.
| 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.
Co-authored-by: Max Maass <1688580+malexmave@users.noreply.github.com>
3 participants
No description provided.