[Twisted-Python] startTLS errors not propagating to Factory

[Glyph]

> On Apr 28, 2021, at 2:43 PM, Richard van der Hoff <richard at matrix.org> wrote:
> 
> On 28/04/2021 07:06, Glyph wrote:
>>> 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 The code as written here appears to be expecting this sequence:
>> 1. failVerification is called with a reason containing a helpful
>>    OpenSSL verification error
>> 2. we save that reason as `self._reason` for reporting later, calling
>>    abortConnection()
>> 3. since the connection got aborted, we expect our underlying transport
>>    to call loseConnection on us
>> 4. 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 
> 
> Thanks very much for your thoughts on this, Glyph - it's always helpful to have an insight into the intended design when trying to resolve this sort of thing.
> 
> I don't follow your reasoning though. I think you might have misread the line you point to (https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/protocols/tls.py#L391). It is "self._reason or reason" - ie, if self._reason is already set, it will take precedence over reason.

Sigh. You're right, I read it backwards.

> In my tests at least, TLSMemoryBIOProtocol.connectionLost is doing exactly the right thing - it is called with an unhelpful reason, but substitutes back in the helpful reason which has already been stashed.
> 
> Rather, the problem, as I see it, is that it's not TLSMemoryBIOProtocol.connectionLost that calls Factory.clientConnectionLost. That is done by tcp.Client.connectionLost, via one of tcp.Client's myriad of base classes, at https://github.com/twisted/twisted/blob/3c868ac11786eef7ea269caa3056f00854128957/src/twisted/internet/tcp.py#L508. Of course, that doesn't get the benefit of TLSMemoryBIOProtocol's reason switcheroo.
> 
> I'm still not quite sure who is in the wrong here.

Aah, yeah, this is a weird quirk of the ancient-style layering in the SMTP code :-|.  The way this should work is by using HostnameEndpoint.

I'm not sure exactly where we're going off the rails, but by using both the old 'startTLS' style of starting a TLS connection, as well as relying on ClientFactory rather than an Endpoint of some kind, means that we're getting this duplicate notification; the one that you get to Protocol.connectionLost will come from TLS and have useful information, but the one that goes to the Connector will be coming straight from TCP.

The right thing to fix here, I think, is to ignore clientConnectionLost entirely, and instead to have the protocol object relay its failure to some other differently-named method on SMTPSenderFactory.

-g

> 
> Cheers
> 
> Richard
> 
> 
> PS: yes, once we figure out what's going wrong here, I'll at least write up an issue...
> 
> _______________________________________________
> Twisted-Python mailing list
> Twisted-Python at twistedmatrix.com
> https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20210428/3f019d6b/attachment-0001.htm>