In-memory channel implementation from PEP 543 #35
Conversation
| ) | ||
|
|
||
| if tls is not None: | ||
| TLSAgainErrors = (tls.WantReadError,) # TODO: How to catch ssl.SSLSyscallError? |
There was a problem hiding this comment.
asyncio also wants to handle ssl.SSLSyscallError, which is not something we currently expose in the API. Should we go through all stdlib modules that use the ssl module and check which exceptions need to be exposed? How do we deal with choosing exceptions to specify in the API in general?
There was a problem hiding this comment.
That is a good question. Looking at the docs, there are 7 unique exception classes in ssl:
ssl.SSLErrorssl.SSLZeroReturnErrorssl.SSLWantReadErrorssl.SSLWantWriteErrorssl.SSLSyscallErrorssl.SSLEOFErrorssl.SSLCertVerificationError
We currently have our own types for WantReadError and WantWriteError, and the rest get converted to a generic TLSError.
IMO, creating our own types for the other 5 should be fine. We could document each new exception type so that it has the same docstring as in the ssl docs, and then future implementations can adapt their own errors to these 7 classes.
@woodruffw WDYT?
There was a problem hiding this comment.
Hmm -- I worry about exposing too many distinct exception types here, in part because of impedance mismatch between other future implementations (e.g. not all implementations may choose to distinguish ZeroReturnError from a generic SSLError).
In practice I think the only way to hit SSLSyscallError is to have an underlying socket syscall get hit with a signal, causing EINTR. So I think this might be appropriate to wrap in an equivalent WantRead or WantWrite, indicating that the call should be re-tried? But I'm not 100% sure about that.
There was a problem hiding this comment.
I actually already added a conversion for ssl.SSLEOFError into RaggedEOF in #42, which for some reason was missing before. ssl.SSLZeroReturnError seems like a strange error (and is also very hard to trigger), but ssl.SSLCertVerificationError may make sense for our API.
I'm not sure about ssl.SSLSyscallError, but I can look into whether there are any other open source libraries handling it.
There was a problem hiding this comment.
A couple of asyncio clones (uvloop, Winloop) use it in the same way as asyncio.
Some other libraries seem to treat it as a ssl.SSLEEOFError that gets raised in strange situations.
From trio, which converts ssl.SSLSyscallError to a BrokenResourceError:
# For some reason, EOF before handshake sometimes raises
# SSLSyscallError instead of SSLEOFError (e.g. on my linux
# laptop, but not on appveyor). Thanks openssl.
Similarly, anyio also converts it to a BrokenResourceError (despite being built on top of asyncio?)
However, these are two completely contradictory ways to interpret the error: the asyncio interpretation means you should retry, whereas the EOF interpretation means you should shutdown.
| self._extra.update( | ||
| peercert=peercert, | ||
| cipher=tlsobj.cipher(), | ||
| # compression=tlsobj.compression(), #TODO: compression? |
There was a problem hiding this comment.
Our API forces no compression (because the original PEP 543 implementation did). Should we allow users to choose instead, and expose the choice in the API?
There was a problem hiding this comment.
from a random SO anwser:
TLS compression is removed in TLS 1.3. And even for lower TLS versions it is no longer available in the browsers and disabled by default in OpenSSL.
So maybe it makes sense to not allow users to set it.
There was a problem hiding this comment.
+1 -- TLS compression is a mis-feature.
There was a problem hiding this comment.
Alright, I will document this somewhere so that it's clear why we made this choice.
|
|
||
| # Add extra info that becomes available after handshake. | ||
| self._extra.update( | ||
| peercert=peercert, |
There was a problem hiding this comment.
The peercert stored here now becomes an OpenSSLCertificate, which is basically a path to a (temporary) binary file containing the cert. In the current asyncio implementation it will provide a dict instead (using ssl's getpeercert(False)). We could add a flag to handle this, but I don't think it's a good idea because the concept of Certificate is not well-specified in the API. Allowing access to whatever ssl returns would limit other backends.
There was a problem hiding this comment.
I agree. The dict is not well specified, and the code that converts the certificate to the dict when passing binary_form=False is not straightforward . I also vote for not adding the flag
There was a problem hiding this comment.
Alright, I will document this.
| except Exception as exc: | ||
| handshake_exc = None | ||
| self._set_state(TLSProtocolState.UNWRAPPED) | ||
| # if isinstance(exc, ssl.CertificateError): #TODO: how to catch certificateerror? |
There was a problem hiding this comment.
Same question regarding catching of errors as above: should we promote this exception to be specified in the API?
I lifted the wrapped buffers implementation from PEP 543, sorted out typing information and arranged for some test coverage. It's pretty straightforward since we can reuse all of our context configuration functions. There are some TODOs in the PEP 543 code that I'm planning to tackle, but I would first like to have a discussion about what API we want.