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 c11c028b4 [rpc] validate security-related parameters earlier c11c028b4 is described below commit c11c028b43349a69cce29653e8db4032825e5aa9 Author: Alexey Serbin <ale...@apache.org> AuthorDate: Fri Jan 26 16:14:08 2024 -0800 [rpc] validate security-related parameters earlier This patch removes the verification of a few security-related RPC flags from the connection negotiation time (i.e. DoServerNegotiation()) since that's already done in one of the group flag validators. Apparently, this way it's more optimal: * misconfiguration are spotted earlier: if this type of issue is detected, the outcome is visible and actionable outright since the server simply refuses to start, reporting on the issue * there isn't a way to have a Kudu server running with such an issue * some amount of CPU resources is spared since a server no longer performs the same verification over and over again when establishing a new RPC connection, but performs it once during the start-up time Change-Id: Ie16537c440041c9ee74276da35f2599592ef7e04 Reviewed-on: http://gerrit.cloudera.org:8080/20961 Tested-by: Alexey Serbin <ale...@apache.org> Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com> --- src/kudu/integration-tests/CMakeLists.txt | 1 + src/kudu/integration-tests/security-flags-itest.cc | 112 +++++++++++++++++++++ src/kudu/rpc/negotiation.cc | 11 +- 3 files changed, 117 insertions(+), 7 deletions(-) diff --git a/src/kudu/integration-tests/CMakeLists.txt b/src/kudu/integration-tests/CMakeLists.txt index 0e7ca24b5..b40359fc6 100644 --- a/src/kudu/integration-tests/CMakeLists.txt +++ b/src/kudu/integration-tests/CMakeLists.txt @@ -115,6 +115,7 @@ ADD_KUDU_TEST(replace_tablet-itest) ADD_KUDU_TEST(registration-test RESOURCE_LOCK "master-web-port") ADD_KUDU_TEST(same_tablet_concurrent_writes-itest) ADD_KUDU_TEST(security-faults-itest) +ADD_KUDU_TEST(security-flags-itest) ADD_KUDU_TEST(security-itest) ADD_KUDU_TEST(security-master-auth-itest) ADD_KUDU_TEST(security-unknown-tsk-itest PROCESSORS 4) diff --git a/src/kudu/integration-tests/security-flags-itest.cc b/src/kudu/integration-tests/security-flags-itest.cc new file mode 100644 index 000000000..81476215e --- /dev/null +++ b/src/kudu/integration-tests/security-flags-itest.cc @@ -0,0 +1,112 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include <string> +#include <utility> +#include <vector> + +#include <gflags/gflags.h> +#include <gtest/gtest.h> + +#include "kudu/mini-cluster/external_mini_cluster.h" +#include "kudu/util/flags.h" +#include "kudu/util/status.h" +#include "kudu/util/test_macros.h" +#include "kudu/util/test_util.h" + +using gflags::SetCommandLineOption; +using kudu::cluster::ExternalMiniCluster; +using kudu::cluster::ExternalMiniClusterOptions; + +namespace kudu { +namespace security { + +class SecurityFlagsTest : public KuduTest { +}; + +TEST_F(SecurityFlagsTest, CheckRpcAuthnFlagsGroupValidator) { + // Do not rely on default values for the flags in question but explicitly + // set them to the required values instead to verify the functionality + // of the corresponding group flag validator. + ASSERT_NE("", SetCommandLineOption("unlock_experimental_flags", "true")); + ASSERT_NE("", SetCommandLineOption("rpc_authentication", "required")); + ASSERT_NE("", SetCommandLineOption("keytab_file", "")); + ASSERT_NE("", SetCommandLineOption("rpc_certificate_file", "")); + ASSERT_DEATH({ ValidateFlags(); }, + ".*RPC authentication \\(--rpc_authentication\\) may not be " + "required unless Kerberos \\(--keytab_file) or external PKI " + "\\(--rpc_certificate_file et al\\) are configured" + ".*Detected inconsistency in command-line flags; exiting"); +} + +TEST_F(SecurityFlagsTest, RpcAuthnFlagsMasterStartup) { + ExternalMiniClusterOptions opts; + + // Explicitly disable Kerberos authentication. + opts.enable_kerberos = false; + + // Trying to start only master: it should detect an inconsistency in its + // command-line flags. + opts.num_masters = 1; + opts.num_tablet_servers = 0; + + // Explicitly require RPC authentication for the master. + opts.extra_master_flags = { "--rpc_authentication=required" }; + + ExternalMiniCluster cluster(std::move(opts)); + const auto s = cluster.Start(); + + // Master should not be running. + ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); + const auto& errmsg = s.ToString(); + // Master should detect an inconsistency in the command-line flags. + ASSERT_STR_CONTAINS(errmsg, "Unable to start Master"); + ASSERT_STR_CONTAINS(errmsg, "process exited with non-zero status 1"); +} + +TEST_F(SecurityFlagsTest, RpcAuthnFlagsTabletServerStartup) { + ExternalMiniClusterOptions opts; + + // Explicitly disable Kerberos authentication. + opts.enable_kerberos = false; + + // The cluster has one master and just one tablet server. To simplify this + // test scenario which relies on the ExternalMiniCluster's functionality, + // allow the single master to start, but the tablet server should not be able + // to start since it should detect an inconsistency in the command-line flags. + opts.num_masters = 1; + opts.num_tablet_servers = 1; + + // Explicitly require RPC authentication for the tablet server. + // The master doesn't have this extra flag, so it should be able + // to start up just fine. + opts.extra_tserver_flags = { "--rpc_authentication=required" }; + + cluster::ExternalMiniCluster cluster(std::move(opts)); + const auto s = cluster.Start(); + + // Master should be able to start up and be running. + ASSERT_TRUE(cluster.master(0)->IsProcessAlive()); + // However, the tablet server shouldn't be running. + ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); + const auto& errmsg = s.ToString(); + ASSERT_STR_CONTAINS(errmsg, "failed to start tablet server"); + ASSERT_STR_CONTAINS(errmsg, "process exited with non-zero status 1"); +} + +} // namespace security +} // namespace kudu diff --git a/src/kudu/rpc/negotiation.cc b/src/kudu/rpc/negotiation.cc index 1796d8170..b7d7ed31b 100644 --- a/src/kudu/rpc/negotiation.cc +++ b/src/kudu/rpc/negotiation.cc @@ -257,13 +257,10 @@ static Status DoServerNegotiation(Connection* conn, bool encrypt_loopback, const MonoTime& deadline) { auto* messenger = conn->reactor_thread()->reactor()->messenger(); - if (authentication == RpcAuthentication::REQUIRED && - messenger->keytab_file().empty() && - !messenger->tls_context().is_external_cert()) { - return Status::InvalidArgument("RPC authentication (--rpc_authentication) may not be " - "required unless Kerberos (--keytab_file) or external PKI " - "(--rpc_certificate_file et al) are configured"); - } + DCHECK(authentication != RpcAuthentication::REQUIRED || + !messenger->keytab_file().empty() || + messenger->tls_context().is_external_cert()) + << "misconfiguration: check ValidateRpcAuthnFlags() for details"; if (FLAGS_rpc_negotiation_inject_delay_ms > 0) { LOG(WARNING) << "Injecting " << FLAGS_rpc_negotiation_inject_delay_ms