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/08/25 00:34:10 UTC
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Hello Henry Robinson, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7821
to review the following change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
---
M src/kudu/security/tls_context.cc
1 file changed, 20 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/1
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3: Code-Review+2
I tested this on platforms with OpenSSL 1.0.0 and 1.0.1e, by building on one and running on the other, and vice versa. And it does what is expected. We will do the same in Impala.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:12:42 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@52
PS2, Line 52: // --rpc-tls-min-protocol=TLSv1.2 option, negotiations will fail at runtime with
: // a 'missing protocol' error:
: /
> I think it's possible to look at SSLv23_method()->version just after initia
In other words, SSLv23_method()->version reports the highest version of the TLS/SSL protocol supported by the OpenSSL library. And it's straightforward to compare the reported version with the required protocol version:
SSL2_VERSION 0x0002
SSL3_VERSION 0x0300
TLS1_VERSION 0x0301
TLS1_1_VERSION 0x0302
TLS1_2_VERSION 0x0303
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 16 Jan 2018 19:19:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS2, Line 71: 0x10000000U
> Here is a fun thing I discovered:
That's an interesting discovery, Henry. Let's try to avoid diverting the code between Impala and Kudu here, so if you go with runtime detection we should do the same.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS2, Line 52: // --rpc-tls-min-protocol=TLSv1.2 option, negotiations will fail at runtime with
: // a 'missing protocol' error:
: /
is there any way we can make this fail earlier? ie at startup rather than at negotiation-time? I'm surprised that it fails only during negotiation time since the call is during TlsContext::Init
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3:
I've updated according to Alexey's suggestion. The check now happens at startup, which is a definite improvement.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 01:09:06 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Henry Robinson, Adar Dembo, Sailesh Mukil, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7821
to look at the new patch set (#3).
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
---
M src/kudu/security/tls_context.cc
1 file changed, 36 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/3
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@155
PS2, Line 155: options |= SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1;
> I'm curious what happens if the built binary is run with the library that d
No, negotiation will fail with the error on line 55.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 20:40:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3: Code-Review+1
LGTM (deferring to other reviewers who might have some more feedback).
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 02:32:06 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@52
PS2, Line 52: // --rpc-tls-min-protocol=TLSv1.2 option, negotiations will fail at runtime with
: // a 'missing protocol' error:
: /
> is there any way we can make this fail earlier? ie at startup rather than a
I think it's possible to look at SSLv23_method()->version just after initialization of the OpenSSL library. It looks like a hack, but it works for the way how the SSLv23_method() is implemented.
As a POC, I compiled the code below at CentOS 6.4 with OpenSSL 1.0.0-stable and then ran both against 1.0.0u and 1.0.1e version. The output was (it's in hexadecimal):
1.0.0u: 301
1.0.1e: 303
301 corresponds to TLSv1
303 corresponds to TLSv1.2
--------
#include <openssl/ssl.h>
#include <openssl/err.h>
#include <iostream>
using namespace std;
void init_openssl() {
SSL_load_error_strings();
OpenSSL_add_ssl_algorithms();
}
void cleanup_openssl() {
EVP_cleanup();
}
int main() {
init_openssl();
cout << std::hex << SSLv23_method()->version << endl;
cleanup_openssl();
return 0;
}
--------
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 21:20:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has uploaded a new patch set (#2).
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
---
M src/kudu/security/tls_context.cc
1 file changed, 25 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/2
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 18:54:18 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@155
PS2, Line 155: options |= SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1;
I'm curious what happens if the built binary is run with the library that does not support TLSv1.2. Is it going just to silently run the with TLSv1.1 here regardless of the fact that the --rpc_tls_min_protocol=TLSv1.2 is set?
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 20:25:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3:
> LD_LIBRARY_PATH against both versions before pushing just in case we missed something
Confirmed
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 18:42:31 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from OpenSSL
> Makes sense, but rather than the macros here, can we do something like this
Done
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from OpenSSL
Makes sense, but rather than the macros here, can we do something like this earlier, after including the openssl headers:
#ifndef SSL_OP_NO_TLSv1
#define SSL_OP_NO_TLSv1 0x04000000U
#endif
#ifndef SSL_OP_NO_TLSv1_1
#define SSL_OP_NO_TLSv1_1 0x10000000U
#endif
That should improve readability in the code down here.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
PS2, Line 71: 0x10000000U
Here is a fun thing I discovered:
henry@hnr-optiplex:/data/henry/src/cloudera/impala-toolchain/openssl-1.0.0s/include (master) $ grep -r 0x10000000L *
openssl/ssl.h:# define SSL_OP_PKCS1_CHECK_2 0x10000000L
henry@hnr-optiplex:/data/henry/src/cloudera/impala-toolchain/openssl-1.0.0s/include (master) $ cd ../../openssl-1.0.2l/include/
henry@hnr-optiplex:/data/henry/src/cloudera/impala-toolchain/openssl-1.0.2l/include $ grep -r 0x10000000L *
openssl/ssl.h:# define SSL_OP_NO_TLSv1_1 0x10000000L
In OpenSSL 1.0.0, the constant that became SSL_OP_NO_TLSv1_1 in 1.0.1 was already in use for an esoteric option that messes with the cryptographic protocol for fault injection (deprecated in 1.0.1).
I can't recall enough about your requirements to puzzle through whether this is a real problem for you, but theoretically it does mean that anyone linked against 1.0.0 that tries to set --rpc_tls_min_protocol=tlsv1.1 will get some unexpected behaviour. IIRC, Kudu isn't expected to work against 1.0.0 anyhow, so this may be an academic issue for you. In Impala, we're probably going to have to use SSLeay() to detect the OpenSSL version at runtime.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@155
PS2, Line 155: options |= SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1;
> No, negotiation will fail with the error on line 55.
Ah, right, thanks. It seems I missed read that comment, starting from this place, my bad.
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jan 2018 20:43:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:
http://gerrit.cloudera.org:8080/#/c/7821/2/src/kudu/security/tls_context.cc@71
PS2, Line 71: 0x10000000U
> That's an interesting discovery, Henry. Let's try to avoid diverting the co
Squeasel did commit the equivalent of this commit, so maybe we should go with this commit to keep it the same as squeasel?
https://github.com/cloudera/squeasel/commit/9335b81317a6451d5a37c5dc7ec088eecbf68c82
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 16 Oct 2017 22:46:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 3: Code-Review+1
lgtm, please test using LD_LIBRARY_PATH against both versions before pushing just in case we missed something
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 06:18:38 +0000
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/7821 )
Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
......................................................................
rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Reviewed-on: http://gerrit.cloudera.org:8080/7821
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Sailesh Mukil <sa...@cloudera.com>
---
M src/kudu/security/tls_context.cc
1 file changed, 36 insertions(+), 12 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
Sailesh Mukil: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-Change-Number: 7821
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>