Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: KUDU-2543 pt 1: basic checks for authz tokens ...................................................................... Patch Set 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_header.proto@316 PS7, Line 316: // The authorization token is invalid or expired. Unlike : // FATAL_INVALID_AUTHENTICATION_TOKEN, receipt of this code does not mean : // that the connection should be droppe > Curious why an invalid authz token is just an error while an invalid authn Based on the comment at L289, these indicate that the connection shouldn't be dropped. We must drop the connection for invalid authn tokens, since they authenticate the connection itself; a new connection should be negotiated with a new authn token. This isn't the case for authz tokens; an invalid authz token doesn't hold any bearing on the status of the connection. I'll amend the comments to describe this. http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc File src/kudu/rpc/rpc_verification_util.cc: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc@45 PS7, Line 45: } > warning: std::move of the variable 'retry_error' of the trivially-copyable Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/rpc/rpc_verification_util.cc@59 PS7, Line 59: return Status::NotAuthorized(VerificationResultToString(result)); > Looks like this can be pulled out of the three cases where it's explicitly Done Don't think so; these are all the cases right now, and if we ever introduce a new verification result, this should be updated, otherwise a compiler warning should surface. It's worth noting that `VerificationResult` is coming from a server-side function, not the client, so there's no risk of a hacked client feeding some bogus enum here. http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/CMakeLists.txt File src/kudu/tserver/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/CMakeLists.txt@180 PS7, Line 180: ADD_KUDU_TEST(tablet_server_authorization-test) > Nit: maybe tablet_server_authorization-test, or ts_authorization-test? Just Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@399 PS7, Line 399: return privilege.scan_privilege() || has_column_privileges; > warning: redundant boolean literal in conditional return statement [readabi Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1438 PS7, Line 1438: metrics->set_cfile_cache_hit_bytes( : context->trace()->metrics()->GetMetric(cfile::CFILE_CACHE_HIT_BYTES_METRIC_NAME)); : } : } // anonymous namespace : : void TabletServiceImpl::Scan(const ScanRequestPB* req, : ScanResponsePB* resp, : rpc::RpcContext* context) { : TRACE_EVENT0("tserver", "TabletServiceImpl::Scan"); : // Validate the request: user must pass a new_scan_request or : // a scanner ID, but not both. : if (PREDICT_FALSE(req->has_scanner_id() && : req-> > Looks like this can be shared amongst Scan/Checksum/SplitKeyRange? Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1562 PS7, Line 1562: > "split key range" I guess. Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tablet_service.cc@1703 PS7, Line 1703: > checksum Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver.proto@170 PS7, Line 170: message ListTabletsRequestPB { > Does this RPC need authorization since there's table metadata in the respon Not quite, see https://gerrit.cloudera.org/c/11752/1/src/kudu/tserver/tablet_service.h http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver_authorization-test.cc File src/kudu/tserver/tserver_authorization-test.cc: http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver_authorization-test.cc@174 PS7, Line 174: > warning: argument name 'authz_token_validity' in comment does not match par Done http://gerrit.cloudera.org:8080/#/c/11751/7/src/kudu/tserver/tserver_authorization-test.cc@227 PS7, Line 227: > Doesn't look like there's any shared state between requestors, so why not p Done -- To view, visit http://gerrit.cloudera.org:8080/11751 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99555e0ab2d09d4abcbc12b1100658a9a17590f4 Gerrit-Change-Number: 11751 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 03 Jan 2019 02:44:19 +0000 Gerrit-HasComments: Yes
