Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19156 )
Change subject: jwt: Additional test ...................................................................... Patch Set 6: (19 comments) http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc File src/kudu/integration-tests/security-itest.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@515 PS6, Line 515: TEST_F(SecurityITest, TestJwtMiniCluster) { Since this scenario runs over 3 seconds now, consider adding SKIP_IF_SLOW_NOT_ALLOWED() http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@540 PS6, Line 540: if (delay_ms != 0) { : SleepFor(MonoDelta::FromMilliseconds(delay_ms)); : } nit: could use SleepFor(MonoDelta::FromMilliseconds(0)) to simplify the code since scheduler anomalies could pop up elsewhere anyways. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/integration-tests/security-itest.cc@572 PS6, Line 572: ASSERT_FALSE(s.ok()) << s.ToString(); Here and elsewhere in this test scenario: this sort of non-OK assertion should be on a specific status type since KuduClientBuilder is a part of the public API, at least. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/mini-cluster/external_mini_cluster.cc@273 PS6, Line 273: std::make_shared<SimpleJwtVerifier>(); Is this still needed? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: PS6: It's better to keep non-related changes out of the patch. If you want to update the comments w.r.t. KUDU-1921, it's better to do so in a separate patch (BTW, there are other occurrences KUDU-1921 related TODO in the code). http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc File src/kudu/rpc/negotiation-test.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@127 PS6, Line 127: bool jwt; Add a comment similar to the comment for 'token' above to clarify on the semantics for the server and the client side negotiators. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@242 PS6, Line 242: string jwks_file_name = "keys.jwks"; : string jwt_test_dir = GetTestPath("jwt"); : string jwt_data = kudu::CreateTestJWT(true); nit here and elsewhere: could these be marked as constant? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@247 PS6, Line 247: true nit for better readability: add a comment for parameter name to clarify on its semantics for readers; alternatively, consider introducing enum with meaningful names and use it here and elsewhere instead of a boolean parameter http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@995 PS6, Line 995: PkiConfig::EXTERNALLY_SIGNED, nit: looks a bit strange that the indent has changed only for this block, but not for other blocks updated above http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1079 PS6, Line 1079: nit: extra spaces? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1113 PS6, Line 1113: Status::OK(), : Status::OK(), : AuthenticationType::JWT, Does this look like a valid result with an empty list of SASL mechanisms to use during negotiation? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1200 PS6, Line 1200: nit: extra empty lines http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation-test.cc@1202 PS6, Line 1202: )); I think it's necessary to add more test sub-scenarios, at least to test how the negotiation fires when * the client and the server differ in JWT capabilities * TLS encryption is optional at least at one of the sides http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/negotiation.cc@178 PS6, Line 178: const auto jwt = (conn->credentials_policy() == CredentialsPolicy::PRIMARY_CREDENTIALS) : ? std::nullopt : messenger->jwt(); IIUC, JWT is this context represents primary credentials, not secondary: JWT is a substitute for Kerberos authn there. Secondary credentials are the credentials created and dispensed by Kudu itself, not something from independent authentication system. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/server_negotiation.cc File src/kudu/rpc/server_negotiation.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/rpc/server_negotiation.cc@208 PS6, Line 208: and jwt_verifier So, where is that CHECK then? I don't see it in the code below. And since you are touching this: it seems this comment isn't exactly relevant in the isolated scope of this ServerNegotiation::Negotiate() method without PreflightCheckGSSAPI around. Maybe, update this comment stating that those should be non-null since the connection negotiation process needs them from now on? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/server/server_base.cc@703 PS6, Line 703: = std::make_shared<SimpleJwtVerifier>(); Is this still needed? http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/CMakeLists.txt@369 PS6, Line 369: set (KUDU_JWT_UTIL_SRCS jwt-util.cc jwt_test_certs.cc) : add_library(kudu_jwt_util ${KUDU_JWT_UTIL_SRCS}) What drives this change? I don't think we want to have test certs linked into the code of kudu-master and kudu-tserver. If keeping jwt_test_certs.cc in test-only mini_oidc library isn't a good fit, consider separating them into their own library and link it instead of mini_oidc where needed. http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.h File src/kudu/util/jwt_test_certs.h: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.h@22 PS6, Line 22: namespace kudu { style nit: add an empty line before 'namespace kudu' http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.cc File src/kudu/util/jwt_test_certs.cc: http://gerrit.cloudera.org:8080/#/c/19156/6/src/kudu/util/jwt_test_certs.cc@373 PS6, Line 373: RETURN_NOT_OK(WriteStringToFile(Env::Default(), jwks_content, : JoinPathSegments(dir, file_name))); : return Status::OK(); nit: could be shorten into return WriteStringToFile(...); -- To view, visit http://gerrit.cloudera.org:8080/19156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1977c80b70fd9628ac800671f6cf16e9fa96c0f0 Gerrit-Change-Number: 19156 Gerrit-PatchSet: 6 Gerrit-Owner: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Andrew Wong <anj...@gmail.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Zoltan Chovan <zcho...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Mar 2023 00:59:45 +0000 Gerrit-HasComments: Yes
