Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19910 )
Change subject: [jwt] switching JWT verification to KeyBasedJwtVerifier ...................................................................... Patch Set 6: (8 comments) > Do we need a new test for this? Or are the existing tests enough? Just did a quick look over the patch, I'm low on time right now. +1 for adding a new test at least to cover the situation that this patch is addressing Also, if they want to change the verifier later on, maybe it makes sense to make it configurable w.r.t. what type of verifier is instantiated. http://gerrit.cloudera.org:8080/#/c/19910/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19910/6//COMMIT_MSG@10 PS6, Line 10: oidc nit: OIDC (since using JWT, not jwt) http://gerrit.cloudera.org:8080/#/c/19910/6//COMMIT_MSG@11 PS6, Line 11: jwks nit: JWKS http://gerrit.cloudera.org:8080/#/c/19910/6//COMMIT_MSG@15 PS6, Line 15: not able not be able ? http://gerrit.cloudera.org:8080/#/c/19910/6//COMMIT_MSG@16 PS6, Line 16: url nit: URL http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/jwt-util.h@107 PS6, Line 107: KeyBasedJwtVerifier(std::string jwks_uri, bool is_local_file, bool jwks_verify_server_certificate, : const std::string& jwks_ca_certificate) : : jwt_(JWTHelper::GetInstance()), : jwks_uri_(std::move(jwks_uri)), : is_local_file_(is_local_file), : jwks_verify_server_certificate_(jwks_verify_server_certificate), : jwks_ca_certificate_(jwks_ca_certificate) { : } Maybe, make the signature of the constructors more tailored to the usage patterns, i.e. having two constructors instead of one: KeyBasedJwtVerifier(const string& jwks_file_path); KeyBasedJwtVerifier(const string& jwks_uri, bool jwks_verify_server_certificate, const string& jwks_ca_certificate); http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/mini_oidc.h File src/kudu/util/mini_oidc.h: http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/mini_oidc.h@93 PS6, Line 93: return jwks_url_; nit: add a DCHECK() to make sure this method is called only when jwks_url_ has already been initialized http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/mini_oidc.cc File src/kudu/util/mini_oidc.cc: http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/mini_oidc.cc@141 PS6, Line 141: "RS256", : kRsaPubKeyJwkN, : kRsaPubKeyJwkE, kKid2, : "RS256", : kRsaInvalidPubKeyJwkN, : kRsaPubKeyJwkE), nit: wrong indent? http://gerrit.cloudera.org:8080/#/c/19910/6/src/kudu/util/mini_oidc.cc@147 PS6, Line 147: nit: wrong indent? -- To view, visit http://gerrit.cloudera.org:8080/19910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1f166807bfcf7051bda7843e186eacfbe379eba Gerrit-Change-Number: 19910 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 26 May 2023 00:04:47 +0000 Gerrit-HasComments: Yes
