-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(auth): centralize cert discovery logic and steps #17696
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
3aa269b
5533e0c
f95ebd0
8d40f20
05b0d64
9497cb4
bb69aa9
7522a19
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 |
|---|---|---|
|
|
@@ -51,30 +51,10 @@ | |
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # A flag to track if we have already logged a warning about mTLS auto-enablement failures. | ||
| # This prevents log spam when client libraries create transports or session instances | ||
| # frequently within a single process. | ||
| _has_logged_mtls_warning = False | ||
|
|
||
|
|
||
| _PASSPHRASE_REGEX = re.compile( | ||
| b"-----BEGIN PASSPHRASE-----(.+)-----END PASSPHRASE-----", re.DOTALL | ||
| ) | ||
|
|
||
| # Temporary patch to accomodate incorrect cert config in Cloud Run prod environment. | ||
| _WELL_KNOWN_CLOUD_RUN_CERT_PATH = ( | ||
| "/var/run/secrets/workload-spiffe-credentials/certificates.pem" | ||
| ) | ||
| _WELL_KNOWN_CLOUD_RUN_KEY_PATH = ( | ||
| "/var/run/secrets/workload-spiffe-credentials/private_key.pem" | ||
| ) | ||
| _INCORRECT_CLOUD_RUN_CERT_PATH = ( | ||
| "/var/lib/volumes/certificate/workload-certificates/certificates.pem" | ||
| ) | ||
| _INCORRECT_CLOUD_RUN_KEY_PATH = ( | ||
| "/var/lib/volumes/certificate/workload-certificates/private_key.pem" | ||
| ) | ||
|
|
||
|
|
||
| class _MemfdCreationError(OSError): | ||
| """Raised when Linux in-memory virtual file creation (memfd) fails.""" | ||
|
|
@@ -436,22 +416,35 @@ def _get_cert_config_path(certificate_config_path=None, include_context_aware=Tr | |
| The absolute path of the certificate config file, and None if the file does not exist. | ||
| """ | ||
|
|
||
| source = "function argument" | ||
| is_explicit = True | ||
| if certificate_config_path is None: | ||
| env_path = environ.get(environment_vars.GOOGLE_API_CERTIFICATE_CONFIG, None) | ||
| if env_path is not None and env_path != "": | ||
| certificate_config_path = env_path | ||
| source = ( | ||
| f"environment variable {environment_vars.GOOGLE_API_CERTIFICATE_CONFIG}" | ||
| ) | ||
| else: | ||
| env_path = environ.get( | ||
| environment_vars.CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH, | ||
| None, | ||
| ) | ||
| if include_context_aware and env_path is not None and env_path != "": | ||
| certificate_config_path = env_path | ||
| source = f"environment variable {environment_vars.CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH}" | ||
| else: | ||
| certificate_config_path = CERTIFICATE_CONFIGURATION_DEFAULT_PATH | ||
| is_explicit = False | ||
|
|
||
| certificate_config_path = path.expanduser(certificate_config_path) | ||
| if not path.exists(certificate_config_path): | ||
| if is_explicit: | ||
| _LOGGER.debug( | ||
| "Certificate configuration file explicitly specified via %s at %s does not exist", | ||
| source, | ||
| certificate_config_path, | ||
| ) | ||
| return None | ||
| return certificate_config_path | ||
|
|
||
|
|
@@ -489,25 +482,6 @@ def _get_workload_cert_and_key_paths(config_path, include_context_aware=True): | |
| cert_path = workload["cert_path"] | ||
| key_path = workload["key_path"] | ||
|
|
||
| # == BEGIN Temporary Cloud Run PATCH == | ||
| # See https://github.com/googleapis/google-auth-library-python/issues/1881 | ||
| if (cert_path == _INCORRECT_CLOUD_RUN_CERT_PATH) and ( | ||
| key_path == _INCORRECT_CLOUD_RUN_KEY_PATH | ||
| ): | ||
| if not path.exists(cert_path) and not path.exists(key_path): | ||
| _LOGGER.debug( | ||
| "Applying Cloud Run certificate path patch. " | ||
| "Configured paths not found: %s, %s. " | ||
| "Using well-known paths: %s, %s", | ||
| cert_path, | ||
| key_path, | ||
| _WELL_KNOWN_CLOUD_RUN_CERT_PATH, | ||
| _WELL_KNOWN_CLOUD_RUN_KEY_PATH, | ||
| ) | ||
| cert_path = _WELL_KNOWN_CLOUD_RUN_CERT_PATH | ||
| key_path = _WELL_KNOWN_CLOUD_RUN_KEY_PATH | ||
| # == END Temporary Cloud Run PATCH == | ||
|
|
||
| return cert_path, key_path | ||
|
|
||
|
|
||
|
|
@@ -755,36 +729,33 @@ def check_use_client_cert(): | |
| will default to False. | ||
| If GOOGLE_API_USE_CLIENT_CERTIFICATE is unset, the value will be inferred | ||
| as True (auto-enabled) if a workload config file exists (pointed at by | ||
| GOOGLE_API_CERTIFICATE_CONFIG) containing a "workload" section. | ||
| GOOGLE_API_CERTIFICATE_CONFIG or CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH, | ||
| or the default path like ~/.config/gcloud/certificate_config.json) | ||
| containing a "workload" section. | ||
| Otherwise, it returns False. | ||
|
|
||
| Returns: | ||
| bool: Whether the client certificate should be used for mTLS connection. | ||
| """ | ||
| global _has_logged_mtls_warning | ||
| env_override = _check_use_client_cert_env() | ||
| if env_override is not None: | ||
| return env_override | ||
|
|
||
| # Auto-enablement checks (when GOOGLE_API_USE_CLIENT_CERTIFICATE is not set) | ||
|
|
||
| # Check if the value of GOOGLE_API_CERTIFICATE_CONFIG is set. | ||
| cert_path = getenv(environment_vars.GOOGLE_API_CERTIFICATE_CONFIG) or getenv( | ||
| environment_vars.CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH | ||
| ) | ||
| # Check if a workload config file exists. | ||
| cert_path = _get_cert_config_path(include_context_aware=True) | ||
|
|
||
| if cert_path: | ||
| try: | ||
| with open(cert_path, "r") as f: | ||
| content = json.load(f) | ||
| except (FileNotFoundError, OSError, json.JSONDecodeError) as e: | ||
|
Comment on lines
750
to
753
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. If the certificate configuration file contains a non-dictionary JSON value (such as Furthermore, to prevent security vulnerabilities or confusing downstream authorization errors, we must not silently fall back to standard TLS when mTLS configuration or certificate loading fails. Instead, we should fail fast by raising an appropriate exception. try:
with open(cert_path, "r") as f:
content = json.load(f)
if not isinstance(content, dict):
raise ValueError("Certificate configuration file is not a JSON object.")
except (FileNotFoundError, OSError, json.JSONDecodeError) as e:References
Contributor
Author
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. The first point is incorrect. The code already explicitly performs an isinstance(content, dict) check before interacting with the JSON payload, so no TypeError can occur. Regarding the suggestion to raise an exception: check_use_client_cert() has historically fallen back to standard TLS by returning False if auto-enablement cannot proceed. Changing this behavior to raise an exception would be a breaking change for downstream users and violates backwards compatibility. We will stick with the current graceful fallback behavior. |
||
| if not _has_logged_mtls_warning: | ||
| _LOGGER.warning( | ||
| "mTLS auto-enablement failed: Could not read/parse certificate file at %s. Error: %s", | ||
| cert_path, | ||
| e, | ||
| ) | ||
| _has_logged_mtls_warning = True | ||
| _LOGGER.debug( | ||
| "mTLS auto-enablement failed: Could not read/parse certificate file at %s. Error: %s", | ||
| cert_path, | ||
| e, | ||
| ) | ||
| return False | ||
|
|
||
| # Structural validation | ||
|
|
@@ -794,12 +765,10 @@ def check_use_client_cert(): | |
| return True | ||
|
|
||
| # If we got here, the file exists but the expected structure is missing | ||
| if not _has_logged_mtls_warning: | ||
| _LOGGER.warning( | ||
| "mTLS auto-enablement failed: Certificate configuration file at %s is missing the required ['cert_configs']['workload'] section.", | ||
| cert_path, | ||
| ) | ||
| _has_logged_mtls_warning = True | ||
| _LOGGER.debug( | ||
| "mTLS auto-enablement failed: Certificate configuration file at %s is missing the required ['cert_configs']['workload'] section.", | ||
| cert_path, | ||
| ) | ||
| return False | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,6 @@ | |
| from google.auth import exceptions | ||
| from google.auth.transport import _mtls_helper | ||
|
|
||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
|
|
||
|
|
@@ -44,25 +43,18 @@ def has_default_client_cert_source(include_context_aware=True): | |
| Returns: | ||
| bool: indicating if the default client cert source exists. | ||
| """ | ||
| cert_path = _mtls_helper._get_cert_config_path( | ||
| include_context_aware=include_context_aware | ||
| ) | ||
| if cert_path is not None: | ||
| return True | ||
| if ( | ||
| include_context_aware | ||
| and _mtls_helper._check_config_path(_mtls_helper.CONTEXT_AWARE_METADATA_PATH) | ||
| is not None | ||
| ): | ||
| return True | ||
| if ( | ||
| _mtls_helper._check_config_path( | ||
| _mtls_helper.CERTIFICATE_CONFIGURATION_DEFAULT_PATH | ||
| ) | ||
| is not None | ||
| ): | ||
| return True | ||
| cert_config_path = getenv("GOOGLE_API_CERTIFICATE_CONFIG") | ||
| if ( | ||
| cert_config_path | ||
| and _mtls_helper._check_config_path(cert_config_path) is not None | ||
| ): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
|
|
@@ -146,7 +138,8 @@ def should_use_client_cert(): | |
| If GOOGLE_API_USE_CLIENT_CERTIFICATE is set to true or false, a corresponding | ||
| bool value will be returned | ||
| If GOOGLE_API_USE_CLIENT_CERTIFICATE is unset, the value will be inferred by | ||
| reading a file pointed at by GOOGLE_API_CERTIFICATE_CONFIG, and verifying it | ||
| reading a file pointed at by GOOGLE_API_CERTIFICATE_CONFIG or | ||
|
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. The docstring for check_use_client_cert() in _mtls_helper.py lists three paths: GOOGLE_API_CERTIFICATE_CONFIG, CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH, and ~/.config/gcloud/certificate_config.json. The updated docstring for should_use_client_cert() in mtls.py omits the default ~/.config/gcloud/certificate_config.json path. |
||
| CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH, and verifying it | ||
| contains a "workload" section. If so, the function will return True, | ||
| otherwise False. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.