Dialer.RequireStartTLS to configure STARTTLS security #25
Conversation
In order to prevent `mail.doTestSendMail` from being overwhelmed by parameters, stop creating `mockClient` inside that method and have its caller create it. Since there are `testSendMail` and `testSendMailTimeout` helpers, they create the `mockClient`.
When `mockClient.startTLS` is true, then an SMTP transaction asking for the STARTTLS extension will find it available. Otherwise, it is missing.
It is possible to use a man-in-the-middle attack to strip STARTTLS security from an SMTP connection. Because all modern SMTP servers support STARTTLS, we recommend that `Dialer.RequireStartTLS` be set to `mail.MandatoryStartTLS`. For backwards compatibility, this library defaults to `mail.OpportunisticStartTLS`, which means that the client will fallback to a cleartext connection if the SMTP server does not support the STARTTLS extension. This has no effect on SMTPS connections where TLS wraps the connection itself.
|
@ivy Please review this draft patch. I'd be happy to take any suggestions on how to make things simpler when it comes to setting options on the |
| @@ -27,9 +27,17 @@ type Dialer struct { | |||
| // most cases since the authentication mechanism should use the STARTTLS | |||
| // extension instead. | |||
| SSL bool | |||
| // TSLConfig represents the TLS configuration used for the TLS (when the | |||
| // TLSConfig represents the TLS configuration used for the TLS (when the | |||
smtp.go
Outdated
| // but we recommend MandatoryStartTLS for all modern SMTP servers. | ||
| // | ||
| // This option has no effect if SSL is set to true. | ||
| RequireStartTLS StartTLSSecurity |
There was a problem hiding this comment.
I think StartTLSPolicy would be a more fitting name for both RequireStartTLS and StartTLSSecurity.
smtp.go
Outdated
| if !d.SSL && d.RequireStartTLS != NoStartTLS { | ||
| ok, _ := c.Extension("STARTTLS") | ||
| if !ok && d.RequireStartTLS == MandatoryStartTLS { | ||
| err := fmt.Errorf( |
There was a problem hiding this comment.
We should export an error for this (e.g. ErrStartTLSUnsupported).
There was a problem hiding this comment.
Fixed in d3eed43, but named StartTLSUnsupportedError to follow Go conventions: https://blog.golang.org/error-handling-and-go
There was a problem hiding this comment.
That much better than my suggestion. I like your use of StartTLSPolicy.String() for handling the error message.😄
Instead of returning a bare error, `Dial` returns `StartTLSUnsupportedError` when `Dialer.StartTLSPolicy` requires STARTTLS, but it is unsupported by the server.
Add Dialer.StartTLSPolicy for securing STARTTLS
|
@sfllaw Thanks for implementing this along with the changes I suggested. 👍 |
|
@ivy Thanks for catching the missing change in Do you have plans to release 2.2.0 soon? I'd like to tighten up STARTTLS in some of our code. 😄 |
|
@sfllaw Sure thing, I tagged and released 2.2.0. Thanks again for helping tighten up security! 👍 |
It is possible to use a man-in-the-middle attack to strip STARTTLS security from an SMTP connection. Because all modern SMTP servers support STARTTLS, we recommend that
Dialer.RequireStartTLSbe set tomail.MandatoryStartTLS.For backwards compatibility, this library defaults to
mail.OpportunisticStartTLS, which means that the client will fallback to a cleartext connection if the SMTP server does not support the STARTTLS extension.This has no effect on SMTPS connections where TLS wraps the connection itself.
Closes #23.