Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala ...................................................................... Patch Set 8: (7 comments) 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: otAut > Did you miss to address this comment or there is some other reason behind? This is a bit weird, I think it's okay if we move these to constant, but they are not used anywhere else. At the end these only indicate which struct to pass to the verifier's allow_algorithm method as a parameter. Unfortunately the allow_algorithm's parameter is a struct (jwt::algorithm::*) from the jwt-cpp thirdparty library. E.g https://github.com/Thalhammer/jwt-cpp/blob/master/include/jwt-cpp/jwt.h#L971 So we can leave these as they are single use, or move them to constants, alternatively we can re-work the constructors for the public key variants, but that might take some serious refactoring. What would you recommend? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@688 PS2, Line 688: if (jwks_update_thread_ != nullptr) jwks_update_thread_->Join() > Did you find the reason behind having this as a warning instead of an error Sorry I haven't replied to this. The only reason why this may be okay as a warning is that the JWKS information might be changing over time, if for some reason, the file the URI is pointing at becomes empty, it'll still be downloaded, but no keys will be verified successfully (due to no public keys in the jwks to do so). Since the UpdateJWKSThread is still alive, if the jwks file/endpoint is fixed, then the verification will be successful again. 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: DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero); : : using std::string; : using strings::Substitute; : : namespace kudu { : > nit: instead of duplicating the code, consider introducing a function and t Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@474 PS7, Line 474: // ECDSA using P-521 and SHA-512 (OBJ_txt2nid("secp521r1")). : eccgrp = NID_secp521r1; : } else { : return Status::NotSupported(Substitute("Unsupported alg: '$0'", algorithm)); : } : } : : auto it_x = kv_map.find("x"); : auto it_y = kv_map.find("y"); > Since there isn't any return statement involved in three if() scopes below, Ack http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@734 PS7, Line 734: << status.ToString(); > nit: this could be moved out of the cycle and saved to be reused in this cy Done http://gerrit.cloudera.org:8080/#/c/18467/7/src/kudu/util/jwt-util.cc@804 PS7, Line 804: } catch (const std::invalid_argument& e) { : status = Status::NotAuthorized( : Substitute("Token is not in correct format, error: $0", e.what())); : } catch (const std::runtime_error& e) { : status = Status::NotAuthorized( : Substitute("Base64 decoding failed or invalid json, error: $0", e.what())); : } > nit: could you structured binding here instead Done 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 wel Done -- 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: 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: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 07 Nov 2022 10:00:17 +0000 Gerrit-HasComments: Yes
