fix(pop3): track STLS state per session instead of per server (#626)

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 <antoine@antoinedeschenes.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

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.
+	*Server                         // Reference to the server we belong to.
-	id         int               // Session ID number.
+	id         int                  // Session ID number.
-	conn       net.Conn          // Our network connection.
+	conn       net.Conn             // Our network connection.
-	remoteHost string            // IP address of client.
+	remoteHost string               // IP address of client.
-	sendError  error             // Used to bail out of read loop on send error.
+	sendError  error                // Used to bail out of read loop on send error.
-	state      State             // Current session state.
+	state      State                // Current session state.
-	reader     *bufio.Reader     // Buffered reader for our net conn.
+	reader     *bufio.Reader        // Buffered reader for our net conn.
-	user       string            // Mailbox name.
+	user       string               // Mailbox name.
-	messages   []storage.Message // Slice of messages in mailbox.
+	messages   []storage.Message    // Slice of messages in mailbox.
-	retain     []bool            // Messages to retain upon UPDATE (true=retain).
+	retain     []bool               // Messages to retain upon UPDATE (true=retain).
-	msgCount   int               // Number of undeleted messages.
+	msgCount   int                  // Number of undeleted messages.
-	logger     zerolog.Logger    // Session specific logger.
+	logger     zerolog.Logger       // Session specific logger.
-	debug      bool              // Print network traffic to stdout.
+	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)
+		tlsState = new(tls.ConnectionState)
-		*s.tlsState = tlsConn.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(".")

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)

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.