Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18472 )
Change subject: jwt: determine discovery endpoint from token ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/server/server_base.cc@261 PS9, Line 261: DEFINE_string(jwks_discovery_endpoint_base, "", I guess it make sense to add the 'experimental' tag for this flag as well. 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::VerifyToken() concurrently from multiple threads? 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::FromMilliseconds(static_cast<int64_t>(FLAGS_jwks_pulling_timeout_s * 1000))) Could this be just MonoDelta::FromSeconds(FLAGS_jwks_pulling_timeout_s) ? http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc@974 PS9, Line 974: if (dst.size() > 0) { : 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"); : jwks_uri = string(jwks_uri_member->value.GetString()); : } else { : return Status::RuntimeError("Discovery Endpoint returned an empty document"); : } readability nit: it might be a bit more readable when rewritten like if (dst.empty()) { return Status::RuntimeError("Discovery Endpoint returned an empty document"; } ... http://gerrit.cloudera.org:8080/#/c/18472/9/src/kudu/util/jwt-util.cc@1001 PS9, Line 1001: EmplaceOrDie(&jwt_by_account_id_, account_id, std::move(new_helper)); A few questions here. 1) Could PerAccountKeyBasedJwtVerifier::JWTHelperForToken() be called by multiple threads concurrently? If yes, how do we protect against concurrent access to the jwt_by_account_id_ map? 2) Instead of crashing here when an element is in the map already, could we use LookupOrEmplace() instead and return the result helper regardless it was present or not? -- 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: 9 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: Mon, 19 Dec 2022 19:17:35 +0000 Gerrit-HasComments: Yes
