fix(auth): centralize cert discovery logic and steps#17696
Conversation
- Centralize mTLS configuration file path discovery by replacing disparate checks in `check_use_client_cert` and `has_default_client_cert_source` with a unified `_get_cert_config_path` helper. - Ensure X.509 Workload Identity Federation (WIF) takes precedence over SecureConnect by checking `_get_cert_config_path` before `CONTEXT_AWARE_METADATA_PATH` in `has_default_client_cert_source`. - Remove temporary Cloud Run certificate and key path overrides from `_get_workload_cert_and_key_paths`. - Properly honor `CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH` in availability checks, fixing an existing mismatch where auto-enablement would evaluate to true but the downstream source checker would miss the path. - Allow mTLS auto-enablement to properly trigger off the default `gcloud` configuration file (`~/.config/gcloud/certificate_config.json`), resolving previous contradictory behavior. - Ensure strict environment variable precedence: invalid paths supplied via environment variables now correctly result in failures rather than silently falling back to default disk locations. - Update tests to reflect the new centralized resolution and precedence logic.
There was a problem hiding this comment.
Code Review
This pull request removes the temporary Cloud Run certificate path patch and its associated tests. It also refactors and simplifies certificate configuration path resolution in check_use_client_cert and has_default_client_cert_source by utilizing the helper function _get_cert_config_path, updating the corresponding unit tests to match. I have no feedback to provide as there are no review comments.
d91693a to
f95ebd0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors mTLS configuration path resolution by centralizing path retrieval in _get_cert_config_path and removing the temporary Cloud Run patch. It also changes mTLS auto-enablement failure logging from warnings to debug messages. The review feedback highlights a potential TypeError crash if the loaded certificate configuration JSON is not a dictionary, and suggests validating the JSON type before accessing its keys.
| try: | ||
| with open(cert_path, "r") as f: | ||
| content = json.load(f) | ||
| except (FileNotFoundError, OSError, json.JSONDecodeError) as e: |
There was a problem hiding this comment.
If the certificate configuration file contains a non-dictionary JSON value (such as null, 123, or true), json.load(f) will return a non-dict type. Checking "cert_configs" in content will raise a TypeError and crash the application.
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
- Do not silently fall back to standard TLS if mutual TLS (mTLS) configuration or certificate loading fails. Silently falling back can introduce security vulnerabilities or cause confusing downstream authorization errors; instead, fail fast by raising an appropriate exception.
There was a problem hiding this comment.
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.
andyrzhao
left a comment
There was a problem hiding this comment.
LGTM overall - thanks for working on this Negar!
| def test_no_env_vars_set(self): | ||
| @mock.patch("os.path.exists", autospec=True) | ||
| def test_no_env_vars_set(self, mock_exists, mock_open): | ||
| _mtls_helper._has_logged_mtls_warning = False |
There was a problem hiding this comment.
_has_logged_mtls_warning from _mtls_helper.py was deleted, but this still sets _mtls_helper._has_logged_mtls_warning = False
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The PR introduces two untested branches:
- No test calls has_default_client_cert_source(include_context_aware=False) to verify that setting include_context_aware=False skips checking CONTEXT_AWARE_METADATA_PATH.
- No test verifies that _get_cert_config_path logs a debug message when an explicitly specified certificate configuration file (is_explicit=True) does not exist.
check_use_client_certandhas_default_client_cert_sourcewith a unified_get_cert_config_pathhelper._get_cert_config_pathbeforeCONTEXT_AWARE_METADATA_PATHinhas_default_client_cert_source._get_workload_cert_and_key_paths.CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATHin availability checks, fixing an existing mismatch where auto-enablement would evaluate to true but the downstream source checker would miss the path.gcloudconfiguration file (~/.config/gcloud/certificate_config.json), resolving previous contradictory behavior._has_logged_mtls_warningmasks distinct mTLS configuration errors #17697