wneessen / go-mail

📧 Easy to use, yet comprehensive library for sending mails with Go
https://go-mail.dev
MIT License
677 stars 48 forks source link

Add default ports and fallback behavior for SSL and TLS #170

Closed wneessen closed 9 months ago

wneessen commented 9 months ago

Introduced default ports for SSL/TLS and STARTTLS connections in SMTP client. Also added fallback behavior, allowing the client to attempt connections on port 25 using plaintext if secured connections fail. Deprecated old methods and implemented new ones to enforce these changes effectively.

This is a copy of the PR #106 (muhlemmer:enhance-default-tls-port) by @muhlemmer. Since they unfortunately didn't reply in the PR anymore I cloned it instead. They will be fully attributed in the PR, though.

Closes #105 and #106.

codecov-commenter commented 9 months ago

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (1c39dc8) 80.99% compared to head (4a90c2d) 80.62%.

:exclamation: Current head 4a90c2d differs from pull request most recent head 1efac7f. Consider uploading reports for the commit 1efac7f to get more accurate results

Files Patch % Lines
client.go 68.42% 9 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #170 +/- ## ========================================== - Coverage 80.99% 80.62% -0.37% ========================================== Files 24 24 Lines 2078 2116 +38 ========================================== + Hits 1683 1706 +23 - Misses 281 292 +11 - Partials 114 118 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mitar commented 8 months ago

This change makes no sense to me? It hard-codes port to 25 for non-TLS connections, it seems to me? There is no way to set fallbackPort?

Previous behavior made more sense to me. With TLSOpportunistic, I want to connect to port X, if it supports STARTTLS, use TLS, otherwise, use the same port, but just do not use TLS. Currently, if I use SetTLSPortPolicy(TLSOpportunistic), if the server does not support STARTTLS on port X, it tries to connect to port 25 on that server - which might not even be available (or accessible - firewalls and stuff).

I do not get issue #105 either. Is this about behavior if one does not call WithPort? I think if WithPort is not made, then default should be to connect to port 25 (or 587?) and try STARTTLS on it, if it fails and TLSOpportunistic is set, use no TLS on same port. The behavior to try different ports makes no sense to me. You use one port, but potentially negotiate different configuration of how you use it. And WithPort is then used to configure which port is the one the client uses.

Luckily, WithTLSPolicy still works and if I use WithTLSPolicy it is exactly how I would want. Maybe just remove deprecation on it?

But I do think that connecting to multiple ports is a strange approach. That might be done by some higher-level library to auto-detect configuration of an unknown SMTP server. But not by a low-level library like this one where you generally know the server you are connecting to.

mitar commented 8 months ago

Also, interestingly, SetTLSPolicy was not market as deprecated.

wneessen commented 8 months ago

Hi @mitar,

thanks for raising your concerns. The main reason for #105 and therefore this change was, that we wanted to follow established standards, but I agree that not recognizing WithPort() when using *TLSPortPolicy is an oversight on my end. WithPort should always have higher priority over automations/auto-detections. I'll rework this patch taking your comments into consideration.

mitar commented 8 months ago

Sorry, which established standard suggest automatic use of port 25? RFC section referenced from #105 does not mention so, to my understanding? It even says:

However, to maximize the use of encryption for submission, it is desirable to support both mechanisms for Message Submission over TLS for a transition period of several years. As a result, clients and servers SHOULD implement both STARTTLS on port 587 and Implicit TLS on port 465 for this transition period. Note that there is no significant difference between the security properties of STARTTLS on port 587 and Implicit TLS on port 465 if the implementations are correct and if both the client and the server are configured to require successful negotiation of TLS prior to Message Submission.

To me this reads really like "negotiate on the same port" - while 587 is the one where there can be STARTTLS or not.

wneessen commented 8 months ago

Correct. Before we were defaulting to port 25, which is not best practice. The fallback option was just to keep compatiblity with that behaviour. This will be reworked in #181

wneessen commented 7 months ago

@mitar The whole TLSPortPolicy() situation has been refined in #181. I've removed the deprecation notes and replaced them with recommendation notes instead. Also TLSPortPolicy (and the SSL equivalent) will not change the ports anymore, if the port has been set already otherwise (i. e. via WithPort()). I think this is a better solution now and given that the deprecation notes are gone, the users are free to chose either solution - either the "handholding" one or the "low-level" one.

mitar commented 7 months ago

Awesome, thanks!