Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL ...................................................................... Patch Set 19: (5 comments) http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193 PS16, Line 193: return Stat > Yep, that's right Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@307 PS16, Line 307: if (algorithm == "hs256") { : jwt_pub_key.reset(new HS256JWTPublicKey(algorithm, it_k->second)); : } else if (algorithm == "hs384") { : jwt_pub_key.reset(new HS384JWTPublicKey(algorithm, it_k->second)); : } else if (algorithm == "hs512") { : jwt_pub_key.reset(new HS512JWTPublicKey(algorithm, it_k->second)); : } else { : return Status::InvalidArgument(Substitute("Invalid 'alg' property value: '$0'", algorithm)); : } : } catch (const std::exception& e) { : r > nit for here and elsewhere with allocating JWTPublicKey: since using unique Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611 PS16, Line 611: // TODO support CurlAuthType > We should add a flag to turn on verification of the peer's certificate in f @Alexey, yes this makes sense, I agree we should make it possible to enable peer cert verification. Thanks for the attack vector explanations! As both you and @Wenzhe has mentioned, I'll create a new change to add the flag to do this. http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@620 PS18, Line 620: // Append '\0' so t > We can move line #617 (*is_changed = false) to the beginning of the functio Done http://gerrit.cloudera.org:8080/#/c/18468/18/src/kudu/util/jwt-util.cc@739 PS18, Line 739: SetJWKSSnapshot(new_jwks); : if (new_jwks->IsEmpty()) { : > Add a warning log message? yes it's a leftover, but I do agree with Wenzhe, we should add a warning here -- To view, visit http://gerrit.cloudera.org:8080/18468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ief272c813b62e789d747a88e1f3be8c406eed3f8 Gerrit-Change-Number: 18468 Gerrit-PatchSet: 19 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: Sat, 19 Nov 2022 17:57:18 +0000 Gerrit-HasComments: Yes
