From 5b6ac035fff5d3b020d7cb45be7226e3313badf5 Mon Sep 17 00:00:00 2001 From: Michael Montgomery Date: Thu, 29 Oct 2020 12:38:19 -0500 Subject: [PATCH 1/3] Resolves #6074. Adds new option to configure HTTP Server's MaxHeaderBytes with option `-http-max-header-bytes` Adds tests for behavior --- agent/agent.go | 8 +++-- agent/agent_test.go | 57 ++++++++++++++++++++++++++++++++++++ agent/apiserver.go | 2 ++ agent/config/builder.go | 1 + agent/config/config.go | 1 + agent/config/flags.go | 1 + agent/config/runtime.go | 8 +++++ agent/config/runtime_test.go | 16 +++++++++- 8 files changed, 90 insertions(+), 4 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index ef0098f828..149998718f 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -775,9 +775,10 @@ func (a *Agent) listenHTTP() ([]apiServer, error) { a.configReloaders = append(a.configReloaders, srv.ReloadConfig) a.httpHandlers = srv httpServer := &http.Server{ - Addr: l.Addr().String(), - TLSConfig: tlscfg, - Handler: srv.handler(a.config.EnableDebug), + Addr: l.Addr().String(), + TLSConfig: tlscfg, + Handler: srv.handler(a.config.EnableDebug), + MaxHeaderBytes: a.config.HTTPMaxHeaderBytes, } // Load the connlimit helper into the server @@ -802,6 +803,7 @@ func (a *Agent) listenHTTP() ([]apiServer, error) { } return fmt.Errorf("%s server %s failed: %w", proto, l.Addr(), err) }, + MaxHeaderBytes: a.config.HTTPMaxHeaderBytes, }) } return nil diff --git a/agent/agent_test.go b/agent/agent_test.go index 73dc829604..833b8bb24e 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -239,6 +239,63 @@ func TestAgent_ReconnectConfigSettings(t *testing.T) { }() } +func TestAgent_HTTPMaxHeaderBytes(t *testing.T) { + tests := []struct { + name string + maxHeaderBytes int + expectError bool + expectedHTTPResponse int + }{ + { + "max header bytes 1 returns 431 http response when too large headers are sent", + 1, + false, + 431, + }, + { + "max header bytes 0 returns 200 http response, as the http.DefaultMaxHeaderBytes size of 1MB is used", + 0, + false, + 200, + }, + { + "negative maxHeaderBytes returns 200 http response, as the http.DefaultMaxHeaderBytes size of 1MB is used", + -10, + false, + 200, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := NewTestAgent(t, fmt.Sprintf(` + http_config { + max_header_bytes = %d + } + `, tt.maxHeaderBytes)) + defer a.Shutdown() + + require.Equal(t, tt.maxHeaderBytes, a.Agent.config.HTTPMaxHeaderBytes) + + req, err := http.NewRequest(http.MethodGet, "http://"+a.HTTPAddr()+"/v1/health/state/passing", nil) + require.NoError(t, err, "unexpected error creating new http request") + + // This is directly pulled from the testing of request limits in the net/http source + // https://github.com/golang/go/blob/go1.15.3/src/net/http/serve_test.go#L2897-L2900 + var bytesPerHeader = len("header12345: val12345\r\n") + t.Logf("bytesPerHeader: %d", bytesPerHeader) + for i := 0; i < ((tt.maxHeaderBytes+4096)/bytesPerHeader)+1; i++ { + req.Header.Set(fmt.Sprintf("header%05d", i), fmt.Sprintf("val%05d", i)) + } + + var res *http.Response + res, err = http.DefaultClient.Do(req) + require.True(t, (tt.expectError && (err != nil)) || !tt.expectError && (err == nil)) + require.Equal(t, tt.expectedHTTPResponse, res.StatusCode, "expected a '%d' http response, got '%d'", tt.expectedHTTPResponse, res.StatusCode) + }) + } + +} + func TestAgent_ReconnectConfigWanDisabled(t *testing.T) { t.Parallel() diff --git a/agent/apiserver.go b/agent/apiserver.go index 27087829a6..a187d226b0 100644 --- a/agent/apiserver.go +++ b/agent/apiserver.go @@ -37,6 +37,8 @@ type apiServer struct { Run func() error // Shutdown function used to stop the server Shutdown func(context.Context) error + + MaxHeaderBytes int } // NewAPIServers returns an empty apiServers that is ready to Start servers. diff --git a/agent/config/builder.go b/agent/config/builder.go index e2ec9ac037..b14f9d53bb 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -918,6 +918,7 @@ func (b *Builder) Build() (rt RuntimeConfig, err error) { HTTPAddrs: httpAddrs, HTTPSAddrs: httpsAddrs, HTTPBlockEndpoints: c.HTTPConfig.BlockEndpoints, + HTTPMaxHeaderBytes: b.intVal(c.HTTPConfig.MaxHeaderBytes), HTTPResponseHeaders: c.HTTPConfig.ResponseHeaders, AllowWriteHTTPFrom: b.cidrsVal("allow_write_http_from", c.HTTPConfig.AllowWriteHTTPFrom), HTTPUseCache: b.boolValWithDefault(c.HTTPConfig.UseCache, true), diff --git a/agent/config/config.go b/agent/config/config.go index b5254d7092..0a97215a32 100644 --- a/agent/config/config.go +++ b/agent/config/config.go @@ -613,6 +613,7 @@ type HTTPConfig struct { AllowWriteHTTPFrom []string `json:"allow_write_http_from,omitempty" hcl:"allow_write_http_from" mapstructure:"allow_write_http_from"` ResponseHeaders map[string]string `json:"response_headers,omitempty" hcl:"response_headers" mapstructure:"response_headers"` UseCache *bool `json:"use_cache,omitempty" hcl:"use_cache" mapstructure:"use_cache"` + MaxHeaderBytes *int `json:"max_header_bytes,omitempty" hcl:"max_header_bytes" mapstructure:"max_header_bytes"` } type Performance struct { diff --git a/agent/config/flags.go b/agent/config/flags.go index a032944d3e..9bce6c18f4 100644 --- a/agent/config/flags.go +++ b/agent/config/flags.go @@ -75,6 +75,7 @@ func AddFlags(fs *flag.FlagSet, f *BuilderOpts) { add(&f.Config.HTTPConfig.AllowWriteHTTPFrom, "allow-write-http-from", "Only allow write endpoint calls from given network. CIDR format, can be specified multiple times.") add(&f.Config.EncryptKey, "encrypt", "Provides the gossip encryption key.") add(&f.Config.Ports.GRPC, "grpc-port", "Sets the gRPC API port to listen on (currently needed for Envoy xDS only).") + add(&f.Config.HTTPConfig.MaxHeaderBytes, "http-max-header-bytes", "Sets the HTTP Server's Max Header Bytes.") add(&f.Config.Ports.HTTP, "http-port", "Sets the HTTP API port to listen on.") add(&f.Config.Ports.HTTPS, "https-port", "Sets the HTTPS API port to listen on.") add(&f.Config.StartJoinAddrsLAN, "join", "Address of an agent to join at start time. Can be specified multiple times.") diff --git a/agent/config/runtime.go b/agent/config/runtime.go index c3a7e3ec9b..5383ed5555 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -779,6 +779,14 @@ type RuntimeConfig struct { // hcl: limits{ http_max_conns_per_client = 200 } HTTPMaxConnsPerClient int + // HTTPMaxHeaderBytes controls the maximum number of bytes the + // server will read parsing the request header's keys and + // values, including the request line. It does not limit the + // size of the request body. + // + // If zero, or negative, http.DefaultMaxHeaderBytes is used. + HTTPMaxHeaderBytes int + // HTTPSHandshakeTimeout is the time allowed for HTTPS client to complete the // TLS handshake and send first bytes of the request. // diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index dc3a7a6d3f..4160108397 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -509,6 +509,17 @@ func TestBuilder_BuildAndValidate_ConfigFlagsAndEdgecases(t *testing.T) { rt.DataDir = dataDir }, }, + { + desc: "-http-max-header-bytes", + args: []string{ + `-http-max-header-bytes=1`, + `-data-dir=` + dataDir, + }, + patch: func(rt *RuntimeConfig) { + rt.HTTPMaxHeaderBytes = 1 + rt.DataDir = dataDir + }, + }, { desc: "-join", args: []string{ @@ -5074,7 +5085,8 @@ func TestFullConfig(t *testing.T) { "M6TKa9NP": "xjuxjOzQ", "JRCrHZed": "rl0mTx81" }, - "use_cache": false + "use_cache": false, + "max_header_bytes": 10 }, "key_file": "IEkkwgIA", "leave_on_terminate": true, @@ -5761,6 +5773,7 @@ func TestFullConfig(t *testing.T) { "JRCrHZed" = "rl0mTx81" } use_cache = false + max_header_bytes = 10 } key_file = "IEkkwgIA" leave_on_terminate = true @@ -6538,6 +6551,7 @@ func TestFullConfig(t *testing.T) { HTTPResponseHeaders: map[string]string{"M6TKa9NP": "xjuxjOzQ", "JRCrHZed": "rl0mTx81"}, HTTPSAddrs: []net.Addr{tcpAddr("95.17.17.19:15127")}, HTTPMaxConnsPerClient: 100, + HTTPMaxHeaderBytes: 10, HTTPSHandshakeTimeout: 2391 * time.Millisecond, HTTPSPort: 15127, HTTPUseCache: false, From 817903b925d43566c123b3d2b444f0df71891500 Mon Sep 17 00:00:00 2001 From: Michael Montgomery Date: Wed, 30 Dec 2020 14:03:48 -0600 Subject: [PATCH 2/3] Fixed failing tests Removed use of `NewTestAgent`, per review comment Removed CLI flag, per review comment Updated website documentation Added changelog entry --- .changelog/9067.txt | 3 ++ agent/agent_test.go | 76 +++++++++++++++++++--------- agent/config/flags.go | 1 - agent/config/runtime_test.go | 1 + website/pages/docs/agent/options.mdx | 2 + 5 files changed, 58 insertions(+), 25 deletions(-) create mode 100644 .changelog/9067.txt diff --git a/.changelog/9067.txt b/.changelog/9067.txt new file mode 100644 index 0000000000..cce35c2ba8 --- /dev/null +++ b/.changelog/9067.txt @@ -0,0 +1,3 @@ +```release-note:feature +agent: add config flag `MaxHeaderBytes` to control the maximum size of the http header per client request. +``` \ No newline at end of file diff --git a/agent/agent_test.go b/agent/agent_test.go index 0dfb223457..86dd8fc6d5 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -250,57 +250,85 @@ func TestAgent_HTTPMaxHeaderBytes(t *testing.T) { tests := []struct { name string maxHeaderBytes int - expectError bool expectedHTTPResponse int }{ { "max header bytes 1 returns 431 http response when too large headers are sent", 1, - false, 431, }, { "max header bytes 0 returns 200 http response, as the http.DefaultMaxHeaderBytes size of 1MB is used", 0, - false, 200, }, { "negative maxHeaderBytes returns 200 http response, as the http.DefaultMaxHeaderBytes size of 1MB is used", -10, - false, 200, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - a := NewTestAgent(t, fmt.Sprintf(` - http_config { - max_header_bytes = %d - } - `, tt.maxHeaderBytes)) - defer a.Shutdown() + ports, err := freeport.Take(1) + require.NoError(t, err) + t.Cleanup(func() { freeport.Return(ports) }) - require.Equal(t, tt.maxHeaderBytes, a.Agent.config.HTTPMaxHeaderBytes) + caConfig := tlsutil.Config{} + tlsConf, err := tlsutil.NewConfigurator(caConfig, hclog.New(nil)) + require.NoError(t, err) - req, err := http.NewRequest(http.MethodGet, "http://"+a.HTTPAddr()+"/v1/health/state/passing", nil) - require.NoError(t, err, "unexpected error creating new http request") + bd := BaseDeps{ + Deps: consul.Deps{ + Logger: hclog.NewInterceptLogger(nil), + Tokens: new(token.Store), + TLSConfigurator: tlsConf, + }, + RuntimeConfig: &config.RuntimeConfig{ + HTTPAddrs: []net.Addr{ + &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: ports[0]}, + }, + HTTPMaxHeaderBytes: tt.maxHeaderBytes, + }, + Cache: cache.New(cache.Options{}), + } + a, err := New(bd) + require.NoError(t, err) - // This is directly pulled from the testing of request limits in the net/http source - // https://github.com/golang/go/blob/go1.15.3/src/net/http/serve_test.go#L2897-L2900 - var bytesPerHeader = len("header12345: val12345\r\n") - t.Logf("bytesPerHeader: %d", bytesPerHeader) - for i := 0; i < ((tt.maxHeaderBytes+4096)/bytesPerHeader)+1; i++ { - req.Header.Set(fmt.Sprintf("header%05d", i), fmt.Sprintf("val%05d", i)) + srvs, err := a.listenHTTP() + require.NoError(t, err) + + require.Equal(t, tt.maxHeaderBytes, a.config.HTTPMaxHeaderBytes) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + t.Cleanup(cancel) + + g := new(errgroup.Group) + for _, s := range srvs { + g.Go(s.Run) } - var res *http.Response - res, err = http.DefaultClient.Do(req) - require.True(t, (tt.expectError && (err != nil)) || !tt.expectError && (err == nil)) - require.Equal(t, tt.expectedHTTPResponse, res.StatusCode, "expected a '%d' http response, got '%d'", tt.expectedHTTPResponse, res.StatusCode) + require.Len(t, srvs, 1) + + client := &http.Client{} + for _, s := range srvs { + u := url.URL{Scheme: s.Protocol, Host: s.Addr.String()} + req, err := http.NewRequest(http.MethodGet, u.String(), nil) + require.NoError(t, err) + + // This is directly pulled from the testing of request limits in the net/http source + // https://github.com/golang/go/blob/go1.15.3/src/net/http/serve_test.go#L2897-L2900 + var bytesPerHeader = len("header12345: val12345\r\n") + for i := 0; i < ((tt.maxHeaderBytes+4096)/bytesPerHeader)+1; i++ { + req.Header.Set(fmt.Sprintf("header%05d", i), fmt.Sprintf("val%05d", i)) + } + + resp, err := client.Do(req.WithContext(ctx)) + require.NoError(t, err) + require.Equal(t, tt.expectedHTTPResponse, resp.StatusCode, "expected a '%d' http response, got '%d'", tt.expectedHTTPResponse, resp.StatusCode) + } }) } - } func TestAgent_ReconnectConfigWanDisabled(t *testing.T) { diff --git a/agent/config/flags.go b/agent/config/flags.go index be57c23b32..77dc2abb38 100644 --- a/agent/config/flags.go +++ b/agent/config/flags.go @@ -75,7 +75,6 @@ func AddFlags(fs *flag.FlagSet, f *BuilderOpts) { add(&f.Config.HTTPConfig.AllowWriteHTTPFrom, "allow-write-http-from", "Only allow write endpoint calls from given network. CIDR format, can be specified multiple times.") add(&f.Config.EncryptKey, "encrypt", "Provides the gossip encryption key.") add(&f.Config.Ports.GRPC, "grpc-port", "Sets the gRPC API port to listen on (currently needed for Envoy xDS only).") - add(&f.Config.HTTPConfig.MaxHeaderBytes, "http-max-header-bytes", "Sets the HTTP Server's Max Header Bytes.") add(&f.Config.Ports.HTTP, "http-port", "Sets the HTTP API port to listen on.") add(&f.Config.Ports.HTTPS, "https-port", "Sets the HTTPS API port to listen on.") add(&f.Config.StartJoinAddrsLAN, "join", "Address of an agent to join at start time. Can be specified multiple times.") diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 5e95811fcd..53f59ae133 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -7675,6 +7675,7 @@ func TestSanitize(t *testing.T) { ], "HTTPBlockEndpoints": [], "HTTPMaxConnsPerClient": 0, + "HTTPMaxHeaderBytes": 0, "HTTPPort": 0, "HTTPResponseHeaders": {}, "HTTPUseCache": false, diff --git a/website/pages/docs/agent/options.mdx b/website/pages/docs/agent/options.mdx index df7f41e1b9..f96bd7dd26 100644 --- a/website/pages/docs/agent/options.mdx +++ b/website/pages/docs/agent/options.mdx @@ -1631,6 +1631,8 @@ Valid time units are 'ns', 'us' (or 'µs'), 'ms', 's', 'm', 'h'." - `use_cache` ((#http_config_use_cache)) Defaults to true. If disabled, the agent won't be using [agent caching](/api/features/caching) to answer the request. Even when the url parameter is provided. + - `max_header_bytes` This setting controls the maximum number of bytes the consul http server will read parsing the request header's keys and values, including the request line. It does not limit the size of the request body. If zero, or negative, http.DefaultMaxHeaderBytes is used, which equates to 1 Megabyte. + - `leave_on_terminate` If enabled, when the agent receives a TERM signal, it will send a `Leave` message to the rest of the cluster and gracefully leave. The default behavior for this feature varies based on whether or not the agent is running as a client or a server (prior to Consul 0.7 the default value was unconditionally set to `false`). On agents in client-mode, this defaults to `true` and for agents in server-mode, this defaults to `false`. - `limits` Available in Consul 0.9.3 and later, this is a nested From ba3fe0c87566b7842db9dbd0d57668d3070dd661 Mon Sep 17 00:00:00 2001 From: Michael Montgomery Date: Mon, 4 Jan 2021 19:47:13 -0600 Subject: [PATCH 3/3] Remove unneeded test --- agent/config/runtime_test.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 812eedae06..a5bdc2d737 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -514,17 +514,6 @@ func TestBuilder_BuildAndValidate_ConfigFlagsAndEdgecases(t *testing.T) { rt.DataDir = dataDir }, }, - { - desc: "-http-max-header-bytes", - args: []string{ - `-http-max-header-bytes=1`, - `-data-dir=` + dataDir, - }, - patch: func(rt *RuntimeConfig) { - rt.HTTPMaxHeaderBytes = 1 - rt.DataDir = dataDir - }, - }, { desc: "-join", args: []string{