diff --git a/README.md b/README.md index 6ff2bd3..d374368 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,10 @@ export MEALQUEST_OIDC_ALLOWED_GROUPS=mealquest-admin # and always MEALQUEST_SESSION_MAX after sign-in (default 4320h). # export MEALQUEST_SESSION_IDLE=720h # export MEALQUEST_SESSION_MAX=4320h +# Optional: comma-separated IPs/CIDRs of reverse proxies whose +# X-Forwarded-For/-Proto headers are honoured (production: Traefik). Unset +# trusts no peer, which is right for local development. +# export MEALQUEST_TRUSTED_PROXY=10.60.1.33 make dev ``` @@ -47,7 +51,8 @@ make dev (which proxies `/api`, `/auth` and `/media` to `:8080`). Open http://localhost:5173. Configuration is environment-only and fails fast: every variable above is required except the commented-out session -lifetimes (when set they must be positive, with MAX at least IDLE), and +lifetimes (when set they must be positive, with MAX at least IDLE) and +trusted proxy list (every entry must be an IP or CIDR), and secrets are read only from files named by `*_FILE` variables. `MEALQUEST_PUBLIC_URL`, `MEALQUEST_OIDC_ISSUER` and the endpoints the issuer advertises must be `https://`; plain `http://` is accepted only for `localhost`, `127.0.0.1` and `[::1]`. diff --git a/cmd/server/main.go b/cmd/server/main.go index 229a69a..c937169 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -15,6 +15,7 @@ import ( "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/auth" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/config" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/frontend" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/server" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" @@ -31,7 +32,7 @@ func main() { os.Exit(1) } - logger := slog.New(slog.NewJSONHandler(os.Stdout, nil)) + logger := slog.New(forwarded.LogHandler(slog.NewJSONHandler(os.Stdout, nil))) if err := run(cfg, logger); err != nil { logger.Error("server stopped", "error", err) os.Exit(1) @@ -68,7 +69,7 @@ func run(cfg config.Config, logger *slog.Logger) error { return err } - router, err := server.NewRouter(logger, version, authFlow, sessions, frontend.Assets()) + router, err := server.NewRouter(logger, version, cfg.TrustedProxy, authFlow, sessions, frontend.Assets()) if err != nil { return err } @@ -126,6 +127,7 @@ func logListening(logger *slog.Logger, cfg config.Config) { logger.Info("listening", "addr", cfg.HTTPAddr, "public_origin", cfg.PublicOrigin(), + "trusted_proxy", cfg.TrustedProxy, "data_dir", cfg.DataDir, "version", version, ) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4beae8c..6f9eaab 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -29,6 +29,15 @@ docker compose project "mealquest" — 10.60.1.14 One Go service serves the API, the auth flow, media and the PWA. Its SQLite database is replicated by Litestream to the NAS. +Traefik terminates TLS. The service honours `X-Forwarded-For` and +`X-Forwarded-Proto` only from peers listed in `MEALQUEST_TRUSTED_PROXY` +(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. 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 | Path | Purpose | diff --git a/internal/config/config.go b/internal/config/config.go index 699003b..d3ab5e2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/secureurl" ) @@ -30,6 +31,8 @@ const ( EnvSessionIdle = "MEALQUEST_SESSION_IDLE" EnvSessionMax = "MEALQUEST_SESSION_MAX" + + EnvTrustedProxy = "MEALQUEST_TRUSTED_PROXY" ) // Session lifetime defaults (D-004): long enough that the kitchen tablet @@ -58,6 +61,10 @@ type Config struct { SessionIdle time.Duration // SessionMax ends every session this long after sign-in, used or not. SessionMax time.Duration + + // TrustedProxy lists the reverse proxies whose X-Forwarded-For and + // X-Forwarded-Proto headers are honoured. Empty trusts no peer. + TrustedProxy forwarded.Trusted } // PublicOrigin returns the scheme and host of PublicURL, the only parts of it @@ -88,6 +95,8 @@ func load(lookup func(string) (string, bool)) (Config, error) { SessionIdle: r.sessionDuration(EnvSessionIdle, DefaultSessionIdle), SessionMax: r.sessionDuration(EnvSessionMax, DefaultSessionMax), + + TrustedProxy: r.trustedProxy(EnvTrustedProxy), } if cfg.SessionIdle > 0 && cfg.SessionMax > 0 && cfg.SessionMax < cfg.SessionIdle { r.fail(EnvSessionMax, "must be at least %s (%s)", EnvSessionIdle, cfg.SessionIdle) @@ -177,6 +186,18 @@ func (r *reader) list(name string) []string { return items } +// trustedProxy parses an optional comma-separated list of IP addresses and +// CIDR prefixes with forwarded.ParseTrusted. Unset or blank trusts no peer. +func (r *reader) trustedProxy(name string) forwarded.Trusted { + v, _ := r.lookup(name) + t, err := forwarded.ParseTrusted(v) + if err != nil { + r.fail(name, "%v", err) + return nil + } + return t +} + func (r *reader) listenAddr(name string) string { v, ok := r.required(name) if !ok { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4dafec1..8816bf7 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "errors" "fmt" + "net/netip" "net/url" "os" "path/filepath" @@ -11,6 +12,8 @@ import ( "strings" "testing" "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/forwarded" ) func TestLoad(t *testing.T) { @@ -293,6 +296,48 @@ func TestLoad(t *testing.T) { env: with(map[string]string{EnvSessionMax: "719h"}), badVars: []string{EnvSessionMax}, }, + { + name: "trusted proxy IP", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.33"}), + }, + { + name: "trusted proxy IPs and CIDRs", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.33, 172.16.0.0/12,2001:db8::/32,::1"}), + }, + { + name: "blank trusted proxy trusts no peer", + env: with(map[string]string{EnvTrustedProxy: " "}), + }, + { + name: "trusted proxy not an IP", + env: with(map[string]string{EnvTrustedProxy: "traefik"}), + badVars: []string{EnvTrustedProxy}, + }, + { + name: "trusted proxy invalid CIDR", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.0/33"}), + badVars: []string{EnvTrustedProxy}, + }, + { + name: "trusted proxy CIDR with host bits", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.33/24"}), + badVars: []string{EnvTrustedProxy}, + }, + { + name: "trusted proxy empty entry", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.33,,10.60.1.34"}), + badVars: []string{EnvTrustedProxy}, + }, + { + name: "trusted proxy trusting everyone", + env: with(map[string]string{EnvTrustedProxy: "0.0.0.0/0"}), + badVars: []string{EnvTrustedProxy}, + }, + { + name: "trusted proxy with port", + env: with(map[string]string{EnvTrustedProxy: "10.60.1.33:443"}), + badVars: []string{EnvTrustedProxy}, + }, { name: "several invalid values are all named", env: with(map[string]string{ @@ -402,8 +447,13 @@ func TestLoadValues(t *testing.T) { t.Errorf("SessionIdle, SessionMax = %v, %v, want defaults 720h, 4320h", cfg.SessionIdle, cfg.SessionMax) } + if cfg.TrustedProxy != nil { + t.Errorf("TrustedProxy = %v, want none when unset", cfg.TrustedProxy) + } + env[EnvSessionIdle] = "12h" env[EnvSessionMax] = "36h" + env[EnvTrustedProxy] = "10.60.1.33, 172.16.0.0/12" cfg, err = load(func(k string) (string, bool) { v, ok := env[k]; return v, ok }) if err != nil { t.Fatalf("load() error = %v", err) @@ -411,6 +461,10 @@ func TestLoadValues(t *testing.T) { if cfg.SessionIdle != 12*time.Hour || cfg.SessionMax != 36*time.Hour { t.Errorf("SessionIdle, SessionMax = %v, %v, want 12h, 36h", cfg.SessionIdle, cfg.SessionMax) } + want := forwarded.Trusted{netip.MustParsePrefix("10.60.1.33/32"), netip.MustParsePrefix("172.16.0.0/12")} + if !slices.Equal(cfg.TrustedProxy, want) { + t.Errorf("TrustedProxy = %v, want %v", cfg.TrustedProxy, want) + } } func TestSecretRedacted(t *testing.T) { diff --git a/internal/forwarded/forwarded.go b/internal/forwarded/forwarded.go new file mode 100644 index 0000000..8ac429e --- /dev/null +++ b/internal/forwarded/forwarded.go @@ -0,0 +1,221 @@ +// Package forwarded resolves where a request really came from. The service +// runs behind a reverse proxy (Traefik), so the TCP peer is usually the proxy; +// the X-Forwarded-For and X-Forwarded-Proto headers it adds are believed only +// when the peer is a trusted proxy. From anyone else they are ignored, so a +// client cannot spoof its address or scheme. +package forwarded + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/http" + "net/netip" + "strings" +) + +// Header names honoured from trusted proxies. +const ( + HeaderFor = "X-Forwarded-For" + HeaderProto = "X-Forwarded-Proto" +) + +// ErrInvalidHeader marks a malformed forwarded header from a trusted peer. +var ErrInvalidHeader = errors.New("invalid forwarded header") + +// Trusted is the set of proxy addresses whose forwarded headers are honoured. +// The zero value trusts no peer. +type Trusted []netip.Prefix + +// ParseTrusted parses a comma-separated list of IP addresses and CIDR +// prefixes. A blank list trusts no peer. Each entry must be a plain address +// or a prefix with no host bits set; zones, IPv4-mapped IPv6 forms and +// zero-length prefixes, which would trust every peer, are rejected. +func ParseTrusted(s string) (Trusted, error) { + if strings.TrimSpace(s) == "" { + return nil, nil + } + var t Trusted + for _, entry := range strings.Split(s, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + return nil, fmt.Errorf("empty entry in comma-separated list %q", s) + } + p, err := parseEntry(entry) + if err != nil { + return nil, err + } + t = append(t, p) + } + return t, nil +} + +func parseEntry(entry string) (netip.Prefix, error) { + var p netip.Prefix + if strings.Contains(entry, "/") { + var err error + p, err = netip.ParsePrefix(entry) + if err != nil { + return netip.Prefix{}, fmt.Errorf("invalid CIDR %q", entry) + } + if p != p.Masked() { + return netip.Prefix{}, fmt.Errorf("CIDR %q has host bits set (did you mean %s?)", entry, p.Masked()) + } + } else { + a, err := netip.ParseAddr(entry) + if err != nil { + return netip.Prefix{}, fmt.Errorf("invalid IP address %q", entry) + } + if a.Zone() != "" { + return netip.Prefix{}, fmt.Errorf("IP address %q must not have a zone", entry) + } + p = netip.PrefixFrom(a, a.BitLen()) + } + switch { + case p.Addr().Is4In6(): + return netip.Prefix{}, fmt.Errorf("%q is IPv4-mapped IPv6; write the IPv4 form", entry) + case p.Bits() == 0: + return netip.Prefix{}, fmt.Errorf("%q would trust every peer", entry) + } + return p, nil +} + +// Contains reports whether a is a trusted proxy address. IPv4-mapped IPv6 +// addresses, as a dual-stack listener reports IPv4 peers, match their IPv4 +// form. +func (t Trusted) Contains(a netip.Addr) bool { + a = normalize(a) + for _, p := range t { + if p.Contains(a) { + return true + } + } + return false +} + +func normalize(a netip.Addr) netip.Addr { return a.Unmap().WithZone("") } + +// Origin is where a request came from. +type Origin struct { + // ClientIP is the client's address. + ClientIP netip.Addr + // Scheme is "https" or "http": the scheme the client used. + Scheme string +} + +// Resolve works out r's Origin. When the TCP peer (r.RemoteAddr) is not in +// trusted, both forwarded headers are ignored: the client is the peer and the +// scheme is that of the connection. +// +// When the peer is trusted: +// - X-Forwarded-For entries (across all its header lines, in order) are +// walked right to left, skipping trusted proxy addresses; the first +// untrusted address is the client. Entries left of it are client-supplied +// and not examined. If every entry is trusted the leftmost is the client; +// with no X-Forwarded-For the peer is the client. +// - X-Forwarded-Proto, if present, must be a single "http" or "https" and +// is the scheme; without it the connection's scheme is used. +// +// A malformed header from a trusted peer is an error wrapping +// ErrInvalidHeader; any other error means RemoteAddr is not an IP and port. +func Resolve(r *http.Request, trusted Trusted) (Origin, error) { + peer, err := netip.ParseAddrPort(r.RemoteAddr) + if err != nil { + return Origin{}, fmt.Errorf("peer address %q is not an IP and port", r.RemoteAddr) + } + o := Origin{ClientIP: normalize(peer.Addr()), Scheme: "http"} + if r.TLS != nil { + o.Scheme = "https" + } + if !trusted.Contains(o.ClientIP) { + return o, nil + } + if values := r.Header.Values(HeaderFor); len(values) > 0 { + o.ClientIP, err = clientFromChain(strings.Split(strings.Join(values, ","), ","), trusted) + if err != nil { + return Origin{}, err + } + } + switch values := r.Header.Values(HeaderProto); len(values) { + case 0: + case 1: + switch scheme := strings.ToLower(strings.TrimSpace(values[0])); scheme { + case "http", "https": + o.Scheme = scheme + default: + return Origin{}, fmt.Errorf("%w: %s %q is not http or https", ErrInvalidHeader, HeaderProto, values[0]) + } + default: + return Origin{}, fmt.Errorf("%w: %d %s headers, want one", ErrInvalidHeader, len(values), HeaderProto) + } + return o, nil +} + +// clientFromChain returns the rightmost untrusted address in chain, or the +// leftmost address when all are trusted. +func clientFromChain(chain []string, trusted Trusted) (netip.Addr, error) { + var client netip.Addr + for i := len(chain) - 1; i >= 0; i-- { + a, err := netip.ParseAddr(strings.TrimSpace(chain[i])) + if err != nil { + return netip.Addr{}, fmt.Errorf("%w: %s entry %q is not an IP address", ErrInvalidHeader, HeaderFor, strings.TrimSpace(chain[i])) + } + client = normalize(a) + if !trusted.Contains(client) { + break + } + } + return client, nil +} + +type contextKey struct{} + +// NewContext returns ctx carrying o. +func NewContext(ctx context.Context, o Origin) context.Context { + return context.WithValue(ctx, contextKey{}, o) +} + +// FromContext returns the Origin ctx carries; ok is false when the request +// was not resolved. +func FromContext(ctx context.Context) (o Origin, ok bool) { + o, ok = ctx.Value(contextKey{}).(Origin) + return o, ok +} + +// ClientIP returns the resolved client address ctx carries. +func ClientIP(ctx context.Context) (netip.Addr, bool) { + o, ok := FromContext(ctx) + return o.ClientIP, ok +} + +// Scheme returns the resolved request scheme ctx carries. +func Scheme(ctx context.Context) (string, bool) { + o, ok := FromContext(ctx) + return o.Scheme, ok +} + +// LogAttr is the log attribute key for the resolved client address. +const LogAttr = "client_ip" + +// LogHandler wraps h so every record logged with a resolved request's +// context carries the client address as LogAttr. +func LogHandler(h slog.Handler) slog.Handler { return logHandler{h} } + +type logHandler struct{ slog.Handler } + +func (h logHandler) Handle(ctx context.Context, rec slog.Record) error { + if ip, ok := ClientIP(ctx); ok { + rec = rec.Clone() + rec.AddAttrs(slog.String(LogAttr, ip.String())) + } + return h.Handler.Handle(ctx, rec) +} + +func (h logHandler) WithAttrs(attrs []slog.Attr) slog.Handler { + return logHandler{h.Handler.WithAttrs(attrs)} +} + +func (h logHandler) WithGroup(name string) slog.Handler { + return logHandler{h.Handler.WithGroup(name)} +} diff --git a/internal/forwarded/forwarded_test.go b/internal/forwarded/forwarded_test.go new file mode 100644 index 0000000..298e9ab --- /dev/null +++ b/internal/forwarded/forwarded_test.go @@ -0,0 +1,260 @@ +package forwarded + +import ( + "bytes" + "context" + "crypto/tls" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "net/netip" + "slices" + "strings" + "testing" +) + +func TestParseTrusted(t *testing.T) { + t.Parallel() + tests := []struct { + in string + want []string + wantErr string + }{ + {in: "", want: nil}, + {in: " ", want: nil}, + {in: "10.60.1.33", want: []string{"10.60.1.33/32"}}, + {in: "10.60.1.33, 172.16.0.0/12", want: []string{"10.60.1.33/32", "172.16.0.0/12"}}, + {in: "2001:db8::1,2001:db8::/32", want: []string{"2001:db8::1/128", "2001:db8::/32"}}, + {in: "traefik", wantErr: "invalid IP address"}, + {in: "10.60.1.33:443", wantErr: "invalid IP address"}, + {in: "10.60.1.0/33", wantErr: "invalid CIDR"}, + {in: "10.60.1.33/24", wantErr: "did you mean 10.60.1.0/24"}, + {in: "10.60.1.33,,10.60.1.34", wantErr: "empty entry"}, + {in: "10.60.1.33,", wantErr: "empty entry"}, + {in: "fe80::1%eth0", wantErr: "zone"}, + {in: "::ffff:10.60.1.33", wantErr: "IPv4-mapped"}, + {in: "0.0.0.0/0", wantErr: "every peer"}, + {in: "::/0", wantErr: "every peer"}, + } + for _, tt := range tests { + got, err := ParseTrusted(tt.in) + if tt.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("ParseTrusted(%q) error = %v, want one containing %q", tt.in, err, tt.wantErr) + } + continue + } + if err != nil { + t.Errorf("ParseTrusted(%q) error = %v", tt.in, err) + continue + } + var gotStr []string + for _, p := range got { + gotStr = append(gotStr, p.String()) + } + if !slices.Equal(gotStr, tt.want) { + t.Errorf("ParseTrusted(%q) = %v, want %v", tt.in, gotStr, tt.want) + } + } +} + +func TestTrustedContainsMappedIPv4(t *testing.T) { + t.Parallel() + trusted := mustTrusted(t, "10.60.1.33") + if !trusted.Contains(netip.MustParseAddr("::ffff:10.60.1.33")) { + t.Error("IPv4-mapped form of a trusted address is not trusted") + } + if Trusted(nil).Contains(netip.MustParseAddr("10.60.1.33")) { + t.Error("an empty set trusts a peer") + } +} + +func mustTrusted(t *testing.T, s string) Trusted { + t.Helper() + tr, err := ParseTrusted(s) + if err != nil { + t.Fatal(err) + } + return tr +} + +func TestResolve(t *testing.T) { + t.Parallel() + // 10.60.1.33 is Traefik; 10.60.2.0/24 is an inner proxy tier. + trusted := mustTrusted(t, "10.60.1.33,10.60.2.0/24") + tests := []struct { + name string + peer string + tls bool + xff []string + xfp []string + wantIP string + wantScheme string + }{ + { + name: "untrusted peer, no headers", + peer: "198.51.100.9:5000", wantIP: "198.51.100.9", wantScheme: "http", + }, + { + name: "untrusted peer over TLS", + peer: "198.51.100.9:5000", tls: true, wantIP: "198.51.100.9", wantScheme: "https", + }, + { + name: "untrusted peer spoofing both headers is ignored", + peer: "198.51.100.9:5000", xff: []string{"203.0.113.7"}, xfp: []string{"https"}, + wantIP: "198.51.100.9", wantScheme: "http", + }, + { + name: "untrusted peer's malformed headers are ignored, not rejected", + peer: "198.51.100.9:5000", xff: []string{"not-an-ip"}, xfp: []string{"gopher"}, + wantIP: "198.51.100.9", wantScheme: "http", + }, + { + name: "trusted peer: client from X-Forwarded-For, scheme from X-Forwarded-Proto", + peer: "10.60.1.33:41000", xff: []string{"203.0.113.7"}, xfp: []string{"https"}, + wantIP: "203.0.113.7", wantScheme: "https", + }, + { + name: "trusted peer seen as IPv4-mapped IPv6", + peer: "[::ffff:10.60.1.33]:41000", xff: []string{"203.0.113.7"}, xfp: []string{"https"}, + wantIP: "203.0.113.7", wantScheme: "https", + }, + { + name: "trusted peer without headers is itself the client", + peer: "10.60.1.33:41000", wantIP: "10.60.1.33", wantScheme: "http", + }, + { + name: "rightmost untrusted address wins over a client-supplied prefix", + peer: "10.60.1.33:41000", xff: []string{"1.2.3.4, 203.0.113.7"}, xfp: []string{"https"}, + wantIP: "203.0.113.7", wantScheme: "https", + }, + { + name: "trusted proxies in the chain are skipped", + peer: "10.60.1.33:41000", xff: []string{"1.2.3.4, 203.0.113.7, 10.60.2.5"}, xfp: []string{"https"}, + wantIP: "203.0.113.7", wantScheme: "https", + }, + { + name: "several header lines form one chain", + peer: "10.60.1.33:41000", xff: []string{"1.2.3.4", "203.0.113.7, 10.60.2.5"}, + wantIP: "203.0.113.7", wantScheme: "http", + }, + { + name: "garbage left of the client is not examined", + peer: "10.60.1.33:41000", xff: []string{"spoofed, 203.0.113.7"}, + wantIP: "203.0.113.7", wantScheme: "http", + }, + { + name: "all trusted: leftmost is the client", + peer: "10.60.1.33:41000", xff: []string{"10.60.2.5, 10.60.2.6"}, + wantIP: "10.60.2.5", wantScheme: "http", + }, + { + name: "IPv6 client", + peer: "10.60.1.33:41000", xff: []string{"2001:db8::7"}, xfp: []string{"HTTPS"}, + wantIP: "2001:db8::7", wantScheme: "https", + }, + { + name: "trusted peer may downgrade to http", + peer: "10.60.1.33:41000", tls: true, xfp: []string{"http"}, + wantIP: "10.60.1.33", wantScheme: "http", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + r := request(tt.peer, tt.xff, tt.xfp) + if tt.tls { + r.TLS = &tls.ConnectionState{} + } + o, err := Resolve(r, trusted) + if err != nil { + t.Fatalf("Resolve error = %v", err) + } + if o.ClientIP.String() != tt.wantIP || o.Scheme != tt.wantScheme { + t.Errorf("Resolve = %s %s, want %s %s", o.ClientIP, o.Scheme, tt.wantIP, tt.wantScheme) + } + }) + } +} + +func TestResolveRejectsMalformedHeadersFromTrustedPeer(t *testing.T) { + t.Parallel() + trusted := mustTrusted(t, "10.60.1.33") + for _, tt := range []struct { + name string + xff, xfp []string + }{ + {"X-Forwarded-For not an IP", []string{"not-an-ip"}, nil}, + {"X-Forwarded-For with a port", []string{"203.0.113.7:5000"}, nil}, + {"X-Forwarded-For empty entry", []string{"203.0.113.7, "}, nil}, + {"X-Forwarded-For empty", []string{""}, nil}, + {"X-Forwarded-Proto unknown scheme", nil, []string{"ftp"}}, + {"X-Forwarded-Proto list", nil, []string{"https, http"}}, + {"X-Forwarded-Proto repeated", nil, []string{"https", "https"}}, + } { + _, err := Resolve(request("10.60.1.33:41000", tt.xff, tt.xfp), trusted) + if !errors.Is(err, ErrInvalidHeader) { + t.Errorf("%s: Resolve error = %v, want ErrInvalidHeader", tt.name, err) + } + } +} + +func TestResolveBadPeerAddress(t *testing.T) { + t.Parallel() + r := request("@", nil, nil) + _, err := Resolve(r, nil) + if err == nil || errors.Is(err, ErrInvalidHeader) { + t.Errorf("Resolve error = %v, want a peer address error", err) + } +} + +func request(peer string, xff, xfp []string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = peer + for _, v := range xff { + r.Header.Add(HeaderFor, v) + } + for _, v := range xfp { + r.Header.Add(HeaderProto, v) + } + return r +} + +func TestContextHelpers(t *testing.T) { + t.Parallel() + if _, ok := ClientIP(context.Background()); ok { + t.Error("ClientIP ok on an unresolved context") + } + if _, ok := Scheme(context.Background()); ok { + t.Error("Scheme ok on an unresolved context") + } + ctx := NewContext(context.Background(), Origin{ClientIP: netip.MustParseAddr("203.0.113.7"), Scheme: "https"}) + if ip, ok := ClientIP(ctx); !ok || ip.String() != "203.0.113.7" { + t.Errorf("ClientIP = %s %v, want 203.0.113.7 true", ip, ok) + } + if s, ok := Scheme(ctx); !ok || s != "https" { + t.Errorf("Scheme = %q %v, want https true", s, ok) + } +} + +func TestLogHandler(t *testing.T) { + t.Parallel() + var buf bytes.Buffer + logger := slog.New(LogHandler(slog.NewTextHandler(&buf, nil))).With("component", "test") + ctx := NewContext(context.Background(), Origin{ClientIP: netip.MustParseAddr("203.0.113.7"), Scheme: "https"}) + + logger.InfoContext(ctx, "resolved") + logger.InfoContext(context.Background(), "unresolved") + + lines := strings.Split(strings.TrimSpace(buf.String()), "\n") + if len(lines) != 2 { + t.Fatalf("logged %d lines, want 2: %q", len(lines), buf.String()) + } + if !strings.Contains(lines[0], "client_ip=203.0.113.7") || !strings.Contains(lines[0], "component=test") { + t.Errorf("resolved line = %q, want client_ip and the With attribute", lines[0]) + } + if strings.Contains(lines[1], "client_ip") { + t.Errorf("unresolved line = %q, want no client_ip", lines[1]) + } +} 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) + } + }) + } +} diff --git a/internal/server/origin.go b/internal/server/origin.go new file mode 100644 index 0000000..9f711ad --- /dev/null +++ b/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))) + } + }) + } +} diff --git a/internal/server/origin_test.go b/internal/server/origin_test.go new file mode 100644 index 0000000..1df8212 --- /dev/null +++ b/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) + } + } +} diff --git a/internal/server/server.go b/internal/server/server.go index fe336a2..82a1e8c 100644 --- a/internal/server/server.go +++ b/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) diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 05d877f..7419658 100644 --- a/internal/server/server_test.go +++ b/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) }