Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8439 )
Change subject: IMPALA-5054: [SECURITY] Enable KRPC w/ TLS in Impala ...................................................................... Patch Set 4: (15 comments) http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@78 PS4, Line 78: string IMPALA_HOME(getenv("IMPALA_HOME")); > const static Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@90 PS4, Line 90: TLSv1.0 compatible ciphers > Are there ways to detect the TLS version supported on the platform and choo There actually is a way now, but the patch that introduces some basic utils for it is still in review here: https://gerrit.cloudera.org/#/c/9060/ I can add tests for TLSv1.2 as a follow on patch. http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@268 PS4, Line 268: RunTlsTestTemplate > What do you think about factoring line 331 to line 371 below into a single Done. I removed RunTlsTestTemplate() and made every test use the new function RunMultipleServicesTestTemplate() http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@309 PS4, Line 309: > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@331 PS4, Line 331: // Test that a service can be started, and will respond to requests. : unique_ptr<ServiceIf> ping_impl( : new PingServiceImpl(tls_rpc_mgr.metric_entity(), tls_rpc_mgr.result_tracker())); : ASSERT_OK(tls_rpc_mgr.RegisterService(10, 10, move(ping_impl))); : : // Test that a second service, that verifies the RPC payload is not corrupted, : // can be started. : unique_ptr<ServiceIf> scan_mem_impl( : new ScanMemServiceImpl(tls_rpc_mgr.metric_entity(), tls_rpc_mgr.result_tracker())); : ASSERT_OK(tls_rpc_mgr.RegisterService(10, 10, move(scan_mem_impl))); : : FLAGS_num_acceptor_threads = 2; : FLAGS_num_reactor_threads = 10; : ASSERT_OK(tls_rpc_mgr.StartServices(tls_krpc_address)); : : unique_ptr<PingServiceProxy> ping_proxy; : ASSERT_OK(tls_rpc_mgr.GetProxy<PingServiceProxy>(tls_krpc_address, &ping_proxy)); : : unique_ptr<ScanMemServiceProxy> scan_mem_proxy; : ASSERT_OK(tls_rpc_mgr.GetProxy<ScanMemServiceProxy>(tls_krpc_address, &scan_mem_proxy)); : : RpcController controller; : srand(0); : // Randomly invoke either services to make sure a RpcMgr can host multiple : // services at the same time. : for (int i = 0; i < 100; ++i) { : controller.Reset(); : if (random() % 2 == 0) { : PingRequestPB request; : PingResponsePB response; : kudu::Status status = ping_proxy->Ping(request, &response, &controller); : ASSERT_TRUE(status.ok()); : ASSERT_EQ(response.int_response(), 42); : } else { : ScanMemRequestPB request; : ScanMemResponsePB response; : SetupScanMemRequest(&request, &controller); : kudu::Status status = scan_mem_proxy->ScanMem(request, &response, &controller); : ASSERT_TRUE(status.ok()); : } : } > Please see comments above on how this may be factored into a function to be Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@378 PS4, Line 378: // Misconfigure TLS with bad CA certificate. > nit: may as well move it to before line 377 as block comment to describe wh Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@392 PS4, Line 392: : // Misconfigure TLS with a bad password command for the passowrd protected private key. > nit: may as well move it to before line 391 as block comment for this funct Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@412 PS4, Line 412: // Configure TLS with a password protected private key and the correct password for it. > nit: same here. Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@430 PS4, Line 430: // Misconfigure TLS with a bad cipher. > nit: same here Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@446 PS4, Line 446: // Enable TLS with a TLSv1 compatible cipher list. > nit: same here. Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.h@100 PS4, Line 100: { : } > nit: one line { } Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.cc File be/src/rpc/rpc-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.cc@93 PS4, Line 93: > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.cc@154 PS4, Line 154: > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/runtime/exec-env.cc@185 PS4, Line 185: > nit: blank line not needed. Done http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/util/openssl-util.cc@57 PS4, Line 57: certificate > certificate (FLAGS_ssl_client_ca_certificate) Done -- To view, visit http://gerrit.cloudera.org:8080/8439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9a14a44fdea9ab668f3714eb69fdb188bce38f5a Gerrit-Change-Number: 8439 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Fri, 19 Jan 2018 22:15:25 +0000 Gerrit-HasComments: Yes