Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18471 )
Change subject: plumb JWT authentication into clients ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/integration-tests/security-itest.cc@545 PS9, Line 545: FLAGS_rpc_trace_negotiation = true; nit: would be great to add a comment why customizing the setting for this flag http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@247 PS9, Line 247: Please mark all these new flags with the 'experimental' tag. http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@248 PS9, Line 248: jwt_token_auth Maybe, rename this into 'enable_jwt_token_authn'? http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@251 PS9, Line 251: jwt_validate_signature I guess jwt_validate_signature flag should be tagged 'unsafe'. http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@253 PS9, Line 253: DEFINE_bool(jwt_allow_without_tls, false, I guess jwt_allow_without_tls flag should be tagged 'unsafe'. http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@360 PS9, Line 360: ERROR > Should we change the log level to WARNING? Once this function returns 'false', a tablet server exits while processing input flags, so keeping it as ERROR makes sense, IMO. http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/server/server_base.cc@684 PS9, Line 684: } else { > Check if FLAGS_jwt_token_auth is true before writing warning message +1 Probably, there should be an extra outer scope like 'if (FLAGS_jwt_token_auth) { ... }' http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/util/jwt-util-test.cc@877 PS9, Line 877: impala nit: maybe change this to 'kudu'? http://gerrit.cloudera.org:8080/#/c/18471/9/src/kudu/util/jwt-util-test.cc@887 PS9, Line 887: ** nit: would be great to add the parameter name -- To view, visit http://gerrit.cloudera.org:8080/18471 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibdfc2640c047a2e3bb5ea03aea4439cea2532e23 Gerrit-Change-Number: 18471 Gerrit-PatchSet: 9 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: Wed, 14 Dec 2022 20:43:55 +0000 Gerrit-HasComments: Yes
