fix(api): bound how long a client may take to send request headers

pkg/api/server/server.go

@@ -57,9 +57,42 @@ const (
 	UnlimitedServiceDuration = 0 * time.Second
 )
 
+// DefaultReadHeaderTimeout is how long a client may take to send the headers of
+// a request before the service closes the connection.  Unlike ReadTimeout it
+// only covers the headers, so it is safe for the endpoints that stream for an
+// unbounded amount of time: net/http drops the deadline as soon as the headers
+// have been read, and again when a handler hijacks the connection.
+const DefaultReadHeaderTimeout = 30 * time.Second
+
 // shutdownOnce ensures Shutdown() may safely be called from several go routines
 var shutdownOnce sync.Once
 
+// newHTTPServer configures the http.Server used to serve the API.
+//
+// Note that neither ReadTimeout nor WriteTimeout are set: attach, exec, events,
+// logs and image push/pull all stream for as long as the client wants them to,
+// and both of those deadlines cover the whole request.  readHeaderTimeout is the
+// one deadline that can be applied without breaking those endpoints.
+func newHTTPServer(handler http.Handler, tracker *idle.Tracker, readHeaderTimeout time.Duration) http.Server {
+	protocols := &http.Protocols{}
+	protocols.SetHTTP1(true)
+	protocols.SetHTTP2(true)
+
+	return http.Server{
+		ConnContext: func(ctx context.Context, c net.Conn) context.Context {
+			return context.WithValue(ctx, types.ConnKey, c)
+		},
+		ConnState: tracker.ConnState,
+		ErrorLog:  log.New(logrus.StandardLogger().Out, "", 0),
+		Handler:   handler,
+		// The http idle connection timeout is 2x the API idle window, see
+		// idle.NewTracker().
+		IdleTimeout:       tracker.Duration * 2,
+		ReadHeaderTimeout: readHeaderTimeout,
+		Protocols:         protocols,
+	}
+}
+
 // NewServerWithSettings will create and configure a new API server using provided settings
 func NewServerWithSettings(runtime *libpod.Runtime, listener net.Listener, opts entities.ServiceOptions) (*APIServer, error) {
 	return newServer(runtime, listener, opts)
@@ -76,21 +109,8 @@ func newServer(runtime *libpod.Runtime, listener net.Listener, opts entities.Ser
 	router := mux.NewRouter().UseEncodedPath()
 	tracker := idle.NewTracker(opts.Timeout)
 
-	serverProtocols := &http.Protocols{}
-	serverProtocols.SetHTTP1(true)
-	serverProtocols.SetHTTP2(true)
-
 	server := APIServer{
-		Server: http.Server{
-			ConnContext: func(ctx context.Context, c net.Conn) context.Context {
-				return context.WithValue(ctx, types.ConnKey, c)
-			},
-			ConnState:   tracker.ConnState,
-			ErrorLog:    log.New(logrus.StandardLogger().Out, "", 0),
-			Handler:     router,
-			IdleTimeout: opts.Timeout * 2,
-			Protocols:   serverProtocols,
-		},
+		Server:          newHTTPServer(router, tracker, DefaultReadHeaderTimeout),
 		grpc:            grpc.NewServer(),
 		CorsHeaders:     opts.CorsHeaders,
 		Listener:        listener,

pkg/api/server/server_test.go

@@ -0,0 +1,131 @@
+//go:build !remote && (linux || freebsd)
+
+package server
+
+import (
+	"bufio"
+	"errors"
+	"io"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.podman.io/podman/v6/pkg/api/server/idle"
+)
+
+// serveTest starts the API http.Server on a loopback listener and returns its
+// address.
+func serveTest(t *testing.T, handler http.Handler, readHeaderTimeout time.Duration) string {
+	t.Helper()
+
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+
+	server := newHTTPServer(handler, idle.NewTracker(time.Minute), readHeaderTimeout)
+	go func() {
+		_ = server.Serve(listener)
+	}()
+	t.Cleanup(func() {
+		_ = server.Close()
+	})
+
+	return listener.Addr().String()
+}
+
+// stallMidHeader sends a request header block that is never terminated and
+// reports whether the service closed the connection within timeout.
+func stallMidHeader(t *testing.T, addr string, timeout time.Duration) bool {
+	t.Helper()
+
+	conn, err := net.Dial("tcp", addr)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = conn.Close()
+	})
+
+	// Note the missing final CRLF, the client never finishes the headers.
+	_, err = io.WriteString(conn, "GET /v5.0.0/libpod/_ping HTTP/1.1\r\nHost: podman\r\n")
+	require.NoError(t, err)
+
+	require.NoError(t, conn.SetReadDeadline(time.Now().Add(timeout)))
+	_, err = conn.Read(make([]byte, 1))
+
+	var netErr net.Error
+	if errors.As(err, &netErr) && netErr.Timeout() {
+		// Our own deadline expired, the service is still holding the connection.
+		return false
+	}
+	return err != nil
+}
+
+func TestNewHTTPServerReadHeaderTimeout(t *testing.T) {
+	assert.NotZero(t, DefaultReadHeaderTimeout,
+		"the service must bound how long a client may take to send its headers")
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	})
+
+	t.Run("closes a connection stalled mid-header", func(t *testing.T) {
+		addr := serveTest(t, handler, 250*time.Millisecond)
+		assert.True(t, stallMidHeader(t, addr, 30*time.Second),
+			"connection stalled mid-header was not closed")
+	})
+
+	t.Run("without a timeout the connection is held forever", func(t *testing.T) {
+		// Guards the regression: with ReadHeaderTimeout unset a client can pin a
+		// file descriptor and a goroutine per connection for the life of the
+		// service.
+		addr := serveTest(t, handler, 0)
+		assert.False(t, stallMidHeader(t, addr, time.Second),
+			"connection was closed although no ReadHeaderTimeout was configured")
+	})
+}
+
+// TestNewHTTPServerStreamsAfterHeaderTimeout covers the endpoints that hijack
+// the connection, such as attach and exec: once the headers have been read the
+// deadline no longer applies and the connection may stream indefinitely.
+func TestNewHTTPServerStreamsAfterHeaderTimeout(t *testing.T) {
+	const readHeaderTimeout = 250 * time.Millisecond
+	const payload = "streamed after the header timeout\n"
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		conn, buf, err := w.(http.Hijacker).Hijack()
+		if !assert.NoError(t, err) {
+			return
+		}
+		defer conn.Close()
+
+		_, _ = buf.WriteString("HTTP/1.1 200 OK\r\n\r\n")
+		assert.NoError(t, buf.Flush())
+
+		// Sit idle well past the header timeout before streaming, the way an
+		// attached container that produces no output does.
+		time.Sleep(4 * readHeaderTimeout)
+
+		_, _ = buf.WriteString(payload)
+		assert.NoError(t, buf.Flush())
+	})
+
+	addr := serveTest(t, handler, readHeaderTimeout)
+
+	conn, err := net.Dial("tcp", addr)
+	require.NoError(t, err)
+	defer conn.Close()
+
+	_, err = io.WriteString(conn, "GET /v5.0.0/libpod/containers/ctr/attach HTTP/1.1\r\nHost: podman\r\n\r\n")
+	require.NoError(t, err)
+	require.NoError(t, conn.SetReadDeadline(time.Now().Add(30*time.Second)))
+
+	reader := bufio.NewReader(conn)
+	for {
+		line, err := reader.ReadString('\n')
+		require.NoError(t, err, "hijacked connection was closed while streaming")
+		if line == payload {
+			return
+		}
+	}
+}