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 18: (11 comments) http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h File src/kudu/util/jwt-util-internal.h: http://gerrit.cloudera.org:8080/#/c/18468/15/src/kudu/util/jwt-util-internal.h@246 PS15, Line 246: public: > Wenzhe, is your concern about adding virtual destructor in case if this cla As per Alexey suggested, I've marked this class as final. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@397 PS16, Line 397: NewTempWritableFile(opts, &name_[0], &created_filename, &tmp_file); > The underlying tmp_file will be deleted automatically when 'tmp_file' goes So the TempTestDataFile class should have a std::unique_ptr<WritableFile> tmp_file_ member? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@1225 PS16, Line 1225: > Could we add coverage for negative scenarios as well? JWKSMockServer will be replaced in a followup patch with the MiniOIDC component. I think it might be better to cover these scenarios (some are already covered) there. What do you think? 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@182 PS16, Line 182: RETURN_NOT_OK(ReadKeyProperty(key, json_key, &value, /*required*/ false)); : if (!EmplaceIfNotPresent(&kv_map, key, value)) { : LOG(WARNING) << "Duplicate property of JWK: " << key; : } : } > nit: to avoid double lookups, this could be Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@189 PS16, Line 189: return Sta > nit: we strive to use wrappers from map-util.h for things like this, so con Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193 PS16, Line 193: return Stat > nit: could this be 'const auto& key_id' to avoid copying? since the FindOrNull returns the value already from the map, this line becomes unneccesary http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@201 PS16, Line 201: jwks_->AddHSKey > nit for here and below: you could add 'using std::unique_ptr;' to file file Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611 PS16, Line 611: RETURN_NOT_OK_PREPEND(curl.F > That looks pretty weird since it opens all this JWT stuff to various MITM a We can add a flag for this, but I'm not sure I understand the attack vector. The information in the JWKS is basically public keys for the generated tokens for verification. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615 PS16, Line 615: jwks_checksum_ = > What if the server doesn't have any keys in its response? Should the alrea I haven't really found anything about this in example implementations. However the comment in the header states the following: // Download JWKS JSON file from the given URL, then load the public keys if the // checksum of JWKS object is changed. If no keys were given in the URL, the internal // maps will be empty. So I think the known keys should be removed. However, currently the ParseKeys() method returns with an invalid argument status when there are no keys in the newly fetched jwks object, see line 154 of this file. Should I update the behaviour to match the comments? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@618 PS16, Line 618: return Status::OK(); > Should the code set '*is_changed = false;' here before returning from this Done http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@707 PS16, Line 707: eriodically c > Why is this set to "impala-server"? Maybe, set this to "JWT" or something Done -- 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: 18 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: Fri, 18 Nov 2022 17:25:42 +0000 Gerrit-HasComments: Yes
