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:55:47 UTC
[kudu-CR] [TLS cert management] security service implementation
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5674
Change subject: [TLS cert management] security service implementation
......................................................................
[TLS cert management] security service implementation
Added implementation of the server-side component which
serves TLS certification management RPC.
The units tests are coming as a separate changelist.
Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/security_service.cc
A src/kudu/security/security_service.h
5 files changed, 260 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5674/1
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [TLS cert management] security service implementation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [TLS cert management] security service implementation
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.cc
File src/kudu/security/security_service.cc:
PS1, Line 61: (s)
this macro evaluates 's' a bunch of times, which is no good if it has side effects.
The usual style we do is something like:
const Status& _s = (s);
... and then use _s
PS1, Line 77: ((s).ToString()
this is redundant with the string which already ends up in the statuspb (set by StatusToPB above)
PS1, Line 86: ConvertFromPBFormat
usual style is 'DataFormatFromPB'
Line 88: if (PREDICT_FALSE(!format)) {
just CHECK or DCHECK
PS1, Line 111: server->result_tracker()
hm, given that this .cc depends on kudu/rpc, I think we probably need it to be a separate cmake module. That or just put it directly into src/kudu/master/ since our only usage of it will be within the master
Line 136: unique_ptr<string> ca_cert_str(new string);
this is a bit strange - I think a more readable way with the same perf would be:
string ca_cert_str;
...
ca_cert_pb->mutable_data()->swap(ca_cert_str);
or
*ca_cert_pb->mutable_data() = std::move(ca_cert_str);
Line 167: unique_ptr<string> cert_str(new string);
see above
http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.h
File src/kudu/security/security_service.h:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
think it makes sense to name this file specifically for the cert-signing service (do we anticipate multiple new services?)
PS1, Line 42: master::Master
this cyclic reference is a no-no.
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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] security service implementation
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] security service implementation
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.cc
File src/kudu/security/security_service.cc:
PS1, Line 61: (s)
> this macro evaluates 's' a bunch of times, which is no good if it has side
Done
PS1, Line 77: ((s).ToString()
> this is redundant with the string which already ends up in the statuspb (se
Done
PS1, Line 86: ConvertFromPBFormat
> usual style is 'DataFormatFromPB'
Done
Line 88: if (PREDICT_FALSE(!format)) {
> just CHECK or DCHECK
Done
PS1, Line 111: server->result_tracker()
> hm, given that this .cc depends on kudu/rpc, I think we probably need it to
I separated this into securit_ca library.
Line 136: unique_ptr<string> ca_cert_str(new string);
> this is a bit strange - I think a more readable way with the same perf woul
Looks good -- I think the performance of this could be a little bit better.
Line 167: unique_ptr<string> cert_str(new string);
> see above
Done
http://gerrit.cloudera.org:8080/#/c/5674/1/src/kudu/security/security_service.h
File src/kudu/security/security_service.h:
Line 1: // Licensed to the Apache Software Foundation (ASF) under one
> think it makes sense to name this file specifically for the cert-signing se
Done
PS1, Line 42: master::Master
> this cyclic reference is a no-no.
Good catch. It does not require master::Master, but just kudu::server::ServerBase to provide metrics and result tracker for the RPC server stub.
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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] security service implementation
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/5674
to look at the new patch set (#2).
Change subject: [TLS cert management] security service implementation
......................................................................
[TLS cert management] security service implementation
Added implementation of the server-side component which
serves TLS certification management RPC.
The units tests are coming as a separate changelist.
Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_signing_service.cc
A src/kudu/security/ca/cert_signing_service.h
5 files changed, 271 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5674/2
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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] [security] Kudu certificate 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/5674
to look at the new patch set (#3).
Change subject: [security] Kudu certificate signing service
......................................................................
[security] Kudu certificate signing service
Initial implementation of the Kudu certificate signing service.
The service provides RPC interface to sign X509 CSRs for the
end-entities (i.e. tablet servers). Also, it's possible to get list
of all valid CA root certificates at the given moment.
The units tests are added as well.
Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_signing_service.cc
A src/kudu/security/ca/cert_signing_service.h
A src/kudu/security/test/cert_signing_service-test.cc
6 files changed, 463 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5674/3
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] Kudu certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] Kudu certificate signing service
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5674/3/src/kudu/master/master.cc
File src/kudu/master/master.cc:
Line 129: if (FLAGS_master_enable_cert_signing) {
per comment elsewhere, I think we should figure out how this is actually going to get integrated into our real RPCs -- a standalone service seems less useful than piggy-backing into existing places.
That said, most of the code is still relevant.
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] Kudu certificate signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] Kudu certificate signing service
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5674/3/src/kudu/master/master.cc
File src/kudu/master/master.cc:
Line 129: if (FLAGS_master_enable_cert_signing) {
> per comment elsewhere, I think we should figure out how this is actually go
That sounds good to me -- automated propagation of certs is much better, of course.
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] Kudu certificate 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/5674
to look at the new patch set (#4).
Change subject: [security] Kudu certificate signing service
......................................................................
[security] Kudu certificate signing service
Initial implementation of the Kudu certificate signing service.
The service provides RPC interface to sign X509 CSRs for the
end-entities (i.e. tablet servers). Also, it's possible to get list
of all valid CA root certificates at the given moment.
The units tests are added as well.
Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
---
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/ca/cert_signing_service.cc
A src/kudu/security/ca/cert_signing_service.h
A src/kudu/security/test/cert_signing_service-test.cc
6 files changed, 466 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5674/4
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] Kudu certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change.
Change subject: [security] Kudu certificate signing service
......................................................................
Abandoned
got done elsewhere
--
To view, visit http://gerrit.cloudera.org:8080/5674
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4e4319c752ce64c53046db5d29c3d50306bdcdc5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>