- Move certificate kwargs behind `*,` in `validate_arguments`, `setup_session`
and `verify_client` so pre-existing positional callers keep binding
`tenant_id`/`client_id`/`azure_credentials`/`region_config` correctly.
- Hoist the transient `RequestsTransport` in `verify_client` to a local so
`finally` can close it even when `CertificateCredential.__init__` raises
before the credential is bound.
- Drop the unused `client_id` parameter from `check_certificate_creds_env_vars`
(no caller propagates it) and always require `AZURE_CLIENT_ID` on the
pure env-var flow.
- Update `_normalize_pem_bundle` to iterate every private-key block so a
bundle whose leaf pairs with a non-first key is normalized correctly;
preserve the encrypted-key `TypeError` for single-key bundles.
- Add `inspect.signature(...).bind(...)` regressions for the restored
signatures and a multi-key PEM regression.
Address Hugo's four review comments on the certificate authentication
work:
1. `AzureProvider.__init__` and `AzureProvider.validate_static_credentials`
inserted the certificate kwargs between existing positional
parameters. A caller that previously passed `resource_groups` or
`region_config` positionally would silently rebind their argument to
a certificate flag. Both signatures now keep the pre-existing
positional layout and mark only the certificate kwargs as
keyword-only.
2. `verify_client`'s certificate path used to catch `ServiceRequestError`
directly from `credential.get_token()`, but `azure.identity` wraps
`_request_token` with `wrap_exceptions`, so a real connect or read
timeout arrived here as `ClientAuthenticationError` and was reported
to the user as an invalid certificate. Catch
`ClientAuthenticationError` and walk `__cause__`/`__context__` via
the new `_find_transport_cause`: a `ServiceRequestError` or
`ServiceResponseError` cause maps to
`AzureCredentialsUnavailableError`; anything else keeps the invalid
certificate mapping. Pass `retry_total=0` so Azure Core cannot
multiply the effective deadline, and close the transient credential
in `finally`, logging and swallowing cleanup failures so `close()`
cannot replace the primary typed exception.
3. Rewrite the certificate timeout tests to exercise the real
`CertificateCredential` and `RequestsTransport` pipeline, stubbing
only `requests.Session.request` with `ConnectTimeout` and
`ReadTimeout`. Assert `session.request.call_count == 1` to prove
`retry_total=0` is honoured, cover
`test_connection(..., raise_on_exception=False)`, and add a
cleanup-failure case proving `close()` cannot mask the typed error.
4. Add `inspect.signature(...).bind(...)` regressions for `__init__`,
`test_connection` and `validate_static_credentials` using the
pre-existing positional call shape, asserting the certificate
kwargs are `KEYWORD_ONLY`.
Replace the `ThreadPoolExecutor` + `future.result(timeout=...)` pattern
in `verify_client`'s certificate path with a `RequestsTransport` that
carries the connection/read deadlines. The executor approach could
not cancel a running `credential.get_token`, so timed-out or otherwise
failing calls left the underlying worker and network request alive:
under Entra ID degradation the API and Celery paths accumulated
background workers, and non-timeout exceptions bypassed executor
shutdown entirely.
Transport-layer timeouts terminate the request itself, so there is no
worker to leak and no cleanup path to miss. Translate the resulting
`ServiceRequestError` to `AzureCredentialsUnavailableError` to keep the
existing contract for `verify_client` and `test_connection`.
Update the timeout and leaf-first tests to match the new codepath.
Move `provider_id` back to its original positional slot in
`AzureProvider.test_connection`; the keyword-only barrier introduced by
the certificate kwargs was breaking external callers passing it
positionally.
Add the regression coverage Hugo asked for in the SDK PR review:
- `verify_client` translates `FuturesTimeoutError` to
`AzureCredentialsUnavailableError` for both certificate_content and
certificate_path (the background token-request path)
- `test_connection(..., raise_on_exception=False)` returns
`Connection(error=AzureCredentialsUnavailableError)` for both
certificate variants
- End-to-end leaf-first assertions for the remaining
`CertificateCredential` call sites: `setup_session` azure_credentials
certificate_path branch, `verify_client` with content and with path,
and `validate_static_credentials` re-encoded output
Reproduce the original bug where `--tenant-id` was ignored when
AZURE_TENANT_ID was unset: `check_certificate_creds_env_vars` must not
raise when the explicit tenant replaces a missing env var.
Add the missing `requests.exceptions.Timeout` coverage: verify_client
translates the transport error to AzureCredentialsUnavailableError, and
test_connection with raise_on_exception=False returns
Connection(error=AzureCredentialsUnavailableError) instead of a raw
transport exception.
Assert end-to-end that CertificateCredential receives a leaf-first
bundle on both the env-var / --certificate-path branch and the
azure_credentials (API/UI) branch. A regression that skips the
normalization at any call site would silently break auth today; the
helper-only test could not catch that.
`cryptography.hazmat.primitives.serialization.load_pem_private_key`
raises TypeError, not ValueError, when the caller passes password=None
against an encrypted key. Add TypeError to verify_client's except tuple
so that path becomes AzureNotValidCertificateContentError or
AzureNotValidCertificatePathError instead of leaking. Assert the leaf-
first ordering explicitly in the bundle test so a regression that returns
the input unchanged cannot silently pass.
Normalize the PEM bundle at every CertificateCredential call site so
azure-identity uses the leaf certificate for its thumbprint even when
the source bundle lists an intermediate first. `check_certificate_creds_env_vars`
now accepts the explicit CLI tenant_id/client_id so callers don't require
matching AZURE_* env vars when they already have the values. Catch
requests.exceptions.Timeout on the client-secret verification path so the
Entra ID timeout raises a typed AzureCredentialsUnavailableError instead
of leaking a requests exception.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- verify_client certificate branch now manages the ThreadPoolExecutor
explicitly so shutdown(wait=False) on timeout does not extend the
30s deadline while credential.get_token is still running.
- client-secret token endpoint uses the shared
_TOKEN_ACQUISITION_TIMEOUT_SECONDS constant instead of hardcoded 30.
- setup_session env-var certificate branch catches TypeError (raised by
load_pem_private_key when a PEM key is password-protected and no
password is supplied) alongside binascii.Error, OSError and friends.
- setup_session re-raises AzureNotValidCertificateContentError and
AzureNotValidCertificatePathError before the outer except Exception
wraps them into AzureSetUpSessionError, so callers still see the
certificate-specific error type.
- validate_arguments error message no longer names --certificate-content
as a CLI flag (the option only exists on the API/UI credential shape).
- Changelog fragment drops the redundant 'Add' verb per the prowler
changelog convention.
- Test paths that need a non-existent file use tmp_path instead of
hardcoded /tmp/ locations that could collide on shared runners.
- validate_arguments, setup_identity and verify_client docstrings
document the new certificate parameters and typed errors.
Address the 15 findings from the SDK code review:
- Certificate bundle validation now walks every PEM certificate block so
intermediate-before-leaf order (openssl / Key Vault exports) is
accepted, covers encrypted PKCS#8/DSA/OpenSSH private-key labels, and
catches cryptography.UnsupportedAlgorithm alongside ValueError.
- validate_arguments rejects --certificate-content/--certificate-path
without --certificate-auth (or a full static-credentials trio) and no
longer requires --tenant-id when --certificate-auth is used with an
env-var flow. --certificate-auth --tenant-id X no longer mistakenly
raises the browser-auth error.
- setup_session prefers explicit --tenant-id over AZURE_TENANT_ID on the
env-var certificate path, gates the env-var check on the absence of a
static-credentials dict, runs validate_certificate_bundle before
instantiating CertificateCredential, and maps base64/OS errors to
typed certificate errors.
- verify_client runs the certificate get_token off-thread with a 30s
hard timeout so a stalled Entra ID endpoint cannot pin a request
thread or Celery worker, and catches the same base64/OS errors on the
certificate branch.
- _compute_certificate_thumbprint logs each parser failure instead of
silently discarding them, and the setattr on CertificateCredential
falls back to a module-level map keyed by id() so a future
azure-identity release that adds __slots__ cannot break the feature.
Address CodeRabbit review:
- Log caught exceptions in the certificate content and path validation
handlers so failures are diagnosable from the log file, matching the
established caught-exception logging idiom.
- Assert the full credentials dict in the certificate acceptance tests
so a stray truthy client_secret or certificate_content that would
route setup_session to the wrong branch is caught.
Add certificate-based Service Principal authentication to the Azure
provider. AzureProvider accepts a certificate (base64 content or file
path) and authenticates via azure.identity.CertificateCredential,
mirroring the M365 provider flow. Includes CLI flags
(--certificate-auth, --certificate-content, --certificate-path),
key-pair validation for PEM and PKCS#12 bundles, and unit tests.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Daniel Barranquero <danielbo2001@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Lydia Vilchez