You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/09 01:12:13 UTC

[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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert 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/CreateSSLPrivateKey in rpc-test-base.h with this?


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 */, &key_password, &stderr);
Adar and I traced through the other day and found that we never shelled out in production Kudu code - is it OK to do so here?  I assume it's not too bad since it's close to startup?


Line 166:     } else {
Is this else attached to the correct if?  It would make more sense to me on the inner if.


PS2, Line 197: SimpleItoa
consider using std::to_string


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_peer_ instead of 0.


-- 
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 */, &key_password, &stderr);
> 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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 (#2).

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, 296 insertions(+), 23 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/2
-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Burkert <da...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 (#8).

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/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(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/5015/8
-- 
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: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] webserver: improve SSL certificate handling

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes