fix!: remove TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA cipher by default (coder#13837)

coadler · web-flow · commit af001773dbea · 2024-07-09T17:18:27.000Z
This cipher is included by default in Go as a fallback, but is marked as an insecure cipher. This removes the 3des cipher by default. Before: ``` $ nmap --script ssl-enum-ciphers -p 443 xxxxxxx Starting Nmap 7.94 ( https://nmap.org ) at 2024-07-08 14:16 CDT Nmap scan report for xxxxx (xxx.xxx.xxx.xxx) Host is up (0.038s latency). rDNS record for xxx.xxx.xxx.xxx: xxx.xxx.xxx.xxx.bc.googleusercontent.com PORT STATE SERVICE 443/tcp open https | ssl-enum-ciphers: | TLSv1.2: | ciphers: | TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A | TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A | TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA (secp256r1) - C | compressors: | NULL | cipher preference: server | warnings: | 64-bit block cipher 3DES vulnerable to SWEET32 attack | TLSv1.3: | ciphers: | TLS_AKE_WITH_AES_128_GCM_SHA256 (ecdh_x25519) - A | TLS_AKE_WITH_AES_256_GCM_SHA384 (ecdh_x25519) - A | TLS_AKE_WITH_CHACHA20_POLY1305_SHA256 (ecdh_x25519) - A | cipher preference: server |_ least strength: C ``` After: ``` $ nmap --script ssl-enum-ciphers -p 443 xxxxxxx Starting Nmap 7.94 ( https://nmap.org ) at 2024-07-08 15:04 CDT Nmap scan report for xxxxx (xxx.xxx.xxx.xxx) Host is up (0.039s latency). rDNS record for xxx.xxx.xxx.xxx: xxx.xxx.xxx.xxx.bc.googleusercontent.com PORT STATE SERVICE 443/tcp open https | ssl-enum-ciphers: | TLSv1.2: | ciphers: | TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (secp256r1) - A | TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (secp256r1) - A | TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (secp256r1) - A | compressors: | NULL | cipher preference: client | TLSv1.3: | ciphers: | TLS_AKE_WITH_AES_128_GCM_SHA256 (ecdh_x25519) - A | TLS_AKE_WITH_AES_256_GCM_SHA384 (ecdh_x25519) - A | TLS_AKE_WITH_CHACHA20_POLY1305_SHA256 (ecdh_x25519) - A | cipher preference: server |_ least strength: A ``` * fixup! fix!(cli): remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default * fixup! fix!(cli): remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default
diff --git a/.github/workflows/typos.toml b/.github/workflows/typos.toml
@@ -14,8 +14,12 @@ darcula = "darcula"
 Hashi = "Hashi"
 trialer = "trialer"
 encrypter = "encrypter"
-hel = "hel"             # as in helsinki
-pn = "pn"               # this is used as proto node
+# as in helsinki
+hel = "hel"
+# this is used as proto node
+pn = "pn"
+# typos doesn't like the EDE in TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
+EDE = "EDE"
 
 [files]
 extend-exclude = [
diff --git a/cli/server.go b/cli/server.go
@@ -1569,6 +1569,19 @@ func generateSelfSignedCertificate() (*tls.Certificate, error) {
 	return &cert, nil
 }
 
+// defaultCipherSuites is a list of safe cipher suites that we default to. This
+// is different from Golang's list of defaults, which unfortunately includes
+// `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA`.
+var defaultCipherSuites = func() []uint16 {
+	ret := []uint16{}
+
+	for _, suite := range tls.CipherSuites() {
+		ret = append(ret, suite.ID)
+	}
+
+	return ret
+}()
+
 // configureServerTLS returns the TLS config used for the Coderd server
 // connections to clients. A logger is passed in to allow printing warning
 // messages that do not block startup.
@@ -1599,6 +1612,8 @@ func configureServerTLS(ctx context.Context, logger slog.Logger, tlsMinVersion,
 			return nil, err
 		}
 		tlsConfig.CipherSuites = cipherIDs
+	} else {
+		tlsConfig.CipherSuites = defaultCipherSuites
 	}
 
 	switch tlsClientAuth {
diff --git a/cli/server_internal_test.go b/cli/server_internal_test.go
@@ -20,6 +20,28 @@ import (
 	"github.com/coder/serpent"
 )
 
+func Test_configureServerTLS(t *testing.T) {
+	t.Parallel()
+	t.Run("DefaultNoInsecureCiphers", func(t *testing.T) {
+		t.Parallel()
+		logger := slogtest.Make(t, nil)
+		cfg, err := configureServerTLS(context.Background(), logger, "tls12", "none", nil, nil, "", nil, false)
+		require.NoError(t, err)
+
+		require.NotEmpty(t, cfg)
+
+		insecureCiphers := tls.InsecureCipherSuites()
+		for _, cipher := range cfg.CipherSuites {
+			for _, insecure := range insecureCiphers {
+				if cipher == insecure.ID {
+					t.Logf("Insecure cipher found by default: %s", insecure.Name)
+					t.Fail()
+				}
+			}
+		}
+	})
+}
+
 func Test_configureCipherSuites(t *testing.T) {
 	t.Parallel()
 
