From d8d339740b5d818edfe944ef006fba9c974aa5cf Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Tue, 24 Jan 2017 08:15:25 -0700 Subject: [PATCH] New 'timeouts' directive to configure timeouts; default timeouts enabled (#1368) --- caddyhttp/caddyhttp.go | 1 + caddyhttp/caddyhttp_test.go | 2 +- caddyhttp/httpserver/plugin.go | 3 +- caddyhttp/httpserver/server.go | 78 +++++++++++++-- caddyhttp/httpserver/server_test.go | 99 +++++++++++++++++++ caddyhttp/httpserver/siteconfig.go | 32 +++++- caddyhttp/timeouts/timeouts.go | 106 ++++++++++++++++++++ caddyhttp/timeouts/timeouts_test.go | 147 ++++++++++++++++++++++++++++ 8 files changed, 457 insertions(+), 11 deletions(-) create mode 100644 caddyhttp/timeouts/timeouts.go create mode 100644 caddyhttp/timeouts/timeouts_test.go diff --git a/caddyhttp/caddyhttp.go b/caddyhttp/caddyhttp.go index f400443a2..0bda8d096 100644 --- a/caddyhttp/caddyhttp.go +++ b/caddyhttp/caddyhttp.go @@ -26,6 +26,7 @@ import ( _ "github.com/mholt/caddy/caddyhttp/root" _ "github.com/mholt/caddy/caddyhttp/status" _ "github.com/mholt/caddy/caddyhttp/templates" + _ "github.com/mholt/caddy/caddyhttp/timeouts" _ "github.com/mholt/caddy/caddyhttp/websocket" _ "github.com/mholt/caddy/startupshutdown" ) diff --git a/caddyhttp/caddyhttp_test.go b/caddyhttp/caddyhttp_test.go index fa9f7edf3..f8a5331cc 100644 --- a/caddyhttp/caddyhttp_test.go +++ b/caddyhttp/caddyhttp_test.go @@ -11,7 +11,7 @@ import ( // ensure that the standard plugins are in fact plugged in // and registered properly; this is a quick/naive way to do it. func TestStandardPlugins(t *testing.T) { - numStandardPlugins := 28 // importing caddyhttp plugs in this many plugins + numStandardPlugins := 29 // importing caddyhttp plugs in this many plugins s := caddy.DescribePlugins() if got, want := strings.Count(s, "\n"), numStandardPlugins+5; got != want { t.Errorf("Expected all standard plugins to be plugged in, got:\n%s", s) diff --git a/caddyhttp/httpserver/plugin.go b/caddyhttp/httpserver/plugin.go index d9ad8ab89..c1bd6a570 100644 --- a/caddyhttp/httpserver/plugin.go +++ b/caddyhttp/httpserver/plugin.go @@ -415,7 +415,8 @@ var directives = []string{ // primitive actions that set up the fundamental vitals of each config "root", "bind", - "maxrequestbody", + "maxrequestbody", // TODO: 'limits' + "timeouts", "tls", // services/utilities, or other directives that don't necessarily inject handlers diff --git a/caddyhttp/httpserver/server.go b/caddyhttp/httpserver/server.go index 7cc360402..b75e46ca0 100644 --- a/caddyhttp/httpserver/server.go +++ b/caddyhttp/httpserver/server.go @@ -40,13 +40,7 @@ var _ caddy.GracefulServer = new(Server) // and will serve the sites configured in group. func NewServer(addr string, group []*SiteConfig) (*Server, error) { s := &Server{ - Server: &http.Server{ - Addr: addr, - // TODO: Make these values configurable? - // ReadTimeout: 2 * time.Minute, - // WriteTimeout: 2 * time.Minute, - // MaxHeaderBytes: 1 << 16, - }, + Server: makeHTTPServer(addr, group), vhosts: newVHostTrie(), sites: group, connTimeout: GracefulTimeout, @@ -84,10 +78,10 @@ func NewServer(addr string, group []*SiteConfig) (*Server, error) { // Set up TLS configuration var tlsConfigs []*caddytls.Config - var err error for _, site := range group { tlsConfigs = append(tlsConfigs, site.TLS) } + var err error s.Server.TLSConfig, err = caddytls.MakeTLSConfig(tlsConfigs) if err != nil { return nil, err @@ -380,6 +374,74 @@ func (s *Server) OnStartupComplete() { } } +// defaultTimeouts stores the default timeout values to use +// if left unset by user configuration. Default timeouts, +// especially for ReadTimeout, are important for mitigating +// slowloris attacks. +var defaultTimeouts = Timeouts{ + ReadTimeout: 10 * time.Second, + ReadHeaderTimeout: 10 * time.Second, + WriteTimeout: 20 * time.Second, + IdleTimeout: 2 * time.Minute, +} + +// makeHTTPServer makes an http.Server from the group of configs +// in a way that configures timeouts (or, if not set, it uses the +// default timeouts) and other http.Server properties by combining +// the configuration of each SiteConfig in the group. (Timeouts +// are important for mitigating slowloris attacks.) +func makeHTTPServer(addr string, group []*SiteConfig) *http.Server { + s := &http.Server{Addr: addr} + + // find the minimum duration configured for each timeout + var min Timeouts + for _, cfg := range group { + if cfg.Timeouts.ReadTimeoutSet && + (!min.ReadTimeoutSet || cfg.Timeouts.ReadTimeout < min.ReadTimeout) { + min.ReadTimeoutSet = true + min.ReadTimeout = cfg.Timeouts.ReadTimeout + } + if cfg.Timeouts.ReadHeaderTimeoutSet && + (!min.ReadHeaderTimeoutSet || cfg.Timeouts.ReadHeaderTimeout < min.ReadHeaderTimeout) { + min.ReadHeaderTimeoutSet = true + min.ReadHeaderTimeout = cfg.Timeouts.ReadHeaderTimeout + } + if cfg.Timeouts.WriteTimeoutSet && + (!min.WriteTimeoutSet || cfg.Timeouts.WriteTimeout < min.WriteTimeout) { + min.WriteTimeoutSet = true + min.WriteTimeout = cfg.Timeouts.WriteTimeout + } + if cfg.Timeouts.IdleTimeoutSet && + (!min.IdleTimeoutSet || cfg.Timeouts.IdleTimeout < min.IdleTimeout) { + min.IdleTimeoutSet = true + min.IdleTimeout = cfg.Timeouts.IdleTimeout + } + } + + // for the values that were not set, use defaults + if !min.ReadTimeoutSet { + min.ReadTimeout = defaultTimeouts.ReadTimeout + } + if !min.ReadHeaderTimeoutSet { + min.ReadHeaderTimeout = defaultTimeouts.ReadHeaderTimeout + } + if !min.WriteTimeoutSet { + min.WriteTimeout = defaultTimeouts.WriteTimeout + } + if !min.IdleTimeoutSet { + min.IdleTimeout = defaultTimeouts.IdleTimeout + } + + // set the final values on the server + // TODO: ReadHeaderTimeout and IdleTimeout require Go 1.8 + s.ReadTimeout = min.ReadTimeout + // s.ReadHeaderTimeout = min.ReadHeaderTimeout + s.WriteTimeout = min.WriteTimeout + // s.IdleTimeout = min.IdleTimeout + + return s +} + // tcpKeepAliveListener sets TCP keep-alive timeouts on accepted // connections. It's used by ListenAndServe and ListenAndServeTLS so // dead TCP connections (e.g. closing laptop mid-download) eventually diff --git a/caddyhttp/httpserver/server_test.go b/caddyhttp/httpserver/server_test.go index d8e53c100..46b53925a 100644 --- a/caddyhttp/httpserver/server_test.go +++ b/caddyhttp/httpserver/server_test.go @@ -3,6 +3,7 @@ package httpserver import ( "net/http" "testing" + "time" ) func TestAddress(t *testing.T) { @@ -13,3 +14,101 @@ func TestAddress(t *testing.T) { t.Errorf("Expected '%s' but got '%s'", want, got) } } + +func TestMakeHTTPServer(t *testing.T) { + for i, tc := range []struct { + group []*SiteConfig + expected Timeouts + }{ + { + group: []*SiteConfig{{Timeouts: Timeouts{}}}, + expected: Timeouts{ + ReadTimeout: defaultTimeouts.ReadTimeout, + ReadHeaderTimeout: defaultTimeouts.ReadHeaderTimeout, + WriteTimeout: defaultTimeouts.WriteTimeout, + IdleTimeout: defaultTimeouts.IdleTimeout, + }, + }, + { + group: []*SiteConfig{{Timeouts: Timeouts{ + ReadTimeout: 1 * time.Second, + ReadTimeoutSet: true, + ReadHeaderTimeout: 2 * time.Second, + ReadHeaderTimeoutSet: true, + }}}, + expected: Timeouts{ + ReadTimeout: 1 * time.Second, + ReadHeaderTimeout: 2 * time.Second, + WriteTimeout: defaultTimeouts.WriteTimeout, + IdleTimeout: defaultTimeouts.IdleTimeout, + }, + }, + { + group: []*SiteConfig{{Timeouts: Timeouts{ + ReadTimeoutSet: true, + WriteTimeoutSet: true, + }}}, + expected: Timeouts{ + ReadTimeout: 0, + ReadHeaderTimeout: defaultTimeouts.ReadHeaderTimeout, + WriteTimeout: 0, + IdleTimeout: defaultTimeouts.IdleTimeout, + }, + }, + { + group: []*SiteConfig{ + {Timeouts: Timeouts{ + ReadTimeout: 2 * time.Second, + ReadTimeoutSet: true, + WriteTimeout: 2 * time.Second, + WriteTimeoutSet: true, + }}, + {Timeouts: Timeouts{ + ReadTimeout: 1 * time.Second, + ReadTimeoutSet: true, + WriteTimeout: 1 * time.Second, + WriteTimeoutSet: true, + }}, + }, + expected: Timeouts{ + ReadTimeout: 1 * time.Second, + ReadHeaderTimeout: defaultTimeouts.ReadHeaderTimeout, + WriteTimeout: 1 * time.Second, + IdleTimeout: defaultTimeouts.IdleTimeout, + }, + }, + { + group: []*SiteConfig{{Timeouts: Timeouts{ + ReadHeaderTimeout: 5 * time.Second, + ReadHeaderTimeoutSet: true, + IdleTimeout: 10 * time.Second, + IdleTimeoutSet: true, + }}}, + expected: Timeouts{ + ReadTimeout: defaultTimeouts.ReadTimeout, + ReadHeaderTimeout: 5 * time.Second, + WriteTimeout: defaultTimeouts.WriteTimeout, + IdleTimeout: 10 * time.Second, + }, + }, + } { + actual := makeHTTPServer("127.0.0.1:9005", tc.group) + + if got, want := actual.Addr, "127.0.0.1:9005"; got != want { + t.Errorf("Test %d: Expected Addr=%s, but was %s", i, want, got) + } + if got, want := actual.ReadTimeout, tc.expected.ReadTimeout; got != want { + t.Errorf("Test %d: Expected ReadTimeout=%v, but was %v", i, want, got) + } + // TODO: ReadHeaderTimeout and IdleTimeout require Go 1.8 + // if got, want := actual.ReadHeaderTimeout, tc.expected.ReadHeaderTimeout; got != want { + // t.Errorf("Test %d: Expected ReadHeaderTimeout=%v, but was %v", i, want, got) + // } + if got, want := actual.WriteTimeout, tc.expected.WriteTimeout; got != want { + t.Errorf("Test %d: Expected WriteTimeout=%v, but was %v", i, want, got) + } + // if got, want := actual.IdleTimeout, tc.expected.IdleTimeout; got != want { + // t.Errorf("Test %d: Expected IdleTimeout=%v, but was %v", i, want, got) + // } + } +} diff --git a/caddyhttp/httpserver/siteconfig.go b/caddyhttp/httpserver/siteconfig.go index e5e18b50b..e98801923 100644 --- a/caddyhttp/httpserver/siteconfig.go +++ b/caddyhttp/httpserver/siteconfig.go @@ -1,6 +1,10 @@ package httpserver -import "github.com/mholt/caddy/caddytls" +import ( + "time" + + "github.com/mholt/caddy/caddytls" +) // SiteConfig contains information about a site // (also known as a virtual host). @@ -36,6 +40,32 @@ type SiteConfig struct { // The path to the Caddyfile used to generate this site config originCaddyfile string + + // These timeout values are used, in conjunction with other + // site configs on the same server instance, to set the + // respective timeout values on the http.Server that + // is created. Sensible values will mitigate slowloris + // attacks and overcome faulty networks, while still + // preserving functionality needed for proxying, + // websockets, etc. + Timeouts Timeouts +} + +// Timeouts specify various timeouts for a server to use. +// If the assocated bool field is true, then the duration +// value should be treated literally (i.e. a zero-value +// duration would mean "no timeout"). If false, the duration +// was left unset, so a zero-value duration would mean to +// use a default value (even if default is non-zero). +type Timeouts struct { + ReadTimeout time.Duration + ReadTimeoutSet bool + ReadHeaderTimeout time.Duration + ReadHeaderTimeoutSet bool + WriteTimeout time.Duration + WriteTimeoutSet bool + IdleTimeout time.Duration + IdleTimeoutSet bool } // PathLimit is a mapping from a site's path to its corresponding diff --git a/caddyhttp/timeouts/timeouts.go b/caddyhttp/timeouts/timeouts.go new file mode 100644 index 000000000..2f1630388 --- /dev/null +++ b/caddyhttp/timeouts/timeouts.go @@ -0,0 +1,106 @@ +package timeouts + +import ( + "time" + + "github.com/mholt/caddy" + "github.com/mholt/caddy/caddyhttp/httpserver" +) + +func init() { + caddy.RegisterPlugin("timeouts", caddy.Plugin{ + ServerType: "http", + Action: setupTimeouts, + }) +} + +func setupTimeouts(c *caddy.Controller) error { + config := httpserver.GetConfig(c) + + for c.Next() { + var hasOptionalBlock bool + for c.NextBlock() { + hasOptionalBlock = true + + // ensure the kind of timeout is recognized + kind := c.Val() + if kind != "read" && kind != "header" && kind != "write" && kind != "idle" { + return c.Errf("unknown timeout '%s': must be read, header, write, or idle", kind) + } + + // parse the timeout duration + if !c.NextArg() { + return c.ArgErr() + } + if c.NextArg() { + // only one value permitted + return c.ArgErr() + } + var dur time.Duration + if c.Val() != "none" { + var err error + dur, err = time.ParseDuration(c.Val()) + if err != nil { + return c.Errf("%v", err) + } + if dur < 0 { + return c.Err("non-negative duration required for timeout value") + } + } + + // set this timeout's duration + switch kind { + case "read": + config.Timeouts.ReadTimeout = dur + config.Timeouts.ReadTimeoutSet = true + case "header": + config.Timeouts.ReadHeaderTimeout = dur + config.Timeouts.ReadHeaderTimeoutSet = true + case "write": + config.Timeouts.WriteTimeout = dur + config.Timeouts.WriteTimeoutSet = true + case "idle": + config.Timeouts.IdleTimeout = dur + config.Timeouts.IdleTimeoutSet = true + } + } + if !hasOptionalBlock { + // set all timeouts to the same value + + if !c.NextArg() { + return c.ArgErr() + } + if c.NextArg() { + // only one value permitted + return c.ArgErr() + } + val := c.Val() + + config.Timeouts.ReadTimeoutSet = true + config.Timeouts.ReadHeaderTimeoutSet = true + config.Timeouts.WriteTimeoutSet = true + config.Timeouts.IdleTimeoutSet = true + + if val == "none" { + config.Timeouts.ReadTimeout = 0 + config.Timeouts.ReadHeaderTimeout = 0 + config.Timeouts.WriteTimeout = 0 + config.Timeouts.IdleTimeout = 0 + } else { + dur, err := time.ParseDuration(val) + if err != nil { + return c.Errf("unknown timeout duration: %v", err) + } + if dur < 0 { + return c.Err("non-negative duration required for timeout value") + } + config.Timeouts.ReadTimeout = dur + config.Timeouts.ReadHeaderTimeout = dur + config.Timeouts.WriteTimeout = dur + config.Timeouts.IdleTimeout = dur + } + } + } + + return nil +} diff --git a/caddyhttp/timeouts/timeouts_test.go b/caddyhttp/timeouts/timeouts_test.go new file mode 100644 index 000000000..239f4a68c --- /dev/null +++ b/caddyhttp/timeouts/timeouts_test.go @@ -0,0 +1,147 @@ +package timeouts + +import ( + "testing" + "time" + + "github.com/mholt/caddy" + "github.com/mholt/caddy/caddyhttp/httpserver" +) + +func TestSetupTimeouts(t *testing.T) { + testCases := []struct { + input string + shouldErr bool + }{ + {input: "timeouts none", shouldErr: false}, + {input: "timeouts 5s", shouldErr: false}, + {input: "timeouts 0", shouldErr: false}, + {input: "timeouts { \n read 15s \n }", shouldErr: false}, + {input: "timeouts { \n read 15s \n idle 10s \n }", shouldErr: false}, + {input: "timeouts", shouldErr: true}, + {input: "timeouts 5s 10s", shouldErr: true}, + {input: "timeouts 12", shouldErr: true}, + {input: "timeouts -2s", shouldErr: true}, + {input: "timeouts { \n foo 1s \n }", shouldErr: true}, + {input: "timeouts { \n read \n }", shouldErr: true}, + {input: "timeouts { \n read 1s 2s \n }", shouldErr: true}, + {input: "timeouts { \n foo \n }", shouldErr: true}, + } + for i, tc := range testCases { + controller := caddy.NewTestController("", tc.input) + err := setupTimeouts(controller) + if tc.shouldErr && err == nil { + t.Errorf("Test %d: Expected an error, but did not have one", i) + } + if !tc.shouldErr && err != nil { + t.Errorf("Test %d: Did not expect error, but got: %v", i, err) + } + } +} + +func TestTimeoutsSetProperly(t *testing.T) { + testCases := []struct { + input string + expected httpserver.Timeouts + }{ + { + input: "timeouts none", + expected: httpserver.Timeouts{ + ReadTimeout: 0, ReadTimeoutSet: true, + ReadHeaderTimeout: 0, ReadHeaderTimeoutSet: true, + WriteTimeout: 0, WriteTimeoutSet: true, + IdleTimeout: 0, IdleTimeoutSet: true, + }, + }, + { + input: "timeouts {\n read 15s \n}", + expected: httpserver.Timeouts{ + ReadTimeout: 15 * time.Second, ReadTimeoutSet: true, + }, + }, + { + input: "timeouts {\n header 15s \n}", + expected: httpserver.Timeouts{ + ReadHeaderTimeout: 15 * time.Second, ReadHeaderTimeoutSet: true, + }, + }, + { + input: "timeouts {\n write 15s \n}", + expected: httpserver.Timeouts{ + WriteTimeout: 15 * time.Second, WriteTimeoutSet: true, + }, + }, + { + input: "timeouts {\n idle 15s \n}", + expected: httpserver.Timeouts{ + IdleTimeout: 15 * time.Second, IdleTimeoutSet: true, + }, + }, + { + input: "timeouts {\n idle 15s \n read 1m \n }", + expected: httpserver.Timeouts{ + IdleTimeout: 15 * time.Second, IdleTimeoutSet: true, + ReadTimeout: 1 * time.Minute, ReadTimeoutSet: true, + }, + }, + { + input: "timeouts {\n read none \n }", + expected: httpserver.Timeouts{ + ReadTimeout: 0, ReadTimeoutSet: true, + }, + }, + { + input: "timeouts {\n write 0 \n }", + expected: httpserver.Timeouts{ + WriteTimeout: 0, WriteTimeoutSet: true, + }, + }, + { + input: "timeouts {\n write 1s \n write 2s \n }", + expected: httpserver.Timeouts{ + WriteTimeout: 2 * time.Second, WriteTimeoutSet: true, + }, + }, + { + input: "timeouts 1s\ntimeouts 2s", + expected: httpserver.Timeouts{ + ReadTimeout: 2 * time.Second, ReadTimeoutSet: true, + ReadHeaderTimeout: 2 * time.Second, ReadHeaderTimeoutSet: true, + WriteTimeout: 2 * time.Second, WriteTimeoutSet: true, + IdleTimeout: 2 * time.Second, IdleTimeoutSet: true, + }, + }, + } + for i, tc := range testCases { + controller := caddy.NewTestController("", tc.input) + err := setupTimeouts(controller) + if err != nil { + t.Fatalf("Test %d: Did not expect error, but got: %v", i, err) + } + cfg := httpserver.GetConfig(controller) + if got, want := cfg.Timeouts.ReadTimeout, tc.expected.ReadTimeout; got != want { + t.Errorf("Test %d: Expected ReadTimeout=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.ReadTimeoutSet, tc.expected.ReadTimeoutSet; got != want { + t.Errorf("Test %d: Expected ReadTimeoutSet=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.ReadHeaderTimeout, tc.expected.ReadHeaderTimeout; got != want { + t.Errorf("Test %d: Expected ReadHeaderTimeout=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.ReadHeaderTimeoutSet, tc.expected.ReadHeaderTimeoutSet; got != want { + t.Errorf("Test %d: Expected ReadHeaderTimeoutSet=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.WriteTimeout, tc.expected.WriteTimeout; got != want { + t.Errorf("Test %d: Expected WriteTimeout=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.WriteTimeoutSet, tc.expected.WriteTimeoutSet; got != want { + t.Errorf("Test %d: Expected WriteTimeoutSet=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.IdleTimeout, tc.expected.IdleTimeout; got != want { + t.Errorf("Test %d: Expected IdleTimeout=%v, got %v", i, want, got) + } + if got, want := cfg.Timeouts.IdleTimeoutSet, tc.expected.IdleTimeoutSet; got != want { + t.Errorf("Test %d: Expected IdleTimeoutSet=%v, got %v", i, want, got) + } + } +}