emersion / emersion/go-smtp

server: DATA command timeout should close the connection

Open
#196 3 comments 0 reactions 0 assignees View on GitHub
server
Dominant language
Go
Stars
2.1k
Forks
274
Avg merge
6d 22h
Merged PRs (30d)
1

Description

Basically go-smtp issues two timeouts, configured by `ReadTimeout`: for DATA command, for the idle connection:

```
354 2.0.0 Go ahead. End your data with .

# doing nothing in DATA command

421 4.0.0 read tcp 127.0.0.1:1025->127.0.0.1:56290: i/o timeout # timeout triggered after X time
221 2.4.2 Idle timeout, bye bye # another timeout triggered after another X time
# TCP connection is closed
```

For instance AWS SES closes the connection on data timeout:

```
354 End data with .

# doing nothing in DATA command

451 4.4.2 Timeout waiting for data from client.
# TCP connection is closed
```

See [RFC5321](https://www.rfc-editor.org/rfc/rfc5321.html#section-4.2.2) `451 Requested action aborted: error in processing`.

Not closing the connection on DATA timeout causes a slow postfix client talking to go-smtp server to send the data even, when the go-smtp server responds with `421 4.0.0` and the DATA's leftover data is considered as a mangled SMTP command in go-smtp server.

I suggest to catch the DATA timeout exception and close the connection, e.g.

```diff
--- a/conn.go
+++ b/conn.go
@@ -673,18 +673,25 @@ func (c *Conn) handleData(arg string) {
// We have recipients, go to accept data
c.WriteResponse(354, EnhancedCode{2, 0, 0}, "Go ahead. End your data with .")

- defer c.reset()
-
if c.server.LMTP {
c.handleDataLMTP()
+ c.reset()
return
}

r := newDataReader(c)
- code, enhancedCode, msg := toSMTPStatus(c.Session().Data(r))
+ err := c.Session().Data(r)
+ code, enhancedCode, msg := toSMTPStatus(err)
r.limited = false
io.Copy(ioutil.Discard, r) // Make sure all the data has been consumed
c.WriteResponse(code, enhancedCode, msg)
+
+ c.reset()
+
+ // close a connection on data command timeout
+ if err == ErrDataTimeout {
+ c.Close()
+ }
}

func (c *Conn) handleBdat(arg string) {
--- a/data.go
+++ b/data.go
@@ -3,6 +3,7 @@ package smtp
import (
"bufio"
"io"
+ "net"
)

type EnhancedCode [3]int
@@ -42,6 +43,12 @@ var ErrDataTooLarge = &SMTPError{
Message: "Maximum message size exceeded",
}

+var ErrDataTimeout = &SMTPError{
+ Code: 451,
+ EnhancedCode: EnhancedCode{4, 4, 2},
+ Message: "Timeout waiting for data from client",
+}
+
type dataReader struct {
r *bufio.Reader
state int
@@ -93,6 +100,9 @@ func (r *dataReader) Read(b []byte) (n int, err error) {
if err == io.EOF {
err = io.ErrUnexpectedEOF
}
+ if e, ok := err.(net.Error); ok && e.Timeout() {
+ err = ErrDataTimeout
+ }
break
}
switch r.state {
```

Ideally it would be better to preserve the original err inside the SMTPError struct, e.g. implement some kind of err wrapping like it's done with https://go.dev/blog/go1.13-errors#errors-in-go-113

Contributor guide

Open the contributing guide

Research direction

Start in conn.go at Conn.handleData and in data.go at dataReader.Read; trace how read timeouts become SMTP responses and how the connection is reset. Done means a DATA timeout returns the 451 response and closes the connection, while normal and LMTP handling retain their existing behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.