Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider ...................................................................... Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/4//COMMIT_MSG@10 PS4, Line 10: retries, reconnections, HA, error : handling, and concurrent requests this is all handled by the existing thrift::HaClient. 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@67 PS4, Line 67: AuthorizeGetTableMetadata Where is this intended to be used? 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 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 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? 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 earlier we can catch this, the easier it will be to surface a good error message, and consequently debug. 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. Interesting - is there a link or somewhere you can reference in Sentry that explains why this is? http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@140 PS4, Line 140: // For table alteration (with table rename) requires Same thing here, I think there's a SENTRY JIRA that explains this distinction, would be good to link for future reference. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187 PS4, Line 187: // Note: the RPC addresses of the Sentry service(s) can be with/without We already have HostPort::ParseStrings which does almost the exact same thing as this, however it doesn't handle schemes. I'd like to see this use that method instead of implementing largely the same logic again, which means either we need to not handle addresses with schemes, or add scheme support to ParseStrings. I'm leaning toward the former since I don't think schemes are used much for Sentry, but I don't really have data to back that up. I'm curious what others think. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@244 PS4, Line 244: (privilege.grantOption == TSentryGrantOption::UNSET || : privilege.grantOption == TSentryGrantOption::FALSE) might be a bit safer to express this as 'privilege.grantOption != TSentryGrantOption::TRUE'. It's a bit simpler, and in the unlikely case that an additional variant is added it will 'fail closed'. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@250 PS4, Line 250: Status s = SentryAction::FromString(privilege.action, &granted_action); I'd add a WARN_NOT_OK or similar here so that we get an indication in the logs if we aren't handling a new action type. This is especially useful since we are returning a non-specific error. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256 PS4, Line 256: Substitute substitute isn't necessary http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@256 PS4, Line 256: return Status::NotAuthorized(Substitute("unauthorized action")); Is there a security reason to have a generic error message? It may be helpful to try and explain why the action is unauthorized, but maybe that could be a sidechannel leak? I don't have anything specific in mind, but it seems like a possible risk. In any case there should probably be a comment explaining which way we go. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@275 PS4, Line 275: CHECK(false) << "unsupported authorizable scope"; Prefer LOG(FATAL) -- 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: 4 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: Fri, 26 Oct 2018 00:25:14 +0000 Gerrit-HasComments: Yes
