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:45:24 UTC
[kudu-CR] [TLS cert management] added protobuf interface
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5673
Change subject: [TLS cert management] added protobuf interface
......................................................................
[TLS cert management] added protobuf interface
Added protobuf interface for Kudu TLS certificate management RPC.
This is just an interface declaration and appropriate cmake file
sections. The implemetation of the service is coming in a separate
changelist.
Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/security.proto
2 files changed, 218 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/5673/1
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [security] interface for certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 3:
This seems pretty reasonable, but I'm not sure whether this makes sense as a standalone RPC service.
In the real application, I'd guess that the tserver would just want to piggy-back the CSR with its "Register" RPC that it already sends to MasterService. And fetching the CAs would be piggy-backed on the client asking for an authentication token, etc.
That said, I think the protobufs for the CSRs, errors, etc, are reasonable, and might be handy, but I think we should hold off comitting the service part until we figure out how it's actually integrated?
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No
[kudu-CR] [TLS cert management] added protobuf interface
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [TLS cert management] added protobuf interface
......................................................................
Patch Set 1:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/5673/1/src/kudu/security/security.proto
File src/kudu/security/security.proto:
Line 52: DER = 0;
should have an UNKNOWN = 999 first
PS1, Line 50: // X509 formats.
: enum DataFormat {
: DER = 0;
: PEM = 1;
: }
per comment elsewhere, dunno if it's really advantageous to support both... though looking at the Java libraries it seems like it only supports PKCS7 and DER, not PEM.
PS1, Line 63: X509CertSigningRequestPB
I think to clarify that this isn't a normal 'RequestPB' (i.e. an RPC request) we should call this X509CsrPB
Line 112: optional Error error = 1;
If you're using the 'extend ErrorStatusPB' thing above, you don't need to include this in your PB response.
PS1, Line 114: prior/current/next
This makes it sound like there will always be three valid ones. From the client perspective, do we care, or is it just 'all valid CA certs'?
Line 134: enum Code {
instead of separate codes for each method, might be better to just have a PB and set of codes for all CA-related errors
Line 171: optional Error error = 1;
see above
PS1, Line 174: correspoding
typo
PS1, Line 184: authentity
typo
PS1, Line 187: Get prior, current and next root CA certificates. At every moment,
: // only one pair of (private key, CA cert) is active:
see above
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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] added protobuf interface
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/5673
to look at the new patch set (#2).
Change subject: [TLS cert management] added protobuf interface
......................................................................
[TLS cert management] added protobuf interface
Added protobuf interface for Kudu TLS certificate management RPC.
This is just an interface declaration and appropriate cmake file
sections. The implemetation of the service is coming in a separate
changelist.
Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/security_ca.proto
2 files changed, 182 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/5673/2
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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] interface for certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 4:
I think parts of the code will end up grabbed into MasterService but I think we agreed not to have a separate standalone service impl, because the guts are going to be piggy-backed onto existing master RPCs for the most part.
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No
[kudu-CR] [security] interface for certificate signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 3: Verified+1
Unrelevant test breakage: RaftConsensusITest.TestConfigChangeUnderLoad
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [security] interface for 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/5673
to look at the new patch set (#3).
Change subject: [security] interface for certificate signing service
......................................................................
[security] interface for certificate signing service
Added protobuf interface for Kudu certificate signing service.
This is just an interface declaration and appropriate cmake file
sections. The implemetation of the service is coming in a separate
changelist.
Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/security_ca.proto
2 files changed, 182 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/5673/3
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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] interface for certificate signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 3:
> This seems pretty reasonable, but I'm not sure whether this makes
> sense as a standalone RPC service.
>
> In the real application, I'd guess that the tserver would just want
> to piggy-back the CSR with its "Register" RPC that it already sends
> to MasterService. And fetching the CAs would be piggy-backed on the
> client asking for an authentication token, etc.
>
> That said, I think the protobufs for the CSRs, errors, etc, are
> reasonable, and might be handy, but I think we should hold off
> comitting the service part until we figure out how it's actually
> integrated?
> This seems pretty reasonable, but I'm not sure whether this makes
> sense as a standalone RPC service.
>
> In the real application, I'd guess that the tserver would just want
> to piggy-back the CSR with its "Register" RPC that it already sends
> to MasterService. And fetching the CAs would be piggy-backed on the
> client asking for an authentication token, etc.
>
> That said, I think the protobufs for the CSRs, errors, etc, are
> reasonable, and might be handy, but I think we should hold off
> comitting the service part until we figure out how it's actually
> integrated?
Thank you for the review and the analysis.
Yes, you are right -- we need to get some sort of 'automation' of in requesting/retrieving the certificates. I think the way you mention is a very good option.
There is no rush from my side in pushing this -- we can update this to make it fit our need any way we want. Let me know when it's time to tailor this a bit as needed.
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No
[kudu-CR] [TLS cert management] added protobuf interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] added protobuf interface
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5673/1/src/kudu/security/security.proto
File src/kudu/security/security.proto:
PS1, Line 50: // X509 formats.
: enum DataFormat {
: DER = 0;
: PEM = 1;
: }
> per comment elsewhere, dunno if it's really advantageous to support both...
That's pretty surprising -- PEM is a ubiquitous format.
So, do you think we should leave only DER format in the context of this RPC interface?
PS1, Line 114: prior/current/next
> This makes it sound like there will always be three valid ones. From the cl
After some consideration I think there is no 'next' certificate: it's always current and valid prior ones, if any. We switch to the newly one as soon as it's generated, making it current. When we start the very first time, there will be just the newly generated one.
Depending on the parameters of pre-expire cert generation, there might be more than 3 valid cert. Of course, the parameters should be set to allow having not too many valid certs at a time, but I don't think we should put a limit on the number of those if we control the generation of the certs using just 2 parameters: cert validity interval (for how long the generated certs are valid, starting from now) and pre-expiration time (for how long before current cert expires generate a new one).
However, I like the brevity and simplicity of 'all valid CA certs' :) Will just add that the current comes first.
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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] interface for certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 3:
OK. I'm starting to hack on the tablet server side to generate a cert and CSR and send it to the master for signing. I'll just use an ephemeral cert on the master for now, and assume that you're working on the persistence of a cert? sound good?
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No
[kudu-CR] [security] interface for certificate signing service
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has abandoned this change.
Change subject: [security] interface for certificate signing service
......................................................................
Abandoned
got done elsewhere
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
[kudu-CR] [TLS cert management] added protobuf interface
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [TLS cert management] added protobuf interface
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/5673/1/src/kudu/security/security.proto
File src/kudu/security/security.proto:
Line 52: DER = 0;
> should have an UNKNOWN = 999 first
Done
PS1, Line 63: X509CertSigningRequestPB
> I think to clarify that this isn't a normal 'RequestPB' (i.e. an RPC reques
Done
Line 112: optional Error error = 1;
> If you're using the 'extend ErrorStatusPB' thing above, you don't need to i
woops, I was confused with the nested extension syntax.
Line 134: enum Code {
> instead of separate codes for each method, might be better to just have a P
Good idea.
Line 171: optional Error error = 1;
> see above
Done
PS1, Line 174: correspoding
> typo
Done
PS1, Line 184: authentity
> typo
Done
PS1, Line 187: Get prior, current and next root CA certificates. At every moment,
: // only one pair of (private key, CA cert) is active:
> see above
Done
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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] interface for certificate signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has uploaded a new patch set (#4).
Change subject: [security] interface for certificate signing service
......................................................................
[security] interface for certificate signing service
Added protobuf interface for Kudu certificate signing service.
This is just an interface declaration and appropriate cmake file
sections. The implemetation of the service is coming in a separate
changelist.
Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
---
M src/kudu/security/CMakeLists.txt
A src/kudu/security/security_ca.proto
2 files changed, 182 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/73/5673/4
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
[kudu-CR] [security] interface for certificate signing service
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 3:
> OK. I'm starting to hack on the tablet server side to generate a
> cert and CSR and send it to the master for signing. I'll just use
> an ephemeral cert on the master for now, and assume that you're
> working on the persistence of a cert? sound good?
Yep, that sounds good to me. I'll update the rest of my patches to address your comments -- may be, it's worth committing the TLS groundwork patch.
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No
[kudu-CR] [security] interface for certificate signing service
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] interface for certificate signing service
......................................................................
Patch Set 4:
Is this obsoleted by https://gerrit.cloudera.org/#/c/5766/ ?
--
To view, visit http://gerrit.cloudera.org:8080/5673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9ff31e56be42bfa8d0f9b908ba2ccd2734407f55
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>
Gerrit-HasComments: No