Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/2/src/kudu/master/catalog_manager.cc@1405 PS2, Line 1405: Skip authorization validation if a : // remote call context is not provided, which implies there is no remote : // calls. Does this ever happen? If it's not a case we expect, maybe this should be a D/CHECK(rpc) instead. http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1710 PS3, Line 1710: return SetupError(authz_provider_->AuthorizeGetTableMetadata(username, table_name), : resp, MasterErrorPB::NOT_AUTHORIZED); Hrm, WDYT about baking IsTrustedUser into these AuthzProvider::Authorize* calls? Then we don't have to do that check in the catalog manager code, eg at L1411, L1770, etc. Also we'd be able to avoid these calls entirely in case the DefaultAuthzProvider is being used. http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1771 PS3, Line 1771: authz_func(*user, table_name); I'm a little surprised we're using the authz function to authorize locking the table. Why doesn't it need to be METADATA ON DATABASE or something? http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/catalog_manager.cc@1778 PS3, Line 1778: NOT_AUTHOIRZED nit: NOT_AUTHORIZED http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11797/3/src/kudu/master/master.proto@85 PS3, Line 85: // The caller is not authorized to perform the attempted operation. > In your mind, how is this different from Status::NotAuthorized? If I issue It's worth noting what we do for scanner-hijacking (see SetupErrorAndRespond() tserver/tablet_service.cc). Within the context of the tserver, we pass around a TabletServerErrorPB::NOT_AUTHORIZED that indicates that the user was not authorized to make the request and that the RPC response should reflect that. It does so by returning FATAL_UNAUTHORIZED as the RPC error, and it attaches a Status that has more context. Client-side, an RPC error results in a RemoteError, and the attached Status is just a part of the Status' body. For scanner-hijacking, this Status is NotAuthorized, though it doesn't necessarily need to be -- it's just there to add information. The TabletServerErrorPB exists solely to decouple tserver error (NOT_AUTHORIZED) and RPC error (FATAL_UNAUTHORIZED). RemoteError is also what the client would see for coarse-grained authorization errors (see ServerBase::Authorized() and how it's setting the RPC as a failure). IIUC, this is taking a different approach: it returns that the RPC was successful, transforming a Status::NotAuthorized error into an AppStatusPB on the master (see SetupError() in master/catalog_manager.cc), and the client unwraps and gets the original Status::NotAuthorized error. This is happening by virtue of Status::NotAuthorized being returned from the catalog manager, not the NOT_AUTHORIZED codes. If we wanted to make this symmetric with the tserver, we could probably do something similar: have the catalog manager set NOT_AUTHORIZED, and have the master service check for NOT_AUTHORIZED codes and return FATAL_UNAUTHORIZED. I can kind of see the merit of having it be an application error though; the request itself makes it well past the RPC layer, which makes it a little strange that we're responding with remote errors. WDYT? -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 12 Dec 2018 20:54:32 +0000 Gerrit-HasComments: Yes
