From af001773dbead4602b97555321f6a411bdf2e8ae Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 9 Jul 2024 12:18:27 -0500 Subject: [PATCH] fix!: remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default (#13837) 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 --- .github/workflows/typos.toml | 8 ++++++-- cli/server.go | 15 +++++++++++++++ cli/server_internal_test.go | 22 ++++++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/.github/workflows/typos.toml b/.github/workflows/typos.toml index 7ee9554f0c..4197628dfd 100644 --- 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 index 6a35e8aaa9..9c80ab1d9b 100644 --- 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 index 4e4f3b01c6..cbfc60a1ff 100644 --- 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()