Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18467 )
Change subject: util: pull jwt code from Impala ...................................................................... Patch Set 2: (24 comments) Both LINT and IWYU fail -- please make sure those pass. From my assessment, this code is of not high enough quality to meet standards we use for Kudu. I'd suggest to try improving that right away, but we can discuss other options. Also, did you check recent updates on this functionality in the Impala repo? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h File src/kudu/util/jwt-util-internal.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49 PS2, Line 49: algorithm missed std::move()? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@49 PS2, Line 49: pub_key missed std::move()? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@65 PS2, Line 65: const std::string algorithm_; This looks a bit lame to use arbitrary string representation for an enumerated entity. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@78 PS2, Line 78: std::string algorithm, here and below: I'm not sure I understand why to have this 'algorithm' as a parameter when it's already clear this is a key for a particular algorithm (here it's HS256 algorithm)? Am I missing something? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-internal.h@79 PS2, Line 79: style nit for here and below: per Kudu code style, the indent is wrong (see https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists) http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@35 PS2, Line 35: std::string rsa_priv_key_pem = R"(-----BEGIN PRIVATE KEY----- Should all these be constant strings? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@370 PS2, Line 370: int fd = mkstemp(&name_[0]); IIRC, in Kudu we do have env::NewTempWritableFile() for this. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@372 PS2, Line 372: std::cout << "Error creating temp file; " << strerror(errno) << std::endl; Throughout this code: * errors are usually written into std:cerr, not std::cout * before doing anything that might change errno (like memory allocation or other activity in outputting to streams) it's necessary to store errno into a temporary variable, and only then work with it. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@542 PS2, Line 542: EXPECT_OK(status); What is this for? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util-test.cc@1141 PS2, Line 1141: ASSERT_TRUE(status.ToString().find("Verification failed, error: token expired") : != std::string::npos) Here and elsewhere: we do have ASSERT_STR_CONTAINS() for this in Kudu. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h File src/kudu/util/jwt-util.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.h@71 PS2, Line 71: const JWTDecodedToken* decoded_token In Kudu we do pass input parameters as constant references, not constant pointers. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@20 PS2, Line 20: #include <string.h> nit: use C++ style includes instead (i.e. this should be <cstring>) http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@90 PS2, Line 90: tring NameOfTypeOfJsonValue(const Value& value) { This can be a static method. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@118 PS2, Line 118: else nit: remove 'else' for readability http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@272 PS2, Line 272: hs256 Should these strings be constants and used as such elsewhere in the code? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@341 PS2, Line 341: status Why not just return the status from here? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@346 PS2, Line 346: if (!status.ok()) return status; There is RETURN_NOT_OK() for this http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@358 PS2, Line 358: (const unsigned char*) Use C++ casts instead http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@372 PS2, Line 372: BIO* bio = BIO_new(BIO_s_mem()); : PEM_write_bio_RSA_PUBKEY(bio, rsa); We do have safer ssl_make_unique() for these in Kudu. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@616 PS2, Line 616: auto find_it = hs_key_map_.find(kid); : if (find_it == hs_key_map_.end()) { : // Could not find key for the given key ID. : return nullptr; : } : return find_it->second.get(); In Kudu we have FindPtrOrNull() for this. http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/jwt-util.cc@793 PS2, Line 793: decoded_token->decoded_jwt_ nit: create a reference decoded_token->decoded_jwt_ and use it here and below http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h File src/kudu/util/promise.h: http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@51 PS2, Line 51: const T& Get(int64_t timeout_millis, bool* timed_out) const I think the overall semantics of this method is somewhat bogus due to the signature that returns a constant reference to the underlying 'val_' object (i.e. the future object). In case of timeout, the returned value wouldn't make much sense and might cause issues if accessed by the callee. Why WaitFor(const MonoDelta&) isn't enough? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@53 PS2, Line 53: return val_; : *timed_out = false; It seems 'return' is misplaced here? http://gerrit.cloudera.org:8080/#/c/18467/2/src/kudu/util/promise.h@56 PS2, Line 56: *timed_out = true; It seems 'return' is missed here? -- To view, visit http://gerrit.cloudera.org:8080/18467 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib546762aa7ff53009f32688c30c5642ee4df494b Gerrit-Change-Number: 18467 Gerrit-PatchSet: 2 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: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 19 Oct 2022 19:36:37 +0000 Gerrit-HasComments: Yes
