You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/01/23 04:18:26 UTC
[kudu-CR] security: generate certs on the tserver, sign them on the master
Hello Dan Burkert, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5766
to review the following change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Tablet Servers have a new TSCertManager instance which generate a
private key on startup. They also generate a CSR and adopt a signed
cert once provided. This is also a convenient place to stash the set
of CA certs and plumb them through to the SSL library, though that
isn't implemented yet.
* Similarly, the master has a MasterCertManager instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by tablet servers. This may change a bit in the future as the
CA public keys will likely have to be persisted in the catalog table,
etc, but I figure this is at least a starting point to unblock other
work.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Additionally, there are plenty of TODOs which we'll need to address in
follow-on patches.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_manager.cc
A src/kudu/master/master_cert_manager.h
M src/kudu/master/master_service.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_server.h
A src/kudu/tserver/ts_cert_manager.cc
A src/kudu/tserver/ts_cert_manager.h
15 files changed, 513 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/1
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/5/src/kudu/security/server_cert_manager.cc
File src/kudu/security/server_cert_manager.cc:
PS5, Line 33: server
> I was pondering whether this cert may also want to end up as a self-signed
Sounds good.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/ts_cert_manager.h
File src/kudu/tserver/ts_cert_manager.h:
Line 47: class TSCertManager {
> I think we will want to use this class on the master as well to generate a
hrm, yea, this probably will end up being more like 'server_cert_manager' or something. let me ponder how that'll look (forgot about the master getting its own TLS cert signed by itself)
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5766
to look at the new patch set (#6).
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Servers (both TS and Master) have a new ServerCertManager instance
which generate a private key on startup. They also generate a CSR and
adopt a signed cert once provided. This is also a convenient place to
stash the set of CA certs and plumb them through to the SSL library,
though that isn't implemented yet.
* Similarly, the master has a MasterCertAuthority instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by other servers. This may change a bit in the future as the
CA cert will have to be loaded from the system tablet if it's
available, rather than generated on startup.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:
* Currently the master doesn't yet sign its own cert. This is going to
have some interaction with the storage of certs in the catalog table,
so want to wait until that code is integrated before figuring out
where to plug this in.
* The built-in PKI stuff should have a flag to disable it. Again I
wasn't sure the best place to put it for now, and it's nice to get the
test coverage of this new code all the time. We can add this flag at
the same point when we add the appropriate flags to configure your own
PKI.
* Various other questions and vague thoughts that we can address as we
go.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 511 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/6
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
Nope, just curious if I may have missed something. I guess if it becomes flaky going forward we'll notice it. Definitely not a functional issue, potentially just a timing change due to slower startup.
(we could consider setting the RSA key size to 512 bits for tests or somesuch)
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5766
to look at the new patch set (#4).
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Servers (both TS and Master) have a new ServerCertManager instance
which generate a private key on startup. They also generate a CSR and
adopt a signed cert once provided. This is also a convenient place to
stash the set of CA certs and plumb them through to the SSL library,
though that isn't implemented yet.
* Similarly, the master has a MasterCertAuthority instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by other servers. This may change a bit in the future as the
CA cert will have to be loaded from the system tablet if it's
available, rather than generated on startup.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:
* Currently the master doesn't yet sign its own cert. This is going to
have some interaction with the storage of certs in the catalog table,
so want to wait until that code is integrated before figuring out
where to plug this in.
* The built-in PKI stuff should have a flag to disable it. Again I
wasn't sure the best place to put it for now, and it's nice to get the
test coverage of this new code all the time. We can add this flag at
the same point when we add the appropriate flags to configure your own
PKI.
* Various other questions and vague thoughts that we can address as we
go.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 512 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/4
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
> Not sure how this patch could be causing the Java test to fail,
> unless it's the extra load of generating certs on startup and the
> test is super sensitive. Anyone have any ideas?
To me the test failure looks like a glitch. Do you see it reproducible on your local machine?
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/4/src/kudu/security/server_cert_manager.cc
File src/kudu/security/server_cert_manager.cc:
PS4, Line 33: (tserver
> This isn't specific to the tserver.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 6:
Going to commit this even though the test will be flaky for a short period of time - don't want to rebase/retest these 3 patches again
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Servers (both TS and Master) have a new ServerCertManager instance
which generate a private key on startup. They also generate a CSR and
adopt a signed cert once provided. This is also a convenient place to
stash the set of CA certs and plumb them through to the SSL library,
though that isn't implemented yet.
* Similarly, the master has a MasterCertAuthority instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by other servers. This may change a bit in the future as the
CA cert will have to be loaded from the system tablet if it's
available, rather than generated on startup.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:
* Currently the master doesn't yet sign its own cert. This is going to
have some interaction with the storage of certs in the catalog table,
so want to wait until that code is integrated before figuring out
where to plug this in.
* The built-in PKI stuff should have a flag to disable it. Again I
wasn't sure the best place to put it for now, and it's nice to get the
test coverage of this new code all the time. We can add this flag at
the same point when we add the appropriate flags to configure your own
PKI.
* Various other questions and vague thoughts that we can address as we
go.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Reviewed-on: http://gerrit.cloudera.org:8080/5766
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 511 insertions(+), 11 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5766
to look at the new patch set (#5).
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Servers (both TS and Master) have a new ServerCertManager instance
which generate a private key on startup. They also generate a CSR and
adopt a signed cert once provided. This is also a convenient place to
stash the set of CA certs and plumb them through to the SSL library,
though that isn't implemented yet.
* Similarly, the master has a MasterCertAuthority instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by other servers. This may change a bit in the future as the
CA cert will have to be loaded from the system tablet if it's
available, rather than generated on startup.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:
* Currently the master doesn't yet sign its own cert. This is going to
have some interaction with the storage of certs in the catalog table,
so want to wait until that code is integrated before figuring out
where to plug this in.
* The built-in PKI stuff should have a flag to disable it. Again I
wasn't sure the best place to put it for now, and it's nice to get the
test coverage of this new code all the time. We can add this flag at
the same point when we add the appropriate flags to configure your own
PKI.
* Various other questions and vague thoughts that we can address as we
go.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 511 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/5
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:
Line 98: cert_manager_.reset(new MasterCertManager(fs_manager_->uuid()));
Probably, this should be optional. I.e., if running Kudu with no security option or using external PKI, then cert_manager_ is not needed.
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 257: optional bytes csr_der = 5;
Why not to depend on security_ca.proto and use X509CsrPB here instead?
Line 284: optional bytes signed_cert_der = 7;
Ditto for X509CertPB from security_ca.proto
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/tablet_server.h
File src/kudu/tserver/tablet_server.h:
Line 108: std::unique_ptr<TSCertManager> cert_manager_;
> Looks like this doesn't necessarily need to be wrapped in unique_ptr, any r
Probably, in case of non-TLS case, it's just a wrapper around nil?
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/ts_cert_manager.cc
File src/kudu/tserver/ts_cert_manager.cc:
Line 61: // TODO(aserbin): do these fields actually have to be set?
ok, I'll take a look. Basically, I just need to make sure whether we are loading openssl.conf or not. I think we should not (need just to double-check). And if so, then only common name (uuid) is needed. Comment might make sense if we want to add some Kudu-specific information as well. And either hostnames or ips are necessary.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
Fixed in r5, r6 will just be a rebase of r5 (trivial conflict in CMakeLists.txt)
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master.proto
File src/kudu/master/master.proto:
Line 257: optional bytes csr_der = 5;
> Why not to depend on security_ca.proto and use X509CsrPB here instead?
For this use case, I don't think we gain much from the flexibility of having the PEM vs DER as an option, so i was following the KISS principle of just sending a string
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/tablet_server.h
File src/kudu/tserver/tablet_server.h:
Line 108: std::unique_ptr<TSCertManager> cert_manager_;
> Probably, in case of non-TLS case, it's just a wrapper around nil?
also allows just forward declaration here, which improves compile time (and the "cost" of the pointer indirection isn't substantial for this path)
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/ts_cert_manager.cc
File src/kudu/tserver/ts_cert_manager.cc:
Line 61: // TODO(aserbin): do these fields actually have to be set?
> ok, I'll take a look. Basically, I just need to make sure whether we are l
got it. Even hostname I'm not 100% sure it's necessary for our use case, since we can check against the uuid matching, no? will have to see how the TLS client can be configured.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5766
to look at the new patch set (#2).
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Tablet Servers have a new TSCertManager instance which generate a
private key on startup. They also generate a CSR and adopt a signed
cert once provided. This is also a convenient place to stash the set
of CA certs and plumb them through to the SSL library, though that
isn't implemented yet.
* Similarly, the master has a MasterCertManager instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by tablet servers. This may change a bit in the future as the
CA public keys will likely have to be persisted in the catalog table,
etc, but I figure this is at least a starting point to unblock other
work.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Additionally, there are plenty of TODOs which we'll need to address in
follow-on patches.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_manager.cc
A src/kudu/master/master_cert_manager.h
M src/kudu/master/master_service.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_server.h
A src/kudu/tserver/ts_cert_manager.cc
A src/kudu/tserver/ts_cert_manager.h
15 files changed, 508 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/2
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/5/src/kudu/security/server_cert_manager.cc
File src/kudu/security/server_cert_manager.cc:
PS5, Line 33: server
> on second thought, what do you think about 'rpc'? I think that distinguish
I was pondering whether this cert may also want to end up as a self-signed cert in the webserver so people can have encryption by default (vs making their own self-signed cert). I know you think encryption without authentication is useless but... :)
Anyway I marked it experimental so we're free to come back and rename it until we decide it's good. Let's wait until the end and then do a once-over of all config and see where names can be made more consistent?
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/4/src/kudu/security/server_cert_manager.cc
File src/kudu/security/server_cert_manager.cc:
PS4, Line 33: (tserver
This isn't specific to the tserver.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master_cert_manager.cc
File src/kudu/master/master_cert_manager.cc:
PS2, Line 36: master_ca_rsa_key_length_bits
I think 'master' in this config flag is redundant, really it's just CA vs not CA that makes this flag distinguished from the other key_length_bits flag.
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/master/master_cert_manager.h
File src/kudu/master/master_cert_manager.h:
Line 46: explicit MasterCertManager(std::string uuid);
Could you add a note about what the UUID signifies, or consider renaming to server_uuid if that's what it's for.
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/tablet_server.h
File src/kudu/tserver/tablet_server.h:
Line 108: std::unique_ptr<TSCertManager> cert_manager_;
Looks like this doesn't necessarily need to be wrapped in unique_ptr, any reason to prefer it?
http://gerrit.cloudera.org:8080/#/c/5766/2/src/kudu/tserver/ts_cert_manager.h
File src/kudu/tserver/ts_cert_manager.h:
Line 47: class TSCertManager {
I think we will want to use this class on the master as well to generate a cert for TLS, right? Or is the plan to just use the CA cert?
Line 86: std::unique_ptr<security::ca::Cert> signed_cert_;
Cert and Key are already effectively newtype'd unique_ptrs, so this is just adding an unecessary level of indirection.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/5/src/kudu/security/server_cert_manager.cc
File src/kudu/security/server_cert_manager.cc:
PS5, Line 33: server
on second thought, what do you think about 'rpc'? I think that distinguishes it better than server. So we would have
rpc_rsa_key_length_bits and
ca_rsa_key_length_bits
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 6:
arg, that Java test failed again. I'll investigate before committing.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5766/4/src/kudu/master/master_cert_authority.cc
File src/kudu/master/master_cert_authority.cc:
PS4, Line 110: can the signer
: // modify the CSR to add fields, etc, indicating when/where it was signed?
: // maybe useful for debugging.
Good idea. I think we can add functionality to allow the cert signer to specify comment (there is 'Netscape comment' extension). Right now that field is in the CSR request itself, but I think we can allow the cert signer to modify that -- I'll take a look at that.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 6:
OK, indeed the test was sensitive to slower startup times of the daemons (caused now because of the RSA key generation).
I put up a patch http://gerrit.cloudera.org:8080/5803 to address. We should also consider making the tests use 512-bit RSA keys. I'll do that separately.
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4:
Not sure how this patch could be causing the Java test to fail, unless it's the extra load of generating certs on startup and the test is super sensitive. Anyone have any ideas?
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] security: generate certs on the tserver, sign them on the master
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5766
to look at the new patch set (#3).
Change subject: security: generate certs on the tserver, sign them on the master
......................................................................
security: generate certs on the tserver, sign them on the master
This adds a bit of plumbing for the self-hosted PKI:
* Servers (both TS and Master) have a new ServerCertManager instance
which generate a private key on startup. They also generate a CSR and
adopt a signed cert once provided. This is also a convenient place to
stash the set of CA certs and plumb them through to the SSL library,
though that isn't implemented yet.
* Similarly, the master has a MasterCertAuthority instance which generates
a key and self-signed CA cert on startup. It can then sign certs
provided by other servers. This may change a bit in the future as the
CA cert will have to be loaded from the system tablet if it's
available, rather than generated on startup.
* When the TS heartbeats, it checks if the cert manager has a signed
cert yet. If not, it sends the CSR to the master in DER format.
* If the master gets a heartbeat with a CSR, it signs it and returns the
signed cert in the heartbeat response. The tablet server then adopts
this as its cert.
A number of items are left as follow-ons. I noted them with "TODO(PKI)"
so that they'll be easy to grep for before we call this feature done.
In particular:
* Currently the master doesn't yet sign its own cert. This is going to
have some interaction with the storage of certs in the catalog table,
so want to wait until that code is integrated before figuring out
where to plug this in.
* The built-in PKI stuff should have a flag to disable it. Again I
wasn't sure the best place to put it for now, and it's nice to get the
test coverage of this new code all the time. We can add this flag at
the same point when we add the appropriate flags to configure your own
PKI.
* Various other questions and vague thoughts that we can address as we
go.
Note that this doesn't add any actual functionality, since the resulting
certs aren't actually attached to the RPC system in any way.
Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
A src/kudu/master/master_cert_authority.cc
A src/kudu/master/master_cert_authority.h
M src/kudu/master/master_service.cc
M src/kudu/security/CMakeLists.txt
A src/kudu/security/server_cert_manager.cc
A src/kudu/security/server_cert_manager.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/heartbeater.cc
15 files changed, 510 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5766/3
--
To view, visit http://gerrit.cloudera.org:8080/5766
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3eb8ab4edc17e2fa1a54e0123a06dabc59a0489b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>