From d3455aed57fc7867b6a1999821205b079453c8a4 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 29 Dec 2020 10:45:23 -0500 Subject: [PATCH 1/9] Remember stats for configured ciphers. Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 40 +++++++++-------- .../transport_sockets/tls/context_impl.h | 2 + test/extensions/transport_sockets/tls/BUILD | 1 + .../tls/context_impl_test.cc | 44 +++++++++++++++++++ 4 files changed, 68 insertions(+), 19 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index e79a1aeadb6dc..120454c8f5f1b 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -463,27 +463,26 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); - // To enumerate the required builtin ciphers, curves, algorithms, and - // versions, uncomment '#define LOG_BUILTIN_STAT_NAMES' below, and run - // bazel test //test/extensions/transport_sockets/tls/... --test_output=streamed - // | grep " Builtin ssl." | sort | uniq - // #define LOG_BUILTIN_STAT_NAMES - // - // TODO(#8035): improve tooling to find any other built-ins needed to avoid - // contention. - - // Ciphers - stat_name_set_->rememberBuiltin("AEAD-AES128-GCM-SHA256"); - stat_name_set_->rememberBuiltin("ECDHE-ECDSA-AES128-GCM-SHA256"); - stat_name_set_->rememberBuiltin("ECDHE-RSA-AES128-GCM-SHA256"); - stat_name_set_->rememberBuiltin("ECDHE-RSA-AES128-SHA"); - stat_name_set_->rememberBuiltin("ECDHE-RSA-CHACHA20-POLY1305"); + // Ciphers are configured as a string delimited by ":", with equivalence + // groups given as "[alt1|alt2]", and exclusions preceded by "!". For the + // purposes of collecting stats -- we want to track all these names. We don't + // need to fully parse out the structure of the cipher suites -- just extract + // out the names. + for (absl::string_view cipher_suite : + absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]!"))) { + stat_name_set_->rememberBuiltin(cipher_suite); + } + + // This cipher is referenced in a test, though it's not super-obvious how. stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 stat_name_set_->rememberBuiltins( {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"}); + for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ":")) { + stat_name_set_->rememberBuiltin(curve); + } // Algorithms stat_name_set_->rememberBuiltins({"ecdsa_secp256r1_sha256", "rsa_pss_rsae_sha256"}); @@ -640,12 +639,15 @@ Envoy::Ssl::ClientValidationStatus ContextImpl::verifyCertificate( void ContextImpl::incCounter(const Stats::StatName name, absl::string_view value, const Stats::StatName fallback) const { - Stats::Counter& counter = Stats::Utility::counterFromElements( - scope_, {name, stat_name_set_->getBuiltin(value, fallback)}); - counter.inc(); + const Stats::StatName value_stat_name = stat_name_set_->getBuiltin(value, fallback); + if (value_stat_name == fallback) { + ENVOY_LOG_PERIODIC_MISC(error, std::chrono::minutes(1), "Unexpected {} value: {}", + scope_.symbolTable().toString(name), value); + } + Stats::Utility::counterFromElements(scope_, {name, value_stat_name}).inc(); #ifdef LOG_BUILTIN_STAT_NAMES - std::cerr << absl::StrCat("Builtin ", symbol_table.toString(name), ": ", value, "\n") + std::cerr << absl::StrCat("Builtin ", scope_.symbolTable().toString(name), ": ", value, "\n") << std::flush; #endif } diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h index c533e0cc9c4fe..06d239c851cb5 100644 --- a/source/extensions/transport_sockets/tls/context_impl.h +++ b/source/extensions/transport_sockets/tls/context_impl.h @@ -110,6 +110,8 @@ class ContextImpl : public virtual Envoy::Ssl::Context { bool verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediates, std::string& error_details); protected: + friend class SslContextStatsTest; + ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config, TimeSource& time_source); diff --git a/test/extensions/transport_sockets/tls/BUILD b/test/extensions/transport_sockets/tls/BUILD index c3d2275a271fb..df7b645a25412 100644 --- a/test/extensions/transport_sockets/tls/BUILD +++ b/test/extensions/transport_sockets/tls/BUILD @@ -56,6 +56,7 @@ envoy_cc_test( "//test/mocks/ssl:ssl_mocks", "//test/mocks/stats:stats_mocks", "//test/test_common:environment_lib", + "//test/test_common:logging_lib", "//test/test_common:network_utility_lib", "//test/test_common:registry_lib", "//test/test_common:simulated_time_system_lib", diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 49445e4ec616d..55641669a1e30 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1934,6 +1934,50 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod) "Certificate configuration can't have both private_key and private_key_provider"); } +class SslContextStatsTest : public SslContextImplTest { +protected: + SslContextStatsTest() { + TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context_); + client_context_config_ = + std::make_unique(tls_context_, factory_context_); + context_.reset(new ContextImpl(store_, *client_context_config_, time_system_)); + } + + void incCounter(absl::string_view name1, absl::string_view name2) { + Stats::StatNamePool pool(store_.symbolTable()); + context_->incCounter(pool.add(name1), name2, pool.add("fallback")); + } + + Stats::TestUtil::TestStore store_; + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context_; + std::unique_ptr client_context_config_; + std::unique_ptr context_; + const std::string yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + )EOF"; +}; + +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. + incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); + Stats::CounterOptConstRef cipher = + store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384"); + EXPECT_TRUE(cipher); + EXPECT_EQ(1, cipher->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 + // test as it is dependent on timing and test-ordering. + incCounter("ssl.ciphers", "unexpected"); + EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected")); +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions From a8f7b25efb5851f8fe8f20f840134dd88bf69d71 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 29 Dec 2020 17:12:14 -0500 Subject: [PATCH 2/9] clean up clang-tidy error and skip exclusion. Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 120454c8f5f1b..05bb40d2df500 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -467,20 +467,27 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // groups given as "[alt1|alt2]", and exclusions preceded by "!". For the // purposes of collecting stats -- we want to track all these names. We don't // need to fully parse out the structure of the cipher suites -- just extract - // out the names. + // out the names. We skip exclusions as those do not show up as entire + // stat-name segments, so they would not match. for (absl::string_view cipher_suite : - absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]!"))) { - stat_name_set_->rememberBuiltin(cipher_suite); + absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]"))) { + if (!cipher_suite.empty() && cipher_suite[0] != '!') { // skip exclusions. + stat_name_set_->rememberBuiltin(cipher_suite); + } } - // This cipher is referenced in a test, though it's not super-obvious how. + // This cipher is referenced in a test, though it's not obvious how or why. stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 + // + // TODO(jmarantz): consider whether we need this hard-coded list in addition + // to the ones from ecdhCurves below. Note there is no harm in remembering a + // curve more than once. stat_name_set_->rememberBuiltins( {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"}); - for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ":")) { + for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) { stat_name_set_->rememberBuiltin(curve); } From fd8518036cb05e2301f184951f1b539d7e066956 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 30 Dec 2020 14:52:29 -0500 Subject: [PATCH 3/9] Address review comments. Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 19 +++++++++--- .../transport_sockets/tls/context_impl.h | 2 -- .../tls/context_impl_test.cc | 30 +++++++++++++------ 3 files changed, 36 insertions(+), 15 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 05bb40d2df500..ece9cf83931af 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -465,20 +465,31 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // Ciphers are configured as a string delimited by ":", with equivalence // groups given as "[alt1|alt2]", and exclusions preceded by "!". For the - // purposes of collecting stats -- we want to track all these names. We don't - // need to fully parse out the structure of the cipher suites -- just extract + // purposes of collecting stats, we want to track all these names. We don't + // need to fully parse out the structure of the cipher suites. Just extract // out the names. We skip exclusions as those do not show up as entire // stat-name segments, so they would not match. for (absl::string_view cipher_suite : absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]"))) { - if (!cipher_suite.empty() && cipher_suite[0] != '!') { // skip exclusions. + if (!cipher_suite.empty() && cipher_suite[0] != '!') { // skip exclusions and empty strings. stat_name_set_->rememberBuiltin(cipher_suite); } } - // This cipher is referenced in a test, though it's not obvious how or why. + // This cipher is referenced from + // IpVersionsClientVersions/SslCertficateIntegrationTest.ServerRsa/IPv4_TLSv1_3, + // possibly due to the call to + // ClientSslTransportOptions().setSigningAlgorithmsForTest + // in test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc, function + // rsaOnlyClientOptions. stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); + // This cipher's appearance appears to be induced by setting "cipher_suites" to + // "TLS_RSA_WITH_AES_128_GCM_SHA256" in the configuration for + // SslSocketTest.RsaPrivateKeyProviderAsyncDecryptSuccess in + // test/extensions/transport_sockets/tls/ssl_socket_test.cc + stat_name_set_->rememberBuiltin("AES128-GCM-SHA256"); + // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 // diff --git a/source/extensions/transport_sockets/tls/context_impl.h b/source/extensions/transport_sockets/tls/context_impl.h index 06d239c851cb5..c533e0cc9c4fe 100644 --- a/source/extensions/transport_sockets/tls/context_impl.h +++ b/source/extensions/transport_sockets/tls/context_impl.h @@ -110,8 +110,6 @@ class ContextImpl : public virtual Envoy::Ssl::Context { bool verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediates, std::string& error_details); protected: - friend class SslContextStatsTest; - ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config, TimeSource& time_source); diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 55641669a1e30..6c8f486aa0238 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1934,24 +1934,36 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod) "Certificate configuration can't have both private_key and private_key_provider"); } +// Subclass ContextImpl so we can instantiate directly from tests, despite the +// constructor being protected. +class TestContextImpl : public ContextImpl { +public: + TestContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& config, + TimeSource& time_source) + : ContextImpl(scope, config, time_source), pool_(scope.symbolTable()), + fallback_(pool_.add("fallback")) {} + + void incCounter(absl::string_view name, absl::string_view value) { + ContextImpl::incCounter(pool_.add(name), value, fallback_); + } + + Stats::StatNamePool pool_; + const Stats::StatName fallback_; +}; + class SslContextStatsTest : public SslContextImplTest { protected: SslContextStatsTest() { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context_); client_context_config_ = std::make_unique(tls_context_, factory_context_); - context_.reset(new ContextImpl(store_, *client_context_config_, time_system_)); - } - - void incCounter(absl::string_view name1, absl::string_view name2) { - Stats::StatNamePool pool(store_.symbolTable()); - context_->incCounter(pool.add(name1), name2, pool.add("fallback")); + context_ = std::make_unique(store_, *client_context_config_, time_system_); } Stats::TestUtil::TestStore store_; envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context_; std::unique_ptr client_context_config_; - std::unique_ptr context_; + std::unique_ptr context_; const std::string yaml = R"EOF( common_tls_context: tls_certificates: @@ -1965,7 +1977,7 @@ 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. - incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); + context_->incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); Stats::CounterOptConstRef cipher = store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384"); EXPECT_TRUE(cipher); @@ -1974,7 +1986,7 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // 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 // test as it is dependent on timing and test-ordering. - incCounter("ssl.ciphers", "unexpected"); + context_->incCounter("ssl.ciphers", "unexpected"); EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected")); } From af644ed7c807e0458d2257707833149448a4920f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Dec 2020 10:24:48 -0500 Subject: [PATCH 4/9] Use SSL API to enumerate the ciphers rather than parsing the config, plus other review comments. Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 36 ++++++++----------- .../tls/context_impl_test.cc | 7 +++- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index ece9cf83931af..7cc26378a4314 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -463,39 +463,31 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); - // Ciphers are configured as a string delimited by ":", with equivalence - // groups given as "[alt1|alt2]", and exclusions preceded by "!". For the - // purposes of collecting stats, we want to track all these names. We don't - // need to fully parse out the structure of the cipher suites. Just extract - // out the names. We skip exclusions as those do not show up as entire - // stat-name segments, so they would not match. - for (absl::string_view cipher_suite : - absl::StrSplit(config.cipherSuites(), absl::ByAnyChar(":|[]"))) { - if (!cipher_suite.empty() && cipher_suite[0] != '!') { // skip exclusions and empty strings. - stat_name_set_->rememberBuiltin(cipher_suite); + // Use the SSL library to iterate over the configured ciphers. + { + bssl::UniquePtr ssl = newSsl(nullptr); + STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get()); + for (uint32_t i = 0, n = sk_SSL_CIPHER_num(ciphers); i < n; ++i) { + const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); + stat_name_set_->rememberBuiltin(SSL_CIPHER_get_name(cipher)); + ENVOY_LOG_MISC(error, SSL_CIPHER_get_name(cipher)); } } - // This cipher is referenced from + // TLS_AES_128_GCM_SHA256 is referenced from // IpVersionsClientVersions/SslCertficateIntegrationTest.ServerRsa/IPv4_TLSv1_3, // possibly due to the call to // ClientSslTransportOptions().setSigningAlgorithmsForTest // in test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc, function // rsaOnlyClientOptions. - stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); - - // This cipher's appearance appears to be induced by setting "cipher_suites" to - // "TLS_RSA_WITH_AES_128_GCM_SHA256" in the configuration for - // SslSocketTest.RsaPrivateKeyProviderAsyncDecryptSuccess in - // test/extensions/transport_sockets/tls/ssl_socket_test.cc - stat_name_set_->rememberBuiltin("AES128-GCM-SHA256"); + // + // We must also add two more that are hardcoded into the SSL 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"}); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 - // - // TODO(jmarantz): consider whether we need this hard-coded list in addition - // to the ones from ecdhCurves below. Note there is no harm in remembering a - // curve more than once. stat_name_set_->rememberBuiltins( {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"}); for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) { diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 6c8f486aa0238..fc262c1577e52 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1980,7 +1980,7 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { context_->incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); Stats::CounterOptConstRef cipher = store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384"); - EXPECT_TRUE(cipher); + ASSERT_TRUE(cipher); EXPECT_EQ(1, cipher->get().value()); // Incrementing a stat for a random unknown cipher does not work. A @@ -1988,6 +1988,11 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { // test as it is dependent on timing and test-ordering. context_->incCounter("ssl.ciphers", "unexpected"); EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected")); + + // We will account for the 'unexpected' cipher as "fallback". + cipher = store_.findCounterByString("ssl.ciphers.fallback"); + ASSERT_TRUE(cipher); + EXPECT_EQ(1, cipher->get().value()); } } // namespace Tls From ec3fb6881c5e400fff4f478d5789ae214ea407db Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Thu, 31 Dec 2020 20:17:09 -0500 Subject: [PATCH 5/9] work around clang-tidy warning about calling a virtual method from the ctor. Signed-off-by: Joshua Marantz --- source/extensions/transport_sockets/tls/context_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 7cc26378a4314..679969d795b23 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -464,8 +464,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); // Use the SSL library to iterate over the configured ciphers. - { - bssl::UniquePtr ssl = newSsl(nullptr); + if (!tls_contexts_.empty()) { + // We avoid calling newSsl here to avoid the clang-tidy error about calling + // a virtual method from the constructor. + bssl::UniquePtr ssl = bssl::UniquePtr(SSL_new(tls_contexts_[0].ssl_ctx_.get())); STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get()); for (uint32_t i = 0, n = sk_SSL_CIPHER_num(ciphers); i < n; ++i) { const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); From 9fb5df3fb632817f42a6efdba3f7011b376766bb Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Fri, 1 Jan 2021 10:31:18 -0500 Subject: [PATCH 6/9] allow stat-names from all tls contexts. Signed-off-by: Joshua Marantz --- source/extensions/transport_sockets/tls/context_impl.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 679969d795b23..cccaa674e9841 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -464,10 +464,8 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c parsed_alpn_protocols_ = parseAlpnProtocols(config.alpnProtocols()); // Use the SSL library to iterate over the configured ciphers. - if (!tls_contexts_.empty()) { - // We avoid calling newSsl here to avoid the clang-tidy error about calling - // a virtual method from the constructor. - bssl::UniquePtr ssl = bssl::UniquePtr(SSL_new(tls_contexts_[0].ssl_ctx_.get())); + for (TlsContext& tls_context : tls_contexts_) { + bssl::UniquePtr ssl = bssl::UniquePtr(SSL_new(tls_context.ssl_ctx_.get())); STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get()); for (uint32_t i = 0, n = sk_SSL_CIPHER_num(ciphers); i < n; ++i) { const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); From 9d7d0ed568a9d91bc83c4912861993f201448590 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sat, 2 Jan 2021 09:43:43 -0500 Subject: [PATCH 7/9] review comments Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index cccaa674e9841..6331240b7e846 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -465,29 +465,24 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // Use the SSL library to iterate over the configured ciphers. for (TlsContext& tls_context : tls_contexts_) { - bssl::UniquePtr ssl = bssl::UniquePtr(SSL_new(tls_context.ssl_ctx_.get())); - STACK_OF(SSL_CIPHER)* ciphers = SSL_get_ciphers(ssl.get()); - for (uint32_t i = 0, n = sk_SSL_CIPHER_num(ciphers); i < n; ++i) { - const SSL_CIPHER* cipher = sk_SSL_CIPHER_value(ciphers, i); + for (const SSL_CIPHER* cipher : SSL_CTX_get_ciphers(tls_context.ssl_ctx_.get())) { stat_name_set_->rememberBuiltin(SSL_CIPHER_get_name(cipher)); ENVOY_LOG_MISC(error, SSL_CIPHER_get_name(cipher)); } } - // TLS_AES_128_GCM_SHA256 is referenced from - // IpVersionsClientVersions/SslCertficateIntegrationTest.ServerRsa/IPv4_TLSv1_3, - // possibly due to the call to - // ClientSslTransportOptions().setSigningAlgorithmsForTest - // in test/extensions/transport_sockets/tls/integration/ssl_integration_test.cc, function - // rsaOnlyClientOptions. - // - // We must also add two more that are hardcoded into the SSL 1.3 spec: + // We must also add two more that are hardcoded into 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"}); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 + // + // We include these in case there are implicit ones available in the library, + // and we include the configured ones below to cover any new ones that are + // added to the library and configured by users. There is no harm in + // specifying any of these multiple times. stat_name_set_->rememberBuiltins( {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"}); for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) { From e5e31926e16362b775b900cf66f66a7483ecc22d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Sun, 3 Jan 2021 11:01:50 -0500 Subject: [PATCH 8/9] comment rewording Signed-off-by: Joshua Marantz --- 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 6331240b7e846..a56c0bac67dee 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 } } - // We must also add two more that are hardcoded into the TLS 1.3 spec: + // 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"}); From 2f3798047356ca6e0b00ec9b6d5d9f52dc715bd2 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 5 Jan 2021 17:08:19 -0500 Subject: [PATCH 9/9] review comments Signed-off-by: Joshua Marantz --- .../transport_sockets/tls/context_impl.cc | 21 ++++--------------- .../tls/context_impl_test.cc | 14 +++++++++---- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index a56c0bac67dee..2ba6a6ee89be7 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -467,7 +467,6 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c for (TlsContext& tls_context : tls_contexts_) { for (const SSL_CIPHER* cipher : SSL_CTX_get_ciphers(tls_context.ssl_ctx_.get())) { stat_name_set_->rememberBuiltin(SSL_CIPHER_get_name(cipher)); - ENVOY_LOG_MISC(error, SSL_CIPHER_get_name(cipher)); } } @@ -479,15 +478,10 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 // - // We include these in case there are implicit ones available in the library, - // and we include the configured ones below to cover any new ones that are - // added to the library and configured by users. There is no harm in - // specifying any of these multiple times. + // Note that if a curve is configured outside this set, we'll issue an ENVOY_BUG so + // it will hopefully be caught. stat_name_set_->rememberBuiltins( {"P-224", "P-256", "P-384", "P-521", "X25519", "CECPQ2", "CECPQ2b"}); - for (absl::string_view curve : absl::StrSplit(config.ecdhCurves(), ':')) { - stat_name_set_->rememberBuiltin(curve); - } // Algorithms stat_name_set_->rememberBuiltins({"ecdsa_secp256r1_sha256", "rsa_pss_rsae_sha256"}); @@ -645,16 +639,9 @@ Envoy::Ssl::ClientValidationStatus ContextImpl::verifyCertificate( void ContextImpl::incCounter(const Stats::StatName name, absl::string_view value, const Stats::StatName fallback) const { const Stats::StatName value_stat_name = stat_name_set_->getBuiltin(value, fallback); - if (value_stat_name == fallback) { - ENVOY_LOG_PERIODIC_MISC(error, std::chrono::minutes(1), "Unexpected {} value: {}", - scope_.symbolTable().toString(name), value); - } + ENVOY_BUG(value_stat_name != fallback, + absl::StrCat("Unexpected ", scope_.symbolTable().toString(name), " value: ", value)); Stats::Utility::counterFromElements(scope_, {name, value_stat_name}).inc(); - -#ifdef LOG_BUILTIN_STAT_NAMES - std::cerr << absl::StrCat("Builtin ", scope_.symbolTable().toString(name), ": ", value, "\n") - << std::flush; -#endif } void ContextImpl::logHandshake(SSL* ssl) const { diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index fc262c1577e52..4d0c0e204ccf6 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1980,19 +1980,25 @@ TEST_F(SslContextStatsTest, IncOnlyKnownCounters) { context_->incCounter("ssl.ciphers", "ECDHE-ECDSA-AES256-GCM-SHA384"); Stats::CounterOptConstRef cipher = store_.findCounterByString("ssl.ciphers.ECDHE-ECDSA-AES256-GCM-SHA384"); - ASSERT_TRUE(cipher); + ASSERT_TRUE(cipher.has_value()); EXPECT_EQ(1, cipher->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 // test as it is dependent on timing and test-ordering. - context_->incCounter("ssl.ciphers", "unexpected"); + EXPECT_DEBUG_DEATH(context_->incCounter("ssl.ciphers", "unexpected"), + "Unexpected ssl.ciphers value: unexpected"); EXPECT_FALSE(store_.findCounterByString("ssl.ciphers.unexpected")); - // We will account for the 'unexpected' cipher as "fallback". + // We will account for the 'unexpected' cipher as "fallback", however in debug + // mode that will not work as the ENVOY_BUG macro will assert first, thus the + // 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); + ASSERT_TRUE(cipher.has_value()); EXPECT_EQ(1, cipher->get().value()); +#endif } } // namespace Tls