FIX "PLAIN" auth during notification via smtp-over-tls on port 465

[ckressibucher]

When creating a NewClient, pass only the hostname
as value for the host parameter, instead of n.conf.Smarthost,
which is hostname port.

This is the same as smtp.Dial does, which is called in
the else-branch.

The value passed as host argument to NewClient is
later compared against the plainAuth.host value:
net/smtp.(*Client).Auth and net/smtp.(*plainAuth).Start
(plainAuth is created created here: https://github.com/prometheus/alertmanager/blob/master/notify/impl.go#L245)

In my tests, this fixed the problems described in #980

Note: after this change, the strings which are compared in
https://golang.org/src/net/smtp/auth.go at (if server.Name != a.host)
are derived from the same config value. I think that is correct, but as it
might affect security, it should be reviewed carefully.

[simonpasquier]

Thanks for the PR! A quick look makes me believe that it is correct. Especially when looking at smtp.Dial() code and seeing the same pattern. Also the fact that the second parameter of smtp.NewClient is named host and not addr. That being said and as you noted, it would require careful reviewing and testing. FWIW this part of the code hasn't changed since #704 that introduced the support for port 465.

func Dial(addr string) (*Client, error) {
	conn, err := net.Dial("tcp", addr)
	if err != nil {
		return nil, err
	}
	host, _, _ := net.SplitHostPort(addr)
	return NewClient(conn, host)
}

[stuartnelson3]

I don't feel well versed enough in email to understand the implications of this code change. If anyone feels confident about their knowledge and ability to test this, by all means raise a hand.

[dirkmb]

👍 for this PR.
Just debugged the 'wrong host error' myself and created the same fix...
should have checked the open PRs first :(

[dirkmb]

If there would be no ServerName set, the DialWithDialer function in crypto/tls/tls.go:132 sets ServerName to the hostname (which does not include the port number)
Also the Dial method in net/smtp/smtp.go:51 does pass the hostname to NewClient.

So what's the reason this PR is not merged yet? ;)

[simonpasquier]

So what's the reason this PR is not merged yet? ;)

See the comments above. We were basically waiting for feedback from people testing this PR. Taking into account your report, I'd say 👍 for me.

[chanjarster]

@simonpasquier Thanks, that works. I have tested gmail(port 587) and another mail provider (port 465 with ssl), everything goes fine.

[simonpasquier]

@mxinden @stuartnelson3 this PR looks reasonable to me and seems to fix the original issue. Any problem from your side merging it?

[mxinden]

Thanks for the help on this @ckressibucher @dirkmb and @chanjarster.