From commits-return-7601-archive-asf-public=cust-asf.ponee.io@kudu.apache.org Thu Jun 20 12:46:43 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id A613E18077F for ; Thu, 20 Jun 2019 14:46:42 +0200 (CEST) Received: (qmail 8095 invoked by uid 500); 20 Jun 2019 12:46:42 -0000 Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kudu.apache.org Delivered-To: mailing list commits@kudu.apache.org Received: (qmail 8032 invoked by uid 99); 20 Jun 2019 12:46:42 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 20 Jun 2019 12:46:42 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 7D46987AD8; Thu, 20 Jun 2019 12:46:41 +0000 (UTC) Date: Thu, 20 Jun 2019 12:46:42 +0000 To: "commits@kudu.apache.org" Subject: [kudu] 01/03: KUDU-2870: allow super-user to skip authz checks in Checksum MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit From: granthenke@apache.org In-Reply-To: <156103480142.32714.7501455359138121716@gitbox.apache.org> References: <156103480142.32714.7501455359138121716@gitbox.apache.org> X-Git-Host: gitbox.apache.org X-Git-Repo: kudu X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Rev: 5dca59eba4cb2717a2cf1dfbc5bd750fd2535366 X-Git-NotificationType: diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated Message-Id: <20190620124641.7D46987AD8@gitbox.apache.org> This is an automated email from the ASF dual-hosted git repository. granthenke pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git commit 5dca59eba4cb2717a2cf1dfbc5bd750fd2535366 Author: Andrew Wong AuthorDate: Wed Jun 19 14:05:59 2019 -0700 KUDU-2870: allow super-user to skip authz checks in Checksum In order to allow for the Kudu CLI to run a checksum scan (a process which currently doesn't fetch an authz token from the Master), this patch allows Checksum to proceed if the requesting user is a super-user. Testing: - A test is added to run the CLI against a tserver that enforces fine-grained access control. - A new test is added to test the super-user permissions for the Checksum endpoint. Change-Id: I9da21f41702da747a081ab037d75865748d981a8 Reviewed-on: http://gerrit.cloudera.org:8080/13681 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- src/kudu/integration-tests/security-itest.cc | 76 ++++++++++++++++++++++++---- src/kudu/server/server_base.cc | 7 ++- src/kudu/server/server_base.h | 3 ++ src/kudu/tools/kudu-tool-test.cc | 30 +++++++++++ src/kudu/tserver/tablet_service.cc | 7 ++- 5 files changed, 109 insertions(+), 14 deletions(-) diff --git a/src/kudu/integration-tests/security-itest.cc b/src/kudu/integration-tests/security-itest.cc index 92d7322..1003461 100644 --- a/src/kudu/integration-tests/security-itest.cc +++ b/src/kudu/integration-tests/security-itest.cc @@ -34,10 +34,11 @@ #include "kudu/client/shared_ptr.h" #include "kudu/client/write_op.h" #include "kudu/common/partial_row.h" +#include "kudu/common/schema.h" +#include "kudu/common/wire_protocol.h" #include "kudu/common/wire_protocol.pb.h" #include "kudu/consensus/consensus.pb.h" #include "kudu/consensus/consensus.proxy.h" -#include "kudu/gutil/gscoped_ptr.h" #include "kudu/gutil/strings/substitute.h" #include "kudu/master/master.pb.h" #include "kudu/master/master.proxy.h" @@ -50,7 +51,9 @@ #include "kudu/server/server_base.pb.h" #include "kudu/server/server_base.proxy.h" #include "kudu/tablet/key_value_test_schema.h" +#include "kudu/tablet/tablet.pb.h" #include "kudu/tserver/tserver.pb.h" +#include "kudu/tserver/tserver_service.pb.h" #include "kudu/tserver/tserver_service.proxy.h" #include "kudu/util/env.h" #include "kudu/util/monotime.h" @@ -82,6 +85,10 @@ using strings::Substitute; namespace kudu { +static const char* kTableName = "test-table"; +static const Schema kTestSchema = CreateKeyValueTestSchema(); +static const KuduSchema kTestKuduSchema = client::KuduSchema::FromSchema(kTestSchema); + class SecurityITest : public KuduTest { public: SecurityITest() { @@ -111,6 +118,15 @@ class SecurityITest : public KuduTest { return proxy.SetFlag(req, &resp, &controller); } + Status CreateTestTable(const client::sp::shared_ptr& client) { + unique_ptr table_creator(client->NewTableCreator()); + return table_creator->table_name(kTableName) + .set_range_partition_columns({ "key" }) + .schema(&kTestKuduSchema) + .num_replicas(3) + .Create(); + } + // Create a table, insert a row, scan it back, and delete the table. void SmokeTestCluster(); @@ -129,7 +145,7 @@ class SecurityITest : public KuduTest { return proxy.TSHeartbeat(req, &resp, &rpc); } - Status TryListTablets() { + Status TryListTablets(vector* tablet_ids = nullptr) { auto messenger = NewMessengerOrDie(); const auto& addr = cluster_->tablet_server(0)->bound_rpc_addr(); tserver::TabletServerServiceProxy proxy(messenger, addr, addr.host()); @@ -137,7 +153,28 @@ class SecurityITest : public KuduTest { rpc::RpcController rpc; tserver::ListTabletsRequestPB req; tserver::ListTabletsResponsePB resp; - return proxy.ListTablets(req, &resp, &rpc); + RETURN_NOT_OK(proxy.ListTablets(req, &resp, &rpc)); + if (tablet_ids) { + for (int i = 0; i < resp.status_and_schema_size(); i++) { + tablet_ids->emplace_back(resp.status_and_schema(i).tablet_status().tablet_id()); + } + } + return Status::OK(); + } + + // Sends a request to checksum the given tablet without an authz token. + Status TryChecksumWithoutAuthzToken(const string& tablet_id) { + auto messenger = NewMessengerOrDie(); + const auto& addr = cluster_->tablet_server(0)->bound_rpc_addr(); + tserver::TabletServerServiceProxy proxy(messenger, addr, addr.host()); + + rpc::RpcController rpc; + tserver::ChecksumRequestPB req; + tserver::NewScanRequestPB* scan = req.mutable_new_request(); + scan->set_tablet_id(tablet_id); + RETURN_NOT_OK(SchemaToColumnPBs(kTestSchema, scan->mutable_projected_columns())); + tserver::ChecksumResponsePB resp; + return proxy.Checksum(req, &resp, &rpc); } private: @@ -156,18 +193,11 @@ class SecurityITest : public KuduTest { }; void SecurityITest::SmokeTestCluster() { - const char* kTableName = "test-table"; client::sp::shared_ptr client; ASSERT_OK(cluster_->CreateClient(nullptr, &client)); // Create a table. - KuduSchema schema = client::KuduSchema::FromSchema(CreateKeyValueTestSchema()); - gscoped_ptr table_creator(client->NewTableCreator()); - ASSERT_OK(table_creator->table_name(kTableName) - .set_range_partition_columns({ "key" }) - .schema(&schema) - .num_replicas(3) - .Create()); + ASSERT_OK(CreateTestTable(client)); // Insert a row. client::sp::shared_ptr table; @@ -201,6 +231,30 @@ TEST_F(SecurityITest, TestAuthorizationOnListTablets) { ASSERT_OK(TryListTablets()); } +TEST_F(SecurityITest, TestAuthorizationOnChecksum) { + cluster_opts_.extra_tserver_flags.emplace_back("--tserver_enforce_access_control"); + ASSERT_OK(StartCluster()); + client::sp::shared_ptr client; + ASSERT_OK(cluster_->CreateClient(nullptr, &client)); + ASSERT_OK(CreateTestTable(client)); + vector tablet_ids; + ASSERT_OK(TryListTablets(&tablet_ids)); + + // As a regular user, we shouldn't be authorized if we didn't send an authz + // token. + ASSERT_OK(cluster_->kdc()->Kinit("test-user")); + for (const auto& tablet_id : tablet_ids) { + Status s = TryChecksumWithoutAuthzToken(tablet_id); + ASSERT_STR_CONTAINS(s.ToString(), "Not authorized: no authorization token presented"); + } + // As a super-user (e.g. if running the CLI as an admin), this should be + // allowed. + ASSERT_OK(cluster_->kdc()->Kinit("test-admin")); + for (const auto& tablet_id : tablet_ids) { + ASSERT_OK(TryChecksumWithoutAuthzToken(tablet_id)); + } +} + // Test creating a table, writing some data, reading data, and dropping // the table. TEST_F(SecurityITest, SmokeTestAsAuthorizedUser) { diff --git a/src/kudu/server/server_base.cc b/src/kudu/server/server_base.cc index 6cd3075..636cde2 100644 --- a/src/kudu/server/server_base.cc +++ b/src/kudu/server/server_base.cc @@ -585,9 +585,12 @@ void ServerBase::LogUnauthorizedAccess(rpc::RpcContext* rpc) const { << " from " << rpc->requestor_string(); } +bool ServerBase::IsFromSuperUser(const rpc::RpcContext* rpc) { + return superuser_acl_.UserAllowed(rpc->remote_user().username()); +} + bool ServerBase::Authorize(rpc::RpcContext* rpc, uint32_t allowed_roles) { - if ((allowed_roles & SUPER_USER) && - superuser_acl_.UserAllowed(rpc->remote_user().username())) { + if ((allowed_roles & SUPER_USER) && IsFromSuperUser(rpc)) { return true; } diff --git a/src/kudu/server/server_base.h b/src/kudu/server/server_base.h index 587e446..6859e3d 100644 --- a/src/kudu/server/server_base.h +++ b/src/kudu/server/server_base.h @@ -119,6 +119,9 @@ class ServerBase { SERVICE_USER = 1 << 2 }; + // Returns whether or not the rpc is from a super-user. + bool IsFromSuperUser(const rpc::RpcContext* rpc); + // Authorize an RPC. 'allowed_roles' is a bitset of which roles from the above // enum should be allowed to make hthe RPC. // diff --git a/src/kudu/tools/kudu-tool-test.cc b/src/kudu/tools/kudu-tool-test.cc index 8761522..be454f6 100644 --- a/src/kudu/tools/kudu-tool-test.cc +++ b/src/kudu/tools/kudu-tool-test.cc @@ -4995,5 +4995,35 @@ TEST_P(Is343ReplicaUtilTest, Is343Cluster) { } } +class AuthzTServerChecksumTest : public ToolTest { + public: + void SetUp() override { + ExternalMiniClusterOptions opts; + opts.extra_tserver_flags.emplace_back("--tserver_enforce_access_control=true"); + NO_FATALS(StartExternalMiniCluster(std::move(opts))); + } +}; + +// Test the authorization of Checksum scans via the CLI. +TEST_F(AuthzTServerChecksumTest, TestAuthorizeChecksum) { + // First, let's create a table. + const vector loadgen_args = { + "perf", "loadgen", + cluster_->master()->bound_rpc_addr().ToString(), + "--keep_auto_table", + "--num_rows_per_thread=0", + }; + ASSERT_OK(RunKuduTool(loadgen_args)); + + // Running a checksum scan should succeed since the tool is run as the OS + // user, which is the default super-user. + const vector checksum_args = { + "cluster", "ksck", + cluster_->master()->bound_rpc_addr().ToString(), + "--checksum_scan" + }; + ASSERT_OK(RunKuduTool(checksum_args)); +} + } // namespace tools } // namespace kudu diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc index b1b1792..c752dcd 100644 --- a/src/kudu/tserver/tablet_service.cc +++ b/src/kudu/tserver/tablet_service.cc @@ -1933,7 +1933,12 @@ void TabletServiceImpl::Checksum(const ChecksumRequestPB* req, ScanResultChecksummer collector; bool has_more = false; TabletServerErrorPB::Code error_code = TabletServerErrorPB::UNKNOWN_ERROR; - if (FLAGS_tserver_enforce_access_control && req->has_new_request()) { + // TODO(KUDU-2870): the CLI tool doesn't currently fetch authz tokens when + // checksumming. Until it does, allow the super-user to avoid fine-grained + // privilege checking. + if (FLAGS_tserver_enforce_access_control && + !server_->IsFromSuperUser(context) && + req->has_new_request()) { const NewScanRequestPB& new_req = req->new_request(); TokenPB token; if (!VerifyAuthzTokenOrRespond(server_->token_verifier(), req->new_request(),