From 7f3dcdf02fa3f3064a916cc2bcd168fd139677dd Mon Sep 17 00:00:00 2001 From: abdou Date: Tue, 12 May 2026 19:22:19 +0200 Subject: [PATCH] fix(m365): surface AuditLog.Read.All permission errors instead of false positives (#10907) Co-authored-by: Hugo P.Brito --- prowler/CHANGELOG.md | 8 ++ ...s_account_fido2_security_key_registered.py | 9 ++ .../m365/services/entra/entra_service.py | 42 ++++-- .../entra_users_mfa_capable.py | 12 +- ...ount_fido2_security_key_registered_test.py | 124 +++++++++++++++++ .../entra_users_mfa_capable_test.py | 131 ++++++++++++++++++ .../entra/microsoft365_entra_service_test.py | 34 ++++- 7 files changed, 346 insertions(+), 14 deletions(-) diff --git a/prowler/CHANGELOG.md b/prowler/CHANGELOG.md index 6ce149a0a1..aafcd9f345 100644 --- a/prowler/CHANGELOG.md +++ b/prowler/CHANGELOG.md @@ -16,6 +16,14 @@ All notable changes to the **Prowler SDK** are documented in this file. --- +## [5.26.2] (Prowler UNRELEASED) + +### 🐞 Fixed + +- `entra_users_mfa_capable` and `entra_break_glass_account_fido2_security_key_registered` report a preventive FAIL per affected user (with the missing permission named) when the M365 service principal lacks `AuditLog.Read.All`, instead of mass false positives [(#10907)](https://github.com/prowler-cloud/prowler/pull/10907) + +--- + ## [5.26.1] (Prowler v5.26.1) ### 🐞 Fixed diff --git a/prowler/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered.py b/prowler/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered.py index de973e9ffa..e90eed0023 100644 --- a/prowler/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered.py +++ b/prowler/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered.py @@ -85,6 +85,15 @@ class entra_break_glass_account_fido2_security_key_registered(Check): resource_id=user.id, ) + if entra_client.user_registration_details_error: + report.status = "FAIL" + report.status_extended = ( + f"Cannot verify FIDO2 security key registration for break glass account {user.name}: " + f"{entra_client.user_registration_details_error}." + ) + findings.append(report) + continue + auth_methods = set(user.authentication_methods) has_fido2 = "fido2SecurityKey" in auth_methods has_passkey_device_bound = "passKeyDeviceBound" in auth_methods diff --git a/prowler/providers/m365/services/entra/entra_service.py b/prowler/providers/m365/services/entra/entra_service.py index 258a1e8f61..e18c7782fc 100644 --- a/prowler/providers/m365/services/entra/entra_service.py +++ b/prowler/providers/m365/services/entra/entra_service.py @@ -3,7 +3,7 @@ import json from asyncio import gather from datetime import datetime, timezone from enum import Enum -from typing import Dict, List, Optional +from typing import Any, Dict, List, Optional, Tuple from uuid import UUID from kiota_abstractions.base_request_configuration import RequestConfiguration @@ -76,6 +76,7 @@ class Entra(M365Service): self.tenant_domain = provider.identity.tenant_domain self.tenant_id = getattr(provider.identity, "tenant_id", None) + self.user_registration_details_error: Optional[str] = None attributes = loop.run_until_complete( gather( self._get_authorization_policy(), @@ -854,7 +855,9 @@ class Entra(M365Service): for member in members: user_roles_map.setdefault(member.id, []).append(role_template_id) - registration_details = await self._get_user_registration_details() + registration_details, self.user_registration_details_error = ( + await self._get_user_registration_details() + ) while users_response: for user in getattr(users_response, "value", []) or []: @@ -897,18 +900,24 @@ class Entra(M365Service): ) return users - async def _get_user_registration_details(self): + async def _get_user_registration_details( + self, + ) -> Tuple[Dict[str, Dict[str, Any]], Optional[str]]: """Retrieve user authentication method registration details. Fetches registration details from the Microsoft Graph API, including MFA capability and the specific authentication methods each user has registered. Returns: - dict: A dictionary mapping user IDs to their registration details, - where each value is a dict with 'is_mfa_capable' (bool) and - 'authentication_methods' (list of str). + A tuple containing: + - A dictionary mapping user IDs to their registration details, + where each value is a dict with 'is_mfa_capable' (bool) and + 'authentication_methods' (list of str), or an empty dict if + retrieval fails. + - An error message string if there was an access error, None otherwise. """ registration_details = {} + error_message = None try: registration_builder = ( self.client.reports.authentication_methods.user_registration_details @@ -933,16 +942,25 @@ class Entra(M365Service): next_link ).get() - except Exception as error: - if ( - error.__class__.__name__ == "ODataError" - and error.__dict__.get("response_status_code", None) == 403 - ): + except ODataError as error: + error_code = getattr(error.error, "code", None) if error.error else None + if error_code == "Authorization_RequestDenied": + error_message = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All" + logger.error( + f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error_message}" + ) + else: logger.error( f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}" ) + error_message = str(error) + except Exception as error: + logger.error( + f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}]: {error}" + ) + error_message = f"Failed to retrieve user registration details: {error}" - return registration_details + return registration_details, error_message async def _get_oauth_apps(self) -> Optional[Dict[str, "OAuthApp"]]: """ diff --git a/prowler/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable.py b/prowler/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable.py index 58a38d14d5..9485676d30 100644 --- a/prowler/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable.py +++ b/prowler/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable.py @@ -13,6 +13,10 @@ class entra_users_mfa_capable(Check): ("Ensure all member users are 'MFA capable'"). Guest users and disabled accounts are excluded from the evaluation. + + - PASS: The member user is MFA capable. + - FAIL: The member user is not MFA capable, or MFA capability cannot be + verified due to insufficient permissions to read user registration details. """ def execute(self) -> List[CheckReportM365]: @@ -42,7 +46,13 @@ class entra_users_mfa_capable(Check): resource_id=user.id, ) - if not user.is_mfa_capable: + if entra_client.user_registration_details_error: + report.status = "FAIL" + report.status_extended = ( + f"Cannot verify MFA capability for user {user.name}: " + f"{entra_client.user_registration_details_error}." + ) + elif not user.is_mfa_capable: report.status = "FAIL" report.status_extended = f"User {user.name} is not MFA capable." else: diff --git a/tests/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered_test.py b/tests/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered_test.py index 31e7c06a1d..1147b64e8e 100644 --- a/tests/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered_test.py +++ b/tests/providers/m365/services/entra/entra_break_glass_account_fido2_security_key_registered/entra_break_glass_account_fido2_security_key_registered_test.py @@ -67,6 +67,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -104,6 +105,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -142,6 +144,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -178,6 +181,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -228,6 +232,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -275,6 +280,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -321,6 +327,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -368,6 +375,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -422,6 +430,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -457,6 +466,7 @@ class Test_entra_break_glass_account_fido2_security_key_registered: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -500,3 +510,117 @@ class Test_entra_break_glass_account_fido2_security_key_registered: assert len(result) == 1 assert result[0].status == "PASS" assert result[0].resource_name == "BreakGlass1" + + def test_user_registration_details_permission_error(self): + """Test FAIL when there's a permission error reading user registration details.""" + entra_client = mock.MagicMock + entra_client.audited_tenant = "audited_tenant" + entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All" + + with ( + mock.patch( + "prowler.providers.common.provider.Provider.get_global_provider", + return_value=set_mocked_m365_provider(), + ), + mock.patch( + f"{CHECK_MODULE_PATH}.entra_client", + new=entra_client, + ), + ): + from prowler.providers.m365.services.entra.entra_break_glass_account_fido2_security_key_registered.entra_break_glass_account_fido2_security_key_registered import ( + entra_break_glass_account_fido2_security_key_registered, + ) + + policy_id = str(uuid4()) + bg_user_id = str(uuid4()) + + entra_client.conditional_access_policies = { + policy_id: _make_policy(policy_id, excluded_users=[bg_user_id]), + } + entra_client.users = { + bg_user_id: User( + id=bg_user_id, + name="BreakGlass1", + on_premises_sync_enabled=False, + authentication_methods=[], + ), + } + + check = entra_break_glass_account_fido2_security_key_registered() + result = check.execute() + + assert len(result) == 1 + assert result[0].status == "FAIL" + assert ( + "Cannot verify FIDO2 security key registration for break glass account BreakGlass1" + in result[0].status_extended + ) + assert "AuditLog.Read.All" in result[0].status_extended + assert result[0].resource_name == "BreakGlass1" + assert result[0].resource_id == bg_user_id + + def test_user_registration_details_permission_error_with_missing_user(self): + """Per-user emission and missing-user short-circuit on the error path. + + Two break-glass user IDs are excluded from all CAPs, but only one is + present in ``entra_client.users``. With ``user_registration_details_error`` + set, the present user must produce one preventive FAIL anchored to the + real user; the missing user must be skipped by the existing + ``if not user: continue`` guard rather than crash or yield a synthetic + finding. + """ + entra_client = mock.MagicMock + entra_client.audited_tenant = "audited_tenant" + entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All" + + with ( + mock.patch( + "prowler.providers.common.provider.Provider.get_global_provider", + return_value=set_mocked_m365_provider(), + ), + mock.patch( + f"{CHECK_MODULE_PATH}.entra_client", + new=entra_client, + ), + ): + from prowler.providers.m365.services.entra.entra_break_glass_account_fido2_security_key_registered.entra_break_glass_account_fido2_security_key_registered import ( + entra_break_glass_account_fido2_security_key_registered, + ) + + policy_id = str(uuid4()) + present_user_id = str(uuid4()) + missing_user_id = str(uuid4()) + + entra_client.conditional_access_policies = { + policy_id: _make_policy( + policy_id, + excluded_users=[present_user_id, missing_user_id], + ), + } + entra_client.users = { + present_user_id: User( + id=present_user_id, + name="BreakGlass1", + on_premises_sync_enabled=False, + authentication_methods=[], + ), + # missing_user_id intentionally absent — exercises the + # `if not user: continue` short-circuit inside the loop. + } + + check = entra_break_glass_account_fido2_security_key_registered() + result = check.execute() + + # One finding for the present user; the missing one is skipped. + assert len(result) == 1 + assert result[0].status == "FAIL" + assert ( + "Cannot verify FIDO2 security key registration for break glass account BreakGlass1" + in result[0].status_extended + ) + assert "AuditLog.Read.All" in result[0].status_extended + assert result[0].resource == entra_client.users[present_user_id] + assert result[0].resource_name == "BreakGlass1" + assert result[0].resource_id == present_user_id diff --git a/tests/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable_test.py b/tests/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable_test.py index 28cc266347..8bf0c88f77 100644 --- a/tests/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable_test.py +++ b/tests/providers/m365/services/entra/entra_users_mfa_capable/entra_users_mfa_capable_test.py @@ -11,6 +11,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -53,6 +54,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -95,6 +97,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -153,6 +156,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -191,6 +195,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -248,6 +253,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -286,6 +292,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -324,6 +331,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -383,6 +391,7 @@ class Test_entra_users_mfa_capable: entra_client = mock.MagicMock entra_client.audited_tenant = "audited_tenant" entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = None with ( mock.patch( @@ -420,3 +429,125 @@ class Test_entra_users_mfa_capable: assert result[0].resource == entra_client.users[user_id] assert result[0].resource_name == "Test User" assert result[0].resource_id == user_id + + def test_user_registration_details_permission_error(self): + """Test FAIL when there's a permission error reading user registration details.""" + entra_client = mock.MagicMock + entra_client.audited_tenant = "audited_tenant" + entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All" + + with ( + mock.patch( + "prowler.providers.common.provider.Provider.get_global_provider", + return_value=set_mocked_m365_provider(), + ), + mock.patch( + "prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable.entra_client", + new=entra_client, + ), + ): + from prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable import ( + entra_users_mfa_capable, + ) + + user_id = str(uuid4()) + entra_client.users = { + user_id: User( + id=user_id, + name="Test User", + on_premises_sync_enabled=False, + directory_roles_ids=[], + is_mfa_capable=False, + account_enabled=True, + ) + } + + check = entra_users_mfa_capable() + result = check.execute() + + assert len(result) == 1 + assert result[0].status == "FAIL" + assert ( + "Cannot verify MFA capability for user Test User" + in result[0].status_extended + ) + assert "AuditLog.Read.All" in result[0].status_extended + assert result[0].resource == entra_client.users[user_id] + assert result[0].resource_name == "Test User" + assert result[0].resource_id == user_id + + def test_user_registration_details_permission_error_skips_guest_and_disabled(self): + """CIS-scope skip (Guest, disabled) still applies on the permission-error path. + + With ``user_registration_details_error`` set, only enabled member users + should receive a per-user "Cannot verify MFA capability" FAIL — guests + and disabled members are filtered out before the error branch runs. + """ + entra_client = mock.MagicMock + entra_client.audited_tenant = "audited_tenant" + entra_client.audited_domain = DOMAIN + entra_client.user_registration_details_error = "Insufficient privileges to read user registration details. Required permission: AuditLog.Read.All" + + with ( + mock.patch( + "prowler.providers.common.provider.Provider.get_global_provider", + return_value=set_mocked_m365_provider(), + ), + mock.patch( + "prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable.entra_client", + new=entra_client, + ), + ): + from prowler.providers.m365.services.entra.entra_users_mfa_capable.entra_users_mfa_capable import ( + entra_users_mfa_capable, + ) + + member_id = str(uuid4()) + guest_id = str(uuid4()) + disabled_member_id = str(uuid4()) + entra_client.users = { + member_id: User( + id=member_id, + name="Enabled Member", + on_premises_sync_enabled=False, + directory_roles_ids=[], + is_mfa_capable=False, + account_enabled=True, + user_type="Member", + ), + guest_id: User( + id=guest_id, + name="Guest User", + on_premises_sync_enabled=False, + directory_roles_ids=[], + is_mfa_capable=False, + account_enabled=True, + user_type="Guest", + ), + disabled_member_id: User( + id=disabled_member_id, + name="Disabled Member", + on_premises_sync_enabled=False, + directory_roles_ids=[], + is_mfa_capable=False, + account_enabled=False, + user_type="Member", + ), + } + + check = entra_users_mfa_capable() + result = check.execute() + + # Only the enabled member should be reported — Guest and + # disabled member are skipped before the error branch. + assert len(result) == 1 + assert result[0].status == "FAIL" + assert ( + "Cannot verify MFA capability for user Enabled Member" + in result[0].status_extended + ) + assert "AuditLog.Read.All" in result[0].status_extended + assert result[0].resource == entra_client.users[member_id] + assert result[0].resource_name == "Enabled Member" + assert result[0].resource_id == member_id diff --git a/tests/providers/m365/services/entra/microsoft365_entra_service_test.py b/tests/providers/m365/services/entra/microsoft365_entra_service_test.py index 541f8a3126..f2ee0b2c84 100644 --- a/tests/providers/m365/services/entra/microsoft365_entra_service_test.py +++ b/tests/providers/m365/services/entra/microsoft365_entra_service_test.py @@ -665,10 +665,11 @@ class Test_Entra_Service: ) ) - registration_details = asyncio.run( + registration_details, error_message = asyncio.run( entra_service._get_user_registration_details() ) + assert error_message is None assert registration_details == { "user-1": { "is_mfa_capable": True, @@ -686,6 +687,37 @@ class Test_Entra_Service: registration_builder.with_url.assert_called_once_with("next-link") registration_builder_next.get.assert_awaited() + def test__get_user_registration_details_returns_error_on_permission_denied(self): + """Test that 403 Authorization_RequestDenied returns an empty dict and + a descriptive error message naming the missing AuditLog.Read.All permission. + """ + from msgraph.generated.models.o_data_errors.main_error import MainError + from msgraph.generated.models.o_data_errors.o_data_error import ODataError + + odata_error = ODataError() + odata_error.error = MainError() + odata_error.error.code = "Authorization_RequestDenied" + + registration_builder = SimpleNamespace(get=AsyncMock(side_effect=odata_error)) + + entra_service = Entra.__new__(Entra) + entra_service.client = SimpleNamespace( + reports=SimpleNamespace( + authentication_methods=SimpleNamespace( + user_registration_details=registration_builder + ) + ) + ) + + registration_details, error_message = asyncio.run( + entra_service._get_user_registration_details() + ) + + assert registration_details == {} + assert error_message is not None + assert "AuditLog.Read.All" in error_message + assert "user registration details" in error_message + def test__get_service_principals_filters_third_party_owners(self): """Service principals owned by another tenant must not be returned.""" # Mixed-case input to verify the service normalizes both sides before