fix(azure): address review feedback on Azure certificate authentication

.gitignore

@@ -175,3 +175,4 @@ docker-compose.override.yml
 docker-compose-dev.override.yml
 # Local Pi runtime state
 .atl/
+.gga

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:

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:

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