diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index db69c76aa34b..cb819ce3cedb 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -72,45 +72,46 @@ Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { ENVOY_LOG(debug, "tls inspector: new connection accepted"); Network::ConnectionSocket& socket = cb.socket(); ASSERT(file_event_ == nullptr); + cb_ = &cb; ParseState parse_state = onRead(); switch (parse_state) { case ParseState::Error: + // As per discussion in https://github.com/envoyproxy/envoy/issues/7864 + // we don't add new enum in FilterStatus so we have to signal the caller + // the new condition. cb.socket().close(); return Network::FilterStatus::StopIteration; case ParseState::Done: return Network::FilterStatus::Continue; case ParseState::Continue: // do nothing but create the event - break; + file_event_ = cb.dispatcher().createFileEvent( + socket.ioHandle().fd(), + [this](uint32_t events) { + if (events & Event::FileReadyType::Closed) { + config_->stats().connection_closed_.inc(); + done(false); + return; + } + + ASSERT(events == Event::FileReadyType::Read); + ParseState parse_state = onRead(); + switch (parse_state) { + case ParseState::Error: + done(false); + break; + case ParseState::Done: + done(true); + break; + case ParseState::Continue: + // do nothing but wait for the next event + break; + } + }, + Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed); + return Network::FilterStatus::StopIteration; } - file_event_ = cb.dispatcher().createFileEvent( - socket.ioHandle().fd(), - [this](uint32_t events) { - if (events & Event::FileReadyType::Closed) { - config_->stats().connection_closed_.inc(); - done(false); - return; - } - - ASSERT(events == Event::FileReadyType::Read); - ParseState parse_state = onRead(); - switch (parse_state) { - case ParseState::Error: - done(false); - break; - case ParseState::Done: - done(true); - break; - case ParseState::Continue: - // do nothing but wait for the next event - break; - } - }, - Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed); - - cb_ = &cb; - return Network::FilterStatus::StopIteration; } void Filter::onALPN(const unsigned char* data, unsigned int len) { diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index cf9cdb8b6e85..ee353ce9ef08 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -36,7 +36,14 @@ struct TlsInspectorStats { ALL_TLS_INSPECTOR_STATS(GENERATE_COUNTER_STRUCT) }; -enum class ParseState { Done, Continue, Error }; +enum class ParseState { + // Parse result is out. It could be tls or not. + Done, + // Parser expects more data. + Continue, + // Parser reports unrecoverable error. + Error +}; /** * Global configuration for TLS inspector. */ diff --git a/source/server/connection_handler_impl.cc b/source/server/connection_handler_impl.cc index 82076030ef8a..4beb8f2ecd16 100644 --- a/source/server/connection_handler_impl.cc +++ b/source/server/connection_handler_impl.cc @@ -184,6 +184,7 @@ void ConnectionHandlerImpl::ActiveSocket::unlink() { void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) { if (success) { + bool no_error = true; if (iter_ == accept_filters_.end()) { iter_ = accept_filters_.begin(); } else { @@ -196,15 +197,19 @@ void ConnectionHandlerImpl::ActiveSocket::continueFilterChain(bool success) { // The filter is responsible for calling us again at a later time to continue the filter // chain from the next filter. if (!socket().ioHandle().isOpen()) { - return; + // break the loop but should not create new connection + no_error = false; + break; } else { - unlink(); + // Blocking at the filter but no error return; } } } // Successfully ran all the accept filters. - newConnection(); + if (no_error) { + newConnection(); + } } // Filter execution concluded, unlink and delete this ActiveSocket if it was linked. diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index 63cff699cffe..d9793c98a8e6 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -43,6 +43,13 @@ class TlsInspectorTest : public testing::Test { EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_)); EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_)); + // Prepare the first recv attempt during + EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) + .WillOnce( + Invoke([](int fd, void* buffer, size_t length, int flag) -> Api::SysCallSizeResult { + ENVOY_LOG_MISC(error, "In mock syscall recv {} {} {} {}", fd, buffer, length, flag); + return Api::SysCallSizeResult{static_cast(0), 0}; + })); EXPECT_CALL(dispatcher_, createFileEvent_(_, _, Event::FileTriggerType::Edge, Event::FileReadyType::Read | Event::FileReadyType::Closed)) @@ -231,6 +238,36 @@ TEST_F(TlsInspectorTest, NotSsl) { EXPECT_EQ(1, cfg_->stats().tls_not_found_.value()); } +TEST_F(TlsInspectorTest, InlineReadSucceed) { + filter_ = std::make_unique(cfg_); + + EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_)); + EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_)); + EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_)); + const std::vector alpn_protos = {absl::string_view("h2")}; + const std::string servername("example.com"); + std::vector client_hello = Tls::Test::generateClientHello(servername, "\x02h2"); + + EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) + .WillOnce(Invoke( + [&client_hello](int fd, void* buffer, size_t length, int flag) -> Api::SysCallSizeResult { + ENVOY_LOG_MISC(trace, "In mock syscall recv {} {} {} {}", fd, buffer, length, flag); + ASSERT(length >= client_hello.size()); + memcpy(buffer, client_hello.data(), client_hello.size()); + return Api::SysCallSizeResult{ssize_t(client_hello.size()), 0}; + })); + + // No event is created if the inline recv parse the hello. + EXPECT_CALL(dispatcher_, + createFileEvent_(_, _, Event::FileTriggerType::Edge, + Event::FileReadyType::Read | Event::FileReadyType::Closed)) + .Times(0); + + EXPECT_CALL(socket_, setRequestedServerName(Eq(servername))); + EXPECT_CALL(socket_, setRequestedApplicationProtocols(alpn_protos)); + EXPECT_CALL(socket_, setDetectedTransportProtocol(absl::string_view("tls"))); + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onAccept(cb_)); +} } // namespace } // namespace TlsInspector } // namespace ListenerFilters