Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18469 )
Change subject: rpc: plumb JWTs into the RPC layer ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/18469/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18469/8//COMMIT_MSG@10 PS8, Line 10: It is limited in the sense that JWTs can be sent over unencrypted : channels -- this should be addressed before using in production. Has this been addressed in any of the follow-up patches yet? http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/rpc/client_negotiation.h@189 PS8, Line 189: nit: would be nice to add a short blurb to be in-line with the rest of the methods in this interface http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/rpc/client_negotiation.h@191 PS8, Line 191: nit: the indent is off http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/rpc/client_negotiation.cc@357 PS8, Line 357: jwt_ Should we add the requirement of having trusted certificate here as well, the same as for the authn_token_? I guess the idea is that the client should make sure it's not going to send its JWT token to a non-trusted party, no? At least add a TODO to point to the fact that should be eventually addressed, otherwise it's a security flaw. http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: PS8: Should this change be a part of a previous patch? http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt-util.h@97 PS8, Line 97: jwks_uri_(std::move(jwks_uri)), : is_local_file_(is_local_file) { nit: the indent is off http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt-util.cc@925 PS8, Line 925: CHECK_OK(jwt_->Init(jwks_uri_, is_local_file_)); : return Status::OK(); Why not just return jwt_->Init(...); ? I'd assume that Non-OK status of initting JWT environment should not lead server or client app crash. http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt.h File src/kudu/util/jwt.h: http://gerrit.cloudera.org:8080/#/c/18469/8/src/kudu/util/jwt.h@30 PS8, Line 30: nit: this indent should be 2 spaces according the style guideline -- To view, visit http://gerrit.cloudera.org:8080/18469 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I252f1e597d9df4408379c3b695f266dbd7f48dcc Gerrit-Change-Number: 18469 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 08 Nov 2022 03:44:58 +0000 Gerrit-HasComments: Yes
