diff --git a/kcwarden/auditors/client/client_authentication_via_mtls_or_jwt_recommended.py b/kcwarden/auditors/client/client_authentication_via_mtls_or_jwt_recommended.py index b3e79b7..5645de0 100644 --- a/kcwarden/auditors/client/client_authentication_via_mtls_or_jwt_recommended.py +++ b/kcwarden/auditors/client/client_authentication_via_mtls_or_jwt_recommended.py @@ -12,7 +12,12 @@ def should_consider_client(self, client) -> bool: # We are interested in clients that are: # - OIDC Clients # - Confidential Clients - return self.is_not_ignored(client) and client.is_oidc_client() and not client.is_public() + return ( + self.is_not_ignored(client) + and not client.is_realm_specific_client() + and client.is_oidc_client() + and not client.is_public() + ) def client_does_not_use_mtls_or_jwt_auth(self, client) -> bool: # If the clientAuthenticatorType is client-secret, basic client secret authentication is used. diff --git a/kcwarden/auditors/client/client_has_erroneously_configured_wildcard_uri.py b/kcwarden/auditors/client/client_has_erroneously_configured_wildcard_uri.py index ef70a50..114d027 100644 --- a/kcwarden/auditors/client/client_has_erroneously_configured_wildcard_uri.py +++ b/kcwarden/auditors/client/client_has_erroneously_configured_wildcard_uri.py @@ -16,6 +16,7 @@ def should_consider_client(self, client) -> bool: # - At least one flow that uses the redirect_uri active return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (client.has_standard_flow_enabled() or client.has_implicit_flow_enabled()) ) diff --git a/kcwarden/auditors/client/client_has_undefined_base_domain_and_schema.py b/kcwarden/auditors/client/client_has_undefined_base_domain_and_schema.py index da85e92..97b0b73 100644 --- a/kcwarden/auditors/client/client_has_undefined_base_domain_and_schema.py +++ b/kcwarden/auditors/client/client_has_undefined_base_domain_and_schema.py @@ -17,6 +17,7 @@ def should_consider_client(self, client) -> bool: # TODO Are there more flows that use redirect_uri? return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (client.has_standard_flow_enabled() or client.has_implicit_flow_enabled()) ) diff --git a/kcwarden/auditors/client/client_must_not_use_unencrypted_nonlocal_redirect_uri.py b/kcwarden/auditors/client/client_must_not_use_unencrypted_nonlocal_redirect_uri.py index 78f87c0..9081cc2 100644 --- a/kcwarden/auditors/client/client_must_not_use_unencrypted_nonlocal_redirect_uri.py +++ b/kcwarden/auditors/client/client_must_not_use_unencrypted_nonlocal_redirect_uri.py @@ -17,6 +17,7 @@ def should_consider_client(self, client) -> bool: # TODO Are there more flows that use redirect_uri? return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (client.has_standard_flow_enabled() or client.has_implicit_flow_enabled()) ) diff --git a/kcwarden/auditors/client/client_should_disable_implicit_grant_flow.py b/kcwarden/auditors/client/client_should_disable_implicit_grant_flow.py index 3da7f7d..a5b6440 100644 --- a/kcwarden/auditors/client/client_should_disable_implicit_grant_flow.py +++ b/kcwarden/auditors/client/client_should_disable_implicit_grant_flow.py @@ -11,7 +11,7 @@ class ClientShouldDisableImplicitGrantFlow(Auditor): def should_consider_client(self, client) -> bool: # We are interested in clients that are: # - OIDC Clients - return self.is_not_ignored(client) and client.is_oidc_client() + return self.is_not_ignored(client) and client.is_oidc_client() and not client.is_realm_specific_client() def client_uses_implicit_grant_flow(self, client) -> bool: # All clients that have implicit flow enabled are considered suspect diff --git a/kcwarden/auditors/client/client_should_not_use_wildcard_redirect_uri.py b/kcwarden/auditors/client/client_should_not_use_wildcard_redirect_uri.py index fa15aab..71f0ee2 100644 --- a/kcwarden/auditors/client/client_should_not_use_wildcard_redirect_uri.py +++ b/kcwarden/auditors/client/client_should_not_use_wildcard_redirect_uri.py @@ -15,6 +15,7 @@ def should_consider_client(self, client) -> bool: # TODO Are there more flows that use redirect_uri? return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (client.has_standard_flow_enabled() or client.has_implicit_flow_enabled()) ) diff --git a/kcwarden/auditors/client/client_uses_custom_redirect_uri_scheme.py b/kcwarden/auditors/client/client_uses_custom_redirect_uri_scheme.py index 19b5fc3..c79f1e2 100644 --- a/kcwarden/auditors/client/client_uses_custom_redirect_uri_scheme.py +++ b/kcwarden/auditors/client/client_uses_custom_redirect_uri_scheme.py @@ -17,6 +17,7 @@ def should_consider_client(self, client) -> bool: # TODO Are there more flows that use redirect_uri? return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (client.has_standard_flow_enabled() or client.has_implicit_flow_enabled()) ) diff --git a/kcwarden/auditors/client/client_with_default_offline_access_scope.py b/kcwarden/auditors/client/client_with_default_offline_access_scope.py index a63cc58..6cbda79 100644 --- a/kcwarden/auditors/client/client_with_default_offline_access_scope.py +++ b/kcwarden/auditors/client/client_with_default_offline_access_scope.py @@ -12,7 +12,7 @@ class ClientWithDefaultOfflineAccessScope(Auditor): REFERENCE = "" def should_consider_client(self, client) -> bool: - return self.is_not_ignored(client) + return self.is_not_ignored(client) and not client.is_realm_specific_client() def client_can_generate_offline_tokens(self, client) -> bool: # Check if the "offline_access" scope is in the default scopes diff --git a/kcwarden/auditors/client/client_with_full_scope_allowed.py b/kcwarden/auditors/client/client_with_full_scope_allowed.py index 105f208..a583cc8 100644 --- a/kcwarden/auditors/client/client_with_full_scope_allowed.py +++ b/kcwarden/auditors/client/client_with_full_scope_allowed.py @@ -9,7 +9,11 @@ class ClientWithFullScopeAllowed(Auditor): REFERENCE = "" def should_consider_client(self, client) -> bool: - return self.is_not_ignored(client) and client.allows_user_authentication() + return ( + self.is_not_ignored(client) + and client.allows_user_authentication() + and not client.is_realm_specific_client() + ) def client_has_full_scope_allowed(self, client) -> bool: return client.has_full_scope_allowed() diff --git a/kcwarden/auditors/client/client_with_optional_offline_access_scope.py b/kcwarden/auditors/client/client_with_optional_offline_access_scope.py index aa10948..eeb27c9 100644 --- a/kcwarden/auditors/client/client_with_optional_offline_access_scope.py +++ b/kcwarden/auditors/client/client_with_optional_offline_access_scope.py @@ -9,7 +9,7 @@ class ClientWithOptionalOfflineAccessScope(Auditor): REFERENCE = "" def should_consider_client(self, client) -> bool: - return self.is_not_ignored(client) + return self.is_not_ignored(client) and not client.is_realm_specific_client() def client_can_generate_offline_tokens(self, client) -> bool: # Check if the "offline_access" scope is in the optional scopes diff --git a/kcwarden/auditors/client/client_with_service_account_and_other_flow_enabled.py b/kcwarden/auditors/client/client_with_service_account_and_other_flow_enabled.py index 110bcc2..6321537 100644 --- a/kcwarden/auditors/client/client_with_service_account_and_other_flow_enabled.py +++ b/kcwarden/auditors/client/client_with_service_account_and_other_flow_enabled.py @@ -15,6 +15,7 @@ def should_consider_client(self, client) -> bool: # - Has service account return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and (not client.is_public()) and client.has_service_account_enabled() diff --git a/kcwarden/auditors/client/confidential_client_should_disable_direct_access_grants.py b/kcwarden/auditors/client/confidential_client_should_disable_direct_access_grants.py index e0aec58..ba7f6dc 100644 --- a/kcwarden/auditors/client/confidential_client_should_disable_direct_access_grants.py +++ b/kcwarden/auditors/client/confidential_client_should_disable_direct_access_grants.py @@ -12,7 +12,12 @@ def should_consider_client(self, client) -> bool: # We are interested in clients that are: # - OIDC clients # - Are confidential clients - return self.is_not_ignored(client) and client.is_oidc_client() and not client.is_public() + return ( + self.is_not_ignored(client) + and not client.is_realm_specific_client() + and client.is_oidc_client() + and not client.is_public() + ) def client_uses_direct_access_grants(self, client) -> bool: # All clients with direct access grants should be reported diff --git a/kcwarden/auditors/client/confidential_client_should_enforce_pkce.py b/kcwarden/auditors/client/confidential_client_should_enforce_pkce.py index 6b3dda3..c7a51c0 100644 --- a/kcwarden/auditors/client/confidential_client_should_enforce_pkce.py +++ b/kcwarden/auditors/client/confidential_client_should_enforce_pkce.py @@ -16,6 +16,7 @@ def should_consider_client(self, client) -> bool: return ( self.is_not_ignored(client) and client.is_oidc_client() + and not client.is_realm_specific_client() and (not client.is_public()) and client.has_standard_flow_enabled() ) diff --git a/kcwarden/auditors/client/public_client_should_disable_direct_access_grants.py b/kcwarden/auditors/client/public_client_should_disable_direct_access_grants.py index c76dce4..3548466 100644 --- a/kcwarden/auditors/client/public_client_should_disable_direct_access_grants.py +++ b/kcwarden/auditors/client/public_client_should_disable_direct_access_grants.py @@ -12,7 +12,12 @@ def should_consider_client(self, client) -> bool: # We are interested in clients that are: # - OIDC clients # - Are public clients - return self.is_not_ignored(client) and client.is_oidc_client() and client.is_public() + return ( + self.is_not_ignored(client) + and not client.is_realm_specific_client() + and client.is_oidc_client() + and client.is_public() + ) def client_uses_direct_access_grants(self, client) -> bool: # All clients with direct access grants should be reported diff --git a/kcwarden/auditors/client/public_clients_must_enforce_pkce.py b/kcwarden/auditors/client/public_clients_must_enforce_pkce.py index cf31178..ca7c867 100644 --- a/kcwarden/auditors/client/public_clients_must_enforce_pkce.py +++ b/kcwarden/auditors/client/public_clients_must_enforce_pkce.py @@ -15,6 +15,7 @@ def should_consider_client(self, client) -> bool: # - Have the standard flow enabled return ( self.is_not_ignored(client) + and not client.is_realm_specific_client() and client.is_oidc_client() and client.is_public() and client.has_standard_flow_enabled() diff --git a/kcwarden/custom_types/keycloak_object.py b/kcwarden/custom_types/keycloak_object.py index 72805ce..b38f360 100644 --- a/kcwarden/custom_types/keycloak_object.py +++ b/kcwarden/custom_types/keycloak_object.py @@ -505,13 +505,29 @@ def get_service_account_name(self) -> str | None: return "service-account-" + self.get_client_id().lower() # More Specific Properties + def is_realm_specific_client(self) -> bool: + # Each realm in Keycloak will get a realm-specific client created in the + # master realm. This is used to hold realm-specific roles, like the user + # management permissions. These clients behave differently from other + # clients, so we need to exclude them from some of our standard checks. + return ( + self.get_realm().get_name() == "master" and self.get_name().endswith("-realm") and "protocol" not in self._d + ) + def get_protocol(self) -> str: # Every client should have the "protocol" field set, but the "master-realm" # client in the "master" realm for some reason does not include this field. # This code works around that by returning openid-connect in that case. - if self.get_name() == "master-realm" and self.get_realm().get_name() == "master": - return "openid-connect" - return self._d["protocol"] + try: + return self._d["protocol"] + except KeyError: + # If the client is a realm-specific client, it for some reason does not + # have a "protocol" set. Return openid-connect anyway. + if self.is_realm_specific_client(): + return "openid-connect" + # This case should never happen, so instead of blindly returning something, + # we'd like to know about it. Raise an exception. + raise RuntimeError("'protocol' field of Client {} is not set, aborting".format(self.get_name())) def is_oidc_client(self) -> bool: return self.get_protocol() == "openid-connect" @@ -568,7 +584,6 @@ def is_default_keycloak_client(self) -> bool: "broker", "realm-management", "security-admin-console", - "master-realm", ] def allows_user_authentication(self) -> bool: diff --git a/tests/auditors/client/test_client_with_default_offline_access_scope.py b/tests/auditors/client/test_client_with_default_offline_access_scope.py index 5ef31e7..62118df 100644 --- a/tests/auditors/client/test_client_with_default_offline_access_scope.py +++ b/tests/auditors/client/test_client_with_default_offline_access_scope.py @@ -79,6 +79,7 @@ def test_audit_function_multiple_clients(self, auditor): client1.allows_user_authentication.return_value = True client1.get_attributes.return_value = {"use.refresh.tokens": "true"} client1.is_public.return_value = False + client1.is_realm_specific_client.return_value = False client2 = Mock() client2.get_default_client_scopes.return_value = [] @@ -89,6 +90,7 @@ def test_audit_function_multiple_clients(self, auditor): client2.get_attributes.return_value = {"use.refresh.tokens": "false"} client2.is_public.return_value = True client2.allows_user_authentication.return_value = True + client2.is_realm_specific_client.return_value = False client3 = Mock() client3.get_default_client_scopes.return_value = ["offline_access"] @@ -99,6 +101,7 @@ def test_audit_function_multiple_clients(self, auditor): client3.get_attributes.return_value = {"use.refresh.tokens": "true"} client3.is_public.return_value = False client3.allows_user_authentication.return_value = True + client3.is_realm_specific_client.return_value = False auditor._DB.get_all_clients.return_value = [client1, client2, client3] results = list(auditor.audit()) diff --git a/tests/auditors/client/test_client_with_full_scope_allowed.py b/tests/auditors/client/test_client_with_full_scope_allowed.py index 9496a41..4b42773 100644 --- a/tests/auditors/client/test_client_with_full_scope_allowed.py +++ b/tests/auditors/client/test_client_with_full_scope_allowed.py @@ -58,16 +58,19 @@ def test_audit_function_multiple_clients(self, auditor): client1.has_full_scope_allowed.return_value = True client1.get_default_client_scopes.return_value = ["email"] client1.get_optional_client_scopes.return_value = ["profile"] + client1.is_realm_specific_client.return_value = False client2 = Mock() client2.has_full_scope_allowed.return_value = False client2.get_default_client_scopes.return_value = ["email", "profile"] client2.get_optional_client_scopes.return_value = ["address"] + client2.is_realm_specific_client.return_value = False client3 = Mock() client3.has_full_scope_allowed.return_value = True client3.get_default_client_scopes.return_value = ["offline_access"] client3.get_optional_client_scopes.return_value = [] + client3.is_realm_specific_client.return_value = False auditor._DB.get_all_clients.return_value = [client1, client2, client3] results = list(auditor.audit()) diff --git a/tests/auditors/client/test_client_with_optional_offline_access_scope.py b/tests/auditors/client/test_client_with_optional_offline_access_scope.py index a62514b..af6e26b 100644 --- a/tests/auditors/client/test_client_with_optional_offline_access_scope.py +++ b/tests/auditors/client/test_client_with_optional_offline_access_scope.py @@ -76,6 +76,7 @@ def test_audit_function_multiple_clients(self, auditor): client1.has_implicit_flow_enabled.return_value = False client1.get_attributes.return_value = {"use.refresh.tokens": "true"} client1.is_public.return_value = False + client1.is_realm_specific_client.return_value = False client2 = Mock() client2.get_optional_client_scopes.return_value = [] @@ -85,6 +86,7 @@ def test_audit_function_multiple_clients(self, auditor): client2.has_implicit_flow_enabled.return_value = False client2.get_attributes.return_value = {"use.refresh.tokens": "false"} client2.is_public.return_value = True + client2.is_realm_specific_client.return_value = False client3 = Mock() client3.get_optional_client_scopes.return_value = ["offline_access"] @@ -94,6 +96,7 @@ def test_audit_function_multiple_clients(self, auditor): client3.has_implicit_flow_enabled.return_value = True client3.get_attributes.return_value = {"use.refresh.tokens": "true"} client3.is_public.return_value = False + client3.is_realm_specific_client.return_value = False auditor._DB.get_all_clients.return_value = [client1, client2, client3] results = list(auditor.audit()) diff --git a/tests/auditors/client/test_client_with_service_account_and_other_flow_enabled.py b/tests/auditors/client/test_client_with_service_account_and_other_flow_enabled.py index 74ff8a6..bde6358 100644 --- a/tests/auditors/client/test_client_with_service_account_and_other_flow_enabled.py +++ b/tests/auditors/client/test_client_with_service_account_and_other_flow_enabled.py @@ -90,6 +90,7 @@ def test_audit_function_multiple_clients(self, auditor): client1.has_device_authorization_grant_flow_enabled.return_value = True client1.has_standard_flow_enabled.return_value = True client1.allows_user_authentication.return_value = True + client1.is_realm_specific_client.return_value = False client2 = Mock() client2.is_oidc_client.return_value = True @@ -99,7 +100,8 @@ def test_audit_function_multiple_clients(self, auditor): client2.has_implicit_flow_enabled.return_value = False client2.has_device_authorization_grant_flow_enabled.return_value = False client2.has_standard_flow_enabled.return_value = False - client1.allows_user_authentication.return_value = False + client2.allows_user_authentication.return_value = False + client2.is_realm_specific_client.return_value = False client3 = Mock() client3.is_oidc_client.return_value = True @@ -110,6 +112,7 @@ def test_audit_function_multiple_clients(self, auditor): client3.has_standard_flow_enabled.return_value = True client3.has_device_authorization_grant_flow_enabled.return_value = False client3.allows_user_authentication.return_value = True + client3.is_realm_specific_client.return_value = False auditor._DB.get_all_clients.return_value = [client1, client2, client3] results = list(auditor.audit()) diff --git a/tests/conftest.py b/tests/conftest.py index 490d484..a4288a4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -135,6 +135,7 @@ def mock_client(mock_realm): client.get_default_client_scopes.return_value = [] client.get_optional_client_scopes.return_value = [] client.is_oidc_client.return_value = True + client.is_realm_specific_client.return_value = False return client