diff --git a/core/dnsserver/config.go b/core/dnsserver/config.go index 9d5e136e6..6e47fd8d2 100644 --- a/core/dnsserver/config.go +++ b/core/dnsserver/config.go @@ -114,6 +114,10 @@ type Config struct { // This is nil if not specified, allowing for a default to be used. MaxHTTPSConnections *int + // MaxHTTPSStreams defines the maximum number of concurrent HTTP/2 streams per HTTPS connection. + // This is nil if not specified, allowing for a default to be used. + MaxHTTPSStreams *int + // MaxHTTPS3Streams defines the maximum number of concurrent QUIC streams for HTTPS3. // This is nil if not specified, allowing for a default to be used. MaxHTTPS3Streams *int diff --git a/core/dnsserver/config_test.go b/core/dnsserver/config_test.go index 9acd8e0df..dc88eacff 100644 --- a/core/dnsserver/config_test.go +++ b/core/dnsserver/config_test.go @@ -105,6 +105,19 @@ func TestPropagateConfigParamsMaxTCPQueries(t *testing.T) { } } +func TestPropagateConfigParamsMaxHTTPSStreams(t *testing.T) { + n := 7 + first := &Config{MaxHTTPSStreams: &n} + first.firstConfigInBlock = first + second := &Config{firstConfigInBlock: first} + + propagateConfigParams([]*Config{first, second}) + + if second.MaxHTTPSStreams == nil || *second.MaxHTTPSStreams != n { + t.Fatalf("expected MaxHTTPSStreams to propagate to second config as %d, got %v", n, second.MaxHTTPSStreams) + } +} + func TestPropagateConfigParamsAllowedOpcodes(t *testing.T) { first := &Config{} first.firstConfigInBlock = first diff --git a/core/dnsserver/register.go b/core/dnsserver/register.go index f85948d52..bdcaf9fe7 100644 --- a/core/dnsserver/register.go +++ b/core/dnsserver/register.go @@ -296,6 +296,10 @@ func propagateConfigParams(configs []*Config) { // Propagate UDPDecorateWriterFunc so a decorator configured once in a // server block applies to the block's UDP listener(s). c.UDPDecorateWriterFunc = c.firstConfigInBlock.UDPDecorateWriterFunc + + // Propagate MaxHTTPSStreams so a `https { max_streams N }` set once in a + // server block applies to the block's HTTPS key regardless of key order. + c.MaxHTTPSStreams = c.firstConfigInBlock.MaxHTTPSStreams } } diff --git a/core/dnsserver/server_https.go b/core/dnsserver/server_https.go index 8beab0e9e..06af08c46 100644 --- a/core/dnsserver/server_https.go +++ b/core/dnsserver/server_https.go @@ -27,6 +27,10 @@ import ( const ( // DefaultHTTPSMaxConnections is the default maximum number of concurrent connections. DefaultHTTPSMaxConnections = 200 + + // DefaultHTTPSMaxStreams is the default maximum number of concurrent HTTP/2 streams + // per connection, used when max_streams is not specified. + DefaultHTTPSMaxStreams = 250 ) // ServerHTTPS represents an instance of a DNS-over-HTTPS server. @@ -91,6 +95,31 @@ func NewServerHTTPS(addr string, group []*Config) (*ServerHTTPS, error) { IdleTimeout: s.IdleTimeout, ErrorLog: stdlog.New(&loggerAdapter{}, "", 0), } + // max_streams limits the number of concurrent HTTP/2 streams per connection. When unset, + // DefaultHTTPSMaxStreams is applied; a value of 0 leaves the underlying HTTP/2 transport + // default in place; a positive value sets the limit explicitly. The chosen value is + // advertised in the server's SETTINGS frame. Resolve across the whole group since blocks + // sharing a listener share one HTTP/2 server; conflicting explicit values are rejected. + maxStreams := DefaultHTTPSMaxStreams + var resolved *int + for _, conf := range group { + if conf == nil || conf.MaxHTTPSStreams == nil { + continue + } + if resolved != nil && *resolved != *conf.MaxHTTPSStreams { + return nil, fmt.Errorf("conflicting max_streams values for shared HTTPS listener %s: %d and %d", + addr, *resolved, *conf.MaxHTTPSStreams) + } + resolved = conf.MaxHTTPSStreams + } + if resolved != nil { + maxStreams = *resolved + } + if maxStreams > 0 { + srv.HTTP2 = &http.HTTP2Config{ + MaxConcurrentStreams: maxStreams, + } + } maxConnections := DefaultHTTPSMaxConnections if len(group) > 0 && group[0] != nil && group[0].MaxHTTPSConnections != nil { maxConnections = *group[0].MaxHTTPSConnections diff --git a/core/dnsserver/server_https_test.go b/core/dnsserver/server_https_test.go index 9c8b36bfc..0046ea456 100644 --- a/core/dnsserver/server_https_test.go +++ b/core/dnsserver/server_https_test.go @@ -120,6 +120,7 @@ func TestServerHTTPSRejectsUpdate(t *testing.T) { func TestNewServerHTTPSWithCustomLimits(t *testing.T) { maxConnections := 100 + maxStreams := 100 c := Config{ Zone: "example.com.", Transport: "https", @@ -127,6 +128,7 @@ func TestNewServerHTTPSWithCustomLimits(t *testing.T) { ListenHosts: []string{"127.0.0.1"}, Port: "443", MaxHTTPSConnections: &maxConnections, + MaxHTTPSStreams: &maxStreams, } server, err := NewServerHTTPS("127.0.0.1:443", []*Config{&c}) @@ -137,6 +139,12 @@ func TestNewServerHTTPSWithCustomLimits(t *testing.T) { if server.maxConnections != maxConnections { t.Errorf("Expected maxConnections = %d, got %d", maxConnections, server.maxConnections) } + if server.httpsServer.HTTP2 == nil { + t.Fatal("Expected HTTP/2 configuration") + } + if got := server.httpsServer.HTTP2.MaxConcurrentStreams; got != maxStreams { + t.Errorf("Expected MaxConcurrentStreams = %d, got %d", maxStreams, got) + } } func TestNewServerHTTPSDefaults(t *testing.T) { @@ -179,6 +187,122 @@ func TestNewServerHTTPSZeroLimits(t *testing.T) { } } +func TestNewServerHTTPSZeroStreams(t *testing.T) { + // max_streams 0 means "use the underlying HTTP/2 transport default": we must NOT + // explicitly configure HTTP/2 (so Go's built-in default of 250 applies). + zero := 0 + c := Config{ + Zone: "example.com.", + Transport: "https", + TLSConfig: &tls.Config{}, + ListenHosts: []string{"127.0.0.1"}, + Port: "443", + MaxHTTPSStreams: &zero, + } + + server, err := NewServerHTTPS("127.0.0.1:443", []*Config{&c}) + if err != nil { + t.Fatalf("NewServerHTTPS() with zero streams failed: %v", err) + } + + if server.httpsServer.HTTP2 != nil { + t.Errorf("Expected no explicit HTTP/2 configuration for max_streams 0 (transport default), got MaxConcurrentStreams = %d", + server.httpsServer.HTTP2.MaxConcurrentStreams) + } +} + +func TestNewServerHTTPSDefaultStreams(t *testing.T) { + // max_streams omitted means the CoreDNS default (DefaultHTTPSMaxStreams) is applied. + c := Config{ + Zone: "example.com.", + Transport: "https", + TLSConfig: &tls.Config{}, + ListenHosts: []string{"127.0.0.1"}, + Port: "443", + } + + server, err := NewServerHTTPS("127.0.0.1:443", []*Config{&c}) + if err != nil { + t.Fatalf("NewServerHTTPS() failed: %v", err) + } + + if server.httpsServer.HTTP2 == nil { + t.Fatal("Expected HTTP/2 configuration with the default max streams") + } + if got := server.httpsServer.HTTP2.MaxConcurrentStreams; got != DefaultHTTPSMaxStreams { + t.Errorf("Expected default MaxConcurrentStreams = %d, got %d", DefaultHTTPSMaxStreams, got) + } +} + +func TestNewServerHTTPSStreamsAcrossGroup(t *testing.T) { + // Blocks sharing a listener are passed as one group and share one HTTP/2 + // connection, so the limit must resolve to a single value regardless of + // which member carries it, and conflicting values must be rejected. + intPtr := func(v int) *int { return &v } + streamsConfig := func(zone string, v *int) *Config { + return &Config{ + Zone: zone, + Transport: "https", + TLSConfig: &tls.Config{}, + ListenHosts: []string{"127.0.0.1"}, + Port: "443", + MaxHTTPSStreams: v, + } + } + + tests := []struct { + name string + group []*Config + shouldErr bool + want int + }{ + { + name: "set on first member", + group: []*Config{streamsConfig("first.example.", intPtr(7)), streamsConfig("second.example.", nil)}, + want: 7, + }, + { + name: "set on second member", + group: []*Config{streamsConfig("first.example.", nil), streamsConfig("second.example.", intPtr(7))}, + want: 7, + }, + { + name: "omitted on all members", + group: []*Config{streamsConfig("first.example.", nil), streamsConfig("second.example.", nil)}, + want: DefaultHTTPSMaxStreams, + }, + { + name: "conflicting values", + group: []*Config{streamsConfig("first.example.", intPtr(7)), streamsConfig("second.example.", intPtr(9))}, + shouldErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + server, err := NewServerHTTPS("127.0.0.1:443", tc.group) + if tc.shouldErr { + if err == nil { + t.Fatal("Expected error for conflicting max_streams values, got nil") + } + if !strings.Contains(err.Error(), "conflicting max_streams") { + t.Errorf("Expected a conflicting max_streams error, got: %v", err) + } + return + } + if err != nil { + t.Fatalf("NewServerHTTPS() failed: %v", err) + } + if server.httpsServer.HTTP2 == nil { + t.Fatal("Expected HTTP/2 configuration") + } + if got := server.httpsServer.HTTP2.MaxConcurrentStreams; got != tc.want { + t.Errorf("Expected MaxConcurrentStreams = %d, got %d", tc.want, got) + } + }) + } +} + type contextCapturingPlugin struct { capturedContext context.Context contextCancelled bool diff --git a/plugin/https/README.md b/plugin/https/README.md index 938c2dbd2..86cb32076 100644 --- a/plugin/https/README.md +++ b/plugin/https/README.md @@ -14,33 +14,37 @@ This plugin can only be used once per HTTPS listener block. ```txt https { - max_connections POSITIVE_INTEGER + max_connections NON_NEGATIVE_INTEGER + max_streams NON_NEGATIVE_INTEGER } ``` * `max_connections` limits the number of concurrent TCP connections to the HTTPS server. The default value is 200 if not specified. Set to 0 for unbounded. +* `max_streams` limits the number of concurrent HTTP/2 streams per HTTPS connection. This helps prevent unbounded streams on a single connection, exhausting server resources. The default value is 250 if not specified. Set to 0 to use the underlying HTTP/2 transport default. ## Examples -Set custom limits for maximum connections: +Set custom limits for maximum connections and streams: ``` https://.:443 { tls cert.pem key.pem https { max_connections 100 + max_streams 100 } whoami } ``` -Set values to 0 for unbounded, matching CoreDNS behaviour before v1.14.0: +Set both values to 0 to disable the CoreDNS limits (unbounded connections and the underlying HTTP/2 transport stream default), matching CoreDNS behaviour before v1.14.0: ``` https://.:443 { tls cert.pem key.pem https { max_connections 0 + max_streams 0 } whoami } diff --git a/plugin/https/setup.go b/plugin/https/setup.go index 727a37861..a476cd0e1 100644 --- a/plugin/https/setup.go +++ b/plugin/https/setup.go @@ -1,6 +1,7 @@ package https import ( + "math" "strconv" "github.com/coredns/caddy" @@ -54,6 +55,25 @@ func parseDOH(c *caddy.Controller) error { return c.Err("max_connections already defined for this server block") } config.MaxHTTPSConnections = &val + case "max_streams": + args := c.RemainingArgs() + if len(args) != 1 { + return c.ArgErr() + } + val, err := strconv.Atoi(args[0]) + if err != nil { + return c.Errf("invalid max_streams value '%s': %v", args[0], err) + } + if val < 0 { + return c.Errf("max_streams must be a non-negative integer: %d", val) + } + if int64(val) > math.MaxUint32 { + return c.Errf("max_streams must not exceed %d: %d", uint64(math.MaxUint32), val) + } + if config.MaxHTTPSStreams != nil { + return c.Err("max_streams already defined for this server block") + } + config.MaxHTTPSStreams = &val default: return c.Errf("unknown property '%s'", c.Val()) } diff --git a/plugin/https/setup_test.go b/plugin/https/setup_test.go index cb7020a8d..890c2cfd8 100644 --- a/plugin/https/setup_test.go +++ b/plugin/https/setup_test.go @@ -2,6 +2,7 @@ package https import ( "fmt" + "strconv" "strings" "testing" @@ -10,11 +11,27 @@ import ( ) func TestSetup(t *testing.T) { + // The uint32 upper-bound boundary is architecture-dependent. On 64-bit int, + // 4294967295 is accepted and 4294967296 hits the "must not exceed" guard. On + // 32-bit int, both overflow and Atoi rejects them as "invalid max_streams" + // first. Derive the value via Atoi (never a constant literal) so the source + // stays portable to 32-bit targets. + maxUint32Err, maxUint32ErrContent := false, "" + var maxUint32Streams *int + overMaxUint32ErrContent := "must not exceed" + if v, err := strconv.Atoi("4294967295"); err == nil { + maxUint32Streams = &v + } else { + maxUint32Err, maxUint32ErrContent = true, "invalid max_streams value" + overMaxUint32ErrContent = "invalid max_streams value" + } + tests := []struct { input string shouldErr bool expectedErrContent string expectedMaxConnections *int + expectedMaxStreams *int }{ // Valid configurations { @@ -33,6 +50,22 @@ func TestSetup(t *testing.T) { shouldErr: false, expectedMaxConnections: intPtr(200), }, + { + input: `https { + max_streams 100 + }`, + shouldErr: false, + expectedMaxStreams: intPtr(100), + }, + { + input: `https { + max_connections 200 + max_streams 100 + }`, + shouldErr: false, + expectedMaxConnections: intPtr(200), + expectedMaxStreams: intPtr(100), + }, // Zero values (unbounded) { input: `https { @@ -71,6 +104,57 @@ func TestSetup(t *testing.T) { shouldErr: true, expectedErrContent: "already defined", }, + { + input: `https { + max_streams + }`, + shouldErr: true, + expectedErrContent: "Wrong argument count", + }, + { + input: `https { + max_streams abc + }`, + shouldErr: true, + expectedErrContent: "invalid max_streams value", + }, + { + input: `https { + max_streams 0 + }`, + shouldErr: false, + expectedMaxStreams: intPtr(0), + }, + { + input: `https { + max_streams -1 + }`, + shouldErr: true, + expectedErrContent: "must be a non-negative integer", + }, + { + input: `https { + max_streams 4294967295 + }`, + shouldErr: maxUint32Err, + expectedErrContent: maxUint32ErrContent, + expectedMaxStreams: maxUint32Streams, + }, + { + input: `https { + max_streams 4294967296 + }`, + shouldErr: true, + expectedErrContent: overMaxUint32ErrContent, + }, + { + input: `https { + max_streams 100 + max_streams 200 + }`, + shouldErr: true, + expectedErrContent: "already defined", + }, { input: `https { unknown_option 123 @@ -110,6 +194,7 @@ func TestSetup(t *testing.T) { if !test.shouldErr { config := dnsserver.GetConfig(c) assertIntPtrValue(t, i, test.input, "MaxHTTPSConnections", config.MaxHTTPSConnections, test.expectedMaxConnections) + assertIntPtrValue(t, i, test.input, "MaxHTTPSStreams", config.MaxHTTPSStreams, test.expectedMaxStreams) } } } diff --git a/test/https_test.go b/test/https_test.go index 2bf4940f6..a648e345a 100644 --- a/test/https_test.go +++ b/test/https_test.go @@ -3,12 +3,15 @@ package test import ( "bytes" "crypto/tls" + "encoding/binary" "io" "net" "net/http" "testing" "time" + "github.com/coredns/caddy" + "github.com/miekg/dns" ) @@ -175,3 +178,106 @@ func TestHTTPSConnectionLimit(t *testing.T) { } conns = append(conns, conn) } + +// TestHTTPSMaxStreamsKeyOrder verifies that max_streams applies regardless of the +// order of keys in a multi-transport server block. Caddy runs the https directive +// setup only for the first key, so without propagation the setting would be stored +// on the first key's config and dropped for the HTTPS key when it is listed second. +func TestHTTPSMaxStreamsKeyOrder(t *testing.T) { + const maxStreams = 7 + corefiles := map[string]string{ + "https_first": `https://.:0 .:0 { + bind 127.0.0.1 + tls ../plugin/tls/test_cert.pem ../plugin/tls/test_key.pem ../plugin/tls/test_ca.pem + https { + max_streams 7 + } + whoami +}`, + "https_second": `.:0 https://.:0 { + bind 127.0.0.1 + tls ../plugin/tls/test_cert.pem ../plugin/tls/test_key.pem ../plugin/tls/test_ca.pem + https { + max_streams 7 + } + whoami +}`, + } + + for name, corefile := range corefiles { + t.Run(name, func(t *testing.T) { + s, err := CoreDNSServer(corefile) + if err != nil { + t.Fatalf("Could not get CoreDNS serving instance: %s", err) + } + defer s.Stop() + + tcp := httpsListenerAddr(t, s) + if got := advertisedMaxConcurrentStreams(t, tcp); got != maxStreams { + t.Errorf("advertised SETTINGS_MAX_CONCURRENT_STREAMS = %d, want %d", got, maxStreams) + } + }) + } +} + +// httpsListenerAddr returns the TCP address of the instance's HTTPS listener. +// In a multi-transport block the HTTPS server is not necessarily first; it is +// the TCP-only listener (no packetconn), identified by a nil LocalAddr. +func httpsListenerAddr(t *testing.T, i *caddy.Instance) string { + t.Helper() + for _, s := range i.Servers() { + if s.LocalAddr() == nil && s.Addr() != nil { + return s.Addr().String() + } + } + t.Fatal("no HTTPS (TCP-only) listener found among started servers") + return "" +} + +// advertisedMaxConcurrentStreams opens a TLS/h2 connection to addr and returns the +// server's advertised SETTINGS_MAX_CONCURRENT_STREAMS. It returns 0 if the server +// does not send the setting. +func advertisedMaxConcurrentStreams(t *testing.T, addr string) uint32 { + t.Helper() + + conn, err := tls.Dial("tcp", addr, &tls.Config{InsecureSkipVerify: true, NextProtos: []string{"h2"}}) + if err != nil { + t.Fatalf("TLS dial %s failed: %v", addr, err) + } + defer conn.Close() + + if proto := conn.ConnectionState().NegotiatedProtocol; proto != "h2" { + t.Fatalf("ALPN did not negotiate h2, got %q", proto) + } + + if _, err := conn.Write([]byte("PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n")); err != nil { + t.Fatalf("write preface: %v", err) + } + if _, err := conn.Write([]byte{0, 0, 0, 0x4, 0, 0, 0, 0, 0}); err != nil { + t.Fatalf("write client SETTINGS: %v", err) + } + + _ = conn.SetReadDeadline(time.Now().Add(5 * time.Second)) + hdr := make([]byte, 9) + for { + if _, err := io.ReadFull(conn, hdr); err != nil { + t.Fatalf("read frame header: %v", err) + } + length := int(hdr[0])<<16 | int(hdr[1])<<8 | int(hdr[2]) + payload := make([]byte, length) + if length > 0 { + if _, err := io.ReadFull(conn, payload); err != nil { + t.Fatalf("read frame payload: %v", err) + } + } + if hdr[3] != 0x4 || hdr[4]&0x1 != 0 { // not a SETTINGS frame, or a SETTINGS ACK + continue + } + for i := 0; i+6 <= len(payload); i += 6 { + if binary.BigEndian.Uint16(payload[i:i+2]) == 0x3 { // SETTINGS_MAX_CONCURRENT_STREAMS + return binary.BigEndian.Uint32(payload[i+2 : i+6]) + } + } + return 0 + } +}