Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12500 )
Change subject: [sentry] add privilege scope validation to SentryAuthzProvider ...................................................................... Patch Set 7: (28 comments) http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30 PS7, Line 30: 'ALL ON default.a' > In addition to "default", we also include "server1". Does this mean that ev Right, for example 'ALL on SERVER1' or 'CREATE on SERVER1' will be returned. It will return 'all privileges granted to the user that match the authorizable of each scope in the input authorizable hierarchy'. And 'server1' is in the input authorizable hierarchy. http://gerrit.cloudera.org:8080/#/c/12500/7//COMMIT_MSG@30 PS7, Line 30: 'ALL ON default.a' > Ah, I think I confused myself thinking about this. Just to be sure, in this That's right. I think that depends on how the cache is used. If we are able to map a cache entry with higher-level scope to a lower-level scope request, then the higher-level scope request can be useful since it has higher cache coverage, and we don't need to cache lower-level scope request any more(which contains partial duplicated information). 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 Done 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. This test is not to cover that but for verifying authorizable scope filter is behave as expect independent of which action is used. Action verification should be a different dimension. The combination of authorizable and action verification should be tested in TestAuthorizeXXX series. 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 tha Not quite following, if COLUMN scope is passed to Authorize(), there is a DCHECK to disallow that (https://gerrit.cloudera.org/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@176), so I don't think how Scope::COLUMN will be automatically skipped. http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12500/6/src/kudu/master/sentry_authz_provider.h@92 PS6, Line 92: const std::string& user, > Nit: there's a subtle difference between 'required_grant_option' and 'requi Makes sense, updated. 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@176 PS7, Line 176: DCHECK(scope != SentryAuthorizableScope::Scope::COLUMN); > Use DCHECK_NE instead. Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@186 PS7, Line 186: authorizable->__set_db(database.ToString()); > What if 'database' is empty at this point? If that should not happen, add Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@192 PS7, Line 192: LOG(FATAL) << "unsupported SentryAuthorizableScope"; > nit: consider outputting the 'scope' parameter here as well -- might be use Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/master/sentry_authz_provider.cc@211 PS7, Line 211: user == owner > Might be not the part of this changelist, but just to clarify. Are usernam Yes, usernames are case sensitive in Sentry APIs, so hence they are case sensitive in all APIs here. 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 ? Adar suggests require_grant_option, so I am going to go with that. 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' > OK, it seems to be just some information on Sentry. And those widening imp I agree it is strange for cases all on column level implies rename or drop the table. However for better or for worse, the default Sentry policy evaluation indeed considers 'server=S->db=D->table=T->column=*' is implying 'server=S->db=D->table=T' (defined in org.apache.sentry.policy.common.CommonPrivilege). And this 'wildcard authorizable matching' is dropped in Kudu's implementation of policy evaluation. 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 Done 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 Done 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", > +1 Done 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 thi Hmm, I agree we should use WARNING instead of ERROR as this is not something should be logged to standard error. http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry-test-base.h@82 PS7, Line 82: virtual bool KerberosEnabled() = 0; > nit: add 'const' ? Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc File src/kudu/sentry/sentry_action.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_action.cc@20 PS7, Line 20: #include <stdint.h> > nit: use <cstdint> instead Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc File src/kudu/sentry/sentry_authorizable_scope-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@61 PS7, Line 61: SentryAuthorizableScope scope; : for (const auto& valid_scope : valid_scopes) { : ASSERT_OK(SentryAuthorizableScope::FromString(valid_scope, &scope)); > Does it make sense to check for the result scope? From the black-box testi Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@66 PS7, Line 66: throws > returns Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope-test.cc@68 PS7, Line 68: "UNINITIALIZED" > maybe, add 'tablecolumn' into the list of invalid scopes to verify SentryAu Done 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@49 PS7, Line 49: SentryAuthorizableScope(); > Could you add a comment justifying the existence of this constructor? Idea The default constructor is useful for FromString(const std::string& str, SentryAuthorizableScope* scope). And the UNINITIALIZED enum is added so a default constructor could exist without having undefined behavior. There is a similar discussion at https://gerrit.cloudera.org/#/c/11720/2/src/kudu/sentry/sentry_action.h@66 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 Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc File src/kudu/sentry/sentry_authorizable_scope.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@20 PS7, Line 20: #include <stdint.h> > nit: use #include <cstdint> instead Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@39 PS7, Line 39: "SERVER" > a tiny nit for this and other string literals that are shared between this Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_authorizable_scope.cc@96 PS7, Line 96: } > nit: I guess gcc would report a warning here because the function returns n Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client-test.cc@56 PS7, Line 56: bool KerberosEnabled() { > nit: add 'const' ? Done http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h File src/kudu/sentry/sentry_client.h: http://gerrit.cloudera.org:8080/#/c/12500/7/src/kudu/sentry/sentry_client.h@99 PS7, Line 99: // If the user is granted both 'ALL on SERVER server1' and 'SELECT on TABLE db1.a' > Does that mean that if there is a request for a particular column C in a ta Right, if user is granted with ALL on Server 's' and the request is for a particular column C in table 'A', it will return whole contents that match in each privilege scope on the hierarchy of 'server=s->db=db->table=a->column=u', and ALL on Server 's' privilege will be returned as well. -- 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: Thu, 14 Mar 2019 06:51:01 +0000 Gerrit-HasComments: Yes
