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 2: (4 comments) I started taking a look and suddenly it became apparent that this patch needs to be properly rebased first. I left a couple of comments based on what I looked at before realizing the patch isn't current with its base. http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/messenger.h File src/kudu/rpc/messenger.h: http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/messenger.h@399 PS2, Line 399: boost::optional This patch needs to be rebased: it's been long time since we switched from boost::optional to std::optional. This changelist might provide somewhat useful details: https://github.com/apache/kudu/commit/3f0c434143ad6daa8ed4f39d8ceacbe58604dd29 http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/messenger.cc File src/kudu/rpc/messenger.cc: http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/messenger.cc@48 PS2, Line 48: #include "kudu/util/jwt.h" Indeed, the build fails with: 10:09:08 /home/jenkins-slave/workspace/kudu-master/3/src/kudu/rpc/messenger.cc:48:10: fatal error: kudu/util/jwt.h: No such file or directory 10:09:08 #include "kudu/util/jwt.h" 10:09:08 ^~~~~~~~~~~~~~~~~ 10:09:08 compilation terminated. 10:09:08 src/kudu/rpc/CMakeFiles/krpc.dir/build.make:172: recipe for target 'src/kudu/rpc/CMakeFiles/krpc.dir/messenger.cc.o' failed 10:09:08 make[2]: *** [src/kudu/rpc/CMakeFiles/krpc.dir/messenger.cc.o] Error 1 http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/rpc/server_negotiation.cc@701 PS2, Line 701: CHECK(tls_negotiated_); Actually, this widens DOS attack vector -- crashing a server like this is a big no-no :) Instead of CHECK(), change this to DCHECK(): crashing a server in release build doesn't help with preserving the consistency of the data that client has sent, and having DCHECK() is enough to protect against programming errors in Kudu code itself. In addition, add a check for tls_negotiated_ and return Status::IllegalState() and log a warning message if no TLS is negotiated. http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18469/2/src/kudu/util/jwt-util.h@99 PS2, Line 99: CHECK_OK This might lead to unneeded crashes when the JWS server isn't available. Instead, consider moving the jwt_->Init() into a separate method (KeyBasedJwtVerifier::Init()) and call it when on object of KeyBasedJwtVerifier is created or later on when the instance is about to be used (i.e. lazy initialization). -- 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: 2 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: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 19 Oct 2022 23:00:16 +0000 Gerrit-HasComments: Yes
