[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has submitted this change and it was merged. Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Reviewed-on: http://gerrit.cloudera.org:8080/5015 Tested-by: Kudu Jenkins Reviewed-by: Dan BurkertReviewed-by: Alexey Serbin --- M src/kudu/security/security-test-util.h M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h 10 files changed, 246 insertions(+), 22 deletions(-) Approvals: Dan Burkert: Looks good to me, approved Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Alexey Serbin has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] webserver: improve SSL certificate handling
Dan Burkert has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] webserver: improve SSL certificate handling
Dan Burkert has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/7/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 423: CHECK_OK(CreateSSLServerCert(server_cert_path)); > yea, will worry about that when working on these TODOs, right? ie this is j I meant I'd moved CreateSSLServerCert from rpc/ to security/ because I needed it in a security test. If the long term solution is to use security/test/test_cert.h and remove security/security-test-util.h, that sounds good to me. -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/7/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 423: CHECK_OK(CreateSSLServerCert(server_cert_path)); > Unfortunately I completely reconfigured this as well as part of my recent p yea, will worry about that when working on these TODOs, right? ie this is just an FYI and not a comment on this patch? -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Dan Burkert has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/7/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 423: CHECK_OK(CreateSSLServerCert(server_cert_path)); Unfortunately I completely reconfigured this as well as part of my recent patch series, so you are going to have to do some manual rebasing. I'm not attached to the file I introduced (security-test-util.h), so you can probably just remove it and go with your change. -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5015 to look at the new patch set (#7). Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/test/test_certs.cc M src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h 10 files changed, 244 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/7 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 5: (2 comments) Done those. Next revision is a rebase http://gerrit.cloudera.org:8080/#/c/5015/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS5, Line 56: .PEM > I might miss something, but the sentence meaning seems to be '... certifica Done PS5, Line 63: .PEM > nit: I didn't notice in the first pass, but consider dropping. Done -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5015 to look at the new patch set (#6). Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/CMakeLists.txt A src/kudu/security/test/test_certs.cc A src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h 11 files changed, 290 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/6 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Alexey Serbin has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS5, Line 56: .PEM > hrm.. isn't it a file extension? I might miss something, but the sentence meaning seems to be '... certificate file, in some format.' So, I thought PEM in this context is about format of the data stored in the file, not about the extension. It's just a nit, feel free to ignore. -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS5, Line 56: .PEM > nit: it's just PEM -- no preceding dot. Otherwise it looks like a file ext hrm.. isn't it a file extension? -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Alexey Serbin has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/5015/5/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS5, Line 56: .PEM nit: it's just PEM -- no preceding dot. Otherwise it looks like a file extension. PS5, Line 63: .PEM nit: I didn't notice in the first pass, but consider dropping. -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5015 to look at the new patch set (#5). Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. In order to test this, I reconfigured our thirdparty curl build to include HTTPS support. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/CMakeLists.txt A src/kudu/security/test/test_certs.cc A src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h 11 files changed, 292 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/5 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/5015/3/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS3, Line 165: asswo > nit: may be ? Done http://gerrit.cloudera.org:8080/#/c/5015/3/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS3, Line 56: debug > What is 'debug webserver's SSL certificate file'? all of these flags refer to the webserver as the "debug webserver" which I agree is kind of weird, but let's change them for all flags at the same time. PS3, Line 59: --ssl_server_certificate > --webserver_certificate_file ? Done PS3, Line 60: this option must be set as well. > I might miss it, but I didn't see this was enforced. it's sort of implicitly enforced -- I think squeasel will have an error http://gerrit.cloudera.org:8080/#/c/5015/4/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: PS4, Line 71: CURLOPT_SSL_VERIFYPEER > This is just to verify the cert chain, right? What about verifying the hos I don't think it makes sense in the context of tests, since we don't really know our host names, etc, and this is just test utility code. http://gerrit.cloudera.org:8080/#/c/5015/3/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 489: # Configure for a very minimal install - basically only HTTP(S), since we only > Just passing through, but if you rebase, you won't need any changes to this Done -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Alexey Serbin has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/5015/3/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS3, Line 165: asswo nit: may be ? http://gerrit.cloudera.org:8080/#/c/5015/3/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: PS3, Line 56: debug What is 'debug webserver's SSL certificate file'? PS3, Line 56: .pem PEM PS3, Line 59: --ssl_server_certificate --webserver_certificate_file ? PS3, Line 60: this option must be set as well. I might miss it, but I didn't see this was enforced. PS3, Line 60: --ssl_server_certificate --webserver_certificate_file http://gerrit.cloudera.org:8080/#/c/5015/4/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: PS4, Line 71: CURLOPT_SSL_VERIFYPEER This is just to verify the cert chain, right? What about verifying the hostname of the server (cert subj/alt. subj)? Does it make sense to enable to set CURLOPT_SSL_VERIFYHOST if CURLOPT_SSL_VERIFYPEER is set to 1? -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5015 to look at the new patch set (#4). Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. In order to test this, I reconfigured our thirdparty curl build to include HTTPS support. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/CMakeLists.txt A src/kudu/security/test/test_certs.cc A src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h 11 files changed, 289 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/4 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Adar Dembo has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/5015/3/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 489: build_curl() { Just passing through, but if you rebase, you won't need any changes to this file; Mike already removed --without-ssl. -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5015 to look at the new patch set (#3). Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. In order to test this, I reconfigured our thirdparty curl build to include HTTPS support. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/CMakeLists.txt A src/kudu/security/test/test_certs.cc A src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h M thirdparty/build-definitions.sh 12 files changed, 292 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/3 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] webserver: improve SSL certificate handling
Todd Lipcon has posted comments on this change. Change subject: webserver: improve SSL certificate handling .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/5015/2/src/kudu/security/test/test_certs.h File src/kudu/security/test/test_certs.h: Line 28: Status CreateTestSSLCerts(const std::string& dir, > Could you use instead or replace usage of CreateSSLServerCert/CreateSSLPriv This patch adds a TODO there -- the issue is that these certs use a password, but the RPC TLS implementation doesn't yet support using a password. I'll work on that next. http://gerrit.cloudera.org:8080/#/c/5015/2/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: Line 158: Status s = Subprocess::Call(argv, "" /* stdin */, _password, ); > Adar and I traced through the other day and found that we never shelled out Yea, given it's close to startup I'd say it's safe. It's also the accepted way for people to tie in key/password management stuff in all of the other Hadoop ecosystem components, so we don't have much of a choice. (given this is close to startup I don't think it's worth preforking a separate "forker" process and communicating by a pipe or anything) Line 166: } else { > Is this else attached to the correct if? It would make more sense to me on Done PS2, Line 197: SimpleItoa > consider using std::to_string Done http://gerrit.cloudera.org:8080/#/c/5015/2/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: Line 72: RETURN_NOT_OK(TranslateError(curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYPEER, 0))); > This might be simpler without the if block, and setting the value to veify_ Done -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] webserver: improve SSL certificate handling
Hello Dan Burkert, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/5015 to review the following change. Change subject: webserver: improve SSL certificate handling .. webserver: improve SSL certificate handling * Allows users to specify a separate location for PEM private-key file using --webserver_private_key_file (previously required private-key and cert. to be in same file). * Allows users to specify a shell command to run to get the password for the webserver's private-key file using --webserver_private_key_password_cmd The separate configuration of cert and key is apparently more commonly used, so this simplifies deployment. The use of a script to provide the password for the private key makes it easier to integrate with external credential providers. In order to test this, I reconfigured our thirdparty curl build to include HTTPS support. This mirrors the change made in Impala by IMPALA-2051 (8bbe45393c7). Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 --- M src/kudu/rpc/rpc-test-base.h M src/kudu/security/CMakeLists.txt A src/kudu/security/test/test_certs.cc A src/kudu/security/test/test_certs.h M src/kudu/server/CMakeLists.txt M src/kudu/server/webserver-test.cc M src/kudu/server/webserver.cc M src/kudu/server/webserver_options.cc M src/kudu/server/webserver_options.h M src/kudu/util/curl_util.cc M src/kudu/util/curl_util.h M thirdparty/build-definitions.sh 12 files changed, 295 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/1 -- To view, visit http://gerrit.cloudera.org:8080/5015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I4b508cebbe6f31556e6d5a5fba5e5e9fb44cf1b9 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd LipconGerrit-Reviewer: Dan Burkert