From f50630ed4bc18fc65652b42617eb7907598041e6 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 13 Oct 2026 13:28:55 +1000 Subject: [PATCH] fix(spinifexui): stop silent connections blocking the TLS split listener Accept read the protocol-detection byte inline with a 5s deadline, so each client that sent nothing stalled every new visitor. Accept now returns the TLS-wrapped conn immediately and the first byte is sniffed on the first handshake Read, in the connection's own goroutine. --- spinifex/services/spinifexui/tlssplit.go | 80 ++++++------- spinifex/services/spinifexui/tlssplit_test.go | 111 +++++++++++------- 2 files changed, 103 insertions(+), 88 deletions(-) diff --git a/spinifex/services/spinifexui/tlssplit.go b/spinifex/services/spinifexui/tlssplit.go index 89c262488..218561831 100644 --- a/spinifex/services/spinifexui/tlssplit.go +++ b/spinifex/services/spinifexui/tlssplit.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "crypto/tls" + "errors" "io" "net" "net/http" @@ -14,9 +15,12 @@ import ( const tlsRecordTypeHandshake = 0x16 -// tlsSplitListener accepts connections and reads the first byte. If it looks -// like a TLS ClientHello (0x16), the connection is wrapped with TLS. Otherwise, -// a plain-HTTP redirect to HTTPS is sent and the connection is closed. +// errPlainHTTP fails the TLS handshake after a plain-HTTP client was redirected. +var errPlainHTTP = errors.New("plain HTTP redirected to HTTPS") + +// tlsSplitListener serves TLS and plain HTTP on one port. Accept never reads: +// the first byte is sniffed during the handshake, in the connection's own +// goroutine, so a client that sends nothing cannot stall other clients. type tlsSplitListener struct { net.Listener @@ -25,37 +29,36 @@ type tlsSplitListener struct { } func (ln *tlsSplitListener) Accept() (net.Conn, error) { - for { - conn, err := ln.Listener.Accept() - if err != nil { - return nil, err - } - - // Set a deadline for the initial protocol detection byte so a - // slow/malicious client cannot block the accept loop. - _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) - - var buf [1]byte - if _, err := io.ReadFull(conn, buf[:]); err != nil { - _ = conn.Close() - continue - } - - // Clear the deadline before handing off the connection. - _ = conn.SetReadDeadline(time.Time{}) - - if buf[0] == tlsRecordTypeHandshake { - // Prepend the peeked byte and upgrade to TLS. - merged := &prefixConn{ - Conn: conn, - r: io.MultiReader(bytes.NewReader(buf[:]), conn), - } - return tls.Server(merged, ln.tlsCfg), nil - } - - // Plain HTTP — send redirect and close. - go ln.redirectHTTP(conn, buf[0]) + conn, err := ln.Listener.Accept() + if err != nil { + return nil, err } + return tls.Server(&sniffConn{Conn: conn, ln: ln}, ln.tlsCfg), nil +} + +// sniffConn reads the first byte on its first Read. A TLS ClientHello (0x16) +// is replayed to the TLS layer; anything else gets an HTTPS redirect. Reads +// are serialised by tls.Conn and bounded by http.Server's handshake timeout. +type sniffConn struct { + net.Conn + + ln *tlsSplitListener + r io.Reader +} + +func (c *sniffConn) Read(b []byte) (int, error) { + if c.r == nil { + var buf [1]byte + if _, err := io.ReadFull(c.Conn, buf[:]); err != nil { + return 0, err + } + if buf[0] != tlsRecordTypeHandshake { + c.ln.redirectHTTP(c.Conn, buf[0]) + return 0, errPlainHTTP + } + c.r = io.MultiReader(bytes.NewReader(buf[:]), c.Conn) + } + return c.r.Read(b) } // redirectHTTP reads an HTTP request from conn (with the first byte already @@ -91,14 +94,3 @@ func (ln *tlsSplitListener) redirectHTTP(conn net.Conn, firstByte byte) { } _ = resp.Write(conn) } - -// prefixConn wraps a net.Conn with a reader that replays prefixed bytes. -type prefixConn struct { - net.Conn - - r io.Reader -} - -func (c *prefixConn) Read(b []byte) (int, error) { - return c.r.Read(b) -} diff --git a/spinifex/services/spinifexui/tlssplit_test.go b/spinifex/services/spinifexui/tlssplit_test.go index d841e3aab..6bdc879b9 100644 --- a/spinifex/services/spinifexui/tlssplit_test.go +++ b/spinifex/services/spinifexui/tlssplit_test.go @@ -2,7 +2,6 @@ package spinifexui import ( "bufio" - "bytes" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -21,30 +20,9 @@ import ( "github.com/stretchr/testify/require" ) -func TestPrefixConn_Read_ReplaysPrefixThenStream(t *testing.T) { - server, client := net.Pipe() - defer server.Close() - defer client.Close() - - go func() { - _, _ = client.Write([]byte("est")) - }() - - pc := &prefixConn{ - Conn: server, - r: io.MultiReader(bytes.NewReader([]byte{'t'}), server), - } - - buf := make([]byte, 4) - n, err := io.ReadFull(pc, buf) - require.NoError(t, err) - assert.Equal(t, 4, n) - assert.Equal(t, "test", string(buf)) -} - // startSplitListener returns a tlsSplitListener on a random local port. Caller // should close the returned net.Listener when done. The Accept loop runs in a -// goroutine and accepted TLS connections are pushed onto the returned channel. +// goroutine and accepted connections are pushed onto the returned channel. func startSplitListener(t *testing.T, tlsCfg *tls.Config, redirectPort int) (*tlsSplitListener, net.Listener, <-chan net.Conn) { t.Helper() ln, err := net.Listen("tcp", "127.0.0.1:0") @@ -56,7 +34,7 @@ func startSplitListener(t *testing.T, tlsCfg *tls.Config, redirectPort int) (*tl tlsCfg: tlsCfg, } - accepted := make(chan net.Conn, 1) + accepted := make(chan net.Conn, 16) go func() { defer close(accepted) for { @@ -71,6 +49,19 @@ func startSplitListener(t *testing.T, tlsCfg *tls.Config, redirectPort int) (*tl return splitLn, ln, accepted } +// handshakeAll runs the TLS handshake on each accepted connection, as +// http.Server does, which is where the first byte is sniffed. +func handshakeAll(accepted <-chan net.Conn) { + go func() { + for c := range accepted { + go func() { + defer c.Close() + _ = c.(*tls.Conn).Handshake() + }() + } + }() +} + func TestTLSSplitListener_Accept_TLSHandshakeByteWrapsTLS(t *testing.T) { cert := loadTLSCert(t) tlsCfg := &tls.Config{ @@ -102,8 +93,9 @@ func TestTLSSplitListener_Accept_TLSHandshakeByteWrapsTLS(t *testing.T) { } func TestTLSSplitListener_Accept_PlainHTTPRedirectsToHTTPS(t *testing.T) { - _, ln, _ := startSplitListener(t, &tls.Config{}, 8443) + _, ln, accepted := startSplitListener(t, &tls.Config{}, 8443) defer ln.Close() + handshakeAll(accepted) conn, err := net.Dial("tcp", ln.Addr().String()) require.NoError(t, err) @@ -122,8 +114,9 @@ func TestTLSSplitListener_Accept_PlainHTTPRedirectsToHTTPS(t *testing.T) { } func TestTLSSplitListener_Accept_PlainHTTPStripsHostPort(t *testing.T) { - _, ln, _ := startSplitListener(t, &tls.Config{}, 9443) + _, ln, accepted := startSplitListener(t, &tls.Config{}, 9443) defer ln.Close() + handshakeAll(accepted) conn, err := net.Dial("tcp", ln.Addr().String()) require.NoError(t, err) @@ -142,8 +135,9 @@ func TestTLSSplitListener_Accept_PlainHTTPStripsHostPort(t *testing.T) { } func TestTLSSplitListener_Accept_MalformedHTTPClosesSilently(t *testing.T) { - _, ln, _ := startSplitListener(t, &tls.Config{}, 8443) + _, ln, accepted := startSplitListener(t, &tls.Config{}, 8443) defer ln.Close() + handshakeAll(accepted) conn, err := net.Dial("tcp", ln.Addr().String()) require.NoError(t, err) @@ -160,7 +154,7 @@ func TestTLSSplitListener_Accept_MalformedHTTPClosesSilently(t *testing.T) { assert.Equal(t, 0, n, "no response expected for malformed request") } -func TestTLSSplitListener_Accept_ClientClosesBeforeFirstByte(t *testing.T) { +func TestTLSSplitListener_TLSClientCompletesHandshake(t *testing.T) { cert := loadTLSCert(t) tlsCfg := &tls.Config{ Certificates: []tls.Certificate{cert}, @@ -170,27 +164,56 @@ func TestTLSSplitListener_Accept_ClientClosesBeforeFirstByte(t *testing.T) { _, ln, accepted := startSplitListener(t, tlsCfg, 8443) defer ln.Close() - // Dial and immediately close — listener should drop the conn and keep looping. + errc := make(chan error, 1) + go func() { + c, err := tls.Dial("tcp", ln.Addr().String(), &tls.Config{InsecureSkipVerify: true}) + if err == nil { + _ = c.Close() + } + errc <- err + }() + + got := <-accepted + defer got.Close() + require.NoError(t, got.SetDeadline(time.Now().Add(3*time.Second))) + require.NoError(t, got.(*tls.Conn).Handshake(), "sniffed first byte must be replayed to the TLS layer") + require.NoError(t, <-errc) +} + +func TestTLSSplitListener_ClientClosesBeforeFirstByteFailsHandshake(t *testing.T) { + _, ln, accepted := startSplitListener(t, &tls.Config{}, 8443) + defer ln.Close() + c, err := net.Dial("tcp", ln.Addr().String()) require.NoError(t, err) require.NoError(t, c.Close()) - // Now send a valid TLS first byte from a fresh connection to verify the - // loop still serves subsequent clients. - c2, err := net.Dial("tcp", ln.Addr().String()) - require.NoError(t, err) - defer c2.Close() - _, err = c2.Write([]byte{0x16}) - require.NoError(t, err) + got := <-accepted + defer got.Close() + require.NoError(t, got.SetDeadline(time.Now().Add(3*time.Second))) + assert.ErrorIs(t, got.(*tls.Conn).Handshake(), io.EOF) +} - select { - case got := <-accepted: - require.NotNil(t, got) - defer got.Close() - _, ok := got.(*tls.Conn) - assert.True(t, ok) - case <-time.After(2 * time.Second): - t.Fatal("Accept loop did not recover after client disconnect") +func TestTLSSplitListener_Accept_SilentClientsDoNotDelayOthers(t *testing.T) { + _, ln, accepted := startSplitListener(t, &tls.Config{}, 8443) + defer ln.Close() + + // Connections that never send a byte, e.g. a port scanner or `nc host port`. + const clients = 4 + for range clients { + idle, err := net.Dial("tcp", ln.Addr().String()) + require.NoError(t, err) + defer idle.Close() + } + + deadline := time.After(time.Second) + for i := range clients { + select { + case got := <-accepted: + defer got.Close() + case <-deadline: + t.Fatalf("Accept returned %d of %d silent connections; it must not read from them", i, clients) + } } }