net/smtp: in smtp.go, c.cmd(501, "*") takes long for server to respond.

[reasonerjt]

What version of Go are you using (go version)?

go version go1.7.3 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build126580691=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"

What did you do?

I tried to establish a tls connection to an smtp server and call client.Auth to authenticate, code may look like this:

	conn, _ := tls.Dial("tcp", host+":"+port, nil)
	client, _ := smtp.NewClient(conn, host)
	err := client.Auth(smtp.PlainAuth("", user, password, host))

What did you see instead?

When the incorrect username/password is provided the function Auth took 1 minute to return. I found it was due to this line:

https://github.com/golang/go/blob/master/src/net/smtp/smtp.go#L221

that the client tries to send "*" to server and expects "501" which is "Syntax Error", it took very long for server to respond. If I comment this line and rebuild. I can get the error very quickly.

Could anyone let me know why is this line necessary? And is it common the server took 1 minute to respond "*"?

[bradfitz]

No clue. It's been there since the very beginning, in df74d8d from @edsrzf. Maybe Evan knows.

[edsrzf]

I've plumbed the depths of my memory, looked at the code and the code review, and tried to reason about it, but I can't remember or figure it out. Sorry I can't be of more help.

[bradfitz]

That's still helpful.

If an SMTP expert wants to weigh in, maybe it's safe to just delete it. We're already in an error path.

[reasonerjt]

By reading the RFC: https://www.ietf.org/rfc/rfc2554.txt

         The authentication protocol exchange consists of a series of
         server challenges and client answers that are specific to the
         authentication mechanism.  A server challenge, otherwise known
         as a ready response, is a 334 reply with the text part
         containing a BASE64 encoded string.  The client answer consists
         of a line containing a BASE64 encoded string.  If the client
         wishes to cancel an authentication exchange, it issues a line
         with a single "*".  If the server receives such an answer, it
         MUST reject the AUTH command by sending a 501 reply.

And in the case of OP, when incorrect credentials are sent by client, the server returns 535 which means the AUTH exchange has already been rejected, so there's no need to send "*" to ask server to reject the AUTH command.

My first time reading this RFC, so let's see what expert says about this issue.

[horgh]

Feel free to ignore this/close this smtp package issue too. I was already looking at it when you closed the other, I figure I may as well finish what I was writing. Sorry for the noise! At least it may lead to closing some issues :-)

I concur with the RFC reading that sending the * message by the client is only correct when an authentication exchange is ongoing. If the server sent a response such as 535 then it has already rejected the AUTH command, so sending * in that situation (which means the client is ending the exchange) does not make sense. (RFC 4954 obsoleted 2554 and says much the same as in the prior comment.)

However simply deleting sending the * all together does not appear totally correct either. Consider the case where we are using the PLAIN mechanism, and the server (incorrectly) sent a 334 response asking for more information. In that case, plainAuth's Next() function will return an error, and we should tell the server to cancel the exchange by sending * prior to aborting. Although given we Quit() immediately it is perhaps not so important.

We also need to send * if decoding the 334 response failed.

Essentially I think what would be the most correct would be to check if the server rejected the AUTH exchange, and if so, quit without sending *. If it didn't, then we should send * and quit.

Of course, testing this in the wild would be good to confirm.

This could be done by checking err and quitting prior to any Next() call. e.g., something like:

		switch code {
		case 334:
			msg, err = encoding.DecodeString(msg64)
		case 235:
			// the last message isn't base64 because it isn't a challenge
			msg = []byte(msg64)
		default:
			err = &textproto.Error{Code: code, Msg: msg64}
		}

		if err != nil {
			// AUTH was rejected. But note this still leaves the handling of a decoding problem
			// not being handled correctly.
			c.Quit()
			break
		}

		resp, err = a.Next(msg, code == 334)
		if err != nil {
			// abort the AUTH
			c.cmd(501, "*")
			c.Quit()
			break
		}

Note there is still an issue in the above: If decoding failed when code was 334, we should send * too. So I think ideally some refactoring to more carefully take into account status code and what error happened would help here. I'd be happy to work on a patch, but as it seems the smtp package may be completely frozen, I'll leave it here for now.

[smasher164]

I ran a modified example that connects to gmail's smtp servers, and was able to reproduce this behavior on my DigitalOcean VM. I believe this has to do with them blocking SMTP over IPV6.

package main

import (
	"crypto/tls"
	"fmt"
	"net"
	"net/smtp"
)

func main() {
	conn, _ := net.Dial("tcp", "smtp.gmail.com:587")
	host, _, _ := net.SplitHostPort("smtp.gmail.com:587")
	c, _ := smtp.NewClient(conn, host)
	if ok, _ := c.Extension("STARTTLS"); ok {
		config := &tls.Config{ServerName: host}
		c.StartTLS(config)
	}
	auth := smtp.PlainAuth("", "invalid username", "bad sy ]]ntax", "smtp.gmail.com")
	if auth != nil {
		if ok, _ := c.Extension("AUTH"); ok {
			if err := c.Auth(auth); err != nil {
				fmt.Println(err)
			}
		}
	}
}

Running this on my macbook produced the correct error immediately:

535 5.7.8 Username and Password not accepted. Learn more at
5.7.8  https://support.google.com/mail/?p=BadCredentials 24sm2558946qtx.8 - gsmtp

But running it on my VM took ~2 minutes to respond. After researching the error, I came across https://www.digitalocean.com/community/questions/outgoing-connections-on-port-25-587-143-blocked-over-ipv6.

Changing this line conn, _ := net.Dial("tcp", "smtp.gmail.com:587") to
conn, _ := net.Dial("tcp4", "smtp.gmail.com:587") produces an immediate response. I suspect the environment in which @reasonerjt is running their example may be blocking smtp over IPV6 as well.

[gopherbot]

Change https://golang.org/cl/104435 mentions this issue: net/smtp: fix the bug which makes golang cannot handle smtp fail-auth

[cszasd]

Hi, I have a fix from https://golang.org/cl/104435 to resolve this issue, please help review.

The test file need to be changed as well. However as the smtp_test.go contains a private key, I cannot push that change, the git -o nokeycheck not work for me as my git does not have a -o switch. Any idea what shall I do to change the test file?