Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@394 PS7, Line 394: TSentryPrivilege column_priv = GetColumnPrivilege(db, tbl, col, action); : TSentryPrivilege table_priv = GetTablePrivilege(db, tbl, action); : TSentryPrivilege db_priv = GetDatabasePrivilege(db, action); : TSentryPrivilege server_priv = GetServerPrivilege(action); nit: add 'const' if the test isn't intended to update these http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@442 PS7, Line 442: Status s = sentry_authz_provider_->Authorize(scope, SentryAction::Action::ALL, : Substitute("$0.$1", db, tbl), kTestUser); With this verification, is it guaranteed that for other type of actions (e.g., Action::METADATA) are not authorized for higher scopes? If looking at this as a black box, I'd think of adding some coverage for that if it's not guaranteed; otherwise, please a comment explaining why other types of actions are guaranteed to be not authorized in this scenario. Or this test is not supposed to cover that at all? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider-test.cc@451 PS7, Line 451: // Scope::COLUMN is excluded since column scope for table : // authorizable doesn't make sense. I thought adding Scope::COLUMN would automatically skip that check, but that would add coverage for the higher scopes, no? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@265 PS7, Line 265: required_grant_option nit: requires_grant_option or grant_option_required ? http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@279 PS7, Line 279: authorizable 'server=server1->db=*' can imply authorizable 'server=server1' > Wait, that sounds odd. Logically, that should not be the case when it's po OK, it seems to be just some information on Sentry. And those widening implications are not supported, as described. However, as for another example, I would not expect that ALL on all columns like 'server=S->db=D->table=T->column=*' is implying 'server=S->db=D->table=T'. For example, that would be strange that ability to do whatever is possible at column level implies the ability to rename or drop the table itself. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@313 PS7, Line 313: WARN_NOT_OK(s, Substitute("failed to construct sentry action from $0", : privilege.action)); nit: the 'if (!s.ok()) {}' scope is present below, so maybe move this under the 'if' scope below and translate into simply LOG(WARN) << ... http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@321 PS7, Line 321: WARN_NOT_OK(s, Substitute("failed to construct sentry privilege scope from $0", > Nit: for clarity via symmetry with L315, could you break out the s.ok() che +1 Also, please move the 'WARN_NOT_OK' stuff under the 'if (!s.ok())', converting it into simple 'LOG(WARN) << ...' http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@335 PS7, Line 335: ERROR Unrelated to this particular changelist, but since you are here: do you think it's justifiable to log is as ERROR? I would expect this to be INFO or at most WARN since no error has happened here. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.h@58 PS7, Line 58: static Status FromString(const std::string& str, SentryAuthorizableScope* scope) nit: add WARN_UNUSED_RESULT -- To view, visit http://gerrit.cloudera.org:8080/12500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89437a04a4fa18e501d21c3abf5d66a2d22ce58a Gerrit-Change-Number: 12500 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 12 Mar 2019 22:02:43 +0000 Gerrit-HasComments: Yes
