From 76319043398680e4699c460ac8fddc842be58927 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 14:12:24 -0500 Subject: [PATCH 1/7] add missing ciphers Signed-off-by: Asra Ali --- .../transport_sockets/tls/context_impl.cc | 5 +++-- .../transport_sockets/tls/context_impl_test.cc | 15 ++++++++++----- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 2ba6a6ee89be..dd05d771f770 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -472,8 +472,9 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // Add hardcoded cipher suites from the TLS 1.3 spec: // https://tools.ietf.org/html/rfc8446 - stat_name_set_->rememberBuiltins( - {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_AES_128_GCM_SHA256"}); + stat_name_set_->rememberBuiltins({"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", + "TLS_AES_128_CCM_SHA256", "TLS_AES_256_CCM_8_SHA256", + "TLS_CHACHA20_POLY1305_SHA256"}); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 4d0c0e204ccf..bbdb9e84919c 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1977,11 +1977,16 @@ class SslContextStatsTest : public SslContextImplTest { TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // Incrementing a value for a cipher that is part of the configuration works, and // we'll be able to find the value in the stats store. - context_->incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); - Stats::CounterOptConstRef cipher = - store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384"); - ASSERT_TRUE(cipher.has_value()); - EXPECT_EQ(1, cipher->get().value()); + for (const auto& cipher : + {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_AES_128_CCM_SHA256", + "TLS_AES_256_CCM_8_SHA256", "TLS_CHACHA20_POLY1305_SHA256"}) { + // Test all built-in TLS v1.3 cipher suites https://tools.ietf.org/html/rfc8446#appendix-B.4. + context_->incCounter("ssl.ciphers", cipher); + Stats::CounterOptConstRef stat = + store_.findCounterByString(absl::StrCat("ssl.ciphers.", cipher)); + ASSERT_TRUE(stat.has_value()); + EXPECT_EQ(1, stat->get().value()); + } // Incrementing a stat for a random unknown cipher does not work. A // rate-limited error log message will also be generated but that is hard to From f006801152f7b6cb89f3e70b7735454f36e0b7d4 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 14:18:39 -0500 Subject: [PATCH 2/7] update link Signed-off-by: Asra Ali --- source/extensions/transport_sockets/tls/context_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index dd05d771f770..0aa8ebeabf7d 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -471,7 +471,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c } // Add hardcoded cipher suites from the TLS 1.3 spec: - // https://tools.ietf.org/html/rfc8446 + // https://tools.ietf.org/html/rfc8446#appendix-B.4 stat_name_set_->rememberBuiltins({"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_AES_128_CCM_SHA256", "TLS_AES_256_CCM_8_SHA256", "TLS_CHACHA20_POLY1305_SHA256"}); From 5064b64d0d6b87225a77300cb2050c92c02804a0 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 14:38:26 -0500 Subject: [PATCH 3/7] remove aes-ccm Signed-off-by: Asra Ali --- source/extensions/transport_sockets/tls/context_impl.cc | 6 +++--- test/extensions/transport_sockets/tls/context_impl_test.cc | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 0aa8ebeabf7d..f81f2214c870 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -472,9 +472,9 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // Add hardcoded cipher suites from the TLS 1.3 spec: // https://tools.ietf.org/html/rfc8446#appendix-B.4 - stat_name_set_->rememberBuiltins({"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", - "TLS_AES_128_CCM_SHA256", "TLS_AES_256_CCM_8_SHA256", - "TLS_CHACHA20_POLY1305_SHA256"}); + // AES-CCM cipher suites are removed (no BoringSSL support). + stat_name_set_->rememberBuiltins( + {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"}); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index bbdb9e84919c..33fe8369129e 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1978,8 +1978,7 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // Incrementing a value for a cipher that is part of the configuration works, and // we'll be able to find the value in the stats store. for (const auto& cipher : - {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_AES_128_CCM_SHA256", - "TLS_AES_256_CCM_8_SHA256", "TLS_CHACHA20_POLY1305_SHA256"}) { + {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"}) { // Test all built-in TLS v1.3 cipher suites https://tools.ietf.org/html/rfc8446#appendix-B.4. context_->incCounter("ssl.ciphers", cipher); Stats::CounterOptConstRef stat = From 37251e7376bde53239902825888580b93248f466 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 14:38:57 -0500 Subject: [PATCH 4/7] add spelling Signed-off-by: Asra Ali --- tools/spelling/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 8d87a26db725..0819e488b28f 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -35,6 +35,7 @@ STATNAME SkyWalking TIDs ceil +CCM CHACHA CHLO CHMOD From 65b9605b5b58542c535cc56a1c21b40a5200e956 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 14:41:54 -0500 Subject: [PATCH 5/7] fix nit Signed-off-by: Asra Ali --- test/extensions/transport_sockets/tls/context_impl_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 33fe8369129e..43ddc1878765 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1979,7 +1979,8 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // we'll be able to find the value in the stats store. for (const auto& cipher : {"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", "TLS_CHACHA20_POLY1305_SHA256"}) { - // Test all built-in TLS v1.3 cipher suites https://tools.ietf.org/html/rfc8446#appendix-B.4. + // Test supported built-in TLS v1.3 cipher suites + // https://tools.ietf.org/html/rfc8446#appendix-B.4. context_->incCounter("ssl.ciphers", cipher); Stats::CounterOptConstRef stat = store_.findCounterByString(absl::StrCat("ssl.ciphers.", cipher)); From 1698431d748b31d3a382e7ce10ec2afe202c6747 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Tue, 12 Jan 2021 15:21:08 -0500 Subject: [PATCH 6/7] fix test Signed-off-by: Asra Ali --- test/extensions/transport_sockets/tls/context_impl_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 43ddc1878765..1ab0d5a9e8f3 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -2000,9 +2000,9 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // fallback registration does not occur. So we test for the fallback only in // release builds. #ifdef NDEBUG - cipher = store_.findCounterByString("ssl.ciphers.fallback"); - ASSERT_TRUE(cipher.has_value()); - EXPECT_EQ(1, cipher->get().value()); + stat = store_.findCounterByString("ssl.ciphers.fallback"); + ASSERT_TRUE(stat.has_value()); + EXPECT_EQ(1, stat->get().value()); #endif } From 0c856cf8398370c2291ffdce445f2ff9ed2f3885 Mon Sep 17 00:00:00 2001 From: Asra Ali Date: Wed, 13 Jan 2021 09:16:38 -0500 Subject: [PATCH 7/7] fix test Signed-off-by: Asra Ali --- test/extensions/transport_sockets/tls/context_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 1ab0d5a9e8f3..86571982f358 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -2000,7 +2000,7 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // fallback registration does not occur. So we test for the fallback only in // release builds. #ifdef NDEBUG - stat = store_.findCounterByString("ssl.ciphers.fallback"); + Stats::CounterOptConstRef stat = store_.findCounterByString("ssl.ciphers.fallback"); ASSERT_TRUE(stat.has_value()); EXPECT_EQ(1, stat->get().value()); #endif