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 13: (7 comments) http://gerrit.cloudera.org:8080/#/c/18469/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18469/13//COMMIT_MSG@10 PS13, Line 10: It is limited in the sense that JWTs can be sent over unencrypted : channels > In ServerNegotiation::AuthenticateByJwt(), you already added I guess here they meant that the authenticity of the server side isn't verified yet. See https://gerrit.cloudera.org/#/c/18469/8/src/kudu/rpc/client_negotiation.cc@357 for details. At least, in PS13 we have a TODO added to address that later on. http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/client_negotiation.h File src/kudu/rpc/client_negotiation.h: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/client_negotiation.h@191 PS13, Line 191: * style nit: stick the asterisk to the type, not to the parameter/variable name http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/messenger.cc@324 PS13, Line 324: jwt_verifier_(new SimpleJwtVerifier), Why to set a non-functional verifier? Is it possible to keep 'jwt_verifier_' as std::nullopt and set the actual JWT verifier later on? It would help to add a comment on that. http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/negotiation.cc@178 PS13, Line 178: const auto jwt = messenger->jwt(); nit: remove this variable and use messenger->jwt() in-place in the ClientNegotiation constructor http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/rpc/server_negotiation.cc@729 PS13, Line 729: subject nit: could be std::move(subject) ? http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.h@97 PS13, 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/13/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18469/13/src/kudu/util/jwt-util.cc@929 PS13, Line 929: CHECK_OK(jwt_->Init(jwks_uri_, is_local_file_)); : return Status::OK(); Why not just return jwt_->Init(...); ? I'd think we don't want to crash a server or a client if for some reason JWT helper failed to initialize, no? -- 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: 13 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, 06 Dec 2022 02:08:15 +0000 Gerrit-HasComments: Yes
