-
-
Notifications
You must be signed in to change notification settings - Fork 99
Re-raise PyOpenSSL SysCallErrors as respective errno-mapped standard Python ConnectionError subclasses in close()/shutdown()
#786
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
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
julianz- marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -50,6 +50,8 @@ | |||||
| pyopenssl | ||||||
| """ | ||||||
|
|
||||||
| import errno | ||||||
| import os | ||||||
| import socket | ||||||
| import sys | ||||||
| import threading | ||||||
|
|
@@ -77,6 +79,45 @@ | |||||
| from . import Adapter | ||||||
|
|
||||||
|
|
||||||
| @contextlib.contextmanager | ||||||
| def _morph_syscall_to_connection_error(method_name, /): | ||||||
| """ | ||||||
| Handle :exc:`OpenSSL.SSL.SysCallError` in a wrapped method. | ||||||
|
|
||||||
| This context manager catches and re-raises SSL system call errors | ||||||
| with appropriate exception types. | ||||||
|
|
||||||
| Yields: | ||||||
| None: Execution continues within the context block. | ||||||
|
Comment on lines
+90
to
+91
Member
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. we should be using sphinx-native param lists, not napoleon. |
||||||
| """ # noqa: DAR301 | ||||||
|
Member
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 should be possible to drop
Suggested change
|
||||||
| try: | ||||||
| yield | ||||||
| except SSL.SysCallError as ssl_syscall_err: | ||||||
| connection_error_map = { | ||||||
| errno.EBADF: ConnectionError, # socket is gone? | ||||||
| errno.ECONNABORTED: ConnectionAbortedError, | ||||||
| errno.ECONNREFUSED: ConnectionRefusedError, | ||||||
| errno.ECONNRESET: ConnectionResetError, | ||||||
| errno.ENOTCONN: ConnectionError, | ||||||
| errno.EPIPE: BrokenPipeError, | ||||||
| errno.ESHUTDOWN: BrokenPipeError, | ||||||
| } | ||||||
| error_code = ssl_syscall_err.args[0] if ssl_syscall_err.args else None | ||||||
| error_msg = ( | ||||||
| os.strerror(error_code) | ||||||
| if error_code is not None | ||||||
| else repr(ssl_syscall_err) | ||||||
| ) | ||||||
| conn_err_cls = connection_error_map.get( | ||||||
| error_code, | ||||||
| ConnectionError, | ||||||
| ) | ||||||
| raise conn_err_cls( | ||||||
| error_code, | ||||||
| f'Error in calling {method_name!s} on PyOpenSSL connection: {error_msg!s}', | ||||||
| ) from ssl_syscall_err | ||||||
|
|
||||||
|
|
||||||
| class SSLFileobjectMixin: | ||||||
| """Base mixin for a TLS socket stream.""" | ||||||
|
|
||||||
|
|
@@ -224,14 +265,12 @@ def lock_decorator(method): | |||||
| """Create a proxy method for a new class.""" | ||||||
|
|
||||||
| def proxy_wrapper(self, *args): | ||||||
| self._lock.acquire() | ||||||
| try: | ||||||
| new_args = ( | ||||||
| args[:] if method not in proxy_methods_no_args else [] | ||||||
| ) | ||||||
| new_args = ( | ||||||
| args[:] if method not in proxy_methods_no_args else [] | ||||||
| ) | ||||||
| # translate any SysCallError to ConnectionError | ||||||
| with _morph_syscall_to_connection_error(method), self._lock: | ||||||
| return getattr(self._ssl_conn, method)(*new_args) | ||||||
| finally: | ||||||
| self._lock.release() | ||||||
|
|
||||||
| return proxy_wrapper | ||||||
|
|
||||||
|
|
||||||
|
Member
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. Let's move this to
Member
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. No need to have the second |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,106 @@ | ||
| """Tests for :py:mod:`cheroot.ssl.pyopenssl` :py:class:`cheroot.ssl.pyopenssl.SSLConnection` wrapper.""" | ||
|
|
||
| import errno | ||
|
|
||
| import pytest | ||
|
|
||
| from OpenSSL import SSL | ||
|
|
||
| from cheroot.ssl.pyopenssl import SSLConnection | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_ssl_context(mocker): | ||
| """Fixture providing a mock instance of :py:class:`OpenSSL.SSL.Context`.""" | ||
| mock_context = mocker.Mock(spec=SSL.Context) | ||
|
|
||
| # Add a mock _context attribute to simulate SSL.Context behavior | ||
| mock_context._context = mocker.Mock() | ||
| return mock_context | ||
|
|
||
|
|
||
| @pytest.mark.filterwarnings('ignore:Non-callable called') | ||
| @pytest.mark.parametrize( | ||
| ( | ||
| 'tested_method_name', | ||
| 'simulated_errno', | ||
| 'expected_exception', | ||
| ), | ||
| ( | ||
| pytest.param('close', errno.EBADF, ConnectionError, id='close-EBADF'), | ||
| pytest.param( | ||
| 'close', | ||
| errno.ECONNABORTED, | ||
| ConnectionAbortedError, | ||
| id='close-ECONNABORTED', | ||
| ), | ||
| pytest.param( | ||
| 'send', | ||
| errno.EPIPE, | ||
| BrokenPipeError, | ||
| id='send-EPIPE', | ||
| ), # Expanded coverage | ||
| pytest.param( | ||
| 'shutdown', | ||
| errno.EPIPE, | ||
| BrokenPipeError, | ||
| id='shutdown-EPIPE', | ||
| ), | ||
| pytest.param( | ||
| 'shutdown', | ||
| errno.ECONNRESET, | ||
| ConnectionResetError, | ||
| id='shutdown-ECONNRESET', | ||
| ), | ||
| pytest.param( | ||
| 'close', | ||
| errno.ENOTCONN, | ||
| ConnectionError, | ||
| id='close-ENOTCONN', | ||
| ), | ||
| pytest.param('close', errno.EPIPE, BrokenPipeError, id='close-EPIPE'), | ||
| pytest.param( | ||
| 'close', | ||
| errno.ESHUTDOWN, | ||
| BrokenPipeError, | ||
| id='close-ESHUTDOWN', | ||
| ), | ||
| ), | ||
| ) | ||
| def test_close_morphs_syscall_error_correctly( | ||
| mocker, | ||
| mock_ssl_context, | ||
| tested_method_name, | ||
| simulated_errno, | ||
| expected_exception, | ||
| ): | ||
| """Check ``SSLConnection.close()`` morphs ``SysCallError`` to ``ConnectionError``.""" | ||
| # Prevents the real OpenSSL.SSL.Connection.__init__ from running | ||
| mocker.patch('OpenSSL.SSL.Connection') | ||
|
|
||
| # Create SSLConnection object. The patched SSL.Connection() call returns | ||
| # a mock that is stored internally as conn._ssl_conn. | ||
| conn = SSLConnection(mock_ssl_context) | ||
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Show fixed
Hide fixed
Check failureCode scanning / CodeQL Non-callable called Error test
Call to a
non-callable Error loading related location Loading class SSLConnectionProxyMeta Error loading related location Loading |
||
|
|
||
| # Define specific OpenSSL error based on the parameter | ||
| simulated_error = SSL.SysCallError( | ||
| simulated_errno, | ||
| 'Simulated connection error', | ||
| ) | ||
|
|
||
| # Dynamically retrieve the method on the underlying mock | ||
| underlying_method = getattr(conn._ssl_conn, tested_method_name) | ||
|
|
||
| # Patch the method to raise the simulated error | ||
| underlying_method.side_effect = simulated_error | ||
|
|
||
| expected_match = ( | ||
| f'.*Error in calling {tested_method_name} on PyOpenSSL connection.*' | ||
| ) | ||
|
|
||
| # Assert the expected exception is raised based on the parameter | ||
| with pytest.raises(expected_exception, match=expected_match) as excinfo: | ||
| getattr(conn, tested_method_name)() | ||
|
|
||
| # Assert the original SysCallError is included in the new exception's cause | ||
| assert excinfo.value.__cause__ is simulated_error | ||
webknjaz marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| OpenSSL system call errors are now intercepted and re-raised as Python exceptions derived from :exc:`ConnectionError`. | ||
|
|
||
| -- by :user:`julianz-` |
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.