tls: Mainstream compatibility improvements, better security rating

config/setup/tls.go

@@ -6,7 +6,6 @@ import (
 	"strconv"
 	"strings"
 
-	"github.com/mholt/caddy/app"
 	"github.com/mholt/caddy/middleware"
 )
 
@@ -40,7 +39,6 @@ func TLS(c *Controller) (middleware.Middleware, error) {
 				value, ok := supportedProtocols[strings.ToLower(args[0])]
 				if !ok {
 					return nil, c.Errf("Wrong protocol name or protocol not supported '%s'", c.Val())
-
 				}
 				c.TLS.ProtocolMinVersion = value
 				value, ok = supportedProtocols[strings.ToLower(args[1])]
@@ -50,13 +48,10 @@ func TLS(c *Controller) (middleware.Middleware, error) {
 				c.TLS.ProtocolMaxVersion = value
 			case "ciphers":
 				for c.NextArg() {
-					value, ok := supportedCiphers[strings.ToUpper(c.Val())]
+					value, ok := supportedCiphersMap[strings.ToUpper(c.Val())]
 					if !ok {
 						return nil, c.Errf("Wrong cipher name or cipher not supported '%s'", c.Val())
 					}
-					if _, ok := http2CipherSuites[value]; app.Http2 && !ok {
-						return nil, c.Errf("Cipher suite %s is not allowed for HTTP/2", c.Val())
-					}
 					c.TLS.Ciphers = append(c.TLS.Ciphers, value)
 				}
 			case "cache":
@@ -65,7 +60,7 @@ func TLS(c *Controller) (middleware.Middleware, error) {
 				}
 				size, err := strconv.Atoi(c.Val())
 				if err != nil {
-					return nil, c.Errf("Cache parameter must be an number '%s': %v", c.Val(), err)
+					return nil, c.Errf("Cache parameter must be a number '%s': %v", c.Val(), err)
 				}
 				c.TLS.CacheSize = size
 			default:
@@ -76,19 +71,16 @@ func TLS(c *Controller) (middleware.Middleware, error) {
 
 	// If no ciphers provided, use all that Caddy supports for the protocol
 	if len(c.TLS.Ciphers) == 0 {
-		for _, v := range supportedCiphers {
-			if _, ok := http2CipherSuites[v]; !app.Http2 || ok {
-				c.TLS.Ciphers = append(c.TLS.Ciphers, v)
-			}
-		}
+		c.TLS.Ciphers = supportedCiphers
 	}
 
-	// If no ProtocolMin provided, set default MinVersion to TLSv1.1 for security reasons
+	// Not a cipher suite, but still important for mitigating protocol downgrade attacks
+	c.TLS.Ciphers = append(c.TLS.Ciphers, tls.TLS_FALLBACK_SCSV)
+
+	// Set default protocol min and max versions - must balance compatibility and security
 	if c.TLS.ProtocolMinVersion == 0 {
-		c.TLS.ProtocolMinVersion = tls.VersionTLS11
+		c.TLS.ProtocolMinVersion = tls.VersionTLS10
 	}
-
-	//If no ProtocolMax provided, use crypto/tls default MaxVersion(tls1.2)
 	if c.TLS.ProtocolMaxVersion == 0 {
 		c.TLS.ProtocolMaxVersion = tls.VersionTLS12
 	}
@@ -98,6 +90,9 @@ func TLS(c *Controller) (middleware.Middleware, error) {
 		c.TLS.CacheSize = 64
 	}
 
+	// Prefer server cipher suites
+	c.TLS.PreferServerCipherSuites = true
+
 	return nil, nil
 }
 
@@ -111,11 +106,17 @@ var supportedProtocols = map[string]uint16{
 	"tls1.2": tls.VersionTLS12,
 }
 
-// Map of supported ciphers.
+// Map of supported ciphers, used only for parsing config.
 //
 // Note that, at time of writing, HTTP/2 blacklists 276 cipher suites,
 // including all but two of the suites below (the two GCM suites).
-var supportedCiphers = map[string]uint16{
+// See https://http2.github.io/http2-spec/#BadCipherSuites
+//
+// TLS_FALLBACK_SCSV is not in this list because we manually ensure
+// it is always added (even though it is not technically a cipher suite).
+//
+// This map, like any map, is NOT ORDERED. Do not range over this map.
+var supportedCiphersMap = map[string]uint16{
 	"ECDHE-RSA-AES128-GCM-SHA256":   tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
 	"ECDHE-ECDSA-AES128-GCM-SHA256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 	"ECDHE-RSA-AES128-CBC-SHA":      tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
@@ -128,9 +129,21 @@ var supportedCiphers = map[string]uint16{
 	"RSA-3DES-EDE-CBC-SHA":          tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
 }
 
-// Set of cipher suites not blacklisted by HTTP/2 spec.
-// See https://http2.github.io/http2-spec/#BadCipherSuites
-var http2CipherSuites = map[uint16]struct{}{
-	tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256:   struct{}{},
-	tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256: struct{}{},
+// List of supported cipher suites in descending order of preference.
+// Ordering is very important! Getting the wrong order will break
+// mainstream clients, especially with HTTP/2.
+//
+// Note that TLS_FALLBACK_SCSV is not in this list since it is always
+// added manually.
+var supportedCiphers = []uint16{
+	tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+	tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+	tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+	tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+	tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+	tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+	tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+	tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+	tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+	tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
 }

config/setup/tls_test.go

@@ -3,8 +3,6 @@ package setup
 import (
 	"crypto/tls"
 	"testing"
-
-	"github.com/mholt/caddy/app"
 )
 
 func TestTLSParseBasic(t *testing.T) {
@@ -12,9 +10,10 @@ func TestTLSParseBasic(t *testing.T) {
 
 	_, err := TLS(c)
 	if err != nil {
-		t.Error("Expected no errors, but had an error")
+		t.Errorf("Expected no errors, got: %v", err)
 	}
 
+	// Basic checks
 	if c.TLS.Certificate != "cert.pem" {
 		t.Errorf("Expected certificate arg to be 'cert.pem', was '%s'", c.TLS.Certificate)
 	}
@@ -24,30 +23,48 @@ func TestTLSParseBasic(t *testing.T) {
 	if !c.TLS.Enabled {
 		t.Error("Expected TLS Enabled=true, but was false")
 	}
-}
 
-func TestTLSParseNoOptional(t *testing.T) {
-	c := newTestController(`tls cert.crt cert.key`)
-
-	_, err := TLS(c)
-	if err != nil {
-		t.Errorf("Expected no errors, got: %v", err)
+	// Security defaults
+	if c.TLS.ProtocolMinVersion != tls.VersionTLS10 {
+		t.Errorf("Expected 'tls1.0 (0x0301)' as ProtocolMinVersion, got %#v", c.TLS.ProtocolMinVersion)
+	}
+	if c.TLS.ProtocolMaxVersion != tls.VersionTLS12 {
+		t.Errorf("Expected 'tls1.2 (0x0303)' as ProtocolMaxVersion, got %v", c.TLS.ProtocolMaxVersion)
+	}
+	if c.TLS.CacheSize != 64 {
+		t.Errorf("Expected CacheSize 64, got %v", c.TLS.CacheSize)
 	}
 
-	if len(c.TLS.Ciphers) != len(supportedCiphers) {
-		t.Errorf("Expected %v Ciphers, got %v", len(supportedCiphers), len(c.TLS.Ciphers))
+	// Cipher checks
+	expectedCiphers := []uint16{
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
+		tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_256_CBC_SHA,
+		tls.TLS_RSA_WITH_AES_128_CBC_SHA,
+		tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA,
+		tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA,
+		tls.TLS_FALLBACK_SCSV,
 	}
 
-	if c.TLS.ProtocolMinVersion != tls.VersionTLS11 {
-		t.Errorf("Expected 'tls1.1 (0x0302)' as ProtocolMinVersion, got %#v", c.TLS.ProtocolMinVersion)
+	// Ensure count is correct (plus one for TLS_FALLBACK_SCSV)
+	if len(c.TLS.Ciphers) != len(supportedCiphers)+1 {
+		t.Errorf("Expected %v Ciphers (including TLS_FALLBACK_SCSV), got %v",
+			len(supportedCiphers)+1, len(c.TLS.Ciphers))
 	}
 
-	if c.TLS.ProtocolMaxVersion != tls.VersionTLS12 {
-		t.Errorf("Expected 'tls1.2 (0x0303)' as ProtocolMaxVersion, got %v", c.TLS.ProtocolMaxVersion)
+	// Ensure ordering is correct
+	for i, actual := range c.TLS.Ciphers {
+		if actual != expectedCiphers[i] {
+			t.Errorf("Expected cipher in position %d to be %0x, got %0x", i, expectedCiphers[i], actual)
+		}
 	}
 
-	if c.TLS.CacheSize != 64 {
-		t.Errorf("Expected CacheSize 64, got %v", c.TLS.CacheSize)
+	if !c.TLS.PreferServerCipherSuites {
+		t.Error("Expected PreferServerCipherSuites = true, but was false")
 	}
 }
 
@@ -88,8 +105,8 @@ func TestTLSParseWithOptionalParams(t *testing.T) {
 		t.Errorf("Expected 'tls1.2 (0x0302)' as ProtocolMaxVersion, got %#v", c.TLS.ProtocolMaxVersion)
 	}
 
-	if len(c.TLS.Ciphers) != 3 {
-		t.Errorf("Expected 3 Ciphers, got %v", len(c.TLS.Ciphers))
+	if len(c.TLS.Ciphers)-1 != 3 {
+		t.Errorf("Expected 3 Ciphers (not including TLS_FALLBACK_SCSV), got %v", len(c.TLS.Ciphers))
 	}
 
 	if c.TLS.CacheSize != 128 {
@@ -127,43 +144,3 @@ func TestTLSParseWithWrongOptionalParams(t *testing.T) {
 		t.Errorf("Expected errors, but no error returned")
 	}
 }
-
-func TestTLSParseWithHTTP2Requirements(t *testing.T) {
-	params := `tls cert.crt cert.key`
-	c := newTestController(params)
-
-	// With HTTP2, cipher suites should be limited
-	app.Http2 = true
-	_, err := TLS(c)
-	if err != nil {
-		t.Errorf("Expected no errors, got: %v", err)
-	}
-	if len(c.TLS.Ciphers) != len(http2CipherSuites) {
-		t.Errorf("With HTTP/2 on, expected %d supported ciphers, got %d",
-			len(http2CipherSuites), len(c.TLS.Ciphers))
-	}
-
-	params = `tls cert.crt cert.key {
-			ciphers RSA-AES128-CBC-SHA
-		}`
-	c = newTestController(params)
-	// Should not be able to specify a blacklisted cipher suite with HTTP2 on
-	_, err = TLS(c)
-	if err == nil {
-		t.Error("Expected an error because cipher suite is invalid for HTTP/2")
-	}
-
-	params = `tls cert.crt cert.key`
-	c = newTestController(params)
-
-	// Without HTTP2, cipher suites should not be as restricted
-	app.Http2 = false
-	_, err = TLS(c)
-	if err != nil {
-		t.Errorf("Expected no errors, got: %v", err)
-	}
-	if len(c.TLS.Ciphers) != len(supportedCiphers) {
-		t.Errorf("With HTTP/2 off, expected %d supported ciphers, got %d",
-			len(supportedCiphers), len(c.TLS.Ciphers))
-	}
-}

server/config.go

@@ -55,7 +55,7 @@ func (c Config) Address() string {
 
 // TLSConfig describes how TLS should be configured and used,
 // if at all. A certificate and key are both required.
-// Ciphers, Protocols and CacheSize are optional
+// The rest is optional.
 type TLSConfig struct {
 	Enabled                  bool
 	Certificate              string
@@ -64,4 +64,5 @@ type TLSConfig struct {
 	ProtocolMinVersion       uint16
 	ProtocolMaxVersion       uint16
 	CacheSize                int
+	PreferServerCipherSuites bool
 }

server/server.go

@@ -137,7 +137,7 @@ func ListenAndServeTLSWithSNI(srv *http.Server, tlsConfigs []TLSConfig) error {
 	config.MinVersion = tlsConfigs[0].ProtocolMinVersion
 	config.MaxVersion = tlsConfigs[0].ProtocolMaxVersion
 	config.CipherSuites = tlsConfigs[0].Ciphers
-	config.PreferServerCipherSuites = true
+	config.PreferServerCipherSuites = tlsConfigs[0].PreferServerCipherSuites
 
 	conn, err := net.Listen("tcp", addr)
 	if err != nil {