You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/01 02:23:09 UTC

[kudu-CR] rpc: add basic service and method-level authorization 

Hello Dan Burkert, Alexey Serbin,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/4897

to review the following change.

Change subject: rpc: add basic service and method-level authorization
......................................................................

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
---
M docs/design-docs/rpc.md
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 197 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4897/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

Line 117:         // Fall out of the 'if' statement to the normal path.
> So what's the decision here?  FWIW I still think we should check authz up f
k, I'll move the authz up... hopefully the case is rare enough that someone's retry fails due to the auth changing after an original success.


-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4897

to look at the new patch set (#4).

Change subject: rpc: add basic service and method-level authorization
......................................................................

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
---
M docs/design-docs/rpc.md
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 213 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4897/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

Line 117:         // Fall out of the 'if' statement to the normal path.
> hrm, I can see an argument either way. I think my thinking here is that, if
So what's the decision here?  FWIW I still think we should check authz up front, anything else risks security vulnerabilities.  As just one example, consider whether not delaying authz checks introduces timing attacks.  If no, is that property resistant to future refactors?


-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: rpc: add basic service and method-level authorization
......................................................................


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 <da...@apache.org>
Tested-by: Kudu Jenkins
---
M docs/design-docs/rpc.md
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 213 insertions(+), 16 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:

PS1, Line 23: #include <boost/optional.hpp>
            : #include <ctype.h>
            : #include <glog/logging.h>
            : #include <google/protobuf/compiler/code_generator.h>
            : #include <google/protobuf/compiler/plugin.h>
            : #include <google/protobuf/descriptor.h>
            : #include <google/protobuf/descriptor.pb.h>
            : #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>
While you are at it, could you please re-organize those in accordance with the styling guide?

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

PS1, Line 262:   // An option to set the authorization method for this particular
             :   // RPC method.
Does it make sense to add a comment that if the authz method is not specified, then access is allowed at this level by default?


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

PS1, Line 131:   }
It's just a tiny stylistic nit, but do you think it would be better to promote a proper pattern for handling authz method results?  I think the following would be more idiomatic:

if (!method_info->authz_method(ctx->request_pb(), resp, ctx)) {
  return;
}

method_info->func(ctx->request_pb(), resp, ctx);


-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 4:

needed a rebase (conflicts just in rpc.md, everything else clean)

-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

Line 117:         // Fall out of the 'if' statement to the normal path.
Why are we checking the tracker result before checking authz?  Seems like we should always check authz before doing anything, otherwise an attacker could potentially forge a request id.  I don't thing we should rely on UUIDs being unguessable if we can avoid it.


-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4897

to look at the new patch set (#2).

Change subject: rpc: add basic service and method-level authorization
......................................................................

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
---
M docs/design-docs/rpc.md
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 213 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4897/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 1:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/protoc-gen-krpc.cc
File src/kudu/rpc/protoc-gen-krpc.cc:

PS1, Line 23: #include <boost/optional.hpp>
            : #include <ctype.h>
            : #include <glog/logging.h>
            : #include <google/protobuf/compiler/code_generator.h>
            : #include <google/protobuf/compiler/plugin.h>
            : #include <google/protobuf/descriptor.h>
            : #include <google/protobuf/descriptor.pb.h>
            : #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>
> While you are at it, could you please re-organize those in accordance with 
Done


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/rpc-test-base.h
File src/kudu/rpc/rpc-test-base.h:

Line 283:   bool AuthorizeDisallowAlice(const google::protobuf::Message* req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


Line 284:                               google::protobuf::Message* resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done


Line 293:   bool AuthorizeDisallowBob(const google::protobuf::Message* req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


Line 294:                             google::protobuf::Message* resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

PS1, Line 262:   // An option to set the authorization method for this particular
             :   // RPC method.
> Does it make sense to add a comment that if the authz method is not specifi
Done


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.cc
File src/kudu/rpc/service_if.cc:

Line 117:         // Fall out of the 'if' statement to the normal path.
> Why are we checking the tracker result before checking authz?  Seems like w
hrm, I can see an argument either way. I think my thinking here is that, if for some reason the authorization changes (eg someone got revoked privileges), it would be incorrect to now return a "NotAuthorized" error when in fact the original call did succeed. I think we have to assume that request IDs are non-forgeable random UUIDs. I will make a note of that in the task list doc.


PS1, Line 131:   }
> It's just a tiny stylistic nit, but do you think it would be better to prom
Done


http://gerrit.cloudera.org:8080/#/c/4897/1/src/kudu/rpc/service_if.h
File src/kudu/rpc/service_if.h:

Line 99:   bool AuthorizeAllowAll(const google::protobuf::Message* req,
> warning: parameter 'req' is unused [misc-unused-parameters]
Done


Line 100:                          google::protobuf::Message* resp,
> warning: parameter 'resp' is unused [misc-unused-parameters]
Done


Line 101:                          RpcContext* ctx) {
> warning: parameter 'ctx' is unused [misc-unused-parameters]
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: rpc: add basic service and method-level authorization
......................................................................


Patch Set 4: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] rpc: add basic service and method-level authorization

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4897

to look at the new patch set (#3).

Change subject: rpc: add basic service and method-level authorization
......................................................................

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
---
M docs/design-docs/rpc.md
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 214 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/4897/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia9206f5f89391d8bccfa30952d2b252900ab6566
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>