Refactor of cheroot.ssl.pyopenssl removing SSLConnectionProxyMeta
#787
Conversation
cheroot/ssl/pyopenssl.py
Outdated
| def proxy_wrapper(self, *args): | ||
| new_args = ( | ||
| args[:] | ||
| if method not in SSLConnection.proxy_methods_no_args |
There was a problem hiding this comment.
I wonder why this separation is even necessary just for shutdown. It might've been for some ancient pyopenssl compat.
There was a problem hiding this comment.
Shutdown on Python sockets expects a how parameter e.g. socket.SHUT_WR. OpenSSL does not take a similar parameter so PyOpenSSL doesn't either. It's not clear we need to support this parameter. I do see a couple of places where this question might be considered:
In threadpool.py:
def _force_close(conn):
if conn.rfile.closed:
return
try:
try:
conn.socket.shutdown(socket.SHUT_RD)
except TypeError:
# pyOpenSSL sockets don't take an arg
conn.socket.shutdown()
except OSError:
# shutdown sometimes fails (race with 'closed' check?)
# ref #238
passand server.py
def _close_kernel_socket(self):
"""Terminate the connection at the transport level."""
# Honor ``sock_shutdown`` for PyOpenSSL connections.
shutdown = getattr(
self.socket,
'sock_shutdown',
self.socket.shutdown,
)
try:
shutdown(socket.SHUT_RDWR) # actually send a TCP FIN
except errors.acceptable_sock_shutdown_exceptions:
pass
except socket.error as e:
if e.errno not in errors.acceptable_sock_shutdown_error_codes:
raiseThere was a problem hiding this comment.
Okay.. So in server.py the arg is passed in but in threadpool.py it's not. Got it. I'm not sure how I feel that the locking wrappers at all. I wish the hackery was just dropped.
There was a problem hiding this comment.
i think we can get rid of the locking wrappers. see next comment.
There was a problem hiding this comment.
Yes i think so. In this version, SSLConnection is pretty bare bones but we can add the error handling in #786.
cheroot/ssl/pyopenssl.py
Outdated
| proxy_methods = ( | ||
| 'get_context', | ||
| 'pending', | ||
| 'send', | ||
| 'write', | ||
| 'recv', | ||
| 'read', | ||
| 'renegotiate', | ||
| 'bind', | ||
| 'listen', | ||
| 'connect', | ||
| 'accept', | ||
| 'setblocking', | ||
| 'fileno', | ||
| 'close', | ||
| 'get_cipher_list', | ||
| 'getpeername', | ||
| 'getsockname', | ||
| 'getsockopt', | ||
| 'setsockopt', | ||
| 'makefile', | ||
| 'get_app_data', | ||
| 'set_app_data', | ||
| 'state_string', | ||
| 'sock_shutdown', | ||
| 'get_peer_certificate', | ||
| 'want_read', | ||
| 'want_write', | ||
| 'set_connect_state', | ||
| 'set_accept_state', | ||
| 'connect_ex', | ||
| 'sendall', | ||
| 'settimeout', | ||
| 'gettimeout', | ||
| 'shutdown', | ||
| ) | ||
| proxy_methods_no_args = ('shutdown',) | ||
| proxy_props = ('family',) | ||
|
|
||
| @staticmethod | ||
| def _lock_decorator(method): | ||
| """Create the thread-safe, error-translating proxy method.""" | ||
|
|
||
| def proxy_wrapper(self, *args): | ||
| new_args = ( | ||
| args[:] | ||
| if method not in SSLConnection.proxy_methods_no_args | ||
| else [] | ||
| ) | ||
| with self._lock, _morph_syscall_to_connection_error(method): | ||
| return getattr(self._ssl_conn, method)(*new_args) | ||
|
|
||
| # Doesn't work via super() for some reason. | ||
| # Falling back to type() instead: | ||
| return type(name, bases, nmspc) | ||
| proxy_wrapper.__name__ = method | ||
| return proxy_wrapper | ||
|
|
||
| @staticmethod | ||
| def _make_property(property_): | ||
| """Create the property proxy.""" | ||
|
|
||
| class SSLConnection(metaclass=SSLConnectionProxyMeta): | ||
| r"""A thread-safe wrapper for an ``SSL.Connection``. | ||
| def proxy_prop_wrapper(self): | ||
| return getattr(self._ssl_conn, property_) | ||
|
|
||
| :param tuple args: the arguments to create the wrapped \ | ||
| :py:class:`SSL.Connection(*args) \ | ||
| <pyopenssl:OpenSSL.SSL.Connection>` | ||
| """ | ||
| proxy_prop_wrapper.__name__ = property_ | ||
| return property(proxy_prop_wrapper) |
There was a problem hiding this comment.
I would expect this to be somewhere in the helper, though. So we wouldn't be adding public attributes.
Additionally, I don't fully understand the property patching bit — why is it necessary? Also, do does the connection object even need to be made thread-safe? Why?
There was a problem hiding this comment.
I've revamped the whole thing - getting rid of the meta class, decorator, property patching, and avoiding adding any public attributes. See what you think. Great question about thread-safety. I think you are right - I don't think it's needed as the usual pattern involves one connection per thread. If so we can further simplify the SSLConnection class. Let me know if you think we should remove.
There was a problem hiding this comment.
@julianz- so I did some research and here's my observations:
- mass-searching source code, it seems like only CherryPy/Cheroot (often being vendored into something else) have any references to thread-safety: https://grep.app/search?f.lang=Python®exp=true&q=%28threadsafe|thread-safe|thread+safe%29.*\n.*\n.*SSL\.Connection (CherryPy is what first had this module until the HTTP/WSGI part was split out of it).
- mature projects like urllib3 that also make use of PyOpenSSL don't have any locking: https://github.com/urllib3/urllib3/blob/b20c836/src/urllib3/contrib/pyopenssl.py (although, it might be interesting to look at how they organize their socket wrapper)
- somebody asked about thread-safety upstream and Paul said that it's fine: Is the pyopenssl thread-safe? pyca/pyopenssl#424. But he made a remark about subinterpreters /
mod_wsgiand I know that CherryPy has some code that is probably not being tested in CI related tomod_wsgi. But this was nearly a decade ago. - https://stackoverflow.com/a/61826374/595220 hints that prior to OpenSSL 1.1.0 there would be some problems but not anymore. That version hit EOL 6 years ago: https://endoflife.date/openssl. So we shouldn't need to care about it.
- Is SSL_SESSION supposed to be thread-safe? openssl/openssl#25343 / https://crypto.stackexchange.com/a/41344/70030 / https://docs.openssl.org/1.0.2/man3/threads/#description claim that low-level
SSL_SESSIONisn't thread-safe. But given the PyOpenSSL issue, I'm inclined to believe that it's fine in the Python land. - I did some Git paleontology and the commit that added locking on Sep 12, 2006 cherrypy/cherrypy@f2e3c1c (f2e3c1c); being terrified of
exec(), I rewrote it to a metaclass on Sep 2, 2018 in e9e070b
That commit does not really explain anything but referencespyOpenSSL/tsafe.py. This was pre-PyPI which explains why things were being copied across projects and/or vendored. The first PyOpenSSL release on PyPI was 0.6 on Jun 4, 2008. So it's already two years past the CherryPy commit. We can't say for sure which version this was copied from. The very first import of PyOpenSSL from some other VCS into Git was on Feb 2, 2008 (pyca/pyopenssl@897bc25) but it's tagged as v0.14a1 while is probably an error or some weird historic artifact. However, that commit does contain theexec()snippet with locking that ended up in CherryPy — https://github.com/pyca/pyopenssl/blob/897bc2556fed43b76f6d1b14470c3e806df15af8/tsafe.py. On Jul 28, 2010, that module moved within the repo to https://github.com/pyca/pyopenssl/blob/94cc894795226537daf2f4e79e2d142ff51eacec/OpenSSL/tsafe.py (pyca/pyopenssl@94cc894#diff-eb4dc4606a8f26382d033c9859d6b5be8043e8bc511522f2e014e181bd09c09d). More history of this module: https://github.com/pyca/pyopenssl/commits/e41098f271dc96d6c411172bb11bbe5816f78710/src/OpenSSL/tsafe.py. It was deprecated on Sep 4, 2017 in PyOpenSSL v17.3.0 (Deprecate OpenSSL.tsafe pyca/pyopenssl#655 / Fixes #655 -- deprecate OpenSSL.tsafe pyca/pyopenssl#673) and removed fully on Nov 27, 2020 in v20.0.0 (Remove deprecated tsafe module. pyca/pyopenssl#913). The maintainers thought nobody uses it and weren't sure it was functional. At the time of deletion, it was still using the originalexec()(with an additional method added to the list throughout its life).
So I'd get rid for the hacks and made it as simple as possible, instead of trying to preserve them w/o a good recorded/explainable justification.
There was a problem hiding this comment.
Very thorough! But obviously removing the thread-safety is a potentially breaking change if anyone is relying on it.
julianz-
force-pushed
the
metaclass-refactor
branch
from
d2672c9 to
85503d9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #787 +/- ##
==========================================
- Coverage 79.60% 79.41% -0.20%
==========================================
Files 29 29
Lines 4261 4240 -21
Branches 542 539 -3
==========================================
- Hits 3392 3367 -25
- Misses 726 729 +3
- Partials 143 144 +1 |
julianz-
changed the title
julianz-
force-pushed
the
metaclass-refactor
branch
from
85503d9 to
cdcab0c
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
julianz-
force-pushed
the
metaclass-refactor
branch
5 times, most recently
from
4cda563 to
57ab3fb
Compare
|
@julianz- linking the thread where I posted more thoughts in case it gets lost in the UI: #787 (comment). |
cheroot/ssl/pyopenssl.py
Outdated
|
|
||
| def proxy_prop_wrapper(self): | ||
| return getattr(self._ssl_conn, property_) | ||
| def _handle_syscall_error(self, ssl_syscall_err, method_name) -> NoReturn: |
There was a problem hiding this comment.
Can we plz keep the error conversion in a standalone PR? Also, I still prefer this to be a context manager rather than a callable.
julianz-
force-pushed
the
metaclass-refactor
branch
7 times, most recently
from
0e19fce to
4e5decd
Compare
Removes custom thread-safety locking logic as the wrapper is not required to be thread-safe. The wrapper now uses __getattr__ to intercept and handle proxy attributes.
julianz-
force-pushed
the
metaclass-refactor
branch
from
4e5decd to
2aa8da3
Compare
webknjaz
changed the title
cheroot.ssl.pyopenssl removing SSLConnectionProxyMeta
| Present here for interface compatibility with Python | ||
| socket.shutdown(how). | ||
| """ | ||
| with self._lock: |
|
|
||
| def __init__(self, *args) -> None: ... | ||
| def __getattr__(self, name: str) -> Any: ... | ||
| def shutdown(self, how: Optional[int] = None) -> None: ... |
There was a problem hiding this comment.
typing.Optional is considered confusing because it's actually Union[thing, None].
| def shutdown(self, how: Optional[int] = None) -> None: ... | |
| def shutdown(self, how: int | None = None) -> None: ... |
| def __getattr__(self, name): | ||
| """Proxy attribute access to the underlying ``SSL.Connection``.""" | ||
| return getattr(self._ssl_conn, name) |
There was a problem hiding this comment.
Instead of this, can we just have the class inherit SSL.Connection? I'd like to avoid dynamic things that aren't really lintable staticly (by things like MyPy and similar).
| :py:class:`SSL.Connection(*args) \ | ||
| <pyopenssl:OpenSSL.SSL.Connection>` | ||
| This class exists primarily to ensure the standard Python socket method | ||
| ` shutdown(how=None)` is available for interface compatibility.. |
There was a problem hiding this comment.
Docstrings are expected to be RST, so it's either double backticks or proper roles to reference things. Try this:
| ` shutdown(how=None)` is available for interface compatibility.. | |
| :py:meth:`.shutdown` is available for interface compatibility. |
This is where it's rendered: https://cheroot--787.org.readthedocs.build/en/787/pkg/cheroot.ssl.pyopenssl/#cheroot.ssl.pyopenssl.SSLConnection
| cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS130, WPS201, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS602, WPS608, WPS615 | ||
| cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS325, WPS354, WPS420, WPS422, WPS430, WPS457 | ||
| cheroot/test/helper.py: DAR101, DAR201, DAR401, I001, I003, I004, N802, WPS110, WPS111, WPS121, WPS201, WPS220, WPS231, WPS301, WPS414, WPS421, WPS422, WPS505 | ||
| cheroot/test/ssl/test_ssl_pyopenssl.py: DAR101, DAR201, WPS226 |
There was a problem hiding this comment.
| cheroot/test/ssl/test_ssl_pyopenssl.py: DAR101, DAR201, WPS226 |
| cheroot/server.py: DAR003, DAR101, DAR201, DAR202, DAR301, DAR401, E800, I001, I003, I004, I005, N806, RST201, RST301, RST303, RST304, WPS100, WPS110, WPS111, WPS115, WPS120, WPS121, WPS122, WPS130, WPS132, WPS201, WPS202, WPS204, WPS210, WPS211, WPS212, WPS213, WPS214, WPS220, WPS221, WPS225, WPS226, WPS229, WPS230, WPS231, WPS236, WPS237, WPS238, WPS301, WPS338, WPS342, WPS410, WPS420, WPS421, WPS422, WPS429, WPS432, WPS504, WPS505, WPS601, WPS602, WPS608, WPS617 | ||
| cheroot/ssl/builtin.py: DAR101, DAR201, DAR401, I001, I003, N806, RST304, WPS110, WPS111, WPS115, WPS117, WPS120, WPS121, WPS122, WPS130, WPS201, WPS210, WPS214, WPS229, WPS231, WPS338, WPS422, WPS501, WPS505, WPS529, WPS608, WPS612 | ||
| cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS130, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS608, WPS615 | ||
| cheroot/ssl/pyopenssl.py: C815, DAR101, DAR201, DAR401, I001, I003, I005, N801, N804, RST304, WPS100, WPS110, WPS111, WPS117, WPS120, WPS121, WPS130, WPS201, WPS210, WPS220, WPS221, WPS225, WPS229, WPS231, WPS238, WPS301, WPS335, WPS338, WPS420, WPS422, WPS430, WPS432, WPS501, WPS504, WPS505, WPS601, WPS602, WPS608, WPS615 |
| """Proxy attribute access to the underlying ``SSL.Connection``.""" | ||
| return getattr(self._ssl_conn, name) | ||
|
|
||
| def shutdown(self, how=None): |
There was a problem hiding this comment.
I wonder if we should have something for sock_shutdown().
| how: Ignored. | ||
| PyOpenSSL's shutdown() method does not accept any arguments. | ||
| Present here for interface compatibility with Python | ||
| socket.shutdown(how). |
There was a problem hiding this comment.
Let's try to make this better: https://cheroot--787.org.readthedocs.build/en/787/pkg/cheroot.ssl.pyopenssl/#cheroot.ssl.pyopenssl.SSLConnection.shutdown
| how: Ignored. | |
| PyOpenSSL's shutdown() method does not accept any arguments. | |
| Present here for interface compatibility with Python | |
| socket.shutdown(how). | |
| :param how: Ignored. PyOpenSSL's :py:meth:`~OpenSSL.SSL.Connection.shutdown` \ | |
| method does not accept any arguments.\ | |
| Present here for interface compatibility with Python \ | |
| :py:meth:`socket.shutdown` that :py:class:`ssl.SSLSocket` wrapper \ | |
| exposes. | |
| :type how: object |
| pre | ||
| preconfigure | ||
| py | ||
| pyOpenSSL |
There was a problem hiding this comment.
This is probably redundant once its wrapped in the docstring properly.
| pyOpenSSL |
| @@ -0,0 +1,5 @@ | |||
| Removes thread-safety locking logic from | |||
There was a problem hiding this comment.
Changelog does't do things, it doesn't remove locking. It is supposed to describe what's changed. Use a different writing style to show what the effect is compared to the previous release. It can be the past tense or present simple but not third person.
Maybe this explanation will work for you https://pip-tools.rtfd.io/en/stable/contributing/#rationale
| r"""A thread-safe wrapper for an ``SSL.Connection``. | ||
| class SSLConnection: | ||
| """ | ||
| A compatibility wrapper around pyOpenSSL's SSL.Connection. |
There was a problem hiding this comment.
| A compatibility wrapper around pyOpenSSL's SSL.Connection. | |
| A compatibility wrapper around :py:class:`OpenSSL.SSL.Connection`. |
There was a problem hiding this comment.
I'm actually not sure if this constitutes a breaking change. SemVer talks about breaking changes in the context of API compatibility and the API seems to have remained intact. WDYT?
2 participants
Removes metaclass by using
__getattr__(self, name)to interceptmethod calls.