Zoltan Chovan 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: (22 comments) http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19709/1//COMMIT_MSG@19 PS1, Line 19: tls > nit: TLS Done 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 sen Yes, I agree. There are no invalid/untrusted certificates in 'test_certs.cc', however I found that we can generate certificates on the fly with the methods in 'cert_management.cc'. What would you suggest, should I create new methods in test_certs to have invalid/untrusted ready to use, or should I generate them using the cert_managment tools? http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/integration-tests/security-itest.cc@537 PS1, Line 537: > nit: misaligned indent? Done 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? Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/mini-cluster/external_mini_cluster.cc@283 PS1, Line 283: > nit: wrong indent? Done 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'? Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@281 PS1, Line 281: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@282 PS1, Line 282: if > nit: drop 'if'? Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/server/server_base.cc@815 PS1, Line 815: > nit: wrong indent Done 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? sorry, that was left in 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 you can use this to validate a local file that is found at 'jwks_uri', in this case there is no data going through https so there is no need to validate certificates 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? Done 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@606 PS1, Line 606: jwks_ca_certificate > it's unused Done 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 argum Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@798 PS1, Line 798: > nit: wrong indent Done http://gerrit.cloudera.org:8080/#/c/19709/1/src/kudu/util/jwt-util.cc@801 PS1, Line 801: > nit: wrong indent Done 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' a Done 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 Done 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 Done 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 Done 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 addres Done 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 add Done -- 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: Tue, 11 Apr 2023 19:22:23 +0000 Gerrit-HasComments: Yes
