From 9876bb2bd901cd85ea5b650eaff9cc5746c040e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antoine=20Desch=C3=AAnes?= Date: Sun, 4 Oct 2026 13:51:32 -0400 Subject: [PATCH] fix(pop3): track STLS state per session instead of per server (#626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TLS connection state lived on the shared POP3 Server, and Session reached it through its embedded *Server. Once any client completed STLS, every later session saw a non-nil state: CAPA stopped advertising STLS and the STLS command returned "-ERR A TLS session already agreed upon." until the server restarted. Sessions also wrote the shared field concurrently without synchronization. Move tlsState to Session. ForceTLS sessions set it when the session starts, and STLS sets it on the negotiating session only. TestSTLSPerSession runs CAPA, STLS and a TLS handshake on two sessions of one server; it fails against the previous code on the second session. Signed-off-by: Antoine Deschênes Co-authored-by: Claude Opus 5.5 --- pkg/server/pop3/handler.go | 35 ++++++++++++++++++--------------- pkg/server/pop3/handler_test.go | 28 ++++++++++++++++++++++++++ pkg/server/pop3/listener.go | 1 - 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/pkg/server/pop3/handler.go b/pkg/server/pop3/handler.go index b61d326..61c17ec 100644 --- a/pkg/server/pop3/handler.go +++ b/pkg/server/pop3/handler.go @@ -60,19 +60,20 @@ var commands = map[string]bool{ // Session defines an active POP3 session type Session struct { - *Server // Reference to the server we belong to. - id int // Session ID number. - conn net.Conn // Our network connection. - remoteHost string // IP address of client. - sendError error // Used to bail out of read loop on send error. - state State // Current session state. - reader *bufio.Reader // Buffered reader for our net conn. - user string // Mailbox name. - messages []storage.Message // Slice of messages in mailbox. - retain []bool // Messages to retain upon UPDATE (true=retain). - msgCount int // Number of undeleted messages. - logger zerolog.Logger // Session specific logger. - debug bool // Print network traffic to stdout. + *Server // Reference to the server we belong to. + id int // Session ID number. + conn net.Conn // Our network connection. + remoteHost string // IP address of client. + sendError error // Used to bail out of read loop on send error. + state State // Current session state. + reader *bufio.Reader // Buffered reader for our net conn. + user string // Mailbox name. + messages []storage.Message // Slice of messages in mailbox. + retain []bool // Messages to retain upon UPDATE (true=retain). + msgCount int // Number of undeleted messages. + logger zerolog.Logger // Session specific logger. + debug bool // Print network traffic to stdout. + tlsState *tls.ConnectionState // TLS connection state, nil until TLS is set up. } // NewSession creates a new POP3 session @@ -107,11 +108,12 @@ func (s *Server) startSession(ctx context.Context, id int, conn net.Conn) { Int("session", id).Logger() logger.Debug().Msgf("ForceTLS: %t", s.config.ForceTLS) connToClose := conn + var tlsState *tls.ConnectionState if s.config.ForceTLS { logger.Debug().Msg("Setting up TLS for ForceTLS") tlsConn := tls.Server(conn, s.tlsConfig) - s.tlsState = new(tls.ConnectionState) - *s.tlsState = tlsConn.ConnectionState() + tlsState = new(tls.ConnectionState) + *tlsState = tlsConn.ConnectionState() conn = tlsConn } @@ -127,6 +129,7 @@ func (s *Server) startSession(ctx context.Context, id int, conn net.Conn) { }() ssn := NewSession(s, id, conn, logger) + ssn.tlsState = tlsState ssn.send(fmt.Sprintf("+OK Inbucket POP3 server ready <%v.%v@%v>", os.Getpid(), time.Now().Unix(), s.config.Domain)) @@ -144,7 +147,7 @@ func (s *Server) startSession(ctx context.Context, id int, conn net.Conn) { ssn.send("USER") ssn.send("UIDL") ssn.send("IMPLEMENTATION Inbucket") - if s.tlsConfig != nil && s.tlsState == nil && !s.config.ForceTLS { + if s.tlsConfig != nil && ssn.tlsState == nil && !s.config.ForceTLS { ssn.send("STLS") } ssn.send(".") diff --git a/pkg/server/pop3/handler_test.go b/pkg/server/pop3/handler_test.go index baa17ee..47c43c7 100644 --- a/pkg/server/pop3/handler_test.go +++ b/pkg/server/pop3/handler_test.go @@ -21,6 +21,7 @@ import ( "github.com/inbucket/inbucket/v3/pkg/config" "github.com/inbucket/inbucket/v3/pkg/storage" "github.com/inbucket/inbucket/v3/pkg/test" + "github.com/stretchr/testify/require" ) func TestNoTLS(t *testing.T) { @@ -245,6 +246,33 @@ func TestDupStartTLS(t *testing.T) { } } +// TestSTLSPerSession verifies that STLS state belongs to each session: a +// completed STLS negotiation must not affect later sessions on the same server. +func TestSTLSPerSession(t *testing.T) { + server := setupPOPServer(t, test.NewStore(), true, false) + + for i := 1; i <= 2; i++ { + serverConn, clientConn := net.Pipe() + startPOP3Session(t, server, &mockConn{serverConn}) + t.Cleanup(func() { _ = clientConn.Close() }) + + c := textproto.NewConn(clientConn) + readPOP3Greeting(t, c) + playPOP3Script(t, c, []scriptStep{ + {"CAPA", []string{"+OK Capability list follows", "TOP", "USER", "UIDL", "IMPLEMENTATION Inbucket", "STLS", "."}}, + {"STLS", []string{"+OK Begin TLS Negotiation"}}, + }) + + tlsConn := tls.Client(clientConn, &tls.Config{InsecureSkipVerify: true}) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + require.NoErrorf(t, tlsConn.HandshakeContext(ctx), "session %d: TLS handshake", i) + cancel() + playPOP3Script(t, textproto.NewConn(tlsConn), []scriptStep{ + {"QUIT", []string{"+OK Goodnight and good luck"}}, + }) + } +} + func TestForceTLS(t *testing.T) { ds := test.NewStore() server := setupPOPServer(t, ds, true, true) diff --git a/pkg/server/pop3/listener.go b/pkg/server/pop3/listener.go index e2e2136..ba30242 100644 --- a/pkg/server/pop3/listener.go +++ b/pkg/server/pop3/listener.go @@ -21,7 +21,6 @@ type Server struct { wg *sync.WaitGroup // Waitgroup tracking sessions. notify chan error // Notify on fatal error. tlsConfig *tls.Config // TLS encryption configuration. - tlsState *tls.ConnectionState } // NewServer creates a new, unstarted, POP3 server.