[Twisted-Python] startTLS errors not propagating to Factory

[Glyph]

> On Apr 9, 2021, at 4:07 PM, Richard van der Hoff <richard at matrix.org> wrote:
> 
> Hi folks,
> 
> I've been investigating https://github.com/matrix-org/synapse/issues/9566, which amounts to: "when there is a TLS error connecting to the SMTP server, the resultant exception is unreadable".
> 
> I think I've traced the problem to the fact that SMTPSenderFactory.clientConnectionFailed is being called with an unhelpful ConnectionAborted rather than anything more descriptive.
> 
> I've then reproduced that with a simpler test case: see https://gist.github.com/richvdh/909761ff5dab23f0873eeddd7936a740. As you can see, the output is: "Factory lost connection. Reason: Connection was aborted locally using ITCPTransport.abortConnection."
> 
> This seems to be thanks to TLSMemoryBIOProtocol.failVerification, which stashes the error and calls abortConnection(): https://github.com/twisted/twisted/blob/trunk/src/twisted/protocols/tls.py#L427.
> 
> At this point I'm struggling. Is the SMTP code holding the Factory wrong? Or is it reasonable to expect the verification error to propagate into clientConnectionFailed - in which case, how could this work?
> 
> Thanks for your thoughts!

Hi Richard,

Sorry for the delayed response here.

This is a bug in Twisted, and I think it boils down to this line: https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/protocols/tls.py#L391 <https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/protocols/tls.py#L391> 

The code as written here appears to be expecting this sequence:

failVerification is called with a reason containing a helpful OpenSSL verification error
we save that reason as `self._reason` for reporting later, calling abortConnection()
since the connection got aborted, we expect our underlying transport to call loseConnection on us
we will then get a falsey reason [?!?!] and as such we will use self._reason instead

Assumption 4 is nonsense and has never been true within Twisted as far as I know; connectionLost always gets a Failure, Failures are never falsey, so we will never use self._reason.  To fix this we need to actually honor self._reason under the conditions we expect, i.e. it's a ConnectionAborted https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/internet/error.py#L209 <https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/internet/error.py#L209> 

Does this make sense?  (Will you be able to file / fix this bug?)

Thanks for the report,

-g
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20210427/2a6b9a01/attachment.htm>