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:32:23 UTC
[kudu-CR] [TLS cert management] TLS cert generation unit tests
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5672
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
[TLS cert management] TLS cert generation unit tests
Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/test/cert_management-test.cc
2 files changed, 733 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/5672/1
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TLS cert management] TLS cert generation unit tests
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5672/1/src/kudu/security/test/cert_management-test.cc
File src/kudu/security/test/cert_management-test.cc:
PS1, Line 115: .ca_cert_path = ca_cert_file_,
: .ca_private_key_path = ca_private_key_file_,
in real usage, we'll be using a CA cert generated programmatically instead of loaded from a file, right?
PS1, Line 140: size_t
I think int is more appropriate here
Line 143: CHECK_OK(GenerateKey(key_bits, &key));
curious: when we generate a new key, does that drain system entropy in such a way that this test is likely to hang?
PS1, Line 297: aExp
can you rename this to avoid abbreviating 'Expired'?
PS1, Line 352:
: // A test which verifies that non-initialized CertRequestGenerator and
: // CertSigner behave as expected.
per comments on the code, I dont think it's worth coding for or testing these negative cases. These are just internal Kudu-facing APIs, so need to be this defensive, instead of just crashing in the case that a Kudu developer forgets to initialize.
PS1, Line 382: static const struct TestKeyInfo {
: const string fpath;
: const char* data;
: } kKeyInfo[] = {
: {
: ca_private_key_file_,
: kCaPrivateKey_,
: },
: {
: ca_exp_private_key_file_,
: kCaExpPrivateKey_,
: },
: };
hrm, I think this would be simpler to read in a more C++-style:
const vector<pair<string, string>> key_info = {
{ca_private_key_file_, kCaPrivateKey_},
{ca_exp_private_key_file_, kCaExpPrivateKey_}
};
and then a simple foreach loop
PS1, Line 614: transformtion
typo
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
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 cert management] TLS cert generation unit tests
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/5672
to look at the new patch set (#3).
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
[TLS cert management] TLS cert generation unit tests
Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
---
M src/kudu/security/CMakeLists.txt
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
4 files changed, 744 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/5672/3
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
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] [TLS cert management] TLS cert generation unit tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5672/1/src/kudu/security/test/cert_management-test.cc
File src/kudu/security/test/cert_management-test.cc:
PS1, Line 382: static const struct TestKeyInfo {
: const string fpath;
: const char* data;
: } kKeyInfo[] = {
: {
: ca_private_key_file_,
: kCaPrivateKey_,
: },
: {
: ca_exp_private_key_file_,
: kCaExpPrivateKey_,
: },
: };
> I think having proper named field is more readable than first, second. But
I decided to simplify things and remove the loop at all.
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
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 cert management] TLS cert generation unit tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
Abandoned
Abandoning -- the unit tests are added into 5671 item.
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
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] [TLS cert management] TLS cert generation unit tests
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/5672
to look at the new patch set (#2).
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
[TLS cert management] TLS cert generation unit tests
Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/test/cert_management-test.cc
2 files changed, 675 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/5672/2
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
Gerrit-PatchSet: 2
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 cert management] TLS cert generation unit tests
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] TLS cert generation unit tests
......................................................................
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/5672/1/src/kudu/security/test/cert_management-test.cc
File src/kudu/security/test/cert_management-test.cc:
PS1, Line 115: .ca_cert_path = ca_cert_file_,
: .ca_private_key_path = ca_private_key_file_,
> in real usage, we'll be using a CA cert generated programmatically instead
In case of our own CA, yes. In case of keys deployed by some external PKI -- read them from a file, I think. But in this context, at this point they should be already read into memory, so here should be strings/buffers with some data -- you are right. Will fix.
PS1, Line 140: size_t
> I think int is more appropriate here
Why? The 'i' is of size_t type. Or you mean the type for 'i' should be int as well?
Line 143: CHECK_OK(GenerateKey(key_bits, &key));
> curious: when we generate a new key, does that drain system entropy in such
This test runs not so fast, so probably it does drain the entropy, but I do not know how to measure that. However, timewise it's not like reading many bytes from /dev/urandom and I haven't seen this test ever hung.
PS1, Line 297: aExp
> can you rename this to avoid abbreviating 'Expired'?
Done
PS1, Line 352:
: // A test which verifies that non-initialized CertRequestGenerator and
: // CertSigner behave as expected.
> per comments on the code, I dont think it's worth coding for or testing the
I thought it's better to avoid crash if it's possible consistently resolve this error for both the caller and the callee, at least in release mode. The idea is that we cannot guarantee we cover all code paths in debug mode with our tests. Crashing in production in cases when it's possible to consistently handle the error is not enticing to me.
Do you think it's better to crash regardless?
PS1, Line 382: static const struct TestKeyInfo {
: const string fpath;
: const char* data;
: } kKeyInfo[] = {
: {
: ca_private_key_file_,
: kCaPrivateKey_,
: },
: {
: ca_exp_private_key_file_,
: kCaExpPrivateKey_,
: },
: };
> hrm, I think this would be simpler to read in a more C++-style:
I think having proper named field is more readable than first, second. But I like the idea of simple foreach loop. Will update.
PS1, Line 614: transformtion
> typo
Done
--
To view, visit http://gerrit.cloudera.org:8080/5672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I281a7b1135c3f64d300a1cc31f304fa2609d54fd
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