From cba537fc3cf33a7d118f967f39e81914e3f2382a Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Thu, 23 Aug 2018 10:09:25 +0300 Subject: [PATCH 01/17] Add networkLevelRequestedServer to ReadFilterCallbacks Signed-off-by: ronenschafferibm --- include/envoy/network/filter.h | 10 ++++++++++ source/common/network/filter_manager_impl.h | 7 +++++++ 2 files changed, 17 insertions(+) diff --git a/include/envoy/network/filter.h b/include/envoy/network/filter.h index 87b982969fca1..540d377ddc07c 100644 --- a/include/envoy/network/filter.h +++ b/include/envoy/network/filter.h @@ -76,6 +76,16 @@ class ReadFilterCallbacks { * Set the currently selected upstream host for the connection. */ virtual void upstreamHost(Upstream::HostDescriptionConstSharedPtr host) PURE; + + /** + * Return the requested server name (e.g. SNI in TLS) of the network level, if any. + */ + virtual absl::string_view networkLevelRequestedServerName() PURE; + + /** + * Set the requested server name (e.g. SNI in TLS) of the network level, if any. + */ + virtual void networkLevelRequestedServerName(absl::string_view name) PURE; }; /** diff --git a/source/common/network/filter_manager_impl.h b/source/common/network/filter_manager_impl.h index b2e49b61fe021..f211017eb0c96 100644 --- a/source/common/network/filter_manager_impl.h +++ b/source/common/network/filter_manager_impl.h @@ -61,6 +61,12 @@ class FilterManagerImpl { void upstreamHost(Upstream::HostDescriptionConstSharedPtr host) override { parent_.host_description_ = host; } + absl::string_view networkLevelRequestedServerName() override { + return parent_.network_level_requested_server_name_; + } + void networkLevelRequestedServerName(absl::string_view name) override { + parent_.network_level_requested_server_name_ = name; + } FilterManagerImpl& parent_; ReadFilterSharedPtr filter_; @@ -73,6 +79,7 @@ class FilterManagerImpl { Connection& connection_; BufferSource& buffer_source_; + absl::string_view network_level_requested_server_name_; Upstream::HostDescriptionConstSharedPtr host_description_; std::list upstream_filters_; std::list downstream_filters_; From f9920ddef2044938454a7d922ccb3a18dccd7561 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Thu, 23 Aug 2018 13:18:32 +0300 Subject: [PATCH 02/17] Add initial work on inner SNI reader Signed-off-by: ronenschafferibm --- source/common/network/filter_manager_impl.h | 2 + source/extensions/extensions_build_config.bzl | 1 + .../filters/network/inner_sni_reader/BUILD | 34 +++++ .../network/inner_sni_reader/config.cc | 54 +++++++ .../inner_sni_reader/inner_sni_reader.cc | 142 ++++++++++++++++++ .../inner_sni_reader/inner_sni_reader.h | 94 ++++++++++++ .../filters/network/well_known_names.h | 2 + 7 files changed, 329 insertions(+) create mode 100644 source/extensions/filters/network/inner_sni_reader/BUILD create mode 100644 source/extensions/filters/network/inner_sni_reader/config.cc create mode 100644 source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc create mode 100644 source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h diff --git a/source/common/network/filter_manager_impl.h b/source/common/network/filter_manager_impl.h index f211017eb0c96..b4bf81b1dec17 100644 --- a/source/common/network/filter_manager_impl.h +++ b/source/common/network/filter_manager_impl.h @@ -62,6 +62,8 @@ class FilterManagerImpl { parent_.host_description_ = host; } absl::string_view networkLevelRequestedServerName() override { + // TODO: return an empty string and write a warning to log when inner SNI reader is not + // configured. return parent_.network_level_requested_server_name_; } void networkLevelRequestedServerName(absl::string_view name) override { diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index 555da8826383f..c9ecf69f592c2 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -64,6 +64,7 @@ EXTENSIONS = { "envoy.filters.network.echo": "//source/extensions/filters/network/echo:config", "envoy.filters.network.ext_authz": "//source/extensions/filters/network/ext_authz:config", "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config", + "envoy.filters.network.inner_sni_reader": "//source/extensions/filters/network/inner_sni_reader:config", "envoy.filters.network.mongo_proxy": "//source/extensions/filters/network/mongo_proxy:config", "envoy.filters.network.ratelimit": "//source/extensions/filters/network/ratelimit:config", "envoy.filters.network.rbac": "//source/extensions/filters/network/rbac:config", diff --git a/source/extensions/filters/network/inner_sni_reader/BUILD b/source/extensions/filters/network/inner_sni_reader/BUILD new file mode 100644 index 0000000000000..150170255312c --- /dev/null +++ b/source/extensions/filters/network/inner_sni_reader/BUILD @@ -0,0 +1,34 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "inner_sni_reader", + srcs = ["inner_sni_reader.cc"], + hdrs = ["inner_sni_reader.h"], + external_deps = ["ssl"], + deps = [ + "//include/envoy/buffer:buffer_interface", + "//include/envoy/network:connection_interface", + "//include/envoy/network:filter_interface", + "//source/common/common:assert_lib", + "//source/common/common:minimal_logger_lib", + ], +) + +envoy_cc_library( + name = "config", + srcs = ["config.cc"], + deps = [ + ":inner_sni_reader", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/network:well_known_names", + ], +) diff --git a/source/extensions/filters/network/inner_sni_reader/config.cc b/source/extensions/filters/network/inner_sni_reader/config.cc new file mode 100644 index 0000000000000..5e8f51cea4b84 --- /dev/null +++ b/source/extensions/filters/network/inner_sni_reader/config.cc @@ -0,0 +1,54 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "extensions/filters/network/inner_sni_reader/inner_sni_reader.h" +#include "extensions/filters/network/well_known_names.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace InnerSniReader { + +/** + * Config registration for the inner_sni_reader filter. @see NamedNetworkFilterConfigFactory. + */ +class InnerSniReaderConfigFactory : public Server::Configuration::NamedNetworkFilterConfigFactory { +public: + // NamedNetworkFilterConfigFactory + Network::FilterFactoryCb + createFilterFactory(const Json::Object&, + Server::Configuration::FactoryContext& context) override { + // TODO: call createFilterFactoryFromProto and remove duplicate code + ConfigSharedPtr filter_config(new Config(context.scope())); + return [filter_config](Network::FilterManager& filter_manager) -> void { + filter_manager.addReadFilter(std::make_shared(filter_config)); + }; + } + + Network::FilterFactoryCb + createFilterFactoryFromProto(const Protobuf::Message&, + Server::Configuration::FactoryContext& context) override { + ConfigSharedPtr filter_config(new Config(context.scope())); + return [filter_config](Network::FilterManager& filter_manager) -> void { + filter_manager.addReadFilter(std::make_shared(filter_config)); + }; + } + + ProtobufTypes::MessagePtr createEmptyConfigProto() override { + return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()}; + } + + std::string name() override { return NetworkFilterNames::get().InnerSniReader; } +}; + +/** + * Static registration for the echo filter. @see RegisterFactory. + */ +static Registry::RegisterFactory + registered_; + +} // namespace InnerSniReader +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc b/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc new file mode 100644 index 0000000000000..94940c7ac09cd --- /dev/null +++ b/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc @@ -0,0 +1,142 @@ +#include "extensions/filters/network/inner_sni_reader/inner_sni_reader.h" + +#include "envoy/buffer/buffer.h" +#include "envoy/common/exception.h" +#include "envoy/network/connection.h" +#include "envoy/stats/scope.h" + +#include "common/common/assert.h" + +#include "openssl/bytestring.h" +#include "openssl/ssl.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace InnerSniReader { + +Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) + : stats_{ALL_INNER_SNI_READER_STATS(POOL_COUNTER_PREFIX(scope, "inner_sni_reader."))}, + ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), + max_client_hello_size_(max_client_hello_size) { + + if (max_client_hello_size_ > TLS_MAX_CLIENT_HELLO) { + throw EnvoyException(fmt::format("max_client_hello_size of {} is greater than maximum of {}.", + max_client_hello_size_, size_t(TLS_MAX_CLIENT_HELLO))); + } + + SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); + SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); + // TODO: Remove following +// SSL_CTX_set_select_certificate_cb( +// ssl_ctx_.get(), [](const SSL_CLIENT_HELLO* client_hello) -> ssl_select_cert_result_t { +// const uint8_t* data; +// size_t len; +// if (SSL_early_callback_ctx_extension_get( +// client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) { +// InnerSniReaderFilter* filter = static_cast(SSL_get_app_data(client_hello->ssl)); +//// filter->onALPN(data, len); +// if (filter != nullptr) {} +// +// } +// return ssl_select_cert_success; +// }); + SSL_CTX_set_tlsext_servername_callback( + ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { + InnerSniReaderFilter* filter = static_cast(SSL_get_app_data(ssl)); + filter->onServername(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)); + if (filter != nullptr) {} + + // Return an error to stop the handshake; we have what we wanted already. + *out_alert = SSL_AD_USER_CANCELLED; + return SSL_TLSEXT_ERR_ALERT_FATAL; + }); +} + +bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ctx_.get())}; } + +thread_local uint8_t InnerSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HELLO]; + +InnerSniReaderFilter::InnerSniReaderFilter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { + RELEASE_ASSERT(sizeof(buf_) >= config_->maxClientHelloSize(), ""); + + SSL_set_app_data(ssl_.get(), this); + SSL_set_accept_state(ssl_.get()); +} + +Network::FilterStatus InnerSniReaderFilter::onData(Buffer::Instance& data, bool) { + ENVOY_CONN_LOG(trace, "InnerSniReader: got {} bytes", read_callbacks_->connection(), data.length()); + + // TODO: append data to the buffer instead of overwriting it. + size_t len = (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO ; + data.copyOut(0, len, buf_); + + parseClientHello(buf_, len); + + return Network::FilterStatus::Continue; +} + +void InnerSniReaderFilter::onServername(absl::string_view name) { + ENVOY_CONN_LOG(debug, "inner sni reader: servername: {}", read_callbacks_->connection(), name); + if (!name.empty()) { + config_->stats().sni_found_.inc(); + read_callbacks_->networkLevelRequestedServerName(name); + } else { + config_->stats().sni_not_found_.inc(); + } + clienthello_success_ = true; +} + +void InnerSniReaderFilter::parseClientHello(const void* data, size_t len) { + // Ownership is passed to ssl_ in SSL_set_bio() + bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); + + // Make the mem-BIO return that there is more data + // available beyond it's end + BIO_set_mem_eof_return(bio.get(), -1); + + SSL_set_bio(ssl_.get(), bio.get(), bio.get()); + bio.release(); + + int ret = SSL_do_handshake(ssl_.get()); + + // This should never succeed because an error is always returned from the SNI callback. + ASSERT(ret <= 0); + switch (SSL_get_error(ssl_.get(), ret)) { + case SSL_ERROR_WANT_READ: + if (read_ == config_->maxClientHelloSize()) { + // We've hit the specified size limit. This is an unreasonably large ClientHello; + // indicate failure. + config_->stats().client_hello_too_large_.inc(); + done(false); + } + break; + case SSL_ERROR_SSL: + if (clienthello_success_) { + config_->stats().tls_found_.inc(); + if (alpn_found_) { + config_->stats().alpn_found_.inc(); + } else { + config_->stats().alpn_not_found_.inc(); + } +// cb_->socket().setDetectedTransportProtocol(TransportSockets::TransportSocketNames::get().Tls); + } else { + config_->stats().tls_not_found_.inc(); + } + done(true); + break; + default: + done(false); + break; + } +} + +void InnerSniReaderFilter::done(bool success) { + ENVOY_LOG(trace, "inner sni reader: done: {}", success); + read_callbacks_->continueReading(); +} + +} // namespace InnerSniReader +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h b/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h new file mode 100644 index 0000000000000..88d8cb5b0d3fd --- /dev/null +++ b/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h @@ -0,0 +1,94 @@ +#pragma once + +#include "envoy/network/filter.h" +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" + +#include "common/common/logger.h" + +#include "openssl/bytestring.h" +#include "openssl/ssl.h" + +namespace Envoy { +namespace Extensions { +namespace NetworkFilters { +namespace InnerSniReader { + +/** + * All stats for the inner SNI reader. @see stats_macros.h + */ +// TODO: Remove unnecessary stats +#define ALL_INNER_SNI_READER_STATS(COUNTER) \ + COUNTER(connection_closed) \ + COUNTER(client_hello_too_large) \ + COUNTER(read_error) \ + COUNTER(read_timeout) \ + COUNTER(tls_found) \ + COUNTER(tls_not_found) \ + COUNTER(alpn_found) \ + COUNTER(alpn_not_found) \ + COUNTER(sni_found) \ + COUNTER(sni_not_found) + +/** + * Definition of all stats for the inner SNI reader. @see stats_macros.h + */ +struct InnerSniReaderStats { + ALL_INNER_SNI_READER_STATS(GENERATE_COUNTER_STRUCT) +}; + +/** + * Global configuration for inner SNI reader. + */ +class Config { +public: + Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); + + const InnerSniReaderStats& stats() const { return stats_; } + bssl::UniquePtr newSsl(); + uint32_t maxClientHelloSize() const { return max_client_hello_size_; } + + static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; + +private: + InnerSniReaderStats stats_; + bssl::UniquePtr ssl_ctx_; + const uint32_t max_client_hello_size_; +}; + +typedef std::shared_ptr ConfigSharedPtr; + +class InnerSniReaderFilter : public Network::ReadFilter, Logger::Loggable { +public: + InnerSniReaderFilter(const ConfigSharedPtr config); + + // Network::ReadFilter + Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; + Network::FilterStatus onNewConnection() override { return Network::FilterStatus::Continue; } + void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override { + read_callbacks_ = &callbacks; + } + +private: + void parseClientHello(const void* data, size_t len); + void done(bool success); + void onServername(absl::string_view name); + + ConfigSharedPtr config_; + Network::ReadFilterCallbacks* read_callbacks_{}; + + bssl::UniquePtr ssl_; + uint64_t read_{0}; + bool alpn_found_{false}; + bool clienthello_success_{false}; + + static thread_local uint8_t buf_[Config::TLS_MAX_CLIENT_HELLO]; + + // Allows callbacks on the SSL_CTX to set fields in this class. + friend class Config; +}; + +} // namespace InnerSniReader +} // namespace NetworkFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/network/well_known_names.h b/source/extensions/filters/network/well_known_names.h index 06123fd6df511..b13b63312c73a 100644 --- a/source/extensions/filters/network/well_known_names.h +++ b/source/extensions/filters/network/well_known_names.h @@ -18,6 +18,8 @@ class NetworkFilterNameValues { const std::string Echo = "envoy.echo"; // HTTP connection manager filter const std::string HttpConnectionManager = "envoy.http_connection_manager"; + // HTTP connection manager filter + const std::string InnerSniReader = "envoy.inner_sni_reader"; // Mongo proxy filter const std::string MongoProxy = "envoy.mongo_proxy"; // Rate limit filter From 0412a0a9056767b2087991213ceaa1209489bf4e Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Thu, 23 Aug 2018 14:46:38 +0300 Subject: [PATCH 03/17] Rename inner_sni_reader -> network_level_sni_reader Signed-off-by: ronenschafferibm --- source/extensions/extensions_build_config.bzl | 2 +- .../BUILD | 8 ++--- .../config.cc | 20 ++++++------ .../network_level_sni_reader.cc} | 31 +++++++++---------- .../network_level_sni_reader.h} | 24 +++++++------- .../filters/network/well_known_names.h | 4 +-- 6 files changed, 45 insertions(+), 44 deletions(-) rename source/extensions/filters/network/{inner_sni_reader => network_level_sni_reader}/BUILD (81%) rename source/extensions/filters/network/{inner_sni_reader => network_level_sni_reader}/config.cc (68%) rename source/extensions/filters/network/{inner_sni_reader/inner_sni_reader.cc => network_level_sni_reader/network_level_sni_reader.cc} (74%) rename source/extensions/filters/network/{inner_sni_reader/inner_sni_reader.h => network_level_sni_reader/network_level_sni_reader.h} (77%) diff --git a/source/extensions/extensions_build_config.bzl b/source/extensions/extensions_build_config.bzl index c9ecf69f592c2..2274a3959052a 100644 --- a/source/extensions/extensions_build_config.bzl +++ b/source/extensions/extensions_build_config.bzl @@ -64,7 +64,7 @@ EXTENSIONS = { "envoy.filters.network.echo": "//source/extensions/filters/network/echo:config", "envoy.filters.network.ext_authz": "//source/extensions/filters/network/ext_authz:config", "envoy.filters.network.http_connection_manager": "//source/extensions/filters/network/http_connection_manager:config", - "envoy.filters.network.inner_sni_reader": "//source/extensions/filters/network/inner_sni_reader:config", + "envoy.filters.network.network_level_sni_reader": "//source/extensions/filters/network/network_level_sni_reader:config", "envoy.filters.network.mongo_proxy": "//source/extensions/filters/network/mongo_proxy:config", "envoy.filters.network.ratelimit": "//source/extensions/filters/network/ratelimit:config", "envoy.filters.network.rbac": "//source/extensions/filters/network/rbac:config", diff --git a/source/extensions/filters/network/inner_sni_reader/BUILD b/source/extensions/filters/network/network_level_sni_reader/BUILD similarity index 81% rename from source/extensions/filters/network/inner_sni_reader/BUILD rename to source/extensions/filters/network/network_level_sni_reader/BUILD index 150170255312c..1390c92afb3ef 100644 --- a/source/extensions/filters/network/inner_sni_reader/BUILD +++ b/source/extensions/filters/network/network_level_sni_reader/BUILD @@ -9,9 +9,9 @@ load( envoy_package() envoy_cc_library( - name = "inner_sni_reader", - srcs = ["inner_sni_reader.cc"], - hdrs = ["inner_sni_reader.h"], + name = "network_level_sni_reader", + srcs = ["network_level_sni_reader.cc"], + hdrs = ["network_level_sni_reader.h"], external_deps = ["ssl"], deps = [ "//include/envoy/buffer:buffer_interface", @@ -26,7 +26,7 @@ envoy_cc_library( name = "config", srcs = ["config.cc"], deps = [ - ":inner_sni_reader", + ":network_level_sni_reader", "//include/envoy/registry", "//include/envoy/server:filter_config_interface", "//source/extensions/filters/network:well_known_names", diff --git a/source/extensions/filters/network/inner_sni_reader/config.cc b/source/extensions/filters/network/network_level_sni_reader/config.cc similarity index 68% rename from source/extensions/filters/network/inner_sni_reader/config.cc rename to source/extensions/filters/network/network_level_sni_reader/config.cc index 5e8f51cea4b84..7ccc3487d64e8 100644 --- a/source/extensions/filters/network/inner_sni_reader/config.cc +++ b/source/extensions/filters/network/network_level_sni_reader/config.cc @@ -1,18 +1,20 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" -#include "extensions/filters/network/inner_sni_reader/inner_sni_reader.h" +#include "extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h" #include "extensions/filters/network/well_known_names.h" namespace Envoy { namespace Extensions { namespace NetworkFilters { -namespace InnerSniReader { +namespace NetworkLevelSniReader { /** - * Config registration for the inner_sni_reader filter. @see NamedNetworkFilterConfigFactory. + * Config registration for the network level SNI reader filter. @see + * NamedNetworkFilterConfigFactory. */ -class InnerSniReaderConfigFactory : public Server::Configuration::NamedNetworkFilterConfigFactory { +class NetworkLevelSniReaderConfigFactory + : public Server::Configuration::NamedNetworkFilterConfigFactory { public: // NamedNetworkFilterConfigFactory Network::FilterFactoryCb @@ -21,7 +23,7 @@ class InnerSniReaderConfigFactory : public Server::Configuration::NamedNetworkFi // TODO: call createFilterFactoryFromProto and remove duplicate code ConfigSharedPtr filter_config(new Config(context.scope())); return [filter_config](Network::FilterManager& filter_manager) -> void { - filter_manager.addReadFilter(std::make_shared(filter_config)); + filter_manager.addReadFilter(std::make_shared(filter_config)); }; } @@ -30,7 +32,7 @@ class InnerSniReaderConfigFactory : public Server::Configuration::NamedNetworkFi Server::Configuration::FactoryContext& context) override { ConfigSharedPtr filter_config(new Config(context.scope())); return [filter_config](Network::FilterManager& filter_manager) -> void { - filter_manager.addReadFilter(std::make_shared(filter_config)); + filter_manager.addReadFilter(std::make_shared(filter_config)); }; } @@ -38,17 +40,17 @@ class InnerSniReaderConfigFactory : public Server::Configuration::NamedNetworkFi return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()}; } - std::string name() override { return NetworkFilterNames::get().InnerSniReader; } + std::string name() override { return NetworkFilterNames::get().NetworkLevelSniReader; } }; /** * Static registration for the echo filter. @see RegisterFactory. */ -static Registry::RegisterFactory registered_; -} // namespace InnerSniReader +} // namespace NetworkLevelSniReader } // namespace NetworkFilters } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc similarity index 74% rename from source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc rename to source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 94940c7ac09cd..fbf31f4cb9326 100644 --- a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -1,4 +1,4 @@ -#include "extensions/filters/network/inner_sni_reader/inner_sni_reader.h" +#include "extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h" #include "envoy/buffer/buffer.h" #include "envoy/common/exception.h" @@ -13,10 +13,10 @@ namespace Envoy { namespace Extensions { namespace NetworkFilters { -namespace InnerSniReader { +namespace NetworkLevelSniReader { Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) - : stats_{ALL_INNER_SNI_READER_STATS(POOL_COUNTER_PREFIX(scope, "inner_sni_reader."))}, + : stats_{ALL_NETWORK_LEVEL_SNI_READER_STATS(POOL_COUNTER_PREFIX(scope, "network_levelsni_reader."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), max_client_hello_size_(max_client_hello_size) { @@ -34,7 +34,7 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) // size_t len; // if (SSL_early_callback_ctx_extension_get( // client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) { -// InnerSniReaderFilter* filter = static_cast(SSL_get_app_data(client_hello->ssl)); +// NetworkLevelSniReaderFilter* filter = static_cast(SSL_get_app_data(client_hello->ssl)); //// filter->onALPN(data, len); // if (filter != nullptr) {} // @@ -43,7 +43,7 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) // }); SSL_CTX_set_tlsext_servername_callback( ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { - InnerSniReaderFilter* filter = static_cast(SSL_get_app_data(ssl)); + NetworkLevelSniReaderFilter* filter = static_cast(SSL_get_app_data(ssl)); filter->onServername(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)); if (filter != nullptr) {} @@ -55,17 +55,17 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ctx_.get())}; } -thread_local uint8_t InnerSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HELLO]; +thread_local uint8_t NetworkLevelSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HELLO]; -InnerSniReaderFilter::InnerSniReaderFilter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { +NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { RELEASE_ASSERT(sizeof(buf_) >= config_->maxClientHelloSize(), ""); SSL_set_app_data(ssl_.get(), this); SSL_set_accept_state(ssl_.get()); } -Network::FilterStatus InnerSniReaderFilter::onData(Buffer::Instance& data, bool) { - ENVOY_CONN_LOG(trace, "InnerSniReader: got {} bytes", read_callbacks_->connection(), data.length()); +Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data, bool) { + ENVOY_CONN_LOG(trace, "NetworkLevelSniReader: got {} bytes", read_callbacks_->connection(), data.length()); // TODO: append data to the buffer instead of overwriting it. size_t len = (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO ; @@ -76,8 +76,8 @@ Network::FilterStatus InnerSniReaderFilter::onData(Buffer::Instance& data, bool) return Network::FilterStatus::Continue; } -void InnerSniReaderFilter::onServername(absl::string_view name) { - ENVOY_CONN_LOG(debug, "inner sni reader: servername: {}", read_callbacks_->connection(), name); +void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { + ENVOY_CONN_LOG(debug, "network level sni reader: servername: {}", read_callbacks_->connection(), name); if (!name.empty()) { config_->stats().sni_found_.inc(); read_callbacks_->networkLevelRequestedServerName(name); @@ -87,7 +87,7 @@ void InnerSniReaderFilter::onServername(absl::string_view name) { clienthello_success_ = true; } -void InnerSniReaderFilter::parseClientHello(const void* data, size_t len) { +void NetworkLevelSniReaderFilter::parseClientHello(const void* data, size_t len) { // Ownership is passed to ssl_ in SSL_set_bio() bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); @@ -119,7 +119,6 @@ void InnerSniReaderFilter::parseClientHello(const void* data, size_t len) { } else { config_->stats().alpn_not_found_.inc(); } -// cb_->socket().setDetectedTransportProtocol(TransportSockets::TransportSocketNames::get().Tls); } else { config_->stats().tls_not_found_.inc(); } @@ -131,12 +130,12 @@ void InnerSniReaderFilter::parseClientHello(const void* data, size_t len) { } } -void InnerSniReaderFilter::done(bool success) { - ENVOY_LOG(trace, "inner sni reader: done: {}", success); +void NetworkLevelSniReaderFilter::done(bool success) { + ENVOY_LOG(trace, "network level sni reader: done: {}", success); read_callbacks_->continueReading(); } -} // namespace InnerSniReader +} // namespace NetworkLevelSniReader } // namespace NetworkFilters } // namespace Extensions } // namespace Envoy diff --git a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h similarity index 77% rename from source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h rename to source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 88d8cb5b0d3fd..995c386bec345 100644 --- a/source/extensions/filters/network/inner_sni_reader/inner_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -12,13 +12,13 @@ namespace Envoy { namespace Extensions { namespace NetworkFilters { -namespace InnerSniReader { +namespace NetworkLevelSniReader { /** - * All stats for the inner SNI reader. @see stats_macros.h + * All stats for the network level SNI reader. @see stats_macros.h */ // TODO: Remove unnecessary stats -#define ALL_INNER_SNI_READER_STATS(COUNTER) \ +#define ALL_NETWORK_LEVEL_SNI_READER_STATS(COUNTER) \ COUNTER(connection_closed) \ COUNTER(client_hello_too_large) \ COUNTER(read_error) \ @@ -31,36 +31,36 @@ namespace InnerSniReader { COUNTER(sni_not_found) /** - * Definition of all stats for the inner SNI reader. @see stats_macros.h + * Definition of all stats for the network level SNI reader. @see stats_macros.h */ -struct InnerSniReaderStats { - ALL_INNER_SNI_READER_STATS(GENERATE_COUNTER_STRUCT) +struct NetworkLevelSniReaderStats { + ALL_NETWORK_LEVEL_SNI_READER_STATS(GENERATE_COUNTER_STRUCT) }; /** - * Global configuration for inner SNI reader. + * Global configuration for network level SNI reader. */ class Config { public: Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - const InnerSniReaderStats& stats() const { return stats_; } + const NetworkLevelSniReaderStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; private: - InnerSniReaderStats stats_; + NetworkLevelSniReaderStats stats_; bssl::UniquePtr ssl_ctx_; const uint32_t max_client_hello_size_; }; typedef std::shared_ptr ConfigSharedPtr; -class InnerSniReaderFilter : public Network::ReadFilter, Logger::Loggable { +class NetworkLevelSniReaderFilter : public Network::ReadFilter, Logger::Loggable { public: - InnerSniReaderFilter(const ConfigSharedPtr config); + NetworkLevelSniReaderFilter(const ConfigSharedPtr config); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; @@ -88,7 +88,7 @@ class InnerSniReaderFilter : public Network::ReadFilter, Logger::Loggable Date: Sun, 26 Aug 2018 16:13:14 +0300 Subject: [PATCH 04/17] Extract parseClientHello and TlsStats to ssl/utility Signed-off-by: ronenschafferibm --- source/common/ssl/BUILD | 4 ++ source/common/ssl/utility.cc | 53 +++++++++++++++++++ source/common/ssl/utility.h | 29 ++++++++++ .../filters/listener/tls_inspector/BUILD | 1 + .../listener/tls_inspector/tls_inspector.cc | 10 +++- .../listener/tls_inspector/tls_inspector.h | 28 ++-------- .../network/network_level_sni_reader/BUILD | 1 + .../network_level_sni_reader.cc | 49 ++++++++++------- .../network_level_sni_reader.h | 32 ++--------- 9 files changed, 133 insertions(+), 74 deletions(-) diff --git a/source/common/ssl/BUILD b/source/common/ssl/BUILD index abc98f802ff0b..bf4e7f75341e1 100644 --- a/source/common/ssl/BUILD +++ b/source/common/ssl/BUILD @@ -92,4 +92,8 @@ envoy_cc_library( external_deps = [ "ssl", ], + deps = [ + "//include/envoy/stats:stats_macros", + "//source/common/common:assert_lib", + ], ) diff --git a/source/common/ssl/utility.cc b/source/common/ssl/utility.cc index 6f98864c97ac6..188864ab8794c 100644 --- a/source/common/ssl/utility.cc +++ b/source/common/ssl/utility.cc @@ -1,5 +1,10 @@ #include "common/ssl/utility.h" +#include "common/common/assert.h" + +#include "openssl/bytestring.h" +#include "openssl/ssl.h" + namespace Envoy { namespace Ssl { @@ -18,5 +23,53 @@ std::string Utility::getSerialNumberFromCertificate(X509& cert) { return ""; } +void Utility::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl_, + uint64_t read_, uint32_t maxClientHelloSize, + const Ssl::Utility::TlsStats& stats, std::function done, + bool& alpn_found_, bool& clienthello_success_, + std::function onSuccess) { + // Ownership is passed to ssl_ in SSL_set_bio() + bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); + + // Make the mem-BIO return that there is more data + // available beyond it's end + BIO_set_mem_eof_return(bio.get(), -1); + + SSL_set_bio(ssl_.get(), bio.get(), bio.get()); + bio.release(); + + int ret = SSL_do_handshake(ssl_.get()); + + // This should never succeed because an error is always returned from the SNI callback. + ASSERT(ret <= 0); + switch (SSL_get_error(ssl_.get(), ret)) { + case SSL_ERROR_WANT_READ: + if (read_ == maxClientHelloSize) { + // We've hit the specified size limit. This is an unreasonably large ClientHello; + // indicate failure. + stats.client_hello_too_large_.inc(); + done(false); + } + break; + case SSL_ERROR_SSL: + if (clienthello_success_) { + stats.tls_found_.inc(); + if (alpn_found_) { + stats.alpn_found_.inc(); + } else { + stats.alpn_not_found_.inc(); + } + onSuccess(); + } else { + stats.tls_not_found_.inc(); + } + done(true); + break; + default: + done(false); + break; + } +} + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/utility.h b/source/common/ssl/utility.h index cb41056e0f0fb..e8f2a7ecd8996 100644 --- a/source/common/ssl/utility.h +++ b/source/common/ssl/utility.h @@ -2,12 +2,36 @@ #include +#include "envoy/stats/stats_macros.h" + #include "openssl/ssl.h" namespace Envoy { namespace Ssl { namespace Utility { +/** + * All stats for the TLS inspector. @see stats_macros.h + */ +#define TLS_STATS(COUNTER) \ + COUNTER(connection_closed) \ + COUNTER(client_hello_too_large) \ + COUNTER(read_error) \ + COUNTER(read_timeout) \ + COUNTER(tls_found) \ + COUNTER(tls_not_found) \ + COUNTER(alpn_found) \ + COUNTER(alpn_not_found) \ + COUNTER(sni_found) \ + COUNTER(sni_not_found) + +/** + * Definition of stats for the TLS. @see stats_macros.h + */ +struct TlsStats { + TLS_STATS(GENERATE_COUNTER_STRUCT) +}; + /** * Retrieve the serial number of a certificate. * @param ssl the certificate @@ -16,6 +40,11 @@ namespace Utility { */ std::string getSerialNumberFromCertificate(X509& cert); +void parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl_, uint64_t read_, + uint32_t maxClientHelloSize, const Ssl::Utility::TlsStats& stats, + std::function, bool& alpn_found_, bool& clienthello_success_, + std::function onSuccess); + } // namespace Utility } // namespace Ssl } // namespace Envoy diff --git a/source/extensions/filters/listener/tls_inspector/BUILD b/source/extensions/filters/listener/tls_inspector/BUILD index af90ed9fcd4af..0b9d688aadfc0 100644 --- a/source/extensions/filters/listener/tls_inspector/BUILD +++ b/source/extensions/filters/listener/tls_inspector/BUILD @@ -24,6 +24,7 @@ envoy_cc_library( "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", + "//source/common/ssl:utility_lib", "//source/extensions/transport_sockets:well_known_names", ], ) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 16090aaaec7b7..35a1c1dbfc225 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -25,7 +25,7 @@ namespace ListenerFilters { namespace TlsInspector { Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) - : stats_{ALL_TLS_INSPECTOR_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, + : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), max_client_hello_size_(max_client_hello_size) { @@ -161,7 +161,13 @@ void Filter::onRead() { const uint8_t* data = buf_ + read_; const size_t len = result.rc_ - read_; read_ = result.rc_; - parseClientHello(data, len); + Ssl::Utility::parseClientHello(data, len, ssl_, read_, config_->maxClientHelloSize(), + config_->stats(), [&](bool success) -> void { done(success); }, + alpn_found_, clienthello_success_, + [&]() -> void { + cb_->socket().setDetectedTransportProtocol( + TransportSockets::TransportSocketNames::get().Tls); + }); } } diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index c927c56d13276..0e5a0e33d861a 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -4,9 +4,9 @@ #include "envoy/event/timer.h" #include "envoy/network/filter.h" #include "envoy/stats/scope.h" -#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" +#include "common/ssl/utility.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -16,28 +16,6 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { -/** - * All stats for the TLS inspector. @see stats_macros.h - */ -#define ALL_TLS_INSPECTOR_STATS(COUNTER) \ - COUNTER(connection_closed) \ - COUNTER(client_hello_too_large) \ - COUNTER(read_error) \ - COUNTER(read_timeout) \ - COUNTER(tls_found) \ - COUNTER(tls_not_found) \ - COUNTER(alpn_found) \ - COUNTER(alpn_not_found) \ - COUNTER(sni_found) \ - COUNTER(sni_not_found) - -/** - * Definition of all stats for the TLS inspector. @see stats_macros.h - */ -struct TlsInspectorStats { - ALL_TLS_INSPECTOR_STATS(GENERATE_COUNTER_STRUCT) -}; - /** * Global configuration for TLS inspector. */ @@ -45,14 +23,14 @@ class Config { public: Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - const TlsInspectorStats& stats() const { return stats_; } + const Ssl::Utility::TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; private: - TlsInspectorStats stats_; + Ssl::Utility::TlsStats stats_; bssl::UniquePtr ssl_ctx_; const uint32_t max_client_hello_size_; }; diff --git a/source/extensions/filters/network/network_level_sni_reader/BUILD b/source/extensions/filters/network/network_level_sni_reader/BUILD index 1390c92afb3ef..41996f7e3eb3c 100644 --- a/source/extensions/filters/network/network_level_sni_reader/BUILD +++ b/source/extensions/filters/network/network_level_sni_reader/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//include/envoy/network:filter_interface", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", + "//source/common/ssl:utility_lib", ], ) diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index fbf31f4cb9326..7e5df491d98f3 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -16,7 +16,7 @@ namespace NetworkFilters { namespace NetworkLevelSniReader { Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) - : stats_{ALL_NETWORK_LEVEL_SNI_READER_STATS(POOL_COUNTER_PREFIX(scope, "network_levelsni_reader."))}, + : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, "network_levelsni_reader."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), max_client_hello_size_(max_client_hello_size) { @@ -28,24 +28,26 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); // TODO: Remove following -// SSL_CTX_set_select_certificate_cb( -// ssl_ctx_.get(), [](const SSL_CLIENT_HELLO* client_hello) -> ssl_select_cert_result_t { -// const uint8_t* data; -// size_t len; -// if (SSL_early_callback_ctx_extension_get( -// client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) { -// NetworkLevelSniReaderFilter* filter = static_cast(SSL_get_app_data(client_hello->ssl)); -//// filter->onALPN(data, len); -// if (filter != nullptr) {} -// -// } -// return ssl_select_cert_success; -// }); + // SSL_CTX_set_select_certificate_cb( + // ssl_ctx_.get(), [](const SSL_CLIENT_HELLO* client_hello) -> ssl_select_cert_result_t { + // const uint8_t* data; + // size_t len; + // if (SSL_early_callback_ctx_extension_get( + // client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) + // { + // NetworkLevelSniReaderFilter* filter = + // static_cast(SSL_get_app_data(client_hello->ssl)); + //// filter->onALPN(data, len); + // if (filter != nullptr) {} + // + // } + // return ssl_select_cert_success; + // }); SSL_CTX_set_tlsext_servername_callback( ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { - NetworkLevelSniReaderFilter* filter = static_cast(SSL_get_app_data(ssl)); + NetworkLevelSniReaderFilter* filter = + static_cast(SSL_get_app_data(ssl)); filter->onServername(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)); - if (filter != nullptr) {} // Return an error to stop the handshake; we have what we wanted already. *out_alert = SSL_AD_USER_CANCELLED; @@ -57,7 +59,8 @@ bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ thread_local uint8_t NetworkLevelSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HELLO]; -NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { +NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr config) + : config_(config), ssl_(config_->newSsl()) { RELEASE_ASSERT(sizeof(buf_) >= config_->maxClientHelloSize(), ""); SSL_set_app_data(ssl_.get(), this); @@ -65,19 +68,25 @@ NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr c } Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data, bool) { - ENVOY_CONN_LOG(trace, "NetworkLevelSniReader: got {} bytes", read_callbacks_->connection(), data.length()); + ENVOY_CONN_LOG(trace, "NetworkLevelSniReader: got {} bytes", read_callbacks_->connection(), + data.length()); // TODO: append data to the buffer instead of overwriting it. - size_t len = (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO ; + size_t len = + (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO; data.copyOut(0, len, buf_); + Ssl::Utility::parseClientHello(buf_, len, ssl_, read_, config_->maxClientHelloSize(), + config_->stats(), [&](bool success) -> void { done(success); }, + alpn_found_, clienthello_success_, []() -> void {}); parseClientHello(buf_, len); return Network::FilterStatus::Continue; } void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { - ENVOY_CONN_LOG(debug, "network level sni reader: servername: {}", read_callbacks_->connection(), name); + ENVOY_CONN_LOG(debug, "network level sni reader: servername: {}", read_callbacks_->connection(), + name); if (!name.empty()) { config_->stats().sni_found_.inc(); read_callbacks_->networkLevelRequestedServerName(name); diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 995c386bec345..2e7f25ecc2f4f 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -2,9 +2,9 @@ #include "envoy/network/filter.h" #include "envoy/stats/scope.h" -#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" +#include "common/ssl/utility.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -14,29 +14,6 @@ namespace Extensions { namespace NetworkFilters { namespace NetworkLevelSniReader { -/** - * All stats for the network level SNI reader. @see stats_macros.h - */ -// TODO: Remove unnecessary stats -#define ALL_NETWORK_LEVEL_SNI_READER_STATS(COUNTER) \ - COUNTER(connection_closed) \ - COUNTER(client_hello_too_large) \ - COUNTER(read_error) \ - COUNTER(read_timeout) \ - COUNTER(tls_found) \ - COUNTER(tls_not_found) \ - COUNTER(alpn_found) \ - COUNTER(alpn_not_found) \ - COUNTER(sni_found) \ - COUNTER(sni_not_found) - -/** - * Definition of all stats for the network level SNI reader. @see stats_macros.h - */ -struct NetworkLevelSniReaderStats { - ALL_NETWORK_LEVEL_SNI_READER_STATS(GENERATE_COUNTER_STRUCT) -}; - /** * Global configuration for network level SNI reader. */ @@ -44,21 +21,22 @@ class Config { public: Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - const NetworkLevelSniReaderStats& stats() const { return stats_; } + const Ssl::Utility::TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; private: - NetworkLevelSniReaderStats stats_; + Ssl::Utility::TlsStats stats_; bssl::UniquePtr ssl_ctx_; const uint32_t max_client_hello_size_; }; typedef std::shared_ptr ConfigSharedPtr; -class NetworkLevelSniReaderFilter : public Network::ReadFilter, Logger::Loggable { +class NetworkLevelSniReaderFilter : public Network::ReadFilter, + Logger::Loggable { public: NetworkLevelSniReaderFilter(const ConfigSharedPtr config); From dda1beec5020f31da2eec47308c7021657fc0d15 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 26 Aug 2018 16:52:36 +0300 Subject: [PATCH 05/17] Cleanup Signed-off-by: ronenschafferibm --- source/common/ssl/utility.cc | 20 +++---- source/common/ssl/utility.h | 4 +- .../network_level_sni_reader.cc | 60 ------------------- .../network_level_sni_reader.h | 1 - 4 files changed, 12 insertions(+), 73 deletions(-) diff --git a/source/common/ssl/utility.cc b/source/common/ssl/utility.cc index 188864ab8794c..238735b4a0807 100644 --- a/source/common/ssl/utility.cc +++ b/source/common/ssl/utility.cc @@ -23,28 +23,28 @@ std::string Utility::getSerialNumberFromCertificate(X509& cert) { return ""; } -void Utility::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl_, - uint64_t read_, uint32_t maxClientHelloSize, +void Utility::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, + uint64_t read, uint32_t maxClientHelloSize, const Ssl::Utility::TlsStats& stats, std::function done, - bool& alpn_found_, bool& clienthello_success_, + bool& alpn_found, bool& clienthello_success, std::function onSuccess) { - // Ownership is passed to ssl_ in SSL_set_bio() + // Ownership is passed to ssl in SSL_set_bio() bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); // Make the mem-BIO return that there is more data // available beyond it's end BIO_set_mem_eof_return(bio.get(), -1); - SSL_set_bio(ssl_.get(), bio.get(), bio.get()); + SSL_set_bio(ssl.get(), bio.get(), bio.get()); bio.release(); - int ret = SSL_do_handshake(ssl_.get()); + int ret = SSL_do_handshake(ssl.get()); // This should never succeed because an error is always returned from the SNI callback. ASSERT(ret <= 0); - switch (SSL_get_error(ssl_.get(), ret)) { + switch (SSL_get_error(ssl.get(), ret)) { case SSL_ERROR_WANT_READ: - if (read_ == maxClientHelloSize) { + if (read == maxClientHelloSize) { // We've hit the specified size limit. This is an unreasonably large ClientHello; // indicate failure. stats.client_hello_too_large_.inc(); @@ -52,9 +52,9 @@ void Utility::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl_, uint64_t read_, +void parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, uint64_t read, uint32_t maxClientHelloSize, const Ssl::Utility::TlsStats& stats, - std::function, bool& alpn_found_, bool& clienthello_success_, + std::function done, bool& alpn_found, bool& clienthello_success, std::function onSuccess); } // namespace Utility diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 7e5df491d98f3..b0ce2cb18a10b 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -27,22 +27,6 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); - // TODO: Remove following - // SSL_CTX_set_select_certificate_cb( - // ssl_ctx_.get(), [](const SSL_CLIENT_HELLO* client_hello) -> ssl_select_cert_result_t { - // const uint8_t* data; - // size_t len; - // if (SSL_early_callback_ctx_extension_get( - // client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) - // { - // NetworkLevelSniReaderFilter* filter = - // static_cast(SSL_get_app_data(client_hello->ssl)); - //// filter->onALPN(data, len); - // if (filter != nullptr) {} - // - // } - // return ssl_select_cert_success; - // }); SSL_CTX_set_tlsext_servername_callback( ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { NetworkLevelSniReaderFilter* filter = @@ -79,7 +63,6 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data Ssl::Utility::parseClientHello(buf_, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, []() -> void {}); - parseClientHello(buf_, len); return Network::FilterStatus::Continue; } @@ -96,49 +79,6 @@ void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { clienthello_success_ = true; } -void NetworkLevelSniReaderFilter::parseClientHello(const void* data, size_t len) { - // Ownership is passed to ssl_ in SSL_set_bio() - bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); - - // Make the mem-BIO return that there is more data - // available beyond it's end - BIO_set_mem_eof_return(bio.get(), -1); - - SSL_set_bio(ssl_.get(), bio.get(), bio.get()); - bio.release(); - - int ret = SSL_do_handshake(ssl_.get()); - - // This should never succeed because an error is always returned from the SNI callback. - ASSERT(ret <= 0); - switch (SSL_get_error(ssl_.get(), ret)) { - case SSL_ERROR_WANT_READ: - if (read_ == config_->maxClientHelloSize()) { - // We've hit the specified size limit. This is an unreasonably large ClientHello; - // indicate failure. - config_->stats().client_hello_too_large_.inc(); - done(false); - } - break; - case SSL_ERROR_SSL: - if (clienthello_success_) { - config_->stats().tls_found_.inc(); - if (alpn_found_) { - config_->stats().alpn_found_.inc(); - } else { - config_->stats().alpn_not_found_.inc(); - } - } else { - config_->stats().tls_not_found_.inc(); - } - done(true); - break; - default: - done(false); - break; - } -} - void NetworkLevelSniReaderFilter::done(bool success) { ENVOY_LOG(trace, "network level sni reader: done: {}", success); read_callbacks_->continueReading(); diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 2e7f25ecc2f4f..6c30485740558 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -48,7 +48,6 @@ class NetworkLevelSniReaderFilter : public Network::ReadFilter, } private: - void parseClientHello(const void* data, size_t len); void done(bool success); void onServername(absl::string_view name); From d4c69757549fef3432ca712a76911d3ffa3a4aec Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 26 Aug 2018 17:08:29 +0300 Subject: [PATCH 06/17] Cleanup Signed-off-by: ronenschafferibm --- .../network/network_level_sni_reader/network_level_sni_reader.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index b0ce2cb18a10b..9f255c54a12a7 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -63,7 +63,6 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data Ssl::Utility::parseClientHello(buf_, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, []() -> void {}); - return Network::FilterStatus::Continue; } From 345eea8c59552c9a578b3eb80b5a0be78b103bad Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 26 Aug 2018 18:24:49 +0300 Subject: [PATCH 07/17] Stop processing parseClientHello() when done Signed-off-by: ronenschafferibm --- .../network_level_sni_reader.cc | 11 ++++++++--- .../network_level_sni_reader.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 9f255c54a12a7..f0d02a3cfcb6a 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -54,7 +54,9 @@ NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr c Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data, bool) { ENVOY_CONN_LOG(trace, "NetworkLevelSniReader: got {} bytes", read_callbacks_->connection(), data.length()); - + if (done_) { + return Network::FilterStatus::Continue; + } // TODO: append data to the buffer instead of overwriting it. size_t len = (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO; @@ -63,7 +65,7 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data Ssl::Utility::parseClientHello(buf_, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, []() -> void {}); - return Network::FilterStatus::Continue; + return done_ ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration; } void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { @@ -80,7 +82,10 @@ void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { void NetworkLevelSniReaderFilter::done(bool success) { ENVOY_LOG(trace, "network level sni reader: done: {}", success); - read_callbacks_->continueReading(); + if (success) { + done_ = true; + read_callbacks_->continueReading(); + } } } // namespace NetworkLevelSniReader diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 6c30485740558..97cf2b7680714 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -58,6 +58,7 @@ class NetworkLevelSniReaderFilter : public Network::ReadFilter, uint64_t read_{0}; bool alpn_found_{false}; bool clienthello_success_{false}; + bool done_{false}; static thread_local uint8_t buf_[Config::TLS_MAX_CLIENT_HELLO]; From cbf686eca1c58f4f6975df7377e9657b349e265f Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 28 Aug 2018 14:38:06 +0300 Subject: [PATCH 08/17] Change network_level_requested_server_name_ type to std::string so calls to networkLevelRequestedServerName() won't return pointer to null in case network_level_requested_server_name_ is not set. Signed-off-by: ronenschafferibm --- source/common/network/filter_manager_impl.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/common/network/filter_manager_impl.h b/source/common/network/filter_manager_impl.h index b4bf81b1dec17..832e5068d14a9 100644 --- a/source/common/network/filter_manager_impl.h +++ b/source/common/network/filter_manager_impl.h @@ -62,12 +62,11 @@ class FilterManagerImpl { parent_.host_description_ = host; } absl::string_view networkLevelRequestedServerName() override { - // TODO: return an empty string and write a warning to log when inner SNI reader is not - // configured. + // TODO: write a warning to log when inner SNI reader is not set. return parent_.network_level_requested_server_name_; } void networkLevelRequestedServerName(absl::string_view name) override { - parent_.network_level_requested_server_name_ = name; + parent_.network_level_requested_server_name_ = std::string(name); } FilterManagerImpl& parent_; @@ -81,7 +80,7 @@ class FilterManagerImpl { Connection& connection_; BufferSource& buffer_source_; - absl::string_view network_level_requested_server_name_; + std::string network_level_requested_server_name_; Upstream::HostDescriptionConstSharedPtr host_description_; std::list upstream_filters_; std::list downstream_filters_; From d97095a46a536c2eb5d093d8b18262b49ddaad0e Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 28 Aug 2018 14:45:27 +0300 Subject: [PATCH 09/17] Extract duplicate code to a method Signed-off-by: ronenschafferibm --- .../network_level_sni_reader/config.cc | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/network/network_level_sni_reader/config.cc b/source/extensions/filters/network/network_level_sni_reader/config.cc index 7ccc3487d64e8..cba1409f87845 100644 --- a/source/extensions/filters/network/network_level_sni_reader/config.cc +++ b/source/extensions/filters/network/network_level_sni_reader/config.cc @@ -20,20 +20,13 @@ class NetworkLevelSniReaderConfigFactory Network::FilterFactoryCb createFilterFactory(const Json::Object&, Server::Configuration::FactoryContext& context) override { - // TODO: call createFilterFactoryFromProto and remove duplicate code - ConfigSharedPtr filter_config(new Config(context.scope())); - return [filter_config](Network::FilterManager& filter_manager) -> void { - filter_manager.addReadFilter(std::make_shared(filter_config)); - }; + return createFilterFactoryFromContext(context); } Network::FilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message&, Server::Configuration::FactoryContext& context) override { - ConfigSharedPtr filter_config(new Config(context.scope())); - return [filter_config](Network::FilterManager& filter_manager) -> void { - filter_manager.addReadFilter(std::make_shared(filter_config)); - }; + return createFilterFactoryFromContext(context); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -41,6 +34,15 @@ class NetworkLevelSniReaderConfigFactory } std::string name() override { return NetworkFilterNames::get().NetworkLevelSniReader; } + +private: + Network::FilterFactoryCb + createFilterFactoryFromContext(Server::Configuration::FactoryContext& context) { + ConfigSharedPtr filter_config(new Config(context.scope())); + return [filter_config](Network::FilterManager& filter_manager) -> void { + filter_manager.addReadFilter(std::make_shared(filter_config)); + }; + } }; /** From bedce5bb7659efe93adf9bc3170f2b4a22e10e81 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 28 Aug 2018 15:04:46 +0300 Subject: [PATCH 10/17] Move TlsStats and parseClientHello() back to tls_inspector Signed-off-by: ronenschafferibm --- .../listener/tls_inspector/tls_inspector.cc | 45 ++++++++++--------- .../listener/tls_inspector/tls_inspector.h | 33 ++++++++++++-- 2 files changed, 53 insertions(+), 25 deletions(-) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 35a1c1dbfc225..a04d86932f84f 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -161,13 +161,13 @@ void Filter::onRead() { const uint8_t* data = buf_ + read_; const size_t len = result.rc_ - read_; read_ = result.rc_; - Ssl::Utility::parseClientHello(data, len, ssl_, read_, config_->maxClientHelloSize(), - config_->stats(), [&](bool success) -> void { done(success); }, - alpn_found_, clienthello_success_, - [&]() -> void { - cb_->socket().setDetectedTransportProtocol( - TransportSockets::TransportSocketNames::get().Tls); - }); + parseClientHello(data, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), + [&](bool success) -> void { done(success); }, alpn_found_, + clienthello_success_, + [&]() -> void { + cb_->socket().setDetectedTransportProtocol( + TransportSockets::TransportSocketNames::get().Tls); + }); } } @@ -184,41 +184,44 @@ void Filter::done(bool success) { cb_->continueFilterChain(success); } -void Filter::parseClientHello(const void* data, size_t len) { - // Ownership is passed to ssl_ in SSL_set_bio() +void Filter::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, + uint64_t read, uint32_t maxClientHelloSize, const TlsStats& stats, + std::function done, bool& alpn_found, + bool& clienthello_success, std::function onSuccess) { + // Ownership is passed to ssl in SSL_set_bio() bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); // Make the mem-BIO return that there is more data // available beyond it's end BIO_set_mem_eof_return(bio.get(), -1); - SSL_set_bio(ssl_.get(), bio.get(), bio.get()); + SSL_set_bio(ssl.get(), bio.get(), bio.get()); bio.release(); - int ret = SSL_do_handshake(ssl_.get()); + int ret = SSL_do_handshake(ssl.get()); // This should never succeed because an error is always returned from the SNI callback. ASSERT(ret <= 0); - switch (SSL_get_error(ssl_.get(), ret)) { + switch (SSL_get_error(ssl.get(), ret)) { case SSL_ERROR_WANT_READ: - if (read_ == config_->maxClientHelloSize()) { + if (read == maxClientHelloSize) { // We've hit the specified size limit. This is an unreasonably large ClientHello; // indicate failure. - config_->stats().client_hello_too_large_.inc(); + stats.client_hello_too_large_.inc(); done(false); } break; case SSL_ERROR_SSL: - if (clienthello_success_) { - config_->stats().tls_found_.inc(); - if (alpn_found_) { - config_->stats().alpn_found_.inc(); + if (clienthello_success) { + stats.tls_found_.inc(); + if (alpn_found) { + stats.alpn_found_.inc(); } else { - config_->stats().alpn_not_found_.inc(); + stats.alpn_not_found_.inc(); } - cb_->socket().setDetectedTransportProtocol(TransportSockets::TransportSocketNames::get().Tls); + onSuccess(); } else { - config_->stats().tls_not_found_.inc(); + stats.tls_not_found_.inc(); } done(true); break; diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 0e5a0e33d861a..b082727813a28 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -4,9 +4,9 @@ #include "envoy/event/timer.h" #include "envoy/network/filter.h" #include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" #include "common/common/logger.h" -#include "common/ssl/utility.h" #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -16,6 +16,28 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { +/** + * All stats for the TLS inspector. @see stats_macros.h + */ +#define TLS_STATS(COUNTER) \ + COUNTER(connection_closed) \ + COUNTER(client_hello_too_large) \ + COUNTER(read_error) \ + COUNTER(read_timeout) \ + COUNTER(tls_found) \ + COUNTER(tls_not_found) \ + COUNTER(alpn_found) \ + COUNTER(alpn_not_found) \ + COUNTER(sni_found) \ + COUNTER(sni_not_found) + +/** + * Definition of stats for the TLS. @see stats_macros.h + */ +struct TlsStats { + TLS_STATS(GENERATE_COUNTER_STRUCT) +}; + /** * Global configuration for TLS inspector. */ @@ -23,14 +45,14 @@ class Config { public: Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - const Ssl::Utility::TlsStats& stats() const { return stats_; } + const TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; private: - Ssl::Utility::TlsStats stats_; + TlsStats stats_; bssl::UniquePtr ssl_ctx_; const uint32_t max_client_hello_size_; }; @@ -48,7 +70,10 @@ class Filter : public Network::ListenerFilter, Logger::Loggable& ssl, uint64_t read, + uint32_t maxClientHelloSize, const TlsStats& stats, + std::function done, bool& alpn_found, bool& clienthello_success, + std::function onSuccess); void onRead(); void onTimeout(); void done(bool success); From 20c0a6e337ff94acb4e40d83273af7885b64a7d2 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 28 Aug 2018 15:40:53 +0300 Subject: [PATCH 11/17] Make parseClientHello() static and use it in NetworkLevelSniReader Signed-off-by: ronenschafferibm --- source/common/ssl/utility.cc | 48 ------------------- source/common/ssl/utility.h | 27 ----------- .../filters/listener/tls_inspector/BUILD | 1 - .../listener/tls_inspector/tls_inspector.h | 9 ++-- .../network/network_level_sni_reader/BUILD | 3 +- .../network_level_sni_reader/config.cc | 1 + .../network_level_sni_reader.cc | 9 ++-- .../network_level_sni_reader.h | 7 ++- 8 files changed, 18 insertions(+), 87 deletions(-) diff --git a/source/common/ssl/utility.cc b/source/common/ssl/utility.cc index 238735b4a0807..eb80a32c461d8 100644 --- a/source/common/ssl/utility.cc +++ b/source/common/ssl/utility.cc @@ -23,53 +23,5 @@ std::string Utility::getSerialNumberFromCertificate(X509& cert) { return ""; } -void Utility::parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, - uint64_t read, uint32_t maxClientHelloSize, - const Ssl::Utility::TlsStats& stats, std::function done, - bool& alpn_found, bool& clienthello_success, - std::function onSuccess) { - // Ownership is passed to ssl in SSL_set_bio() - bssl::UniquePtr bio(BIO_new_mem_buf(data, len)); - - // Make the mem-BIO return that there is more data - // available beyond it's end - BIO_set_mem_eof_return(bio.get(), -1); - - SSL_set_bio(ssl.get(), bio.get(), bio.get()); - bio.release(); - - int ret = SSL_do_handshake(ssl.get()); - - // This should never succeed because an error is always returned from the SNI callback. - ASSERT(ret <= 0); - switch (SSL_get_error(ssl.get(), ret)) { - case SSL_ERROR_WANT_READ: - if (read == maxClientHelloSize) { - // We've hit the specified size limit. This is an unreasonably large ClientHello; - // indicate failure. - stats.client_hello_too_large_.inc(); - done(false); - } - break; - case SSL_ERROR_SSL: - if (clienthello_success) { - stats.tls_found_.inc(); - if (alpn_found) { - stats.alpn_found_.inc(); - } else { - stats.alpn_not_found_.inc(); - } - onSuccess(); - } else { - stats.tls_not_found_.inc(); - } - done(true); - break; - default: - done(false); - break; - } -} - } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/utility.h b/source/common/ssl/utility.h index 0d73f0353a437..1981cb4b5b7e7 100644 --- a/source/common/ssl/utility.h +++ b/source/common/ssl/utility.h @@ -10,28 +10,6 @@ namespace Envoy { namespace Ssl { namespace Utility { -/** - * All stats for the TLS inspector. @see stats_macros.h - */ -#define TLS_STATS(COUNTER) \ - COUNTER(connection_closed) \ - COUNTER(client_hello_too_large) \ - COUNTER(read_error) \ - COUNTER(read_timeout) \ - COUNTER(tls_found) \ - COUNTER(tls_not_found) \ - COUNTER(alpn_found) \ - COUNTER(alpn_not_found) \ - COUNTER(sni_found) \ - COUNTER(sni_not_found) - -/** - * Definition of stats for the TLS. @see stats_macros.h - */ -struct TlsStats { - TLS_STATS(GENERATE_COUNTER_STRUCT) -}; - /** * Retrieve the serial number of a certificate. * @param ssl the certificate @@ -40,11 +18,6 @@ struct TlsStats { */ std::string getSerialNumberFromCertificate(X509& cert); -void parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, uint64_t read, - uint32_t maxClientHelloSize, const Ssl::Utility::TlsStats& stats, - std::function done, bool& alpn_found, bool& clienthello_success, - std::function onSuccess); - } // namespace Utility } // namespace Ssl } // namespace Envoy diff --git a/source/extensions/filters/listener/tls_inspector/BUILD b/source/extensions/filters/listener/tls_inspector/BUILD index 0b9d688aadfc0..af90ed9fcd4af 100644 --- a/source/extensions/filters/listener/tls_inspector/BUILD +++ b/source/extensions/filters/listener/tls_inspector/BUILD @@ -24,7 +24,6 @@ envoy_cc_library( "//source/common/api:os_sys_calls_lib", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", - "//source/common/ssl:utility_lib", "//source/extensions/transport_sockets:well_known_names", ], ) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index b082727813a28..4d57bb3ba6137 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -69,11 +69,12 @@ class Filter : public Network::ListenerFilter, Logger::Loggable& ssl, + uint64_t read, uint32_t maxClientHelloSize, const TlsStats& stats, + std::function done, bool& alpn_found, + bool& clienthello_success, std::function onSuccess); + private: - void parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, uint64_t read, - uint32_t maxClientHelloSize, const TlsStats& stats, - std::function done, bool& alpn_found, bool& clienthello_success, - std::function onSuccess); void onRead(); void onTimeout(); void done(bool success); diff --git a/source/extensions/filters/network/network_level_sni_reader/BUILD b/source/extensions/filters/network/network_level_sni_reader/BUILD index 41996f7e3eb3c..59f5d06dbdab3 100644 --- a/source/extensions/filters/network/network_level_sni_reader/BUILD +++ b/source/extensions/filters/network/network_level_sni_reader/BUILD @@ -19,7 +19,7 @@ envoy_cc_library( "//include/envoy/network:filter_interface", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", - "//source/common/ssl:utility_lib", + "//source/extensions/filters/listener/tls_inspector:tls_inspector_lib", ], ) @@ -30,6 +30,7 @@ envoy_cc_library( ":network_level_sni_reader", "//include/envoy/registry", "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/listener/tls_inspector:tls_inspector_lib", "//source/extensions/filters/network:well_known_names", ], ) diff --git a/source/extensions/filters/network/network_level_sni_reader/config.cc b/source/extensions/filters/network/network_level_sni_reader/config.cc index cba1409f87845..d2d6c6bc7627e 100644 --- a/source/extensions/filters/network/network_level_sni_reader/config.cc +++ b/source/extensions/filters/network/network_level_sni_reader/config.cc @@ -1,6 +1,7 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" +#include "extensions/filters/listener/tls_inspector/tls_inspector.h" #include "extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h" #include "extensions/filters/network/well_known_names.h" diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index f0d02a3cfcb6a..611cd83a5afb8 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -62,9 +62,10 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO; data.copyOut(0, len, buf_); - Ssl::Utility::parseClientHello(buf_, len, ssl_, read_, config_->maxClientHelloSize(), - config_->stats(), [&](bool success) -> void { done(success); }, - alpn_found_, clienthello_success_, []() -> void {}); + Extensions::ListenerFilters::TlsInspector::Filter::parseClientHello( + buf_, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), + [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, + []() -> void {}); return done_ ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration; } @@ -82,8 +83,8 @@ void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { void NetworkLevelSniReaderFilter::done(bool success) { ENVOY_LOG(trace, "network level sni reader: done: {}", success); + done_ = true; if (success) { - done_ = true; read_callbacks_->continueReading(); } } diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 97cf2b7680714..707204df918be 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -6,6 +6,8 @@ #include "common/common/logger.h" #include "common/ssl/utility.h" +#include "extensions/filters/listener/tls_inspector/tls_inspector.h" + #include "openssl/bytestring.h" #include "openssl/ssl.h" @@ -21,14 +23,15 @@ class Config { public: Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - const Ssl::Utility::TlsStats& stats() const { return stats_; } + const Extensions::ListenerFilters::TlsInspector::TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); uint32_t maxClientHelloSize() const { return max_client_hello_size_; } + // TODO: extract the following to common static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; private: - Ssl::Utility::TlsStats stats_; + Extensions::ListenerFilters::TlsInspector::TlsStats stats_; bssl::UniquePtr ssl_ctx_; const uint32_t max_client_hello_size_; }; From 8a8b35945f1bf47f3165940ecea00ae759fbdd57 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 28 Aug 2018 16:58:29 +0300 Subject: [PATCH 12/17] Extract common logic of tls_inspector and share it with NetworkLevelSniReader Signed-off-by: ronenschafferibm --- .../listener/tls_inspector/tls_inspector.cc | 33 +++++++++++++++---- .../listener/tls_inspector/tls_inspector.h | 6 ++++ .../network_level_sni_reader.cc | 17 +++++----- .../network_level_sni_reader.h | 1 - 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index a04d86932f84f..21ed0797ba083 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -100,6 +100,16 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { } void Filter::onALPN(const unsigned char* data, unsigned int len) { + doOnALPN(data, len, + [&](std::vector protocols) { + cb_->socket().setRequestedApplicationProtocols(protocols); + }, + alpn_found_); +} + +void Filter::doOnALPN(const unsigned char* data, unsigned int len, + std::function protocols)> onAlpnCb, + bool& alpn_found) { CBS wire, list; CBS_init(&wire, reinterpret_cast(data), static_cast(len)); if (!CBS_get_u16_length_prefixed(&wire, &list) || CBS_len(&wire) != 0 || CBS_len(&list) < 2) { @@ -115,18 +125,27 @@ void Filter::onALPN(const unsigned char* data, unsigned int len) { } protocols.emplace_back(reinterpret_cast(CBS_data(&name)), CBS_len(&name)); } - cb_->socket().setRequestedApplicationProtocols(protocols); - alpn_found_ = true; + onAlpnCb(protocols); + alpn_found = true; +} + +void Filter::onServername(absl::string_view servername) { + doOnServername( + servername, config_->stats(), + [&](absl::string_view name) -> void { cb_->socket().setRequestedServerName(name); }, + clienthello_success_); } -void Filter::onServername(absl::string_view name) { +void Filter::doOnServername(absl::string_view name, const TlsStats& stats, + std::function onServernameCb, + bool& clienthello_success) { if (!name.empty()) { - config_->stats().sni_found_.inc(); - cb_->socket().setRequestedServerName(name); + stats.sni_found_.inc(); + onServernameCb(name); } else { - config_->stats().sni_not_found_.inc(); + stats.sni_not_found_.inc(); } - clienthello_success_ = true; + clienthello_success = true; } void Filter::onRead() { diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 4d57bb3ba6137..7aa2afee085fc 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -73,6 +73,12 @@ class Filter : public Network::ListenerFilter, Logger::Loggable done, bool& alpn_found, bool& clienthello_success, std::function onSuccess); + static void doOnServername(absl::string_view name, const TlsStats& stats, + std::function onServernameCb, + bool& clienthello_success_); + static void doOnALPN(const unsigned char* data, unsigned int len, + std::function protocols)> onAlpnCb, + bool& alpn_found); private: void onRead(); diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 611cd83a5afb8..a2c3f72520494 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -69,16 +69,15 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data return done_ ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration; } -void NetworkLevelSniReaderFilter::onServername(absl::string_view name) { +void NetworkLevelSniReaderFilter::onServername(absl::string_view servername) { ENVOY_CONN_LOG(debug, "network level sni reader: servername: {}", read_callbacks_->connection(), - name); - if (!name.empty()) { - config_->stats().sni_found_.inc(); - read_callbacks_->networkLevelRequestedServerName(name); - } else { - config_->stats().sni_not_found_.inc(); - } - clienthello_success_ = true; + servername); + Extensions::ListenerFilters::TlsInspector::Filter::doOnServername( + servername, config_->stats(), + [&](absl::string_view name) -> void { + read_callbacks_->networkLevelRequestedServerName(name); + }, + clienthello_success_); } void NetworkLevelSniReaderFilter::done(bool success) { diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 707204df918be..87c1113c08317 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -4,7 +4,6 @@ #include "envoy/stats/scope.h" #include "common/common/logger.h" -#include "common/ssl/utility.h" #include "extensions/filters/listener/tls_inspector/tls_inspector.h" From e9e6f025d5970c849fbb19693812fb1d62c01a7d Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 2 Sep 2018 11:17:21 +0300 Subject: [PATCH 13/17] Extract the ssl initialize logic of tls_inspector and share it with NetworkLevelSniReader Signed-off-by: ronenschafferibm --- .../listener/tls_inspector/tls_inspector.cc | 16 ++++++++++------ .../listener/tls_inspector/tls_inspector.h | 3 ++- .../network_level_sni_reader.cc | 6 ++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 865b5e2fa7eec..2749f480c5360 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -63,10 +63,15 @@ bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ thread_local uint8_t Filter::buf_[Config::TLS_MAX_CLIENT_HELLO]; Filter::Filter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { - RELEASE_ASSERT(sizeof(buf_) >= config_->maxClientHelloSize(), ""); + initializeSsl(config->maxClientHelloSize(), sizeof(buf_), ssl_, this); +} + +void Filter::initializeSsl(uint32_t maxClientHelloSize, size_t bufSize, + const bssl::UniquePtr& ssl, void* appData) { + RELEASE_ASSERT(bufSize >= maxClientHelloSize, ""); - SSL_set_app_data(ssl_.get(), this); - SSL_set_accept_state(ssl_.get()); + SSL_set_app_data(ssl.get(), appData); + SSL_set_accept_state(ssl.get()); } Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { @@ -130,11 +135,10 @@ void Filter::doOnALPN(const unsigned char* data, unsigned int len, } void Filter::onServername(absl::string_view servername) { + ENVOY_LOG(debug, "tls:onServerName(), requestedServerName: {}", servername); doOnServername( servername, config_->stats(), - [&](absl::string_view name) -> void { - cb_->socket().setRequestedServerName(name); - ENVOY_LOG(debug, "tls:onServerName(), requestedServerName: {}", name); }, + [&](absl::string_view name) -> void { cb_->socket().setRequestedServerName(name); }, clienthello_success_); } diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 7aa2afee085fc..9f5236266f942 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -68,7 +68,8 @@ class Filter : public Network::ListenerFilter, Logger::Loggable& ssl, void* appData); static void parseClientHello(const void* data, size_t len, bssl::UniquePtr& ssl, uint64_t read, uint32_t maxClientHelloSize, const TlsStats& stats, std::function done, bool& alpn_found, diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index a2c3f72520494..4fafa90a8506e 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -45,10 +45,8 @@ thread_local uint8_t NetworkLevelSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HE NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { - RELEASE_ASSERT(sizeof(buf_) >= config_->maxClientHelloSize(), ""); - - SSL_set_app_data(ssl_.get(), this); - SSL_set_accept_state(ssl_.get()); + Extensions::ListenerFilters::TlsInspector::Filter::initializeSsl(config->maxClientHelloSize(), + sizeof(buf_), ssl_, this); } Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data, bool) { From 80c804f75eceaae1bb25a0b77461c0d9482ec83f Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 2 Sep 2018 15:41:54 +0300 Subject: [PATCH 14/17] Share tls_inspector's Config with NetworkLeveSniReader Signed-off-by: ronenschafferibm --- .../filters/listener/tls_inspector/config.cc | 2 +- .../listener/tls_inspector/tls_inspector.cc | 11 ++--- .../listener/tls_inspector/tls_inspector.h | 27 ++++++++---- .../network_level_sni_reader/config.cc | 4 +- .../network_level_sni_reader.cc | 41 ++++--------------- .../network_level_sni_reader.h | 38 ++++------------- 6 files changed, 48 insertions(+), 75 deletions(-) diff --git a/source/extensions/filters/listener/tls_inspector/config.cc b/source/extensions/filters/listener/tls_inspector/config.cc index af8f8170683c6..372294e53aaed 100644 --- a/source/extensions/filters/listener/tls_inspector/config.cc +++ b/source/extensions/filters/listener/tls_inspector/config.cc @@ -20,7 +20,7 @@ class TlsInspectorConfigFactory : public Server::Configuration::NamedListenerFil Network::ListenerFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message&, Server::Configuration::ListenerFactoryContext& context) override { - ConfigSharedPtr config(new Config(context.scope())); + ConfigSharedPtr config(new Config(context.scope(), "tls_inspector.")); return [config](Network::ListenerFilterManager& filter_manager) -> void { filter_manager.addAcceptFilter(std::make_unique(config)); }; diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 2749f480c5360..718ad3ad84df7 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -24,8 +24,8 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { -Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) - : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, +Config::Config(Stats::Scope& scope, std::string stat_prefix, uint32_t max_client_hello_size) + : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), max_client_hello_size_(max_client_hello_size) { @@ -42,14 +42,14 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) size_t len; if (SSL_early_callback_ctx_extension_get( client_hello, TLSEXT_TYPE_application_layer_protocol_negotiation, &data, &len)) { - Filter* filter = static_cast(SSL_get_app_data(client_hello->ssl)); + TlsFilterBase* filter = static_cast(SSL_get_app_data(client_hello->ssl)); filter->onALPN(data, len); } return ssl_select_cert_success; }); SSL_CTX_set_tlsext_servername_callback( ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { - Filter* filter = static_cast(SSL_get_app_data(ssl)); + TlsFilterBase* filter = static_cast(SSL_get_app_data(ssl)); filter->onServername(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)); // Return an error to stop the handshake; we have what we wanted already. @@ -63,7 +63,8 @@ bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ thread_local uint8_t Filter::buf_[Config::TLS_MAX_CLIENT_HELLO]; Filter::Filter(const ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { - initializeSsl(config->maxClientHelloSize(), sizeof(buf_), ssl_, this); + initializeSsl(config->maxClientHelloSize(), sizeof(buf_), ssl_, + static_cast(this)); } void Filter::initializeSsl(uint32_t maxClientHelloSize, size_t bufSize, diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 9f5236266f942..00e0484716926 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -43,7 +43,8 @@ struct TlsStats { */ class Config { public: - Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); + Config(Stats::Scope& scope, std::string stat_prefix, + uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); const TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); @@ -59,10 +60,24 @@ class Config { typedef std::shared_ptr ConfigSharedPtr; +class TlsFilterBase { +public: + virtual ~TlsFilterBase() {} + +private: + virtual void onALPN(const unsigned char* data, unsigned int len) PURE; + virtual void onServername(absl::string_view name) PURE; + + // Allows callbacks on the SSL_CTX to set fields in this class. + friend class Config; +}; + /** * TLS inspector listener filter. */ -class Filter : public Network::ListenerFilter, Logger::Loggable { +class Filter : public Network::ListenerFilter, + public TlsFilterBase, + Logger::Loggable { public: Filter(const ConfigSharedPtr config); @@ -85,8 +100,9 @@ class Filter : public Network::ListenerFilter, Logger::Loggable void { filter_manager.addReadFilter(std::make_shared(filter_config)); }; diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 4fafa90a8506e..05a759b6e13f0 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -15,38 +15,15 @@ namespace Extensions { namespace NetworkFilters { namespace NetworkLevelSniReader { -Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) - : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, "network_levelsni_reader."))}, - ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), - max_client_hello_size_(max_client_hello_size) { +thread_local uint8_t NetworkLevelSniReaderFilter::buf_ + [Extensions::ListenerFilters::TlsInspector::Config::TLS_MAX_CLIENT_HELLO]; - if (max_client_hello_size_ > TLS_MAX_CLIENT_HELLO) { - throw EnvoyException(fmt::format("max_client_hello_size of {} is greater than maximum of {}.", - max_client_hello_size_, size_t(TLS_MAX_CLIENT_HELLO))); - } - - 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( - ssl_ctx_.get(), [](SSL* ssl, int* out_alert, void*) -> int { - NetworkLevelSniReaderFilter* filter = - static_cast(SSL_get_app_data(ssl)); - filter->onServername(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)); - - // Return an error to stop the handshake; we have what we wanted already. - *out_alert = SSL_AD_USER_CANCELLED; - return SSL_TLSEXT_ERR_ALERT_FATAL; - }); -} - -bssl::UniquePtr Config::newSsl() { return bssl::UniquePtr{SSL_new(ssl_ctx_.get())}; } - -thread_local uint8_t NetworkLevelSniReaderFilter::buf_[Config::TLS_MAX_CLIENT_HELLO]; - -NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter(const ConfigSharedPtr config) +NetworkLevelSniReaderFilter::NetworkLevelSniReaderFilter( + const Extensions::ListenerFilters::TlsInspector::ConfigSharedPtr config) : config_(config), ssl_(config_->newSsl()) { - Extensions::ListenerFilters::TlsInspector::Filter::initializeSsl(config->maxClientHelloSize(), - sizeof(buf_), ssl_, this); + Extensions::ListenerFilters::TlsInspector::Filter::initializeSsl( + config->maxClientHelloSize(), sizeof(buf_), ssl_, + static_cast(this)); } Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data, bool) { @@ -56,8 +33,8 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data return Network::FilterStatus::Continue; } // TODO: append data to the buffer instead of overwriting it. - size_t len = - (data.length() < Config::TLS_MAX_CLIENT_HELLO) ? data.length() : Config::TLS_MAX_CLIENT_HELLO; + size_t len = (data.length() < config_->maxClientHelloSize()) ? data.length() + : config_->maxClientHelloSize(); data.copyOut(0, len, buf_); Extensions::ListenerFilters::TlsInspector::Filter::parseClientHello( diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h index 87c1113c08317..8d972cdb0d9aa 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.h @@ -15,32 +15,12 @@ namespace Extensions { namespace NetworkFilters { namespace NetworkLevelSniReader { -/** - * Global configuration for network level SNI reader. - */ -class Config { -public: - Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); - - const Extensions::ListenerFilters::TlsInspector::TlsStats& stats() const { return stats_; } - bssl::UniquePtr newSsl(); - uint32_t maxClientHelloSize() const { return max_client_hello_size_; } - - // TODO: extract the following to common - static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; - -private: - Extensions::ListenerFilters::TlsInspector::TlsStats stats_; - bssl::UniquePtr ssl_ctx_; - const uint32_t max_client_hello_size_; -}; - -typedef std::shared_ptr ConfigSharedPtr; - class NetworkLevelSniReaderFilter : public Network::ReadFilter, + public Extensions::ListenerFilters::TlsInspector::TlsFilterBase, Logger::Loggable { public: - NetworkLevelSniReaderFilter(const ConfigSharedPtr config); + NetworkLevelSniReaderFilter( + const Extensions::ListenerFilters::TlsInspector::ConfigSharedPtr config); // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data, bool end_stream) override; @@ -51,9 +31,11 @@ class NetworkLevelSniReaderFilter : public Network::ReadFilter, private: void done(bool success); - void onServername(absl::string_view name); + // Extensions::ListenerFilters::TlsInspector::TlsFilterBase + void onServername(absl::string_view name) override; + void onALPN(const unsigned char*, unsigned int) override{}; - ConfigSharedPtr config_; + Extensions::ListenerFilters::TlsInspector::ConfigSharedPtr config_; Network::ReadFilterCallbacks* read_callbacks_{}; bssl::UniquePtr ssl_; @@ -62,10 +44,8 @@ class NetworkLevelSniReaderFilter : public Network::ReadFilter, bool clienthello_success_{false}; bool done_{false}; - static thread_local uint8_t buf_[Config::TLS_MAX_CLIENT_HELLO]; - - // Allows callbacks on the SSL_CTX to set fields in this class. - friend class Config; + static thread_local uint8_t + buf_[Extensions::ListenerFilters::TlsInspector::Config::TLS_MAX_CLIENT_HELLO]; }; } // namespace NetworkLevelSniReader From 0f5f73ebbb46c4c377570dcd2ec8640d6b6ed613 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Sun, 2 Sep 2018 18:35:50 +0300 Subject: [PATCH 15/17] Append data to the buffer instead of overwriting it Signed-off-by: ronenschafferibm --- .../network_level_sni_reader.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 05a759b6e13f0..79ddc4286d547 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -32,15 +32,16 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data if (done_) { return Network::FilterStatus::Continue; } - // TODO: append data to the buffer instead of overwriting it. - size_t len = (data.length() < config_->maxClientHelloSize()) ? data.length() - : config_->maxClientHelloSize(); - data.copyOut(0, len, buf_); + size_t freeSpaceInBuf = sizeof(buf_) - read_; + size_t lenToRead = (data.length() < freeSpaceInBuf) ? data.length() : freeSpaceInBuf; + data.copyOut(0, lenToRead, buf_ + read_); + read_ += lenToRead; Extensions::ListenerFilters::TlsInspector::Filter::parseClientHello( - buf_, len, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), + buf_ + read_, lenToRead, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, []() -> void {}); + return done_ ? Network::FilterStatus::Continue : Network::FilterStatus::StopIteration; } From 487114b05cc5724842423549c19cec9776a38235 Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Thu, 6 Sep 2018 15:43:20 +0300 Subject: [PATCH 16/17] Pass the correct location in buf to parseClientHello() Signed-off-by: ronenschafferibm --- .../network_level_sni_reader/network_level_sni_reader.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc index 79ddc4286d547..af6d20b86ce17 100644 --- a/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc +++ b/source/extensions/filters/network/network_level_sni_reader/network_level_sni_reader.cc @@ -35,10 +35,11 @@ Network::FilterStatus NetworkLevelSniReaderFilter::onData(Buffer::Instance& data size_t freeSpaceInBuf = sizeof(buf_) - read_; size_t lenToRead = (data.length() < freeSpaceInBuf) ? data.length() : freeSpaceInBuf; - data.copyOut(0, lenToRead, buf_ + read_); + uint8_t* bufToParse = buf_ + read_; + data.copyOut(0, lenToRead, bufToParse); read_ += lenToRead; Extensions::ListenerFilters::TlsInspector::Filter::parseClientHello( - buf_ + read_, lenToRead, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), + bufToParse, lenToRead, ssl_, read_, config_->maxClientHelloSize(), config_->stats(), [&](bool success) -> void { done(success); }, alpn_found_, clienthello_success_, []() -> void {}); From e270efaa37f315b89b1cd73ea9b7a5ac42c0a88d Mon Sep 17 00:00:00 2001 From: ronenschafferibm Date: Tue, 4 Sep 2018 16:37:20 +0300 Subject: [PATCH 17/17] Change Config's Ctor params order and set default value to stat_prefix Signed-off-by: ronenschafferibm --- source/extensions/filters/listener/tls_inspector/config.cc | 2 +- .../filters/listener/tls_inspector/tls_inspector.cc | 2 +- .../filters/listener/tls_inspector/tls_inspector.h | 4 ++-- .../filters/network/network_level_sni_reader/config.cc | 5 +++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/listener/tls_inspector/config.cc b/source/extensions/filters/listener/tls_inspector/config.cc index 372294e53aaed..af8f8170683c6 100644 --- a/source/extensions/filters/listener/tls_inspector/config.cc +++ b/source/extensions/filters/listener/tls_inspector/config.cc @@ -20,7 +20,7 @@ class TlsInspectorConfigFactory : public Server::Configuration::NamedListenerFil Network::ListenerFilterFactoryCb createFilterFactoryFromProto(const Protobuf::Message&, Server::Configuration::ListenerFactoryContext& context) override { - ConfigSharedPtr config(new Config(context.scope(), "tls_inspector.")); + ConfigSharedPtr config(new Config(context.scope())); return [config](Network::ListenerFilterManager& filter_manager) -> void { filter_manager.addAcceptFilter(std::make_unique(config)); }; diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index 718ad3ad84df7..f3928b6500902 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -24,7 +24,7 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { -Config::Config(Stats::Scope& scope, std::string stat_prefix, uint32_t max_client_hello_size) +Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size, const std::string& stat_prefix) : stats_{TLS_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), max_client_hello_size_(max_client_hello_size) { diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index 00e0484716926..6fce0c14a00de 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -43,8 +43,8 @@ struct TlsStats { */ class Config { public: - Config(Stats::Scope& scope, std::string stat_prefix, - uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO); + Config(Stats::Scope& scope, uint32_t max_client_hello_size = TLS_MAX_CLIENT_HELLO, + const std::string& stat_prefix = "tls_inspector."); const TlsStats& stats() const { return stats_; } bssl::UniquePtr newSsl(); diff --git a/source/extensions/filters/network/network_level_sni_reader/config.cc b/source/extensions/filters/network/network_level_sni_reader/config.cc index 24bc5f3d61c6b..606a810d672fa 100644 --- a/source/extensions/filters/network/network_level_sni_reader/config.cc +++ b/source/extensions/filters/network/network_level_sni_reader/config.cc @@ -40,8 +40,9 @@ class NetworkLevelSniReaderConfigFactory Network::FilterFactoryCb createFilterFactoryFromContext(Server::Configuration::FactoryContext& context) { Extensions::ListenerFilters::TlsInspector::ConfigSharedPtr filter_config( - new Extensions::ListenerFilters::TlsInspector::Config(context.scope(), - "network_level_sni_reader.")); + new Extensions::ListenerFilters::TlsInspector::Config( + context.scope(), Extensions::ListenerFilters::TlsInspector::Config::TLS_MAX_CLIENT_HELLO, + "network_level_sni_reader.")); return [filter_config](Network::FilterManager& filter_manager) -> void { filter_manager.addReadFilter(std::make_shared(filter_config)); };