diff --git a/prowler/changelog.d/external-checks-builtin-provider-resolution.fixed.md b/prowler/changelog.d/external-checks-builtin-provider-resolution.fixed.md new file mode 100644 index 0000000000..2720644613 --- /dev/null +++ b/prowler/changelog.d/external-checks-builtin-provider-resolution.fixed.md @@ -0,0 +1 @@ +Checks registered through the `prowler.checks.` entry-point group can now run against built-in providers. The built-in probe in `_resolve_check_module` used a bare `find_spec`, which imports the parent package to search it and so raised `ModuleNotFoundError` for a plug-in check instead of returning `None`, aborting the lookup before the entry points were consulted. Such a check was discovered, listed and selected for execution, then silently produced no findings. diff --git a/prowler/lib/check/check.py b/prowler/lib/check/check.py index a8e21c982e..283d8d2e8b 100644 --- a/prowler/lib/check/check.py +++ b/prowler/lib/check/check.py @@ -21,7 +21,11 @@ from prowler.lib.check.utils import recover_checks_from_provider from prowler.lib.logger import logger from prowler.lib.outputs.outputs import report from prowler.lib.utils.utils import open_file, parse_json_file, print_boxes -from prowler.providers.common.builtin import is_builtin_provider +from prowler.providers.common.builtin import ( + builtin_check_module, + is_builtin_check, + is_builtin_provider, +) from prowler.providers.common.models import Audit_Metadata @@ -401,21 +405,23 @@ def _resolve_check_module( when a plug-in tries to override, so the user knows their plug-in duplicate is being ignored and can rename it. - Gates the built-in branch on `is_builtin_provider(provider_type)` — - calling `find_spec` on `prowler.providers.{provider_type}.services...` - directly would propagate `ModuleNotFoundError` for external providers - (their parent package `prowler.providers.{provider_type}` does not - exist) instead of returning None. The leaf helper encapsulates the - safe lookup, so external providers go straight to entry points. For - built-ins we still use `find_spec` to distinguish "check doesn't - exist" from "check exists but failed to import" (broken transitive - dep, etc.). + Both probes are gated on leaf helpers rather than a raw `find_spec`, + because `find_spec` imports the parent package in order to search it and + so propagates `ModuleNotFoundError` instead of returning None whenever + that parent is absent. That happens on both axes: for an external + provider (no `prowler.providers.{provider_type}` package) and, on a + built-in provider, for an external check (no + `prowler.providers.{provider_type}.services.{service}.{check_name}` + package). Either one, probed naively, aborts the lookup before the entry + points are ever consulted. `is_builtin_check` still distinguishes "check + doesn't exist" from "check exists but failed to import" (broken + transitive dep, etc.), which a blanket except would flatten. """ # Built-in first — built-in wins on CheckID collision - if is_builtin_provider(provider_type): - builtin_path = f"prowler.providers.{provider_type}.services.{service}.{check_name}.{check_name}" - if importlib.util.find_spec(builtin_path) is not None: - return import_check(builtin_path) + if is_builtin_provider(provider_type) and is_builtin_check( + provider_type, service, check_name + ): + return import_check(builtin_check_module(provider_type, service, check_name)) # Entry point lookup — only consulted when the built-in truly doesn't exist for ep in importlib.metadata.entry_points(group=f"prowler.checks.{provider_type}"): diff --git a/prowler/providers/common/builtin.py b/prowler/providers/common/builtin.py index d60b5483d9..f726b73fd8 100644 --- a/prowler/providers/common/builtin.py +++ b/prowler/providers/common/builtin.py @@ -27,3 +27,44 @@ def is_builtin_provider(provider: str) -> bool: return spec is not None except (ImportError, ValueError): return False + + +def builtin_check_module(provider: str, service: str, check_name: str) -> str: + """Return the module path a built-in check would live at.""" + return f"prowler.providers.{provider}.services.{service}.{check_name}.{check_name}" + + +def is_builtin_check(provider: str, service: str, check_name: str) -> bool: + """Return True if the check's module ships with the SDK. + + Sibling of `is_builtin_provider`, and unsafe for the same reason if probed + naively: `find_spec` imports the parent package in order to search it, so + asking about a check that lives in a plug-in raises `ModuleNotFoundError` + rather than returning `None`. A check registered through + `prowler.checks.{provider}` never has a parent under + `prowler.providers.{provider}.services.{service}`, so the naive probe makes + every external check on a built-in provider unresolvable. + + Unlike its sibling this one narrows the exception instead of swallowing + every `ImportError`. A provider either ships with the SDK or it does not, + but callers rely on this probe to tell "the check is not built-in" apart + from "the check is built-in and its imports are broken". Reporting the + second as the first would turn a broken dependency into a silent + "check not found". + """ + module = builtin_check_module(provider, service, check_name) + try: + return importlib.util.find_spec(module) is not None + except ModuleNotFoundError as error: + # Only absorb "this check is simply not here". `error.name` is the + # module that could not be imported; when it is the check's own path + # (or a prefix of it) the check does not ship with the SDK. Anything + # else — a missing third-party dependency, say — belongs to a built-in + # check that does exist and must stay loud. + if error.name is None or ( + error.name != module and not module.startswith(f"{error.name}.") + ): + raise + return False + except ValueError: + return False diff --git a/tests/providers/common/builtin_test.py b/tests/providers/common/builtin_test.py new file mode 100644 index 0000000000..bf83fcf82a --- /dev/null +++ b/tests/providers/common/builtin_test.py @@ -0,0 +1,119 @@ +from unittest.mock import patch + +import pytest + +from prowler.providers.common.builtin import ( + builtin_check_module, + is_builtin_check, + is_builtin_provider, +) + + +class TestBuiltinCheckModule: + def test_builds_the_sdk_module_path(self): + assert ( + builtin_check_module("aws", "ec2", "ec2_instance_public_ip") + == "prowler.providers.aws.services.ec2.ec2_instance_public_ip.ec2_instance_public_ip" + ) + + +class TestIsBuiltinProvider: + def test_true_for_a_provider_shipped_with_the_sdk(self): + assert is_builtin_provider("aws") is True + + def test_false_for_a_provider_that_lives_in_a_plugin(self): + # No `prowler.providers.acme` package: find_spec raises on the absent + # parent rather than returning None, and the helper absorbs it. + assert is_builtin_provider("acme") is False + + +class TestIsBuiltinCheck: + def test_true_for_a_check_shipped_with_the_sdk(self): + assert is_builtin_check("aws", "ec2", "ec2_instance_public_ip") is True + + def test_false_for_an_external_check_on_a_builtin_provider(self): + """The case that made every plug-in check unresolvable. + + `prowler.providers.aws.services.ec2` exists, so the naive probe gets + far enough to try importing the check package as a parent — and that + package only exists inside the plug-in. find_spec raises instead of + returning None. + """ + assert ( + is_builtin_check("aws", "ec2", "ec2_acme_instance_has_owner_tag") is False + ) + + def test_false_for_a_service_that_does_not_exist(self): + assert ( + is_builtin_check("aws", "acmeservice", "acmeservice_thing_is_fine") is False + ) + + def test_false_for_an_external_provider(self): + assert ( + is_builtin_check("acme", "inventory", "inventory_item_has_owner") is False + ) + + def test_reraises_when_a_builtin_checks_own_dependency_is_missing(self): + """A broken import must not read as "the check is not built-in". + + Collapsing the two would turn a missing dependency into a silent + "check not found", which is the failure mode this probe exists to + avoid. + """ + module = builtin_check_module("aws", "ec2", "ec2_instance_public_ip") + + with patch( + "prowler.providers.common.builtin.importlib.util.find_spec", + side_effect=ModuleNotFoundError("No module named 'boto3'", name="boto3"), + ): + with pytest.raises(ModuleNotFoundError): + is_builtin_check("aws", "ec2", "ec2_instance_public_ip") + + # Sanity: the same error naming the check's own path is absorbed. + with patch( + "prowler.providers.common.builtin.importlib.util.find_spec", + side_effect=ModuleNotFoundError(f"No module named '{module}'", name=module), + ): + assert is_builtin_check("aws", "ec2", "ec2_instance_public_ip") is False + + def test_reraises_when_missing_module_name_is_only_a_textual_prefix(self): + """A sibling module prefix must not read as the check's missing parent.""" + sibling_prefix = "prowler.providers.aws.services.ec2.ec2" + + with patch( + "prowler.providers.common.builtin.importlib.util.find_spec", + side_effect=ModuleNotFoundError( + f"No module named '{sibling_prefix}'", name=sibling_prefix + ), + ): + with pytest.raises(ModuleNotFoundError): + is_builtin_check("aws", "ec2", "ec2_instance_public_ip") + + @pytest.mark.parametrize( + "error", + [ + ValueError("namespace package edge case"), + ], + ids=["value_error"], + ) + def test_false_when_find_spec_raises_value_error(self, error): + """Mirrors the guard `is_builtin_provider` already carries. + + `find_spec` can fail for reasons that are not "the module is absent" — + a namespace-package edge case raises ValueError. That does not say the + check ships with the SDK, so it falls through to the entry points. + """ + with patch( + "prowler.providers.common.builtin.importlib.util.find_spec", + side_effect=error, + ): + assert is_builtin_check("aws", "ec2", "ec2_instance_public_ip") is False + + def test_reraises_plain_import_error(self): + """A plain ImportError can indicate a broken built-in check import.""" + with patch( + "prowler.providers.common.builtin.importlib.util.find_spec", + side_effect=ImportError("partially initialised"), + ): + with pytest.raises(ImportError, match="partially initialised"): + is_builtin_check("aws", "ec2", "ec2_instance_public_ip") diff --git a/tests/providers/external/test_dynamic_provider_loading.py b/tests/providers/external/test_dynamic_provider_loading.py index ed367ad518..d4e7b4665d 100644 --- a/tests/providers/external/test_dynamic_provider_loading.py +++ b/tests/providers/external/test_dynamic_provider_loading.py @@ -1333,15 +1333,17 @@ class TestCheckDiscovery: class TestCheckExecution: """Tests 15-17: _resolve_check_module.""" - @patch("prowler.lib.check.check.importlib.util.find_spec") + @patch("prowler.lib.check.check.is_builtin_check") @patch("prowler.lib.check.check.import_check") - def test_resolve_check_module_builtin_first(self, mock_import, mock_find_spec): + def test_resolve_check_module_builtin_first( + self, mock_import, mock_is_builtin_check + ): """Test 15: _resolve_check_module resolves built-in checks first.""" from prowler.lib.check.check import _resolve_check_module mock_module = MagicMock() mock_import.return_value = mock_module - mock_find_spec.return_value = MagicMock() # built-in package exists + mock_is_builtin_check.return_value = True # built-in check exists result = _resolve_check_module("aws", "ec2", "my_check") @@ -1350,15 +1352,15 @@ class TestCheckExecution: "prowler.providers.aws.services.ec2.my_check.my_check" ) - @patch("prowler.lib.check.check.importlib.util.find_spec") + @patch("prowler.lib.check.check.is_builtin_check") @patch("prowler.lib.check.check.import_check") def test_resolve_check_module_fallback_to_entry_point( - self, mock_import_check, mock_find_spec + self, mock_import_check, mock_is_builtin_check ): """Test 16: _resolve_check_module falls back to entry point when built-in is absent.""" from prowler.lib.check.check import _resolve_check_module - mock_find_spec.return_value = None # built-in does not exist + mock_is_builtin_check.return_value = False # built-in does not exist mock_ext_module = MagicMock() ep = _make_entry_point( @@ -1375,10 +1377,10 @@ class TestCheckExecution: mock_imp.assert_called_with("ext_pkg.checks.my_check") mock_import_check.assert_not_called() - @patch("prowler.lib.check.check.importlib.util.find_spec") + @patch("prowler.lib.check.check.is_builtin_check") @patch("prowler.lib.check.check.import_check") def test_resolve_check_module_builtin_wins_over_entry_point( - self, mock_import_check, mock_find_spec + self, mock_import_check, mock_is_builtin_check ): """Regression guard: when both a built-in and an entry-point check exist with the same CheckID, the BUILT-IN wins. Plug-ins extend @@ -1389,7 +1391,7 @@ class TestCheckExecution: review (HugoPBrito).""" from prowler.lib.check.check import _resolve_check_module - mock_find_spec.return_value = MagicMock() # built-in exists + mock_is_builtin_check.return_value = True # built-in exists builtin_module = MagicMock() mock_import_check.return_value = builtin_module @@ -1414,21 +1416,23 @@ class TestCheckExecution: mock_imp.assert_not_called() @patch("prowler.lib.check.check.importlib.metadata.entry_points") - @patch("prowler.lib.check.check.importlib.util.find_spec") - def test_resolve_check_module_raises_when_not_found(self, mock_find_spec, mock_ep): + @patch("prowler.lib.check.check.is_builtin_check") + def test_resolve_check_module_raises_when_not_found( + self, mock_is_builtin_check, mock_ep + ): """Test 17: _resolve_check_module raises ModuleNotFoundError when both fail.""" from prowler.lib.check.check import _resolve_check_module - mock_find_spec.return_value = None + mock_is_builtin_check.return_value = False mock_ep.return_value = [] with pytest.raises(ModuleNotFoundError, match="not found"): _resolve_check_module("fake", "svc", "nonexistent_check") - @patch("prowler.lib.check.check.importlib.util.find_spec") + @patch("prowler.lib.check.check.is_builtin_check") @patch("prowler.lib.check.check.import_check") def test_resolve_check_module_surfaces_error_when_builtin_import_fails( - self, mock_import_check, mock_find_spec + self, mock_import_check, mock_is_builtin_check ): """Regression guard: when no plug-in entry-point overrides the check, a built-in whose module exists but fails to import (e.g. @@ -1437,7 +1441,7 @@ class TestCheckExecution: (HugoPBrito).""" from prowler.lib.check.check import _resolve_check_module - mock_find_spec.return_value = MagicMock() # built-in module exists + mock_is_builtin_check.return_value = True # built-in module exists mock_import_check.side_effect = ImportError("missing transitive dep: foo") # No plug-in override — the built-in's import failure must propagate @@ -1445,6 +1449,40 @@ class TestCheckExecution: with pytest.raises(ImportError, match="missing transitive dep"): _resolve_check_module("aws", "ec2", "ec2_instance_public_ip") + def test_resolve_check_module_entry_point_check_on_builtin_provider(self): + """Regression guard: a plug-in check attached to a BUILT-IN provider. + + Deliberately does not mock the built-in probe. The bug this guards + against was invisible to every other test here precisely because they + mock `find_spec` and hand it `None`, while the real call raises: it + imports `prowler.providers.aws.services.ec2.` as the parent it + must search, and that package only exists inside the plug-in. The raw + exception escaped `_resolve_check_module` before the entry points were + ever consulted, so no external check could run against aws, azure, gcp + or any other built-in provider. + """ + from prowler.lib.check.check import _resolve_check_module + + mock_module = MagicMock() + ep = _make_entry_point( + "ec2_acme_instance_has_owner_tag", + "acme_checks.services.ec2.ec2_acme_instance_has_owner_tag.ec2_acme_instance_has_owner_tag", + "prowler.checks.aws", + ) + + with ( + patch("importlib.metadata.entry_points", return_value=[ep]), + patch("importlib.import_module", return_value=mock_module) as mock_imp, + ): + result = _resolve_check_module( + "aws", "ec2", "ec2_acme_instance_has_owner_tag" + ) + + assert result is mock_module + mock_imp.assert_called_with( + "acme_checks.services.ec2.ec2_acme_instance_has_owner_tag.ec2_acme_instance_has_owner_tag" + ) + # =========================================================================== # 5. CLI Arguments