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 7: (12 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: > Done It seems this one was missed. http://gerrit.cloudera.org:8080/#/c/18468/2/src/kudu/util/jwt-util-test.cc@1201 PS2, Line 1201: > picojson is the JSON library that the jwt-cpp library is using to handle JS Thanks for the clarification. 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: } : : 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 both validators? http://gerrit.cloudera.org:8080/#/c/18468/5/src/kudu/util/jwt-util.cc@209 PS5, Line 209: JWTPublicKey* jwt_pub_key; : status = ECJWTPublicKeyBuilder::CreateJWKPublicKey(kv_ here and below: why not to use RETURN_NOT_OK() instead? The result status isn't used below expect for just 'return status' anyways. 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: Status status; : try { : // Call jwt-cpp API to verify token's signature. : verifier_.verify(decoded_jwt); : } catch (const jwt::error::rsa_exception& e) { : status = Status::NotAuthorized(Substitute("RSA error: $0", e.what())); : } catch (const jwt::error::token_verification_exception& e) { : status = Status::NotAuthorized( : Substitute("Verification failed, error: $0", e.what())); : } catch (const std::exception& e) { : status = Status::NotAuthorized( : Substitute("Verification failed, error: $0", e.what())); : } : return status; nit: assigning status and continuing further hints that there is some non-trivial activity before calling 'return status' but de-facto there is nothing. Maybe, change this pattern here and elsewhere in this file to be simply try { ... } catch (...) { return Status::NotAuthorized(...); } catch (...) { return Status::NotAuthorized(...); } return Status::OK(); ? http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@381 PS7, Line 381: status = Status::NotAuthorized(Substitute("RSA error: $0", e.what())); : RETURN_NOT_OK(status); It seems that eventually became an oxymoron: RETURN_NOT_OK(status) will always return right away due to the assignment at the line above. Maybe, change these two lines here and below to return Status::NotAuthorized(...); ? http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@526 PS7, Line 526: BIGNUM* x = 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 instead and get rid of BN_free() at line 554 http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@534 PS7, Line 534: EC_KEY* ecKey = EC_KEY_new_by_curve_name(eccgrp); nit: could use c_unique_ptr<EC_KEY> ecKey { EC_KEY_new_by_curve_name(eccgrp), &EC_KEY_free } instead and get rid of EC_KEY_free(ecKey) at line 553. http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@538 PS7, Line 538: unsigned char desc[1024]; : memset(desc, 0, 1024); Is these two lines result in the same as unsigned char desc[1024] = { 0 }; ? http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@540 PS7, Line 540: bio = BIO_new(BIO_s_mem()); nit: could use 'auto bio = ssl_make_unique(BIO_new(BIO_s_mem()))' and get rid of line 552. http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@542 PS7, Line 542: if (BIO_read(bio, desc, 1024) > 0) { : pub_key = reinterpret_cast<char*>(desc); Per https://www.openssl.org/docs/man1.1.1/man3/BIO_read.html I cannot see that the result of BIO_read() is always NULL-terminated, so if there is 1024 or more bytes to read in the 'bio', I suspect there might be a buffer overrun when doing "pub_key = reinterpret_cast<char*>(desc);" I'd suggest reading one less byte than the size of the array, assuming the array is zeroed. http://gerrit.cloudera.org:8080/#/c/18468/7/src/kudu/util/jwt-util.cc@902 PS7, Line 902: // Get value of custom claim 'username' from the token payload. : if (decoded_jwt.has_payload_claim(jwt_custom_claim_username)) { : // Assume the claim data type of 'username' is string. : username.assign( : decoded_jwt.get_payload_claim(jwt_custom_claim_username) : .to_json() : .to_str()); : if (username.empty()) { : status = Status::NotAuthorized( : Substitute("Claim '$0' is empty", jwt_custom_claim_username)); : } : } else { : status = Status::NotAuthorized( : Substitute("Claim '$0' was not present", jwt_custom_claim_username)); : } nit: this could be a bit more readable if (!decoded_jwt.has_payload_claim(jwt_custom_claim_username)) { return Status::NotAuthorized(...); } username = decoded_jwt.get_payload_claim(jwt_custom_claim_username).to_json().to_str(); if (username.empty()) { return Status::NotAuthorized(); } -- 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: 7 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: Tue, 08 Nov 2022 03:20:07 +0000 Gerrit-HasComments: Yes
