Michael Ho 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 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 choose some TLSv1.2 ciphers too if the platform supports it ? 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 function and use it in the tests below and TEST_F(RpcMgrTest, MultipleServices) ? In this way, we can exercise more than a single ping RPC in the tests below when TLS is enabled. http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr-test.cc@309 PS4, Line 309: nit: blank line 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 shared by multiple tests. 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 what this function tests for. 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 function. Same for other functions. 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. 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 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. 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 { } 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 http://gerrit.cloudera.org:8080/#/c/8439/4/be/src/rpc/rpc-mgr.cc@154 PS4, Line 154: nit: blank line 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. 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) -- 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 08:58:28 +0000 Gerrit-HasComments: Yes
