This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 6f4c534a08c78184330f8b44416e2b4d8b6c6ccc Author: Alexey Serbin <[email protected]> AuthorDate: Wed Mar 17 20:03:03 2021 -0700 [security] enhance tls_handshake test This patch adds extra verification step into the TestHandshakeSequence scenario of TestTlsHandshake. That's to assert that both parties can send and receive application data using OpenSSL API (i.e. SSL_Read() and SSL_Write() function) once TlsHandshake::Continue() returns Status::OK. In addition, the newly introduced utility methods in TestTlsHandshake are useful for a follow-up changelist as well. In addition, I cleaned up code in {client,server}_negotiation.cc and in tls_context.cc. Change-Id: Ie77d1f35e12fe74be9f4022e8cd7087500052743 Reviewed-on: http://gerrit.cloudera.org:8080/17198 Tested-by: Kudu Jenkins Reviewed-by: Grant Henke <[email protected]> Reviewed-by: Attila Bukor <[email protected]> --- src/kudu/rpc/client_negotiation.cc | 16 +++---- src/kudu/rpc/server_negotiation.cc | 15 +++---- src/kudu/security/tls_context.cc | 7 ++- src/kudu/security/tls_handshake-test.cc | 77 +++++++++++++++++++++++++++++++++ src/kudu/security/tls_handshake.h | 3 ++ 5 files changed, 98 insertions(+), 20 deletions(-) diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index e5696b9..d876e35 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -247,7 +247,7 @@ Status ClientNegotiation::SendNegotiatePB(const NegotiatePB& msg) { DCHECK(msg.has_step()) << "message must have a step"; TRACE("Sending $0 NegotiatePB request", NegotiatePB::NegotiateStep_Name(msg.step())); - return SendFramedMessageBlocking(socket(), header, msg, deadline_); + return SendFramedMessageBlocking(socket_.get(), header, msg, deadline_); } Status ClientNegotiation::RecvNegotiatePB(NegotiatePB* msg, @@ -255,7 +255,8 @@ Status ClientNegotiation::RecvNegotiatePB(NegotiatePB* msg, unique_ptr<ErrorStatusPB>* rpc_error) { ResponseHeader header; Slice param_buf; - RETURN_NOT_OK(ReceiveFramedMessageBlocking(socket(), buffer, &header, ¶m_buf, deadline_)); + RETURN_NOT_OK(ReceiveFramedMessageBlocking( + socket_.get(), buffer, &header, ¶m_buf, deadline_)); RETURN_NOT_OK(helper_.CheckNegotiateCallId(header.call_id())); if (header.is_error()) { @@ -263,7 +264,8 @@ Status ClientNegotiation::RecvNegotiatePB(NegotiatePB* msg, } RETURN_NOT_OK(helper_.ParseNegotiatePB(param_buf, msg)); - TRACE("Received $0 NegotiatePB response", NegotiatePB::NegotiateStep_Name(msg->step())); + TRACE("Received $0 NegotiatePB response", + NegotiatePB::NegotiateStep_Name(msg->step())); return Status::OK(); } @@ -288,7 +290,7 @@ Status ClientNegotiation::SendConnectionHeader() { uint8_t buf[buflen]; serialization::SerializeConnHeader(buf); size_t nsent; - return socket()->BlockingWrite(buf, buflen, &nsent, deadline_); + return socket_->BlockingWrite(buf, buflen, &nsent, deadline_); } Status ClientNegotiation::InitSaslClient() { @@ -352,8 +354,7 @@ Status ClientNegotiation::SendNegotiate() { return Status::NotAuthorized("client is not configured with an authentication type"); } - RETURN_NOT_OK(SendNegotiatePB(msg)); - return Status::OK(); + return SendNegotiatePB(msg); } Status ClientNegotiation::HandleNegotiate(const NegotiatePB& response) { @@ -434,7 +435,6 @@ Status ClientNegotiation::HandleNegotiate(const NegotiatePB& response) { // TODO(KUDU-1921): allow the client to require authentication. if (ContainsKey(client_mechs, SaslMechanism::GSSAPI) && ContainsKey(server_mechs, SaslMechanism::GSSAPI)) { - // Check that the client has local Kerberos credentials, and if not fall // back to an alternate mechanism. Status s = CheckGSSAPI(); @@ -730,7 +730,7 @@ Status ClientNegotiation::SendConnectionContext() { *conn_context.mutable_encoded_nonce() = ciphertext.ToString(); } - return SendFramedMessageBlocking(socket(), header, conn_context, deadline_); + return SendFramedMessageBlocking(socket_.get(), header, conn_context, deadline_); } int ClientNegotiation::GetOptionCb(const char* plugin_name, const char* option, diff --git a/src/kudu/rpc/server_negotiation.cc b/src/kudu/rpc/server_negotiation.cc index c636856..e7936ba 100644 --- a/src/kudu/rpc/server_negotiation.cc +++ b/src/kudu/rpc/server_negotiation.cc @@ -335,7 +335,8 @@ Status ServerNegotiation::PreflightCheckGSSAPI(const std::string& sasl_proto_nam Status ServerNegotiation::RecvNegotiatePB(NegotiatePB* msg, faststring* recv_buf) { RequestHeader header; Slice param_buf; - RETURN_NOT_OK(ReceiveFramedMessageBlocking(socket(), recv_buf, &header, ¶m_buf, deadline_)); + RETURN_NOT_OK(ReceiveFramedMessageBlocking( + socket_.get(), recv_buf, &header, ¶m_buf, deadline_)); Status s = helper_.CheckNegotiateCallId(header.call_id()); if (!s.ok()) { RETURN_NOT_OK(SendError(ErrorStatusPB::FATAL_INVALID_RPC_HEADER, s)); @@ -361,7 +362,7 @@ Status ServerNegotiation::SendNegotiatePB(const NegotiatePB& msg) { DCHECK(msg.has_step()) << "message must have a step"; TRACE("Sending $0 NegotiatePB response", NegotiatePB::NegotiateStep_Name(msg.step())); - return SendFramedMessageBlocking(socket(), header, msg, deadline_); + return SendFramedMessageBlocking(socket_.get(), header, msg, deadline_); } Status ServerNegotiation::SendError(ErrorStatusPB::RpcErrorCodePB code, const Status& err) { @@ -378,9 +379,7 @@ Status ServerNegotiation::SendError(ErrorStatusPB::RpcErrorCodePB code, const St msg.set_message(err.ToString()); TRACE("Sending RPC error: $0: $1", ErrorStatusPB::RpcErrorCodePB_Name(code), err.ToString()); - RETURN_NOT_OK(SendFramedMessageBlocking(socket(), header, msg, deadline_)); - - return Status::OK(); + return SendFramedMessageBlocking(socket_.get(), header, msg, deadline_); } Status ServerNegotiation::ValidateConnectionHeader(faststring* recv_buf) { @@ -919,15 +918,15 @@ Status ServerNegotiation::SendSaslSuccess() { } } - RETURN_NOT_OK(SendNegotiatePB(response)); - return Status::OK(); + return SendNegotiatePB(response); } Status ServerNegotiation::RecvConnectionContext(faststring* recv_buf) { TRACE("Waiting for connection context"); RequestHeader header; Slice param_buf; - RETURN_NOT_OK(ReceiveFramedMessageBlocking(socket(), recv_buf, &header, ¶m_buf, deadline_)); + RETURN_NOT_OK(ReceiveFramedMessageBlocking( + socket_.get(), recv_buf, &header, ¶m_buf, deadline_)); DCHECK(header.IsInitialized()); if (header.call_id() != kConnectionContextCallId) { diff --git a/src/kudu/security/tls_context.cc b/src/kudu/security/tls_context.cc index 0ba7053..52d8f0a 100644 --- a/src/kudu/security/tls_context.cc +++ b/src/kudu/security/tls_context.cc @@ -78,6 +78,7 @@ #define TLS1_2_VERSION 0x0303 #endif +using kudu::security::ca::CertRequestGenerator; using strings::Substitute; using std::string; using std::unique_lock; @@ -97,8 +98,6 @@ TAG_FLAG(openssl_security_level_override, unsafe); namespace kudu { namespace security { -using ca::CertRequestGenerator; - template<> struct SslTypeTraits<SSL> { static constexpr auto kFreeFunc = &SSL_free; }; @@ -127,8 +126,8 @@ Status CheckMaxSupportedTlsVersion(int tls_version, const char* tls_version_str) } // anonymous namespace TlsContext::TlsContext() - : tls_ciphers_(kudu::security::SecurityDefaults::kDefaultTlsCiphers), - tls_min_protocol_(kudu::security::SecurityDefaults::kDefaultTlsMinVersion), + : tls_ciphers_(SecurityDefaults::kDefaultTlsCiphers), + tls_min_protocol_(SecurityDefaults::kDefaultTlsMinVersion), lock_(RWMutex::Priority::PREFER_READING), trusted_cert_count_(0), has_cert_(false), diff --git a/src/kudu/security/tls_handshake-test.cc b/src/kudu/security/tls_handshake-test.cc index 698af49..c3dc94c 100644 --- a/src/kudu/security/tls_handshake-test.cc +++ b/src/kudu/security/tls_handshake-test.cc @@ -17,6 +17,8 @@ #include "kudu/security/tls_handshake.h" +#include <openssl/ssl.h> + #include <atomic> #include <iostream> #include <string> @@ -31,6 +33,7 @@ #include "kudu/security/ca/cert_management.h" #include "kudu/security/cert.h" #include "kudu/security/crypto.h" +#include "kudu/security/openssl_util.h" #include "kudu/security/security-test-util.h" #include "kudu/security/tls_context.h" #include "kudu/util/monotime.h" @@ -127,6 +130,67 @@ class TestTlsHandshakeBase : public KuduTest { return Status::OK(); } + // Read data through the specified SSL handle using OpenSSL API. + static void ReadAndCompare(SSL* ssl, const string& expected_data) { + ASSERT_GT(expected_data.size(), 0); + string result; + string buf; + buf.resize(expected_data.size()); + int yet_to_read = expected_data.size(); + while (yet_to_read > 0) { + auto bytes_read = SSL_read(ssl, buf.data(), yet_to_read); + if (bytes_read <= 0) { + auto error_code = SSL_get_error(ssl, bytes_read); + EXPECT_EQ(SSL_ERROR_WANT_READ, error_code); + if (error_code != SSL_ERROR_WANT_READ) { + FAIL() << "OpenSSL error: " << GetSSLErrorDescription(error_code); + } + continue; + } + yet_to_read -= bytes_read; + result += buf.substr(0, bytes_read); + } + ASSERT_EQ(expected_data, result); + } + + // Write data through the specified SSL handle using OpenSSL API. + static void Write(SSL* ssl, const string& data) { + // TODO(aserbin): handle SSL_ERROR_WANT_WRITE if transferring more data + auto bytes_written = SSL_write(ssl, data.data(), data.size()); + EXPECT_EQ(data.size(), bytes_written); + if (bytes_written != data.size()) { + auto error_code = SSL_get_error(ssl, bytes_written); + FAIL() << "OpenSSL error: " << GetSSLErrorDescription(error_code); + } + } + + // Transfer data between the source and the destination SSL handles. + // + // Since the TlsHandshake class uses memory BIO for running the TLS handshake, + // data between the client and the server TlsHandshake objects is transferred + // using the BIO read/write API on the encrypted side of the TLS engine. + // This diagram illustrates the approach used by the data transfer method + // below: + // +-----+ + // +--> BIO_write(rbio) -->| |--> SSL_read(ssl) --> application data IN + // | | SSL | + // +---- BIO_read(wbio) <--| |<-- SSL_write(ssl) <-- application data OUT + // +-----+ + static void Transfer(SSL* src, SSL* dst) { + BIO* wbio = SSL_get_wbio(src); + int pending_wr = BIO_ctrl_pending(wbio); + + string data; + data.resize(pending_wr); + auto bytes_read = BIO_read(wbio, &data[0], data.size()); + ASSERT_EQ(data.size(), bytes_read); + ASSERT_EQ(0, BIO_ctrl_pending(wbio)); + + BIO* rbio = SSL_get_rbio(dst); + auto bytes_written = BIO_write(rbio, &data[0], data.size()); + ASSERT_EQ(data.size(), bytes_written); + } + TlsContext client_tls_; TlsContext server_tls_; @@ -213,6 +277,19 @@ TEST_F(TestTlsHandshake, TestHandshakeSequence) { // Client receives server Finished ASSERT_OK(client.Continue(buf1, &buf2)); ASSERT_EQ(buf2.size(), 0); + + auto* client_ssl = client.ssl(); + auto* server_ssl = server.ssl(); + + // Make sure it's possible to exchange data between the client and the server + // once the TLS handshake is complete. + NO_FATALS(Write(client_ssl, "hello")); + NO_FATALS(Transfer(client_ssl, server_ssl)); + NO_FATALS(ReadAndCompare(server_ssl, "hello")); + + NO_FATALS(Write(client_ssl, "bye")); + NO_FATALS(Transfer(client_ssl, server_ssl)); + NO_FATALS(ReadAndCompare(server_ssl, "bye")); } // Tests that the TlsContext can transition from self signed cert to signed diff --git a/src/kudu/security/tls_handshake.h b/src/kudu/security/tls_handshake.h index 41a7627..70cdc94 100644 --- a/src/kudu/security/tls_handshake.h +++ b/src/kudu/security/tls_handshake.h @@ -22,6 +22,7 @@ #include <string> #include <glog/logging.h> +#include <gtest/gtest_prod.h> #include "kudu/gutil/port.h" #include "kudu/security/cert.h" @@ -138,6 +139,8 @@ class TlsHandshake { std::string GetCipherDescription() const; private: + FRIEND_TEST(TestTlsHandshake, TestHandshakeSequence); + // Set the verification mode on the underlying SSL object. void SetSSLVerify();
