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 0ade8c6f21f0887e90b261ae6b1a57f4a6d1eff1 Author: Alexey Serbin <[email protected]> AuthorDate: Wed Oct 20 16:39:22 2021 -0700 KUDU-3297 fix Thrift client used for HMS integration As it turns out, in the context of KUDU-3297, the SASL negotiation code needs to be updated in one more place: src/kudu/thrift/sasl_client_transport.cc I also thought about unifying the code between the Thrift client and the RPC code to have a single place to have the correct ordering between the calls EnableProtection() and sasl_client_start(), but after some consideration I realized it's not worth it. As for the testing, I verified that before this patch the following scenarios in hms_client-test were failing every time when running on RedHat/CentOS 8.4: * ProtectionTypes/HmsClientTest.TestHmsOperations/1 * ProtectionTypes/HmsClientTest.TestHmsOperations/3 * ProtectionTypes/HmsClientTest.TestLargeObjects/1 * ProtectionTypes/HmsClientTest.TestLargeObjects/3 The output of the failed test scenarios always contained the following: Bad status: Runtime error: failed to open Hive Metastore connection: SASL(-15): mechanism too weak for this user: With this patch, all scenarios of the hms_client-test pass when running on RedHat/CentOS 8.4: This is a follow-up to fff48ea4e5eadd365a85a05a82f66b3eb76d0b0b. Change-Id: Ic6af12932647eda7092f9f42a57eb211fe31f062 Reviewed-on: http://gerrit.cloudera.org:8080/17958 Tested-by: Kudu Jenkins Reviewed-by: Bankim Bhavsar <[email protected]> Reviewed-by: Abhishek Chennaka <[email protected]> Reviewed-by: Attila Bukor <[email protected]> --- src/kudu/rpc/client_negotiation.cc | 2 +- src/kudu/thrift/sasl_client_transport.cc | 23 +++++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index 9557ff3..59f3886 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -609,7 +609,7 @@ Status ClientNegotiation::SendSaslInitiate() { &negotiated_mech); // Filled in on success. }, kDesc); - if (PREDICT_FALSE(!s.IsIncomplete() && !s.ok())) { + if (PREDICT_FALSE(!s.ok() && !s.IsIncomplete())) { return s; } diff --git a/src/kudu/thrift/sasl_client_transport.cc b/src/kudu/thrift/sasl_client_transport.cc index f12045b..a1766c3 100644 --- a/src/kudu/thrift/sasl_client_transport.cc +++ b/src/kudu/thrift/sasl_client_transport.cc @@ -341,11 +341,18 @@ NegotiationStatus SaslClientTransport::ReceiveSaslMessage(faststring* payload) { } void SaslClientTransport::SendSaslStart() { + auto s = rpc::EnableProtection(sasl_conn_.get(), + rpc::SaslProtection::kAuthentication, + max_recv_buf_size_); + if (PREDICT_FALSE(!s.ok())) { + throw SaslException(std::move(s)); + } + const char* init_msg = nullptr; unsigned init_msg_len = 0; const char* negotiated_mech = nullptr; - Status s = WrapSaslCall(sasl_conn_.get(), [&] { + s = WrapSaslCall(sasl_conn_.get(), [&] { return sasl_client_start( sasl_conn_.get(), // The SASL connection context created by sasl_client_new() SaslMechanism::name_of(SaslMechanism::GSSAPI), // The mechanism to use. @@ -355,18 +362,12 @@ void SaslClientTransport::SendSaslStart() { &negotiated_mech); // Filled in on success. }, "calling sasl_client_start()"); - if (PREDICT_FALSE(!s.IsIncomplete() && !s.ok())) { + if (PREDICT_FALSE(!s.ok() && !s.IsIncomplete())) { throw SaslException(std::move(s)); } // Check that the SASL library is using the mechanism that we picked. DCHECK_EQ(SaslMechanism::value_of(negotiated_mech), SaslMechanism::GSSAPI); - s = rpc::EnableProtection(sasl_conn_.get(), - rpc::SaslProtection::kAuthentication, - max_recv_buf_size_); - if (!s.ok()) { - throw SaslException(s); - } // These two calls comprise a single message in the thrift-sasl protocol. SendSaslMessage(TSASL_START, Slice(negotiated_mech)); @@ -374,8 +375,10 @@ void SaslClientTransport::SendSaslStart() { transport_->flush(); } -int SaslClientTransport::GetOptionCb(const char* plugin_name, const char* option, - const char** result, unsigned* len) { +int SaslClientTransport::GetOptionCb(const char* plugin_name, + const char* option, + const char** result, + unsigned* len) { return sasl_helper_.GetOptionCb(plugin_name, option, result, len); }
