You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/01/11 01:19:37 UTC
[kudu-CR] [TLS certs management] initial commit
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5671
Change subject: [TLS certs management] initial commit
......................................................................
[TLS certs management] initial commit
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are coming in a separate changelist.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/crypto/cert_management.cc
A src/kudu/security/crypto/cert_management.h
A src/kudu/security/crypto/crypto_common.cc
A src/kudu/security/crypto/crypto_common.h
A src/kudu/security/crypto/crypto_engine.cc
A src/kudu/security/crypto/crypto_engine.h
7 files changed, 1,297 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/1
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#7).
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,658 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/7
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to
: // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames
: // and ips should have been fields of the Config structure.
> hrm, you removed the comment, but now it doesn't really make sense why it's
OK, here is the issue: non-devetoolset gcc compiler used by Jenkins slaves does not support non-trivial designated initializers:
20:22:58 /home/jenkins-slave/workspace/kudu-0/src/kudu/security/test/cert_management-test.cc:94:5: sorry, unimplemented: non-trivial designated initializers not supported
Yes, that's C99 feature and C++11 standard does not have this mentioned, and it isn't valid C++11 syntax. I'll update this.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5671/6/src/kudu/security/test/cert_management-test.cc
File src/kudu/security/test/cert_management-test.cc:
Line 23: #include <vector>
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] groundwork for cert signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#8).
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,661 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/8
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5671/4/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:
PS4, Line 632:
:
:
per comment on the earlier revision, can you make this look less like a bug by doing something like:
// X509v3 is, surprisingly, indicated by the value '2'.
const int kX509V3 = 2;
CERT_RET_NOT_OK(X509_set_version(ret, kX509V3));
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to
: // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames
: // and ips should have been fields of the Config structure.
> I had some issues compiling the described alternative on ve0518.halxg.cloud
hrm, you removed the comment, but now it doesn't really make sense why it's a separate parameter.
Keep in mind that ve0518 is an el6 box, and so you need to be building with devtoolset. Were you trying to build this with te standard system compiler? Was the issue with the aggregate initialization you are doing elsewhere? iirc that isn't actually valid C++ syntax, so maybe we should just fix it there? Still seems weird that these are separate from the config.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS certs management] initial commit
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#3).
Change subject: [TLS certs management] initial commit
......................................................................
[TLS certs management] initial commit
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are coming in a separate changelist.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
4 files changed, 911 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/3
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#4).
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,655 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/4
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Re: [kudu-CR] [TLS certs management] initial commit
Posted by Alexey Serbin <as...@cloudera.com>.
Right; that version were posted accidentally -- I actually only posted the
update for the next patch (unit tests), not going to post the update for
the first one because it was not ready yet. But gerrit updated this one as
it sensed the base has been changed as well.
Please hold on reviewing this version -- I'll post it later, most likely
tomorrow in the morning.
Thanks!
/Alexey
On Thu, Jan 12, 2017 at 9:06 PM, Dan Burkert (Code Review) <
gerrit@cloudera.org> wrote:
> Dan Burkert has posted comments on this change.
>
> Change subject: [TLS certs management] initial commit
> ......................................................................
>
>
> Patch Set 2:
>
> crypto_engine and crypto_common are no longer needed, right?
>
> --
> To view, visit http://gerrit.cloudera.org:8080/5671
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: comment
> Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
> Gerrit-PatchSet: 2
> Gerrit-Project: kudu
> Gerrit-Branch: master
> Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
> Gerrit-Reviewer: Dan Burkert <da...@apache.org>
> Gerrit-Reviewer: Kudu Jenkins
> Gerrit-Reviewer: Mike Percy <mp...@apache.org>
> Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
> Gerrit-HasComments: No
>
[kudu-CR] [TLS certs management] initial commit
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [TLS certs management] initial commit
......................................................................
Patch Set 2:
crypto_engine and crypto_common are no longer needed, right?
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] groundwork for cert signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Reviewed-on: http://gerrit.cloudera.org:8080/5671
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,661 insertions(+), 4 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5671/4/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:
PS4, Line 632:
:
:
> per comment on the earlier revision, can you make this look less like a bug
Good idea. I didn't find the proper constant in OpenSSL header files and forgot about this.
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc
File src/kudu/security/crypto/cert_management.cc:
Line 113: "BIO_get_mem_ptr() failed");
> I generally feel like if the only way you'd hit this is a memory allocation
ok, this sounds reasonable -- thank you for the clarification. Will update accordingly.
Line 315: "Error setting parameters for RSA key generation");
> See above -- yea, I think crashing on malloc failure is appropriate.
Done
Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR");
> yep (see above)
Done
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to
: // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames
: // and ips should have been fields of the Config structure.
> hrm, you removed the comment, but now it doesn't really make sense why it's
Oh! Thanks for pointing at this. It seems I removed the comment but I missed the crucial part on clarifying on the issue.
Yes, it's fine if compiling with the clang++ from the devtoolset. IIRC, there was an issue while I was trying to do that C++11-style structure initialization if compiling with the default system compiler.
Should be OK now -- I put those fields into the configuration structure. Let's see how it behaves on Jenkins build machines.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] groundwork for cert signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc
File src/kudu/security/crypto/cert_management.cc:
Line 113: "BIO_get_mem_ptr() failed");
> Yes, but why is it better than just returning an error?
I generally feel like if the only way you'd hit this is a memory allocation failure, then it's probably not worth worrying about trying to do any handling/recovery. eg returning a bad status itself will have to malloc and that'll probably fail, or else something else will fail shortly thereafter. In general in the way people typically configure Linux, malloc never fails, it just overcommits and later kills you from the OOM killer, so trying to gracefully handle these paths just means that we'll have code paths that never get executed.
Line 315: "Error setting parameters for RSA key generation");
> Yes, it's possible to replace it with CHECK, but I would prefer to keep it
See above -- yea, I think crashing on malloc failure is appropriate.
Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR");
> That field might be null in case of transient memory allocation failure. D
yep (see above)
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#5).
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,656 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/5
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [TLS certs management] initial commit
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [TLS certs management] initial commit
......................................................................
Patch Set 1:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc
File src/kudu/security/crypto/cert_management.cc:
PS1, Line 55: !(call)
can you check (call) == nullptr instead?
PS1, Line 80: FromBIO(bio.get(), format),
: Substitute("$0: unable to load data key from file: $1",
: fpath, GetErrors()
it seems odd to be calling GetErrors() here instead of FromBIO itself being the one to call it. In other words, I expect to only see GetErrors directly after a failed direct call of the SSL API, and anywhere we call our own wrapper, the wrapper should be responsible for converting the errors to Status. Looking at the implementations, it seems like they do already fetch the errors so this is redundant.
PS1, Line 96: U
here and elsewhere, we've been trying to start status messages with lower case, so they look better when chained to each other.
PS1, Line 102: if (PREDICT_FALSE(!data)) {
: static const string kErrMsg = "Null placeholder for output data";
: LOG(DFATAL) << kErrMsg;
: return Status::InvalidArgument(kErrMsg);
: }
this would just be a programmer error, no? i think CHECK(data) is fine, or even just let it crash
PS1, Line 109: Error writing data to memory buffer:
this is an odd looking error, because it's impossible to have an error writing to memory. The real error would be if for some reason the serialization failed, right? maybe just "error serializing" or something?
Line 113: "BIO_get_mem_ptr() failed");
can we make this a CHECK as well?
Line 123: if (PREDICT_FALSE(!bio)) {
see above
Line 149: if (PREDICT_FALSE(!elem_)) {
same (just CHECK)
Line 163: "Error exporing private key in DER format");
exporting
PS1, Line 304: uint16_t num_bits,
change this to an int, and do some basic bounds checking, like CHECK_GE(num_bits, 512); CHECK_LE(num_bits, 4096); or something
PS1, Line 305: if (!ret) {
: static const string kErrMsg = "Null placeholder for the result";
: LOG(DFATAL) << kErrMsg;
: return Status::InvalidArgument(kErrMsg);
: }
:
CHECK(ret)
Line 315: "Error setting parameters for RSA key generation");
this should be able to be a CHECK too
PS1, Line 342: if (!Initialized()) {
: return Status::IllegalState("Not initialized");
: }
CHECK
Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR");
this should always succeed too, right?
PS1, Line 555: RETURN_NOT_OK(CryptoEngine::get()->Init());
: RETURN_NOT_OK(ca_cert_.FromFile(config_.ca_cert_path, PEM));
: RETURN_NOT_OK(ca_private_key_.FromFile(config_.ca_private_key_path, PEM));
: CERT_RET_NOT_OK(X509_check_private_key(ca_cert_.get(), ca_private_key_.get()),
: Substitute("$0, $1: CA certificate and private key "
: "do not match",
: config_.ca_cert_path, config_.ca_private_key_path));
this is somewhat temporary, right? in the real implementation we'll need to pass these in because they'll be stored in our tables, not in a file on disk, right?
PS1, Line 606: STACK_OF(X509_EXTENSION)* exts = X509_REQ_get_extensions(req);
: auto exts_cleanup = MakeScopedCleanup([&exts]() {
: sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);
: });
: for (size_t i = 0; i < sk_X509_EXTENSION_num(exts); ++i) {
: X509_EXTENSION* ext = sk_X509_EXTENSION_value(exts, i);
: ASN1_OBJECT* obj = X509_EXTENSION_get_object(ext);
: int32_t idx = X509_get_ext_by_OBJ(x, obj, -1);
: if (idx != -1) {
: // If extension exits, delete all extensions of same type.
: do {
: uni_ptr<X509_EXTENSION> tmpext(X509_get_ext(x, idx),
: X509_EXTENSION_free);
: X509_delete_ext(x, idx);
: idx = X509_get_ext_by_OBJ(x, obj, -1);
: } while (idx != -1);
: }
: CERT_RET_NOT_OK(X509_add_ext(x, ext, -1), "Error adding extension");
: }
this stuff is somewhat black-boxy to me. Can you add a comment pointing to the reference code for this? I'm guessing it is modeled after the usage in the utilities included with OpenSSL.
Perhaps a link to http://wiki.nikhef.nl/grid/How_to_handle_OpenSSL_and_not_get_hurt_and_what_does_that_library_call_really_do%3F for the free-related stuff?
PS1, Line 635: if (!req->req_info ||
: !req->req_info->pubkey ||
: !req->req_info->pubkey->public_key ||
: !req->req_info->pubkey->public_key->data) {
: return Status::RuntimeError("Corrupted CSR: no public key");
: }
isn't this redundant with the get_pubkey call below?
Line 680: CHECK_NOTNULL(ret);
this will give a warning in gcc I think. instead you can use this inline below like X509_set_issuer_name(CHECK_NOTNULL(ret))
PS1, Line 686: v3
: CERT_RET_NOT_OK(X509_set_version(ret, 2
says v3 but you set to 2?
Line 692: CERT_RET_IF_NULL(X509_gmtime_adj(X509_get_notAfter(ret), exp_seconds),
http://openssl.6102.n7.nabble.com/overflow-when-calling-X509-gmtime-adj-on-32-bit-systems-td43468.html recommends using the following instead:
X509_time_adj_ex(X509_get_notAfter(x), days, 0, NULL);
to avoid potential overflow issues.. but maybe it's not an issue given we only support 64-bit? have you tried large values of 'seconds'?
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
PS1, Line 37: DER = 0, // DER/ASN1 format (binary)
: PEM = 1, // PEM format (ASCII)
Why do we care about supporting both formats? Is there a use case? If not it seems like we should just pick one and be able to eliminate a bit of code.
As for which one, I think PEM is more typically used by users, and it would be nice to be able to print out the PEM format from debugging tools, etc. Of course it's slightly larger than DER but I don't think we transfer around keys frequently enough that it's a big issue, right?
(I may be wrong and missing some reason why it's good to support both)
Line 62: class Wrapper {
having a little trouble understanding what the advantage of this is over just using its underlying unique_ptr?
PS1, Line 65: elemFreeFunc_
nit: no camelCase in C++ style
PS1, Line 87: public BasicWrapper, public Wrapper<EVP_PKEY>
Multiple inheritance isn't allowed: https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance
why not have the Wrapper<EVP_PKEY> as a member? Or just directly hold the unique_ptr<>?
Or perhaps the 'BasicWrapper' should actually be something like a pure interface 'SSLSerializable', and the FromFile/FromString/ToString can be static methods?
PS1, Line 120: uint16_t
I think better to take 'int' and check input (https://google.github.io/styleguide/cppguide.html#Integer_Types)
Perhaps better to call this GeneratePrivateKey or GenerateRandomPrivateKey?
PS1, Line 144: method
it's a function rather than a method (it's not part of any class)
PS1, Line 144: The generated key will be needed later on
: // when using the X509 certificate generated from the CSR.
not sure this extra sentence adds much value -- I think it's sort of implicit in the knowledge of what a CSR does.
Or, if you think it's useful to have some context in the code here, maybe a class-level or file-level doc comment explaining typical usage, like:
// On the client:
// -----------------
// Key key;
// CHECK_OK(GenerateKey(2048, &key));
// CertSignRequest csr;
// CHECK_OK(csr_gen->GenerateRequest(key, &csr));
// ... transfer the CSR to a CA ... (example code how to serialize the CSR)
//
// On the CA:
// -------------
// // receive the CSR from the client
// <appropriate code to deserialize and sign the CSR>
// // send back the signed cert to the client
//
// On the client after signature received:
// ---------------
// <example what to do with the signed cert response>
PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to
: // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames
: // and ips should have been fields of the Config structure.
confused about this - we don't support gcc <4.8
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.cc
File src/kudu/security/crypto/crypto_common.cc:
PS1, Line 38: 256
curious why 256 - we have good evidence that no errors are longer than this?
Line 44: serr << " ";
a little funny that we end with a whitespace
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.h
File src/kudu/security/crypto/crypto_common.h:
PS1, Line 38: uni_ptr
this was confusing me a bit in the other files -- it wasn't obvious what this was relative to a normal unique_ptr.
Perhaps something like "c_unique_ptr' or 'unique_ptr_with_free' or something a bit more descriptive? or even ssl_unique_ptr
PS1, Line 41: GetErrors
please rename to something like GetSSLErrors() or SSLErrorString(). Even though it's in the crypto:: namespace, typical callers will also be in this namespace and it isn't super obvious where it's coming from otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS certs management] initial commit
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#2).
Change subject: [TLS certs management] initial commit
......................................................................
[TLS certs management] initial commit
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are coming in a separate changelist.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
A src/kudu/security/crypto/crypto_common.cc
A src/kudu/security/crypto/crypto_common.h
A src/kudu/security/crypto/crypto_engine.cc
A src/kudu/security/crypto/crypto_engine.h
7 files changed, 1,300 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/2
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5671
to look at the new patch set (#6).
Change subject: [security] groundwork for cert signing service
......................................................................
[security] groundwork for cert signing service
Added code for TLS certificate management in the scope of generating
root CA and Tablet Server certificates for Kudu. The code provides
wrappers for OpenSSL functions with Kudu-specific interface.
The code is compiled into a library which Kudu's X509 CSR server
and client should be linked with (master is going to be a CSR server
and tablet server is going to be a CSR client).
Unit tests are adedd as well.
Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_management.cc
A src/kudu/security/ca/cert_management.h
M src/kudu/security/openssl_util.cc
A src/kudu/security/test/cert_management-test.cc
A src/kudu/security/test/test_certs.cc
A src/kudu/security/test/test_certs.h
7 files changed, 1,658 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/5671/6
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] groundwork for cert signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] groundwork for cert signing service
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5671/5/src/kudu/security/test/cert_management-test.cc
File src/kudu/security/test/cert_management-test.cc:
Line 89: .hostnames = hostnames,
> warning: parameter 'hostnames' is passed by value and only copied once; con
Done
Line 90: .ips = ips,
> warning: parameter 'ips' is passed by value and only copied once; consider
Done
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS certs management] initial commit
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS certs management] initial commit
......................................................................
Patch Set 1:
(33 comments)
Thank you for the review. I'll post a new version soon.
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.cc
File src/kudu/security/crypto/cert_management.cc:
PS1, Line 55: !(call)
> can you check (call) == nullptr instead?
Done
PS1, Line 80: FromBIO(bio.get(), format),
: Substitute("$0: unable to load data key from file: $1",
: fpath, GetErrors()
> it seems odd to be calling GetErrors() here instead of FromBIO itself being
Good catch! Indeed, that's redundant. Will update.
PS1, Line 96: U
> here and elsewhere, we've been trying to start status messages with lower c
Ha, that's fun: some time ago I tried to do that (since it's the policy in Go and easier to type as well), but my Kudu reviewers told me otherwise.
It's time do to switch to other convention. :) Will update.
PS1, Line 102: if (PREDICT_FALSE(!data)) {
: static const string kErrMsg = "Null placeholder for output data";
: LOG(DFATAL) << kErrMsg;
: return Status::InvalidArgument(kErrMsg);
: }
> this would just be a programmer error, no? i think CHECK(data) is fine, or
The main premise was to avoid crash in production given the fact we cannot guarantee our tests give 100% path coverage. ok, it seems the consensus is that we prefer the brevity of the code to such defensive techniques.
PS1, Line 109: Error writing data to memory buffer:
> this is an odd looking error, because it's impossible to have an error writ
Done
Line 113: "BIO_get_mem_ptr() failed");
> can we make this a CHECK as well?
Yes, but why is it better than just returning an error?
Line 123: if (PREDICT_FALSE(!bio)) {
> see above
Done
Line 149: if (PREDICT_FALSE(!elem_)) {
> same (just CHECK)
Done
Line 163: "Error exporing private key in DER format");
> exporting
Done
PS1, Line 304: uint16_t num_bits,
> change this to an int, and do some basic bounds checking, like CHECK_GE(num
I'll replace the type with 'int'. The OpenSSL implementation is supposed to return an error for unsupported key length, so I think check for x < num_bits < y is redundant. There are a lot more non-supported key lengths even in interval of [512, 4096] -- let OpenSSL handle that since it knows best.
PS1, Line 305: if (!ret) {
: static const string kErrMsg = "Null placeholder for the result";
: LOG(DFATAL) << kErrMsg;
: return Status::InvalidArgument(kErrMsg);
: }
:
> CHECK(ret)
Done
Line 315: "Error setting parameters for RSA key generation");
> this should be able to be a CHECK too
Yes, it's possible to replace it with CHECK, but I would prefer to keep it as is: BN_set_word() could return error if OPENSSL_malloc() could not allocate memory, which might be a transient error. Do you think it's better to crash in that case?
PS1, Line 342: if (!Initialized()) {
: return Status::IllegalState("Not initialized");
: }
> CHECK
Done
Line 349: CERT_RET_IF_NULL(name, "Null subject name sub-structure in X509 CSR");
> this should always succeed too, right?
That field might be null in case of transient memory allocation failure. Do you think it's better to crash in that case?
PS1, Line 555: RETURN_NOT_OK(CryptoEngine::get()->Init());
: RETURN_NOT_OK(ca_cert_.FromFile(config_.ca_cert_path, PEM));
: RETURN_NOT_OK(ca_private_key_.FromFile(config_.ca_private_key_path, PEM));
: CERT_RET_NOT_OK(X509_check_private_key(ca_cert_.get(), ca_private_key_.get()),
: Substitute("$0, $1: CA certificate and private key "
: "do not match",
: config_.ca_cert_path, config_.ca_private_key_path));
> this is somewhat temporary, right? in the real implementation we'll need to
Sure -- that's the intermediate state before the functionality to load that data from the system tables is implemented.
As for the external PKI mode, this code should not be used at all, right.
PS1, Line 606: STACK_OF(X509_EXTENSION)* exts = X509_REQ_get_extensions(req);
: auto exts_cleanup = MakeScopedCleanup([&exts]() {
: sk_X509_EXTENSION_pop_free(exts, X509_EXTENSION_free);
: });
: for (size_t i = 0; i < sk_X509_EXTENSION_num(exts); ++i) {
: X509_EXTENSION* ext = sk_X509_EXTENSION_value(exts, i);
: ASN1_OBJECT* obj = X509_EXTENSION_get_object(ext);
: int32_t idx = X509_get_ext_by_OBJ(x, obj, -1);
: if (idx != -1) {
: // If extension exits, delete all extensions of same type.
: do {
: uni_ptr<X509_EXTENSION> tmpext(X509_get_ext(x, idx),
: X509_EXTENSION_free);
: X509_delete_ext(x, idx);
: idx = X509_get_ext_by_OBJ(x, obj, -1);
: } while (idx != -1);
: }
: CERT_RET_NOT_OK(X509_add_ext(x, ext, -1), "Error adding extension");
: }
> this stuff is somewhat black-boxy to me. Can you add a comment pointing to
Right -- the most of the code is modeled after code in some utilities in $OPENSSL_ROOT/apps.
It's from apps.ccopy_extensions() in apps.c. I'll add a comment.
PS1, Line 635: if (!req->req_info ||
: !req->req_info->pubkey ||
: !req->req_info->pubkey->public_key ||
: !req->req_info->pubkey->public_key->data) {
: return Status::RuntimeError("Corrupted CSR: no public key");
: }
> isn't this redundant with the get_pubkey call below?
X509_REQ_get_pubkey() implementation in 1.0.2 does not check for req->req_info->pubkey->public_key->data
Line 680: CHECK_NOTNULL(ret);
> this will give a warning in gcc I think. instead you can use this inline be
Good catch! Yes, that's a typo.
PS1, Line 686: v3
: CERT_RET_NOT_OK(X509_set_version(ret, 2
> says v3 but you set to 2?
Yes, it is what it is -- this is not a typo. May be, they have a proper constant for that -- will take a look.
https://www.openssl.org/docs/manmaster/man3/X509_set_version.html
Line 692: CERT_RET_IF_NULL(X509_gmtime_adj(X509_get_notAfter(ret), exp_seconds),
> http://openssl.6102.n7.nabble.com/overflow-when-calling-X509-gmtime-adj-on-
The second parameter of X509_time_adj() is long, so it should not be a problem given the fact Kudu works only on 64-bit machines. I tried up to 20 years of certificate validity, but it was still under 2^31. I'm not sure anybody is about to generate internal Kudu certs which would be valid for 70 years or more :)
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
Line 36: enum DataFormat {
> Consider making this an enum class.
Done
PS1, Line 37: DER = 0, // DER/ASN1 format (binary)
: PEM = 1, // PEM format (ASCII)
> Why do we care about supporting both formats? Is there a use case? If not i
As you mentioned, PEM is good for print-outs and reading from a file, DER is good for passing data on the wire. So I thought it's worth supporting both -- one binary, one text-oriented.
Also, I could not see all use-cases and supporting just a couple didn't seem to be a daunting task when I implemented this.
BTW, I recall you also mentioned most of the Java packages you looked at recently do not support PEM format.
Probably, we need to discuss this in more details before making a decision.
Line 62: class Wrapper {
> having a little trouble understanding what the advantage of this is over ju
Using unique_ptr would require mentioning elem_free_func or calling get_deleter() every time while constructing or adopting/swapping an object this type. That's the only reason.
PS1, Line 65: elemFreeFunc_
> nit: no camelCase in C++ style
Done
PS1, Line 87: public BasicWrapper, public Wrapper<EVP_PKEY>
> Multiple inheritance isn't allowed: https://google.github.io/styleguide/cpp
Aggregating Wrapper<> or unique_ptr<> would require proxying a couple of methods. Besides, Wrapper<> might be useful in other scenarios, that's why I separated its interface.
However, in the end it turned out there is no need for separate Wrapper<> interface elsewhere. So, I'll aggregate the Wrapper just as unique_ptr<>, etc. Thanks for pointing at this.
PS1, Line 120: uint16_t
> I think better to take 'int' and check input (https://google.github.io/styl
Done
PS1, Line 144: method
> it's a function rather than a method (it's not part of any class)
Done
PS1, Line 144: The generated key will be needed later on
: // when using the X509 certificate generated from the CSR.
> not sure this extra sentence adds much value -- I think it's sort of implic
Done
PS1, Line 174: // The configuration and hostnames/ips are made separate parameters to
: // work around limitations of old gcc 4.4.x compiler. Ideally, both hostnames
: // and ips should have been fields of the Config structure.
> confused about this - we don't support gcc <4.8
I had some issues compiling the described alternative on ve0518.halxg.cloudera.com with the default gcc compiler (not sure we have 4.8 there). I'll remove this misleading comment.
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.cc
File src/kudu/security/crypto/crypto_common.cc:
PS1, Line 38: 256
> curious why 256 - we have good evidence that no errors are longer than this
I saw this in err_prn.c, ERR_print_errors_cb() function in OpenSSL code, so it should be enough, I think -- it does not include any file/path. But it would not hurt to put 1024 or 4096, if we want to be really paranoid.
Line 44: serr << " ";
> a little funny that we end with a whitespace
fixed
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/crypto_common.h
File src/kudu/security/crypto/crypto_common.h:
PS1, Line 38: uni_ptr
> this was confusing me a bit in the other files -- it wasn't obvious what th
Yep, that's openssl_unique_ptr_with_custom_deleter_and_other_perks :)
I didn't realize uni_ptr might be so confusing. I think it would be less confusing if you looked into this file first :)
I'll rename it into c_unique_ptr.
PS1, Line 41: GetErrors
> please rename to something like GetSSLErrors() or SSLErrorString(). Even th
Dan already did so in openssl_util.h -- I'll merge with that code.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [TLS certs management] initial commit
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [TLS certs management] initial commit
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5671/1/src/kudu/security/crypto/cert_management.h
File src/kudu/security/crypto/cert_management.h:
Line 36: enum DataFormat {
Consider making this an enum class.
--
To view, visit http://gerrit.cloudera.org:8080/5671
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic2ce55d38f4d06172fadaaa702f4550997d9bc8f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes