Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11754 )
Change subject: authz: verify tokens on writes ...................................................................... Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/11754/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11754/2//COMMIT_MSG@12 PS2, Line 12: As such, this patch pushes : authorization into the prepare phase, right after decoding the : operations. I think I agree with this, but in my mind the biggest concern is the possibility of an unauthorized user DoS'ing the cluster. Do you think the authz front-loading vs. back-loading strategy has an effect on that? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h File src/kudu/tablet/transactions/write_transaction.h: http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@a169 PS2, Line 169: : : : : This approach was never actually merged; does this patch need to be rebased? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@67 PS2, Line 67: inline std::string WritePrivilegeToString(const WritePrivilegeType& type) { You sure you want to inline this vs. define it in the .cc file? It's not performance-sensitive code, but it does seem like the sort of thing that might be used often enough that multiple copies may not be desirable. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@92 PS2, Line 92: RowOpTypes requested_op_types; RowOpTypes isn't defined in this patch, and its parent patch no longer uses it. You'd probably want to iterate over the transaction's decoded ops in CheckPrivileges, right? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc File src/kudu/tserver/tablet_server_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@473 PS2, Line 473: vector<T> RandomFromList(const vector<T>& full_list, int min_to_return) { Is this the same as Random::ReservoirSample? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@474 PS2, Line 474: int num_to_return = min_to_return + rand() % (full_list.size() - min_to_return); Should probably assert/check/whatever that min_to_return > full_list.size(). http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1145 PS2, Line 1145: void SetUp() override { Seems like TestInvalidAuthzTokens could benefit from this too. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1204 PS2, Line 1204: const WritePrivileges& privileges) const { Nit: indentation. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1205 PS2, Line 1205: RpcController rpc; : WriteResponsePB resp; : WriteRequestPB req = BuildRequest(write_ops, privileges); Nit: reorder as req -> resp -> rpc? I think that's almost a convention. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1225 PS2, Line 1225: vector<WritePrivilegeType> priveleges_copy(privileges.begin(), privileges.end()); Why is the copy necessary? Why can't you select the priv to remove directly from 'privileges', then erase it? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1328 PS2, Line 1328: vector<RowOperationsPB::Type> random; Nit: rename this? I thought it was of type Random originally. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1336 PS2, Line 1336: switch (op_type) { The "convert row op type into privilege type" logic also exists in write_transaction.cc; can you consolidate? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@636 PS2, Line 636: if (status_.IsNotAuthorized()) { : LOG(WARNING) << Substitute("rejecting transaction from $0: $1", : context_->requestor_string(), status_.ToString()); : } Curious why we want to log these and not other write errors? http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@1083 PS2, Line 1083: if (!CheckMatchingTableId(privilege, replica->tablet_metadata()->table_id(), Should probably suffix this with OrRespond to clear all doubt about whether there's a missing response here. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@1097 PS2, Line 1097: if (privileges.empty()) { Should add a comment here about how we can reject with no privs outright, but otherwise we'll defer authz to Prepare (when we've decoded the row ops and know whether the user's privileges allow for the requested op types). -- To view, visit http://gerrit.cloudera.org:8080/11754 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefa2215d528a64f525e04bec111b25f8bc17c086 Gerrit-Change-Number: 11754 Gerrit-PatchSet: 2 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: Mon, 25 Mar 2019 00:01:16 +0000 Gerrit-HasComments: Yes
