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

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

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

wneessen
Copy link
Owner

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.

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 muhlemmer:enhance-default-tls-port by @muhlemmer. Since they unfortunately didn't reply in the PR anymore I cloned the PR. They will be fully attributed in the PR, though.
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

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%.

❗ 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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wneessen and others added 3 commits January 25, 2024 14:03
Enhanced the unit tests in client_test.go by adding more test cases for SSL Port and TLS Port Policy. These additions contribute to more effective coverage of edge cases and increase the robustness of the test suite.
Test cases have been added for numerous client functionalities including WithTLSPortPolicy option for the NewClient method, Client.SetSSLPort method, and the Client.DialWithContext method with the fallback port functionality. Minor code simplification has also been performed in the 'SetSSLPort' function in client.go file.
@wneessen wneessen merged commit 7dced4b into main Jan 31, 2024
24 checks passed
@wneessen wneessen deleted the feature/105_enhance-default-tls-port branch January 31, 2024 10:32
@mitar
Copy link

mitar commented Feb 27, 2024

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
Copy link

mitar commented Feb 27, 2024

Also, interestingly, SetTLSPolicy was not market as deprecated.

@wneessen
Copy link
Owner Author

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.

@wneessen wneessen mentioned this pull request Feb 27, 2024
@mitar
Copy link

mitar commented Feb 27, 2024

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
Copy link
Owner Author

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
Copy link
Owner Author

wneessen commented Apr 6, 2024

@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
Copy link

mitar commented Apr 6, 2024

Awesome, thanks!

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.

Use appropriate standard port when setting SSL/TLS
3 participants