Commit 3ed2d68
fix: gracefully fallback if workload fields are missing from cert config (#16022)
Prevents exceptions during gECC flows or when falling back to
SecureConnect by returning None instead of raising ClientCertError when
X.509 workload fields are absent.
---------
Co-authored-by: Daniel Sanche <d.sanche14@gmail.com>1 parent 99fa08e commit 3ed2d68
3 files changed
Lines changed: 36 additions & 21 deletions
File tree
- packages/google-auth
- google/auth/transport
- tests/transport
- aio
Lines changed: 8 additions & 15 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -191,28 +191,21 @@ def _get_workload_cert_and_key_paths(config_path, include_context_aware=True): | |||
| 191 | 191 | ) | |
| 192 | 192 | cert_configs = data["cert_configs"] | |
| 193 | 193 | ||
| 194 | + # We return None, None if the expected workload fields are not present. | ||
| 195 | + # The certificate config might be present for other types of connections (e.g. gECC), | ||
| 196 | + # and we want to gracefully fallback to testing other mTLS configurations | ||
| 197 | + # like SecureConnect instead of throwing an exception. | ||
| 198 | + | ||
| 194 | 199 | if "workload" not in cert_configs: | |
| 195 | - raise exceptions.ClientCertError( | ||
| 196 | - 'Certificate config file {} is in an invalid format, a "workload" cert config is expected'.format( | ||
| 197 | - absolute_path | ||
| 198 | - ) | ||
| 199 | - ) | ||
| 200 | + return None, None | ||
| 200 | 201 | workload = cert_configs["workload"] | |
| 201 | 202 | ||
| 202 | 203 | if "cert_path" not in workload: | |
| 203 | - raise exceptions.ClientCertError( | ||
| 204 | - 'Certificate config file {} is in an invalid format, a "cert_path" is expected in the workload cert config'.format( | ||
| 205 | - absolute_path | ||
| 206 | - ) | ||
| 207 | - ) | ||
| 204 | + return None, None | ||
| 208 | 205 | cert_path = workload["cert_path"] | |
| 209 | 206 | ||
| 210 | 207 | if "key_path" not in workload: | |
| 211 | - raise exceptions.ClientCertError( | ||
| 212 | - 'Certificate config file {} is in an invalid format, a "key_path" is expected in the workload cert config'.format( | ||
| 213 | - absolute_path | ||
| 214 | - ) | ||
| 215 | - ) | ||
| 208 | + return None, None | ||
| 216 | 209 | key_path = workload["key_path"] | |
| 217 | 210 | ||
| 218 | 211 | # == BEGIN Temporary Cloud Run PATCH == | |
Lines changed: 19 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -98,6 +98,25 @@ async def test_configure_mtls_channel_invalid_format(self): | |||
| 98 | 98 | with pytest.raises(exceptions.MutualTLSChannelError): | |
| 99 | 99 | await session.configure_mtls_channel() | |
| 100 | 100 | ||
| 101 | + @pytest.mark.asyncio | ||
| 102 | + async def test_configure_mtls_channel_invalud_fields(self): | ||
| 103 | + """ | ||
| 104 | + If cert is missing expected keys, it should fail gracefully | ||
| 105 | + """ | ||
| 106 | + with mock.patch.dict( | ||
| 107 | + os.environ, {"GOOGLE_API_USE_CLIENT_CERTIFICATE": "true"} | ||
| 108 | + ), mock.patch("os.path.exists") as mock_exists, mock.patch( | ||
| 109 | + "builtins.open", mock.mock_open(read_data='{"cert_configs": {}}') | ||
| 110 | + ): | ||
| 111 | + mock_exists.return_value = True | ||
| 112 | + mock_creds = mock.AsyncMock(spec=credentials.Credentials) | ||
| 113 | + session = sessions.AsyncAuthorizedSession(mock_creds) | ||
| 114 | + | ||
| 115 | + await session.configure_mtls_channel() | ||
| 116 | + | ||
| 117 | + # If the file couldn't be parsed, it shouldn't error; it just won't use mTLS | ||
| 118 | + assert session._is_mtls is False | ||
| 119 | + | ||
| 101 | 120 | @pytest.mark.asyncio | |
| 102 | 121 | async def test_configure_mtls_channel_mock_callback(self): | |
| 103 | 122 | """ | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -577,8 +577,9 @@ def test_no_workload(self, mock_get_cert_config_path, mock_load_json_file): | |||
| 577 | 577 | mock_get_cert_config_path.return_value = "/path/to/cert" | |
| 578 | 578 | mock_load_json_file.return_value = {"cert_configs": {}} | |
| 579 | 579 | ||
| 580 | - with pytest.raises(exceptions.ClientCertError): | ||
| 581 | - _mtls_helper._get_workload_cert_and_key("") | ||
| 580 | + actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key("") | ||
| 581 | + assert actual_cert is None | ||
| 582 | + assert actual_key is None | ||
| 582 | 583 | ||
| 583 | 584 | @mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True) | |
| 584 | 585 | @mock.patch( | |
@@ -590,8 +591,9 @@ def test_no_cert_file(self, mock_get_cert_config_path, mock_load_json_file): | |||
| 590 | 591 | "cert_configs": {"workload": {"key_path": "path/to/key"}} | |
| 591 | 592 | } | |
| 592 | 593 | ||
| 593 | - with pytest.raises(exceptions.ClientCertError): | ||
| 594 | - _mtls_helper._get_workload_cert_and_key("") | ||
| 594 | + actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key("") | ||
| 595 | + assert actual_cert is None | ||
| 596 | + assert actual_key is None | ||
| 595 | 597 | ||
| 596 | 598 | @mock.patch("google.auth.transport._mtls_helper._load_json_file", autospec=True) | |
| 597 | 599 | @mock.patch( | |
@@ -603,8 +605,9 @@ def test_no_key_file(self, mock_get_cert_config_path, mock_load_json_file): | |||
| 603 | 605 | "cert_configs": {"workload": {"cert_path": "path/to/key"}} | |
| 604 | 606 | } | |
| 605 | 607 | ||
| 606 | - with pytest.raises(exceptions.ClientCertError): | ||
| 607 | - _mtls_helper._get_workload_cert_and_key("") | ||
| 608 | + actual_cert, actual_key = _mtls_helper._get_workload_cert_and_key("") | ||
| 609 | + assert actual_cert is None | ||
| 610 | + assert actual_key is None | ||
| 608 | 611 | ||
| 609 | 612 | ||
| 610 | 613 | class TestReadCertAndKeyFile(object): | |
0 commit comments