Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18468 )
Change subject: jwt: add test for fetching JWKS via URL ...................................................................... Patch Set 16: (13 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: > Even the public keys in JWTPublicKeyMap are unique pointer, we still need t Wenzhe, is your concern about adding virtual destructor in case if this class is used as a base in the future? That's a good point, and as alternative it's possible to declare this class as final (Kudu uses C++17, so it's possible to use final classes). 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 out of scope, i.e. once the constructor is done. I guess it's necessary to make std::unique_ptr<WritableFile> a member of this class to avoid this to happen? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util-test.cc@410 PS16, Line 410: void TempTestDataFile::Delete() { : if (deleted_) return; : deleted_ = true; : if (remove(name_.c_str()) != 0) { : std::cout << "Error deleting temp file; " << strerror(errno) << std::endl; : abort(); : } : } I'm not sure this is needed since the file created with NewTempWritableFile isn't supposed to be here once its unique_ptr wrapper goes out of scope. 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? * server is not yet started upon calling JWTHelper::Init() * server is started with different algorithm than the created JWT token * server was running upon calling JWTHelper::Init(), but shutdown when calling JWTHelper::Verify() on a token * server is started with different algorithm than the created token, but later on the algorithm is updated to match the algorithm (or the corresponding algorithm is added) * server was started with necessary algorithm, but later on it was restarted with an empty set of keys: what happens when attempting to verify a token that was valid some time ago when the server supplied the key matching the token, but now no key are supplied by the server? 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: if (kv_map.find(key) == kv_map.end()) { : kv_map.insert(make_pair(key, value)); : } else { : LOG(WARNING) << "Duplicate property of JWK: " << key; : } nit: to avoid double lookups, this could be if (!EmplaceIfNotPresent(&kv_map, key, value)) { LOG(WARNING) << "Duplicate property of JWK: " << key; } http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@189 PS16, Line 189: kv_map.find nit: we strive to use wrappers from map-util.h for things like this, so consider updating this piece to use the following pattern const auto* kty = FindOrNull(kv_map, "kty"); if (!kty) { return Status::InvalidArgument("'kty' property is required"); } http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@193 PS16, Line 193: string key_id nit: could this be 'const auto& key_id' to avoid copying? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@201 PS16, Line 201: std::unique_ptr nit for here and below: you could add 'using std::unique_ptr;' to file file and drop the 'std::' namespace prefix http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@307 PS16, Line 307: JWTPublicKey* jwt_pub_key = nullptr; : try { : if (algorithm == "hs256") { : jwt_pub_key = new HS256JWTPublicKey(algorithm, it_k->second); : } else if (algorithm == "hs384") { : jwt_pub_key = new HS384JWTPublicKey(algorithm, it_k->second); : } else if (algorithm == "hs512") { : jwt_pub_key = new HS512JWTPublicKey(algorithm, it_k->second); : } else { : return Status::InvalidArgument(Substitute("Invalid 'alg' property value: '$0'", algorithm)); : } nit for here and elsewhere with allocating JWTPublicKey: since using unique_ptr, it would make sense to update this to be more leak-proof (there isn't a leak possibility right now, but it might suddenly appear if this code modified later): unique_ptr<JWTPublicKey> jwt_pub_key; ... if (...) { jwt_pub_key.reset(new HS256JWTPublicKey(...)); } else if (...) { jwt_pub_key.reset(new ...); } *pub_key_out = std::move(jwt_pub_key); http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@611 PS16, Line 611: curl.set_verify_peer(false); That looks pretty weird since it opens all this JWT stuff to various MITM attacks. Should we at least add a flag to be able to turn verification of the peer's certificate on and off, and by default it should be on, while for testing purposes we can turn off the verification of the server's certificate? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@615 PS16, Line 615: if (dst.size() > 0) What if the server doesn't have any keys in its response? Should the already known keys be removed in that case? http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@618 PS16, Line 618: if (jwks_checksum_ == cur_jwks_checksum) return Status::OK(); Should the code set '*is_changed = false;' here before returning from this method? Otherwise, the caller might be reading uninitialized variable passed to 'is_changed' as an output parameter. http://gerrit.cloudera.org:8080/#/c/18468/16/src/kudu/util/jwt-util.cc@707 PS16, Line 707: impala-server Why is this set to "impala-server"? Maybe, set this to "JWT" or something like that? -- 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: 16 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 02:07:14 +0000 Gerrit-HasComments: Yes
