Use appropriate standard port when setting SSL/TLS

[muhlemmer]

Is your feature request related to a problem? Please describe.

When using SSL or STARTTLS, the default port 25 is always used, which is against best practices as described in RFC8314, section 3.3.

I'm willing to implement and send a PR if this enhancement is accepted.

Describe the solution you'd like

1. WithSSL() to use the port default of 465, unless WithPort() is called;
2. WithTLSPolicy() with TLSMandatory or TLSOpportunistic to use port 587, and in case of the latter use fallback to port 25;

Describe alternatives you've considered

Explicitly setting the port number with WithPort(587) works in my case. My mail server does not allow fallback to plain text SMTP on port 25. But if it where the case, I expect that TLSOpportunistic would not work as intended, as it will always try to use port 587, instead of falling back to 25.

Additional context

To reproduce:

client, err := mail.NewClient(host,
    mail.WithSSL(),
    mail.WithSMTPAuth(mail.SMTPAuthPlain),
    mail.WithUsername(user),
    mail.WithPassword(password),
)

Or

client, err := mail.NewClient(host,
    mail.WithTLSPolicy(mail.TLSMandatory),
    mail.WithSMTPAuth(mail.SMTPAuthPlain),
    mail.WithUsername(user),
    mail.WithPassword(password),
)

Will result in connection failures to compliant hosts.

The above was inspired by the bulk mailer example, which doesn't seem to set an alternative port either.

[wneessen]

Thanks for the issue and the offer to send a PR. The fact that we use port 25 as the default is on me. I've been using 25 as default for too long already, but I 100% agree that we should follow the RFC and not personal "defaults".

So yes, if you are willing to provide a PR, it's more than welcome. Otherwise I'll gladly take this.

[wneessen]

Since some people might rely on the current default, we should mark this change as BREAKING in the release notes, though.

[muhlemmer]

How about adding new methods with the newly described logic:

WithSSLPort()
WithTLSPortPolicy()

And deprecating the old ones? This way it wouldn't constitute a breaking change and code relying on the old behavior won't break.

[wneessen]

Sounds good to me.

[james-d-elliott]

I don't think this is a bad idea, quite a convenient one.

I do think the spec may have been misread however for future reference. I read it as MUA's connecting to specific ports should assume and/or enforce specific TLS modes based on the port in question. In specific what I think was misread is that it's not implying the use of STARTTLS means port 587, but rather the use of port 587 implies the use of STARTTLS (similar for Implicit TLS).

Looking at that I'd propose what I think you've already proposed:

1. If WithSSL() is used and WithPort() is NOT used, i.e. port value is 0, default to port 465.
2. If WithTLSPolicy(TLSMandatory) is used and WithPort() is NOT used, i.e. port value is 0, default to port 587.

I'd also separately (i.e. separate PR) propose:

1. A new option to disable the subsequent requirement items in this list (optional depending on if we want to support configurations that do not comply with RFC8314; maybe DisableRFC8314Section33Compliance() or an ugly function for an ugly thing to support).
2. If WithPort(465) is used WithSSL() (or the effects of) is forcibly applied.
3. If WithPort(587) is used WithTLSPolicy(TLSMandatory) (or the effects of) is forcibly applied.

[wneessen]

Thanks for your inpuit James. Valid points indeed and I second the newly proposed changes. @muhlemmer Let me know if you want to take these as well or if I should take them