Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18472 )
Change subject: jwt: determine discovery endpoint from token ...................................................................... Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util-test.cc File src/kudu/util/jwt-util-test.cc: http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util-test.cc@1071 PS9, Line 1071: > Does it make sense to add a test that calls PerAccountKeyBasedJwtVerifier:: Yes, with multiple concurrent clients it would make sense to cover this as well. I'll add a new test in the next patchset. http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc File src/kudu/util/jwt-util.cc: http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc@969 PS9, Line 969: kudu::MonoDelta::FromSeconds(static_cast<int64_t>(FLAGS_jwks_pulling_timeout_s))); > Could this be just Done http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc@974 PS9, Line 974: : if (dst.size() <= 0) { : return Status::RuntimeError("Discovery Endpoint returned an empty document"); : } : : dst.push_back('\0'); : Document endpoint_doc; : endpoint_doc.Parse(reinterpret_cast<char*>(dst.data())); : #define RETURN_INVALID_IF(stmt, msg) \ : if (PREDICT_FALSE(stmt)) { \ : return Status::InvalidArgument(msg); \ : } : RETURN_INVALID_IF(endpoint_doc.HasParseError(), GetParseError_En(endpoint_doc.GetParseError())); : RETURN_INVALID_IF(!endpoint_doc.IsObject(), "root element must be a JSON Object"); : auto jwks_uri_member = endpoint_doc.FindMember("jwks_uri"); : RETURN_INVALID_IF(jwks_uri_member == endpoint_doc.MemberEnd(), "jwks_uri is required"); : RETURN_INVALID_IF(!jwks_uri_member->value.IsString(), "jwks_uri must be a string"); : j > readability nit: it might be a bit more readable when rewritten like dst is a faststring, so it doesn't have an empty() method, but I'll switch the branches http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc@1001 PS9, Line 1001: *helper = new_helper.get(); > A few questions here. The PerAccountKeyBasedJwtVerifier::JWTHelperForToken() is only called from the same class' VerifyToken method. Which is only invoked when a server/client negotiation is taking place. So if multiple clients are connecting at the same time it is possible that this is going to be invoked by multiple threads at the same time if I understand correctly. I've added a lock to this method. -- To view, visit http://gerrit.cloudera.org:8080/18472 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I970bcc6d894c0206160196418d549b71c35ac973 Gerrit-Change-Number: 18472 Gerrit-PatchSet: 10 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: Wed, 11 Jan 2023 17:38:11 +0000 Gerrit-HasComments: Yes
