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
The following commit(s) were added to refs/heads/master by this push:
new 6b2077e48 [rpc] clean up JWT-related client-side negotiation code
6b2077e48 is described below
commit 6b2077e48e1e96cf6520db09ddd8c2d3ca97334d
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
---
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
