From 81d87dd8367bd8b01702d2ab994fa8d5898f1c7d Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Mon, 8 Jul 2024 20:26:58 +0000 Subject: [PATCH 1/3] fix!(cli): remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default 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 ``` --- cli/server.go | 15 +++++++++++++++ cli/server_internal_test.go | 22 ++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/cli/server.go b/cli/server.go index 6a35e8aaa95ea..9c80ab1d9b8c7 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 4e4f3b01c6ce5..cbfc60a1ff2d7 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() From 964d3bd6ce847b4a7e75f3c1431e57947de63051 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 9 Jul 2024 04:35:00 +0000 Subject: [PATCH 2/3] fixup! fix!(cli): remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default --- .github/workflows/typos.toml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/typos.toml b/.github/workflows/typos.toml index 7ee9554f0cdc3..061933c68e280 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 this +TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA = "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA" [files] extend-exclude = [ From fed28134d19afb6f6512a580c70a927850681f60 Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 9 Jul 2024 11:41:35 -0500 Subject: [PATCH 3/3] fixup! fix!(cli): remove `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` cipher by default --- .github/workflows/typos.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/typos.toml b/.github/workflows/typos.toml index 061933c68e280..4197628dfd7d0 100644 --- a/.github/workflows/typos.toml +++ b/.github/workflows/typos.toml @@ -18,8 +18,8 @@ encrypter = "encrypter" hel = "hel" # this is used as proto node pn = "pn" -# typos doesn't like the EDE in this -TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA = "TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA" +# typos doesn't like the EDE in TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA +EDE = "EDE" [files] extend-exclude = [