[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-17 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10436 )

Change subject: WIP: KUDU-1889: support openssl 1.1
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc@345
PS3, Line 345:   if (!req->req_info ||
> So is this safe to skip because sometime between 1.0.0 and 1.1.0, OpenSSL i
Whether or not it's safe is somewhat orthogonal: in OpenSSL 1.1 we can't access 
these fields anymore, so even if we should check them, we can't.

Anyway, these checks are still live for pre-1.1.0 OpenSSL. Or maybe I'm 
misunderstanding your question?


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc@114
PS3, Line 114:   OPENSSL_init_ssl(0, nullptr);
> What happens if the user application is buggy as per the check below, has a
I'm just going off the manpages, but yeah, the call is expected to be 
idempotent. Notably, SSL_CTX_new() always returns not-null in 1.1 (because it 
implicitly initializes the library), so we can't use that as a test to see if 
someone else has initialized the library.

I didn't check the return value because I assumed that 
SCOPED_OPENSSL_NO_PENDING_ERRORS would surface any error, as it does for the 
various 1.0 initialization calls. I guess I could CHECK on it though.


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc@117
PS3, Line 117:   // In case the user's thread has left some error around, clear 
it.
 :   ERR_clear_error();
 :   SCOPED_OPENSSL_NO_PENDING_ERRORS;
> I think it might be crucial to keep in on top, doing the clean-up even if g
Okay. But we can't do that for 1.1; the library must be initialized before any 
openssl error-related calls, otherwise we may leak memory.



-- 
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 17 May 2018 21:48:37 +
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-17 Thread Alexey Serbin (Code Review) 
Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10436 )

Change subject: WIP: KUDU-1889: support openssl 1.1
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc@345
PS3, Line 345:   if (!req->req_info ||
> So is this safe to skip because sometime between 1.0.0 and 1.1.0, OpenSSL i
IIRC with 1.0.0 that would just crash if not doing that check in case of 
missing public key in the CSR.  Unfortunately, I didn't add a unit test for 
that.


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc@117
PS3, Line 117:   // In case the user's thread has left some error around, clear 
it.
 :   ERR_clear_error();
 :   SCOPED_OPENSSL_NO_PENDING_ERRORS;
I think it might be crucial to keep in on top, doing the clean-up even if 
g_disable_ssl_init is 'false'.



--
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 17 May 2018 21:29:14 +
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-17 Thread Dan Burkert (Code Review) 
Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10436 )

Change subject: WIP: KUDU-1889: support openssl 1.1
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc
File src/kudu/security/ca/cert_management.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/ca/cert_management.cc@345
PS3, Line 345:   if (!req->req_info ||
So is this safe to skip because sometime between 1.0.0 and 1.1.0, OpenSSL is 
now able to handle such a corrupted CSR properly by returning nullptr from 
X509_REQ_get_pubkey?  The manpage for 1.1.0 says this is the case, but that 
manpage doesn't exist for < 1.1.0 afaict.


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/crypto.cc
File src/kudu/security/crypto.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/crypto.cc@94
PS3, Line 94:   static constexpr auto kFreeFunc = _MD_CTX_free;
I get the feeling that the OpenSSL developers are intentionally fucking with 
developers.


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc
File src/kudu/security/openssl_util.cc:

http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc@98
PS3, Line 98: initialisation
ah this explains the misanthropy


http://gerrit.cloudera.org:8080/#/c/10436/3/src/kudu/security/openssl_util.cc@114
PS3, Line 114:   OPENSSL_init_ssl(0, nullptr);
What happens if the user application is buggy as per the check below, has 
already initialized OpenSSL either implicitly or explicitly, and then we 
initialize explicitly here? (I checked the man page and it implies idempotency, 
so I suspect it's fine)

Related, there's a return integer from OEPNSSL_init_ssl, should it be checked?



--
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 17 May 2018 18:32:56 +
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-17 Thread Adar Dembo (Code Review) 
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/10436 )

Change subject: WIP: KUDU-1889: support openssl 1.1
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-17 Thread Adar Dembo (Code Review) 
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10436 )

Change subject: WIP: KUDU-1889: support openssl 1.1
..


Patch Set 3: Verified+1

Overriding Jenkins, another instance of KUDU-2109.


--
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 17 May 2018 18:10:06 +
Gerrit-HasComments: No

[kudu-CR] WIP: KUDU-1889: support openssl 1.1

2018-05-16 Thread Adar Dembo (Code Review) 
Hello Todd Lipcon,

I'd like you to do a code review. Please visit

http://gerrit.cloudera.org:8080/10436

to review the following change.


Change subject: WIP: KUDU-1889: support openssl 1.1
..

WIP: KUDU-1889: support openssl 1.1

OpenSSL 1.1 is the default version available in Ubuntu 18.04.

Note that because of KUDU-2439, tests built against OpenSSL 1.1 are still
somewhat unstable. I've seen some TSAN tests occasionally crash, and some
ASAN tests (seemingly those that use the CLI to connect to a remote server)
report memory leaks.

WIP because I have no idea what I'm doing.

Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
---
M build-support/tsan-suppressions.txt
M src/kudu/rpc/reactor.cc
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/openssl_util.cc
M src/kudu/security/security-test-util.cc
M src/kudu/security/tls_context.cc
M src/kudu/security/tls_handshake.cc
11 files changed, 123 insertions(+), 28 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/36/10436/1
--
To view, visit http://gerrit.cloudera.org:8080/10436
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If1e1c57b5563d1a4cd926b4c4a9a3c271460be04
Gerrit-Change-Number: 10436
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon