[rpc] tighten TLS cert requirements for JWT authn

This patch addresses a few JWT-related issues in the context of RPC
connection negotiation:

  * At the client side (C++), the negotiating actor now requires the
    server to have a trusted TLS certificate.  That's to address one
    of the TODOs in the code which is very important from the security
    standpoint.  Without verifying the authenticity of the negotiating
    server-side party, the client might send its bearer token to
    a malicious impostor that would be able to hijack the client's
    credentials, which would be is a serious security flaw in real world
    scenarios.  With the stricter requirements introduced, JWT
    authentication is now available only when the Kudu's IPKI CA cert
    is in the client's certificate bundle, or Kudu servers are run with
    TLS certificates that are signed by a reputable CA that is present
    in the client's certificate bundle.  A test-only
    --jwt_client_require_trusted_tls_cert flag is added to relax this
    requirement for test scenario simplification, abstracting away
    from certificate deployment issues (the latter is not in the scope
    of this patch).

  * At the server side, JWT authn mechanism is now advertised to the
    client only when the server has a CA-signed TLS certificate,
    so the client at least has an ability to verify the server's
    certificate.  That's similar to the part of the policy used for
    advertising the TOKEN (Kudu authentication token) mechanism.

I updated the existing JWT-related C++ tests to pass with these changes.
Correspondingly, since there isn't a way to control the newly introduced
--jwt_client_require_trusted_tls_cert flag in the Python client,
and the functionality to customize the client's CA certificate
chain/bundle is missing as of now, the TestJwt is skipped.

I hadn't touched the Java client and the existing tests didn't fail,
so I didn't look deeper.  Anyways, I was not going to update the
corresponding parts of the Java client in this patch.

There isn't currently a means to provide a bundle of trusted CA
certificates in Kudu client API, and that deficiency should be
addressed in follow-up changelists.

Change-Id: Id2b45227cc4d827b8fab2d9517c09b62135fd757
Reviewed-on: http://gerrit.cloudera.org:8080/19896
Tested-by: Kudu Jenkins
Reviewed-by: Wenzhe Zhou <[email protected]>
Reviewed-by: Zoltan Chovan <[email protected]>
Reviewed-by: Abhishek Chennaka <[email protected]>

python/kudu/tests/test_client.py

@@ -29,6 +29,7 @@
                          KuduValue)
 import kudu
 import datetime
+import unittest
 from pytz import utc
 
 
@@ -895,6 +896,7 @@ def test_require_authn(self):
                      require_authentication=True)
 
 class TestJwt(KuduTestBase, CompatUnitTest):
+    @unittest.skip("needs Kudu IPKI CA cert to be in the client's cert bundle")
     def test_jwt(self):
         jwt = self.get_jwt(valid=True)
         client = kudu.connect(self.master_hosts, self.master_ports,

src/kudu/integration-tests/security-itest.cc

@@ -80,6 +80,7 @@
 #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;
@@ -94,6 +95,8 @@ using kudu::client::sp::shared_ptr;
 using kudu::cluster::ExternalMiniCluster;
 using kudu::cluster::ExternalMiniClusterOptions;
 using kudu::rpc::Messenger;
+using kudu::security::CreateTestSSLCertWithChainSignedByRoot;
+using kudu::security::CreateTestSSLExpiredCertWithChainSignedByRoot;
 using std::get;
 using std::string;
 using std::tuple;
@@ -513,6 +516,18 @@ void GetFullBinaryPath(string* binary) {
   (*binary) = JoinPathSegments(DirName(exe), *binary);
 }
 
+namespace {
+
+// In this testing environment, TLS certificates of Kudu servers are
+// self-signed since Kudu uses its own IPKI. To simplify the testing, the
+// certificate chain of the client isn't modified to include the Kudu's
+// IPKI CA, so it's necessary to allow the client sending its JWT to a server
+// without trusted TLS certificate.
+void RelaxJwtAuthnClientRequirements() {
+  FLAGS_jwt_client_require_trusted_tls_cert = false;
+}
+
+} // anonymous namespace
 
 TEST_F(SecurityITest, TestJwtMiniCluster) {
   SKIP_IF_SLOW_NOT_ALLOWED();
@@ -534,10 +549,10 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
   string ca_certificate_file;
   string private_key_file;
   string certificate_file;
-  ASSERT_OK(kudu::security::CreateTestSSLCertWithChainSignedByRoot(GetTestDataDirectory(),
-                                                                   &certificate_file,
-                                                                   &private_key_file,
-                                                                   &ca_certificate_file));
+  ASSERT_OK(CreateTestSSLCertWithChainSignedByRoot(GetTestDataDirectory(),
+                                                   &certificate_file,
+                                                   &private_key_file,
+                                                   &ca_certificate_file));
   // set the certs and private key for the jwks webserver
   oidc_opts.private_key_file = private_key_file;
   oidc_opts.server_certificate = certificate_file;
@@ -547,6 +562,12 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
 
   cluster_opts_.mini_oidc_options = std::move(oidc_opts);
   ASSERT_OK(StartCluster());
+
+  // Wait for the catalog manager to finish initialization: this is necessary
+  // to make sure the master's TLS certificate used for RPC connections is
+  // signed with the IPKI CA, so it will list JWT as authn mechanism available.
+  ASSERT_OK(cluster_->master()->WaitForCatalogManager());
+
   const auto* const kSubject = "kudu-user";
   const auto configure_builder_for =
       [&] (const string& account_id, KuduClientBuilder* b, const uint64_t delay_ms) {
@@ -558,7 +579,9 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
     SleepFor(MonoDelta::FromMilliseconds(delay_ms));
   };
 
+  RelaxJwtAuthnClientRequirements();
   {
+    SCOPED_TRACE("Valid JWT");
     KuduClientBuilder valid_builder;
     shared_ptr<KuduClient> client;
     configure_builder_for(kValidAccount, &valid_builder, 0);
@@ -567,6 +590,7 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
     ASSERT_OK(client->ListTables(&tables));
   }
   {
+    SCOPED_TRACE("Invalid JWT");
     KuduClientBuilder invalid_builder;
     shared_ptr<KuduClient> client;
     configure_builder_for(kInvalidAccount, &invalid_builder, 0);
@@ -575,6 +599,7 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
     ASSERT_STR_CONTAINS(s.ToString(), "FATAL_INVALID_JWT");
   }
   {
+    SCOPED_TRACE("Expired JWT");
     KuduClientBuilder timeout_builder;
     shared_ptr<KuduClient> client;
     configure_builder_for(kValidAccount, &timeout_builder, 3 * kLifetimeMs);
@@ -583,6 +608,7 @@ TEST_F(SecurityITest, TestJwtMiniCluster) {
     ASSERT_STR_CONTAINS(s.ToString(), "token expired");
   }
   {
+    SCOPED_TRACE("No JWT provided");
     KuduClientBuilder no_jwt_builder;
     shared_ptr<KuduClient> client;
     for (auto i = 0; i < cluster_->num_masters(); ++i) {
@@ -610,13 +636,13 @@ TEST_F(SecurityITest, TestJwtMiniClusterWithInvalidCert) {
 
   // Set up certificates for the JWKS server
   string ca_certificate_file;
-  string private_key_file;
   string certificate_file;
-
-  ASSERT_OK(kudu::security::CreateTestSSLExpiredCertWithChainSignedByRoot(GetTestDataDirectory(),
-                                                                   &certificate_file,
-                                                                   &private_key_file,
-                                                                   &ca_certificate_file));
+  string private_key_file;
+  ASSERT_OK(CreateTestSSLExpiredCertWithChainSignedByRoot(
+      GetTestDataDirectory(),
+      &certificate_file,
+      &private_key_file,
+      &ca_certificate_file));
 
   // set the certs and private key for the jwks webserver
   oidc_opts.private_key_file = private_key_file;
@@ -628,6 +654,12 @@ TEST_F(SecurityITest, TestJwtMiniClusterWithInvalidCert) {
   cluster_opts_.mini_oidc_options = std::move(oidc_opts);
   ASSERT_OK(StartCluster());
 
+  // Wait for the catalog manager to finish initialization: this is necessary
+  // to make sure the master's TLS certificate used for RPC connections is
+  // signed with the IPKI CA, so it will list JWT as authn mechanism available.
+  ASSERT_OK(cluster_->master()->WaitForCatalogManager());
+
+  RelaxJwtAuthnClientRequirements();
   KuduClientBuilder client_builder;
   for (auto i = 0; i < cluster_->num_masters(); ++i) {
     client_builder.add_master_server_addr(cluster_->master(i)->bound_rpc_addr().ToString());
@@ -658,10 +690,10 @@ TEST_F(SecurityITest, TestJwtMiniClusterWithUntrustedCert) {
   string ca_certificate_file;
   string private_key_file;
   string certificate_file;
-  ASSERT_OK(kudu::security::CreateTestSSLCertWithChainSignedByRoot(GetTestDataDirectory(),
-                                                                   &certificate_file,
-                                                                   &private_key_file,
-                                                                   &ca_certificate_file));
+  ASSERT_OK(CreateTestSSLCertWithChainSignedByRoot(GetTestDataDirectory(),
+                                                   &certificate_file,
+                                                   &private_key_file,
+                                                   &ca_certificate_file));
   // set the certs and private key for the jwks webserver
   // jwks certificate verification is enabled by default, so we won't have to set it
   oidc_opts.private_key_file = private_key_file;
@@ -670,6 +702,12 @@ TEST_F(SecurityITest, TestJwtMiniClusterWithUntrustedCert) {
   cluster_opts_.mini_oidc_options = std::move(oidc_opts);
   ASSERT_OK(StartCluster());
 
+  // Wait for the catalog manager to finish initialization: this is necessary
+  // to make sure the master's TLS certificate used for RPC connections is
+  // signed with the IPKI CA, so it will list JWT as authn mechanism available.
+  ASSERT_OK(cluster_->master()->WaitForCatalogManager());
+
+  RelaxJwtAuthnClientRequirements();
   KuduClientBuilder client_builder;
   for (auto i = 0; i < cluster_->num_masters(); ++i) {
     client_builder.add_master_server_addr(cluster_->master(i)->bound_rpc_addr().ToString());

src/kudu/rpc/client_negotiation.cc

@@ -29,10 +29,11 @@
 #include <set>
 #include <string>
 
-#include <gflags/gflags_declare.h>
+#include <gflags/gflags.h>
 #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"
@@ -50,6 +51,7 @@
 #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"
@@ -62,6 +64,16 @@
 #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;
@@ -354,9 +366,13 @@ Status ClientNegotiation::SendNegotiate() {
     // reliably on clients?
     msg.add_authn_types()->mutable_token();
   }
-
-  if (jwt_) {
-    // TODO(zchovan): make sure that we are using a trusted certificate
+  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();
   }
 

src/kudu/rpc/negotiation-test.cc

@@ -457,7 +457,7 @@ INSTANTIATE_TEST_SUITE_P(NegotiationCombinations,
           },
           false,
           false,
-          Status::NotAuthorized(".*client is not configured with an authentication type"),
+          Status::NotAuthorized("client is not configured with an authentication type"),
           Status::NetworkError(""),
           AuthenticationType::INVALID,
           SaslMechanism::INVALID,
@@ -1119,8 +1119,18 @@ INSTANTIATE_TEST_SUITE_P(NegotiationCombinations,
           },
           true,
           true,
-          Status::OK(),
-          Status::OK(),
+
+          // 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 handy, it doesn't
+          // advertise its JWT authentication capability since the server
+          // doesn't have a TLS certificate trusted by the client (the IPKI CA
+          // certificate isn't in the client's CA certificate bundle).
+          // With that, the server sees no authentication type presented and
+          // responds with proper NotAuthorized status code.
+          Status::NotAuthorized("client is not configured with an authentication type"),
+
+          Status::NetworkError(""),
           AuthenticationType::JWT,
           SaslMechanism::INVALID,
           true,

src/kudu/rpc/server_negotiation.cc

@@ -540,7 +540,14 @@ Status ServerNegotiation::HandleNegotiate(const NegotiatePB& request) {
     // message.
     negotiated_authn_ = AuthenticationType::TOKEN;
   } else if (ContainsKey(authn_types, AuthenticationType::JWT) &&
-             encryption_ != RpcEncryption::DISABLED) {
+             encryption_ != RpcEncryption::DISABLED &&
+             tls_context_->has_signed_cert()) {
+    // The client should send its JWT only to servers that it trusts because
+    // an untrusted server could be run by a malicious impostor who might steal
+    // the client's credentials. So, a benevolent server advertises its JWT
+    // authentication ability to clients only if it has a CA-signed TLS
+    // certificate that the client might verify against its bundle of trusted
+    // CA certificates.
     negotiated_authn_ = AuthenticationType::JWT;
   } else {
     // Otherwise we always can fallback to SASL.