Serve per-listener muxes instead of http.DefaultServeMux

metrics/server.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"time"
 
 	"github.com/ava-labs/avalanchego/api/metrics"
 	"github.com/ava-labs/avalanchego/utils/logging"
@@ -13,6 +14,12 @@ import (
 	"go.uber.org/zap"
 )
 
+const (
+	MetricsPath = "/metrics"
+
+	metricsReadHeaderTimeout = 10 * time.Second
+)
+
 // Starts a metrics server on the given port and registers the provided names with the metrics gatherer.
 // Returns a map of registries, keyed by the provided names.
 func StartMetricsServer(logger logging.Logger, port uint16, names []string) (map[string]*prometheus.Registry, error) {
@@ -27,17 +34,26 @@ func StartMetricsServer(logger logging.Logger, port uint16, names []string) (map
 		registries[name] = registry
 	}
 
-	http.Handle(
-		"/metrics",
+	// Serve a dedicated mux rather than http.DefaultServeMux so that the metrics listener
+	// exposes only /metrics, and never the service APIs registered elsewhere in the process.
+	mux := http.NewServeMux()
+	mux.Handle(
+		MetricsPath,
 		promhttp.HandlerFor(gatherer, promhttp.HandlerOpts{}),
 	)
 
+	server := &http.Server{
+		Addr:              fmt.Sprintf(":%d", port),
+		Handler:           mux,
+		ReadHeaderTimeout: metricsReadHeaderTimeout,
+	}
+
 	go func() {
 		logger.Info(
 			"Starting metrics server...",
 			zap.Uint16("port", port),
 		)
-		err := http.ListenAndServe(fmt.Sprintf(":%d", port), nil)
+		err := server.ListenAndServe()
 		if errors.Is(err, http.ErrServerClosed) {
 			logger.Info("Metrics check server closed")
 		} else if err != nil {

metrics/server_test.go

@@ -0,0 +1,52 @@
+package metrics
+
+import (
+	"fmt"
+	"net"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/ava-labs/avalanchego/utils/logging"
+	"github.com/stretchr/testify/require"
+)
+
+// freePort reserves an ephemeral port and releases it so that the metrics server can bind to it.
+func freePort(t *testing.T) uint16 {
+	t.Helper()
+	listener, err := net.Listen("tcp", "127.0.0.1:0")
+	require.NoError(t, err)
+	port := listener.Addr().(*net.TCPAddr).Port
+	require.NoError(t, listener.Close())
+	return uint16(port)
+}
+
+// The metrics listener must serve only /metrics. Anything registered on http.DefaultServeMux
+// elsewhere in the process (e.g. the service API endpoints) must not be reachable on it.
+func TestMetricsServerDoesNotServeDefaultServeMux(t *testing.T) {
+	const sentinelPath = "/sentinel-api-endpoint"
+	http.Handle(sentinelPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	port := freePort(t)
+	_, err := StartMetricsServer(logging.NoLog{}, port, []string{"test"})
+	require.NoError(t, err)
+
+	baseURL := fmt.Sprintf("http://127.0.0.1:%d", port)
+	client := &http.Client{Timeout: 5 * time.Second}
+
+	require.Eventually(t, func() bool {
+		resp, err := client.Get(baseURL + MetricsPath)
+		if err != nil {
+			return false
+		}
+		defer resp.Body.Close()
+		return resp.StatusCode == http.StatusOK
+	}, 10*time.Second, 50*time.Millisecond, "metrics endpoint never became available")
+
+	resp, err := client.Get(baseURL + sentinelPath)
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	require.Equal(t, http.StatusNotFound, resp.StatusCode)
+}

relayer/api/health_check.go

@@ -15,11 +15,12 @@ import (
 const HealthAPIPath = "/health"
 
 func HandleHealthCheck(
+	mux *http.ServeMux,
 	logger logging.Logger,
 	relayerHealth map[ids.ID]*atomic.Bool,
 	networkHealth func(context.Context) error,
 ) {
-	http.Handle(HealthAPIPath, healthCheckHandler(logger, relayerHealth, networkHealth))
+	mux.Handle(HealthAPIPath, healthCheckHandler(logger, relayerHealth, networkHealth))
 }
 
 func healthCheckHandler(

relayer/api/mux_test.go

@@ -0,0 +1,40 @@
+package api
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/ava-labs/avalanchego/utils/logging"
+	"github.com/stretchr/testify/require"
+)
+
+// The relayer API endpoints must be registered on the mux they are given, and that mux must not
+// expose anything registered elsewhere in the process, in particular the /metrics endpoint.
+func TestRelayerAPIRegistrationIsScopedToItsMux(t *testing.T) {
+	mux := http.NewServeMux()
+	HandleRelay(mux, logging.NoLog{}, nil)
+	HandleRelayMessage(mux, logging.NoLog{}, nil)
+
+	// Registering on the default mux must have no effect on the API mux.
+	http.Handle("/relayer-api-test-metrics", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	for _, path := range []string{RelayAPIPath, RelayMessageAPIPath} {
+		// An undecodable body short-circuits before the message coordinator is used,
+		// so a 400 confirms the endpoint is routed on this mux.
+		req := httptest.NewRequest(http.MethodPost, path, strings.NewReader("not json"))
+		rec := httptest.NewRecorder()
+		mux.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusBadRequest, rec.Code, "expected %s to be served by the API mux", path)
+	}
+
+	for _, path := range []string{"/metrics", "/relayer-api-test-metrics"} {
+		req := httptest.NewRequest(http.MethodGet, path, nil)
+		rec := httptest.NewRecorder()
+		mux.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusNotFound, rec.Code, "expected %s to be absent from the API mux", path)
+	}
+}

relayer/api/relay_message.go

@@ -45,12 +45,12 @@ type ManualWarpMessageRequest struct {
 	SourceAddress        string `json:"source-address"`
 }
 
-func HandleRelayMessage(logger logging.Logger, messageCoordinator *relayer.MessageCoordinator) {
-	http.Handle(RelayMessageAPIPath, relayMessageAPIHandler(logger, messageCoordinator))
+func HandleRelayMessage(mux *http.ServeMux, logger logging.Logger, messageCoordinator *relayer.MessageCoordinator) {
+	mux.Handle(RelayMessageAPIPath, relayMessageAPIHandler(logger, messageCoordinator))
 }
 
-func HandleRelay(logger logging.Logger, messageCoordinator *relayer.MessageCoordinator) {
-	http.Handle(RelayAPIPath, relayAPIHandler(logger, messageCoordinator))
+func HandleRelay(mux *http.ServeMux, logger logging.Logger, messageCoordinator *relayer.MessageCoordinator) {
+	mux.Handle(RelayAPIPath, relayAPIHandler(logger, messageCoordinator))
 }
 
 func relayMessageAPIHandler(logger logging.Logger, messageCoordinator *relayer.MessageCoordinator) http.Handler {

relayer/main/main.go

@@ -58,6 +58,8 @@ const (
 	// The size of the FIFO cache for epoched validator sets
 	// The Cache will store validator sets for the most recent N P-Chain heights.
 	validatorSetCacheSize = 100
+
+	apiReadHeaderTimeout = 10 * time.Second
 )
 
 func main() {
@@ -289,14 +291,20 @@ func main() {
 
 	networkHealthFunc := network.GetNetworkHealthFunc(cfg.GetTrackedSubnets().List())
 
+	// The API endpoints are registered on a dedicated mux, rather than http.DefaultServeMux,
+	// so that they are only reachable on the API port and not on the metrics port.
+	apiMux := http.NewServeMux()
+
 	// Each Listener goroutine will have an atomic bool that it can set to false to indicate an unrecoverable error
-	api.HandleHealthCheck(logger, relayerHealth, networkHealthFunc)
-	api.HandleRelay(logger, messageCoordinator)
-	api.HandleRelayMessage(logger, messageCoordinator)
+	api.HandleHealthCheck(apiMux, logger, relayerHealth, networkHealthFunc)
+	api.HandleRelay(apiMux, logger, messageCoordinator)
+	api.HandleRelayMessage(apiMux, logger, messageCoordinator)
 
 	errGroup.Go(func() error {
 		httpServer := &http.Server{
-			Addr: fmt.Sprintf(":%d", cfg.APIPort),
+			Addr:              fmt.Sprintf(":%d", cfg.APIPort),
+			Handler:           apiMux,
+			ReadHeaderTimeout: apiReadHeaderTimeout,
 		}
 		// Handle graceful shutdown
 		go func() {

signature-aggregator/api/api.go

@@ -51,11 +51,12 @@ type AggregateSignatureErrorResponse struct {
 }
 
 func HandleAggregateSignaturesByRawMsgRequest(
+	mux *http.ServeMux,
 	logger logging.Logger,
 	metrics *metrics.SignatureAggregatorMetrics,
 	signatureAggregator *aggregator.SignatureAggregator,
 ) {
-	http.Handle(
+	mux.Handle(
 		APIPath,
 		signatureAggregationAPIHandler(
 			logger,

signature-aggregator/api/mux_test.go

@@ -0,0 +1,44 @@
+package api
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/ava-labs/avalanchego/utils/logging"
+	"github.com/ava-labs/icm-services/signature-aggregator/metrics"
+	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/require"
+)
+
+// The aggregation endpoint must be registered on the mux it is given, and that mux must not
+// expose anything registered elsewhere in the process, in particular the /metrics endpoint.
+func TestAggregateSignaturesRegistrationIsScopedToItsMux(t *testing.T) {
+	mux := http.NewServeMux()
+	HandleAggregateSignaturesByRawMsgRequest(
+		mux,
+		logging.NoLog{},
+		metrics.NewSignatureAggregatorMetrics(prometheus.NewRegistry()),
+		nil,
+	)
+
+	// Registering on the default mux must have no effect on the API mux.
+	http.Handle("/sig-agg-api-test-metrics", http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+
+	// An undecodable body short-circuits before the aggregator is used,
+	// so a 400 confirms the endpoint is routed on this mux.
+	req := httptest.NewRequest(http.MethodPost, APIPath, strings.NewReader("not json"))
+	rec := httptest.NewRecorder()
+	mux.ServeHTTP(rec, req)
+	require.Equal(t, http.StatusBadRequest, rec.Code)
+
+	for _, path := range []string{"/metrics", "/sig-agg-api-test-metrics"} {
+		req := httptest.NewRequest(http.MethodGet, path, nil)
+		rec := httptest.NewRecorder()
+		mux.ServeHTTP(rec, req)
+		require.Equal(t, http.StatusNotFound, rec.Code, "expected %s to be absent from the API mux", path)
+	}
+}

signature-aggregator/healthcheck/healthcheck.go

@@ -7,13 +7,15 @@ import (
 	"github.com/alexliesenfeld/health"
 )
 
-func HandleHealthCheckRequest(checkFunc func(context.Context) error) {
+const HealthAPIPath = "/health"
+
+func HandleHealthCheckRequest(mux *http.ServeMux, checkFunc func(context.Context) error) {
 	healthChecker := health.NewChecker(
 		health.WithCheck(health.Check{
 			Name:  "signature-aggregator-health",
 			Check: checkFunc,
 		}),
 	)
 
-	http.Handle("/health", health.NewHandler(healthChecker))
+	mux.Handle(HealthAPIPath, health.NewHandler(healthChecker))
 }

signature-aggregator/main/main.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/signal"
 	"syscall"
+	"time"
 
 	"github.com/ava-labs/avalanchego/api/info"
 	"github.com/ava-labs/avalanchego/graft/subnet-evm/plugin/evm"
@@ -41,6 +42,8 @@ const (
 	// The size of the FIFO cache for epoched validator sets
 	// The Cache will store validator sets for the most recent N P-Chain heights.
 	validatorSetCacheSize = 750
+
+	apiReadHeaderTimeout = 10 * time.Second
 )
 
 func main() {
@@ -174,7 +177,12 @@ func main() {
 		os.Exit(1)
 	}
 
+	// The API endpoints are registered on a dedicated mux, rather than http.DefaultServeMux,
+	// so that they are only reachable on the API port and not on the metrics port.
+	apiMux := http.NewServeMux()
+
 	api.HandleAggregateSignaturesByRawMsgRequest(
+		apiMux,
 		logger,
 		metricsInstance,
 		signatureAggregator,
@@ -183,11 +191,13 @@ func main() {
 	healthCheckSubnets := cfg.GetTrackedSubnets().List()
 	healthCheckSubnets = append(healthCheckSubnets, constants.PrimaryNetworkID)
 	networkHealthcheckFunc := network.GetNetworkHealthFunc(healthCheckSubnets)
-	healthcheck.HandleHealthCheckRequest(networkHealthcheckFunc)
+	healthcheck.HandleHealthCheckRequest(apiMux, networkHealthcheckFunc)
 
 	errGroup.Go(func() error {
 		httpServer := &http.Server{
-			Addr: fmt.Sprintf(":%d", cfg.APIPort),
+			Addr:              fmt.Sprintf(":%d", cfg.APIPort),
+			Handler:           apiMux,
+			ReadHeaderTimeout: apiReadHeaderTimeout,
 		}
 		// Handle graceful shutdown
 		go func() {