Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: WIP KUDU-2543: pass around default authz tokens ...................................................................... Patch Set 1: (17 comments) http://gerrit.cloudera.org:8080/#/c/11751/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11751/1//COMMIT_MSG@16 PS1, Line 16: GetTabletSchema GetTableSchema http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/client-internal.cc@664 PS1, Line 664: if (resp.has_authz_token()) { If the response doesn't have an authorization token, should an existing token in the cache get removed? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/client-internal.cc@749 PS1, Line 749: AuthorizeForTable nit: naming wise I think something like 'RetrieveAuthorizationToken' or similar would be better, 'AuthorizeForTable' implies there's a specific action to authorize. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.h@92 PS1, Line 92: , here and above the comma isn't necessary. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.cc@197 PS1, Line 197: client $0 'user $0 for table $1' http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.cc@348 PS1, Line 348: VLOG(1) << "no authz token for table " << table_->id(); This is an expected outcome when connecting to an older master, right? May want to add a comment to that effect. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master.proto@635 PS1, Line 635: and authz is enabled on : // the cluster I think we want to have it always enabled, right? At that point it would only be missing if the master was an older version. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master.proto@636 PS1, Line 636: reutrns returns http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h@99 PS1, Line 99: z Authn http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_header.proto File src/kudu/rpc/rpc_header.proto: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_header.proto@333 PS1, Line 333: authentication authorization http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_header.proto@335 PS1, Line 335: FATAL_INVALID_AUTHORIZATION_TOKEN This should be an ERROR, not FATAL, since the connection should be able to continue after the client grabs a new authz token. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/server_negotiation.cc@650 PS1, Line 650: // TODO(KUDU-1924): propagate the specific token verification failure back to the client, Is this done now? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@142 PS1, Line 142: DEFINE_bool(enforce_access_control, false, We should think about whether this configuration could be dynamically set by checking whether the master is issuing authz tokens, and if so turning it on. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@382 PS1, Line 382: static bool VerifyAuthzTokenOrRespond(const TokenVerifier& token_verifier, Should this be checking that the username in the token and the username from the context match? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@881 PS1, Line 881: boost::optional<TablePrivilegePB> table_privilege; not used? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@892 PS1, Line 892: context->RespondRpcFailure(rpc::ErrorStatusPB::FATAL_UNAUTHORIZED, I assume this should have a TODO? http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1394 PS1, Line 1394: } Also needs a TODO? -- 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: 1 Gerrit-Owner: Andrew Wong <[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, 23 Oct 2018 00:10:27 +0000 Gerrit-HasComments: Yes
