monitoring: Use method-based routing and CSRF for http requests

The monitoring http server is mostly read-only but shows sensitive data,
and also has one mutating endpoint, /exit.

This patch updates the routing to use method-based routing, and also
wraps it on CSRF to reject cross-origin requests from browsers.

Note that does not affect direct POST /exit requests via tools like
curl or wget, which continue to work just like today. This is exercised
in the (unchanged) integration tests.

internal/smtpsrv/server.go

@@ -237,7 +237,7 @@ func (s *Server) InitQueue(path string, localC, remoteC courier.Courier) {
 	}
 	s.queue = q
 
-	http.HandleFunc("/debug/queue",
+	http.HandleFunc("GET /debug/queue",
 		func(w http.ResponseWriter, r *http.Request) {
 			_, _ = w.Write([]byte(q.DumpString()))
 		})

monitoring.go

@@ -91,6 +91,24 @@ func parseVersionInfo() {
 func launchMonitoringServer(conf *config.Config) {
 	log.Infof("Monitoring HTTP server listening on %s", conf.MonitoringAddress)
 
+	srv := &http.Server{
+		Addr:              conf.MonitoringAddress,
+		ReadHeaderTimeout: 10 * time.Second,
+	}
+
+	// Use the default mux, as that's where the expvar and pprof handlers
+	// register themselves.
+	srv.Handler = monitoringHandler(http.DefaultServeMux, srv, conf)
+
+	if err := srv.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) {
+		log.Fatalf("Monitoring server failed: %v", err)
+	}
+}
+
+// monitoringHandler registers the monitoring handlers in the given mux, and
+// returns the handler to use for the monitoring server.
+func monitoringHandler(mux *http.ServeMux, srv *http.Server,
+	conf *config.Config) http.Handler {
 	osHostname, _ := os.Hostname()
 
 	indexData := struct {
@@ -109,27 +127,23 @@ func launchMonitoringServer(conf *config.Config) {
 		Hostname:   osHostname,
 	}
 
-	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+	mux.HandleFunc("GET /{$}", func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/" {
-			http.NotFound(w, r)
-			return
-		}
 		if err := monitoringHTMLIndex.Execute(w, indexData); err != nil {
 			log.Infof("monitoring handler error: %v", err)
 		}
 	})
 
-	srv := &http.Server{Addr: conf.MonitoringAddress}
+	mux.HandleFunc("POST /exit", exitHandler(srv))
+	mux.HandleFunc("GET /metrics", expvarom.MetricsHandler)
+	mux.HandleFunc("GET /debug/flags", debugFlagsHandler)
+	mux.HandleFunc("GET /debug/config", debugConfigHandler(conf))
+	mux.HandleFunc("GET /debug/traces", nettrace.RenderTraces)
 
-	http.HandleFunc("/exit", exitHandler(srv))
+	// Reject cross-origin requests from browsers, except for safe methods
-	http.HandleFunc("/metrics", expvarom.MetricsHandler)
+	// (like GET). This prevents cross-site request forgery: otherwise, any
-	http.HandleFunc("/debug/flags", debugFlagsHandler)
+	// web page could make the browser of someone who can reach the monitoring
-	http.HandleFunc("/debug/config", debugConfigHandler(conf))
+	// server send it a POST /exit.
-	http.HandleFunc("/debug/traces", nettrace.RenderTraces)
+	return http.NewCrossOriginProtection().Handler(mux)
-
-	if err := srv.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) {
-		log.Fatalf("Monitoring server failed: %v", err)
-	}
 }
 
 // Functions available inside the templates.
@@ -207,11 +221,6 @@ os hostname <i>{{.Hostname}}</i><br>
 
 func exitHandler(srv *http.Server) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "POST" {
-			http.Error(w, "Use POST method for exiting", http.StatusMethodNotAllowed)
-			return
-		}
-
 		log.Infof("Received /exit")
 		http.Error(w, "OK exiting", http.StatusOK)
 

monitoring_test.go

@@ -0,0 +1,55 @@
+package main
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"blitiri.com.ar/go/chasquid/internal/config"
+)
+
+func TestMonitoringHandler(t *testing.T) {
+	conf := &config.Config{Hostname: "testhost"}
+	h := monitoringHandler(http.NewServeMux(), &http.Server{}, conf)
+
+	cases := []struct {
+		method string
+		path   string
+		header map[string]string
+		want   int
+	}{
+		{"GET", "/", nil, http.StatusOK},
+		{"HEAD", "/", nil, http.StatusOK},
+		{"POST", "/", nil, http.StatusMethodNotAllowed},
+		{"GET", "/doesnotexist", nil, http.StatusNotFound},
+		{"GET", "/metrics", nil, http.StatusOK},
+		{"GET", "/debug/flags", nil, http.StatusOK},
+		{"GET", "/debug/config", nil, http.StatusOK},
+		{"POST", "/debug/config", nil, http.StatusMethodNotAllowed},
+		{"GET", "/debug/traces", nil, http.StatusOK},
+
+		// /exit is only allowed via POST.
+		{"GET", "/exit", nil, http.StatusMethodNotAllowed},
+
+		// Cross-origin requests from browsers must be rejected, to prevent
+		// CSRF. They must not reach the handler, which would exit.
+		{"POST", "/exit",
+			map[string]string{"Sec-Fetch-Site": "cross-site"},
+			http.StatusForbidden},
+		{"POST", "/exit",
+			map[string]string{"Origin": "https://evil.example"},
+			http.StatusForbidden},
+	}
+	for _, c := range cases {
+		req := httptest.NewRequest(c.method, c.path, nil)
+		for k, v := range c.header {
+			req.Header.Set(k, v)
+		}
+		rec := httptest.NewRecorder()
+		h.ServeHTTP(rec, req)
+		if rec.Code != c.want {
+			t.Errorf("%s %s %v: got status %d, expected %d",
+				c.method, c.path, c.header, rec.Code, c.want)
+		}
+	}
+}