From ee05b3a90155acfa425cf489876962d5f7615a6e Mon Sep 17 00:00:00 2001 From: Max Maass Date: Tue, 1 Oct 2024 14:32:42 +0200 Subject: [PATCH] Include SAML support --- docs/auditors/idp.md | 7 +++++-- ...der_with_signature_verification_disabled.py} | 6 +++--- ...ider_with_signature_verification_disabled.py | 17 +++++++++++------ 3 files changed, 19 insertions(+), 11 deletions(-) rename kcwarden/auditors/idp/{oidc_identity_provider_with_signature_verification_disabled.py => identity_provider_with_signature_verification_disabled.py} (86%) diff --git a/docs/auditors/idp.md b/docs/auditors/idp.md index 384f273..cb091f1 100644 --- a/docs/auditors/idp.md +++ b/docs/auditors/idp.md @@ -9,13 +9,16 @@ Keycloak can be configured to delegate user authentication to an upstream Identi These auditors are currently fairly bare-bones, as we haven't yet had time to read up on what specific problems may lurk in the different possible setups. If you have expertise in this area, please reach out or contribute your own auditors. -## OIDCIdentityProviderWithSignatureVerificationDisabled +## IdentityProviderWithSignatureVerificationDisabled -This auditor warns about OIDC Identity Providers configured not to check the signatures of the upstream IDP. +This auditor warns about Identity Providers configured not to check the signatures of the upstream IDP. Not checking the signatures of the tokens the IDP provides is dangerous, as the tokens are no longer cryptographically protected against tampering. This may lead to an account takeover or other attacks. We strongly recommend to set up signature checks. +The auditor supports OIDC, Keycloak OIDC, and SAML IDPs. +Provider-specific IDPs (like GitHub, GitLab, etc.) do not have an option to disable signature verification and should thus be safe by default. + ## OIDCIdentityProviderWithoutPKCE This auditor warns about OIDC Identity Providers configured within a realm that do not have the Proof Key for Code diff --git a/kcwarden/auditors/idp/oidc_identity_provider_with_signature_verification_disabled.py b/kcwarden/auditors/idp/identity_provider_with_signature_verification_disabled.py similarity index 86% rename from kcwarden/auditors/idp/oidc_identity_provider_with_signature_verification_disabled.py rename to kcwarden/auditors/idp/identity_provider_with_signature_verification_disabled.py index f6c5feb..4c1c6a8 100644 --- a/kcwarden/auditors/idp/oidc_identity_provider_with_signature_verification_disabled.py +++ b/kcwarden/auditors/idp/identity_provider_with_signature_verification_disabled.py @@ -2,14 +2,14 @@ from kcwarden.custom_types.result import Severity -class OIDCIdentityProviderWithSignatureVerificationDisabled(Auditor): +class IdentityProviderWithSignatureVerificationDisabled(Auditor): DEFAULT_SEVERITY = Severity.Critical - SHORT_DESCRIPTION = "OIDC Identity Provider does not verify upstream IDPs signatures" + SHORT_DESCRIPTION = "Identity Provider does not verify upstream IDPs signatures" LONG_DESCRIPTION = "Keycloak allows you to configure external identity providers. When using OpenID Connect, it is important to verify the cryptographic signature that secures the identity and access tokens generated by the identity provider. This IDP configuration disables this signature check, which makes it vulnerable to accepting forged access tokens, leading to account takeover and other security issues." REFERENCE = "" def should_consider_idp(self, idp) -> bool: - return self.is_not_ignored(idp) and idp.get_provider_id() in ["oidc", "keycloak-oidc"] + return self.is_not_ignored(idp) and idp.get_provider_id() in ["oidc", "keycloak-oidc", "saml"] def idp_does_not_verify_signatures(self, config): return config.get("validateSignature") == "false" diff --git a/tests/auditors/idp/test_oidc_identity_provider_with_signature_verification_disabled.py b/tests/auditors/idp/test_oidc_identity_provider_with_signature_verification_disabled.py index b610881..1346ed6 100644 --- a/tests/auditors/idp/test_oidc_identity_provider_with_signature_verification_disabled.py +++ b/tests/auditors/idp/test_oidc_identity_provider_with_signature_verification_disabled.py @@ -1,8 +1,8 @@ import pytest from unittest.mock import Mock -from kcwarden.auditors.idp.oidc_identity_provider_with_signature_verification_disabled import ( - OIDCIdentityProviderWithSignatureVerificationDisabled, +from kcwarden.auditors.idp.identity_provider_with_signature_verification_disabled import ( + IdentityProviderWithSignatureVerificationDisabled, ) from kcwarden.custom_types import config_keys @@ -10,7 +10,7 @@ class TestOIDCIdentityProviderWithSignatureVerificationDisabled: @pytest.fixture def auditor(self, database, default_config): - auditor_instance = OIDCIdentityProviderWithSignatureVerificationDisabled(database, default_config) + auditor_instance = IdentityProviderWithSignatureVerificationDisabled(database, default_config) auditor_instance._DB = Mock() return auditor_instance @@ -19,7 +19,8 @@ def auditor(self, database, default_config): [ ("oidc", True), # OIDC provider should be considered ("keycloak-oidc", True), # Keycloak OIDC provider should be considered - ("saml", False), # SAML provider should not be considered + ("saml", True), # SAML provider should also be considered + ("github", False), # SAML provider should also be considered ], ) def test_should_consider_idp(self, auditor, provider_id, expected): @@ -69,9 +70,13 @@ def test_audit_function_multiple_idps(self, auditor): idp3.get_provider_id.return_value = "keycloak-oidc" idp3.get_config.return_value = {"validateSignature": "false"} - auditor._DB.get_all_identity_providers.return_value = [idp1, idp2, idp3] + idp4 = Mock() + idp4.get_provider_id.return_value = "saml" + idp4.get_config.return_value = {"validateSignature": "false"} + + auditor._DB.get_all_identity_providers.return_value = [idp1, idp2, idp3, idp4] results = list(auditor.audit()) - assert len(results) == 2 # Expect findings from idp1 and idp3 + assert len(results) == 3 # Expect findings from idp1, idp3 and idp4 def test_ignore_list_functionality(self, auditor, mock_idp): # Setup IDP without force sync mode and with mappers