Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala ...................................................................... Patch Set 7: Code-Review+1 (7 comments) Looks better now: a few nits http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272 PS2, Line 272: > Should these strings be constants and used as such elsewhere in the code? Did you miss to address this comment or there is some other reason behind? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: return FindPointeeOrNull(ec_pub_key_map_, kid); > That's a bit funny to see as a warning since in other places it's considere Did you find the reason behind having this as a warning instead of an error? Would be great adding a comment on that since it's treated as error in other part of the JWT code. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@84 PS7, Line 84: [](const char* name, const int32_t val) { : if (val <= 0) { : LOG(ERROR) << Substitute("Invalid value for $0 flag: $1", name, val); : return false; : } : return true; : }); nit: instead of duplicating the code, consider introducing a function and then refer it in both validators http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474 PS7, Line 474: if (algorithm == "es256") { : // ECDSA using P-256 and SHA-256 (OBJ_txt2nid("prime256v1")). : eccgrp = NID_X9_62_prime256v1; : } : if (algorithm == "es384") { : // ECDSA using P-384 and SHA-384 (OBJ_txt2nid("secp384r1")). : eccgrp = NID_secp384r1; : } : if (algorithm == "es512") { Since there isn't any return statement involved in three if() scopes below, keeping the original 'else if' would make the code more efficient? http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: MonoDelta::FromMilliseconds(timeout_millis) nit: this could be moved out of the cycle and saved to be reused in this cycle over and over again. http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804 PS7, Line 804: for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_header_claims()) { : msg << claim.first << "=" << claim.second.to_json().serialize() << ";"; : } : msg << " JWT token payload: "; : for (auto& claim : decoded_token_out.get()->decoded_jwt_.get_payload_claims()) { : msg << claim.first << "=" << claim.second.to_json().serialize() << ";"; : } nit: could you structured binding here instead for (auto& [key, val] : ...) { msg << key << ...; } https://en.cppreference.com/w/cpp/language/structured_binding http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/promise.h@64 PS7, Line 64: NULL nit: since you are making changes here, maybe change NULL to nullptr as well -- To view, visit http://gerrit.cloudera.org:8080/18467 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 7 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: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Sun, 06 Nov 2022 16:56:30 +0000 Gerrit-HasComments: Yes
