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 13: (14 comments) http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1173 PS2, Line 1173: > It seems this one was missed. Ack http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@84 PS5, Line 84: return true; : } : : DEFINE_validator(jwks_update_frequency_s, &ValidateBiggerThanZero); : DEFINE_validator(jwks_pulling_timeout_s, &ValidateBiggerThanZero); : : namespace kudu { > nit: instead of duplicating code, maybe introduce a method and use it for b I already fixed this, you might have been looking at an earlier changeset. http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@209 PS5, Line 209: JWTPublicKey* jwt_pub_key; : RETURN_NOT_OK(ECJWTPublicKeyBuilder::CreateJWKPublicKe > here and below: why not to use RETURN_NOT_OK() instead? The result status Done http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@267 PS7, Line 267: try { : // Call jwt-cpp API to verify token's signature. : verifier_.verify(decoded_jwt); : } catch (const jwt::error::rsa_exception& e) { : return Status::NotAuthorized(Substitute("RSA error: $0", e.what())); : } catch (const jwt::error::token_verification_exception& e) { : return Status::NotAuthorized( : Substitute("Verification failed, error: $0", e.what())); : } catch (const std::exception& e) { : return Status::NotAuthorized( : Substitute("Verification failed, error: $0", e.what())); : } : return Status::OK(); : } > nit: assigning status and continuing further hints that there is some non-t Done http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@526 PS7, Line 526: BN_bin2bn(reinterpret_cast<const unsigned char*>(ascii_x.c_str()), : static_cast<int>(ascii_x.size()), : nullptr), > nit: could use "c_unique_ptr<BIGNUM> x { BN_bin2bn(...), &BN_free }" here i Done http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@534 PS7, Line 534: nullptr), > nit: could use c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp Done http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@538 PS7, Line 538: security::c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp), &EC_KEY_free }; : EC_KEY_set_asn1_flag(e > Is these two lines result in the same as yepp, changed it http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@540 PS7, Line 540: if (EC_KEY_set_public_key_a > nit: could use 'auto bio = ssl_make_unique(BIO_new(BIO_s_mem()))' and get r Done http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@542 PS7, Line 542: unsigned char desc[1024] = { 0 }; : auto bio = security::ssl_make_unique(BIO_new > Per https://www.openssl.org/docs/man1.1.1/man3/BIO_read.html I cannot see t Ack http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@902 PS7, Line 902: return Status::NotAuthorized( : Substitute("Claim '$0' was not present", jwt_custom_claim_username)); : } : : username = decoded_jwt.get_payload_claim(jwt_custom_claim_username).to_json().to_str(); : : if (username.empty()) { : return Status::NotAuthorized( : Substitute("Claim '$0' is empty", jwt_custom_claim_username)); : } : : } catch (const std::runtime_error& e) { : return Status::NotAuthorized( : Substitute("Claim '$0' was not present, error: $1", jwt_custom_claim_username, : > nit: this could be a bit more readable Done http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@30 PS11, Line 30: #include <openssl/ssl.h> : #include <openssl > nit: move these two lines to line #23 to keep alphabet order Done http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@405 PS11, Line 405: nullptr) > nit: alignment Done http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@717 PS11, Line 717: JWKSMgr::UpdateJWKSThread() { : std::shared_ptr<JWKSSna > rebase with parent patch Done http://gerrit.cloudera.org:8080/#/c/18468/11/src/kudu/util/jwt-util.cc@872 PS11, Line 872: return Status::NotA > return status; 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: 13 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, 11 Nov 2022 19:47:10 +0000 Gerrit-HasComments: Yes
