Skip to content

Commit b9a1379

Browse files
authored
fix(auth): prevent TypeError and support home-dir cert fallback for X… (#18016)
….509 WIF on ECP machines - Prevent TypeError crash in identity_pool.py by raising ClientCertError if _get_mtls_cert_and_key_paths() returns None for the certificate path. - Add fallback in _mtls_helper.py to check the default home directory configuration ~/.config/gcloud/certificate_config.json if the env-var-resolved config does not contain a workload block. - Add unit tests to cover both behaviors and verify they function correctly. Fixes: b/542359992 ## Bug / Context Fixes the TypeError (NoneType) crash that occurs when developers attempt X.509 Workload Identity Federation on ECP machines. (b/542359992) ## Changes - **Fallback to user configuration:** In `_mtls_helper.py`, if the ECP system-wide config lacks a `workload` block, fallback to check the home folder's `~/.config/gcloud/certificate_config.json`. - **TypeError Prevention:** In `identity_pool.py`, raise a clean `ClientCertError` if no certificate path is configured, preventing a generic `NoneType` crash in `open()`. - **Defensive Check:** Defensively assert that loaded JSON config data is a dictionary before accessing fields. - **Unit Tests:** Added tests to cover these fallback and error handling scenarios. ## Verification You can verify the fix by running this python script with the local packages loaded: ```python import os import sys import json from google.auth import identity_pool from google.auth import exceptions # Setup dummy config using default cert path config cred_config = { "type": "external_account", "audience": "//iam.googleapis.com/projects/123456/locations/global/workloadIdentityPools/test-pool/providers/test-provider", "subject_token_type": "urn:ietf:params:oauth:token-type:jwt", "token_url": "https://sts.googleapis.com/v1/token", "credential_source": { "certificate": { "use_default_certificate_config": True } } } config_file = "verify_cred_config.json" with open(config_file, "w") as f: json.dump(cred_config, f) try: credentials = identity_pool.Credentials.from_file(config_file) cert_bytes = credentials._get_cert_bytes() print("Success: Certificate read successfully.") except exceptions.ClientCertError as e: print(f"Verified: Prevented TypeError crash. Raised ClientCertError: {e}") finally: if os.path.exists(config_file): os.remove(config_file)
1 parent 61435be commit b9a1379

5 files changed

Lines changed: 331 additions & 4 deletions

File tree

GEMINI.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Google Cloud Python Workspace Rules
2+
3+
These guidelines are automatically applied to Python development tasks within this repository.
4+
5+
---
6+
7+
## 1. Filesystem and Path Resolution
8+
* **Dynamic Configuration Directories:** Never hardcode paths like `~/.config/gcloud/` or standard user directories. Always utilize existing SDK helpers (such as `_cloud_sdk.get_config_path()`) to dynamically locate system and configuration files.
9+
* **Path Normalization:** When comparing path strings (especially paths retrieved from environment variables or dynamically built), always normalize them using `os.path.normpath` or `pathlib.Path` to prevent Windows vs Unix slash mismatch issues (`\` vs `/`).
10+
11+
## 2. Input Validation (Defensive Programming)
12+
* **Untrusted File Inputs:** Any data loaded from external configuration files (JSON, YAML, CSV) is untrusted. Always type-validate structure (e.g. check `isinstance(data, dict)` and `isinstance(data.get("sub_key"), dict)`) *before* indexing or calling dictionary lookup keys, avoiding `TypeError` exceptions.
13+
14+
## 3. Exception Contract Compliance
15+
* **Public Interface Contracts:** When introducing new exception pathways in internal helpers, always trace their propagation. If a public-facing API method (e.g. `refresh()`) is documented to raise a specific base exception class (like `RefreshError`), wrap lower-level custom exceptions (like `ClientCertError`) or system exceptions (like `OSError`) and re-raise them under the correct interface exception types.
16+
* **Self-Contained Fallbacks:** Fallback logic must be resilient and self-contained. Always wrap fallback configuration loading in try-except blocks to catch expected exceptions (like `ClientCertError` or `OSError`) and bypass failures gracefully.
17+
18+
## 4. Unit Testing and Mock Hygiene
19+
* **Localized Mocking:** When mocking standard functions or filesystem checks (like `path.exists`), mock the local module import path (e.g., `google.auth.transport._mtls_helper.path.exists`) instead of patching builtins globally (e.g., `os.path.exists`), ensuring mocks are isolated.
20+
* **Fallback Verification:** Fallback test cases must explicitly verify execution flow by asserting the expected call sequence and arguments of mocked helpers using `assert_called_once_with` or `assert_has_calls`.

packages/google-auth/google/auth/identity_pool.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,10 @@ def _get_mtls_cert_and_key_paths(self):
412412

413413
def _get_cert_bytes(self):
414414
cert_path, _ = self._get_mtls_cert_and_key_paths()
415+
if cert_path is None:
416+
raise exceptions.ClientCertError(
417+
"Workload certificate configuration could not be found or does not contain workload certificate paths."
418+
)
415419
return _mtls_helper._read_cert_file(cert_path)
416420

417421
def _mtls_required(self):
@@ -568,7 +572,13 @@ def refresh(self, request):
568572
cert_fingerprint = None
569573
# Check if the credential is X.509 based.
570574
if self._credential_source_certificate is not None:
571-
cert_bytes = self._get_cert_bytes()
575+
try:
576+
cert_bytes = self._get_cert_bytes()
577+
except (exceptions.ClientCertError, OSError) as e:
578+
raise exceptions.RefreshError(
579+
"Failed to retrieve certificate bytes for external"
580+
" account credentials"
581+
) from e
572582
cert = _agent_identity_utils.parse_certificate(cert_bytes)
573583
if _agent_identity_utils.should_request_bound_token(cert):
574584
cert_fingerprint = (

packages/google-auth/google/auth/transport/_mtls_helper.py

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,11 @@ def _get_workload_cert_and_key_paths(config_path, include_context_aware=True):
459459

460460
data = _load_json_file(absolute_path)
461461

462-
if "cert_configs" not in data:
462+
if (
463+
not isinstance(data, dict)
464+
or "cert_configs" not in data
465+
or not isinstance(data["cert_configs"], dict)
466+
):
463467
raise exceptions.ClientCertError(
464468
'Certificate config file {} is in an invalid format, a "cert configs" object is expected'.format(
465469
absolute_path
@@ -472,11 +476,40 @@ def _get_workload_cert_and_key_paths(config_path, include_context_aware=True):
472476
# and we want to gracefully fallback to testing other mTLS configurations
473477
# like SecureConnect instead of throwing an exception.
474478

475-
if "workload" not in cert_configs:
479+
if (
480+
not isinstance(cert_configs, dict) or "workload" not in cert_configs
481+
) and config_path is None:
482+
default_home_path = path.expanduser(
483+
os.path.join(
484+
_cloud_sdk.get_config_path(),
485+
"certificate_config.json",
486+
)
487+
)
488+
if path.exists(default_home_path) and os.path.normpath(
489+
default_home_path
490+
) != os.path.normpath(absolute_path):
491+
try:
492+
home_data = _load_json_file(default_home_path)
493+
if isinstance(home_data, dict):
494+
home_cert_configs = home_data.get("cert_configs")
495+
if (
496+
isinstance(home_cert_configs, dict)
497+
and "workload" in home_cert_configs
498+
):
499+
cert_configs = home_cert_configs
500+
absolute_path = default_home_path
501+
except (exceptions.ClientCertError, OSError):
502+
pass
503+
504+
if not isinstance(cert_configs, dict) or "workload" not in cert_configs:
476505
return None, None
477506
workload = cert_configs["workload"]
478507

479-
if "cert_path" not in workload or "key_path" not in workload:
508+
if (
509+
not isinstance(workload, dict)
510+
or "cert_path" not in workload
511+
or "key_path" not in workload
512+
):
480513
raise exceptions.ClientCertError(
481514
'Workload certificate configuration is missing "cert_path" or "key_path" in {}'.format(
482515
absolute_path

packages/google-auth/tests/test_identity_pool.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1784,6 +1784,57 @@ def test_get_mtls_certs_invalid(self):
17841784
'The credential is not configured to use mtls requests. The credential should include a "certificate" section in the credential source.'
17851785
)
17861786

1787+
@mock.patch(
1788+
"google.auth.transport._mtls_helper._get_workload_cert_and_key_paths",
1789+
return_value=(None, None),
1790+
)
1791+
def test_get_cert_bytes_none_raises_error(
1792+
self, mock_get_workload_cert_and_key_paths
1793+
):
1794+
credentials = self.make_credentials(
1795+
credential_source=self.CREDENTIAL_SOURCE_CERTIFICATE.copy()
1796+
)
1797+
1798+
with pytest.raises(exceptions.ClientCertError) as excinfo:
1799+
credentials._get_cert_bytes()
1800+
1801+
assert excinfo.match(
1802+
"Workload certificate configuration could not be found or does not contain workload certificate paths."
1803+
)
1804+
1805+
@mock.patch.object(
1806+
identity_pool.Credentials,
1807+
"_get_cert_bytes",
1808+
side_effect=exceptions.ClientCertError("mock error"),
1809+
)
1810+
def test_refresh_cert_error_raises_refresh_error(self, mock_get_cert_bytes):
1811+
credentials = self.make_credentials(
1812+
credential_source=self.CREDENTIAL_SOURCE_CERTIFICATE.copy()
1813+
)
1814+
1815+
with pytest.raises(exceptions.RefreshError) as excinfo:
1816+
credentials.refresh(None)
1817+
1818+
assert excinfo.match(
1819+
"Failed to retrieve certificate bytes for external account credentials"
1820+
)
1821+
1822+
@mock.patch.object(
1823+
identity_pool.Credentials,
1824+
"_get_cert_bytes",
1825+
side_effect=OSError("mock os error"),
1826+
)
1827+
def test_refresh_os_error_raises_refresh_error(self, mock_get_cert_bytes):
1828+
credentials = self.make_credentials(
1829+
credential_source=self.CREDENTIAL_SOURCE_CERTIFICATE.copy()
1830+
)
1831+
1832+
with pytest.raises(exceptions.RefreshError) as excinfo:
1833+
credentials.refresh(None)
1834+
1835+
msg = "Failed to retrieve certificate bytes for external"
1836+
assert excinfo.match(msg + " account credentials")
1837+
17871838
@mock.patch("google.auth._agent_identity_utils.parse_certificate")
17881839
@mock.patch(
17891840
"google.auth._agent_identity_utils.should_request_bound_token",

packages/google-auth/tests/transport/test__mtls_helper.py

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,43 @@ def test_no_cert_configs(
499499
with pytest.raises(exceptions.ClientCertError):
500500
_mtls_helper._get_workload_cert_and_key("")
501501

502+
@mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True)
503+
@mock.patch("google.auth.transport._mtls_helper.path.exists", autospec=True)
504+
def test_non_dict_cert_configs_raises_error(
505+
self, mock_path_exists, mock_load_json_file
506+
):
507+
mock_path_exists.return_value = True
508+
509+
for val in [None, [], "not_a_dict"]:
510+
mock_load_json_file.return_value = {"cert_configs": val}
511+
with pytest.raises(exceptions.ClientCertError):
512+
_mtls_helper._get_workload_cert_and_key(None)
513+
514+
@mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True)
515+
@mock.patch("google.auth.transport._mtls_helper.path.exists", autospec=True)
516+
def test_malformed_json_returns_error(self, mock_path_exists, mock_load_json_file):
517+
mock_path_exists.return_value = True
518+
519+
for val in [None, [], "invalid_string"]:
520+
mock_load_json_file.return_value = val
521+
with pytest.raises(exceptions.ClientCertError):
522+
_mtls_helper._get_workload_cert_and_key(None)
523+
524+
@mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True)
525+
@mock.patch("google.auth.transport._mtls_helper.path.exists", autospec=True)
526+
def test_non_dict_workload_raises_error(
527+
self, mock_path_exists, mock_load_json_file
528+
):
529+
mock_path_exists.return_value = True
530+
531+
for invalid_workload in [None, 123, "not_a_dict"]:
532+
mock_load_json_file.return_value = {
533+
"cert_configs": {"workload": invalid_workload}
534+
}
535+
536+
with pytest.raises(exceptions.ClientCertError):
537+
_mtls_helper._get_workload_cert_and_key(None)
538+
502539
@mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True)
503540
@mock.patch(
504541
"google.auth.transport._mtls_helper._get_cert_config_path", autospec=True
@@ -511,6 +548,182 @@ def test_no_workload(self, mock_get_cert_config_path, mock_load_json_file):
511548
assert actual_cert is None
512549
assert actual_key is None
513550

551+
@mock.patch(
552+
"google.auth.transport._mtls_helper._load_json_file", autospec=True
553+
) # noqa: E501
554+
@mock.patch(
555+
"google.auth.transport._mtls_helper._get_cert_config_path",
556+
autospec=True,
557+
) # noqa: E501
558+
@mock.patch(
559+
"google.auth.transport._mtls_helper._read_cert_and_key_files",
560+
autospec=True,
561+
) # noqa: E501
562+
@mock.patch(
563+
"google.auth.transport._mtls_helper.path.exists", autospec=True
564+
) # noqa: E501
565+
def test_no_workload_fallback_to_home(
566+
self,
567+
mock_path_exists,
568+
mock_read_cert_and_key_files,
569+
mock_get_cert_config_path,
570+
mock_load_json_file,
571+
):
572+
ecp_path = "/etc/gcloud/certificate_config.json"
573+
home_path = os.path.join(
574+
_mtls_helper._cloud_sdk.get_config_path(),
575+
"certificate_config.json",
576+
)
577+
mock_get_cert_config_path.return_value = ecp_path
578+
579+
def exists_side_effect(path):
580+
if path == home_path:
581+
return True
582+
return False
583+
584+
mock_path_exists.side_effect = exists_side_effect
585+
586+
def load_json_side_effect(path):
587+
if path == ecp_path:
588+
return {"cert_configs": {"pkcs11": {}}}
589+
elif path == home_path:
590+
return {
591+
"cert_configs": {
592+
"workload": {
593+
"cert_path": "cert/path",
594+
"key_path": "key/path",
595+
}
596+
}
597+
}
598+
return {}
599+
600+
mock_load_json_file.side_effect = load_json_side_effect
601+
mock_read_cert_and_key_files.return_value = (
602+
pytest.public_cert_bytes,
603+
pytest.private_key_bytes,
604+
)
605+
606+
actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key(None)
607+
assert actual_cert == pytest.public_cert_bytes
608+
assert actual_key == pytest.private_key_bytes
609+
610+
mock_get_cert_config_path.assert_called_once_with(None, True)
611+
mock_load_json_file.assert_has_calls(
612+
[mock.call(ecp_path), mock.call(home_path)]
613+
)
614+
mock_read_cert_and_key_files.assert_called_once_with(
615+
"cert/path", "key/path"
616+
) # noqa: E501
617+
618+
@mock.patch(
619+
"google.auth.transport._mtls_helper._load_json_file", autospec=True
620+
) # noqa: E501
621+
@mock.patch(
622+
"google.auth.transport._mtls_helper._get_cert_config_path",
623+
autospec=True,
624+
) # noqa: E501
625+
@mock.patch(
626+
"google.auth.transport._mtls_helper._read_cert_and_key_files",
627+
autospec=True,
628+
) # noqa: E501
629+
@mock.patch(
630+
"google.auth.transport._mtls_helper.path.exists", autospec=True
631+
) # noqa: E501
632+
def test_no_workload_fallback_to_home_error(
633+
self,
634+
mock_path_exists,
635+
mock_read_cert_and_key_files,
636+
mock_get_cert_config_path,
637+
mock_load_json_file,
638+
):
639+
ecp_path = "/etc/gcloud/certificate_config.json"
640+
home_path = os.path.join(
641+
_mtls_helper._cloud_sdk.get_config_path(),
642+
"certificate_config.json",
643+
)
644+
mock_get_cert_config_path.return_value = ecp_path
645+
646+
def exists_side_effect(path):
647+
if path == home_path:
648+
return True
649+
return False
650+
651+
mock_path_exists.side_effect = exists_side_effect
652+
653+
def load_json_side_effect(path):
654+
if path == ecp_path:
655+
return {"cert_configs": {"pkcs11": {}}}
656+
elif path == home_path:
657+
raise exceptions.ClientCertError("mocked unreadable file")
658+
return {}
659+
660+
mock_load_json_file.side_effect = load_json_side_effect
661+
662+
actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key(None)
663+
assert actual_cert is None
664+
assert actual_key is None
665+
666+
mock_get_cert_config_path.assert_called_once_with(None, True)
667+
mock_load_json_file.assert_has_calls(
668+
[mock.call(ecp_path), mock.call(home_path)]
669+
)
670+
mock_read_cert_and_key_files.assert_not_called()
671+
672+
@mock.patch(
673+
"google.auth.transport._mtls_helper._load_json_file", autospec=True
674+
) # noqa: E501
675+
@mock.patch(
676+
"google.auth.transport._mtls_helper._get_cert_config_path",
677+
autospec=True,
678+
)
679+
@mock.patch(
680+
"google.auth.transport._mtls_helper.path.exists", autospec=True
681+
) # noqa: E501
682+
@mock.patch("os.path.normpath", autospec=True)
683+
def test_no_workload_fallback_avoided_same_path_normalization(
684+
self,
685+
mock_normpath,
686+
mock_path_exists,
687+
mock_get_cert_config_path,
688+
mock_load_json_file,
689+
):
690+
ecp_path = "C:/Users/User/.config/gcloud/certificate_config.json"
691+
home_path = "C:\\Users\\User\\.config\\gcloud/certificate_config.json"
692+
mock_get_cert_config_path.return_value = ecp_path
693+
694+
mock_path_exists.return_value = True
695+
696+
# When resolving, the first file has no workload.
697+
mock_load_json_file.return_value = {"cert_configs": {"pkcs11": {}}}
698+
699+
win_path = "C:\\Users\\User\\.config\\gcloud\\certificate_config.json"
700+
701+
# Mock normpath to return the same string for both paths,
702+
# simulating Windows path normalization.
703+
def normpath_side_effect(path):
704+
if path in [ecp_path, home_path]:
705+
return win_path
706+
return path
707+
708+
mock_normpath.side_effect = normpath_side_effect
709+
710+
# Mock get_config_path to construct a path with backslashes
711+
with mock.patch(
712+
"google.auth._cloud_sdk.get_config_path",
713+
return_value="C:\\Users\\User\\.config\\gcloud",
714+
):
715+
actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key(
716+
None
717+
) # noqa: E501
718+
719+
assert actual_cert is None
720+
assert actual_key is None
721+
722+
# Check that it resolved ECP path but never attempted to load
723+
# home_path (because it normalized to the same file).
724+
mock_get_cert_config_path.assert_called_once_with(None, True)
725+
mock_load_json_file.assert_called_once_with(ecp_path)
726+
514727
@mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True)
515728
@mock.patch(
516729
"google.auth.transport._mtls_helper._get_cert_config_path", autospec=True

0 commit comments

Comments
 (0)