Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: WIP:[sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@101 PS1, Line 101: #include "kudu/master/default_authz_provider.h" missing authz_provider.h? http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@257 PS1, Line 257: DEFINE_string(trusted_user_acl, "", spitballing: in order to keep as much of the authz behavior in an easily auditable state, it may be good to instead define this flag in authz_provider.cc, then you can add a helper function which checks a username against the ACL without having to do the std::find stuff repeatedly in catalog manager. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1238 PS1, Line 1238: authz_provider_->Stop(); It should always be initialized, right? http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1403 PS1, Line 1403: ector<string> Probably won't be an issue, but just to head off any potential issues, consider making this an unordered_set, or instead sorting the vector and using std::binary_search. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1403 PS1, Line 1403: vecto Since this isn't a runtime flag, consider doing this once and caching it in a static with std::once. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@1813 PS1, Line 1813: RETURN_NOT_OK(FindAndLockTable(req, resp, LockMode::READ, &table, &l)); This looks like a potential TOCTOU issue. We're also locking the table on line 1835 in the HMS path, and inside DeleteTable in the non-HMS path. There's a potential that the table could be switched out in-between, in which case the authorization would have applied to a different table. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/catalog_manager.cc@2227 PS1, Line 2227: RETURN_NOT_OK(FindAndLockTable(req, resp, LockMode::READ, &table, &l)); Same thing here. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/master.proto@85 PS1, Line 85: NOT_AUTHORIZED = 14; add docs http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: PS1: These changes look like they should be in the SentryAuthzProvider patch. http://gerrit.cloudera.org:8080/#/c/11797/1/src/kudu/master/sentry_authz_provider.cc@146 PS1, Line 146: equals iequals? -- 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: 1 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 26 Oct 2018 18:19:28 +0000 Gerrit-HasComments: Yes
