diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 2df24c176..1c42212b2 100644 --- a/modules/caddyhttp/app.go +++ b/modules/caddyhttp/app.go @@ -216,8 +216,6 @@ func (app *App) Provision(ctx caddy.Context) error { if app.Metrics != nil { app.Metrics.init = sync.Once{} app.Metrics.httpMetrics = &httpMetrics{} - // Scan config for allowed hosts to prevent cardinality explosion - app.Metrics.scanConfigForHosts(app) if err := app.Metrics.provisionOTLP(ctx); err != nil { return err } @@ -419,6 +417,9 @@ func (app *App) Provision(ctx caddy.Context) error { srv.MaxWriteChunk = DefaultMaxWriteChunk } } + if app.Metrics != nil { + app.Metrics.scanConfigForHosts(app) + } ctx.Context = oldContext return nil } diff --git a/modules/caddyhttp/metrics.go b/modules/caddyhttp/metrics.go index a30322e68..cca3ecb9c 100644 --- a/modules/caddyhttp/metrics.go +++ b/modules/caddyhttp/metrics.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "net" "net/http" "strings" "sync" @@ -57,17 +58,17 @@ type Metrics struct { // CARDINALITY PROTECTION: To prevent unbounded cardinality attacks, // only explicitly configured hosts (via host matchers) are allowed // by default. Other hosts are aggregated under the "_other" label. - // See AllowCatchAllHosts to change this behavior. + // See ObserveCatchallHosts to change this behavior. This also applies to + // HTTPS, including On-Demand TLS sites, which do not have host matchers + // for every name they serve. Host labels never include the port. PerHost bool `json:"per_host,omitempty"` // Allow metrics for catch-all hosts (hosts without explicit configuration). // When false (default), only hosts explicitly configured via host matchers // will get individual metrics labels. All other hosts will be aggregated - // under the "_other" label to prevent cardinality explosion. - // - // This is automatically enabled for HTTPS servers (since certificates provide - // some protection against unbounded cardinality), but disabled for HTTP servers - // by default to prevent cardinality attacks from arbitrary Host headers. + // under the "_other" label to prevent cardinality explosion. HTTPS + // requests are not exempt: the Host header is not bound to the TLS + // certificate, so it can be any value. // // Set to true to allow all hosts to get individual metrics (NOT RECOMMENDED // for production environments exposed to the internet). @@ -82,11 +83,10 @@ type Metrics struct { // keep this field false to disable OTLP export. OTLP bool `json:"otlp,omitempty"` - init sync.Once - httpMetrics *httpMetrics - allowedHosts map[string]struct{} - hasHTTPSServer bool - meterProvider *sdkmetric.MeterProvider + init sync.Once + httpMetrics *httpMetrics + allowedHosts map[string]struct{} + meterProvider *sdkmetric.MeterProvider } type httpMetrics struct { @@ -235,44 +235,50 @@ func (m *Metrics) scanConfigForHosts(app *App) { } m.allowedHosts = make(map[string]struct{}) - m.hasHTTPSServer = false - - for _, srv := range app.Servers { - // Check if this server has TLS enabled - serverHasTLS := len(srv.TLSConnPolicies) > 0 - if serverHasTLS { - m.hasHTTPSServer = true - } - - // Collect hosts from route matchers - for _, route := range srv.Routes { + var scanRoutes func(RouteList) + scanRoutes = func(routes RouteList) { + for _, route := range routes { for _, matcherSet := range route.MatcherSets { for _, matcher := range matcherSet { if hm, ok := matcher.(*MatchHost); ok { for _, host := range *hm { // Only allow non-fuzzy hosts to prevent unbounded cardinality if !hm.fuzzy(host) { - m.allowedHosts[strings.ToLower(host)] = struct{}{} + m.allowedHosts[metricsHost(host)] = struct{}{} } } } } } + for _, handler := range route.Handlers { + if sr, ok := handler.(*Subroute); ok { + scanRoutes(sr.Routes) + if sr.Errors != nil { + scanRoutes(sr.Errors.Routes) + } + } + } + } + } + for _, srv := range app.Servers { + scanRoutes(srv.Routes) + if srv.Errors != nil { + scanRoutes(srv.Errors.Routes) + } + for _, route := range srv.NamedRoutes { + scanRoutes(RouteList{*route}) } } } // shouldAllowHostMetrics determines if metrics should be collected for the given host. -// This implements the cardinality protection by only allowing metrics for: -// 1. Explicitly configured hosts -// 2. Catch-all requests on HTTPS servers (if AllowCatchAllHosts is true or auto-enabled) -// 3. Catch-all requests on HTTP servers only if explicitly allowed -func (m *Metrics) shouldAllowHostMetrics(host string, isHTTPS bool) bool { +// Only explicitly configured hosts are allowed unless catch-all observation is enabled. +func (m *Metrics) shouldAllowHostMetrics(host string) bool { if !m.PerHost { return true // host won't be used in labels anyway } - normalizedHost := strings.ToLower(host) + normalizedHost := metricsHost(host) // Always allow explicitly configured hosts if _, exists := m.allowedHosts[normalizedHost]; exists { @@ -280,8 +286,14 @@ func (m *Metrics) shouldAllowHostMetrics(host string, isHTTPS bool) bool { } // For catch-all requests (not in allowed hosts) - allowCatchAll := m.ObserveCatchallHosts || (isHTTPS && m.hasHTTPSServer) - return allowCatchAll + return m.ObserveCatchallHosts +} + +func metricsHost(host string) string { + if hostname, _, err := net.SplitHostPort(host); err == nil { + host = hostname + } + return strings.ToLower(strings.Trim(host, "[]")) } // serverNameFromContext extracts the current server name from the context. @@ -319,14 +331,12 @@ func (h *metricsInstrumentedRoute) ServeHTTP(w http.ResponseWriter, r *http.Requ // of a panic statusLabels := prometheus.Labels{"server": server, "handler": h.handler, "method": method, "code": ""} - // Determine if this is an HTTPS request - isHTTPS := r.TLS != nil - if h.metrics.PerHost { // Apply cardinality protection for host metrics - if h.metrics.shouldAllowHostMetrics(r.Host, isHTTPS) { - labels["host"] = strings.ToLower(r.Host) - statusLabels["host"] = strings.ToLower(r.Host) + host := metricsHost(r.Host) + if h.metrics.shouldAllowHostMetrics(host) { + labels["host"] = host + statusLabels["host"] = host } else { // Use a catch-all label for unallowed hosts to prevent cardinality explosion labels["host"] = "_other" diff --git a/modules/caddyhttp/metrics_test.go b/modules/caddyhttp/metrics_test.go index d75b3cae1..8b4707e83 100644 --- a/modules/caddyhttp/metrics_test.go +++ b/modules/caddyhttp/metrics_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "errors" + "fmt" "net/http" "net/http/httptest" "strings" @@ -434,11 +435,10 @@ func TestMetricsCardinalityProtection(t *testing.T) { func TestMetricsHTTPSCatchAll(t *testing.T) { ctx, _ := caddy.NewContext(caddy.Context{Context: context.Background()}) - // Test that HTTPS requests allow catch-all even when AllowCatchAllHosts is false + // HTTPS must not implicitly disable cardinality protection. metrics := &Metrics{ PerHost: true, ObserveCatchallHosts: false, - hasHTTPSServer: true, // Simulate having HTTPS servers init: sync.Once{}, httpMetrics: &httpMetrics{}, allowedHosts: make(map[string]struct{}), // Empty - no explicitly allowed hosts @@ -451,7 +451,7 @@ func TestMetricsHTTPSCatchAll(t *testing.T) { ih := newMetricsInstrumentedRoute(ctx, "test", h, metrics) - // Test HTTPS request (should be allowed even though not in allowedHosts) + // Test HTTPS request (should be mapped to "_other"). r1 := httptest.NewRequest("GET", "https://unknown.com/", nil) r1.Host = "unknown.com" r1.TLS = &tls.ConnectionState{} // Mark as TLS/HTTPS @@ -465,12 +465,11 @@ func TestMetricsHTTPSCatchAll(t *testing.T) { w2 := httptest.NewRecorder() ih.ServeHTTP(w2, r2) - // Check that HTTPS request gets real host, HTTP gets "_other" + // Both requests should be aggregated. expected := ` # HELP caddy_http_requests_total Counter of HTTP(S) requests made. # TYPE caddy_http_requests_total counter - caddy_http_requests_total{handler="test",host="_other",server="UNKNOWN"} 1 - caddy_http_requests_total{handler="test",host="unknown.com",server="UNKNOWN"} 1 + caddy_http_requests_total{handler="test",host="_other",server="UNKNOWN"} 2 ` if err := testutil.GatherAndCompare(ctx.GetMetricsRegistry(), strings.NewReader(expected), @@ -480,6 +479,126 @@ func TestMetricsHTTPSCatchAll(t *testing.T) { } } +func TestMetricsHostCardinality(t *testing.T) { + for _, scheme := range []string{"http", "https"} { + for _, catchall := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/catchall=%t", scheme, catchall), func(t *testing.T) { + ctx, cancel := caddy.NewContext(caddy.Context{Context: context.Background()}) + defer cancel() + m := &Metrics{ + PerHost: true, + ObserveCatchallHosts: catchall, + httpMetrics: &httpMetrics{}, + allowedHosts: map[string]struct{}{"site.test": {}}, + } + h := newMetricsInstrumentedRoute(ctx, "test", HandlerFunc(func(w http.ResponseWriter, _ *http.Request) error { + w.WriteHeader(http.StatusOK) + return nil + }), m) + for i := range 80 { + for _, host := range []string{ + fmt.Sprintf("SITE.test:%d", 20000+i), + fmt.Sprintf("metric-%d.site.test", i), + fmt.Sprintf("unrelated-%d.test", i), + } { + req := httptest.NewRequest(http.MethodGet, scheme+"://site.test/", nil) + req.Host = host + if req.TLS != nil { + req.TLS.ServerName = "site.test" + } + if err := h.ServeHTTP(httptest.NewRecorder(), req); err != nil { + t.Fatal(err) + } + } + } + families, err := ctx.GetMetricsRegistry().Gather() + if err != nil { + t.Fatal(err) + } + wantCount := 2 + if catchall { + wantCount = 161 + } + found := false + for _, family := range families { + if family.GetName() != "caddy_http_requests_total" { + continue + } + found = true + if len(family.Metric) != wantCount { + t.Fatalf("got %d series, want %d", len(family.Metric), wantCount) + } + for _, metric := range family.Metric { + for _, label := range metric.Label { + if label.GetName() != "host" { + continue + } + host := label.GetValue() + if strings.Contains(host, ":") { + t.Errorf("port persisted in host label %q", host) + } + if !catchall && host != "site.test" && host != "_other" { + t.Errorf("unbounded host label %q", host) + } + if host == "site.test" && metric.Counter.GetValue() != 80 { + t.Errorf("port variants not aggregated: %v", metric.Counter.GetValue()) + } + } + } + } + if !found { + t.Fatal("request counter not collected") + } + }) + } + } +} + +func TestMetricsScanNestedHosts(t *testing.T) { + hostRoute := func(hosts ...string) Route { + matcher := MatchHost(hosts) + return Route{MatcherSets: MatcherSets{{&matcher}}} + } + named := hostRoute("NAMED.test") + m := &Metrics{PerHost: true} + m.scanConfigForHosts(&App{Servers: map[string]*Server{ + "test": { + Routes: RouteList{ + hostRoute("TOP.test", "*.wild.test", "{dynamic}.test"), + {Handlers: []MiddlewareHandler{&Subroute{ + Routes: RouteList{hostRoute("nested.test")}, + Errors: &HTTPErrorConfig{Routes: RouteList{hostRoute("nested-error.test")}}, + }}}, + }, + Errors: &HTTPErrorConfig{Routes: RouteList{hostRoute("error.test")}}, + NamedRoutes: map[string]*Route{"named": &named}, + }, + }}) + for _, host := range []string{"top.test", "NESTED.test:443", "nested-error.test", "error.test", "named.test"} { + if !m.shouldAllowHostMetrics(host) { + t.Errorf("configured host %q was not collected", host) + } + } + if len(m.allowedHosts) != 5 { + t.Fatalf("unexpected allowlist: %v", m.allowedHosts) + } + for _, host := range []string{"random.wild.test", "unknown.test", "attacker.test:999999999"} { + if m.shouldAllowHostMetrics(host) { + t.Errorf("unconfigured host %q admitted", host) + } + } + for _, tc := range []struct{ input, want string }{ + {"EXAMPLE.test:999999999999999999", "example.test"}, + {"[::1]:443", "::1"}, + {"[::1]", "::1"}, + {"::1", "::1"}, + } { + if got := metricsHost(tc.input); got != tc.want { + t.Errorf("metricsHost(%q) = %q, want %q", tc.input, got, tc.want) + } + } +} + func TestMetricsInstrumentedRoute(t *testing.T) { ctx, _ := caddy.NewContext(caddy.Context{Context: context.Background()}) m := &Metrics{