Repository: kudu Updated Branches: refs/heads/branch-1.6.x 6dfc10ac0 -> c50b7fe69
KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL See the associated JIRA for bug details. The fix follows a slightly different approach than suggested by Todd, namely initialization of the RPC tracker in the RPC context is delayed until after authorization. This allows the error path code to remain unchanged, and allows the one caller of the RpcContext constructor to be slightly cleaned up. Change-Id: I9717d36e438bbe68172fa2619c9829ad5fdc779d Reviewed-on: http://gerrit.cloudera.org:8080/11216 Reviewed-by: Alexey Serbin <aser...@cloudera.com> Tested-by: Kudu Jenkins Reviewed-on: http://gerrit.cloudera.org:8080/11220 Reviewed-by: Adar Dembo <a...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/c50b7fe6 Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/c50b7fe6 Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/c50b7fe6 Branch: refs/heads/branch-1.6.x Commit: c50b7fe6915febcb2af480e67a1ca43a708142c0 Parents: 6dfc10a Author: Dan Burkert <danburk...@apache.org> Authored: Tue Aug 14 12:18:32 2018 -0700 Committer: Dan Burkert <danburk...@apache.org> Committed: Tue Aug 14 22:55:37 2018 +0000 ---------------------------------------------------------------------- src/kudu/rpc/rpc_context.cc | 11 +++++++---- src/kudu/rpc/rpc_context.h | 10 ++++++++-- src/kudu/rpc/rpc_stub-test.cc | 36 +++++++++++++++++++++++++++++------- src/kudu/rpc/service_if.cc | 12 +++--------- 4 files changed, 47 insertions(+), 22 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/rpc_context.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/rpc_context.cc b/src/kudu/rpc/rpc_context.cc index 9c3d154..27011b5 100644 --- a/src/kudu/rpc/rpc_context.cc +++ b/src/kudu/rpc/rpc_context.cc @@ -47,12 +47,10 @@ namespace rpc { RpcContext::RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr<ResultTracker>& result_tracker) + google::protobuf::Message *response_pb) : call_(CHECK_NOTNULL(call)), request_pb_(request_pb), - response_pb_(response_pb), - result_tracker_(result_tracker) { + response_pb_(response_pb) { VLOG(4) << call_->remote_method().service_name() << ": Received RPC request for " << call_->ToString() << ":" << std::endl << SecureDebugString(*request_pb_); TRACE_EVENT_ASYNC_BEGIN2("rpc_call", "RPC", this, @@ -63,6 +61,11 @@ RpcContext::RpcContext(InboundCall *call, RpcContext::~RpcContext() { } +void RpcContext::SetResultTracker(scoped_refptr<ResultTracker> result_tracker) { + DCHECK(!result_tracker_); + result_tracker_ = std::move(result_tracker); +} + void RpcContext::RespondSuccess() { if (AreResultsTracked()) { result_tracker_->RecordCompletionAndRespond(call_->header().request_id(), http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/rpc_context.h ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/rpc_context.h b/src/kudu/rpc/rpc_context.h index 42f096e..468e6d1 100644 --- a/src/kudu/rpc/rpc_context.h +++ b/src/kudu/rpc/rpc_context.h @@ -68,11 +68,17 @@ class RpcContext { // and is not a public API. RpcContext(InboundCall *call, const google::protobuf::Message *request_pb, - google::protobuf::Message *response_pb, - const scoped_refptr<ResultTracker>& result_tracker); + google::protobuf::Message *response_pb); ~RpcContext(); + // Initialize a result tracker for the RPC. + // + // This is delayed until after the constructor in order to allow for RPCs to + // be validated and used prior to initializing the tracking (primarily for + // authorization). + void SetResultTracker(scoped_refptr<ResultTracker> result_tracker); + // Return the trace buffer for this call. Trace* trace(); http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/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 5fe2885..cb16c02 100644 --- a/src/kudu/rpc/rpc_stub-test.cc +++ b/src/kudu/rpc/rpc_stub-test.cc @@ -209,13 +209,35 @@ TEST_F(RpcStubTest, TestAuthorization) { 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"); + { + 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"); + } + + // KUDU-2540: Authorization failures on exactly-once RPCs cause FATAL + { + RpcController controller; + + unique_ptr<RequestIdPB> request_id(new RequestIdPB); + request_id->set_client_id("client-id"); + request_id->set_attempt_no(0); + request_id->set_seq_no(0); + request_id->set_first_incomplete_seq_no(-1); + controller.SetRequestIdPB(std::move(request_id)); + + ExactlyOnceRequestPB req; + req.set_value_to_add(1); + ExactlyOnceResponsePB resp; + Status s = p.AddExactlyOnce(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". http://git-wip-us.apache.org/repos/asf/kudu/blob/c50b7fe6/src/kudu/rpc/service_if.cc ---------------------------------------------------------------------- diff --git a/src/kudu/rpc/service_if.cc b/src/kudu/rpc/service_if.cc index 008c478..af53452 100644 --- a/src/kudu/rpc/service_if.cc +++ b/src/kudu/rpc/service_if.cc @@ -110,20 +110,14 @@ void GeneratedServiceIf::Handle(InboundCall *call) { } Message* resp = method_info->resp_prototype->New(); - bool track_result = call->header().has_request_id() - && method_info->track_result - && FLAGS_enable_exactly_once; - RpcContext* ctx = new RpcContext(call, - req.release(), - resp, - track_result ? result_tracker_ : nullptr); + RpcContext* ctx = new RpcContext(call, req.release(), resp); 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()); + if (call->header().has_request_id() && method_info->track_result && FLAGS_enable_exactly_once) { + ctx->SetResultTracker(result_tracker_); ResultTracker::RpcState state = ctx->result_tracker()->TrackRpc( call->header().request_id(), resp,
