Skip to content

Commit

Permalink
Include SAML support
Browse files Browse the repository at this point in the history
  • Loading branch information
malexmave committed Oct 1, 2024
1 parent 4b4df6f commit ee05b3a
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 11 deletions.
7 changes: 5 additions & 2 deletions docs/auditors/idp.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
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


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

Expand All @@ -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):
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit ee05b3a

Please sign in to comment.