You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/02/09 08:55:59 UTC
[kudu-CR] WIP: generate self-signed certs on server startup, remove server cert manager
Hello Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5955
to review the following change.
Change subject: WIP: generate self-signed certs on server startup, remove server_cert_manager
......................................................................
WIP: generate self-signed certs on server startup, remove server_cert_manager
This builds off https://gerrit.cloudera.org/#/c/5948/, and adds the
removal of ServerCertManager in favor of rolling the functionality into
TlsContext. ServerCertManager and TlsContext previously had a lot of
overlap in functionality, and by having them separate it led to awkward
lifetime issues between them.
Before the servers get certs signed by the internal CA, we still need to
have some kind of cert to support GSSAPI-authenticated connections. This
patch makes the servers generate self-signed certs, and changes the RPC
layer to check whether the TlsContext has a cert in order to decide
whether to advertise TLS.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
With this patch, TLS-capable clients and servers will now encrypt
traffic. I checked that using 'kudu table list' and tshark with a
vanilla configuration kudu-master running locally.
WIP:
- may not want to enable TLS by default until we have the localhost
'no-encryption' optimization in place
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
20 files changed, 538 insertions(+), 345 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/1
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] generate self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 6:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:
Line 105: MessengerBuilder& enable_inbound_tls(std::string server_uuid);
> worth a doc note about what effect the UUID has
Done
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS6, Line 110:
: return Status::RuntimeError(
: Substitute("could not verify certificate chain (subject=$0, issuer=$1)",
:
> not sure about this part of the change. The cert that failed to verify may
Done
PS6, Line 192: if (VLOG_IS_ON(2)) {
: string s;
: cert.ToString(&s, DataFormat::PEM);
: VLOG(2) << s;
: }
> I think I accidentally left this VLOG in one of my earlier patches that you
Done
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.h
File src/kudu/security/tls_context.h:
Line 52: // cert, access the CSR, and transtition to a CA-signed cert, repectively
> nit: add '.'
Done
PS6, Line 109: usuing
> nit: typo
Done
Line 115: // boost::none;
> typo: '.'
Done
Line 157: // Protexts has_cert_ and csr_, as well as ctx_ and trusted_cert_count_ when
> typo
Done
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
PS6, Line 54: expected_status_
> nit: don't end in _
Done
PS6, Line 55: : client_cert(client_cert_type),
: client_verification(client_verification_mode),
: server_cert(server_cert_type),
: server_verification(server_verification_mode),
: expected_status(std::move(expected_status_)) {
: }
> given that we are just initializing all of the fields in the same order as
Done
Line 160: Status InitTlsContextCert(const PrivateKey& ca_key,
> nit: don't indent namespaces
Done
PS6, Line 226: ASSERT_TRUE(!server_tls_.has_cert());
: ASSERT_TRUE(!server_tls_.has_signed_cert());
> nit: may read better to use ASSERT_FALSE rather than ASSERT_TRUE(!...)
Done
PS6, Line 293: InitTlsContextCert(ca_key, ca_cert, &client_tls_, test_case.client_cert);
: InitTlsContextCert(ca_key, ca_cert, &server_tls_, test_case.server_cert);
> ASSERT_OK for these?
Done
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
Line 367: csr->ToString(req.mutable_csr_der(), security::DataFormat::DER);
> CHECK_OK maybe?
Added a RETURN_NOT_OK, let me know if I should change it to CHECK.
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5955/5/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 204: ignore_result(X509_check_ca(cert.GetRawData()));
I think we'll also want to do this in AdoptSignedCert as well, although it's possible that one of the checks we are doing there already calls it.
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Reviewed-on: http://gerrit.cloudera.org:8080/5955
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
22 files changed, 622 insertions(+), 363 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5955
to look at the new patch set (#8).
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
22 files changed, 622 insertions(+), 363 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/8
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 6:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/rpc/messenger.h
File src/kudu/rpc/messenger.h:
Line 105: MessengerBuilder& enable_inbound_tls(std::string server_uuid);
worth a doc note about what effect the UUID has
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS6, Line 110:
: return Status::RuntimeError(
: Substitute("could not verify certificate chain (subject=$0, issuer=$1)",
:
not sure about this part of the change. The cert that failed to verify may be the CA cert or an intermediate cert, not the cert passed in as 'cert'
PS6, Line 192: if (VLOG_IS_ON(2)) {
: string s;
: cert.ToString(&s, DataFormat::PEM);
: VLOG(2) << s;
: }
I think I accidentally left this VLOG in one of my earlier patches that you built this off of, but I dont see it being particularly useful. do you?
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_context.h
File src/kudu/security/tls_context.h:
Line 52: // cert, access the CSR, and transtition to a CA-signed cert, repectively
nit: add '.'
PS6, Line 109: usuing
nit: typo
Line 115: // boost::none;
typo: '.'
Line 157: // Protexts has_cert_ and csr_, as well as ctx_ and trusted_cert_count_ when
typo
Odd to see trusted_cert_count_ being an atomic, but protected by a lock. Explain? maybe it should just be a normal int, since iirc it's just for tests
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
PS6, Line 54: expected_status_
nit: don't end in _
PS6, Line 55: : client_cert(client_cert_type),
: client_verification(client_verification_mode),
: server_cert(server_cert_type),
: server_verification(server_verification_mode),
: expected_status(std::move(expected_status_)) {
: }
given that we are just initializing all of the fields in the same order as the ctor params, could we just use brace initialization? not sure the ctor is doing anything.
Line 160: Status InitTlsContextCert(const PrivateKey& ca_key,
nit: don't indent namespaces
PS6, Line 226: ASSERT_TRUE(!server_tls_.has_cert());
: ASSERT_TRUE(!server_tls_.has_signed_cert());
nit: may read better to use ASSERT_FALSE rather than ASSERT_TRUE(!...)
(same below)
PS6, Line 293: InitTlsContextCert(ca_key, ca_cert, &client_tls_, test_case.client_cert);
: InitTlsContextCert(ca_key, ca_cert, &server_tls_, test_case.server_cert);
ASSERT_OK for these?
http://gerrit.cloudera.org:8080/#/c/5955/6/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
Line 367: csr->ToString(req.mutable_csr_der(), security::DataFormat::DER);
CHECK_OK maybe?
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5955/5/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 204: ignore_result(X509_check_ca(cert.GetRawData()));
> I think we'll also want to do this in AdoptSignedCert as well, although it'
Done
Line 206: // Step 2: Adopt the new key and cert.
> This step is out of order.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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] WIP: generate self-signed certs on server startup, remove server cert manager
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5955
to look at the new patch set (#2).
Change subject: WIP: generate self-signed certs on server startup, remove server_cert_manager
......................................................................
WIP: generate self-signed certs on server startup, remove server_cert_manager
This builds off https://gerrit.cloudera.org/#/c/5948/, and adds the
removal of ServerCertManager in favor of rolling the functionality into
TlsContext. ServerCertManager and TlsContext previously had a lot of
overlap in functionality, and by having them separate it led to awkward
lifetime issues between them. It also incidently fixed a bug where the
server cert manager was giving a signed cert to the tls context, the tls
context rejected it, and the server cert manager still went on thinking
no signed cert was needed.
Before the servers get certs signed by the internal CA, we still need to
have some kind of cert to support GSSAPI-authenticated connections. This
patch makes the servers generate self-signed certs, and changes the RPC
layer to check whether the TlsContext has a cert in order to decide
whether to advertise TLS.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
With this patch, TLS-capable clients and servers will now encrypt
traffic. I checked that using 'kudu table list' and tshark with a
vanilla configuration kudu-master running locally.
WIP:
- may not want to enable TLS by default until we have the localhost
'no-encryption' optimization in place
- registration test is failing because the tserver is not loading the
master's CA cert into its trust store.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
21 files changed, 611 insertions(+), 376 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/2
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] generate self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 4:
(9 comments)
It looks like the leak that's causing asan test failures may be internal to OpenSSL. I'm still investigating.
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/integration-tests/registration-test.cc
File src/kudu/integration-tests/registration-test.cc:
Line 206: AssertEventually([&](){
> error: incomplete definition of type 'const security::TlsContext' [clang-di
Done
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/security/ca/cert_management.h
File src/kudu/security/ca/cert_management.h:
Line 158: // Generate a self-signed certificate using the given key and CSR
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 205: csr_ = std::move(csr);
> warning: don't use else after return [readability-else-after-return]
Done
PS1, Line 210:
> nit: consider dropping 'Signed' since we don't have non-signed ones.
Signed here means it must not be self-signed.
PS1, Line 230:
> nit: drop this
Done
PS1, Line 241:
> nit: drop this
Done
PS1, Line 243: // This should never fail sin
> nit: probably, this is just debug log, i.e. use VLOG() or something
Done
PS1, Line 261: UseCertificateAndKey(c, k
> Does this work for intermediate CA certs?
Good point - I'll drop the 'Root'.
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/security/tls_handshake-test.cc
File src/kudu/security/tls_handshake-test.cc:
Line 59: expected_status(std::move(expected_status_)) {
> warning: value argument 'expected_status_' can be moved to avoid copy [misc
Done
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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] WIP: generate self-signed certs on server startup, remove server cert manager
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: WIP: generate self-signed certs on server startup, remove server_cert_manager
......................................................................
Patch Set 1:
(6 comments)
some nits so far.
It seems you are working on revision 2, right?
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS1, Line 210: Signed
nit: consider dropping 'Signed' since we don't have non-signed ones.
PS1, Line 230: signed
nit: drop this
PS1, Line 241: signed
nit: drop this
PS1, Line 243: LOG(INFO) << "resetting CSR";
nit: probably, this is just debug log, i.e. use VLOG() or something
PS1, Line 261: AddTrustedRootCertificate
Does this work for intermediate CA certs?
http://gerrit.cloudera.org:8080/#/c/5955/1/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:
PS1, Line 426: AdoptSignedCert
naming nit while you are at it: why 'Signed'? I think in all cases we have only signed certificates. Consider dropping 'Signed', so it would be just 'AdoptCert(cert)'
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [security] generate self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5955
to look at the new patch set (#4).
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
22 files changed, 627 insertions(+), 372 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/4
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] generate self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: [security] generate self-signed certs on server startup
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5955/5/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 206: // Step 2: Adopt the new key and cert.
This step is out of order.
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5955
to look at the new patch set (#3).
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/client/client-internal.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
23 files changed, 632 insertions(+), 376 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/3
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [security] generate self-signed certs on server startup
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/5955
to look at the new patch set (#5).
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
22 files changed, 636 insertions(+), 372 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/5
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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 self-signed certs on server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5955
to look at the new patch set (#6).
Change subject: [security] generate self-signed certs on server startup
......................................................................
[security] generate self-signed certs on server startup
Masters and tablet servers now generate a keypair and self-signed cert
at startup for accepting TLS connections. Additionally, the tablet
server will now request and receive a CA-signed cert from the Master
during heartbeat. As a result, connections between servers and between
servers and TLS-capable clients will now use TLS.
The responsibilies of ServerCertManager have been rolled into
TlsContext, since they overlapped a great deal. This resulted in an
overall simpler flow for acquiring, storing, and using TLS certs.
This also changes a bit of code to generate proper self-signed certs.
Self-signed certs need to have the 'keyCertSign' attribute set, or else
OpenSSL won't properly recognize the self-signature.
Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
---
M src/kudu/integration-tests/registration-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/security/CMakeLists.txt
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/ca/cert_management.h
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/crypto.cc
M src/kudu/security/crypto.h
M src/kudu/security/openssl_util.h
D src/kudu/security/server_cert_manager.cc
D src/kudu/security/server_cert_manager.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_context.h
M src/kudu/security/tls_handshake-test.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/tserver/heartbeater.cc
22 files changed, 637 insertions(+), 372 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/5955/6
--
To view, visit http://gerrit.cloudera.org:8080/5955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie785cc80d1cd8275defa3987f8e2a3bbcae02622
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@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>