This is an automated email from the ASF dual-hosted git repository. laiyingchun pushed a commit to branch branch-1.17.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit a261fc80d63851dda7164a90c9bde59ed9b42fc2 Author: Alexey Serbin <[email protected]> AuthorDate: Wed Jun 14 19:10:30 2023 -0700 [rpc] clean up JWT-related client-side negotiation code Since now there is an API to add a trusted TLS certificate into the chain of trusted certificates of a Kudu C++ client application, the test-only flag --jwt_client_require_trusted_tls_cert is no longer needed. This patch removes the flag along with corresponding test scenario. Correspondingly, the client now verifies the server's TLS certificate during TLS handshake since there isn't a case when a client would send out its JWT to a server it doesn't trust once the --jwt_client_require_trusted_tls_cert test-only flag is removed. This patch also adds an extra logging about a connection negotiation condition when the client has a JWT, but it doesn't trust the server's TLS certificate. In addition, I took the liberty of removing a few TODOs related to KUDU-1921 since the referred functionality has already been implemented. Change-Id: I85574ed05396fcf3740d9d068afa524cf125f5ff Reviewed-on: http://gerrit.cloudera.org:8080/20076 Reviewed-by: Attila Bukor <[email protected]> Tested-by: Kudu Jenkins (cherry picked from commit 6b2077e48e1e96cf6520db09ddd8c2d3ca97334d) Reviewed-on: http://gerrit.cloudera.org:8080/20092 Reviewed-by: Yingchun Lai <[email protected]> --- src/kudu/integration-tests/security-itest.cc | 23 ------------ src/kudu/rpc/client_negotiation.cc | 53 ++++++++++++++-------------- 2 files changed, 27 insertions(+), 49 deletions(-) diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc index fdf54619f..7cb2bde4a 100644 --- a/src/kudu/integration-tests/security-itest.cc +++ b/src/kudu/integration-tests/security-itest.cc @@ -84,7 +84,6 @@ #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" -DECLARE_bool(jwt_client_require_trusted_tls_cert); DECLARE_string(local_ip_for_outbound_sockets); using kudu::client::KuduClient; @@ -644,28 +643,6 @@ TEST_F(SecurityITest, TestJwtMiniCluster) { ASSERT_STR_CONTAINS(s.ToString(), "client requires authentication, but server does not have"); } - { - SCOPED_TRACE("Valid JWT with relaxed requirements for server's TLS cert"); - KuduClientBuilder cb; - for (auto i = 0; i < cluster_->num_masters(); ++i) { - cb.add_master_server_addr(cluster_->master(i)->bound_rpc_addr().ToString()); - } - cb.jwt(cluster_->oidc()->CreateJwt(kValidAccount, kSubject, true)); - cb.require_authentication(true); - - // If not adding the CA certificate that Kudu RPC server certificates are - // signed with, in simplified test scenarios it's possible to relax the - // requirements at the client side of the Kudu RPC connection negotiation - // protocol. With --jwt_client_require_trusted_tls_cert=false, the client - // does not verify the server's TLS certificate before sending its JWT - // to the server for authentication. - FLAGS_jwt_client_require_trusted_tls_cert = false; - - shared_ptr<KuduClient> client; - ASSERT_OK(cb.Build(&client)); - vector<string> tables; - ASSERT_OK(client->ListTables(&tables)); - } } TEST_F(SecurityITest, TestJwtMiniClusterWithInvalidCert) { diff --git a/src/kudu/rpc/client_negotiation.cc b/src/kudu/rpc/client_negotiation.cc index feb3bec86..42c23f099 100644 --- a/src/kudu/rpc/client_negotiation.cc +++ b/src/kudu/rpc/client_negotiation.cc @@ -33,7 +33,6 @@ #include <glog/logging.h> #include "kudu/gutil/basictypes.h" -#include "kudu/gutil/macros.h" #include "kudu/gutil/map-util.h" #include "kudu/gutil/stl_util.h" #include "kudu/gutil/strings/join.h" @@ -51,7 +50,6 @@ #include "kudu/security/token.pb.h" #include "kudu/util/debug/leakcheck_disabler.h" #include "kudu/util/faststring.h" -#include "kudu/util/flag_tags.h" #include "kudu/util/net/sockaddr.h" #include "kudu/util/net/socket.h" #include "kudu/util/slice.h" @@ -64,16 +62,6 @@ #pragma GCC diagnostic ignored "-Wdeprecated-declarations" #endif // #if defined(__APPLE__) -DEFINE_bool(jwt_client_require_trusted_tls_cert, true, - "When this flag is set to 'false', a Kudu client is willing " - "to send its JSON Web Token over TLS connections to authenticate " - "to a Kudu server whose TLS certificate is not trusted " - "by the client. " - "NOTE: this is used for test purposes only; don't use in any other " - "use case due to security implications"); -TAG_FLAG(jwt_client_require_trusted_tls_cert, hidden); -TAG_FLAG(jwt_client_require_trusted_tls_cert, unsafe); - DECLARE_bool(rpc_encrypt_loopback_connections); using kudu::security::RpcEncryption; @@ -202,14 +190,12 @@ Status ClientNegotiation::Negotiate(unique_ptr<ErrorStatusPB>* rpc_error) { } // Step 3: if both ends support TLS, do a TLS handshake. - // TODO(KUDU-1921): allow the client to require TLS. if (encryption_ != RpcEncryption::DISABLED && ContainsKey(server_features_, TLS)) { RETURN_NOT_OK(tls_context_->InitiateHandshake(&tls_handshake_)); - if (negotiated_authn_ == AuthenticationType::SASL || - negotiated_authn_ == AuthenticationType::JWT) { - // When using SASL or JWT authentication, verifying the server's certificate is + if (negotiated_authn_ == AuthenticationType::SASL) { + // When using SASL authentication, verifying the server's certificate is // not necessary. This allows the client to still use TLS encryption for // connections to servers which only have a self-signed certificate. tls_handshake_.set_verification_mode(security::TlsVerificationMode::VERIFY_NONE); @@ -366,14 +352,31 @@ Status ClientNegotiation::SendNegotiate() { // reliably on clients? msg.add_authn_types()->mutable_token(); } - if (jwt_ && (tls_context_->has_trusted_cert() || - PREDICT_FALSE(!FLAGS_jwt_client_require_trusted_tls_cert))) { - // The client isn't sending its JWT to servers whose authenticity it cannot - // verify, otherwise its authn credentials might be stolen by an impostor. - // So, even if the client has a JWT to use, it does not advertise that - // it's capable of JWT-based authentication when it doesn't trust the - // server's TLS certificate. - msg.add_authn_types()->mutable_jwt(); + if (jwt_) { + if (tls_context_->has_trusted_cert()) { + msg.add_authn_types()->mutable_jwt(); + } else { + // The client isn't sending its JWT to a server whose authenticity + // it cannot verify, otherwise its authn credentials might be stolen + // by an impostor. So, even if the client has a JWT to use, it does not + // advertise its JWT-based authentication capability when it doesn't trust + // the server's TLS certificate. + string server_addr_str; + { + Sockaddr server_addr; + if (auto s = socket_->GetPeerAddress(&server_addr); !s.ok()) { + LOG(WARNING) << "could not deduce server's address from socket info"; + } else { + server_addr_str = server_addr.ToString(); + } + } + const string& server_info = server_addr_str.empty() + ? "server" : Substitute("server at $0", server_addr_str); + LOG(WARNING) << Substitute( + "the client has a JWT but it isn't advertising its JWT-based authn " + "capability since it doesn't trust the certificate of the $0", + server_info); + } } if (PREDICT_FALSE(msg.authn_types().empty())) { @@ -464,8 +467,6 @@ Status ClientNegotiation::HandleNegotiate(const NegotiatePB& response) { // The preference list in order of most to least preferred: // * GSSAPI // * PLAIN - // - // 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
