Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 5: (32 comments) http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@7 PS4, Line 7: [sentry] add AuthzProvider > Could you add a paragraph explaining the module placement choices? Done http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@10 PS4, Line 10: vider. It has a default implementation : which always allow any operations > this is all handled by the existing thrift::HaClient. Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@59 PS4, Line 59: AuthorizeAlterTable > Should we wrap new_table in a boost::optional, to convey that it isn't nece I don't think this is necessary. Since the caller, catalog_manager, cannot distinguish rename alters from non-rename ones without the name comparison. It makes more sense to put such comparison inside AuthorizeAlterTable where it results in different authorization logic. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/authz_provider.h@67 PS4, Line 67: AuthorizeGetTableMetadata > Where is this intended to be used? This is used for master RPCs where any privileges is good enough to get a table's metadata, e.g ListTables, GetTabletLocations. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@32 PS4, Line 32: virtual Status AuthorizeCreateTable(const std::string& table_name, > warning: parameter 'table_name' is unused [misc-unused-parameters] Ignored, since those parameters are required in other implementation. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@33 PS4, Line 33: const std::string& user, > warning: parameter 'user' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@34 PS4, Line 34: const std::string& owner) override WARN_UNUSED_RESULT { > warning: parameter 'owner' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@38 PS4, Line 38: virtual Status AuthorizeDropTable(const std::string& table_name, > warning: parameter 'table_name' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@39 PS4, Line 39: const std::string& user) override WARN_UNUSED_RESULT { > warning: parameter 'user' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@43 PS4, Line 43: virtual Status AuthorizeAlterTable(const std::string& old_table, > warning: parameter 'old_table' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@44 PS4, Line 44: const std::string& new_table, > warning: parameter 'new_table' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@45 PS4, Line 45: const std::string& user) override WARN_UNUSED_RESULT { > warning: parameter 'user' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@49 PS4, Line 49: virtual Status AuthorizeGetTableMetadata(const std::string& table_name, > warning: parameter 'table_name' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@50 PS4, Line 50: const std::string& user) override WARN_UNUSED_RESULT { > warning: parameter 'user' is unused [misc-unused-parameters] Same as above. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider-test.cc@67 PS4, Line 67: " > whitespace here and below Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@57 PS4, Line 57: override > 'override' here and below Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@56 PS4, Line 56: : virtual Status Start() override; : : virtual void Stop() override; > Nit: lower-case override for new code. Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@88 PS4, Line 88: virtual Status AuthorizeGetTableMetadata(const std::string& table_name, > Can you remind me what master/catalog manager ops need to call this? e.g. ListTables, GetTableLocations. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111 PS4, Line 111: or 'kudu' : // (Kudu server name) is a non admin user in Sentry > Is there any way we can recognize this failure scenario as part of Start()? I think the best way is to add config validation as what we did in HMS integration. However, since Kudu doesn't handle user group mapping, I don't see a way to validate it. Another way I can think of is adding an API in Sentry to check if a user is admin. Any other suggestions? http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@110 PS4, Line 110: // then the creating user must have 'ALL on DATABASE' with grant. See > Interesting - is there a link or somewhere you can reference in Sentry that Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@140 PS4, Line 140: // For table alteration (without table rename) requires 'ALTER ON TABLE' > Same thing here, I think there's a SENTRY JIRA that explains this distincti Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187 PS4, Line 187: const string kSchemeSeparator = "://"; > We already have HostPort::ParseStrings which does almost the exact same thi Actually, I am leaning toward the second, since if any configuration is using scheme, this won't work. Though I am thinking to introduce a new method in HostPort to add scheme support instead of adding it to ParseStrings, so that any current callers of ParseStrings won't be affected by this change. Any thoughts? http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@244 PS4, Line 244: lege : response.privileges) { : // A grant option cannot imply the other if the former is set > might be a bit safer to express this as 'privilege.grantOption != TSentryGr Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@250 PS4, Line 250: > I'd add a WARN_NOT_OK or similar here so that we get an indication in the l Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256 PS4, Line 256: > substitute isn't necessary Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256 PS4, Line 256: } > Is there a security reason to have a generic error message? It may be help Discussed this offline with Dan. We decided to just use logging to facilitate debugging and only return generic information to users to avoid sidechannel leak such as 'whether table A exists'. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@262 PS4, Line 262: LOG(ERROR) << "Failed to perform action [" << action.action() << "] on authorizable [" : << authorizable << "] for user [" << user << "]"; : return Status::NotAuthorized(Substitute("unauthorized action")); > I commented on this before, but this can be scoped to TABLE or DATABASE; it Oops, sorry that I skipped that. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@267 PS4, Line 267: Status SentryAuthzProvider::GetAuthorizable(const string& table_name, : AuthorizableScope scope, : TSentryAuthorizable* authorizable) { : Slice database; > Missing break statements here? Or if this is intentional, there's a FALLTHR Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@275 PS4, Line 275: authorizable->__set_table(table.ToString()); > Prefer LOG(FATAL) Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@33 PS4, Line 33: class SentryTestBase : public KuduTest, > Seems like you could push the Kerberos boolean parameterization down into t Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry-test-base.h@67 PS4, Line 67: } > Nit: indent by one character. Done http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/sentry/sentry_client-test.cc@60 PS4, Line 60: void TearDown() override { > Note that when overriding SetUp()/TearDown() you should make sure to chain Done -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 5 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: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 30 Oct 2018 07:01:11 +0000 Gerrit-HasComments: Yes
