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

FEATURE: support DISCOURSE_SMTP_FORCE_TLS option #11733

Merged
merged 1 commit into from Jan 18, 2021

Conversation

godmar
Copy link
Contributor

@godmar godmar commented Jan 16, 2021

Background: RFC 8314 Section 3.3 asks that:

(...) clients and servers SHOULD implement both STARTTLS on port 587 and Implicit TLS on port 465 (...)

I believe that Discourse currently cannot be configured this way, making it impossible to use with mail servers that do not support port 587. In this case, a Net::ReadTimeout results.

This has led to numerous users (including myself) complaining about being unable to set up Discourse,
see 1, 2, 3 to name three that are possibly related to a lack of support for implicit TLS on port 465.

With this patch, it should be possible to set

DISCOURSE_SMTP_FORCE_TLS=true

to use implicit TLS on port 465 (or any port).

This was tested on a container installed via the 30 minute procedure by directly changing config/environments/production.rb.
I did not bootstrap the entire Discourse images etc., so there may be issues I do not see.

The same problem has been reported in other ActionMailer applications; this ultimately abandoned PR also discussed the idea of automatically enabling this option if port 465 is chosen (which would be more user friendly and could be considered here.)

Feedback is welcome.

Background: RFC 8314 3.3 asks that:

clients and servers SHOULD implement both STARTTLS on
port 587 and Implicit TLS on port 465

Discourse currently cannot be configured this way.
With this patch, it's possible to set
DISCOURSE_SMTP_FORCE_TLS=true to use implicit TLS on port 465
@CLAassistant
Copy link

CLAassistant commented Jan 16, 2021

CLA assistant check
All committers have signed the CLA.

@godmar
Copy link
Contributor Author

godmar commented Jan 16, 2021

PS: upon closer reading of the RFC, also reading the discussion by some email providers, this proposed patch may not go far enough. In the future, the expectation appears to be that Implicit TLS will become the standard (on port 465), with STARTTLS on port 587 eventually being deprecated. RFC 8314 notes that:

Although [...STARTTLS...] has been deployed, an alternate mechanism where TLS is negotiated immediately at connection start on a separate port (referred to in this document as "Implicit TLS") has been deployed more successfully.

RFC 8314 recognizes the fragility of the STARTTLS mechanism and recommends the simpler approach of using straight TLS over which to conduct the SMTP session for the purpose of mail submission. Discourse should thus support it, at the very least.

Beyond that, there may be multiple opportunities to smooth this critical step in the installation process:

  • default to Implicit TLS if port is 465
  • default to STARTTLS if port is 587
  • catch the NetTimeout error and retry with Implicit TLS and note and record if successful.
  • DISCOURSE_SMTP_PORT should be a required parameter since the default 25 is in conflict with RFC 8314 4.1 and probably doesn't work for anyone anyway (?)
  • it may also be possible to add the ability to the test_email_settings.rb script (is this what discourse-doctor calls?). Right now, it also lacks support for Implicit TLS.

Even though I put forth this PR, I will likely not have the time to contribute more to this PR so a core developer should take over.

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/sending-email-failed-with-smtps-port-465/65787/10

@eviltrout
Copy link
Contributor

Thanks for this. As an optional setting it seems quite reasonable.

@eviltrout eviltrout merged commit 9aeece4 into discourse:master Jan 18, 2021
@godmar
Copy link
Contributor Author

godmar commented Jan 18, 2021

Thanks for this. As an optional setting it seems quite reasonable.

You're welcome. Please also consider my suggestion for enhancing discourse-doctor to sense NetTimeouts and recommend the setting.

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/feature-endorse-button-to-add-single-click-endorsements/178831/5

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/controversial-run-discourse-without-email-feature/179015/8

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