Handle `SSLZeroReturnError` exceptions in stdlib TLS adapter #518

toppk · 2022-09-22T02:44:33Z

with python 3.8 and above ssl will generate SSLZeroReturnError exceptions for zero-bytes-send connections. These connections are ignored in cheroot when the exception is SSLError with errno=ssl.SSL_ERROR_EOF
this is the cpython commit that introduced the change of behavior.

python/cpython#18772

❓ What kind of change does this PR introduce?

📋 What is the related issue number (starting with #)
Resolves #517

❓ What is the current behavior? (You can also link to an open issue here)

Annoying stack trace on startup, (see issue for full details), but cherrypy still operates normally

❓ What is the new behavior (if this is a feature change)?

No exception is printed, no other change in behavior.

📋 Other information:

📋 Contribution checklist:

(If you're a first-timer, check out
this guide on making great pull requests)

This change is

toppk · 2022-09-22T03:01:48Z

not sure why it doesn't like my commit message.

webknjaz · 2022-09-24T14:22:05Z

not sure why it doesn't like my commit message.

Don't worry, that platisd/bad-commit-message-blocker check is broken, I haven't gotten to fixing it.

But the one in pre-commit (see https://results.pre-commit.ci/run/github/16620627/1663816002.9KC-J1hTS36wzU6ARo-C4w) where there's two flake8 violations introduced is a legit failure. The complexity has been increased past the acceptable limit and so it needs to be simplified for the linting to go green.

webknjaz · 2022-09-24T14:30:36Z

cheroot/ssl/builtin.py

+ # This is almost certainly due to the cherrypy engine
+ # 'pinging' the socket to assert it's connectable;
+ # the 'ping' isn't SSL.


You said that you've disabled the Checker and it's still happening. Pretty sure it's unrelated, then.
It must be this https://github.com/cherrypy/cheroot/blob/3f9b1cb/cheroot/ssl/builtin.py#L55-L83.
By the way, maybe the suppress() in that helper function needs to be extended too.

It isn't the checker, but cherrypy has a call to a module called portend which is used to move the lifecycle of the service to the active state. This is a simple responds to TCP port check. So it is a check but not the checker...

Oh, that's good to know. That would be useful to mention.

webknjaz · 2022-09-24T14:34:53Z

cheroot/ssl/builtin.py

+ # This is almost certainly due to the cherrypy engine
+ # 'pinging' the socket to assert it's connectable;
+ # the 'ping' isn't SSL.
+ return EMPTY_RESULT
 except ssl.SSLError as ex:
 if ex.errno == ssl.SSL_ERROR_EOF:


I wonder if we could declare an internal CherootSSLZeroReturnError in cheroot._compat, and then wrap the TLS calls to re-raise both ssl.SSLZeroReturnError and ssl.SSLError with ssl.SSL_ERROR_EOF as this internal exception. This would allow having a cleaner structural block for handling two incarnations of the same situation as one thing without having to care about two copy-pasted blocks are handled the same way.

Oh, I see, we already have errors.NoSSLError so maybe not _compat but errors would be a better place for the exception.

And maybe all these branchy cases could be treated the same with a shorter and better readable error handling on this layer of abstraction.

in doing a full review of how this exception is handled, and comparing to the pyopenssl implementation, I would say my opinion is that we should change this entire exception code to just swallow the exception and return EMPTY_RESULT, except for the http-on-https port condition.

Something like this.

try: s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, ) return s, self.get_environ(s) except ssl.SSLError as ex: if ex.errno == ssl.SSL_ERROR_SSL: if _assert_ssl_exc_contains(ex, "http request"): # The client is speaking HTTP to an HTTPS server. raise errors.NoSSLError except (generic_socket_error, ssl.SSLZeroReturnError): pass return EMPTY_RESULT

If we ever wanted to log these bad sockets, we can add to this, but right now we dissect a bunch of exceptions all for it to be ignored anyway (sometimes the stack trace is printed, but other than that it is ignored).

I would also get rid of the except Exception: in server.serve() as there shouldn't be any unknown/unexpected exceptions bubbling up there, the only unknown/unexpected exceptions should be coming from user code, and the workers are in different threads.

My understanding is that the reason this is more complicated than pyopenssl is that pyopenssl does some processing internally and turns weird cases into specific exceptions. I'd love the same to happen with the ssl implementation. But it'd have to happen on our side since stdlib doesn't do this. And even if it did, we'd need to have something for compatibility for years.

I would also get rid of the except Exception: in server.serve() as there shouldn't be any unknown/unexpected exceptions bubbling up there, the only unknown/unexpected exceptions should be coming from user code, and the workers are in different threads.

Agreed. Although, this deserves a separate discussion/PR.

webknjaz · 2022-09-24T14:41:13Z

Hey @mxii-ca, you've provided a lot of invaluable input on TLS in the past. Would you mind checking this patch? Am I right that it's related to https://github.com/cherrypy/cheroot/blob/3f9b1cb/cheroot/ssl/builtin.py#L55-L83?

toppk · 2022-09-24T21:17:01Z

I've added a second commit that attempts simplification of the code. The big difference is that the earlier code would reraise some exceptions that wrap_socket raised, and this will just ignore all of them except for that only exception that cheroot handles (http-over-https aka errors.NoSSLError).

If this approach is satisfactory I can squash the commits for a cleaner commit log. I will also remove _compat.IS_ABOVE_OPENSSL10 and related code.

lgtm-com · 2022-09-24T21:19:34Z

This pull request introduces 2 alerts when merging 6c98cef into 3915f35 - view on LGTM.com

new alerts:

1 for Unreachable 'except' block
1 for Unused import

toppk · 2022-09-24T22:24:05Z

This pull request introduces 2 alerts when merging 6c98cef into 3915f35 - view on LGTM.com

new alerts:

thank you bot!

1 for Unreachable 'except' block

yes, I didn't notice that, this code can get removed as well.

1 for Unused import

if this approach is accepted, I will remove this code

cheroot/ssl/builtin.py

webknjaz · 2022-09-24T23:31:46Z

I've added a second commit that attempts simplification of the code. The big difference is that the earlier code would reraise some exceptions that wrap_socket raised, and this will just ignore all of them except for that only exception that cheroot handles (http-over-https aka errors.NoSSLError).

Well, it may cause hard-to-debug states if the exceptions are just ignored silently. The currently listed errors were carefully curated, and I'm not sure if it's reasonable to throw it all away without a mechanism for inspection. It'd be good to at least have some logging there, I'm not sure.

I see your point that most of the exceptions would need to be ignored anyway, but I foresee difficulties for the end-users attempting to figure out what they've set up wrong in the TLS configuration, and it tends to be nontrivial when it comes to understanding crypto stuff like ciphers. How would one see that a client is sending requests with non-matching ciphers enabled, for example?

By the way, I'm not sure if you misunderstood my initial idea — I was hoping that the call to wrap_socket() could be somehow decorated with an exception transformer.

lgtm-com · 2022-09-25T00:21:39Z

This pull request introduces 1 alert when merging 2a9e795 into 03cc9a8 - view on LGTM.com

new alerts:

1 for Unused import

toppk · 2022-09-25T01:09:38Z

Well, it may cause hard-to-debug states if the exceptions are just ignored silently. The currently listed errors were carefully curated, and I'm not sure if it's reasonable to throw it all away without a mechanism for inspection. It'd be good to at least have some logging there, I'm not sure.

I can see that the curation was a hard fought for list, but when I looked at pyopenssl backend I chuckled a bit. As for logging, it is a no win situation, as either it will annoy some people or not annoy others. For an internet facing service, logs will get litter with the results of random port scans, for some development/production scenarios it may be desirable, but that might also include things that are on the curated list.

I see your point that most of the exceptions would need to be ignored anyway, but I foresee difficulties for the end-users attempting to figure out what they've set up wrong in the TLS configuration, and it tends to be nontrivial when it comes to understanding crypto stuff like ciphers. How would one see that a client is sending requests with non-matching ciphers enabled, for example?

of course, tls communicates back to the client failure, so silently ignoring on the server side is not removing all information. The two issues I have with current approach,

that if we are deciding to log an exception, the stack trace isn't the best format
if the user turns on some sort of error logging, we will be generating one on every startup due to the portend port check.

Another thing while the current blacklist approach of generating a stack trace on all unknown exceptions is very nice to send users your way (as it did me), it has the obvious downside where we are chasing new exceptions categories, and it is not obvious what do. An actual whitelist of exceptions we want to log, and a facility for users to turn on/off said logging may be useful.

By the way, I'm not sure if you misunderstood my initial idea — I was hoping that the call to wrap_socket() could be somehow decorated with an exception transformer.

Yeah, I was thinking about extending context, and then wrap socket can produce better errors, or a solution like the with supress() that exists elsewhere, but I think those would just obfuscate the execution from the code. The wrap() function is the clearest way to express how we want to handle the wrap exceptions, and doesn't do much else.

It would be cool if except got some of the PEP 634/Structural Pattern Matching features, but maybe this use case is just going to look like a bunch of duct tape no matter what (bad exceptions being raised, supporting multiple python runtimes). I may send something to python-ideas anyway.

lgtm-com · 2022-09-25T01:18:45Z

This pull request introduces 1 alert when merging 5ede676 into 6ce6e1e - view on LGTM.com

new alerts:

1 for Unused import

webknjaz · 2022-09-28T01:44:52Z

As for logging, it is a no win situation, as either it will annoy some people or not annoy others. For an internet facing service, logs will get litter with the results of random port scans

Yeah. Although, I wouldn't bundle any behavior changes in this PR anyway — I think it should just contain a bugfix with minimum changes. And any refactoring needs to go elsewhere.
I was thinking that it'd be nice to have prod/dev/debug modes that would maybe control this but that's a feature. Also, I've been meaning to look into making use of logging consistently so that the end-users could suppress what they don't need using standard mechanisms. Plus, with such a framework in place, we could log TLS errors nicely, without tracebacks (unless in dev/debug mode), similar to what other web servers typically do.
With that in mind, implementing the use of logging across the project would be a blocker to having env-dependent toggles.

Yeah, I was thinking about extending context, and then wrap socket can produce better errors, or a solution like the with supress() that exists elsewhere, but I think those would just obfuscate the execution from the code. The wrap() function is the clearest way to express how we want to handle the wrap exceptions, and doesn't do much else.

How about a method? self.context is already an instance attribute. So it wouldn't need to be accessed directly in wrap().

def _wrap_context_socket_with_consistent_errors(self, sock):
    try:
        return self.context.wrap_socket(
            sock, do_handshake_on_connect=True, server_side=True,
        )
    except ...:
        raise CherootTLSEOL
    except ...:
        raise CherootTLSEOL

def wrap(self, sock)
    try:
        return self._wrap_context_socket_with_consistent_errors(sock)
    except CherootTLSEOL:
        ...
    except ...:
        ...

It would be cool if except got some of the PEP 634/Structural Pattern Matching features, but maybe this use case is just going to look like a bunch of duct tape no matter what (bad exceptions being raised, supporting multiple python runtimes). I may send something to python-ideas anyway.

That would be a no-go for us for another 3-4 years because of compat requirements. Libs don't have the same luxury that apps do.

lgtm-com · 2022-09-28T02:30:10Z

This pull request introduces 1 alert when merging 24e692a into e0dd9cb - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-09-29T17:13:07Z

This pull request introduces 1 alert when merging 7a4712a into 85b8dc9 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2022-09-29T18:49:00Z

This pull request introduces 1 alert when merging 4c5aba6 into ec51fbb - view on LGTM.com

new alerts:

1 for Unused import

toppk · 2022-09-29T20:02:53Z

i'll take a look at your last recommendation and update this pr. not sure it will pass the complexity review, but we'll see.

thezoggy

Part of the code removed is the usage of IS_ABOVE_OPENSSL10 which is now an unused import.
from .._compat import IS_ABOVE_OPENSSL10, suppress
--------------------------^^^^^^^^^^^^^
So that should also be cleaned up to make CI happy.

webknjaz · 2022-10-05T14:44:15Z

Ref: #346

The-Compiler · 2022-11-08T15:01:24Z

FWIW, I can confirm this fixes #517 for me.

satmandu · 2023-01-04T15:09:45Z

Could we please get this merged? @toppk Are you still planning on pushing this PR to completion?

toppk · 2023-01-04T18:50:57Z

Could we please get this merged? @toppk Are you still planning on pushing this PR to completion?

I am willing to, but I'm having a hard time incorporating the feedback into a new design. I will revisit this week and create something so there can be some further discussion/action on this.

webknjaz · 2023-02-05T21:31:35Z

Do you need any help with this?

webknjaz · 2023-07-11T23:49:46Z

with python 3.8 and above ssl will generate SSLZeroReturnError

@toppk could you clarify why you're talking about ssl.SSLZeroReturnError while the CPython PR you linked is about ssl.SSLEOFError? These are different exceptions. Also, AFAICS both are subclasses of ssl.SSLError.

thezoggy · 2023-07-12T02:55:12Z

with python 3.8 and above ssl will generate SSLZeroReturnError

@toppk could you clarify why you're talking about ssl.SSLZeroReturnError while the CPython PR you linked is about ssl.SSLEOFError? These are different exceptions. Also, AFAICS both are subclasses of ssl.SSLError.

because its one and the same, openssl changed things and cpython reacted and such,
python/cpython@13df5d3

webknjaz · 2023-07-13T01:49:11Z

because its one and the same, openssl changed things and cpython reacted and such,
python/cpython@13df5d3

Oh, I didn't realize, thanks!

Python 3.8 introduced a different exception for zero byte tcp connections. These connections are generated by cherrypy on startup, and so the exception is not displayed as it is expected. Without this fix an exception is displayed although it is harmless. This treats the new exception just like it was treated under the errno exception.

Currently, sockets with SSL exceptions are discarded, except for http-over-https where we send back an http error response. For a subset of those exceptions we silient discard them, and for others we will print out the stack trace. This patch updates the code to silient discard all exceptions unless it is the http-over-https case.

This patch turns a new `ssl.SSLEOFError` into an internally ignored `FatalSSLAlert` allowing it not to leak into the outer abstraction layers in its raw form. The exception is new since Python 3.8 and it's fine to use it unconditionally since we no longer support Python 3.7. This patch also handles `SSLZeroReturnError` same as `SSLEOFError` as it's semantically equivalent per [[1]]. [1]: cherrypy#518 (comment)

Specifically, this patch adds an exception interception code to the place where the socket is being first wrapped. In case of the `builtin` TLS adapter, this is also a place where a handshake attempt happens. This switches the method of relaying an unrecoverable connection error from a sentinel of a tuple with two falsy values to raising a unified exception consistently.

This patch extends the processing of a case when a client attempts sending plain HTTP into an HTTPS port by emitting a log message.

webknjaz · 2024-01-24T04:55:35Z

Thanks @toppk!

toppk force-pushed the main branch 2 times, most recently from 4995f4e to 9f17e91 Compare September 22, 2022 02:58

toppk changed the title ~~handle SSLZeroReturnError exceptions~~ Handle SSLZeroReturnError exceptions Sep 22, 2022

toppk force-pushed the main branch from 9f17e91 to c1ff027 Compare September 22, 2022 03:06

webknjaz reviewed Sep 24, 2022

View reviewed changes

cheroot/ssl/builtin.py Show resolved Hide resolved

webknjaz reviewed Sep 24, 2022

View reviewed changes

cheroot/ssl/builtin.py Outdated Show resolved Hide resolved

toppk force-pushed the main branch from 2a9e795 to 5ede676 Compare September 25, 2022 01:11

webknjaz force-pushed the main branch from 5ede676 to 24e692a Compare September 28, 2022 02:23

webknjaz force-pushed the main branch 2 times, most recently from 202fe0b to 7a4712a Compare September 29, 2022 17:02

webknjaz force-pushed the main branch from 7a4712a to 4c5aba6 Compare September 29, 2022 18:43

thezoggy mentioned this pull request Sep 30, 2022

ENGINE Error in HTTPServer.serve, cheroot, old issue.. back again? sabnzbd/sabnzbd#2228

Closed

thezoggy suggested changes Oct 1, 2022

View reviewed changes

thezoggy mentioned this pull request Oct 21, 2022

ENGINE socket.error 8 sabnzbd/sabnzbd#2065

Closed

The-Compiler mentioned this pull request Nov 8, 2022

ssl.SSLZeroReturnError exception on startup with builtin ssl backend python 3.8 or above #517

Closed

3 tasks

This comment was marked as off-topic.

Sign in to view

webknjaz changed the title ~~Handle SSLZeroReturnError exceptions~~ Handle SSLZeroReturnError exceptions in stdlib TLS adapter Jul 4, 2023

webknjaz force-pushed the main branch from 4c5aba6 to d444baf Compare July 4, 2023 18:43

webknjaz force-pushed the main branch 2 times, most recently from 95ae5ce to 9dca4a9 Compare January 24, 2024 04:28

toppk and others added 2 commits January 24, 2024 05:34

webknjaz force-pushed the main branch from 9dca4a9 to e68ef67 Compare January 24, 2024 04:38

webknjaz added 2 commits January 24, 2024 05:39

Log clients speaking HTTP on the HTTPS port

93d8379

This patch extends the processing of a case when a client attempts sending plain HTTP into an HTTPS port by emitting a log message.

webknjaz force-pushed the main branch from e68ef67 to 93d8379 Compare January 24, 2024 04:45

webknjaz closed this pull request by merging all changes into cherrypy:main in 0fd16f0 Jan 24, 2024

thezoggy mentioned this pull request Jan 24, 2024

Replace cherrypy by something new sabnzbd/sabnzbd#2180

Open

webknjaz mentioned this pull request Feb 1, 2024

Qualys security scanner kills all cheroot worker threads, hangs server, eats all available file descriptors #365

Closed

15 tasks

Helene mentioned this pull request Mar 2, 2024

How works Prometheus Integration ? IBM/ibm-spectrum-scale-bridge-for-grafana#169

Open

webknjaz added a commit that referenced this pull request Apr 14, 2024

📝 Add a change note for PE #518

8117194

webknjaz added a commit that referenced this pull request Apr 14, 2024

📝 Link issue #517 to the PR #518 change note

48a7b90

scyclops added a commit to scyclops/cheroot that referenced this pull request May 3, 2024

merge upstream fixes from cherrypy#518 & cherrypy#649

5f2162f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle `SSLZeroReturnError` exceptions in stdlib TLS adapter #518

Handle `SSLZeroReturnError` exceptions in stdlib TLS adapter #518

toppk commented Sep 22, 2022 •

edited

Loading

toppk commented Sep 22, 2022

webknjaz commented Sep 24, 2022

webknjaz Sep 24, 2022

toppk Sep 24, 2022

webknjaz Sep 24, 2022

webknjaz Sep 24, 2022

webknjaz Sep 24, 2022

webknjaz Sep 24, 2022

toppk Sep 24, 2022 •

edited

Loading

webknjaz Sep 24, 2022

webknjaz commented Sep 24, 2022

toppk commented Sep 24, 2022 •

edited

Loading

lgtm-com bot commented Sep 24, 2022

toppk commented Sep 24, 2022 •

edited

Loading

webknjaz commented Sep 24, 2022

lgtm-com bot commented Sep 25, 2022

toppk commented Sep 25, 2022

lgtm-com bot commented Sep 25, 2022

webknjaz commented Sep 28, 2022

lgtm-com bot commented Sep 28, 2022

lgtm-com bot commented Sep 29, 2022

lgtm-com bot commented Sep 29, 2022

toppk commented Sep 29, 2022

thezoggy left a comment

webknjaz commented Oct 5, 2022

The-Compiler commented Nov 8, 2022

satmandu commented Jan 4, 2023

toppk commented Jan 4, 2023

webknjaz commented Feb 5, 2023

This comment was marked as off-topic.

webknjaz commented Jul 11, 2023

thezoggy commented Jul 12, 2023

webknjaz commented Jul 13, 2023

webknjaz commented Jan 24, 2024

Handle SSLZeroReturnError exceptions in stdlib TLS adapter #518

Handle SSLZeroReturnError exceptions in stdlib TLS adapter #518

Conversation

toppk commented Sep 22, 2022 • edited Loading

toppk commented Sep 22, 2022

webknjaz commented Sep 24, 2022

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

toppk Sep 24, 2022

Choose a reason for hiding this comment

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

toppk Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

webknjaz Sep 24, 2022

Choose a reason for hiding this comment

webknjaz commented Sep 24, 2022

toppk commented Sep 24, 2022 • edited Loading

lgtm-com bot commented Sep 24, 2022

toppk commented Sep 24, 2022 • edited Loading

webknjaz commented Sep 24, 2022

lgtm-com bot commented Sep 25, 2022

toppk commented Sep 25, 2022

lgtm-com bot commented Sep 25, 2022

webknjaz commented Sep 28, 2022

lgtm-com bot commented Sep 28, 2022

lgtm-com bot commented Sep 29, 2022

lgtm-com bot commented Sep 29, 2022

toppk commented Sep 29, 2022

thezoggy left a comment

Choose a reason for hiding this comment

webknjaz commented Oct 5, 2022

The-Compiler commented Nov 8, 2022

satmandu commented Jan 4, 2023

toppk commented Jan 4, 2023

webknjaz commented Feb 5, 2023

This comment was marked as off-topic.

webknjaz commented Jul 11, 2023

thezoggy commented Jul 12, 2023

webknjaz commented Jul 13, 2023

webknjaz commented Jan 24, 2024

Handle `SSLZeroReturnError` exceptions in stdlib TLS adapter #518

Handle `SSLZeroReturnError` exceptions in stdlib TLS adapter #518

toppk commented Sep 22, 2022 •

edited

Loading

toppk Sep 24, 2022 •

edited

Loading

toppk commented Sep 24, 2022 •

edited

Loading

toppk commented Sep 24, 2022 •

edited

Loading