Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 11: (14 comments) Sorry I'm late to the party; hope this is still useful. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/gutil/map-util.h File src/kudu/gutil/map-util.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/gutil/map-util.h@833 PS11, Line 833: void EmplaceValuesFromMap(MapContainer&& map_container, Didn't know that you needed to pass a container as an rvalue reference in order to iterate it with universal references. TIL. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: PS11: With all the new tests, could you double check the NUM_SHARDS value? Does it still make sense? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc@439 PS11, Line 439: // Parameterized by whether Kerberos should be enabled. sentry_authz_provider-test extensively parameterizes with Kerberization, but why is that an interesting axis across which to test? Is there any overlap between Kerberization (or not) and how we do Sentry-based authz? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@51 PS11, Line 51: AuthorizablePrivileges(sentry::SentryAuthorizableScope::Scope scope, My eye is immediately drawn to the lack of a 'server' field; a comment explaining why that is would be helpful here. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@83 PS11, Line 83: // The scope of the authorizable being granted privileges. : const sentry::SentryAuthorizableScope::Scope scope; Can't we figure out the scope by checking which of our strings is empty? That is, if we have db_name="default", table_name="foo", column_name="", we're talking about a table scope, right? As best I can tell, your goal here was defensive programming via sanity checking: receive _more_ information than is absolutely necessary at construction time, and use it for cross-validation purposes. Is that right? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; Might want to name this granted_actions; 'privileges' makes me think of the combination of action and authorizable, but this is only a set of actions. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102 PS11, Line 102: the : // given table. This was a bit confusing as there's no table argument. My takeaway (without having looked at the implementation yet) is that everything in 'privileges' matches a single table. That is, 'privileges' includes entries like default.foo.col1, default.foo.col2, default.foo, or default, but not entries like non_default, non_default.bar, non_default.bar.col1, default.another_table, or default.another_table.colx. Is this enforced anywhere? Perhaps it should be enforced in SentryPrivilegesBranch's constructor? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@371 PS11, Line 371: DCHECK_EQ(FLAGS_server_name, requested_authorizable.server); IIUC, invariant violations on requested_authorizable trigger DCHECKs, while violations on privilege just return false. That makes sense: if we sent the wrong thing that's on us (and a programmer error), but if we got the wrong response we need to handle that gracefully. Might be worth a comment though, since there's a lot of sanity checking going on here. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@424 PS11, Line 424: if (!privilege.__isset.columnName || privilege.columnName.empty()) { Why do we need to validate __isset as well as the strings themselves? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431 PS11, Line 431: !boost::iequals(privilege.tableName, requested_authorizable.table))) { Sentry authorizable names are case-insensitive? Is there a definitive reference for that that we can link here? I guess the same is true for SentryAction and SentryAuthorizableScope, both of which do case-insensitive comparisons. BTW, SentryAuthorizableScope defines something called kServer; should be kServer. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@472 PS11, Line 472: DCHECK(!db.empty() && !table.empty()); Conjunct DCHECKs are hard to debug when they trigger; it's never clear which half of the condition fired. Could you break them out? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@501 PS11, Line 501: unordered_map<string, AuthorizablePrivileges> privileges_map; I think the logic from here on down might be better suited as an "initializing" member (or constructor) of SentryPrivilegesBranch. That's because the SentryPrivilegesBranch documentation talks about certain invariants it provides (such as all privileges having to do with a specific table), but doesn't enforce that in any way. If access to the privileges container were gated via a constructor that performed the adaptation, SentryPrivilegesBranch and its invariants would be self-contained. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@524 PS11, Line 524: privilege.all_with_grant = true; What should we do with privileges whose grant option was enabled but whose action was something unexpected? Should we log them? Treat them as malformed and discard them? http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h@64 PS11, Line 64: static const size_t kMaxAction = Action::OWNER + 1; You might find the templates and macros in "Enumeration Casts and Tests" (gutil/casts.h) useful for this sort of thing (and for SentryAuthorizableScope. -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong <[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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 05 Apr 2019 03:53:26 +0000 Gerrit-HasComments: Yes
