Skip to content
Open
152 changes: 151 additions & 1 deletion packages/google-auth/google/auth/transport/_mtls_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
import subprocess
import sys
import tempfile
import threading
from os import environ, getenv, path
from typing import Generator, List, Optional, Tuple, Union, cast
from typing import Callable, Generator, List, Optional, Tuple, Union, cast
from urllib.parse import urlsplit

from google.auth import _agent_identity_utils, _cloud_sdk, environment_vars, exceptions
Expand Down Expand Up @@ -967,3 +968,152 @@ def is_mtls_endpoint(url: Optional[Union[str, bytes, object]]) -> bool:
return False

return hostname in _MTLS_EXACT_HOSTS or hostname.endswith(_MTLS_HOST_SUFFIXES)


class _MtlsCertManager:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we re-export _MtlsCertManager from google.auth.transport.mtls here or in a follow-up PR so google-api-core doesn't have to import private _mtls_helper directly?

"""Thread-safe manager for mTLS client certificates and generation tracking.

Tracks the active client certificate bytes, private key bytes, leaf
certificate SHA-256 fingerprint, and a ``generation_id`` counter that
increments only when the loaded certificate changes.

Args:
client_cert_source (Optional[Callable[[], Tuple[bytes, bytes]]]):
Optional callback returning ``(cert_bytes, key_bytes)`` in PEM
format. When ``None``, certificates are discovered via
:func:`get_client_ssl_credentials`.
certificate_config_path (Optional[str]): Optional explicit path to a
``certificate_config.json`` file used when ``client_cert_source``
is ``None``.
"""

def __init__(
self,
client_cert_source: Optional[Callable[[], Tuple[bytes, bytes]]] = None,
certificate_config_path: Optional[str] = None,
) -> None:
self._client_cert_source = client_cert_source
self._certificate_config_path = certificate_config_path
self._cert: Optional[bytes] = None
self._key: Optional[bytes] = None
self._cert_fingerprint: Optional[str] = None
self._generation_id: int = 0
self._reload_id: int = 0
self._completed_reload_id: int = 0
self._lock = threading.Lock()

@property
def generation_id(self) -> int:
return self._generation_id

@property
def cert_fingerprint(self) -> Optional[str]:
"""Fingerprint of the current leaf certificate, or ``None`` if not loaded."""
return self._cert_fingerprint

def get_credential_snapshot(
self,
) -> Tuple[Optional[bytes], Optional[bytes], int]:
"""Returns the current ``(cert_bytes, key_bytes, generation_id)`` under the lock."""
with self._lock:
return self._cert, self._key, self._generation_id
Comment on lines +1018 to +1019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does get_credential_snapshot() need to block on self._lock? While refresh() holds the lock across disk reads, mismatch retries, callbacks, or cert_provider_command, snapshot callers on other threads or an async event loop have to wait behind that I/O instead of reading an atomic snapshot tuple.


def _load_cert_and_fingerprint(
self,
) -> Tuple[Optional[bytes], Optional[bytes], Optional[str]]:
"""Loads the certificate, private key, and leaf SHA-256 fingerprint."""
if self._client_cert_source is not None:
cert_bytes, key_bytes = self._client_cert_source()
else:
has_cert, cert_bytes, key_bytes, _ = get_client_ssl_credentials(
generate_encrypted_key=False,
certificate_config_path=self._certificate_config_path,
)
Comment on lines +1028 to +1031

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_client_ssl_credentials falls back to ~/.secureConnect/context_aware_metadata.json whenever workload discovery returns (None, None). If an explicit certificate_config_path or workload config is briefly missing during rotation on a host with ~/.secureConnect, refresh() swaps to the SecureConnect cert instead of keeping the cached cert, and a missing explicit certificate_config_path on initial load returns False instead of raising ClientCertError. Should default discovery here also check GOOGLE_API_USE_CLIENT_CERTIFICATE?

if not has_cert and self._generation_id == 0:
return None, None, None

if not cert_bytes or not key_bytes:
raise exceptions.ClientCertError(
"Client certificate or private key is empty."
)

cert_obj = _agent_identity_utils.parse_certificate(cert_bytes)
fingerprint = _agent_identity_utils.calculate_certificate_fingerprint(cert_obj)

return cert_bytes, key_bytes, fingerprint
Comment on lines +1040 to +1043

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_load_cert_and_fingerprint parses cert_bytes here, but never parses key_bytes or checks that it matches cert_obj. #19043 only runs inside _read_cert_and_key_files, so client_cert_source callbacks, GKE credential bundles, and SecureConnect bypass it. Even on workload files with #19043, an unparseable or encrypted key makes _has_cert_key_mismatch return False, which overwrites the working snapshot and latches _cert_fingerprint to the new cert so _key stays broken after a valid key file arrives.


def refresh(self, expected_generation: Optional[int] = None) -> bool:
"""Loads the client certificate and increments ``generation_id`` if it changed.

Args:
expected_generation (Optional[int]): Generation ID the caller was
using when a request failed. If ``generation_id`` is already
greater than ``expected_generation``, returns ``True`` without
reloading. If another thread started and finished a reload while
this call waited for the lock and the certificate did not change,
returns ``False`` without reloading. If ``None``, always reloads.

Returns:
bool: ``True`` if the certificate changed (or ``generation_id`` is
already greater than ``expected_generation``), ``False``
otherwise.

Raises:
google.auth.exceptions.ClientCertError: If loading or parsing the
certificate fails on the initial load (``generation_id == 0``).
"""
reload_id_at_entry = self._reload_id
with self._lock:
if expected_generation is not None:
if self._generation_id > expected_generation:
return True
# Only reuse a reload that started after entry and did not raise.
if self._completed_reload_id > reload_id_at_entry:
return False

self._reload_id += 1
try:
cert_bytes, key_bytes, fingerprint = self._load_cert_and_fingerprint()
except (
exceptions.ClientCertError,
exceptions.MutualTLSChannelError,
OSError,
ValueError,
ImportError,
) as caught_exc:
Comment on lines +1077 to +1083

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this catch Exception instead of listing five exception types? cryptography.x509.InvalidVersion doesn't inherit from ValueError, malformed context_aware_metadata.json can raise TypeError or IndexError, and custom client_cert_source callbacks can raise RuntimeError or custom errors, all of which crash live reloads at generation_id > 0 and leave _completed_reload_id behind _reload_id. Watch out for the gemini-code-assist suggestion below too, since it's from an earlier commit and drops self._completed_reload_id = self._reload_id.

if self._generation_id > 0:
_LOGGER.debug(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should suppressed reload failures log at warning instead of debug? Otherwise a broken cert path or permission error during rotation stays silent at default log levels until the cached cert expires hours later.

"Failed to reload mTLS client certificate; retaining "
"previous certificate (generation %d): %s",
self._generation_id,
caught_exc,
)
self._completed_reload_id = self._reload_id
return False
if isinstance(caught_exc, exceptions.ClientCertError):
raise
raise exceptions.ClientCertError(caught_exc) from caught_exc
Comment on lines +1077 to +1095

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The except block currently catches a specific list of exceptions (ClientCertError, MutualTLSChannelError, OSError, ValueError). However, when a custom client_cert_source callback is provided, it may raise other unexpected or custom exceptions (e.g., network errors, custom HSM/KMS exceptions, or even TypeError if the callback returns an unexpected format).

If such an exception is raised during a certificate reload (when self._generation_id > 0), it will propagate and crash the application/request instead of being gracefully handled as a transient reload error. To ensure maximum robustness and successfully retain the last known good certificate, we should catch all standard exceptions (Exception) during reload.

            except Exception as caught_exc:
                if self._generation_id > 0:
                    _LOGGER.debug(
                        "Failed to reload mTLS client certificate; retaining "
                        "previous certificate (generation %d): %s",
                        self._generation_id,
                        caught_exc,
                    )
                    return False
                if isinstance(caught_exc, exceptions.ClientCertError):
                    raise
                raise exceptions.ClientCertError(caught_exc) from caught_exc
References
  1. Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.


changed = cert_bytes is not None and fingerprint != self._cert_fingerprint

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should changed check cert_bytes != self._cert or key_bytes != self._key instead of only the leaf fingerprint? Right now if a PEM bundle renews an intermediate CA while keeping the same leaf cert, or updates key_bytes for the same leaf, refresh() returns False and keeps serving the stale bytes.

if changed:
self._cert = cert_bytes
self._key = key_bytes
self._cert_fingerprint = fingerprint
self._generation_id += 1

self._completed_reload_id = self._reload_id
return changed

def __copy__(self):
# Share the same thread-safe manager instance across shallow copies.
return self

def __deepcopy__(self, memo):
# Share the same thread-safe manager instance across deep copies.
return self
Comment thread
nbayati marked this conversation as resolved.
Resolved

def __reduce_ex__(self, protocol):
# The manager holds the private key and often an unpicklable callback
# (such as the closure from mtls.default_client_cert_source()). Objects
# that hold a manager must leave it out of their own pickled state.
raise TypeError(f"cannot pickle {type(self).__name__!r} object")
Loading
Loading