golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.41k stars 17.59k forks source link

net/smtp: unable to retrieve SMTP server response error information after EHLO command in (*Client).hello() function #56125

Open liangxj8 opened 2 years ago

liangxj8 commented 2 years ago

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

$ go 1.12.7

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GOHOSTARCH="amd64"
GOHOSTOS="darwin"

What did you do?

send email using a non-whitelist ip address

What did you expect to see?

function smtp.SendMail return the real error {"Code": 117, "Msg": "The IP Address is not whitelisted: x.x.x.x"} from (*Client).ehlo()

What did you see instead?

function smtp.SendMail return the error EOF from (*Client).helo()

Proposal

Before

The comment of (*Client).helo() is helo sends the HELO greeting to the server. It should be used only when the server does not support ehlo., but in (*Client) hello() the implement is:

// hello runs a hello exchange if needed.
func (c *Client) hello() error {
    if !c.didHello {
        c.didHello = true
        err := c.ehlo()
        if err != nil {
            c.helloError = c.helo()
        }
    }
    return c.helloError
}

When we get a error from (*Client).helo() not only 502 (permitted only with an old-style server that does not support EHLO)(see 4.3.2 Command-Reply Sequences), the c.helo() will still execute and return another error like EOF what I got. When we print error like "EOF" in log, we cannot get any thing helpful to debug.

After

// hello runs a hello exchange if needed.
func (c *Client) hello() error {
    if !c.didHello {
        c.didHello = true
        c.helloError = c.ehlo()
        if c.helloError != nil && c.helloError.(*textproto.Error).Code == 502 {
            c.helloError = c.helo()
        }
    }
    return c.helloError
}
ianlancetaylor commented 2 years ago

There doesn't seem to be an API change here, so I don't think this has to be a proposal. This looks more like a bug fix (though I don't know whether it is the right thing to do). Taking this issue out of the proposal process.