-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(auth): parse hostname for mTLS and PSC endpoint certificate rotat… #18153
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
b629135
6f85765
a9eb0be
9dbd745
f20b946
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 |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ | |
| from cryptography.hazmat.primitives import hashes, serialization | ||
| from cryptography.hazmat.primitives.asymmetric import ec | ||
| import pytest # type: ignore | ||
| import urllib3.util | ||
|
|
||
| from google.auth import environment_vars, exceptions | ||
| from google.auth.transport import _mtls_helper | ||
|
|
@@ -1888,3 +1889,80 @@ def test_remove_oserror_ignored( | |
| mock_fh.flush.assert_called_once() | ||
| mock_fsync.assert_called_once() | ||
| mock_remove.assert_called_once_with("/path/to/secret") | ||
|
|
||
|
|
||
| class TestIsMtlsEndpoint(object): | ||
| @pytest.mark.parametrize( | ||
| "url", | ||
| [ | ||
|
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. This lacks examples with explicit port numbers (example: https://pubsub.mtls.googleapis.com:443/v1) and queries and fragments (e.g. "https://pubsub.mtls.googleapis.com/v1/projects?pageSize=10#frag")
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. Added unit test cases in |
||
| "https://mtls.googleapis.com", | ||
| "https://mtls.googleapis.com/", | ||
| "https://mtls.googleapis.com/v1/projects", | ||
| "https://mtls.sandbox.googleapis.com", | ||
| "https://mtls.sandbox.googleapis.com/v1/projects", | ||
| "https://pubsub.mtls.googleapis.com", | ||
| "https://pubsub.mtls.googleapis.com/v1/projects/my-project", | ||
| "https://storage.mtls.sandbox.googleapis.com/b/my-bucket", | ||
| "https://my-service.us-east1.rep.mtls.googleapis.com/v1", | ||
| "https://my-service.us-east1.rep.mtls.sandbox.googleapis.com/v1", | ||
| "https://storage.p.googleapis.com/b/my-bucket", | ||
| "https://my-custom-endpoint.p.googleapis.com/v1", | ||
| "https://my-service.us-east1.p.googleapis.com/v1", | ||
| "HTTP://PUBSUB.MTLS.GOOGLEAPIS.COM/V1", | ||
| b"https://pubsub.mtls.googleapis.com", | ||
| b"https://storage.p.googleapis.com/b/my-bucket", | ||
| urllib3.util.parse_url("https://pubsub.mtls.googleapis.com/v1"), | ||
| urllib3.util.parse_url("https://storage.p.googleapis.com/b/my-bucket"), | ||
| "https://pubsub.mtls.googleapis.com.", | ||
| "https://storage.p.googleapis.com./b/my-bucket", | ||
| "https://mtls.googleapis.com.", | ||
| "https://pubsub.mtls.googleapis.com:443/v1", | ||
| "https://pubsub.mtls.googleapis.com:8443/v1", | ||
| "https://storage.p.googleapis.com:443/b/my-bucket", | ||
| "https://pubsub.mtls.googleapis.com/v1/projects?pageSize=10#frag", | ||
| "https://pubsub.mtls.googleapis.com:443/v1/projects?pageSize=10&filter=foo#frag", | ||
| "https://storage.p.googleapis.com:443/b/my-bucket?param=1#section", | ||
| "https://mtls.googleapis.com:443/", | ||
| "https://p.googleapis.com", | ||
| "https://p.googleapis.com/", | ||
| "https://p.googleapis.com:443/v1", | ||
| "https://p.googleapis.com.", | ||
| ], | ||
| ) | ||
| def test_is_mtls_endpoint_true(self, url): | ||
| assert _mtls_helper.is_mtls_endpoint(url) is True | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "url", | ||
| [ | ||
|
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. Consider adding a bare PSC case (example "https://p.googleapis.com").
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. Additionally a case like https://[2001:db8::1]:443/mtls.googleapis.com would be good to demonstrated handling of IPv6 syntax handling
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. Added "p.googleapis.com" to Added IPv6 test cases (https://[2001:db8::1]:443/mtls.googleapis.com and https://[::1]:8443/mtls.googleapis.com) to confirm that bracketed IPv6 host syntax is handled properly. |
||
| "https://storage.googleapis.com", | ||
| "https://storage.googleapis.com.", | ||
| "https://storage.googleapis.com:443/b/my-bucket", | ||
| "https://storage.googleapis.com:443/bucket/mtls.googleapis.com?pageSize=10#frag", | ||
| "https://storage.googleapis.com/bucket/mtls.googleapis.com", | ||
| "https://[2001:db8::1]:443/mtls.googleapis.com", | ||
| "https://[::1]:8443/mtls.googleapis.com", | ||
| "https://logging.googleapis.com/v2/entries?filter=mtls.googleapis.com", | ||
| "https://logging.googleapis.com/v2/entries?filter=mtls.sandbox.googleapis.com", | ||
| "https://logging.googleapis.com/v2/entries?filter=service.p.googleapis.com", | ||
| "https://example.com/mtls.googleapis.com", | ||
| "https://fake-mtls.googleapis.com.attacker.com/v1", | ||
| "https://fake-p.googleapis.com.attacker.com/v1", | ||
| "http://localhost:8080/", | ||
| "http://localhost:8080/mtls.googleapis.com", | ||
| b"https://storage.googleapis.com", | ||
| b"https://storage.googleapis.com/bucket/mtls.googleapis.com", | ||
| b"\xff\xfeinvalid", | ||
| urllib3.util.parse_url("https://storage.googleapis.com/b/my-bucket"), | ||
| urllib3.util.parse_url( | ||
| "https://storage.googleapis.com/bucket/mtls.googleapis.com" | ||
| ), | ||
| "https://.", | ||
| "", | ||
| None, | ||
| 123, | ||
| "not a url", | ||
| ], | ||
| ) | ||
| def test_is_mtls_endpoint_false(self, url): | ||
| assert _mtls_helper.is_mtls_endpoint(url) is False | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -664,6 +664,9 @@ def test_configure_mtls_channel_cert_loading_exceptions( | |
|
|
||
| assert not auth_session.is_mtls | ||
|
|
||
| @mock.patch( | ||
| "google.auth.transport._mtls_helper._get_cert_config_path", return_value=None | ||
| ) | ||
| @mock.patch( | ||
| "google.auth.transport._mtls_helper.get_client_cert_and_key", autospec=True | ||
| ) | ||
|
|
@@ -677,7 +680,7 @@ def test_configure_mtls_channel_cert_loading_exceptions( | |
| }, | ||
| ) | ||
| def test_configure_mtls_channel_without_client_cert_env( | ||
| self, get_client_cert_and_key | ||
| self, get_client_cert_and_key, mock_get_cert_config_path | ||
| ): | ||
| env_to_patch = { | ||
| environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE: "", | ||
|
|
@@ -937,9 +940,19 @@ def test_cert_rotation_logic_skipped_on_other_refresh_status_codes(self): | |
| # Assert mTLS check logic was SKIPPED (Inner Check was False) | ||
| assert not mock_helper.check_parameters_for_unauthorized_response.called | ||
|
|
||
| def test_cert_rotation_skipped_on_non_mtls_url(self): | ||
| @pytest.mark.parametrize( | ||
|
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. I think this is redundant - these transports are no longer responsible for mtls checks themselves and the thing under test here shouldn't be if various forms on non-mtls endpoints are detected correctly (that is already covered in the new mtls_helper tests). Instead, I'd suggest just covering one example of mtls and one example of non mtls here to cover the requests logic specifically. Same for urllib3
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. Looks like just this comment is pending and then I'll take one more look. |
||
| "non_mtls_url", | ||
| [ | ||
| "http://example.com/", | ||
| "https://storage.googleapis.com/bucket/mtls.googleapis.com", | ||
| "https://logging.googleapis.com/v2/entries?filter=mtls.googleapis.com", | ||
| "https://example.com/mtls.sandbox.googleapis.com", | ||
| ], | ||
| ) | ||
| def test_cert_rotation_skipped_on_non_mtls_url(self, non_mtls_url): | ||
| """ | ||
| Tests that mTLS cert rotation is skipped on a non-mTLS URL even if | ||
| Tests that mTLS cert rotation is skipped on non-mTLS URLs (including | ||
| those containing mTLS substrings in paths/query parameters) even if | ||
| mTLS is enabled and an UNAUTHORIZED (401) response is received. | ||
| """ | ||
| credentials = mock.Mock(wraps=CredentialsStub()) | ||
|
|
@@ -953,19 +966,53 @@ def test_cert_rotation_skipped_on_non_mtls_url(self): | |
| authed_session = google.auth.transport.requests.AuthorizedSession( | ||
| credentials, refresh_timeout=60 | ||
| ) | ||
| authed_session.mount(self.TEST_URL, adapter) | ||
| authed_session.mount("https://", adapter) | ||
| authed_session.mount("http://", adapter) | ||
| authed_session._is_mtls = True | ||
| authed_session._cached_cert = b"cached_cert" | ||
|
|
||
| with mock.patch( | ||
| "google.auth.transport.requests._mtls_helper", autospec=True | ||
| ) as mock_helper: | ||
| authed_session.request("GET", self.TEST_URL) | ||
| with mock.patch.object( | ||
| google.auth.transport._mtls_helper, | ||
| "check_parameters_for_unauthorized_response", | ||
| ) as mock_check_params: | ||
| authed_session.request("GET", non_mtls_url) | ||
|
|
||
| # Assert refresh happened | ||
| assert credentials.refresh.called | ||
|
|
||
| # Assert mTLS check logic was SKIPPED | ||
| assert not mock_helper.check_parameters_for_unauthorized_response.called | ||
| assert not mock_check_params.called | ||
|
|
||
| def test_cert_rotation_triggered_on_psc_url(self): | ||
| """ | ||
| Tests that mTLS cert rotation IS triggered on a Private Service Connect | ||
| (PSC) mTLS endpoint when an UNAUTHORIZED (401) response is received. | ||
| """ | ||
| credentials = mock.Mock(wraps=CredentialsStub()) | ||
| adapter = AdapterStub( | ||
| [ | ||
| make_response(status=http_client.UNAUTHORIZED), | ||
| make_response(status=http_client.OK), | ||
| ] | ||
| ) | ||
| psc_url = "https://storage.p.googleapis.com/b/my-bucket" | ||
| authed_session = google.auth.transport.requests.AuthorizedSession( | ||
| credentials, refresh_timeout=60 | ||
| ) | ||
| authed_session.mount(psc_url, adapter) | ||
| authed_session._is_mtls = True | ||
| authed_session._cached_cert = b"cached_cert" | ||
|
|
||
| with mock.patch.object( | ||
| google.auth.transport._mtls_helper, | ||
| "check_parameters_for_unauthorized_response", | ||
| return_value=(b"new_cert", b"new_key", "old_fp", "old_fp"), | ||
| ) as mock_check_params: | ||
| authed_session.request("GET", psc_url) | ||
|
|
||
| # Assert mTLS check logic was called on PSC endpoint | ||
| mock_check_params.assert_called_once() | ||
| assert credentials.refresh.called | ||
|
|
||
| def test_configure_mtls_channel_subsequent_failure(self): | ||
| # 1. Setup successful mTLS configuration | ||
|
|
||
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.
In
urllib3, theurlparameter passed tourlopencan be aurllib3.util.Urlobject (or other string-like/URL objects) rather than a plainstrorbytes. Currently, passing aurllib3.util.Urlobject tois_mtls_endpointwill causeurlsplit(url)to raise aTypeError, which is caught and results in returningFalse—even if the object represents a valid mTLS endpoint.To prevent this and ensure robust compatibility with
urllib3's native URL objects, we should check if the input has a.urlattribute (whichurllib3.util.Urlexposes as a property returning the string representation) or fall back to converting it to a string.References
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.
@attharva-24 PTAL
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.
Addressed in the latest commit!
is_mtls_endpointnow checks for.urlonurllib3.util.Url(and other URL objects), supportsbytesinputs with safe UTF-8 decoding, and falls back to string conversion. Added unit tests coveringurllib3.util.Url,bytes, andstrinputs across standard, PSC, and regional mTLS endpoints.