From bd5afb698181c0ef7f6475a7122fe52e4b10b631 Mon Sep 17 00:00:00 2001 From: Lydia Vilchez Date: Thu, 27 Aug 2026 12:46:13 +0200 Subject: [PATCH] fix(azure): address review feedback on Azure certificate authentication --- .gitignore | 1 + prowler/providers/azure/azure_provider.py | 64 ++++++++---- prowler/providers/azure/lib/certificate.py | 55 +++++++--- tests/providers/azure/azure_provider_test.py | 101 +++++++++++++++++++ 4 files changed, 192 insertions(+), 29 deletions(-) diff --git a/.gitignore b/.gitignore index 4545146688..1090980ddf 100644 --- a/.gitignore +++ b/.gitignore @@ -175,3 +175,4 @@ docker-compose.override.yml docker-compose-dev.override.yml # Local Pi runtime state .atl/ +.gga diff --git a/prowler/providers/azure/azure_provider.py b/prowler/providers/azure/azure_provider.py index cc28dc78bc..1dd71c2ac5 100644 --- a/prowler/providers/azure/azure_provider.py +++ b/prowler/providers/azure/azure_provider.py @@ -111,6 +111,20 @@ def _find_transport_cause(error: BaseException) -> Optional[BaseException]: return None +def _decode_certificate_content(certificate_content: str) -> bytes: + """Base64-decode certificate content, tolerating shell-added whitespace. + + ``AZURE_CERTIFICATE_CONTENT`` values almost always arrive with a + trailing newline (shell command substitution) or embedded line + breaks (``openssl base64`` wraps output at 64 columns by default). + ``base64.b64decode(..., validate=True)`` rejects both as + ``binascii.Error``. Strip whitespace before decoding so the strict + validation still rejects non-base64 characters without penalising the + common copy-paste and env-var flows. + """ + return base64.b64decode("".join(certificate_content.split()), validate=True) + + def _build_certificate_credential( tenant_id: str, client_id: str, @@ -417,9 +431,9 @@ class AzureProvider(Provider): sp_env_auth, browser_auth, managed_identity_auth, - certificate_auth, subscription_ids, client_id, + certificate_auth=certificate_auth, ) # TODO: should we keep this here or within the identity? @@ -813,8 +827,8 @@ class AzureProvider(Provider): with open(certificate_path, "rb") as cert_file: certificate_data = cert_file.read() else: - certificate_data = base64.b64decode( - getenv("AZURE_CERTIFICATE_CONTENT"), validate=True + certificate_data = _decode_certificate_content( + getenv("AZURE_CERTIFICATE_CONTENT") ) # Same fail-fast validation the static path runs. The # returned bundle is normalized so the leaf appears @@ -1134,13 +1148,20 @@ class AzureProvider(Provider): if raise_on_exception: raise credential_unavailable_error return Connection(error=credential_unavailable_error) - except AzureDefaultAzureCredentialError as default_credentials_error: + except AzureNotValidCertificateContentError as certificate_content_error: logger.error( - f"{default_credentials_error.__class__.__name__}[{default_credentials_error.__traceback__.tb_lineno}]: {default_credentials_error}" + f"{certificate_content_error.__class__.__name__}[{certificate_content_error.__traceback__.tb_lineno}]: {certificate_content_error}" ) if raise_on_exception: - raise default_credentials_error - return Connection(error=default_credentials_error) + raise certificate_content_error + return Connection(error=certificate_content_error) + except AzureNotValidCertificatePathError as certificate_path_error: + logger.error( + f"{certificate_path_error.__class__.__name__}[{certificate_path_error.__traceback__.tb_lineno}]: {certificate_path_error}" + ) + if raise_on_exception: + raise certificate_path_error + return Connection(error=certificate_path_error) except ( AzureClientIdAndClientSecretNotBelongingToTenantIdError ) as tenant_id_error: @@ -1261,9 +1282,13 @@ class AzureProvider(Provider): sp_env_auth, browser_auth, managed_identity_auth, - certificate_auth, subscription_ids, client_id, + # Keyword-only so external callers using the pre-cert-auth positional + # layout (`..., managed_identity_auth, subscription_ids, client_id`) + # keep binding those slots correctly. + *, + certificate_auth: bool = False, ): """ Sets up the identity for the Azure provider. @@ -1643,7 +1668,7 @@ class AzureProvider(Provider): # exception several call frames deeper if this fails, which # makes for a bad UX in the API/UI. normalized_bundle = validate_certificate_bundle( - base64.b64decode(certificate_content, validate=True) + _decode_certificate_content(certificate_content) ) # Persist the normalized (leaf-first) bundle so downstream # `CertificateCredential` calls don't pick an intermediate CA. @@ -1821,7 +1846,7 @@ class AzureProvider(Provider): transport = None try: if certificate_content: - certificate_data = base64.b64decode(certificate_content, validate=True) + certificate_data = _decode_certificate_content(certificate_content) elif certificate_path: with open(certificate_path, "rb") as cert_file: certificate_data = cert_file.read() @@ -1855,15 +1880,17 @@ class AzureProvider(Provider): f"{error.__class__.__name__}[{error.__traceback__.tb_lineno}] -- {error}" ) # `azure.identity` wraps `_request_token` with `wrap_exceptions`, - # so a `RequestsTransport` connect/read timeout reaches this + # so a `RequestsTransport` connect/read failure reaches this # handler as `ClientAuthenticationError`. Distinguish transport # failures (credentials unavailable) from a genuinely rejected - # certificate by walking the cause chain. + # certificate by walking the cause chain. The transport error + # covers DNS, TLS, connection reset and timeout indistinctly, so + # the message must not claim the request timed out. if _find_transport_cause(error) is not None: raise AzureCredentialsUnavailableError( file=os.path.basename(__file__), message=( - "Timed out waiting for Entra ID to issue a token " + "Entra ID was not reachable while acquiring a token " "for the provided certificate." ), original_exception=error, @@ -1908,18 +1935,21 @@ class AzureProvider(Provider): # Always release the transient credential and its transport: # cleanup failures are logged and swallowed so they cannot # replace the primary typed exception on its way up. The - # transport is closed independently to cover the case where - # `CertificateCredential.__init__` raised before `credential` - # took ownership of it. + # transport is closed as a fallback when the credential was + # never assigned (`CertificateCredential.__init__` raised + # before ownership transferred) or when `credential.close()` + # itself failed and could not release the underlying pipeline. + credential_closed = False if credential is not None: try: credential.close() + credential_closed = True except Exception as cleanup_error: logger.warning( f"{cleanup_error.__class__.__name__}: failed to close " f"CertificateCredential during verify_client cleanup" ) - elif transport is not None: + if transport is not None and not credential_closed: try: transport.close() except Exception as cleanup_error: diff --git a/prowler/providers/azure/lib/certificate.py b/prowler/providers/azure/lib/certificate.py index ace1dca877..600719d948 100644 --- a/prowler/providers/azure/lib/certificate.py +++ b/prowler/providers/azure/lib/certificate.py @@ -39,26 +39,57 @@ def validate_certificate_bundle(certificate_data: bytes) -> bytes: returned as-is. """ try: - private_key, certificate, _ = pkcs12.load_key_and_certificates( + private_key, certificate, additional_certs = pkcs12.load_key_and_certificates( certificate_data, None, default_backend() ) - except (ValueError, UnsupportedAlgorithm): - # Not PKCS#12, or PKCS#12 uses a cipher this build cannot decrypt. - # Fall through to the PEM path. + except (ValueError, UnsupportedAlgorithm) as error: + # `load_key_and_certificates` also raises `ValueError` when the + # PKCS#12 archive is password-protected (message text: "Invalid + # password or PKCS12 data"). Fall through to the PEM parser only + # when the payload smells like PEM; otherwise raise a specific + # error so the caller does not see the misleading "missing + # certificate or key" message from `_normalize_pem_bundle`. + if b"-----BEGIN" not in certificate_data: + raise ValueError( + "the payload is not a valid PEM bundle nor an unencrypted " + "PKCS#12 archive; password-protected PKCS#12 archives are " + "not supported" + ) from error return _normalize_pem_bundle(certificate_data) - if certificate is None or private_key is None: - raise ValueError("the payload must contain a certificate and its private key") + if private_key is None: + raise ValueError("the PKCS#12 archive does not contain a private key") + if certificate is None and not additional_certs: + raise ValueError("the PKCS#12 archive does not contain a certificate") encoding = serialization.Encoding.DER public_format = serialization.PublicFormat.SubjectPublicKeyInfo - if certificate.public_key().public_bytes( - encoding, public_format - ) != private_key.public_key().public_bytes(encoding, public_format): - raise ValueError("the certificate does not match the private key") + key_public_bytes = private_key.public_key().public_bytes(encoding, public_format) + if ( + certificate is not None + and certificate.public_key().public_bytes(encoding, public_format) + == key_public_bytes + ): + # PKCS#12 blobs are consumed directly by azure-identity; no reordering. + return certificate_data - # PKCS#12 blobs are consumed directly by azure-identity; no reordering. - return certificate_data + # Some `openssl pkcs12 -export -certfile` workflows write the leaf + # certificate into the additional-certs bag instead of the primary + # slot. azure-identity reads the primary certificate for the + # thumbprint, so accepting the archive as-is would authenticate + # against the wrong thumbprint. Detect that case and raise an + # actionable error rather than the opaque "does not match" message. + for candidate in additional_certs or (): + if ( + candidate.public_key().public_bytes(encoding, public_format) + == key_public_bytes + ): + raise ValueError( + "the PKCS#12 archive has the certificate matching the " + "private key in the additional-certs bag; re-export the " + "archive with the leaf certificate as the primary entry" + ) + raise ValueError("the certificate does not match the private key") def _normalize_pem_bundle(certificate_data: bytes) -> bytes: diff --git a/tests/providers/azure/azure_provider_test.py b/tests/providers/azure/azure_provider_test.py index eb58b91f12..bf38d2a926 100644 --- a/tests/providers/azure/azure_provider_test.py +++ b/tests/providers/azure/azure_provider_test.py @@ -1391,6 +1391,37 @@ class TestAzureProviderCertificateAuth: region_config=self._region_config(), ) + def test_validate_static_credentials_accepts_wrapped_base64_cert_content(self): + # `openssl base64 -in cert.pfx` wraps at 64 columns by default and + # every shell command substitution adds a trailing newline. The + # UX target is that both are accepted without the operator having + # to strip whitespace by hand — strict `validate=True` decoding + # would otherwise reject perfectly valid payloads with a + # misleading `binascii.Error`. + import base64 + import textwrap + + bundle = self._leaf_first_bundle() + wrapped_with_trailing_newline = ( + textwrap.fill(base64.b64encode(bundle).decode("ascii"), width=64) + "\n" + ) + + with patch.object(AzureProvider, "verify_client"): + credentials = AzureProvider.validate_static_credentials( + tenant_id=self._TENANT_ID, + client_id=self._CLIENT_ID, + client_secret=None, + certificate_content=wrapped_with_trailing_newline, + certificate_path=None, + region_config=self._region_config(), + ) + + # `validate_static_credentials` re-encodes the normalized bundle + # into `certificate_content`; the returned value must decode + # cleanly with strict validation and match the original bundle. + stored = base64.b64decode(credentials["certificate_content"], validate=True) + assert stored == bundle + def test_validate_static_credentials_rejects_key_only_pem(self): import base64 @@ -2215,6 +2246,47 @@ class TestAzureProviderCertificateAuth: ).decode(), ) + def test_verify_client_certificate_cleanup_failure_still_closes_transport(self): + # When `credential.close()` fails, `verify_client` must still fall + # back to closing the underlying `RequestsTransport`; otherwise the + # transport (and its connection pool + retry thread) leaks for the + # lifetime of the worker every time Entra ID misbehaves. + import base64 + + from azure.core.pipeline.transport import RequestsTransport + + with ( + patch.object( + requests.Session, + "request", + side_effect=requests.ConnectTimeout("hung transport"), + ), + patch( + "azure.identity.CertificateCredential.close", + side_effect=RuntimeError("close boom"), + ), + patch.object( + RequestsTransport, + "close", + autospec=True, + ) as transport_close, + pytest.raises(AzureCredentialsUnavailableError), + ): + AzureProvider.verify_client( + self._TENANT_ID, + self._CLIENT_ID, + client_secret=None, + region_config=self._region_config(), + certificate_content=base64.b64encode( + self._leaf_first_bundle() + ).decode(), + ) + + assert transport_close.called, ( + "transport.close() must be invoked as a fallback when " + "credential.close() fails, otherwise the transport leaks" + ) + def test_verify_client_without_credential_material_returns(self): assert ( AzureProvider.verify_client( @@ -3064,6 +3136,35 @@ class TestAzureProviderSignatureCompatibility: parameters[name].kind is inspect.Parameter.KEYWORD_ONLY ), f"setup_session: {name} must be keyword-only" + def test_setup_identity_positional_call_still_binds_subscription_ids(self): + import inspect + + # Master signature: `self, az_cli_auth, sp_env_auth, browser_auth, + # managed_identity_auth, subscription_ids, client_id`. Inserting + # `certificate_auth` as a positional between `managed_identity_auth` + # and `subscription_ids` would silently rebind existing callers. + signature = inspect.signature(AzureProvider.setup_identity) + bound = signature.bind( + None, # self + False, # az_cli_auth + False, # sp_env_auth + False, # browser_auth + False, # managed_identity_auth + ["sub-a"], # subscription_ids + self._CLIENT_ID, # client_id + ) + + assert bound.arguments["subscription_ids"] == ["sub-a"] + assert bound.arguments["client_id"] == self._CLIENT_ID + assert "certificate_auth" not in bound.arguments + + # `certificate_auth` is the only certificate parameter in + # `setup_identity`; assert it is keyword-only. + parameters = inspect.signature(AzureProvider.setup_identity).parameters + assert ( + parameters["certificate_auth"].kind is inspect.Parameter.KEYWORD_ONLY + ), "setup_identity: certificate_auth must be keyword-only" + class TestValidateCertificateBundleMultiKey: """A PEM bundle may legitimately carry more than one private key block