diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index e79a1aeadb6dc..2ba6a6ee89be7 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -463,25 +463,23 @@ 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. + // Use the SSL library to iterate over the configured ciphers. + 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)); + } + } - // 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"); - stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); + // 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"}); // Curves from // https://github.com/google/boringssl/blob/f4d8b969200f1ee2dd872ffb85802e6a0976afe7/ssl/ssl_key_share.cc#L384 + // + // 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"}); @@ -640,14 +638,10 @@ 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(); - -#ifdef LOG_BUILTIN_STAT_NAMES - std::cerr << absl::StrCat("Builtin ", symbol_table.toString(name), ": ", value, "\n") - << std::flush; -#endif + const Stats::StatName value_stat_name = stat_name_set_->getBuiltin(value, fallback); + ENVOY_BUG(value_stat_name != fallback, + absl::StrCat("Unexpected ", scope_.symbolTable().toString(name), " value: ", value)); + Stats::Utility::counterFromElements(scope_, {name, value_stat_name}).inc(); } void ContextImpl::logHandshake(SSL* ssl) const { 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..4d0c0e204ccf6 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1934,6 +1934,73 @@ 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_ = 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_; + 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. + 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()); + + // 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. + 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", 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.has_value()); + EXPECT_EQ(1, cipher->get().value()); +#endif +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions