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 72b325312 [rpc] tighten TLS cert requirements for JWT authn
72b325312 is described below
commit 72b3253124d009514526402ab259296881bc63ae
Author: Alexey Serbin <ale...@apache.org>
AuthorDate: Mon May 15 23:55:32 2023 -0700
[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 <wz...@cloudera.com>
Reviewed-by: Zoltan Chovan <zcho...@cloudera.com>
Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com>
---
python/kudu/tests/test_client.py | 2 +
src/kudu/integration-tests/security-itest.cc | 66 ++++++++++++++++++++++------
src/kudu/rpc/client_negotiation.cc | 24 ++++++++--
src/kudu/rpc/negotiation-test.cc | 16 +++++--
src/kudu/rpc/server_negotiation.cc | 9 +++-
5 files changed, 95 insertions(+), 22 deletions(-)
diff --git a/python/kudu/tests/test_client.py b/python/kudu/tests/test_client.py
index 3978dc112..7c836d751 100755
--- a/python/kudu/tests/test_client.py
+++ b/python/kudu/tests/test_client.py
@@ -29,6 +29,7 @@ from kudu.schema import (Schema,
KuduValue)
import kudu
import datetime
+import unittest
from pytz import utc
@@ -895,6 +896,7 @@ class TestAuthAndEncription(KuduTestBase, CompatUnitTest):
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,
diff --git a/src/kudu/integration-tests/security-itest.cc
b/src/kudu/integration-tests/security-itest.cc
index 71b5727e9..d3ddad2d3 100644
--- a/src/kudu/integration-tests/security-itest.cc
+++ b/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());
diff --git a/src/kudu/rpc/client_negotiation.cc
b/src/kudu/rpc/client_negotiation.cc
index 7fc9f48a8..feb3bec86 100644
--- a/src/kudu/rpc/client_negotiation.cc
+++ b/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();
}
diff --git a/src/kudu/rpc/negotiation-test.cc b/src/kudu/rpc/negotiation-test.cc
index b4d313844..84504afcd 100644
--- a/src/kudu/rpc/negotiation-test.cc
+++ b/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,
diff --git a/src/kudu/rpc/server_negotiation.cc
b/src/kudu/rpc/server_negotiation.cc
index 91c636595..1e2caa606 100644
--- a/src/kudu/rpc/server_negotiation.cc
+++ b/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.
