You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Sailesh Mukil (Code Review)" <ge...@cloudera.org> on 2017/08/12 00:00:38 UTC
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Sailesh Mukil has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7662
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.
STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this STACK_OF(X509)
object.
When we call AddTrustedCertificate(Cert&), we iterate throught the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.
Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain certificates,
if we want to use IPKI with chain certificates, additional functionality
will need to be added with clearer APIs.
External PKI makes use of this ability to add a chain CA if necessary.
Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.
Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
9 files changed, 424 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/1
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 4:
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/9318/ : FAILURE
The test failure looks like a flaky test.
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7662
to look at the new patch set (#2).
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.
STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this STACK_OF(X509)
object.
When we call AddTrustedCertificate(Cert&), we iterate throught the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.
Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain certificates,
if we want to use IPKI with chain certificates, additional functionality
will need to be added with clearer APIs.
External PKI makes use of this ability to add a chain CA if necessary.
Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.
Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 429 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/2
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/7662/2//COMMIT_MSG
Commit Message:
PS2, Line 15:
> nit: please make the string <= 72 characters long
Done
PS2, Line 18: through
> nit: typo
Done
PS2, Line 25:
> nit: please make the string <= 72 characters long
Done
http://gerrit.cloudera.org:8080/#/c/7662/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
Line 149: auto cleanup = MakeScopedCleanup([&]() {
> in the case of a bad PEM file format, I think this would end up leaving a p
Great point. I removed SCOPED_OPENSSL_NO_PENDING_ERRORS here and below.
Line 156: // Iterate through the chain certificate and add each one to the stack.
> similar to above, I think this will result in returning null but with no pe
Agreed. I moved this to the calls Cert::FromString() and Cert::FromFile().
A test for this already exists in CertTest.CertInvalidInput, which works even after moving this from here.
PS3, Line 165:
> the free call is no longer below. maybe just say 'the ScopedCleanup'
Done
Line 175: if (ret <= 0) return ret;
> same
Done
Line 188: }
> same
Done
Line 202:
> same
Done
Line 212: }
> same
Done
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 4: Code-Review+2 Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.
STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this
STACK_OF(X509) object.
When we call AddTrustedCertificate(Cert&), we iterate through the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.
Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain
certificates, if we want to use IPKI with chain certificates,
additional functionality will need to be added with clearer APIs.
External PKI makes use of this ability to add a chain CA if necessary.
Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.
Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Reviewed-on: http://gerrit.cloudera.org:8080/7662
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 431 insertions(+), 55 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 3:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/7662/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
Line 149: if (!info) return nullptr;
in the case of a bad PEM file format, I think this would end up leaving a pending OpenSSL error, and then the SCOPED_OPENSSL_NO_PENDING_ERRORS would crash.
I'm not convinced that the SCOPED_OPENSSL_NO_PENDING_ERRORS is appropriate here, since this is function is supposed to have an SSL-like calling convention (return NULL and leave a pending error on failure). The pending error should be handled (converted to Status) by the code in FromBIO(...) in openssl_util_bio.h.
Line 156: if (chain_len == 0) return nullptr;
similar to above, I think this will result in returning null but with no pending errors on the OpenSSL error stack. So when FromBIO(...) calls GetOpenSSLErrors() it's going to get an empty string and this will then return a RuntimeError with no explanation.
Perhaps it's better to remove this check and change the check to be in the code which consumes the STACK_OF(X509) (ie Cert::FromBIO)
PS3, Line 165: free() call below
the free call is no longer below. maybe just say 'the ScopedCleanup'
Line 175: SCOPED_OPENSSL_NO_PENDING_ERRORS;
same
Line 188: SCOPED_OPENSSL_NO_PENDING_ERRORS;
same
Line 202: SCOPED_OPENSSL_NO_PENDING_ERRORS;
same
Line 212: SCOPED_OPENSSL_NO_PENDING_ERRORS;
same
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
Line 37: #include "kudu/util/status.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done
PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() {
: sk_X509_INFO_pop_free(info, X509_INFO_free);
: });
> nit: if you introduced corresponding trait functions for STACK_OK(X509_INFO
sk_X509_INFO_pop_free() takes 2 arguments but kFreeFunc is always passed one argument, so this will not work unless we create a wrapper function for sk_X509_INFO_pop_free(), which I think is going beyond what we need since we use it only in a single place.
Let me know what you think.
PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will
: // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr.
: // We will take the responsibility of freeing it when we are done with the STACK_OF(X509).
: stack_item->x509 = nullptr;
> Would the scoped cleanup about try to free the data just put into the stack
It would if we didn't set the pointer to nullptr, but since we set it to nullptr, it will ignore it.
Another way of doing this would be to use sk_X509_dup() above instead of sk_X509_push() to copy the X509 items over to the final stack, and not do the following line:
stack_item->x509 = nullptr;
But I thought this would be better for performance in the case of large chains.
PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) {
: return nullptr;
: }
: return sk;
> I think it should be
Ah yes, this seems the right way to do it. Done.
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
PS1, Line 162: CHECK_OK
ASSERT_OK() ?
PS1, Line 171: LOG(INFO) << "Connecting to " << server_addr.ToString();
Is this crucial to have this info line in the test? As I understand, this might be useful only if test fails -- if really necessary, consider using SCOPED_TRACE() with the server_addr.ToString().
PS1, Line 178: for (int i = 0; i < 10; i++) {
nit: just curious why calling it once is not enough
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 214: void Cert::AdoptAndAddRefRawData(STACK_OF(X509)* data) {
Why not just to use X509_chain_up_ref() and remove the constraint on data_chain_len?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:
PS1, Line 76: STACK_OF(X509)
nit: this is RawDataType typedef
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:
Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, pem_password_cb* unused2,
> yea, I think this is better off in the .cc file anyway since it's too large
once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERRORS
PS1, Line 178: STACK_OF(X509)* sk = sk_X509_new_null();
: if (sk_X509_push(sk, x) == 0) return nullptr;
There might be a memory leak if the 'sk' is allocated but xk_X509_push() failed. Consider using ssl_make_unique() construct (you could look around for examples).
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
Line 499: Status CreateTestSSLCertSignedByChain(const string& dir,
> add a comment explaining how this was generated?
nit: indentation for the last 3 parameters
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:
PS1, Line 194: if (cert) remote_cert_.AdoptX509(cert);
nit: consider keeping the original brace style here
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 2:
(5 comments)
Overall looks good to me, just some nits in the commit message. Maybe, Todd also want to take a look?
http://gerrit.cloudera.org:8080/#/c/7662/2//COMMIT_MSG
Commit Message:
PS2, Line 15: 9)
nit: please make the string <= 72 characters long
PS2, Line 18: throught
nit: typo
PS2, Line 25: es,
nit: please make the string <= 72 characters long
http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() {
: sk_X509_INFO_pop_free(info, X509_INFO_free);
: });
> sk_X509_INFO_pop_free() takes 2 arguments but kFreeFunc is always passed on
OK, I think it's good enough.
PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will
: // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr.
: // We will take the responsibility of freeing it when we are done with the STACK_OF(X509).
: stack_item->x509 = nullptr;
> It would if we didn't set the pointer to nullptr, but since we set it to nu
Ah, it seems that was my misunderstanding: I somehow missed the 'X509_INFO *stack_item = sk_X509_INFO_value(info, i)' assignment. Sorry.
I think the way it's implemented now is fine.
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7662
to look at the new patch set (#4).
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.
STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this
STACK_OF(X509) object.
When we call AddTrustedCertificate(Cert&), we iterate through the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.
Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain
certificates, if we want to use IPKI with chain certificates,
additional functionality will need to be added with clearer APIs.
External PKI makes use of this ability to add a chain CA if necessary.
Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.
Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 431 insertions(+), 55 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/4
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 2:
(41 comments)
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
PS1, Line 162: ASSERT_O
> ASSERT_OK() ?
Done
PS1, Line 171: SCOPED_TRACE(strings::Substitute("Connecting to $0", ser
> Is this crucial to have this info line in the test? As I understand, this
I changed it to a scoped trace, so that we can check if the test server started or not easily.
PS1, Line 178: ASSERT_OK(DoTestSyncCall(p, Ge
> nit: just curious why calling it once is not enough
I think it should be fine, but I copied the above TestCall() test. I guess the point is to see if it works reliably. But since we're only testing if the certificates work, I think we can remove it.
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 71: X509* Cert::GetEndOfChainX509() const {
> worth a CHECK_GT(chain_len_, 0); or an if statement and return nullptr in t
I added a CHECK_GT() since we only use it after populating Cert object.
PS1, Line 72: T(chain_len(), 0);
> could you use sk_X509_value(data_.get(), chain_len_ - 1) here?
Done
Line 78: }
> sk_X509_num?
Done
Line 214: DCHECK(cert);
> Why not just to use X509_chain_up_ref() and remove the constraint on data_c
Unfortunately, X509_chain_up_ref() is only available from OpenSSL 1.1.0. Should I add another #ifdef or just leave it as it is?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:
Line 45: // TODO(unknown): Currently, there isn't a mechanism to add to the chain. Implement it when needed.
> warning: missing username/bug in TODO [google-readability-todo]
Done
PS1, Line 63: the e
> the end-user cert?
Done
PS1, Line 76: RawDataType* d
> nit: this is RawDataType typedef
Done
PS1, Line 89: GetEndOfChainX509
> nit: think this should be named GetEndOfChainX509 since it returns an X509*
Done
Line 93: public:
> why not just use sk_X509_num(data_.get()) where necessary? is caching the v
Not really. I initially thought the sk_num() function might be large, but I looked at the sk_X509_num() function and it's very small, so we might as well not re-track it.
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:
Line 125: // SslTypeTraits functions for Type STACK_OF(X509)
> warning: do not use unnamed namespaces in header files [google-build-namesp
Done
Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> warning: parameter 'unused2' is unused [misc-unused-parameters]
Done
Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> warning: parameter 'unused1' is unused [misc-unused-parameters]
Done
Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> once moved into the .cc file, consider adding SCOPED_OPENSSL_NO_PENDING_ERR
Done
Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> warning: function 'PEM_read_STACK_OF_X509' defined in a header file; functi
Done
Line 128: int PEM_write_STACK_OF_X509(BIO* bio, STACK_OF(X509)* obj);
> yea, I think this is better off in the .cc file anyway since it's too large
Done
Line 129: STACK_OF(X509)* DER_read_STACK_OF_X509(BIO* bio, void* /* unused */);
> warning: parameter 'unused3' is unused [misc-unused-parameters]
Done
PS1, Line 131: void free_STACK_OF_X509(STACK_OF(X509)* sk);
:
: t
> combine onto one line
Done
Line 138: static constexpr auto kWriteDerFunc = &DER_write_STACK_OF_X509;
> can you use a ScopedCleanup to make this automatic right after allocating t
Done
PS1, Line 143: plate<> struct SslTypeTraits<X509_REQ> {
: static constexpr auto kF
> combine on one line?
Done
Line 147: static constexpr auto kWritePemFunc = &PEM_write_bio_X509_REQ;
> move into the loop body since it's only used in that scope?
Done
Line 161:
> warning: function 'PEM_write_STACK_OF_X509' defined in a header file; funct
Done
Line 162: // Acceptable formats for keys, X509 certificates and X509 CSRs.
> sk_X509_num?
Done
Line 164: DER = 0, // DER/ASN1 format (binary): for representing object on the wire
> move inside loop
Done
PS1, Line 167:
: // Data format representation as a string.
: c
> combine
Not really, I looked at some tutorials and some stack overflow examples to understand the use of these APIs and wrote this part pretty quickly, so I sort of dropped the ball on the style part. Sorry about that.
Line 175: public:
> warning: parameter 'unused' is unused [misc-unused-parameters]
Done
Line 175: public:
> warning: function 'DER_read_STACK_OF_X509' defined in a header file; functi
Done
Line 176: typedef Type RawDataType;
> is such a thing actually commonplace out there? can we at least detect it a
Do you mean chain certificates in DER format? It is possible to do it if the certificate uses the PKCS7 standard.
The d2i_X509_bio() call below only reads single certificates anyway, so if a chain DER certificate is presented, I would expect that call would fail and return nullptr.
Line 177:
> what if x is null?
Done
PS1, Line 178: RawDataType* GetRawData() const {
: return data_.get();
> There might be a memory leak if the 'sk' is allocated but xk_X509_push() fa
Done
Line 184: }
> warning: function 'DER_write_STACK_OF_X509' defined in a header file; funct
Done
Line 192:
> warning: function 'free_STACK_OF_X509' defined in a header file; function d
Done
Line 196: public:
> warning: anonymous namespace not terminated with a closing comment [google-
Done
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
Line 499: //
> add a comment explaining how this was generated?
I followed a tutorial online which is fairly long and involves configurations files (so not just using commands). So I just added a link to the tutorial. Let me know if you think it'll be better to add the actual commands and the conf file contents itself.
Line 499: //
> nit: indentation for the last 3 parameters
Done
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.h
File src/kudu/security/test/test_certs.h:
Line 74: std::string* cert_file,
> nit: indentation
Done
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 255: X509* inner_cert = sk_X509_value(cert.GetRawData(), i);
> sk_X509_value
Done
Line 270: return Status::OK();
> should you add chain_len() here?
I was thinking about that earlier and I know this part gets a bit confusing. IIUC, the TLS context can trust multiple CAs not related to each other right? In that case, wouldn't leaving it as +=1 be the right thing to do?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_handshake.cc
File src/kudu/security/tls_handshake.cc:
PS1, Line 194: if (cert) {
> nit: consider keeping the original brace style here
Done
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7662
to look at the new patch set (#3).
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Kudu previously did not recognize chain certificates. This patch
enables support for chain certificates by changing the Cert class'
underlying data type to STACK_OF(X509) instead of just X509.
STACK_OF(X509) allows multiple certificates to be held by the same
pointer. When we are presented with a file or a string that contains
multiple X509 certificates, they will be stored inside this STACK_OF(X509)
object.
When we call AddTrustedCertificate(Cert&), we iterate throught the
STACK_OF(X509) contained in the Cert and add each one individually to
the X509_STORE for later verification.
Currently, IPKI does not make use of this ability and still works
with single certificates. DCHECKS are added to make sure that multiple
X509 certificates are not accidentally added to a Cert object.
Although this patch provides a general framework to use chain certificates,
if we want to use IPKI with chain certificates, additional functionality
will need to be added with clearer APIs.
External PKI makes use of this ability to add a chain CA if necessary.
Testing: A new test is added to rpc-test that uses a chain CA. This
test does not work without this patch.
Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/cert.cc
M src/kudu/security/cert.h
M src/kudu/security/openssl_util.cc
M src/kudu/security/openssl_util.h
M src/kudu/security/test/test_certs.cc
M src/kudu/security/test/test_certs.h
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
10 files changed, 430 insertions(+), 53 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7662/3
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 1:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 71: X509* Cert::GetEndOfChainCert() const {
worth a CHECK_GT(chain_len_, 0); or an if statement and return nullptr in the case of an empty stack.
PS1, Line 72: static_cast<X509*>(sk_value
could you use sk_X509_value(data_.get(), chain_len_ - 1) here?
Line 78: if (s.ok()) chain_len_ = sk_num(reinterpret_cast<const _STACK*>(data_.get()));
sk_X509_num?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.h
File src/kudu/security/cert.h:
PS1, Line 63: this
the end-user cert?
PS1, Line 89: GetEndOfChainCert
nit: think this should be named GetEndOfChainX509 since it returns an X509* instead of a Cert wrapper
Line 93: int chain_len_ = 0;
why not just use sk_X509_num(data_.get()) where necessary? is caching the value necessary?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/openssl_util.h
File src/kudu/security/openssl_util.h:
Line 128: STACK_OF(X509)* PEM_read_STACK_OF_X509(BIO* bio, void* unused1, pem_password_cb* unused2,
> warning: function 'PEM_read_STACK_OF_X509' defined in a header file; functi
yea, I think this is better off in the .cc file anyway since it's too large to inline.
PS1, Line 131: STACK_OF(X509_INFO)* info;
: info = PEM_X509_INFO_read_bio(bio, nullptr, nullptr, nullptr);
:
combine onto one line
Line 138: sk_X509_INFO_pop_free(info, X509_INFO_free);
can you use a ScopedCleanup to make this automatic right after allocating the 'info' and avoid having it in three places?
PS1, Line 143: TACK_OF(X509)* sk;
: sk = sk_X509_new_null();
combine on one line?
Line 147: X509_INFO *stack_item = nullptr;
move into the loop body since it's only used in that scope?
Line 162: unsigned chain_len = sk_num(reinterpret_cast<const _STACK*>(obj));
sk_X509_num?
use 'int' instead of 'unsigned' to make unsigned overflow bugs less likely
Line 164: X509* cert_item = nullptr;
move inside loop
PS1, Line 167: int ret;
: ret = PEM_write_bio_X509(bio, cert_item);
:
combine
(it looks like a bunch of this code has this style - I'm guessing you borrowed it from some C language example. Is there any copyright notice we need to retain? is the license OK?)
Line 176: // We don't support chain certificates written in DER format.
is such a thing actually commonplace out there? can we at least detect it and LOG(FATAL) or otherwise return some warning instead of silently ignoring?
Line 177: X509* x = d2i_X509_bio(bio, nullptr);
what if x is null?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.cc
File src/kudu/security/test/test_certs.cc:
Line 499: Status CreateTestSSLCertSignedByChain(const string& dir,
add a comment explaining how this was generated?
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/test/test_certs.h
File src/kudu/security/test/test_certs.h:
Line 74: std::string* cert_file,
nit: indentation
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 255: X509* inner_cert =
sk_X509_value
Line 270: trusted_cert_count_ += 1;
should you add chain_len() here?
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2091: Certificates with intermediate CA's do not work with Kudu
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2091: Certificates with intermediate CA's do not work with Kudu
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
PS1, Line 178: ASSERT_OK(DoTestSyncCall(p, Ge
> I think it should be fine, but I copied the above TestCall() test. I guess
Just was curious whether it was necessary to have it here, but that was not crucial and you removed it already, that's fine.
http://gerrit.cloudera.org:8080/#/c/7662/1/src/kudu/security/cert.cc
File src/kudu/security/cert.cc:
Line 214: DCHECK(cert);
> Unfortunately, X509_chain_up_ref() is only available from OpenSSL 1.1.0. Sh
I think you can leave it as is -- I was just thinking that the code might be simplified. If not and it requires additional ifdefs, then it's better to keep it as is.
http://gerrit.cloudera.org:8080/#/c/7662/2/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:
PS2, Line 150: auto cleanup = MakeScopedCleanup([&]() {
: sk_X509_INFO_pop_free(info, X509_INFO_free);
: });
nit: if you introduced corresponding trait functions for STACK_OK(X509_INFO), that could be handled via ssl_make_unique().
PS2, Line 165: // We don't want the free() call below to free the x509 certificates as well since we will
: // use it as a part of the STACK_OF(X509) object to be returned, so we set it to nullptr.
: // We will take the responsibility of freeing it when we are done with the STACK_OF(X509).
: stack_item->x509 = nullptr;
Would the scoped cleanup about try to free the data just put into the stack upon exiting from the scope?
PS2, Line 193: if (sk_X509_push(sk, x.release()) == 0) {
: return nullptr;
: }
: return sk;
I think it should be
if (sk_X509_push(sk, x.get()) == 0) {
return nullptr;
}
x.release();
return sk;
The idea here is to avoid memory leak if sk_X509_push() fails. I looked into the sk_X509_push() and it can fail if it cannot grow the stack to accommodate the new element or the passed stack pointer is nullptr. If so happens, the caller retains the ownership of the passed data/new element.
--
To view, visit http://gerrit.cloudera.org:8080/7662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7334a5b2f1643848152562bbb1dee27b5290e83f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes