Repository navigation
fix(auth): add startup polling for Cloud Run SPIFFE mTLS certificates #18924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
99d2f85
ec909ed
60dc2fd
e514435
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| import subprocess | ||
| import sys | ||
| import tempfile | ||
| import time | ||
| from os import environ, getenv, path | ||
| from typing import Generator, List, Optional, Tuple, Union, cast | ||
| from urllib.parse import urlsplit | ||
|
|
@@ -415,7 +416,14 @@ def _get_workload_cert_and_key( | |
| the certificate or key information. | ||
| """ | ||
|
|
||
| cert_path, key_path, config_file_path = _get_workload_cert_and_key_paths( | ||
| spiffe_config_path = _get_spiffe_polling_config_path(certificate_config_path) | ||
| if spiffe_config_path is not None: | ||
| _, _, _, cert_data, key_data = _poll_spiffe_workload_credentials( | ||
| spiffe_config_path | ||
| ) | ||
| return cert_data, key_data | ||
|
|
||
| cert_path, key_path, config_file_path = _get_non_spiffe_workload_cert_and_key_paths( | ||
| certificate_config_path, include_context_aware | ||
| ) | ||
|
|
||
|
|
@@ -484,32 +492,34 @@ def _get_cert_config_path(certificate_config_path=None, include_context_aware=Tr | |
|
|
||
|
|
||
| def _get_workload_cert_and_key_paths(config_path=None, include_context_aware=True): | ||
| spiffe_config_path = _get_spiffe_polling_config_path(config_path) | ||
| if spiffe_config_path is not None: | ||
| cert_path, key_path, config_file_path, _, _ = _poll_spiffe_workload_credentials( | ||
| spiffe_config_path | ||
| ) | ||
| return cert_path, key_path, config_file_path | ||
|
|
||
| return _get_non_spiffe_workload_cert_and_key_paths( | ||
| config_path, include_context_aware | ||
| ) | ||
|
|
||
|
|
||
| def _get_non_spiffe_workload_cert_and_key_paths( | ||
| config_path=None, include_context_aware=True | ||
| ): | ||
| absolute_path = _get_cert_config_path(config_path, include_context_aware) | ||
| if absolute_path is None: | ||
| return None, None, None | ||
|
|
||
| data = _load_json_file(absolute_path) | ||
|
|
||
| if ( | ||
| not isinstance(data, dict) | ||
| or "cert_configs" not in data | ||
| or not isinstance(data["cert_configs"], dict) | ||
| ): | ||
| raise exceptions.ClientCertError( | ||
| 'Certificate config file {} is in an invalid format, a "cert configs" object is expected'.format( | ||
| absolute_path | ||
| ) | ||
| ) | ||
| cert_configs = data["cert_configs"] | ||
| cert_path, key_path = _extract_workload_paths_from_config(data, absolute_path) | ||
|
|
||
| # We return None, None if the expected workload fields are not present. | ||
| # The certificate config might be present for other types of connections (e.g. gECC), | ||
| # and we want to gracefully fallback to testing other mTLS configurations | ||
| # like SecureConnect instead of throwing an exception. | ||
|
|
||
| if ( | ||
| not isinstance(cert_configs, dict) or "workload" not in cert_configs | ||
| ) and config_path is None: | ||
| if cert_path is None and key_path is None and config_path is None: | ||
| default_home_path = path.expanduser( | ||
| os.path.join( | ||
| _cloud_sdk.get_config_path(), | ||
|
|
@@ -521,19 +531,53 @@ def _get_workload_cert_and_key_paths(config_path=None, include_context_aware=Tru | |
| ) != os.path.normpath(absolute_path): | ||
| try: | ||
| home_data = _load_json_file(default_home_path) | ||
| if isinstance(home_data, dict): | ||
| home_cert_configs = home_data.get("cert_configs") | ||
| if ( | ||
| isinstance(home_cert_configs, dict) | ||
| and "workload" in home_cert_configs | ||
| ): | ||
| cert_configs = home_cert_configs | ||
| absolute_path = default_home_path | ||
| except (exceptions.ClientCertError, OSError): | ||
| pass | ||
| home_data = None | ||
| if ( | ||
| isinstance(home_data, dict) | ||
| and isinstance(home_data.get("cert_configs"), dict) | ||
| and "workload" in home_data["cert_configs"] | ||
| ): | ||
| cert_path, key_path = _extract_workload_paths_from_config( | ||
| home_data, default_home_path | ||
| ) | ||
| return cert_path, key_path, default_home_path | ||
|
|
||
| return cert_path, key_path, absolute_path | ||
|
|
||
|
|
||
| def _get_spiffe_polling_config_path(config_path=None): | ||
| """Returns the expanded SPIFFE config path if startup polling applies, else None.""" | ||
| if _check_use_client_cert_env() is False: | ||
| return None | ||
| candidate_path = ( | ||
| config_path | ||
| if config_path is not None | ||
| else environ.get(environment_vars.GOOGLE_API_CERTIFICATE_CONFIG) | ||
| ) | ||
| if not candidate_path: | ||
| return None | ||
| expanded_path = path.expanduser(candidate_path) | ||
| if _agent_identity_utils._is_in_well_known_dir(expanded_path): | ||
| return expanded_path | ||
| return None | ||
|
|
||
| if not isinstance(cert_configs, dict) or "workload" not in cert_configs: | ||
| return None, None, absolute_path | ||
|
|
||
| def _extract_workload_paths_from_config(data, absolute_path): | ||
| """Validates certificate_config.json schema and returns (cert_path, key_path).""" | ||
| if ( | ||
| not isinstance(data, dict) | ||
| or "cert_configs" not in data | ||
| or not isinstance(data["cert_configs"], dict) | ||
| ): | ||
| raise exceptions.ClientCertError( | ||
| 'Certificate config file {} is in an invalid format, a "cert configs" object is expected'.format( | ||
| absolute_path | ||
| ) | ||
| ) | ||
| cert_configs = data["cert_configs"] | ||
| if "workload" not in cert_configs: | ||
| return None, None | ||
| workload = cert_configs["workload"] | ||
|
|
||
| if ( | ||
|
|
@@ -548,10 +592,53 @@ def _get_workload_cert_and_key_paths(config_path=None, include_context_aware=Tru | |
| absolute_path | ||
| ) | ||
| ) | ||
| cert_path = workload["cert_path"] | ||
| key_path = workload["key_path"] | ||
| return workload["cert_path"], workload["key_path"] | ||
|
|
||
| return cert_path, key_path, absolute_path | ||
|
|
||
| def _poll_spiffe_workload_credentials(spiffe_config_path): | ||
| """Polls up to 30s for SPIFFE workload config, cert, and key files to be ready.""" | ||
| last_error = None | ||
| for attempt, interval in enumerate(_agent_identity_utils._POLLING_INTERVALS): | ||
| try: | ||
| data = _load_json_file(spiffe_config_path) | ||
| except PermissionError: | ||
| raise | ||
| except (OSError, exceptions.ClientCertError) as caught_exc: | ||
| last_error = caught_exc | ||
| else: | ||
| cert_path, key_path = _extract_workload_paths_from_config( | ||
| data, spiffe_config_path | ||
| ) | ||
| if cert_path is None and key_path is None: | ||
| return None, None, spiffe_config_path, None, None | ||
|
|
||
| cert_path = path.expanduser(cert_path) | ||
| key_path = path.expanduser(key_path) | ||
| try: | ||
| cert_data, key_data = _read_cert_and_key_files(cert_path, key_path) | ||
| return cert_path, key_path, spiffe_config_path, cert_data, key_data | ||
| except PermissionError: | ||
| raise | ||
| except (OSError, exceptions.ClientCertError) as caught_exc: | ||
| if not ( | ||
| _agent_identity_utils._is_in_well_known_dir(cert_path) | ||
| and _agent_identity_utils._is_in_well_known_dir(key_path) | ||
| ): | ||
| raise | ||
| last_error = caught_exc | ||
|
|
||
| if attempt + 1 == _agent_identity_utils._FAST_POLL_CYCLES: | ||
| _LOGGER.debug( | ||
| "Identity certificate files are taking longer than 5 seconds to mount at %s. Retrying...", | ||
| spiffe_config_path, | ||
| ) | ||
| time.sleep(interval) | ||
|
|
||
| raise exceptions.ClientCertError( | ||
| "Identity certificate configuration or certificate/key files at {} were not ready after {}s".format( | ||
| spiffe_config_path, _agent_identity_utils._TOTAL_TIMEOUT | ||
| ) | ||
| ) from last_error | ||
|
|
||
|
|
||
| def _read_credential_bundle_file(bundle_path): | ||
|
|
@@ -839,6 +926,10 @@ def check_use_client_cert(): | |
| containing a "workload" section, or if no certificate config file is | ||
| configured/present and the GKE credential bundle exists at | ||
| /var/run/secrets/workload-spiffe-credentials/x509.credential-bundle.private-key.pem. | ||
| When GOOGLE_API_CERTIFICATE_CONFIG points to the well-known SPIFFE | ||
| directory (/var/run/secrets/workload-spiffe-credentials), this returns True | ||
| without waiting for the file to exist on disk so startup polling can run | ||
| when credentials are loaded. | ||
| Otherwise, it returns False. | ||
|
|
||
| Returns: | ||
|
|
@@ -849,6 +940,8 @@ def check_use_client_cert(): | |
| return env_override | ||
|
|
||
| # Auto-enablement checks (when GOOGLE_API_USE_CLIENT_CERTIFICATE is not set) | ||
| if _get_spiffe_polling_config_path() is not None: | ||
| return True | ||
|
Comment on lines
+943
to
+944
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: The docstring of |
||
|
|
||
| # Check if a workload config file exists. | ||
| cert_path = _get_cert_config_path(include_context_aware=True) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling
_get_spiffe_polling_config_path()and_poll_spiffe_workload_credentials()independently in both_get_workload_cert_and_key()(lines 419–424) and_get_workload_cert_and_key_paths()(lines 495–500) has a few side effects across callers:_get_spiffe_polling_config_path()call on every non-SPIFFE call to_get_workload_cert_and_key(): When_get_workload_cert_and_key(certificate_config_path, include_context_aware)runs for a non-SPIFFE path,_get_spiffe_polling_config_path(certificate_config_path)returnsNoneat line 419, and line 426 then calls_get_workload_cert_and_key_paths(certificate_config_path, include_context_aware), which immediately calls_get_spiffe_polling_config_path(config_path)a second time with the same argument at line 495. Extracting lines 502–538 into a helper (e.g._get_non_spiffe_workload_cert_and_key_paths(config_path=None, include_context_aware=True)) called by both functions after their_get_spiffe_polling_config_path()check avoids this duplicate call while keeping SPIFFE polling for direct callers of_get_workload_cert_and_key_paths().mtls.pycallers): Ingoogle/auth/transport/mtls.py,has_default_client_cert_source()(lines 45–55) wraps_get_workload_cert_and_key_paths()intry ... except (exceptions.ClientCertError, OSError): return Trueso the caller will surface the error. When SPIFFE files fail to mount within 30s,has_default_client_cert_source()blocks for 30s, catches the timeoutClientCertError, and returnsTrue; the caller (load_default_client_cert(),default_client_cert_source(),default_client_encrypted_cert_source(), orgrpc.SslCredentials) then calls_get_workload_cert_and_key(), which runs a second 30s polling loop (60s total; or 90s total inget_default_ssl_context()and GAPIC clients, which callhas_default_client_cert_source()twice before loading credentials)._get_workload_cert_and_key_paths()calls_poll_spiffe_workload_credentials()and discards(cert_data, key_data), callers that checkhas_default_client_cert_source()before callingget_client_ssl_credentials()(oridentity_pool.Credentials._get_cert_bytes(), which calls_get_workload_cert_and_key_paths()followed by_read_cert_file(cert_path)) read and regex-validate the files 2–3 times.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch! I extracted the non-SPIFFE path lookup into a shared helper and updated
mtls.has_default_client_cert_source()to returnTruewithout polling when the SPIFFE config path is configured.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracting the non-SPIFFE path lookup into a shared helper cleanly fixes the first point (
_get_spiffe_polling_config_path()being called twice on non-SPIFFE calls).However, returning
Truefrommtls.has_default_client_cert_source()without polling/reading the config when_get_spiffe_polling_config_path() is not Noneintroduces a regression when the SPIFFE config has{"cert_configs": {}}(no"workload"section, as tested intest_config_without_workload_section_does_not_poll):Because
_get_workload_cert_and_key()returns(None, None)(has_cert=False) rather than raisingClientCertErrorwhen"workload"is absent, havinghas_default_client_cert_source()returnTruewithout checking the file means:mtls.has_default_client_cert_source()returnsTruewhen no cert source exists (previously it returnedFalse).mtls.default_client_cert_source()no longer raisesMutualTLSChannelError("Default client cert source doesn't exist"), and its returnedcallback()returns(None, None)instead of(bytes, bytes)(causing GAPIC clients in_get_client_cert_source()to switch to*.mtls.googleapis.comand pass(None, None)to the transport).mtls.default_client_encrypted_cert_source()()crashes withTypeError: a bytes-like object is required, not 'NoneType'atcert_file.write(cert_bytes).grpc.SslCredentials().is_mtlsis set toTrueon construction before.ssl_credentialsis accessed.Also, for the 3rd point,
identity_pool.Credentials._get_cert_bytes()(google/auth/identity_pool.py:412-418) calls_mtls_helper._get_workload_cert_and_key_paths()directly (nothas_default_client_cert_source()) and then calls_mtls_helper._read_cert_file(cert_path), so_get_workload_cert_and_key_paths()calling_read_cert_and_key_files()inside_poll_spiffe_workload_credentials()still reads the cert file twice there.