Repository: kudu
Updated Branches:
refs/heads/master 7b01d8132 -> 2c052fcb4
rpc: add basic service and method-level authorization
This adds some basic authorization support to the RPC system. The goal
here is to implement service-level and method-level coarse grained
authorization. This can be used for use cases like:
- check that only tablet servers can register or heartbeat to the master
- check that only users in an administrative group can call 'SetFlag'
- check that only other tablet servers can call UpdateConsensus
This facility is quite simple and is not meant to extend to entity-level
authorization checks ("can user X access table Y"). Those checks are
necessarily more complex since they involve inspecting the request, the
target object, etc, and will be implemented in a more ad-hoc manner in
the relevant RPCs.
This patch follows something similar to the "option 3" approach outlined
in a mailing list post[1], except does compile-time binding and checking
that the specified authorization methods are properly defined.
[1]
https://lists.apache.org/thread.html/e31bacbe39a099bc538057ccbe7f96f00a9711dfbbbefaa1c99c97f3@%3Cdev.kudu.apache.org%3E
Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Reviewed-on: http://gerrit.cloudera.org:8080/4897
Reviewed-by: Dan Burkert <[email protected]>
Tested-by: Kudu Jenkins
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/b235487e
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/b235487e
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/b235487e
Branch: refs/heads/master
Commit: b235487e1da52be3840657d6c46852912625c6bb
Parents: 7b01d81
Author: Todd Lipcon <[email protected]>
Authored: Mon Oct 31 19:13:26 2016 -0700
Committer: Todd Lipcon <[email protected]>
Committed: Thu Jan 26 22:14:50 2017 +0000
----------------------------------------------------------------------
docs/design-docs/rpc.md | 50 +++++++++++++++++++++++++++-
src/kudu/rpc/protoc-gen-krpc.cc | 63 ++++++++++++++++++++++++++++++------
src/kudu/rpc/rpc-test-base.h | 20 ++++++++++++
src/kudu/rpc/rpc_header.proto | 16 +++++++--
src/kudu/rpc/rpc_stub-test.cc | 46 ++++++++++++++++++++++++++
src/kudu/rpc/rtest.proto | 6 +++-
src/kudu/rpc/service_if.cc | 10 ++++--
src/kudu/rpc/service_if.h | 18 +++++++++++
8 files changed, 213 insertions(+), 16 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/docs/design-docs/rpc.md
----------------------------------------------------------------------
diff --git a/docs/design-docs/rpc.md b/docs/design-docs/rpc.md
index c565b30..8b4fdeb 100644
--- a/docs/design-docs/rpc.md
+++ b/docs/design-docs/rpc.md
@@ -166,7 +166,7 @@ Example:
```
service CalculatorService {
rpc AddExactlyOnce(ExactlyOnceRequestPB) returns (ExactlyOnceResponsePB) {
- option (kudu.track_rpc_result) = true;
+ option (kudu.rpc.track_rpc_result) = true;
}
}
```
@@ -184,6 +184,54 @@ the lifetime of the server, making sure that responses
survive crashes
and are also available in replicas (for replicated RPCs, like writes) needs
that actions be taken outside of the RPC subsystem.
+## Authorization
+
+The RPC system supports basic hooks for authorization. The authorization
+method can be specified on either a per-service level, or on a per-method
+level (overriding any service-wide default, if set). Similar to above,
+we use custom protobuf 'options' to specify the authorization method:
+
+```
+ service MyService (
+ option (kudu.rpc.default_authz_method) = "MyAuthorization";
+ rpc SomeCall(ReqPB) returns (RespPB) {
+ option (kudu.rpc.authz_method) = "CustomAuthorization";
+ }
+ }
+```
+
+When authorization methods are specified, this enables the generation of pure
+virtual functions in the service interface:
+
+```
+ virtual bool MyAuthorization(const google::protobuf::Message* req,
+ google::protobuf::Message* resp, ::kudu::rpc::RpcContext *context) = 0;
+ virtual bool CustomAuthorization(const google::protobuf::Message* req,
+ google::protobuf::Message* resp, ::kudu::rpc::RpcContext *context) = 0;
+```
+
+You must implement these functions in your service class. The method should
implement
+the desired authorization policy, and then return 'true' if the request is
+authorized. If the request should be rejected, it should call the relevant
+RpcContext method to respond with an error, and then return 'false', indicating
+that no further processing is required.
+
+The authorization methods are run on the same threadpool as the actual request,
+and are functionally equivalent to running the same method as the first
+line of your RPC method implementation. The benefit of declaring the
authorization
+methods in the protobuf file versus inserting explicit function calls is that
+it is easier to apply authorization service-wide, and easier to verify that
+no methods were accidentally overlooked for authorization checks.
+
+For services where some methods have looser authorization checks than others,
+it's recommended that the most restrictive policy be applied on the service
level,
+and then loosened on a per-method basis.
+
+NOTE: if a policy is provided both as a service-level 'default_authz_method'
+option and also as a method-level 'authz_method' option, the method-level
+option _overrides_ the service-level option. They are _not_ combined with
+a boolean 'AND' or 'OR' condition.
+
## RPC Sidecars
RPC sidecars are used to avoid excess copies for large volumes of data.
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/protoc-gen-krpc.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/protoc-gen-krpc.cc b/src/kudu/rpc/protoc-gen-krpc.cc
index 84610e4..de41aa9 100644
--- a/src/kudu/rpc/protoc-gen-krpc.cc
+++ b/src/kudu/rpc/protoc-gen-krpc.cc
@@ -21,6 +21,14 @@
////////////////////////////////////////////////////////////////////////////////
#include <ctype.h>
+
+#include <iostream>
+#include <map>
+#include <memory>
+#include <sstream>
+#include <string>
+
+#include <boost/optional.hpp>
#include <glog/logging.h>
#include <google/protobuf/compiler/code_generator.h>
#include <google/protobuf/compiler/plugin.h>
@@ -29,36 +37,51 @@
#include <google/protobuf/io/printer.h>
#include <google/protobuf/io/zero_copy_stream.h>
#include <google/protobuf/stubs/common.h>
-#include <iostream>
-#include <map>
-#include <memory>
-#include <sstream>
-#include <string>
#include "kudu/gutil/gscoped_ptr.h"
-#include "kudu/gutil/strings/split.h"
#include "kudu/gutil/strings/join.h"
#include "kudu/gutil/strings/numbers.h"
-#include "kudu/gutil/strings/strip.h"
+#include "kudu/gutil/strings/split.h"
#include "kudu/gutil/strings/stringpiece.h"
+#include "kudu/gutil/strings/strip.h"
#include "kudu/gutil/strings/substitute.h"
#include "kudu/gutil/strings/util.h"
#include "kudu/rpc/rpc_header.pb.h"
#include "kudu/util/status.h"
#include "kudu/util/string_case.h"
+using boost::optional;
using google::protobuf::FileDescriptor;
using google::protobuf::io::Printer;
using google::protobuf::MethodDescriptor;
using google::protobuf::ServiceDescriptor;
using std::map;
using std::shared_ptr;
+using std::set;
using std::string;
using std::vector;
namespace kudu {
namespace rpc {
+namespace {
+
+// Return the name of the authorization method specified for this
+// RPC method, or boost::none if none is specified.
+//
+// This handles fallback to the service-wide default.
+optional<string> GetAuthzMethod(const MethodDescriptor& method) {
+ if (method.options().HasExtension(authz_method)) {
+ return method.options().GetExtension(authz_method);
+ }
+ if (method.service()->options().HasExtension(default_authz_method)) {
+ return method.service()->options().GetExtension(default_authz_method);
+ }
+ return boost::none;
+}
+
+} // anonymous namespace
+
class Substituter {
public:
virtual ~Substituter() {}
@@ -185,6 +208,7 @@ class MethodSubstitutions : public Substituter {
(*map)["metric_enum_key"] = strings::Substitute("kMetricIndex$0",
method_->name());
bool track_result =
static_cast<bool>(method_->options().GetExtension(track_rpc_result));
(*map)["track_result"] = track_result ? " true" : "false";
+ (*map)["authz_method"] =
GetAuthzMethod(*method_).get_value_or("AuthorizeAllowAll");
}
// Strips the package from method arguments if they are in the same package
as
@@ -367,6 +391,7 @@ class CodeGenerator : public
::google::protobuf::compiler::CodeGenerator {
"\n"
);
+ set<string> authz_methods;
for (int method_idx = 0; method_idx < service->method_count();
++method_idx) {
const MethodDescriptor *method = service->method(method_idx);
@@ -376,8 +401,22 @@ class CodeGenerator : public
::google::protobuf::compiler::CodeGenerator {
" virtual void $rpc_name$(const $request$ *req,\n"
" $response$ *resp, ::kudu::rpc::RpcContext *context) = 0;\n"
);
-
subs->Pop();
+ if (auto m = GetAuthzMethod(*method)) {
+ authz_methods.insert(m.get());
+ }
+ }
+
+ if (!authz_methods.empty()) {
+ printer->Print(
+ "\n\n"
+ " // Authorization methods\n"
+ " // ---------------------\n\n");
+ }
+ for (const string& m : authz_methods) {
+ printer->Print({ {"m", m} },
+ " virtual bool $m$(const google::protobuf::Message* req,\n"
+ " google::protobuf::Message* resp, ::kudu::rpc::RpcContext
*context) = 0;\n");
}
Print(printer, *subs,
@@ -454,7 +493,7 @@ class CodeGenerator : public
::google::protobuf::compiler::CodeGenerator {
Print(printer, *subs,
"$service_name$If::$service_name$If(const scoped_refptr<MetricEntity>&
entity,"
" const scoped_refptr<ResultTracker>& result_tracker) {\n"
- "result_tracker_ = result_tracker;"
+ "result_tracker_ = result_tracker;\n"
);
for (int method_idx = 0; method_idx < service->method_count();
++method_idx) {
@@ -466,6 +505,12 @@ class CodeGenerator : public
::google::protobuf::compiler::CodeGenerator {
" scoped_refptr<RpcMethodInfo> mi(new RpcMethodInfo());\n"
" mi->req_prototype.reset(new $request$());\n"
" mi->resp_prototype.reset(new $response$());\n"
+ " mi->authz_method = [this](const Message* req, Message*
resp,\n"
+ " RpcContext* ctx) {\n"
+ " return this->$authz_method$(static_cast<const
$request$*>(req),\n"
+ " static_cast<$response$*>(resp),\n"
+ " ctx);\n"
+ " };\n"
" mi->track_result = $track_result$;\n"
" mi->handler_latency_histogram =\n"
"
METRIC_handler_latency_$rpc_full_name_plainchars$.Instantiate(entity);\n"
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/rpc-test-base.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc-test-base.h b/src/kudu/rpc/rpc-test-base.h
index ff194bb..5bdacee 100644
--- a/src/kudu/rpc/rpc-test-base.h
+++ b/src/kudu/rpc/rpc-test-base.h
@@ -288,6 +288,26 @@ class CalculatorService : public CalculatorServiceIf {
context->RespondSuccess();
}
+ bool AuthorizeDisallowAlice(const google::protobuf::Message* /*req*/,
+ google::protobuf::Message* /*resp*/,
+ RpcContext* context) override {
+ if (context->user_credentials().real_user() == "alice") {
+ context->RespondFailure(Status::NotAuthorized("alice is not allowed to
call this method"));
+ return false;
+ }
+ return true;
+ }
+
+ bool AuthorizeDisallowBob(const google::protobuf::Message* /*req*/,
+ google::protobuf::Message* /*resp*/,
+ RpcContext* context) override {
+ if (context->user_credentials().real_user() == "bob") {
+ context->RespondFailure(Status::NotAuthorized("bob is not allowed to
call this method"));
+ return false;
+ }
+ return true;
+ }
+
private:
void DoSleep(const SleepRequestPB *req,
RpcContext *context) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/rpc_header.proto
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc_header.proto b/src/kudu/rpc/rpc_header.proto
index e72b92c..3683a23 100644
--- a/src/kudu/rpc/rpc_header.proto
+++ b/src/kudu/rpc/rpc_header.proto
@@ -270,8 +270,20 @@ message ErrorStatusPB {
extensions 100 to max;
}
-// An option for RPC methods that allows to set whether that method's
-// RPC results should be tracked with a ResultTracker.
extend google.protobuf.MethodOptions {
+ // An option for RPC methods that allows to set whether that method's
+ // RPC results should be tracked with a ResultTracker.
optional bool track_rpc_result = 50006 [default=false];
+
+ // An option to set the authorization method for this particular
+ // RPC method. If this is not specified, the service's 'default_authz_method'
+ // is used.
+ optional string authz_method = 50007;
+}
+
+extend google.protobuf.ServiceOptions {
+ // Set the default authorization method for the RPCs in this service.
+ // If this is not set, then the default authorization is to allow all
+ // RPCs.
+ optional string default_authz_method = 50007;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/rpc_stub-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rpc_stub-test.cc b/src/kudu/rpc/rpc_stub-test.cc
index 09504fd..645dde3 100644
--- a/src/kudu/rpc/rpc_stub-test.cc
+++ b/src/kudu/rpc/rpc_stub-test.cc
@@ -169,6 +169,52 @@ TEST_F(RpcStubTest, TestCustomCredentialsPropagated) {
ASSERT_FALSE(resp.credentials().has_effective_user());
}
+TEST_F(RpcStubTest, TestAuthorization) {
+ // First test calling WhoAmI() as user "alice", who is disallowed.
+ {
+ CalculatorServiceProxy p(client_messenger_, server_addr_);
+ UserCredentials creds;
+ creds.set_real_user("alice");
+ p.set_user_credentials(creds);
+
+ // Alice is disallowed by all RPCs.
+ RpcController controller;
+ WhoAmIRequestPB req;
+ WhoAmIResponsePB resp;
+ Status s = p.WhoAmI(req, &resp, &controller);
+ ASSERT_FALSE(s.ok());
+ ASSERT_EQ(s.ToString(),
+ "Remote error: Not authorized: alice is not allowed to call this
method");
+ }
+
+ // Try some calls as "bob".
+ {
+ CalculatorServiceProxy p(client_messenger_, server_addr_);
+ UserCredentials creds;
+ creds.set_real_user("bob");
+ p.set_user_credentials(creds);
+
+ // "bob" is allowed to call WhoAmI().
+ {
+ RpcController controller;
+ WhoAmIRequestPB req;
+ WhoAmIResponsePB resp;
+ ASSERT_OK(p.WhoAmI(req, &resp, &controller));
+ }
+
+ // "bob" is not allowed to call "Sleep".
+ {
+ RpcController controller;
+ SleepRequestPB req;
+ req.set_sleep_micros(10);
+ SleepResponsePB resp;
+ Status s = p.Sleep(req, &resp, &controller);
+ ASSERT_EQ(s.ToString(),
+ "Remote error: Not authorized: bob is not allowed to call this
method");
+ }
+ }
+}
+
// Test that the user's remote address is accessible to the server.
TEST_F(RpcStubTest, TestRemoteAddress) {
CalculatorServiceProxy p(client_messenger_, server_addr_);
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/rtest.proto
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/rtest.proto b/src/kudu/rpc/rtest.proto
index b138258..df307a6 100644
--- a/src/kudu/rpc/rtest.proto
+++ b/src/kudu/rpc/rtest.proto
@@ -118,8 +118,12 @@ message ExactlyOnceResponsePB {
}
service CalculatorService {
+ option (kudu.rpc.default_authz_method) = "AuthorizeDisallowAlice";
+
rpc Add(AddRequestPB) returns(AddResponsePB);
- rpc Sleep(SleepRequestPB) returns(SleepResponsePB);
+ rpc Sleep(SleepRequestPB) returns(SleepResponsePB) {
+ option (kudu.rpc.authz_method) = "AuthorizeDisallowBob";
+ };
rpc Echo(EchoRequestPB) returns(EchoResponsePB);
rpc WhoAmI(WhoAmIRequestPB) returns (WhoAmIResponsePB);
rpc TestArgumentsInDiffPackage(kudu.rpc_test_diff_package.ReqDiffPackagePB)
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/service_if.cc
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.cc b/src/kudu/rpc/service_if.cc
index aee9a7e..39e9ab5 100644
--- a/src/kudu/rpc/service_if.cc
+++ b/src/kudu/rpc/service_if.cc
@@ -106,6 +106,11 @@ void GeneratedServiceIf::Handle(InboundCall *call) {
req.release(),
resp,
track_result ? result_tracker_ : nullptr);
+ if (!method_info->authz_method(ctx->request_pb(), resp, ctx)) {
+ // The authz_method itself should have responded to the RPC.
+ return;
+ }
+
if (track_result) {
RequestIdPB request_id(call->header().request_id());
ResultTracker::RpcState state = ctx->result_tracker()->TrackRpc(
@@ -114,7 +119,7 @@ void GeneratedServiceIf::Handle(InboundCall *call) {
ctx);
switch (state) {
case ResultTracker::NEW:
- method_info->func(ctx->request_pb(), resp, ctx);
+ // Fall out of the 'if' statement to the normal path.
break;
case ResultTracker::COMPLETED:
case ResultTracker::IN_PROGRESS:
@@ -125,9 +130,8 @@ void GeneratedServiceIf::Handle(InboundCall *call) {
default:
LOG(FATAL) << "Unknown state: " << state;
}
- } else {
- method_info->func(ctx->request_pb(), resp, ctx);
}
+ method_info->func(ctx->request_pb(), resp, ctx);
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/b235487e/src/kudu/rpc/service_if.h
----------------------------------------------------------------------
diff --git a/src/kudu/rpc/service_if.h b/src/kudu/rpc/service_if.h
index 0e38aa0..a3722c6 100644
--- a/src/kudu/rpc/service_if.h
+++ b/src/kudu/rpc/service_if.h
@@ -41,6 +41,7 @@ namespace rpc {
class InboundCall;
class RemoteMethod;
class RpcContext;
+class ServiceIf;
// Generated services define an instance of this class for each
// method that they implement. The generic server code implemented
@@ -58,6 +59,13 @@ struct RpcMethodInfo : public
RefCountedThreadSafe<RpcMethodInfo> {
// Whether we should track this method's result, using ResultTracker.
bool track_result;
+ // The authorization function for this RPC. If this function
+ // returns false, the RPC has already been handled (i.e. rejected)
+ // by the authorization function.
+ std::function<bool(const google::protobuf::Message* req,
+ google::protobuf::Message* resp,
+ RpcContext* ctx)> authz_method;
+
// The actual function to be called.
std::function<void(const google::protobuf::Message* req,
google::protobuf::Message* resp,
@@ -84,6 +92,16 @@ class ServiceIf {
return nullptr;
}
+ // Default authorization method, which just allows all RPCs.
+ //
+ // See docs/design-docs/rpc.md for details on how to add custom
+ // authorization checks to a service.
+ bool AuthorizeAllowAll(const google::protobuf::Message* /*req*/,
+ google::protobuf::Message* /*resp*/,
+ RpcContext* /*ctx*/) {
+ return true;
+ }
+
protected:
bool ParseParam(InboundCall* call, google::protobuf::Message* message);
void RespondBadMethod(InboundCall* call);
