New 'timeouts' directive to configure timeouts; default timeouts enabled (#1368)

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"
 )

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)

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

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{
+		Server:      makeHTTPServer(addr, group),
-			Addr: addr,
-			// TODO: Make these values configurable?
-			// ReadTimeout:    2 * time.Minute,
-			// WriteTimeout:   2 * time.Minute,
-			// MaxHeaderBytes: 1 << 16,
-		},
 		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

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)
+		// }
+	}
+}

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

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
+}

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)
+		}
+	}
+}