Skip to content
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

New Auditor: Flag Identity Providers with disabled signature verification #37

Merged
merged 3 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/auditors/idp.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +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.

## IdentityProviderWithSignatureVerificationDisabled

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
@@ -0,0 +1,24 @@
from kcwarden.api import Auditor
from kcwarden.custom_types.result import Severity


class IdentityProviderWithSignatureVerificationDisabled(Auditor):
DEFAULT_SEVERITY = Severity.Critical
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", "saml"]

def idp_does_not_verify_signatures(self, config):
return config.get("validateSignature") == "false"

def audit(self):
for idp in self._DB.get_all_identity_providers():
# Skip IDPs that were explicitly ignored, or that aren't OIDC IDPs
if not self.should_consider_idp(idp):
continue
# Check if signature verification is disabled
if self.idp_does_not_verify_signatures(idp.get_config()):
yield self.generate_finding(idp)
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import pytest
from unittest.mock import Mock

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 = IdentityProviderWithSignatureVerificationDisabled(database, default_config)
auditor_instance._DB = Mock()
return auditor_instance

@pytest.mark.parametrize(
"provider_id, expected",
[
("oidc", True), # OIDC provider should be considered
("keycloak-oidc", True), # Keycloak OIDC provider should 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):
mock_idp = Mock()
mock_idp.get_provider_id.return_value = provider_id
assert auditor.should_consider_idp(mock_idp) == expected

@pytest.mark.parametrize(
"config, expected",
[
({"validateSignature": "true"}, False), # Signature verification enabled
({"validateSignature": "false"}, True), # Signature verification disabled
],
)
def test_idp_does_not_enforce_pkce(self, auditor, config, expected):
assert auditor.idp_does_not_verify_signatures(config) == expected

def test_audit_function_no_findings(self, auditor, mock_idp):
# Setup IDP with correct PKCE configuration
mock_idp.get_provider_id.return_value = "oidc"
mock_idp.get_config.return_value = {"validateSignature": "true"}
auditor._DB.get_all_identity_providers.return_value = [mock_idp]

results = list(auditor.audit())
assert len(results) == 0

def test_audit_function_with_findings(self, auditor, mock_idp):
# Setup IDP without correct PKCE configuration
mock_idp.get_provider_id.return_value = "oidc"
mock_idp.get_config.return_value = {"validateSignature": "false"}
auditor._DB.get_all_identity_providers.return_value = [mock_idp]

results = list(auditor.audit())
assert len(results) == 1

def test_audit_function_multiple_idps(self, auditor):
# Create separate mock IDPs with distinct settings
idp1 = Mock()
idp1.get_provider_id.return_value = "oidc"
idp1.get_config.return_value = {"validateSignature": "false"}

idp2 = Mock()
idp2.get_provider_id.return_value = "oidc"
idp2.get_config.return_value = {"validateSignature": "true"}

idp3 = Mock()
idp3.get_provider_id.return_value = "keycloak-oidc"
idp3.get_config.return_value = {"validateSignature": "false"}

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) == 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
# Setup IDP without correct PKCE configuration
mock_idp.get_provider_id.return_value = "oidc"
mock_idp.get_config.return_value = {"validateSignature": "false"}
mock_idp.get_alias.return_value = "ignored_idp"
mock_idp.get_name.return_value = mock_idp.get_alias.return_value
auditor._DB.get_all_identity_providers.return_value = [mock_idp]

# Add the IDP to the ignore list
auditor._CONFIG = {config_keys.AUDITOR_CONFIG: {auditor.get_classname(): ["ignored_idp"]}}

results = list(auditor.audit())
assert len(results) == 0 # No findings due to ignore list