Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13681 )

Change subject: KUDU-2870: use coarse-grained authz for Checksum
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13681/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/13681/2/src/kudu/tserver/tablet_service.cc@887
PS2, Line 887: bool TabletServiceImpl::AuthorizeChecksum(const 
google::protobuf::Message* req,
             :                                           
google::protobuf::Message* resp,
             :                                           rpc::RpcContext* 
context) {
             :   if (FLAGS_tserver_enforce_access_control) {
             :     return server_->Authorize(context, ServerBase::SUPER_USER);
             :   }
             :   return AuthorizeClient(req, resp, context);
             : }
> Right, those are similar semantics to what we do for AuthorizeListTablets.
As for the test, you can just disable corresponding scenarios for a while, 
adding the 'DISABLED_' prefix to the name of the correspond scenario (and add 
TODO(KUDU-2870) there as well).

I'm not suggesting to push SUPER_USER checks into VerifyAuthzTokenOrRespond().  
I was suggesting to check for SUPER_USER in Checksum() implementation only if 
fine-grained authz is enabled:


  if (FLAGS_tserver_enforce_access_control && (!IsSuperUser() || 
FLAGS_checksum_require_authz_token) && req->has_new_request()) {
    ...
  }

That way the existing non-authz behavior will be preserved, while 
non-super-user with valid authz token could call this method when fine-grained 
authz is enabled.


http://gerrit.cloudera.org:8080/#/c/13681/2/src/kudu/tserver/tserver_service.proto
File src/kudu/tserver/tserver_service.proto:

http://gerrit.cloudera.org:8080/#/c/13681/2/src/kudu/tserver/tserver_service.proto@54
PS2, Line 54: it retrieves and renews
            :   // authorization tokens
> Ah you're right. I meant this more as commentary on the CLI implementation,
I see.  Then, maybe, remove the new part of the comment to avoid confusion?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9da21f41702da747a081ab037d75865748d981a8
Gerrit-Change-Number: 13681
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Andrew Wong <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Hao Hao <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 20 Jun 2019 00:12:57 +0000
Gerrit-HasComments: Yes

Reply via email to