Repository navigation
chore: Add thread safety to overcome race condition during 401 client certificate rotation in transports - #18168
Open
agrawalradhika-cell wants to merge 38 commits into
Open
agrawalradhika-cell wants to merge 38 commits into
agrawalradhika-cell wants to merge 38 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}
Contributor
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>
nbayati
reviewed
Aug 28, 2026
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>
nolanleastin
approved these changes
Sep 1, 2026
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.
nbayati
reviewed
Sep 4, 2026
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.
nbayati
requested changes
Oct 16, 2026
agrawalradhika-cell
force-pushed
the
http-cert-thread-lock
branch
2 times, most recently
from
October 16, 2026 22:51
330f565 to
fd911d0
Compare
…test real reconfiguration - Move the 401 cert check and channel reconfiguration into _reconfigure_mtls_channel_if_cert_changed() under _mtls_reauth_lock in both AuthorizedSession and AuthorizedHttp, skipping the cert read when another thread already rotated it. - configure_mtls_channel() no longer calls Session.mount(), which mutated session.adapters in place and could raise 'OrderedDict mutated during iteration' in concurrent get_adapter() calls. A new _replace_adapter() helper swaps in an updated copy for both the session and the auth request session. - Concurrency tests now run the real configure_mtls_channel() and stub only the network layer.
Contributor
Author
|
I've pushed an update (2d911c2) addressing the latest round:
All earlier resolved threads are still addressed: lock naming, TimeoutGuard, a failed cert check still refreshes the token, |
When several threads get a 401 for an expired token while the client certificate is unchanged, _cached_cert never changes, so each queued thread re-loaded the certificate one at a time under _mtls_reauth_lock. Track a _mtls_cert_check_generation counter, snapshot it when each request is sent, and skip the check if another thread already checked after that. Add concurrency tests for the unchanged-certificate case.
…tion - configure_mtls_channel() now takes _mtls_reauth_lock and delegates to _configure_mtls_channel_locked(), which the 401 reconfiguration path calls while already holding the lock. Explicit calls can no longer race the automatic rotation (double-closing the old adapter, losing a new one, or leaving _cached_cert out of sync with the mounted channel). - Write the auth request session's https:// adapter while also holding Request._mtls_lock, which guards the same dict during token refresh. - Log tracebacks for failed cert checks and reconfigurations. - Fix the urllib3 docstring to refer to the pool manager. - Add tests that an explicit configure_mtls_channel() waits for an in-progress 401 reconfiguration; make concurrency test threads daemon threads and assert they finished.
…ient_cert_callback
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 🦕