diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7e8b650..6f9eaab 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -34,9 +34,9 @@ Traefik terminates TLS. The service honours `X-Forwarded-For` and (comma-separated IPs/CIDRs; production lists `10.60.1.33`); from any other peer they are ignored. The client is the rightmost `X-Forwarded-For` address that is not a trusted proxy. The resolved client IP is added as `client_ip` -to every log line written while handling a request, and the auth routes -refuse requests whose resolved scheme differs from `MEALQUEST_PUBLIC_URL`'s. The OIDC redirect URI is always built -from `MEALQUEST_PUBLIC_URL`, never from request headers. +to every log line written while handling a request. The OIDC redirect URI +and the Secure cookies are fixed by `MEALQUEST_PUBLIC_URL`, never by request +headers, so sign-in works whichever peer the request arrives from. ## Repository layout diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 61c67bc..4d3c1fb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -19,7 +19,6 @@ import ( "github.com/go-chi/chi/v5" "golang.org/x/oauth2" - "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl" ) @@ -84,10 +83,7 @@ type Auth struct { // endSessionURL is the provider's end_session_endpoint; logout ends // there so the provider's own session ends too. endSessionURL string - // publicScheme is the public URL's scheme; requests to the auth routes - // must have arrived over it. - publicScheme string - logger *slog.Logger + logger *slog.Logger } // New discovers the provider at cfg.Issuer and returns the login flow. It @@ -159,7 +155,6 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger cookie: newLoginCookie(cfg.SecretKey), sessions: sessions, endSessionURL: discovered.EndSessionURI, - publicScheme: cfg.PublicURL.Scheme, logger: logger, }, nil } @@ -198,39 +193,12 @@ func checkRedirect(req *http.Request, via []*http.Request) error { } // Mount registers GET LoginPath, GET CallbackPath and POST LogoutPath on r. -// The routes need the request's resolved scheme in the context (see package -// forwarded) and refuse requests that did not arrive over the public URL's -// scheme. func (a *Auth) Mount(r chi.Router) { - r = r.With(a.requirePublicScheme) r.Get(LoginPath, a.login) r.Get(CallbackPath, a.callback) r.Post(LogoutPath, a.logout) } -// requirePublicScheme refuses a request whose resolved scheme differs from -// the public URL's. The login and session cookies are Secure, and the -// redirect URI is always built from the public URL, so a sign-in reaching the -// service over another scheme cannot work: behind a TLS-terminating proxy -// this means the proxy's X-Forwarded-Proto was not honoured, because the -// proxy is not a trusted peer. -func (a *Auth) requirePublicScheme(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - scheme, ok := forwarded.Scheme(r.Context()) - switch { - case !ok: - a.logger.ErrorContext(r.Context(), "auth route reached without a resolved request scheme") - http.Error(w, "internal error", http.StatusInternalServerError) - case scheme != a.publicScheme: - a.logger.WarnContext(r.Context(), "auth request scheme differs from the public URL; is the proxy a trusted peer?", - "scheme", scheme, "public_scheme", a.publicScheme) - http.Error(w, "sign-in must use the service's public URL", http.StatusBadRequest) - default: - next.ServeHTTP(w, r) - } - }) -} - // EndSessionURL is the provider's end_session_endpoint. func (a *Auth) EndSessionURL() string { return a.endSessionURL } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 7b97b27..4ecf409 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -17,7 +17,6 @@ import ( "math/big" "net/http" "net/http/httptest" - "net/netip" "net/url" "slices" "strings" @@ -27,8 +26,6 @@ import ( "time" "github.com/go-chi/chi/v5" - - "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded" ) const ( @@ -350,32 +347,11 @@ func newHarness(t *testing.T) *harness { if err != nil { t.Fatal(err) } - r := publicRouter(a, testPublicURL.Scheme) + r := chi.NewRouter() + a.Mount(r) return &harness{provider: p, sessions: sessions, router: r, logs: logs} } -// publicRouter mounts a's routes behind the request-origin resolution the -// server installs, with every request taken to have arrived over scheme. -// Requests with noOrigin set skip the resolution. -func publicRouter(a *Auth, scheme string) http.Handler { - r := chi.NewRouter() - r.Use(func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.Header.Get(noOrigin) == "" { - o := forwarded.Origin{ClientIP: netip.MustParseAddr("192.0.2.1"), Scheme: scheme} - req = req.WithContext(forwarded.NewContext(req.Context(), o)) - } - next.ServeHTTP(w, req) - }) - }) - a.Mount(r) - return r -} - -// noOrigin is a test-only request header telling publicRouter to leave the -// request's origin unresolved. -const noOrigin = "X-Test-No-Origin" - func (h *harness) get(target string, cookies ...*http.Cookie) *http.Response { req := httptest.NewRequest(http.MethodGet, target, nil) for _, c := range cookies { @@ -457,127 +433,6 @@ func TestLoginRedirect(t *testing.T) { } } -func TestAuthRoutesRequirePublicScheme(t *testing.T) { - t.Parallel() - p := newFakeProvider(t) - logs := &syncBuffer{} - a, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("server-secret-key"), - AllowedGroups: testAllowedGroups, - }, &memorySessions{}, slog.New(slog.NewTextHandler(logs, nil))) - if err != nil { - t.Fatal(err) - } - - // The proxy's X-Forwarded-Proto was not honoured: the request looks like - // plain http while the public URL is https. - plain := publicRouter(a, "http") - for _, req := range []*http.Request{ - httptest.NewRequest(http.MethodGet, LoginPath, nil), - httptest.NewRequest(http.MethodGet, CallbackPath+"?code=c&state=s", nil), - httptest.NewRequest(http.MethodPost, LogoutPath, nil), - } { - rec := httptest.NewRecorder() - plain.ServeHTTP(rec, req) - if rec.Code != http.StatusBadRequest { - t.Errorf("%s %s over http = %d, want 400", req.Method, req.URL.Path, rec.Code) - } - if loc := rec.Header().Get("Location"); loc != "" { - t.Errorf("%s %s over http redirected to %q", req.Method, req.URL.Path, loc) - } - if c := rec.Result().Cookies(); len(c) != 0 { - t.Errorf("%s %s over http set cookies %v", req.Method, req.URL.Path, c) - } - } - if !strings.Contains(logs.String(), "trusted peer") { - t.Errorf("logs = %q, want the scheme mismatch explained", logs.String()) - } - - bare := chi.NewRouter() - a.Mount(bare) - rec := httptest.NewRecorder() - bare.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, LoginPath, nil)) - if rec.Code != http.StatusInternalServerError { - t.Errorf("login without a resolved origin = %d, want 500", rec.Code) - } -} - -// TestLoginBehindTraefik resolves each request with forwarded.Resolve, as -// the server's middleware does, trusting Traefik only. -func TestLoginBehindTraefik(t *testing.T) { - t.Parallel() - p := newFakeProvider(t) - a, err := New(context.Background(), Config{ - Issuer: p.issuer(), - ClientID: testClientID, - ClientSecret: testClientSecret, - PublicURL: testPublicURL, - SecretKey: []byte("server-secret-key"), - AllowedGroups: testAllowedGroups, - }, &memorySessions{}, slog.New(slog.NewTextHandler(io.Discard, nil))) - if err != nil { - t.Fatal(err) - } - trusted, err := forwarded.ParseTrusted("10.60.1.33") - if err != nil { - t.Fatal(err) - } - r := chi.NewRouter() - r.Use(func(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - o, err := forwarded.Resolve(req, trusted) - if err != nil { - t.Errorf("Resolve: %v", err) - return - } - next.ServeHTTP(w, req.WithContext(forwarded.NewContext(req.Context(), o))) - }) - }) - a.Mount(r) - - login := func(peer string) *http.Response { - // Traefik reaches the service over plain http on its internal - // address; the host headers must not leak into the redirect URI. - req := httptest.NewRequest(http.MethodGet, "http://mealquest:8080"+LoginPath, nil) - req.RemoteAddr = peer - req.Header.Set("X-Forwarded-For", "203.0.113.7") - req.Header.Set("X-Forwarded-Proto", "https") - req.Header.Set("X-Forwarded-Host", "evil.example.com") - rec := httptest.NewRecorder() - r.ServeHTTP(rec, req) - return rec.Result() - } - - resp := login("10.60.1.33:41000") - if resp.StatusCode != http.StatusFound { - t.Fatalf("login via Traefik status = %d, want 302", resp.StatusCode) - } - loc, err := url.Parse(resp.Header.Get("Location")) - if err != nil { - t.Fatal(err) - } - 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 != loginCookieName || !cookies[0].Secure { - t.Errorf("login cookies = %v, want one Secure %s", cookies, loginCookieName) - } - - // The same headers from an untrusted peer are ignored: the request is - // plain http, so sign-in is refused rather than setting a Secure cookie - // the browser would drop. - resp = login("198.51.100.9:5000") - if resp.StatusCode != http.StatusBadRequest || len(resp.Cookies()) != 0 { - t.Errorf("login spoofing X-Forwarded-Proto = %d with cookies %v, want 400 and none", - resp.StatusCode, resp.Cookies()) - } -} - func TestLoginStateIsFreshEachTime(t *testing.T) { t.Parallel() h := newHarness(t) @@ -1049,7 +904,8 @@ func TestCallbackRefusesTokenRedirectToHTTP(t *testing.T) { } // Trust the test server's certificate; the redirect policy stays. a.client.Transport = tlsToken.Client().Transport - r := publicRouter(a, testPublicURL.Scheme) + r := chi.NewRouter() + a.Mount(r) h := &harness{provider: p, sessions: sessions, router: r, logs: logs} l := h.login(t, LoginPath) diff --git a/internal/auth/session_test.go b/internal/auth/session_test.go index 98ce440..3f86f02 100644 --- a/internal/auth/session_test.go +++ b/internal/auth/session_test.go @@ -13,6 +13,8 @@ import ( "testing" "time" + "github.com/go-chi/chi/v5" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/migrations" ) @@ -70,7 +72,8 @@ func newSessionHarness(t *testing.T) *sessionHarness { if err != nil { t.Fatal(err) } - r := publicRouter(a, testPublicURL.Scheme) + r := chi.NewRouter() + a.Mount(r) h.harness = &harness{provider: p, router: r, logs: logs} h.auth = a return h diff --git a/internal/server/login_test.go b/internal/server/login_test.go new file mode 100644 index 0000000..d928de7 --- /dev/null +++ b/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) + } + }) + } +}