Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11753 )
Change subject: authz: verify tokens on scans ...................................................................... Patch Set 5: (13 comments) 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: PS4: > Consider moving this into tablet_server_authorization-test Done http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@78 PS4, Line 78: > warning: #includes are not sorted properly [llvm-include-order] Ack http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@618 PS4, Line 618: > Why the usage of std::set here and in ScanPrivileges? Do you need sorting o It was just so ToString()s would be sorted, but I'll just do the conversions there. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@627 PS4, Line 627: > Surprised JoinStrings() doesn't work directly on projected_cols or predicat Done http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@690 PS4, Line 690: > Is this leaked? Yes http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@697 PS4, Line 697: > typo? Done http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/integration-tests/authz_token-itest.cc@1034 PS4, Line 1034: > You can parameterize the test on this too. Just need to add a tuple and com Done 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: vector<ColumnId> cols = schema.column_ids(); : *required_column_privileges = unordered_set<ColumnId>(cols.begin(), cols.end()); : r > This seems like something worth baking into Schema: a method to extract a s Yeah, and it's laughably easy too! http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@427 PS4, Line 427: return false; > This is kind of non-obvious in that it adds a "column index" (sort of) to a Done Interesting question about IS_DELETED. No additional privileges might be right. Is IS_DELETED tied to the primary key at all? Is the PK required as a part of the projection? What happens if you pass an empty projection but ask for an IS_DELETED column? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@444 PS4, Line 444: if (col_idx == Schema::kColumnNotFound) { : return false; : } : EmplaceIfNotPresent(&required_privileges, schema.column_id(col_idx)); : } : *required_column_privileges = std::move(required_privileges); : return true; : } > This method repeats effectively the same body of code three times. Can we c I think it's difficult since I'd need to templatize based on the list we're iterating over, given they don't all use the same interface. If it makes you feel better, I removed the kColumnNotFound stuff so it's fewer lines at least. http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1503 PS4, Line 1503: if (!VerifyAuthzTokenOrRespond(server_->token_verifier(), > You can scope this inside FLAGS_tserver_enforce_access_control. Done http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1533 PS4, Line 1533: if (!GetRequiredColumnPrivilegesForNewScan(scan_pb, schema, &required_column_privileges)) { > This might be a little more obvious as: Not sure I follow. It's not just the projected columns -- it's the projected columns, predicated columns, sometimes primary key columns. It really depends on the scan request. I think decoupling it from column privileges makes it more confusing because it's *not* just the columns that we're scanning, or projecting on; it's the columns that are required for us to be authorized to scan. It does have to do with privileges, just a privilege on a column ID. Is your feedback that you want the name to indicate that they're columns? What's not obvious as written? http://gerrit.cloudera.org:8080/#/c/11753/4/src/kudu/tserver/tablet_service.cc@1820 PS4, Line 1820: TabletServerErrorPB::Code error_code = TabletServerErrorPB::UNKNOWN_ERROR; > Maybe adjust the control flow so it looks a little more like Scan? Done -- 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: 5 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 07:29:42 +0000 Gerrit-HasComments: Yes
