Repository navigation
Conversation
…requests Allow callers to opt in or out of binding metadata server ID tokens to the workload certificate via `IDTokenCredentials`, `fetch_id_token`, and `fetch_id_token_credentials`.
There was a problem hiding this comment.
Code Review
This pull request introduces support for falling back to the GKE workload credential bundle path when no explicit or implicit certificate configuration is present. It also adds a bind_id_token parameter across several credential and token-fetching functions to allow explicit control over certificate-bound ID tokens. The review feedback highlights a potential crash when parsing malformed JSON configuration files, suggesting that ValueError should be caught alongside ClientCertError and OSError in has_default_client_cert_source, and that the corresponding test should be updated to mock a ValueError to align with this behavior.
| except (exceptions.ClientCertError, OSError): | ||
| cert_path, key_path, config_file_path = None, None, "" |
There was a problem hiding this comment.
The _resolve_workload_cert_and_key_paths function loads a JSON file using json.load(), which can raise a json.JSONDecodeError (a subclass of ValueError) if the file is malformed. Since ValueError is not caught by except (exceptions.ClientCertError, OSError):, any malformed configuration file will cause an unhandled exception and crash the application. We should catch ValueError as well to handle this gracefully.
| except (exceptions.ClientCertError, OSError): | |
| cert_path, key_path, config_file_path = None, None, "" | |
| except (exceptions.ClientCertError, OSError, ValueError): | |
| cert_path, key_path, config_file_path = None, None, "" |
There was a problem hiding this comment.
False flag, rejecting the suggestion.
_load_json_file already catches ValueError (including json.JSONDecodeError) and wraps it in ClientCertError.
| @mock.patch( | ||
| "google.auth.transport._mtls_helper._load_json_file", | ||
| side_effect=exceptions.ClientCertError("Invalid JSON"), | ||
| autospec=True, | ||
| ) |
There was a problem hiding this comment.
In the test test_has_default_client_cert_source_malformed_config_no_gke_fallback, _load_json_file is mocked to raise exceptions.ClientCertError("Invalid JSON"). However, in production, _load_json_file raises json.JSONDecodeError (which inherits from ValueError) when parsing malformed JSON. To make the test more realistic and align with the exception handling in mtls.py, we should mock it to raise ValueError instead.
| @mock.patch( | |
| "google.auth.transport._mtls_helper._load_json_file", | |
| side_effect=exceptions.ClientCertError("Invalid JSON"), | |
| autospec=True, | |
| ) | |
| @mock.patch( | |
| "google.auth.transport._mtls_helper._load_json_file", | |
| side_effect=ValueError("Invalid JSON"), | |
| autospec=True, | |
| ) |
There was a problem hiding this comment.
Rejecting the suggestion. _load_json_file catches ValueError and raises ClientCertError, so mocking _load_json_file to raise ClientCertError matches its exception contract.
…ata into IDTokenCredentials
| except exceptions.ClientCertError: | ||
| # Config exists, but is malformed. Let caller surface error. | ||
| return True | ||
| except OSError: |
There was a problem hiding this comment.
Catching OSError and returning False here causes grpc.SslCredentials, load_default_client_cert(), and get_default_ssl_context() to fall back to plain TLS when certificate_config.json is unreadable, while requests still raises MutualTLSChannelError.
Can we catch ClientCertError and OSError together and return True so unreadable configs surface the error consistently?
There was a problem hiding this comment.
I think we'll need a refactoring of the auth SDK to ensure we're consistently raising exceptions instead of falling back to TLS, following go/sdk-mtls-by-default-cert-discovery. but I agree that at least in the new code paths we should follow the correct behavior, done!
| return ( | ||
| config_file_path is None | ||
| and not _has_explicit_cert_config_env() | ||
| and path.exists(_GKE_CREDENTIAL_BUNDLE_PATH) |
There was a problem hiding this comment.
Checking path.exists here while get_agent_identity_certificate_path() uses _is_certificate_file_ready means an empty or unreadable bundle file auto-enables mTLS while token binding skips it.
Should we consistently use the same check across both?
| if cert_path is not None and key_path is not None: | ||
| return True | ||
| if ( | ||
| _mtls_helper._check_use_client_cert_env() is not False |
There was a problem hiding this comment.
Gating only the GKE bundle branch on _check_use_client_cert_env() is not False means GOOGLE_API_USE_CLIENT_CERTIFICATE=false returns False here while a workload config file still returns True.
Should that env check apply to both branches, or neither since callers check should_use_client_cert() separately?
There was a problem hiding this comment.
Added to both branches.
| target_audience=target_audience, | ||
| use_metadata_identity_endpoint=True, | ||
| quota_project_id=self._quota_project_id, | ||
| bind_id_token=getattr(self, "_bind_id_token", None), |
There was a problem hiding this comment.
Passing bind_id_token to self.__class__ breaks IDTokenCredentials subclasses that keep the existing __init__ signature.
Should we only pass bind_id_token when it is not None?
There was a problem hiding this comment.
Good catch. Updated the code so that with_target_audience and with_quota_project copy _bind_id_token onto the returned instance after construction instead of passing bind_id_token to self.class.
This matches how _make_copy copies internal attributes in service_account and compute_engine.
| except OSError: | ||
| cert_path, key_path, config_file_path = None, None, "" | ||
|
|
||
| if cert_path is not None and key_path is not None: |
There was a problem hiding this comment.
Requiring cert_path and key_path here changes has_default_client_cert_source() from True to False for a certificate_config.json without a workload section, so default_client_cert_source() now raises MutualTLSChannelError instead of returning a None, None callback. Is that change intended?
| bind_id_token (Optional[bool]): Controls whether to request a | ||
| certificate-bound ID token. Can only be set when | ||
| ``use_metadata_identity_endpoint`` is ``True``. | ||
| If ``True``, requests a bound token whenever a valid workload |
There was a problem hiding this comment.
The documented behavior doesn't seem to match the actual behavior:
- When
GOOGLE_API_CERTIFICATE_CONFIGpoints to the well-known path and the cert file is missing,bind_id_token=Truepolls and raisesRefreshErrorrather than falling back to an unbound token, and binding also requires an Agent Identity SPIFFE cert withGOOGLE_API_USE_CLIENT_CERTIFICATEnot set tofalse.
Can we update this docstring and the copies in id_token.py to note that?
There was a problem hiding this comment.
When GOOGLE_API_CERTIFICATE_CONFIG points to the well-known path and the cert file is missing, bind_id_token=True polls and raises RefreshError rather than falling back to an unbound token
This is our intended behavior when an explicit config points to a missing cert. I added RefreshError to the Raises: section of fetch_id_token for missing or invalid configured certificates (IDTokenCredentials.refresh already documents RefreshError).
binding also requires an Agent Identity SPIFFE cert with GOOGLE_API_USE_CLIENT_CERTIFICATE not set to false
Updated the docstrings in credentials.py, _metadata.py, id_token.py, and _id_token_async.py to specify agentic certificate instead of workload certificate. Kept the description focused on GOOGLE_API_ENABLE_RUNTIME_BOUND_TOKEN as the dedicated bound-token switch rather than listing low-level path or mTLS env var details.
| params = {"audience": self._target_audience, "format": "full"} | ||
| bind_id_token = getattr(self, "_bind_id_token", None) | ||
| if bind_id_token is not None: | ||
| bind_token = bind_id_token |
There was a problem hiding this comment.
nit: I find it risky that both bind_token and bind_id_token are present in the scope. Can one of them be given a more distinct name?
Do you need both? Or can you do something like:
bind_token = getattr(self, "_bind_id_token", None)
if bind_token is None:
bind_token = not os.environ.get(
environment_vars.GOOGLE_API_CERTIFICATE_CONFIG
)
method, body, headers = _metadata._build_token_request_options(
metrics.token_request_id_token_mds()
metrics.token_request_id_token_mds(),
bind_token=bind_token,
)
There was a problem hiding this comment.
Yep you're right we don't need both, done!
|
|
||
|
|
||
| def _has_gke_credential_bundle(config_file_path=None): | ||
| """Returns True if GKE workload credential bundle should be used as fallback.""" |
There was a problem hiding this comment.
nit: the function name and the docstring seem inconsistent. Or is it always the case that if the bundle exists, we should use it?
There was a problem hiding this comment.
It looks like this just tests for existence of the file. Is there a chance this file could be used for other things in the future? Would that cause issues, if we assume the file means mtls should be enabled?
| return cert_path, key_path, absolute_path | ||
|
|
||
|
|
||
| def _get_workload_cert_and_key_paths(config_path, include_context_aware=True): |
There was a problem hiding this comment.
nit: do we need this helper? It seems to be a pretty thin wrapper around a different one
| cert_config_path = os.environ.get(environment_vars.GOOGLE_API_CERTIFICATE_CONFIG) | ||
|
|
||
| if not cert_config_path: | ||
| from google.auth.transport import _mtls_helper |
There was a problem hiding this comment.
Does this import need to be in the middle of the function? Or can we keep it at the top?
| ) | ||
| if cert_path is not None: | ||
| if _mtls_helper._check_use_client_cert_env() is False: | ||
| return False |
There was a problem hiding this comment.
This check seems to change the purpose of this function. It's not just checking if credentials exist on the device anymore. Is that safe?
This is a public function that others may be relying on
There was a problem hiding this comment.
Good catch, you're right. I hadn't realized that the other function was already checking this condition. I removed it so it only checks if credentials exist on the device, since callers already gate mTLS on should_use_client_cert().
| if bind_id_token is not None: | ||
| bind_token = bind_id_token | ||
| else: | ||
| # Temporary gate: keep default ID tokens unbound when |
There was a problem hiding this comment.
Is there a bug we can attach to track when this can be removed?
| environment_vars.CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH, | ||
| raising=False, | ||
| ) | ||
| monkeypatch.setattr( |
There was a problem hiding this comment.
iam.py calls check_use_client_cert() at import time to set _IAM_DOMAIN, which happens during test collection before this fixture runs. Can we also redirect _mtls_helper._GKE_CREDENTIAL_BUNDLE_PATH in pytest_configure so those 9 IAM and impersonation tests stay hermetic if the bundle file exists on the host?
| if config_file_path is not None or _has_explicit_cert_config_env(): | ||
| return False | ||
| try: | ||
| return _agent_identity_utils._is_certificate_file_ready( |
There was a problem hiding this comment.
_is_certificate_file_ready only runs os.stat, so a non-empty bundle that fails open() with PermissionError still turns on mTLS here and makes configure_mtls_channel raise MutualTLSChannelError, while the token binding path warns and falls back to an unbound token. Should unreadable bundles consistently raise or skip, and can we add an open-time PermissionError test to TestReadCredentialBundleFile?
| return False | ||
| return False | ||
|
|
||
| return _has_gke_credential_bundle(cert_path) |
There was a problem hiding this comment.
PR #18486 only updated requests.Request, so urllib3.Request and aiohttp_requests.Request still send to iamcredentials.mtls.googleapis.com without a client cert when this returns True. Should those transports also auto-configure mTLS when check_use_client_cert() is True, and can we rebase this branch onto #18486?
| _mtls_helper._get_cert_config_path() | ||
| ) | ||
| ): | ||
| return _mtls_helper._GKE_CREDENTIAL_BUNDLE_PATH |
There was a problem hiding this comment.
When this returns _GKE_CREDENTIAL_BUNDLE_PATH, get_agent_identity_certificate_and_bytes() only checks _CERT_REGEX and never verifies _KEY_REGEX. Should it also check that the bundle contains a single private key so we don't mint a bound token when _read_credential_bundle_file() will reject the bundle for mTLS?
|
|
||
| # Default gcloud config path, to be used with path.expanduser for cross-platform compatibility. | ||
| CERTIFICATE_CONFIGURATION_DEFAULT_PATH = "~/.config/gcloud/certificate_config.json" | ||
| _GKE_CREDENTIAL_BUNDLE_PATH = "/var/run/secrets/workload-spiffe-credentials/x509.credential-bundle.private-key.pem" |
There was a problem hiding this comment.
_has_gke_credential_bundle enables mTLS whenever this file is non-empty without checking the SPIFFE trust domain, while should_request_bound_token() checks _is_agent_identity_certificate. Is that split intended if a non-Agent-Identity pod certificate is projected here?
| include_context_aware=include_context_aware | ||
| ) | ||
| if cert_path is not None: | ||
| if _mtls_helper._check_use_client_cert_env() is False: |
There was a problem hiding this comment.
Nit: Only the false case of this check is tested right now, so changing is False to is not None still passes the test suite. Can we add a test case with GOOGLE_API_USE_CLIENT_CERTIFICATE set to true?
| assert self.credentials._universe_domain == "googleapis.com" | ||
| assert not self.credentials._universe_domain_cached | ||
|
|
||
| @mock.patch( |
There was a problem hiding this comment.
Nit: Now that clean_cert_config_env is autouse in tests/conftest.py, get_agent_identity_certificate_and_bytes() already returns None, None by default. Can we drop the leftover mock.patch decorators and unused mock_get_agent_cert arguments in this file?
| class TestReadCredentialBundleFile(object): | ||
| def test_single_cert_and_key(self, tmpdir): | ||
| bundle_file = tmpdir.join("x509.credential-bundle.private-key.pem") | ||
| bundle_file.write_binary(pytest.public_cert_bytes + pytest.private_key_bytes) |
There was a problem hiding this comment.
Nit: Real GKE podCertificate bundles write the PRIVATE KEY block before the CERTIFICATE chain. Can we add a test case with the key first so the production PEM order is covered?
| mock_get_cert_config_path.return_value = "/path/to/cert" | ||
| mock_load_json_file.return_value = {"cert_configs": {"workload": workload}} | ||
|
|
||
| with pytest.raises(exceptions.ClientCertError): |
There was a problem hiding this comment.
Nit: Can we add match='Workload certificate configuration is missing "cert_path" or "key_path"' here so this doesn't pass on an unrelated ClientCertError?
| ) | ||
|
|
||
| async def _recover_auth_state(): | ||
| is_mtls_endpoint = False |
There was a problem hiding this comment.
Nit: This change already landed on main and seems unrelated to GKE bundle discovery. Should it drop out once the branch is rebased?
This PR enables bound access tokens, bound ID tokens, and default mTLS certificate discovery for Agent Identity workloads on GKE, and adds per-request control for ID token binding:
Fall back to the well-known GKE workload credential bundle (
/var/run/secrets/workload-spiffe-credentials/x509.credential-bundle.private-key.pem) in_agent_identity_utils.get_agent_identity_certificate_path(),_mtls_helper._get_workload_cert_and_key(),_mtls_helper.check_use_client_cert(), andmtls.has_default_client_cert_source()when no explicit or default certificate config file is present.Add an optional
bind_id_tokenparameter (True,False, orNone) tocompute_engine.IDTokenCredentials,fetch_id_token_credentials(), andfetch_id_token()(sync and async) so callers can explicitly opt in or out of binding metadata server ID tokens per request.Scope default bound ID tokens to GKE by keeping ID tokens unbound when
GOOGLE_API_CERTIFICATE_CONFIGis set (unlessbind_id_token=True), while continuing to bind access tokens by default on both Cloud Run and GKE.Add and update unit tests for GKE credential bundle fallback,
bind_id_tokenoptions, and theGOOGLE_API_CERTIFICATE_CONFIGdefault gate.Note:
GKE mounts a single combined PEM file (
x509.credential-bundle.private-key.pem) at/var/run/secrets/workload-spiffe-credentials/without settingGOOGLE_API_CERTIFICATE_CONFIG. With this fallback, GKE Agent Identity workloads automatically use bound access tokens and bound ID tokens by default when the file is present.On Cloud Run (where
GOOGLE_API_CERTIFICATE_CONFIGis set), default ID token requests (bind_id_token=None) temporarily remain unbound so existing callers do not receive bound ID tokens. Callers on Cloud Run can passbind_id_token=Trueto request bound ID tokens now, and a follow-up PR will remove this temporary gate to enable bound ID tokens by default on Cloud Run as well.design: go/sdk-mds-bound-token