Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11751 )
Change subject: WIP KUDU-2543: pass around default authz tokens ...................................................................... Patch Set 2: (37 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: > GetTableSchema Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@94 PS1, Line 94: using rpc::Rpc; > warning: using decl 'Rpc' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@95 PS1, Line 95: using tserver::WriteRequestPB; > warning: using decl 'RpcController' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@96 PS1, Line 96: using tserver::WriteResponsePB; > warning: using decl 'ServerPicker' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@97 PS1, Line 97: using tserver::WriteResponsePB_PerRowErrorPB; > warning: using decl 'SignedTokenPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/batcher.cc@483 PS1, Line 483: o > z ? Done 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: const string& table_name, > If the response doesn't have an authorization token, should an existing tok Good question. That would simplify revoking client privileges if we wanted to get around to implementing that. Otherwise, I'm trying to think of a scenario where this would be desirable. Assuming we always use authz tokens once the feature is shipped, this would only happen if the client starts talking to an older master, no? In which case I don't think it would matter. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/client-internal.cc@749 PS1, Line 749: name = leader_add > nit: naming wise I think something like 'RetrieveAuthorizationToken' or sim Done 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: a > here and above the comma isn't necessary. Done 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@65 PS1, Line 65: using strings::Substitute; > warning: using decl 'SignedTokenPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/client/scanner-internal.cc@348 PS1, Line 348: } > This is an expected outcome when connecting to an older master, right? May Done 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: : > I think we want to have it always enabled, right? At that point it would o True, updated to incorporate both statements. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master.proto@636 PS1, Line 636: > returns Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc@83 PS1, Line 83: using kudu::security::SignedTokenPB; > warning: using decl 'ColumnPrivilegePB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/master/master_service.cc@422 PS1, Line 422: = server_- > This behavior assumes other error cases (like empty remote_user().username( Hrm, indeed. We should probably just return an error, unless having no username is something we support (don't think that's the case though with secured clusters), right? 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: > Authn Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/retriable_rpc.h@175 PS1, Line 175: : template <class Server, class RequestPB, class ResponsePB> : void RetriableRpc<Server, RequestPB, ResponsePB>::GotNewAuthnTokenRetryCb( : const Status& status) { : GotNewTokenRetryCb(status, "authn"); : } : : template <class Server, class RequestPB, class ResponsePB> : void RetriableRpc<Server, RequestPB, ResponsePB>::GotNewAuthzTokenRetryCb( : const Status& status) { : GotNewTokenRetryCb(status, "authz"); : } : > This looks identical to GetNewAuthnTokenAndRetryCb() with exception of VLOG Done 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 Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_header.proto@335 PS1, Line 335: FATAL_INVALID_AUTHENTICATION_TOKE > This should be an ERROR, not FATAL, since the connection should be able to Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h File src/kudu/rpc/rpc_verification_util.h: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@37 PS1, Line 37: RPC > RPC error code? Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@40 PS1, Line 40: ed. > This comment looks very specific to me since there is no 'token' concept de Done, though it's worth noting that VerificationResult is the result of verifying a token. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.h@41 PS1, Line 41: ParseVerificationRes > nit: name it ParseVerificationResult to be more clear? Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc File src/kudu/rpc/rpc_verification_util.cc: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@27 PS1, Line 27: using security::VerificationResult; > warning: using decl 'SignedTokenPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@28 PS1, Line 28: > warning: using decl 'TokenPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@45 PS1, Line 45: > nit here and below: why this variable is necessary? Could it be just Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/rpc/rpc_verification_util.cc@46 PS1, Line 46: > std::move ? Done 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? No, the Jira goes into more detail on what specific things could be sent back. 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: > We should think about whether this configuration could be dynamically set b That's a reasonable thing to consider, though the benefit of having a feature flag is a bit more fail-safe. E.g. if we discover an issue that prevents a cluster from being usable outright, it'd be nice to just flip a flag to get around it (if that sort of thing is acceptable from a security perspective). http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@382 PS1, Line 382: template <class AuthorizableRequest> > Should this be checking that the username in the token and the username fro Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@388 PS1, Line 388: > Does FATAL_UNAUTHORIZED intent to cover both authentication and authorizati Hrm, that's fair. FATAL_UNAUTHORIZED here means that the request is coming from a source that we expect to not have the right permissions to make the request (i.e. it's not just a matter of refreshing their token). In this case, a missing authz token likely means that the request is being made by an old client. And I think we can use it for both; both mean that the client should stop getting anything from the servers (instead of retrying). http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@402 PS1, Line 402: > Why is FATAL_UNAUTHORIZED code chosen here instead of INVALID_AUTHZ_TOKEN? Yeah, we've discusses this here and there; I think a fair summary is that FATAL_UNAUTHORIZED will be used when the user isn't authorized but has a valid token. In all other cases, I think we'll send back a retriable error code. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@881 PS1, Line 881: } > not used? Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@892 PS1, Line 892: if (!VerifyAuthzTokenOrRespond(server_->token_verifier(), req, context, &token)) { > I assume this should have a TODO? Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1390 PS1, Line 1390: > It seems already requests to existing scanners are not going to be examined Addressing this via KUDU-1918. Ah I still need to move the authz token into NewScanRequestPB. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1394 PS1, Line 1394: TRACE_EVENT0("tserver", "TabletServiceImpl::Scan"); > Also needs a TODO? Done http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tablet_service.cc@1489 PS1, Line 1489: > Maybe, add a TODO into this method regarding verification of authz token in Better yet, this needs verification. http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/11751/1/src/kudu/tserver/tserver.proto@307 PS1, Line 307: p if you are > Add SignedTokenPB for ChecksumRequestPB in tserver.proto as well? 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: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Thu, 15 Nov 2018 08:28:13 +0000 Gerrit-HasComments: Yes
