kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From granthe...@apache.org
Subject [kudu] 01/03: KUDU-2870: allow super-user to skip authz checks in Checksum
Date Thu, 20 Jun 2019 12:46:42 GMT
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 <awong@apache.org>
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 <aserbin@cloudera.com>
    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<KuduClient>& client) {
+    unique_ptr<KuduTableCreator> 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<string>* 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<KuduClient> client;
   ASSERT_OK(cluster_->CreateClient(nullptr, &client));
 
   // Create a table.
-  KuduSchema schema = client::KuduSchema::FromSchema(CreateKeyValueTestSchema());
-  gscoped_ptr<KuduTableCreator> 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<KuduTable> 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<KuduClient> client;
+  ASSERT_OK(cluster_->CreateClient(nullptr, &client));
+  ASSERT_OK(CreateTestTable(client));
+  vector<string> 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<string> 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<string> 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(),


Mime
View raw message