fix(auth): parse hostname for mTLS and PSC endpoint certificate rotat… - #18153
fix(auth): parse hostname for mTLS and PSC endpoint certificate rotat…#18153attharva-24 wants to merge 1 commit into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request centralizes and improves mTLS endpoint detection by introducing the is_mtls_endpoint helper function in _mtls_helper.py, replacing previous substring-based checks in both the requests and urllib3 transports. It also adds comprehensive unit tests to verify the new endpoint detection and cert rotation logic. The review feedback highlights a potential TypeError in is_mtls_endpoint when handling bytes URLs, as calling endswith with string suffixes on a bytes hostname outside the try-except block will raise an exception. Decoding bytes inputs to str at the start of the function is recommended to ensure robust error handling.
Sorry, something went wrong.
|
/gemini review |
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request refactors the mTLS endpoint detection logic by introducing a centralized is_mtls_endpoint helper in _mtls_helper.py and updating both requests and urllib3 transports to use it. It also adds comprehensive unit tests to verify the new helper and ensure cert rotation is skipped on non-mTLS URLs. The review feedback suggests improving the robustness of is_mtls_endpoint by handling non-string/non-bytes URL objects (such as urllib3.util.Url) to prevent them from being incorrectly classified as non-mTLS endpoints due to caught TypeErrors.
Sorry, something went wrong.
| if not url: | ||
| return False | ||
| if isinstance(url, bytes): | ||
| try: | ||
| url = url.decode("utf-8") | ||
| except (UnicodeDecodeError, AttributeError): | ||
| return False | ||
| try: | ||
| hostname = urlsplit(url).hostname | ||
| except (ValueError, TypeError, AttributeError): | ||
| return False |
There was a problem hiding this comment.
In urllib3, the url parameter passed to urlopen can be a urllib3.util.Url object (or other string-like/URL objects) rather than a plain str or bytes. Currently, passing a urllib3.util.Url object to is_mtls_endpoint will cause urlsplit(url) to raise a TypeError, which is caught and results in returning False—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 .url attribute (which urllib3.util.Url exposes as a property returning the string representation) or fall back to converting it to a string.
| if not url: | |
| return False | |
| if isinstance(url, bytes): | |
| try: | |
| url = url.decode("utf-8") | |
| except (UnicodeDecodeError, AttributeError): | |
| return False | |
| try: | |
| hostname = urlsplit(url).hostname | |
| except (ValueError, TypeError, AttributeError): | |
| return False | |
| if not url: | |
| return False | |
| if isinstance(url, bytes): | |
| try: | |
| url = url.decode("utf-8") | |
| except (UnicodeDecodeError, AttributeError): | |
| return False | |
| elif not isinstance(url, str): | |
| if hasattr(url, "url"): | |
| url = url.url | |
| else: | |
| try: | |
| url = str(url) | |
| except Exception: | |
| return False | |
| try: | |
| hostname = urlsplit(url).hostname | |
| except (ValueError, TypeError, AttributeError): | |
| return False |
- Adheres to the defensive programming guidelines in Section 2 of the Repository Style Guide by validating and normalizing the input type before performing operations that could raise TypeErrors. (link)
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
Sorry, something went wrong.
There was a problem hiding this comment.
@attharva-24 PTAL
Sorry, something went wrong.
There was a problem hiding this comment.
Addressed in the latest commit!
is_mtls_endpoint now checks for .url on urllib3.util.Url (and other URL objects), supports bytes inputs with safe UTF-8 decoding, and falls back to string conversion. Added unit tests covering urllib3.util.Url, bytes, and str inputs across standard, PSC, and regional mTLS endpoints.
Sorry, something went wrong.
…ion (googleapis#18147) * Isolate hostname using urllib.parse.urlsplit in _mtls_helper.is_mtls_endpoint * Eliminate false positives on non-mTLS URLs containing mtls substrings in paths/queries * Add support for Private Service Connect (*.p.googleapis.com) custom mTLS endpoints * Update AuthorizedSession and AuthorizedHttp to use shared is_mtls_endpoint helper * Add comprehensive unit tests in test__mtls_helper, test_requests, and test_urllib3 Fixes googleapis#18147 Follow-up to googleapis#17928
Reviewers
At least 1 approving review is required to merge this pull request.
Fixes #18147
Follow-up to #17928
Description
This PR resolves two defects in the mTLS endpoint detection logic previously used in requests.py and urllib3.py:
Tests