Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans ...................................................................... Patch Set 4: (11 comments) Didn't finish reviewing the test. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@618 PS4, Line 618: set<string> projected_cols; Why the usage of std::set here and in ScanPrivileges? Do you need sorting on the inserted contents? Or just deduplication? If the latter, unordered_set communicates your needs more clearly. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@627 PS4, Line 627: use_pk, JoinStrings(projected, ", "), JoinStrings(predicated, ", ")); Surprised JoinStrings() doesn't work directly on projected_cols or predicated_cols. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@690 PS4, Line 690: EncodedKey* key = builder.BuildEncodedKey(); Is this leaked? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@697 PS4, Line 697: ono typo? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@1034 PS4, Line 1034: for (bool use_pk : { true, false }) { You can parameterize the test on this too. Just need to add a tuple and compute the Cartesian product when instantiating. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@409 PS4, Line 409: for (int col_idx = 0; col_idx < schema.num_columns(); col_idx++) { : EmplaceIfNotPresent(&required_privileges, schema.column_id(col_idx)); : } This seems like something worth baking into Schema: a method to extract a schema's column IDs, ordered by column index. I wonder where else we repeat this pattern. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@427 PS4, Line 427: EmplaceIfNotPresent(&required_privileges, col_idx); This is kind of non-obvious in that it adds a "column index" (sort of) to a set of ColumnIds. Could we communicate the failure in another way? BTW, we'll need to find a solution for virtual columns like IS_DELETED, which won't be in the tablet schema. I'm guessing the answer is "you don't need any privileges to access them"? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@444 PS4, Line 444: for (int i = 0; i < scan_pb.deprecated_range_predicates_size(); i++) { : const int col_idx = schema.find_column(scan_pb.deprecated_range_predicates(i).column().name()); : if (col_idx == Schema::kColumnNotFound) { : EmplaceIfNotPresent(&required_privileges, col_idx); : return required_privileges; : } : EmplaceIfNotPresent(&required_privileges, schema.column_id(col_idx)); : } This method repeats effectively the same body of code three times. Can we consolidate it behind a lambda? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1503 PS4, Line 1503: unordered_set<ColumnId> authorized_column_ids; You can scope this inside FLAGS_tserver_enforce_access_control. Same for the other modified methods. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1533 PS4, Line 1533: unordered_set<ColumnId> required_privileges = ColumnPrivilegesForNewScan(scan_pb, schema); This might be a little more obvious as: unordered_set<ColumnId> ids_to_scan = GetProjectedColumnIds(...); It doesn't have to do with privileges per se; we're just taking the client's projection, converting it into a set of IDs, and then below making sure that our token grants us privilege to read each of them. Same feedback for the other methods. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1820 PS4, Line 1820: if (FLAGS_tserver_enforce_access_control) { Maybe adjust the control flow so it looks a little more like Scan? if (FLAGS_tserver_enforce_access_control && req->has_new_request()) { <do authz stuff> } if (req->has_new_request()) { <do new scan stuff> } -- 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: 4 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[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: Sat, 16 Mar 2019 05:13:04 +0000 Gerrit-HasComments: Yes
