[Twisted-Python] startTLS errors not propagating to Factory

[Richard van der Hoff]

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.

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.

Cheers

Richard


PS: yes, once we figure out what's going wrong here, I'll at least write 
up an issue...