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

WIP: client: use standard port when setting SSL/TLS #106

Closed
wants to merge 1 commit into from

Conversation

muhlemmer
Copy link

See issue #105.

TODO:

  1. Update / run tests;
  2. In Client.DailWithContext() an error is potentially lost when using fallback. I have to look into appending or logging the error somehow;
  3. The default TLSPolicy is actually already TLSMandatory. However, the default port is 25. I left this unchanged as it would break package consumers depending on these defaults. Perhaps this can be better updated for the next major release?

A preliminary code review on this PR would be appreciated. And if you have suggestions for item 2, it would be welcome.

This change enables automatic use of ports 465
and 587 when SSL or TLS is configured.
Fallback options to port 25 are also provided.

Updates wneessen#105 {WIP}
//
// When the SSL connection fails and fallback is set to true,
// the client will attempt to connect on port 25 using plaintext.
func WithSSLPort(fallback bool) Option {
Copy link
Owner

Choose a reason for hiding this comment

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

Just a cosmetical "issue". So far we've tried to follow the Go "best practices" for variable names. So for consistency I suggest f instead of fallback

@wneessen
Copy link
Owner

See issue #105.

TODO:

1. Update / run tests;

2. In `Client.DailWithContext()` an error is potentially lost when using fallback. I have to look into appending or logging the error somehow;

3. The default `TLSPolicy` is actually already `TLSMandatory`. However, the default port is 25. I left this unchanged as it would break package consumers depending on these defaults. Perhaps this can be better updated for the next major release?

A preliminary code review on this PR would be appreciated. And if you have suggestions for item 2, it would be welcome.

Code looks great. I love the "port is choses automatically" parts. I've added one suggestion for a cosmetical change, but functionallity wise I think this is all fine.

For the error in 2... I was thinking of implementing a go-mail specific error handling, simliar to the SMTPError we recently introduced, as well. In that we could set up combined errors. So for now maybe accept that we might lose the fallback-error but leave the TODO in the code, so we can address this again, once we have a proper custom error handling? I would assume the fallback case only happens occasionally.

For 3. totally agreed. Let's keep that for a separate PR.

@wneessen
Copy link
Owner

@muhlemmer Do you think you'll be able to work on the open issue sometime soon? I'm planning on releasing the next go-mail version soon and am currently checking on open PRs if I should wait for them to be included or not.

yi-ge added a commit to yi-ge/go-mail that referenced this pull request May 17, 2023
@wneessen
Copy link
Owner

wneessen commented Jan 9, 2024

Since there was no response from @muhlemmer over the last year, I'll merge the current state and then will take the open tasks, so that this PR can be closed.

@wneessen
Copy link
Owner

Superseded by #170

@wneessen wneessen closed this Jan 25, 2024
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.

2 participants