From 520f3d88e2b8d032912da29254475d2682888a99 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Thu, 20 Feb 2020 13:25:09 -0800 Subject: [PATCH] Update Envoy SHA to latest and enable TLSv1.3 in sni_verifier. (#17) Previously, SNI verifier didn't support TLSv1.3 and clients configured to use only TLSv1.3 were not recognized as TLS clients. Signed-off-by: Piotr Sikora --- WORKSPACE | 17 ++--- src/envoy/tcp/sni_verifier/sni_verifier.cc | 6 ++ src/envoy/tcp/sni_verifier/sni_verifier.h | 2 + .../tcp/sni_verifier/sni_verifier_test.cc | 67 ++++++++++++------- 4 files changed, 57 insertions(+), 35 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 2acbf245e71..3e8bb1e0fe8 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -37,13 +37,13 @@ bind( # 1. Determine SHA256 `wget https://github.com/istio/envoy/archive/$COMMIT.tar.gz && sha256sum $COMMIT.tar.gz` # 2. Update .bazelrc and .bazelversion files. # -# envoy commit date: 02/13/2020 -ENVOY_SHA = "3eb21010f9a72ab254742646f7e13385f21552d9" +# envoy commit date: 02/20/2020 +ENVOY_SHA = "f8ee9ce9785b46425ce34c94a8e741d386acc3f2" -ENVOY_SHA256 = "e7144c2dcb501f5dda217f42f012d42cd00c0e824a369016731ba6fb70af2720" - -LOCAL_ENVOY_PROJECT = "/PATH/TO/ENVOY" +ENVOY_SHA256 = "b7745d5324e646447614ca87f8081db7a8be5b52f8027c37fa179767a01cb303" +# To override with local envoy, just pass `--override_repository=envoy=/PATH/TO/ENVOY` to Bazel or +# persist the option in `user.bazelrc`. http_archive( name = "envoy", sha256 = ENVOY_SHA256, @@ -51,13 +51,6 @@ http_archive( url = "https://github.com/istio/envoy/archive/" + ENVOY_SHA + ".tar.gz", ) -# TODO(silentdai) Use bazel args to select envoy between local or http -# Uncomment below and comment above http_archive to depends on local envoy. -#local_repository( -# name = "envoy", -# path = LOCAL_ENVOY_PROJECT, -#) - load("@envoy//bazel:api_binding.bzl", "envoy_api_binding") envoy_api_binding() diff --git a/src/envoy/tcp/sni_verifier/sni_verifier.cc b/src/envoy/tcp/sni_verifier/sni_verifier.cc index 0ac5a7a808a..d3f854f7593 100644 --- a/src/envoy/tcp/sni_verifier/sni_verifier.cc +++ b/src/envoy/tcp/sni_verifier/sni_verifier.cc @@ -30,6 +30,10 @@ namespace Envoy { namespace Tcp { namespace SniVerifier { +// Min/max TLS version recognized by the underlying TLS/SSL library. +const unsigned Config::TLS_MIN_SUPPORTED_VERSION = TLS1_VERSION; +const unsigned Config::TLS_MAX_SUPPORTED_VERSION = TLS1_3_VERSION; + Config::Config(Stats::Scope& scope, size_t max_client_hello_size) : stats_{SNI_VERIFIER_STATS(POOL_COUNTER_PREFIX(scope, "sni_verifier."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), @@ -40,6 +44,8 @@ Config::Config(Stats::Scope& scope, size_t max_client_hello_size) max_client_hello_size_, size_t(TLS_MAX_CLIENT_HELLO))); } + SSL_CTX_set_min_proto_version(ssl_ctx_.get(), TLS_MIN_SUPPORTED_VERSION); + SSL_CTX_set_max_proto_version(ssl_ctx_.get(), TLS_MAX_SUPPORTED_VERSION); SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); SSL_CTX_set_tlsext_servername_callback( diff --git a/src/envoy/tcp/sni_verifier/sni_verifier.h b/src/envoy/tcp/sni_verifier/sni_verifier.h index 633a8d73d5b..bfcec841b28 100644 --- a/src/envoy/tcp/sni_verifier/sni_verifier.h +++ b/src/envoy/tcp/sni_verifier/sni_verifier.h @@ -55,6 +55,8 @@ class Config { size_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; + static const unsigned TLS_MIN_SUPPORTED_VERSION; + static const unsigned TLS_MAX_SUPPORTED_VERSION; private: SniVerifierStats stats_; diff --git a/src/envoy/tcp/sni_verifier/sni_verifier_test.cc b/src/envoy/tcp/sni_verifier/sni_verifier_test.cc index 6daf53c4d1f..8fd3ae2b0a0 100644 --- a/src/envoy/tcp/sni_verifier/sni_verifier_test.cc +++ b/src/envoy/tcp/sni_verifier/sni_verifier_test.cc @@ -53,9 +53,10 @@ TEST(SniVerifierTest, MaxClientHelloSize) { "max_client_hello_size of 65537 is greater than maximum of 65536."); } -class SniVerifierFilterTest : public testing::Test { +class SniVerifierFilterTest + : public testing::TestWithParam> { protected: - static constexpr size_t TLS_MAX_CLIENT_HELLO = 200; + static constexpr size_t TLS_MAX_CLIENT_HELLO = 250; void SetUp() override { store_ = std::make_unique(); @@ -69,10 +70,12 @@ class SniVerifierFilterTest : public testing::Test { store_ = nullptr; } - void runTestForClientHello(std::string outer_sni, std::string inner_sni, + void runTestForClientHello(uint16_t tls_min_version, uint16_t tls_max_version, + std::string outer_sni, std::string inner_sni, Network::FilterStatus expected_status, size_t data_installment_size = UINT_MAX) { - auto client_hello = Tls::Test::generateClientHello(inner_sni, ""); + auto client_hello = Tls::Test::generateClientHello( + tls_min_version, tls_max_version, inner_sni, ""); runTestForData(outer_sni, client_hello, expected_status, data_installment_size); } @@ -120,8 +123,18 @@ class SniVerifierFilterTest : public testing::Test { constexpr size_t SniVerifierFilterTest::TLS_MAX_CLIENT_HELLO; // definition -TEST_F(SniVerifierFilterTest, SnisMatch) { - runTestForClientHello("example.com", "example.com", +INSTANTIATE_TEST_SUITE_P( + TlsProtocolVersions, SniVerifierFilterTest, + testing::Values(std::make_tuple(Config::TLS_MIN_SUPPORTED_VERSION, + Config::TLS_MAX_SUPPORTED_VERSION), + std::make_tuple(TLS1_VERSION, TLS1_VERSION), + std::make_tuple(TLS1_1_VERSION, TLS1_1_VERSION), + std::make_tuple(TLS1_2_VERSION, TLS1_2_VERSION), + std::make_tuple(TLS1_3_VERSION, TLS1_3_VERSION))); + +TEST_P(SniVerifierFilterTest, SnisMatch) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "example.com", Network::FilterStatus::Continue); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -131,8 +144,9 @@ TEST_F(SniVerifierFilterTest, SnisMatch) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, SnisDoNotMatch) { - runTestForClientHello("example.com", "istio.io", +TEST_P(SniVerifierFilterTest, SnisDoNotMatch) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "istio.io", Network::FilterStatus::StopIteration); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -142,8 +156,9 @@ TEST_F(SniVerifierFilterTest, SnisDoNotMatch) { EXPECT_EQ(1, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, EmptyOuterSni) { - runTestForClientHello("", "istio.io", Network::FilterStatus::StopIteration); +TEST_P(SniVerifierFilterTest, EmptyOuterSni) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), "", + "istio.io", Network::FilterStatus::StopIteration); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); EXPECT_EQ(0, cfg_->stats().tls_not_found_.value()); @@ -152,8 +167,9 @@ TEST_F(SniVerifierFilterTest, EmptyOuterSni) { EXPECT_EQ(1, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, EmptyInnerSni) { - runTestForClientHello("example.com", "", +TEST_P(SniVerifierFilterTest, EmptyInnerSni) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "", Network::FilterStatus::StopIteration); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -163,8 +179,9 @@ TEST_F(SniVerifierFilterTest, EmptyInnerSni) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, BothSnisEmpty) { - runTestForClientHello("", "", Network::FilterStatus::StopIteration); +TEST_P(SniVerifierFilterTest, BothSnisEmpty) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), "", + "", Network::FilterStatus::StopIteration); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); EXPECT_EQ(0, cfg_->stats().tls_not_found_.value()); @@ -173,8 +190,9 @@ TEST_F(SniVerifierFilterTest, BothSnisEmpty) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, SniTooLarge) { - runTestForClientHello("example.com", std::string(TLS_MAX_CLIENT_HELLO, 'a'), +TEST_P(SniVerifierFilterTest, SniTooLarge) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", std::string(TLS_MAX_CLIENT_HELLO, 'a'), Network::FilterStatus::StopIteration); EXPECT_EQ(1, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(0, cfg_->stats().tls_found_.value()); @@ -184,8 +202,9 @@ TEST_F(SniVerifierFilterTest, SniTooLarge) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfTen) { - runTestForClientHello("example.com", "example.com", +TEST_P(SniVerifierFilterTest, SnisMatchSendDataInChunksOfTen) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "example.com", Network::FilterStatus::Continue, 10); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -195,8 +214,9 @@ TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfTen) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfFifty) { - runTestForClientHello("example.com", "example.com", +TEST_P(SniVerifierFilterTest, SnisMatchSendDataInChunksOfFifty) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "example.com", Network::FilterStatus::Continue, 50); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -206,8 +226,9 @@ TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfFifty) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfHundred) { - runTestForClientHello("example.com", "example.com", +TEST_P(SniVerifierFilterTest, SnisMatchSendDataInChunksOfHundred) { + runTestForClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), + "example.com", "example.com", Network::FilterStatus::Continue, 100); EXPECT_EQ(0, cfg_->stats().client_hello_too_large_.value()); EXPECT_EQ(1, cfg_->stats().tls_found_.value()); @@ -217,7 +238,7 @@ TEST_F(SniVerifierFilterTest, SnisMatchSendDataInChunksOfHundred) { EXPECT_EQ(0, cfg_->stats().snis_do_not_match_.value()); } -TEST_F(SniVerifierFilterTest, NonTLS) { +TEST_P(SniVerifierFilterTest, NonTLS) { std::vector nonTLSData(TLS_MAX_CLIENT_HELLO, 7); runTestForData("example.com", nonTLSData, Network::FilterStatus::StopIteration);