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

Rework TLSPortPolicy() #181

Closed
wneessen opened this issue Feb 27, 2024 · 2 comments · Fixed by #207
Closed

Rework TLSPortPolicy() #181

wneessen opened this issue Feb 27, 2024 · 2 comments · Fixed by #207
Assignees
Labels
bug Something isn't working

Comments

@wneessen
Copy link
Owner

          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.

Originally posted by @mitar in #170 (comment)

@wneessen wneessen self-assigned this Feb 27, 2024
@wneessen wneessen added the bug Something isn't working label Feb 27, 2024
@9214
Copy link

9214 commented Mar 13, 2024

Hey. Just wanted to chime in and note that current client configuration behavior is order-dependent. Consider:

options := []mail.Option{
  mail.WithTLSPortPolicy(mail.NoTLS),
  mail.WithPort(1025),
}
sender, err := mail.NewClient("localhost", options...)

This works as intended (trying to connect to localhost:1025), because WithTLSPortPolicy(mail.NoTLS) sets port to 25, and then WithPort rewrites it back to 1025. But if you swap the options around it obviously fails.

@wneessen
Copy link
Owner Author

That's good information to take into consideration. Thanks @9214

wneessen added a commit that referenced this issue Apr 6, 2024
The update modifies the client's handling of port selection when configuring SSL/TLS connections. The clients' functions `WithSSLPort`, `WithTLSPortPolicy`, `SetTLSPortPolicy`, and `SetSSLPort` are revised to avoid overriding previously set ports. Additionally, the deprecation notes have been removed and replaced with notes on best-practice recommendations, referring the new *Port*() methods. This change revises #105 and takes the comments made in #181 into account.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants