Feature 03 task 5: Trusted proxy handling for forwarded headers (#23)

Co-authored-by: dev_agent <dev_agent@cubecraftlabs.com>
Co-committed-by: dev_agent <dev_agent@cubecraftlabs.com>

internal/server/login_test.go

@@ -0,0 +1,140 @@
+package server
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+	"testing/fstest"
+	"time"
+
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations"
+)
+
+// newDiscoveryProvider serves the OIDC discovery document auth.New needs;
+// login only redirects to its authorization endpoint.
+func newDiscoveryProvider(t *testing.T) *httptest.Server {
+	t.Helper()
+	var srv *httptest.Server
+	srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/.well-known/openid-configuration" {
+			http.NotFound(w, r)
+			return
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]any{
+			"issuer":                                srv.URL,
+			"authorization_endpoint":                srv.URL + "/authorize",
+			"token_endpoint":                        srv.URL + "/token",
+			"jwks_uri":                              srv.URL + "/jwks",
+			"userinfo_endpoint":                     srv.URL + "/userinfo",
+			"end_session_endpoint":                  srv.URL + "/end-session/",
+			"response_types_supported":              []string{"code"},
+			"subject_types_supported":               []string{"public"},
+			"id_token_signing_alg_values_supported": []string{"RS256"},
+		})
+	}))
+	t.Cleanup(srv.Close)
+	return srv
+}
+
+// newLoginRouter wires the production router as cmd/server does: the real
+// auth flow and session manager, trusting Traefik only.
+func newLoginRouter(t *testing.T, issuer string) http.Handler {
+	t.Helper()
+	ctx := context.Background()
+	db, err := store.Open(ctx, t.TempDir())
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := db.Close(); err != nil {
+			t.Error(err)
+		}
+	})
+	if err := store.Migrate(ctx, db.Writer, migrations.FS); err != nil {
+		t.Fatal(err)
+	}
+	sessions, err := auth.NewSessionManager(db, 720*time.Hour, 4320*time.Hour)
+	if err != nil {
+		t.Fatal(err)
+	}
+	logger := slog.New(forwarded.LogHandler(slog.NewTextHandler(io.Discard, nil)))
+	flow, err := auth.New(ctx, auth.Config{
+		Issuer:        issuer,
+		ClientID:      "mealquest",
+		ClientSecret:  "client-s3cret",
+		PublicURL:     &url.URL{Scheme: "https", Host: "meals.example.com"},
+		SecretKey:     []byte("server-secret-key"),
+		AllowedGroups: []string{"mealquest-admin"},
+	}, sessions, logger)
+	if err != nil {
+		t.Fatal(err)
+	}
+	trusted, err := forwarded.ParseTrusted(traefik)
+	if err != nil {
+		t.Fatal(err)
+	}
+	r, err := NewRouter(logger, "dev", trusted, flow, sessions,
+		fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return r
+}
+
+// TestLoginThroughRouter starts a login through the production router. The
+// redirect URI and the Secure login cookie come from MEALQUEST_PUBLIC_URL
+// whatever the direct peer: forwarded headers from an untrusted peer are
+// ignored but never fail sign-in.
+func TestLoginThroughRouter(t *testing.T) {
+	t.Parallel()
+	provider := newDiscoveryProvider(t)
+	router := newLoginRouter(t, provider.URL)
+	// Traefik reaches the service over plain http on its internal address;
+	// the host headers must not leak into the redirect URI.
+	headers := map[string]string{
+		forwarded.HeaderFor:   "203.0.113.7",
+		forwarded.HeaderProto: "https",
+		"X-Forwarded-Host":    "evil.example.com",
+	}
+	tests := []struct {
+		name string
+		peer string
+	}{
+		{"trusted proxy", traefik + ":41000"},
+		{"untrusted peer", "172.17.0.1:5000"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			rec := httptest.NewRecorder()
+			router.ServeHTTP(rec, originRequest("http://mealquest:8080"+auth.LoginPath, tt.peer, headers))
+			resp := rec.Result()
+			if resp.StatusCode != http.StatusFound {
+				t.Fatalf("login status = %d, want 302", resp.StatusCode)
+			}
+			loc, err := url.Parse(resp.Header.Get("Location"))
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got, want := loc.Scheme+"://"+loc.Host+loc.Path, provider.URL+"/authorize"; got != want {
+				t.Errorf("redirect to %s, want %s", got, want)
+			}
+			if got, want := loc.Query().Get("redirect_uri"), "https://meals.example.com/auth/callback"; got != want {
+				t.Errorf("redirect_uri = %q, want %q from the public URL", got, want)
+			}
+			cookies := resp.Cookies()
+			if len(cookies) != 1 || cookies[0].Name != "__Host-mealquest_login" || !cookies[0].Secure || !cookies[0].HttpOnly {
+				t.Errorf("login cookies = %v, want one Secure, HttpOnly __Host-mealquest_login", cookies)
+			}
+		})
+	}
+}

internal/server/origin.go

@@ -0,0 +1,34 @@
+package server
+
+import (
+	"errors"
+	"log/slog"
+	"net/http"
+
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
+)
+
+// resolveOrigin resolves every request's client address and scheme with
+// forwarded.Resolve and puts them in the context (read them with
+// forwarded.ClientIP and forwarded.Scheme). Forwarded headers are honoured
+// only from a peer in trusted. A malformed forwarded header from a trusted
+// peer gets 400 bad_request.
+func resolveOrigin(logger *slog.Logger, trusted forwarded.Trusted) func(http.Handler) http.Handler {
+	badRequest := apiErrorHandler(logger, http.StatusBadRequest, "bad_request", "invalid forwarded headers")
+	internal := apiErrorHandler(logger, http.StatusInternalServerError, "internal", "internal error")
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			o, err := forwarded.Resolve(r, trusted)
+			switch {
+			case errors.Is(err, forwarded.ErrInvalidHeader):
+				logger.WarnContext(r.Context(), "rejected forwarded headers", "peer", r.RemoteAddr, "error", err)
+				badRequest(w, r)
+			case err != nil:
+				logger.ErrorContext(r.Context(), "resolve request origin", "error", err)
+				internal(w, r)
+			default:
+				next.ServeHTTP(w, r.WithContext(forwarded.NewContext(r.Context(), o)))
+			}
+		})
+	}
+}

internal/server/origin_test.go

@@ -0,0 +1,145 @@
+package server
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"testing/fstest"
+
+	"github.com/go-chi/chi/v5"
+
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
+)
+
+// traefik is the production proxy's address.
+const traefik = "10.60.1.33"
+
+// probeAuth mounts GET /auth/probe, which reports the request's resolved
+// origin, in place of the real auth flow.
+type probeAuth struct{}
+
+func (probeAuth) Mount(r chi.Router) {
+	r.Get("/auth/probe", func(w http.ResponseWriter, r *http.Request) {
+		ip, ipOK := forwarded.ClientIP(r.Context())
+		scheme, schemeOK := forwarded.Scheme(r.Context())
+		if !ipOK || !schemeOK {
+			http.Error(w, "origin not resolved", http.StatusInternalServerError)
+			return
+		}
+		fmt.Fprintf(w, "%s %s", ip, scheme)
+	})
+}
+
+// newOriginRouter builds the production router trusting Traefik, logging
+// through forwarded.LogHandler as cmd/server does.
+func newOriginRouter(t *testing.T, logs io.Writer) http.Handler {
+	t.Helper()
+	trusted, err := forwarded.ParseTrusted(traefik)
+	if err != nil {
+		t.Fatal(err)
+	}
+	logger := slog.New(forwarded.LogHandler(slog.NewJSONHandler(logs, nil)))
+	r, err := NewRouter(logger, "dev", trusted, probeAuth{}, stubSessions{},
+		fstest.MapFS{"index.html": {Data: []byte(fixtureIndex)}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return r
+}
+
+func originRequest(target, peer string, headers map[string]string) *http.Request {
+	req := httptest.NewRequest(http.MethodGet, target, nil)
+	req.RemoteAddr = peer
+	for k, v := range headers {
+		req.Header.Set(k, v)
+	}
+	return req
+}
+
+func TestRouterResolvesOrigin(t *testing.T) {
+	t.Parallel()
+	spoofed := map[string]string{forwarded.HeaderFor: "203.0.113.7", forwarded.HeaderProto: "https"}
+	tests := []struct {
+		name string
+		peer string
+		want string
+	}{
+		{"trusted proxy's forwarded headers are honoured", traefik + ":41000", "203.0.113.7 https"},
+		{"untrusted peer spoofing them is ignored", "198.51.100.9:5000", "198.51.100.9 http"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			rec := httptest.NewRecorder()
+			newOriginRouter(t, io.Discard).ServeHTTP(rec, originRequest("/auth/probe", tt.peer, spoofed))
+			if rec.Code != http.StatusOK || rec.Body.String() != tt.want {
+				t.Errorf("probe = %d %q, want 200 %q", rec.Code, rec.Body.String(), tt.want)
+			}
+		})
+	}
+}
+
+func TestRouterLogsResolvedClientIP(t *testing.T) {
+	t.Parallel()
+	spoofed := map[string]string{forwarded.HeaderFor: "203.0.113.7", forwarded.HeaderProto: "https"}
+	tests := []struct {
+		name string
+		peer string
+		want string
+	}{
+		{"trusted proxy", traefik + ":41000", "203.0.113.7"},
+		{"untrusted peer spoofing", "198.51.100.9:5000", "198.51.100.9"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			var logs bytes.Buffer
+			// A failed session lookup is logged with the request's context.
+			req := originRequest("/api/v1/me", tt.peer, spoofed)
+			req.AddCookie(brokenSessionCookie)
+			rec := httptest.NewRecorder()
+			newOriginRouter(t, &logs).ServeHTTP(rec, req)
+			if rec.Code != http.StatusInternalServerError {
+				t.Fatalf("status = %d, want 500", rec.Code)
+			}
+			var line map[string]any
+			if err := json.Unmarshal(logs.Bytes(), &line); err != nil {
+				t.Fatalf("log %q: %v", logs.String(), err)
+			}
+			if line["msg"] != "look up session" || line[forwarded.LogAttr] != tt.want {
+				t.Errorf("log = %v, want the session error with %s=%s", line, forwarded.LogAttr, tt.want)
+			}
+		})
+	}
+}
+
+func TestRouterRejectsMalformedForwardedHeaders(t *testing.T) {
+	t.Parallel()
+	const badRequestJSON = `{"error":{"code":"bad_request","message":"invalid forwarded headers","retryable":false}}`
+	for _, headers := range []map[string]string{
+		{forwarded.HeaderFor: "not-an-ip"},
+		{forwarded.HeaderProto: "ftp"},
+	} {
+		var logs bytes.Buffer
+		rec := httptest.NewRecorder()
+		newOriginRouter(t, &logs).ServeHTTP(rec, originRequest("/health", traefik+":41000", headers))
+		if rec.Code != http.StatusBadRequest || rec.Body.String() != badRequestJSON {
+			t.Errorf("%v: response = %d %s, want 400 %s", headers, rec.Code, rec.Body.String(), badRequestJSON)
+		}
+		if !bytes.Contains(logs.Bytes(), []byte("rejected forwarded headers")) {
+			t.Errorf("%v: logs = %q, want the rejection logged", headers, logs.String())
+		}
+
+		// The same headers from an untrusted peer are not examined at all.
+		rec = httptest.NewRecorder()
+		newOriginRouter(t, io.Discard).ServeHTTP(rec, originRequest("/health", "198.51.100.9:5000", headers))
+		if rec.Code != http.StatusOK {
+			t.Errorf("%v from an untrusted peer: status = %d, want 200", headers, rec.Code)
+		}
+	}
+}

internal/server/server.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/go-chi/chi/v5"
 
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/frontend"
 )
 
@@ -21,12 +22,14 @@ type RouteMounter interface {
 	Mount(r chi.Router)
 }
 
-// NewRouter returns the HTTP handler for the service. version is reported by
+// NewRouter returns the HTTP handler for the service. Every request first
+// has its client address and scheme resolved, honouring forwarded headers
+// only from a peer in trusted (see resolveOrigin). version is reported by
 // GET /health; auth mounts the /auth routes; sessions guards /api/v1 and
 // /media; assets is the web build served for every other GET. /health, /auth
 // and the web build are public. Requests to /api/v1 other than GET, HEAD and
 // OPTIONS also need the session's CSRF token in CSRFHeader.
-func NewRouter(logger *slog.Logger, version string, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) {
+func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) {
 	notFound := apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found")
 	methodNotAllowed := apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed")
 
@@ -36,6 +39,7 @@ func NewRouter(logger *slog.Logger, version string, auth RouteMounter, sessions
 	}
 
 	r := chi.NewRouter()
+	r.Use(resolveOrigin(logger, trusted))
 	r.MethodNotAllowed(methodNotAllowed)
 	r.NotFound(notFound)
 

internal/server/server_test.go

@@ -27,7 +27,7 @@ func newTestRouter(t *testing.T, version string) http.Handler {
 		"index.html":             {Data: []byte(fixtureIndex)},
 		"assets/index-AbC123.js": {Data: []byte(fixtureAsset)},
 	}
-	r, err := NewRouter(logger, version, stubAuth{}, stubSessions{}, assets)
+	r, err := NewRouter(logger, version, nil, stubAuth{}, stubSessions{}, assets)
 	if err != nil {
 		t.Fatal(err)
 	}