Repository navigation
chore: Add thread safety to overcome race condition during 401 client certificate rotation in transports - #18168
agrawalradhika-cell wants to merge 32 commits into
Conversation
Introduce a lock for thread-safe reauthentication when mTLS parameters change.
Added a reauthentication lock to prevent concurrent reconfiguration of mTLS channel when client certificate changes.
Added a test to ensure reauthentication lock is acquired on unauthorized response for MTLS sessions.
Added a test to verify reauthentication lock acquisition on unauthorized responses in AuthorizedHttp.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces thread safety during mutual TLS (mTLS) re-authentication in both the requests and urllib3 transport adapters by utilizing a threading lock. This prevents race conditions when checking and reconfiguring the mTLS channel upon receiving an unauthorized response. Unit tests have been added to verify that the lock is correctly acquired. The review feedback points out redundant duplicate imports of the http.client module in both test files, which should be cleaned up.
Updated the request URL in the test case to be more general.
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Add support for MTLS URL prefixes in authentication
Refactor test to use mock response directly and verify lock behavior.
Log message added to indicate skipping mTLS reconfiguration due to unchanged client certificate.
Removed unnecessary blank lines in test_requests.py.
Remove unnecessary blank line in urllib3.py
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Add test for reauthentication when certificate fingerprint matches.
fix: Refactor test_urllib3.py by removing unnecessary lines
fix: Removed several tests related to timeout, certifi, and credential stubs from test_urllib3.py.
Add test for timeout in urllib3 Request
agrawalradhika-cell
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
Refactor MTLS URL prefix handling for clarity.
Refactor mTLS reconfiguration logic to check 'is_mtls' attribute before proceeding with certificate checks and channel configuration.
Add tests for handling unauthorized responses and token refresh in mTLS sessions.
Refactor certificate and key handling in tests, add new tests for mTLS reauthentication and unauthorized response handling.
Refactor test for reauthentication lock during unauthorized response handling.
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Updated tests to handle multiple responses and assert credential refresh behavior.
| _LOGGER.info( | ||
| "Client certificate has changed, reconfiguring mTLS channel." | ||
| ) | ||
| self.configure_mtls_channel(lambda: (call_cert_bytes, call_key_bytes)) |
There was a problem hiding this comment.
Calling configure_mtls_channel() here mounts the new adapter onto the session while other threads are still making normal requests or refreshing credentials outside the reauth lock. Because requests mutates its adapter dictionary in place during mount(), another thread looking up an adapter at the same time can fail with RuntimeError: OrderedDict mutated during iteration.
| "check_parameters_for_unauthorized_response", | ||
| side_effect=check_parameters_for_unauthorized_response, | ||
| ) as mock_check_params: | ||
| with mock.patch.object( |
There was a problem hiding this comment.
Mocking out configure_mtls_channel and manually setting authed_session._cached_cert inside the mock callback means this test only exercises the lock around a dummy callback and verifies the mock's own state update rather than the real channel reconfiguration path.
| "check_parameters_for_unauthorized_response", | ||
| side_effect=check_parameters_for_unauthorized_response, | ||
| ) as mock_check_params: | ||
| with mock.patch.object( |
There was a problem hiding this comment.
Same as test_requests.py:L1590: mocking out configure_mtls_channel and manually updating authed_http._cached_cert in the side effect skips testing the real pool swap and state update under concurrency.
330f565 to
fd911d0
Compare
This branch has not been deployed
4 participants
chore: Add thread safety to overcome race condition during 401 client certificate rotation in transports
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #17756 🦕