Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: WIP authz: verify tokens on scans ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/common/schema.h File src/kudu/common/schema.h: http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/common/schema.h@575 PS1, Line 575: std::vector<int> get_key_column_ids() const { Use std::vector<ColumnId> so it's clear from the signature. http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/common/schema.h@577 PS1, Line 577: std::iota(keys.begin(), keys.end(), 0); Don't column IDs start at 10 or something random in debug mode? In general I don't think this is a safe assumption. http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/security/privilege_util.h File src/kudu/security/privilege_util.h: http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/security/privilege_util.h@27 PS1, Line 27: bool HasAllScanPrivileges(const TablePrivilegePB& privilege, This API is pretty unintuitive, and I don't think it really pulls its weight. Are there multiple callers of this? If not I'd strongly suggest inlining it into wherever it's used. http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/security/privilege_util.h@28 PS1, Line 28: int ColumnId http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/security/privilege_util.cc File src/kudu/security/privilege_util.cc: http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/security/privilege_util.cc@48 PS1, Line 48: whitespace http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/tserver/tablet_service.cc@434 PS1, Line 434: is it a bug that we didn't verify the table id? yes http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/tserver/tablet_service.cc@435 PS1, Line 435: context->RespondRpcFailure(rpc::ErrorStatusPB::FATAL_UNAUTHORIZED, What if all the columns are authorized? http://gerrit.cloudera.org:8080/#/c/11753/1/src/kudu/tserver/tablet_service.cc@1589 PS1, Line 1589: !authorized_table_id.empty() This is error prone, since it will cause bugs if VerifyAuthzTokenForScanOrRespond fails to set the authorized_table_id string, or sets it to an empty string accidentally. I'd consider looking up the tablet first, then having one big if block with the FLAGS_enforce_access_control check. -- To view, visit http://gerrit.cloudera.org:8080/11753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7a5d81cf215a5d936f8853feba05778038764905 Gerrit-Change-Number: 11753 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[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-Comment-Date: Tue, 23 Oct 2018 00:31:22 +0000 Gerrit-HasComments: Yes
