Handle SSLZeroReturnError exceptions

[toppk]

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.

https://github.com/python/cpython/pull/18772

❓ What kind of change does this PR introduce?

• [X] 🐞 bug fix
• [ ] 🐣 feature
• [ ] 📋 docs update
• [ ] 📋 tests/coverage improvement
• [ ] 📋 refactoring
• [ ] 💥 other

📋 What is the related issue number (starting with #) Resolves https://github.com/cherrypy/cheroot/issues/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)

• [ ] I wrote descriptive pull request text above
• [ ] I think the code is well written
• [ ] I wrote good commit messages
• [ ] I have squashed related commits together after the changes have been approved
• [ ] Unit tests for the changes exist
• [ ] Integration tests for the changes exist (if applicable)
• [ ] I used the same coding conventions as the rest of the project
• [ ] The new code doesn't generate linter offenses
• [ ] Documentation reflects the changes
• [ ] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences

---

This change is 

[toppk]

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

[webknjaz]

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]

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]

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[bot]]

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

new alerts:

• 1 for Unreachable 'except' block
• 1 for Unused import

[toppk]

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

[webknjaz]

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[bot]]

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

new alerts:

• 1 for Unused import

[toppk]

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,

1. that if we are deciding to log an exception, the stack trace isn't the best format
2. 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[bot]]

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

new alerts:

• 1 for Unused import

[webknjaz]

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[bot]]

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

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

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

new alerts:

• 1 for Unused import

[lgtm-com[bot]]

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

new alerts:

• 1 for Unused import

[toppk]

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.

[webknjaz]

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.

Yes, that's what the LGTM service has been reporting, along with the linters. The author acknowledged this too.

[webknjaz]

Ref: https://github.com/cherrypy/cheroot/issues/346

[The-Compiler]

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

[satmandu]

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

[toppk]

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]

Do you need any help with this?

[vps-eric]

Excited for this to get moving again. It's been half a year now.

[webknjaz]

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]

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, https://github.com/python/cpython/commit/13df5d3497e6148b2b42bed7b5593144f8f6216c

[webknjaz]

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

Oh, I didn't realize, thanks!

[webknjaz]

Thanks @toppk!