-
-
Notifications
You must be signed in to change notification settings - Fork 393
Bugfix: Send the TLS alert to the peer upon a ssl.CertificateError.
#3413
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
fd8eebb
34bd8d8
284ded4
1356ae5
86b33f3
dc74283
02aee02
71fe831
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 |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Upon a ``SSL.CertificateError``, a TLS alert is now sent to the peer before | ||
| raising ``trio.BrokenResourceError`` from the original error, preventing the | ||
| client from hanging. | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -394,6 +394,8 @@ def __init__( | |||||||||||||||||
|
|
||||||||||||||||||
| self._estimated_receive_size = STARTING_RECEIVE_SIZE | ||||||||||||||||||
|
|
||||||||||||||||||
| self._ssl_error: _stdlib_ssl.CertificateError | None = None | ||||||||||||||||||
|
|
||||||||||||||||||
| _forwarded: ClassVar = { | ||||||||||||||||||
| "context", | ||||||||||||||||||
| "server_side", | ||||||||||||||||||
|
|
@@ -499,7 +501,15 @@ async def _retry( | |||||||||||||||||
| ret = fn(*args) | ||||||||||||||||||
| except _stdlib_ssl.SSLWantReadError: | ||||||||||||||||||
| want_read = True | ||||||||||||||||||
| except (_stdlib_ssl.SSLError, _stdlib_ssl.CertificateError) as exc: | ||||||||||||||||||
| except _stdlib_ssl.CertificateError as exc: | ||||||||||||||||||
|
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. So my understanding is that we need to do this in |
||||||||||||||||||
| # We assume to have pending data in MemoryBIO: the TLS alert | ||||||||||||||||||
| # corresponding to the exception. As the alert needs to be | ||||||||||||||||||
| # sent to the peer, we stash the exception for now. We'll | ||||||||||||||||||
| # raise it after the alert is sent below, which will surely | ||||||||||||||||||
| # happen as sending has priority over receiving. | ||||||||||||||||||
| assert self._outgoing.pending | ||||||||||||||||||
| self._ssl_error = exc | ||||||||||||||||||
| except _stdlib_ssl.SSLError as exc: | ||||||||||||||||||
| self._state = _State.BROKEN | ||||||||||||||||||
| raise trio.BrokenResourceError from exc | ||||||||||||||||||
| else: | ||||||||||||||||||
|
|
@@ -633,6 +643,13 @@ async def _retry( | |||||||||||||||||
| ) | ||||||||||||||||||
| self._incoming.write(data) | ||||||||||||||||||
| self._inner_recv_count += 1 | ||||||||||||||||||
| if self._ssl_error: | ||||||||||||||||||
|
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'm not convinced we need to store the error in |
||||||||||||||||||
| # Raise the stashed SSLError exception. | ||||||||||||||||||
| self._state = _State.BROKEN | ||||||||||||||||||
|
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. One concern I just had is that maybe it would be better to move this earlier (i.e. within the |
||||||||||||||||||
| # Not sure if this is necessary, but let's clean up | ||||||||||||||||||
| # self._ssl_error just in case. | ||||||||||||||||||
| self._ssl_error, ssl_error = None, self._ssl_error | ||||||||||||||||||
| raise trio.BrokenResourceError from ssl_error | ||||||||||||||||||
|
Comment on lines
+649
to
+652
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 doesn't work; the frame still has a reference to Anyways, the right way is probably:
Suggested change
|
||||||||||||||||||
| if not yielded: | ||||||||||||||||||
| await trio.lowlevel.cancel_shielded_checkpoint() | ||||||||||||||||||
| return ret | ||||||||||||||||||
|
|
||||||||||||||||||
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.
Probably both of these could use single backticks to link to the respective documentation.