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

fix: pass ServerName in tls config to send mails in secure mode #337

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

anujk14
Copy link
Contributor

@anujk14 anujk14 commented Sep 14, 2023

Bug description

If app.mailer.smtp_insecure is set to false in the config, Frontier is unable to send emails, and we get the following error: tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config.

When app.mailer.smtp_insecure is set to true, there is no such error. However, in this mode, TLS is susceptible to main-in-the-middle attacks.

Expected behaviour

Clients should be able to configure app.mailer.smtp_insecure to false, as this is the recommended setting when sending mails in production environments.

Solution

  • Golang's tls module expects a ServerName if InsecureSkipVerify is set to false (Reference to source code). Without that, it raises the error: tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config.
  • The ServerName is used to verify the hostname on the returned certificates.
  • More details about ServerName and InsecureSkipVerify can be found in the tls documentation.
  • This PR fixes the issue by setting ServerName in the tlsConfig, irrespective of the value of InsecureSkipVerify

Golang's tls module expects a ServerName if InsecureSkipVerify is set to false. The ServerName is used to verify the hostname on the returned certificates.
@vercel
Copy link

vercel bot commented Sep 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
frontier ✅ Ready (Inspect) Visit Preview Sep 14, 2023 9:57am

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6183836917

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 51.28%

Totals Coverage Status
Change from base Build 6168126313: 0.0%
Covered Lines: 6049
Relevant Lines: 11796

💛 - Coveralls

Copy link
Member

@kushsharma kushsharma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kushsharma kushsharma merged commit 1dd2ea7 into main Sep 14, 2023
10 checks passed
@kushsharma kushsharma deleted the fix_smtp_secure_mode branch September 14, 2023 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants