Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESMTP client quits when TLS negotiation fails with requireTransportSecurity=false #10334

Open
twisted-trac opened this issue Apr 22, 2022 · 3 comments
Labels

Comments

@twisted-trac
Copy link

justmarini's avatar justmarini reported
Trac ID trac#10334
Type defect
Created 2022-04-22 19:32:40Z

My organization is using twisted 19.10.0. We've noticed that the behavior of the ESMTP client's requireTransportSecurity is inconsistent with other systems, and can be confusing to end-users.

We have a user with an STMP server advertising STARTTLS. Their certificate, however, is a default one, so they've advised us to -not- use TLS (it's an internal system). By specifying requireTransportSecurity=false, we expected that this would work around their system.

However, the observed behavior is for the TLS negotiation to fail, and the client to issue a QUIT.

< 220 testserver.local ESMTP Postfix
> EHLO testclient
< 250-testserver.local
< 250-PIPELINING
< 250-SIZE 10240000
< 250-VRFY
< 250-ETRN
< 250-STARTTLS
< 250-DSN
< 250 SMTPUTF8
> STARTTLS
< 454 TLS not available due to a temporary reason
> QUIT

In smtp.py, the callback is the same for either requireTransportSecurity case.

        elif canTLS:
            self._expected = [220]
            self._okresponse = self.esmtpState_starttls
            self._failresponse = self.esmtpTLSFailed
            self.sendLine(b"STARTTLS")

The response should instead distinguish between the two. If TLS is required, quit on fail. If TLS is not required, continue with the auth attempt.

Searchable metadata
trac-id__10334 10334
type__defect defect
reporter__justmarini justmarini
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__new new
resolution__None None
component__core core
keywords__None None
time__1650655960814253 1650655960814253
changetime__1650655960814253 1650655960814253
version__None None
owner__None None

@lms-jk
Copy link

lms-jk commented Oct 13, 2022

I just ran into a very similar problem with Buildbot 3.6.1 and twisted-22.04.0, the root cause might be the same. Our internal SMTP relay setup is just like the one from justmarini. Buildbot prints the following stacktrace:

Click me
2022-10-13 09:20:07+0200 [-] sending mail (436 bytes) to ['redacted1@domain.com', 'redacted2@domain.com']
2022-10-13 09:20:07+0200 [-] Starting factory <twisted.mail.smtp.ESMTPSenderFactory object at 0x7f3c30695f40>
2022-10-13 09:20:07+0200 [ESMTPSender,client] SMTP Client retrying server. Retry: 5
2022-10-13 09:20:08+0200 [ESMTPSender,client] SMTP Client retrying server. Retry: 4
2022-10-13 09:20:08+0200 [ESMTPSender,client] SMTP Client retrying server. Retry: 3
2022-10-13 09:20:09+0200 [ESMTPSender,client] SMTP Client retrying server. Retry: 2
2022-10-13 09:20:09+0200 [ESMTPSender,client] SMTP Client retrying server. Retry: 1
2022-10-13 09:20:10+0200 [ESMTPSender,client] Got exception when handling reporter events
        Traceback (most recent call last):
          File "/buildbot_venv/lib/python3.9/site-packages/twisted/internet/defer.py", line 1791, in gotResult
            _inlineCallbacks(r, gen, status, context)
          File "/buildbot_venv/lib/python3.9/site-packages/twisted/internet/defer.py", line 1692, in _inlineCallbacks
            result = context.run(
          File "/buildbot_venv/lib/python3.9/site-packages/twisted/python/failure.py", line 518, in throwExceptionIntoGenerator
            return g.throw(self.type, self.value, self.tb)
          File "/buildbot_venv/lib/python3.9/site-packages/buildbot/reporters/base.py", line 113, in _got_event
            log.err(e, 'Got exception when handling reporter events')
        --- <exception caught here> ---
          File "/buildbot_venv/lib/python3.9/site-packages/buildbot/reporters/base.py", line 111, in _got_event
            yield self.sendMessage(reports)
        twisted.mail._except.SMTPConnectError: Unable to connect to server.

2022-10-13 09:20:10+0200 [-] Stopping factory <twisted.mail.smtp.ESMTPSenderFactory object at 0x7f3c30695f40>

I also recorded the packets with Wireshark (sorry I can't publish the captured packets):
TLS Error

It seems twisted does not handle the untrusted certificate well when probing for TLS capabilities.

As workaround I set canTLS = False in smtp.py to hide TLS capabilities from Twisted.

@lms-jk
Copy link

lms-jk commented Dec 6, 2023

My issue was resolved with buildbot/buildbot#5609.

@adiroiban
Copy link
Member

adiroiban commented Dec 6, 2023

Thanks for the update.

I am not sure this is a big... just a missing implementation detail.

I can't fidn the documentation informaing that when requireTransportSecurity=False and startTLS fails, it will continue without raising an error.

# has tls can tls must tls result
# t t t authenticate
# t t f authenticate
# t f t authenticate
# t f f authenticate
# f t t STARTTLS
# f t f STARTTLS
# f f t esmtpTLSRequired
# f f f authenticate
hasTLS = self._tlsMode
canTLS = self.context and b"STARTTLS" in items
mustTLS = self.requireTransportSecurity

... So far the code works as documented, STARTTLS is alwasy triggered.

        # has tls        can tls         must tls       result
        #   f               t               t           STARTTLS
        #   f               t               f           STARTTLS

and documentation for requireTransportSecurity

    @ivar requireTransportSecurity: If C{True}, refuse to proceed if the
        transport cannot be secured. If the transport layer is not already
        secured via TLS, this will override L{heloFallback}.

I guess that we need to end the documentation to also talk about what happen when requireTransportSecurity=false

I think that for canTLS=True and mustTLS=False we can try starttls, but then just log an error and continue with non TLS authentication.

Happy to review a PR for this.

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants