Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19709 )
Change subject: [jwt] Verify JWKS URL server TLS certificate by default ...................................................................... Patch Set 1: (19 comments) http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: PS1: Once certificate verification for JWKS server is in place, does it make sense to add a 'negative' test case where the certificate of the JWKS server is (a) not valid and (b) not trusted, and that leads to a failure while communicating with such JWKS servers? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc@537 PS1, Line 537: nit: misaligned indent? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@280 PS1, Line 280: nit: remove this extra empty line? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@283 PS1, Line 283: nit: wrong indent? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@277 PS1, Line 277: without The name of the flag signals it should be 'with'? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@282 PS1, Line 282: if nit: drop 'if'? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/curl_util.cc@167 PS1, Line 167: todo(zchovan) consolidate these two checks Why? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@72 PS1, Line 72: bool is_local_file) How is this 'is_local_file' relevant once having these 'jwks_uri', 'jwks_ca_certificate' parameters for the signature of this method? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@71 PS1, Line 71: bool jwks_verify_server_certificate, : const std::string& jwks_ca_certificate nit: maybe, switch the order of the 'jwks_verify_server_certificate' and 'jwks_ca_certificate' parameters? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.h@144 PS1, Line 144: bool jwks_verify_server_certificate_ Could this be 'const' as well? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@794 PS1, Line 794: false, "" style nit: add the comments with the names of the parameters for the arguments at this call site http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@798 PS1, Line 798: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@801 PS1, Line 801: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@949 PS1, Line 949: false, "" style nit: add the comments with the names of the parameters for 'false' and '""' arguments http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1025 PS1, Line 1025: false style nit: keep the name of the parameter in the comment http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@1042 PS1, Line 1042: false style nit: keep the name of the parameter in the comment http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h File src/kudu/util/mini_oidc.h: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.h@43 PS1, Line 43: std::string server_certificate; nit: could be great to document the newly added fields in-line as the rest of the fields of MiniOidcOptions http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc File src/kudu/util/mini_oidc.cc: http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@131 PS1, Line 131: bound_addrs nit: consider renaming this variable since this is no longer a bound address? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/mini_oidc.cc@134 PS1, Line 134: RETURN_NOT_OK(addr.ParseString(bound_addrs[0].host(), bound_addrs[0].port())); Perhaps, add a comment that calling ParseString() is just to verify the address components? -- To view, visit http://gerrit.cloudera.org:8080/19709 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0fd7b53d651786bbe57642dd14cd477055b80c78 Gerrit-Change-Number: 19709 Gerrit-PatchSet: 1 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: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Fri, 07 Apr 2023 05:41:38 +0000 Gerrit-HasComments: Yes
