#6524 regression closed fixed (fixed)

twisted.mail.smtp.ESMTPClient tries to STARTTLS multiple times

Reported by: exarkun Owned by: exarkun
Priority: highest Milestone:
Component: mail Keywords:
Cc: ashfall@… Branch: branches/judicious-smtp-starttls-6524
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

If ESMTPClient is used to connect to an SMTP server that offers the STARTTLS feature, it will try to use it. If the SMTP server still offers the STARTTLS after the command has been used once to start TLS, ESMTPClient will try to use it again. This is useless at best, and in practice probably always triggers an error from the server and prevents mail from being sent.

This is a regression introduced by #3989.

Attachments (1)

mailish.py (1.2 KB) - added by exarkun 19 months ago.
It does not work.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 19 months ago by exarkun

Here's a transcript demonstrating the consequences of this bug:

<<< 220 email-smtp.amazonaws.com ESMTP SimpleEmailService-376766033
>>> EHLO top
<<< 250-email-smtp.amazonaws.com
<<< 250-8BITMIME
<<< 250-SIZE 10485760
<<< 250-STARTTLS
<<< 250-AUTH PLAIN LOGIN
<<< 250 Ok
>>> STARTTLS
<<< 220 Ready to start TLS
>>> EHLO top
<<< 250-email-smtp.amazonaws.com
<<< 250-8BITMIME
<<< 250-SIZE 10485760
<<< 250-STARTTLS
<<< 250-AUTH PLAIN LOGIN
<<< 250 Ok
>>> STARTTLS
<<< 454 TLS not available due to temporary reason: TLS already active
>>> QUIT

And attached is a script that reproduces it.

Changed 19 months ago by exarkun

It does not work.

comment:2 Changed 18 months ago by ashfall

  • Cc ashfall@… added

comment:3 Changed 18 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:4 Changed 18 months ago by exarkun

  • Author set to exarkun
  • Branch set to branches/judicious-smtp-starttls-6524

(In [38818]) Branching to 'judicious-smtp-starttls-6524'

comment:5 Changed 18 months ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

Relatively straightforward; only wished I had schematic tables once during the entire development process (arguably the minimum number of times it is possible to wish for schematic tables when writing a piece of software).

comment:6 Changed 18 months ago by radix

  • Owner set to radix
  • Status changed from new to assigned

comment:7 Changed 18 months ago by radix

  • Keywords review removed
  • Owner changed from radix to exarkun
  • Status changed from assigned to new

Schematic tables would be nice. Maybe we should sprint on them some time.

[1] You have a "verify that" in a docstring in a test module, but I guess that's ok because it's not a test_ method itself, just a utility assertion method?

No flakes, tests pass, +1, please merge.

comment:8 Changed 18 months ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [38833]) Merge judicious-smtp-starttls-6524

Author: exarkun
Reviewer: radix
Fixes: #6524

Avoid having twisted.mail.smtp.ESMTPClient try to use the STARTTLS command
if the server offers it but the connection is already using TLS.

Note: See TracTickets for help on using tickets.