From 8a6ce4878186fbd35b623cc66117a88b005ef3e7 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Mon, 8 Feb 2021 15:49:56 +0100 Subject: [PATCH 1/2] Ssl: avoid tracking "unknown" ciphers Try to track the ssl.ciphers.xxx counters by cipher name instead of ssl.ciphers.unknown where possible. Signed-off-by: Otto van der Schaaf --- .../transport_sockets/tls/context_impl.cc | 17 ++++---- .../transport_sockets/tls/ssl_socket_test.cc | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index c0d7a1b8be..d0e950264c 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -398,13 +398,16 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const Envoy::Ssl::ContextConfig& c // 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"); - stat_name_set_->rememberBuiltin("TLS_AES_128_GCM_SHA256"); - + const STACK_OF(SSL_CIPHER) *ciphers = SSL_CTX_get_ciphers(tls_context_.ssl_ctx_.get()); + for (size_t i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { + const SSL_CIPHER *cipher = sk_SSL_CIPHER_value(ciphers, i); + stat_name_set_->rememberBuiltin(SSL_CIPHER_get_name(cipher)); + } + // 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 stat_name_set_->rememberBuiltins( diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index fc173ef26d..73bb9965b7 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -5938,6 +5938,48 @@ TEST_P(SslSocketTest, DISABLED_TestConnectionFailsOnMultipleCertificatesNonePass testUtil(test_options.setExpectedServerStats("ssl.ocsp_staple_failed").enableOcspStapling()); } +// Test a few ciphers and ensure they don't get tracked as "unknown" ciphers. +TEST_P(SslSocketTest, CipherUsageGetsCounted) { + envoy::config::listener::v3::Listener listener; + envoy::config::listener::v3::FilterChain* filter_chain = listener.add_filter_chains(); + envoy::extensions::transport_sockets::tls::v3::TlsCertificate* server_cert = + filter_chain->mutable_hidden_envoy_deprecated_tls_context() + ->mutable_common_tls_context() + ->add_tls_certificates(); + server_cert->mutable_certificate_chain()->set_filename(TestEnvironment::substitute( + "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem")); + server_cert->mutable_private_key()->set_filename(TestEnvironment::substitute( + "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_key.pem")); + envoy::extensions::transport_sockets::tls::v3::TlsParameters* server_params = + filter_chain->mutable_hidden_envoy_deprecated_tls_context() + ->mutable_common_tls_context() + ->mutable_tls_params(); + + envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext client; + envoy::extensions::transport_sockets::tls::v3::TlsParameters* client_params = + client.mutable_common_tls_context()->mutable_tls_params(); + + const std::vector ciphers = { + "ECDHE-RSA-CHACHA20-POLY1305", "ECDHE-RSA-AES128-GCM-SHA256", + "ECDHE-RSA-AES256-GCM-SHA384", "AES256-SHA256", + "AEAD-AES128-GCM-SHA256", "ECDHE-ECDSA-AES128-GCM-SHA256", + "ECDHE-RSA-AES128-SHA", "TLS_AES_128_GCM_SHA256", + "TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", + "TLS_AES_128_GCM_SHA256"}; + + for (const auto& cipher : ciphers) { + const std::string stats = "ssl.ciphers." + cipher; + client_params->clear_cipher_suites(); + server_params->clear_cipher_suites(); + client_params->add_cipher_suites(cipher); + server_params->add_cipher_suites(cipher); + TestUtilOptionsV2 cipher_test_options(listener, client, true, GetParam()); + cipher_test_options.setExpectedCiphersuite(cipher); + cipher_test_options.setExpectedServerStats(stats).setExpectedClientStats(stats); + testUtilV2(cipher_test_options); + } +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions From 56430850faf307a05f9e013515e31a6fd76da7cf Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Tue, 9 Feb 2021 00:44:46 +0100 Subject: [PATCH 2/2] Strip a few ciphers that behave differently with the openssl lib we target. Signed-off-by: Otto van der Schaaf --- test/extensions/transport_sockets/tls/ssl_socket_test.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 73bb9965b7..820f609b2c 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -5962,13 +5962,9 @@ TEST_P(SslSocketTest, CipherUsageGetsCounted) { const std::vector ciphers = { "ECDHE-RSA-CHACHA20-POLY1305", "ECDHE-RSA-AES128-GCM-SHA256", "ECDHE-RSA-AES256-GCM-SHA384", "AES256-SHA256", - "AEAD-AES128-GCM-SHA256", "ECDHE-ECDSA-AES128-GCM-SHA256", - "ECDHE-RSA-AES128-SHA", "TLS_AES_128_GCM_SHA256", - "TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384", - "TLS_AES_128_GCM_SHA256"}; - + "ECDHE-RSA-AES128-SHA"}; for (const auto& cipher : ciphers) { - const std::string stats = "ssl.ciphers." + cipher; + const std::string stats = "ssl.ciphers." + cipher; client_params->clear_cipher_suites(); server_params->clear_cipher_suites(); client_params->add_cipher_suites(cipher);