From 21c49311976b443c5f624067822c42d465624946 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 11 Jun 2014 01:16:49 +0900 Subject: [PATCH] nghttpx: Get rid of openssl filter Libevent Openssl filter is very inconvenient in various respect. The most annoying thing is it somehow emits data when SSL_shutdown is called. The reason we introduced this filter solution is drop connection if TLS renegotiation is detected. This commit implements renegotiation detection and drop connection without filtering. --- src/shrpx_client_handler.cc | 115 +++++++++++++----------------------- src/shrpx_client_handler.h | 3 +- src/shrpx_ssl.cc | 3 +- 3 files changed, 42 insertions(+), 79 deletions(-) diff --git a/src/shrpx_client_handler.cc b/src/shrpx_client_handler.cc index 1dedac16..3318a7a7 100644 --- a/src/shrpx_client_handler.cc +++ b/src/shrpx_client_handler.cc @@ -60,12 +60,6 @@ void upstream_writecb(bufferevent *bev, void *arg) { auto handler = static_cast(arg); - if(handler->get_teardown()) { - // It seems that after calling SSL_shutdown(), this function is - // somehow called from ClientHandler dtor. - return; - } - // We actually depend on write low-water mark == 0. if(handler->get_outbuf_length() > 0) { // Possibly because of deferred callback, we may get this callback @@ -229,30 +223,6 @@ void upstream_http1_connhd_readcb(bufferevent *bev, void *arg) } } // namespace -namespace { -void tls_raw_readcb(evbuffer *buffer, const evbuffer_cb_info *info, void *arg) -{ - auto handler = static_cast(arg); - if(handler->get_tls_renegotiation()) { - if(LOG_ENABLED(INFO)) { - CLOG(INFO, handler) << "Close connection due to TLS renegotiation"; - } - delete handler; - } -} -} // namespace - -namespace { -void tls_raw_writecb(evbuffer *buffer, const evbuffer_cb_info *info, void *arg) -{ - auto handler = static_cast(arg); - // upstream_writecb() is called when external bufferevent - // handler->bev's output buffer gets empty. But the underlying - // bufferevent may have pending output buffer. - upstream_writecb(handler->get_bev(), handler); -} -} // namespace - ClientHandler::ClientHandler(bufferevent *bev, bufferevent_rate_limit_group *rate_limit_group, int fd, SSL *ssl, @@ -261,27 +231,21 @@ ClientHandler::ClientHandler(bufferevent *bev, bev_(bev), http2session_(nullptr), ssl_(ssl), + reneg_shutdown_timerev_(nullptr), left_connhd_len_(NGHTTP2_CLIENT_CONNECTION_PREFACE_LEN), fd_(fd), should_close_after_write_(false), tls_handshake_(false), - tls_renegotiation_(false), - teardown_(false) + tls_renegotiation_(false) { int rv; - auto rate_limit_bev = bufferevent_get_underlying(bev_); - if(!rate_limit_bev) { - rate_limit_bev = bev_; - } - - rv = bufferevent_set_rate_limit(rate_limit_bev, - get_config()->rate_limit_cfg); + rv = bufferevent_set_rate_limit(bev_, get_config()->rate_limit_cfg); if(rv == -1) { CLOG(FATAL, this) << "bufferevent_set_rate_limit() failed"; } - rv = bufferevent_add_to_rate_limit_group(rate_limit_bev, rate_limit_group); + rv = bufferevent_add_to_rate_limit_group(bev_, rate_limit_group); if(rv == -1) { CLOG(FATAL, this) << "bufferevent_add_to_rate_limit_group() failed"; } @@ -293,10 +257,6 @@ ClientHandler::ClientHandler(bufferevent *bev, if(ssl_) { SSL_set_app_data(ssl_, reinterpret_cast(this)); set_bev_cb(nullptr, upstream_writecb, upstream_eventcb); - auto input = bufferevent_get_input(bufferevent_get_underlying(bev_)); - evbuffer_add_cb(input, tls_raw_readcb, this); - auto output = bufferevent_get_output(bufferevent_get_underlying(bev_)); - evbuffer_add_cb(output, tls_raw_writecb, this); } else { // For non-TLS version, first create HttpsUpstream. It may be // upgraded to HTTP/2 through HTTP Upgrade or direct HTTP/2 @@ -308,35 +268,29 @@ ClientHandler::ClientHandler(bufferevent *bev, ClientHandler::~ClientHandler() { - teardown_ = true; - if(LOG_ENABLED(INFO)) { CLOG(INFO, this) << "Deleting"; } + if(reneg_shutdown_timerev_) { + event_free(reneg_shutdown_timerev_); + } + if(ssl_) { SSL_set_app_data(ssl_, nullptr); SSL_set_shutdown(ssl_, SSL_RECEIVED_SHUTDOWN); SSL_shutdown(ssl_); } - auto underlying = bufferevent_get_underlying(bev_); - - if(underlying) { - bufferevent_remove_from_rate_limit_group(underlying); - } else { - bufferevent_remove_from_rate_limit_group(bev_); - } + bufferevent_remove_from_rate_limit_group(bev_); bufferevent_disable(bev_, EV_READ | EV_WRITE); bufferevent_free(bev_); + if(ssl_) { SSL_free(ssl_); } - if(underlying) { - bufferevent_disable(underlying, EV_READ | EV_WRITE); - bufferevent_free(underlying); - } + shutdown(fd_, SHUT_WR); close(fd_); for(auto dconn : dconn_pool_) { @@ -372,13 +326,7 @@ void ClientHandler::set_bev_cb void ClientHandler::set_upstream_timeouts(const timeval *read_timeout, const timeval *write_timeout) { - auto bev = bufferevent_get_underlying(bev_); - - if(!bev) { - bev = bev_; - } - - bufferevent_set_timeouts(bev, read_timeout, write_timeout); + bufferevent_set_timeouts(bev_, read_timeout, write_timeout); } int ClientHandler::validate_next_proto() @@ -521,12 +469,7 @@ DownstreamConnection* ClientHandler::get_downstream_connection() size_t ClientHandler::get_outbuf_length() { - auto underlying = bufferevent_get_underlying(bev_); - auto len = evbuffer_get_length(bufferevent_get_output(bev_)); - if(underlying) { - len += evbuffer_get_length(bufferevent_get_output(underlying)); - } - return len; + return evbuffer_get_length(bufferevent_get_output(bev_)); } SSL* ClientHandler::get_ssl() const @@ -597,6 +540,19 @@ std::string ClientHandler::get_upstream_scheme() const } } +namespace { +void shutdown_cb(evutil_socket_t fd, short what, void *arg) +{ + auto handler = static_cast(arg); + + if(LOG_ENABLED(INFO)) { + CLOG(INFO, handler) << "Close connection due to TLS renegotiation"; + } + + delete handler; +} +} // namespace + void ClientHandler::set_tls_handshake(bool f) { tls_handshake_ = f; @@ -609,6 +565,20 @@ bool ClientHandler::get_tls_handshake() const void ClientHandler::set_tls_renegotiation(bool f) { + if(tls_renegotiation_ == false) { + if(LOG_ENABLED(INFO)) { + CLOG(INFO, this) << "TLS renegotiation detected. " + << "Start shutdown timer now."; + } + + reneg_shutdown_timerev_ = evtimer_new(get_evbase(), shutdown_cb, this); + event_priority_set(reneg_shutdown_timerev_, 0); + + timeval timeout = {0, 0}; + + // TODO What to do if this failed? + evtimer_add(reneg_shutdown_timerev_, &timeout); + } tls_renegotiation_ = f; } @@ -617,9 +587,4 @@ bool ClientHandler::get_tls_renegotiation() const return tls_renegotiation_; } -bool ClientHandler::get_teardown() const -{ - return teardown_; -} - } // namespace shrpx diff --git a/src/shrpx_client_handler.h b/src/shrpx_client_handler.h index d1b51486..92fde773 100644 --- a/src/shrpx_client_handler.h +++ b/src/shrpx_client_handler.h @@ -85,7 +85,6 @@ public: bool get_tls_handshake() const; void set_tls_renegotiation(bool f); bool get_tls_renegotiation() const; - bool get_teardown() const; private: std::set dconn_pool_; std::unique_ptr upstream_; @@ -95,13 +94,13 @@ private: // HTTP2. Not deleted by this object. Http2Session *http2session_; SSL *ssl_; + event *reneg_shutdown_timerev_; // The number of bytes of HTTP/2 client connection header to read size_t left_connhd_len_; int fd_; bool should_close_after_write_; bool tls_handshake_; bool tls_renegotiation_; - bool teardown_; }; } // namespace shrpx diff --git a/src/shrpx_ssl.cc b/src/shrpx_ssl.cc index c81685b2..1313d6a8 100644 --- a/src/shrpx_ssl.cc +++ b/src/shrpx_ssl.cc @@ -494,8 +494,7 @@ ClientHandler* accept_connection // To detect TLS renegotiation and deal with it, we have to use // filter-based OpenSSL bufferevent and set evbuffer callback by // our own. - auto underlying_bev = bufferevent_socket_new(evbase, fd, 0); - bev = bufferevent_openssl_filter_new(evbase, underlying_bev, ssl, + bev = bufferevent_openssl_socket_new(evbase, fd, ssl, BUFFEREVENT_SSL_ACCEPTING, BEV_OPT_DEFER_CALLBACKS); } else {