Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11754 )
Change subject: authz: verify tokens on writes ...................................................................... Patch Set 3: (16 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 possib Yeah, though note it's not DoSing the cluster, but rather, DoSing the tablet. An "unauthorized" user could starve the prepare thread, though it's also worth noting that the user would still need _some_ write privileges to do so; if the user had _no_ write privileges, we'd reject the authz token immediately. We could probably avoid any kind of DoSing if we just decoded the op types up front, and we could even use the client schema and not take any locks in doing so. But in that case, we'd end up having to decode the operations fully again in the prepare thread, which is extra cycles. 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 Ack http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@67 PS2, Line 67: }; > You sure you want to inline this vs. define it in the .cc file? It's not pe Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@92 PS2, Line 92: // - A RowOp structure for each of the rows being inserted or mutated, which itself > RowOpTypes isn't defined in this patch, and its parent patch no longer uses Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tablet/transactions/write_transaction.h@287 PS2, Line 287: > warning: function 'kudu::tablet::WriteTransaction::WriteTransaction' has a Done 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: > Is this the same as Random::ReservoirSample? With some sugar, yes. The behavior I want that isn't in ReservoirSample is returning a non-fixed size. I'll reuse ReservoirSample here though. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@474 PS2, Line 474: // Utility to select an element from a container at random. > Should probably assert/check/whatever that min_to_return > full_list.size() Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1145 PS2, Line 1145: req.set_tablet_id(kTabletId); > Seems like TestInvalidAuthzTokens could benefit from this too. Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1204 PS2, Line 1204: if (!privileges.empty()) { > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1205 PS2, Line 1205: const auto& priv_to_remove = SelectAtRandom<WritePrivileges, WritePrivilegeType>( : required_privileges); : LOG(INFO) << "Removing write privilege: " << WritePrivi > Nit: reorder as req -> resp -> rpc? I think that's almost a convention. Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1225 PS2, Line 1225: } > Why is the copy necessary? Why can't you select the priv to remove directly Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_server_authorization-test.cc@1328 PS2, Line 1328: > Nit: rename this? I thought it was of type Random originally. Done 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@46 PS2, Line 46: #include "kudu/common/row_operations.h" > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@636 PS2, Line 636: public: : RpcTransactionCompletionCallback(rpc::RpcContext* context, : Response* response) : : co > Curious why we want to log these and not other write errors? Good point, probably wouldn't hurt in all cases. http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@1083 PS2, Line 1083: TokenPB token; > Should probably suffix this with OrRespond to clear all doubt about whether Done http://gerrit.cloudera.org:8080/#/c/11754/2/src/kudu/tserver/tablet_service.cc@1097 PS2, Line 1097: InsertOrDie(&privileges, WritePrivilegeType::UPDATE); > Should add a comment here about how we can reject with no privs outright, b Done -- 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: 3 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: Tue, 26 Mar 2019 22:33:51 +0000 Gerrit-HasComments: Yes
