You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/08/29 00:33:29 UTC
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7866
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we
altered Thrift-0.9.0-p11 to compile on all versions, and fail with an
error if an unsupported version is requested at runtime. This patch
is the Impala-side counterpart to that work.
We assume that tests will be run on the same machine that built Impala,
so still execute slightly different tests depending on the detected
OpenSSL version.
Also include corresponding changes to Squeasel from this commit:
https://github.com/cloudera/squeasel/commit/9335b8
Testing: New webserver and thrift-server tests.
TODO Toolchain will need a version bump when thrift-0.9.0-p11 is approved.
Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
---
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/service/impala-server.cc
M be/src/thirdparty/squeasel/squeasel.c
M be/src/util/webserver-test.cc
M bin/impala-config.sh
6 files changed, 55 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/7866/1
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1190/
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/thirdparty/squeasel/squeasel.c
File be/src/thirdparty/squeasel/squeasel.c:
Line 4259: SSL_CTX_set_options(ctx->ssl_ctx, options);
Check the return value of this?
if (!(SSL_CTX_set_options(ctx_, options) & options)) {
cry()
}
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/util/webserver-test.cc
File be/src/util/webserver-test.cc:
PS1, Line 298: 0x10001000L
May be define it as OPENSSL_MIN_VERSION_WITH_TLS_1_1 for readability.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1190/
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7866/1//COMMIT_MSG
Commit Message:
Line 25:
Could you add in the commit message what kind of error messages to expect when using unsupported TLS protocol versions with an older OpenSSL?
It will be useful to lookup and point to if any users have this problem in the future, since you mentioned that the error messages aren't very clear in the JIRA.
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 185: system OpenSSL
"linked OpenSSL library" for clarity? Should users choose to link it to a non-system library.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 2: Code-Review+1
+1 after the commit message update.
Feel free to upgrade to a +2 if no one else is looking at it.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7866/1//COMMIT_MSG
Commit Message:
Line 25:
> Could you add in the commit message what kind of error messages to expect w
Sorry, missed this. Will do.
http://gerrit.cloudera.org:8080/#/c/7866/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS1, Line 185: system OpenSSL
> "linked OpenSSL library" for clarity? Should users choose to link it to a n
I thought about this, but I think the best thing is to emphasise that OpenSSL comes from the system, not from Impala itself. I agree that, if they play tricks with LD_LIBRARY_PATH, this might not be 100% accurate, but I don't feel that's going to confuse users.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7866/3//COMMIT_MSG
Commit Message:
Line 51: Error messages
Thanks for finding and fixing this up. Would it make sense to not startup if the linked SSL does not support the requested protocol (i.e. the protocol returned by StringToProtocol() due to a non matching FLAGS_ssl_minimum_version)?
It looks like with this behavior, the cluster will start but keep returning these errors?
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1192/
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.
Lars Volker has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7866/1//COMMIT_MSG
Commit Message:
PS1, Line 24: TODO Toolchain will need a version bump when thrift-0.9.0-p11 is approved.
Can this be removed?
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 1:
(1 comment)
An update: I discovered that it is *not* backwards compatible to use the constants SSL_OP_NO_TLSv1_1 or SSL_OP_NO_TLSv1_2 in OpenSSL 1.0.0 because their values were defined as different constants that have since been deprecated. So setting these options actually does *something different*, despite the supposed backwards compatibility between 1.0.1 and 1.0.0.
I'll be updating this patch with a fix shortly.
http://gerrit.cloudera.org:8080/#/c/7866/1//COMMIT_MSG
Commit Message:
PS1, Line 24: TODO Toolchain will need a version bump when thrift-0.9.0-p11 is approved.
> Can this be removed?
Will do.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 4: Code-Review+2
Carry +2 w/rebase.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7866/3//COMMIT_MSG
Commit Message:
Line 51: Error messages
> Thanks for finding and fixing this up. Would it make sense to not startup i
That's a good suggestion, but having thought about it I think I'd prefer not to. The reason: this would wind up looking something like
ABORT_IF_ERROR(CheckSslConfiguration());
in each daemon's init method. I'm a bit cautious about this because we might forget (and we have a track record of forgetting to do this kind of thing in every daemon!), or if someone someday writes a new daemon or test, they might forget to do so.
So that makes an argument for retaining the protocol checking logic inside Thrift[Server|Client], and in that case I'd prefer to put the logic in just one place. The cluster should fail to start in either case.
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
(Prerequisite knowledge: Impala must support building and linking
against OpenSSL 1.0.0 and 1.0.1. 1.0.0 only supports TLS1.0. 1.0.1
supports TLS1.1 and 1.2 as well).
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we
altered Thrift-0.9.0-p11 to compile on all versions, and fail with an
error if an unsupported version is requested at runtime. This patch
is the Impala-side counterpart to that work, and also works around a bug.
The bug is in OpenSSL. When disabling, say, TLS1.1 we pass an option
bitmask to SSL_CTX_set_options() that includes SSL_OP_NO_TLSv1_1. In
OpenSSL 1.0.1 this is defined as 0x10000000L but in 1.0.0 it is not
defined. So to support compilation on all OpenSSL versions, we added
definitions of that constant to Thrift if it was not found at compile
time.
However, in OpenSSL 1.0.0 *another* option for SSL_CTX_set_options() has
the *same* value (0x10000000L). This means that passing
SSL_OP_NO_TLSv1_1 to OpenSSL 1.0.0 actually does something completely
different. This lack of backwards compatibility is the bug.
To work around it, we use the SSLeay() function, which, although
cryptically named, returns the value of OPENSSL_VERSION_NUMBER in the
version of OpenSSL currently linked against (I have tested that it
behaves differently when dynamically linked against different versions
of OpenSSL, but compiled against the same one). We use that method to
tell us which of the TLS protocol versions are actually supported, and
raise errors if they are not.
An alternative approach to doing this inside ThriftClient and
ThriftServer would be to do so in Thrift; this might be a reasonable
future option but for now it is too unwieldy to test a toolchain build
that is linked against different OpenSSL versions. To reduce risk, and
the number of ways things can go wrong, we do all the protocol version
whitelisting in the Impala code.
To simplify the code further, we also remove the values of
SSLProtoVersions that allowed us to restrict the TLS protocol to
exactly *one* value, rather than specify the minimum (e.g. we remove
TLSv1_1 but retain TLSv1_1_plus). The more restrictive options were not
intended for production use, and using them now will raise an error.
Error messages
--------------
Passing a valid, but unsupported version to --ssl_minimum_version:
"TLS (6) version not supported (linked OpenSSL version is 1234567)"
Passing an invalid version to --ssl_minimum_version:
"Unknown TLS version: 'tls_4_2'"
Testing
-------
We assume that tests will be run on the same machine that built Impala,
so still execute slightly different tests depending on the detected
OpenSSL version.
For thrift-server-test, we check to see if a protocol version is
supported at runtime, and use that to determine the expected behaviour
of a test which starts all combinations of clients and server versions.
For the webserver-test, we determine at compile-time the set of
supported TLS protocols, and change the test's expected behaviour based
on that.
Tests have been run when compiling and running against OpenSSL 1.0.0 and
OpenSSL 1.0.1.
Also include corresponding changes to Squeasel from this commit:
https://github.com/henryr/squeasel/commit/eded53
Testing: New webserver and thrift-server tests.
Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Reviewed-on: http://gerrit.cloudera.org:8080/7866
Tested-by: Impala Public Jenkins
Reviewed-by: Henry Robinson <he...@cloudera.com>
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/thirdparty/squeasel/squeasel.c
M be/src/util/webserver-test.cc
M bin/impala-config.sh
8 files changed, 77 insertions(+), 35 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Henry Robinson: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Bharath Vissapragada, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7866
to look at the new patch set (#5).
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
(Prerequisite knowledge: Impala must support building and linking
against OpenSSL 1.0.0 and 1.0.1. 1.0.0 only supports TLS1.0. 1.0.1
supports TLS1.1 and 1.2 as well).
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we
altered Thrift-0.9.0-p11 to compile on all versions, and fail with an
error if an unsupported version is requested at runtime. This patch
is the Impala-side counterpart to that work, and also works around a bug.
The bug is in OpenSSL. When disabling, say, TLS1.1 we pass an option
bitmask to SSL_CTX_set_options() that includes SSL_OP_NO_TLSv1_1. In
OpenSSL 1.0.1 this is defined as 0x10000000L but in 1.0.0 it is not
defined. So to support compilation on all OpenSSL versions, we added
definitions of that constant to Thrift if it was not found at compile
time.
However, in OpenSSL 1.0.0 *another* option for SSL_CTX_set_options() has
the *same* value (0x10000000L). This means that passing
SSL_OP_NO_TLSv1_1 to OpenSSL 1.0.0 actually does something completely
different. This lack of backwards compatibility is the bug.
To work around it, we use the SSLeay() function, which, although
cryptically named, returns the value of OPENSSL_VERSION_NUMBER in the
version of OpenSSL currently linked against (I have tested that it
behaves differently when dynamically linked against different versions
of OpenSSL, but compiled against the same one). We use that method to
tell us which of the TLS protocol versions are actually supported, and
raise errors if they are not.
An alternative approach to doing this inside ThriftClient and
ThriftServer would be to do so in Thrift; this might be a reasonable
future option but for now it is too unwieldy to test a toolchain build
that is linked against different OpenSSL versions. To reduce risk, and
the number of ways things can go wrong, we do all the protocol version
whitelisting in the Impala code.
To simplify the code further, we also remove the values of
SSLProtoVersions that allowed us to restrict the TLS protocol to
exactly *one* value, rather than specify the minimum (e.g. we remove
TLSv1_1 but retain TLSv1_1_plus). The more restrictive options were not
intended for production use, and using them now will raise an error.
Error messages
--------------
Passing a valid, but unsupported version to --ssl_minimum_version:
"TLS (6) version not supported (linked OpenSSL version is 1234567)"
Passing an invalid version to --ssl_minimum_version:
"Unknown TLS version: 'tls_4_2'"
Testing
-------
We assume that tests will be run on the same machine that built Impala,
so still execute slightly different tests depending on the detected
OpenSSL version.
For thrift-server-test, we check to see if a protocol version is
supported at runtime, and use that to determine the expected behaviour
of a test which starts all combinations of clients and server versions.
For the webserver-test, we determine at compile-time the set of
supported TLS protocols, and change the test's expected behaviour based
on that.
Tests have been run when compiling and running against OpenSSL 1.0.0 and
OpenSSL 1.0.1.
Also include corresponding changes to Squeasel from this commit:
https://github.com/henryr/squeasel/commit/eded53
Testing: New webserver and thrift-server tests.
Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/thirdparty/squeasel/squeasel.c
M be/src/util/webserver-test.cc
M bin/impala-config.sh
8 files changed, 77 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/7866/5
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7866
to look at the new patch set (#2).
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we
altered Thrift-0.9.0-p11 to compile on all versions, and fail with an
error if an unsupported version is requested at runtime. This patch
is the Impala-side counterpart to that work.
We assume that tests will be run on the same machine that built Impala,
so still execute slightly different tests depending on the detected
OpenSSL version.
Also include corresponding changes to Squeasel from this commit:
https://github.com/cloudera/squeasel/commit/9335b8
Testing: New webserver and thrift-server tests.
TODO Toolchain will need a version bump when thrift-0.9.0-p11 is approved.
Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
---
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/service/impala-server.cc
M be/src/thirdparty/squeasel/squeasel.c
M be/src/util/webserver-test.cc
M bin/impala-config.sh
6 files changed, 60 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/7866/2
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Bharath Vissapragada, Sailesh Mukil,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7866
to look at the new patch set (#3).
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
(Prerequisite knowledge: Impala must support building and linking
against OpenSSL 1.0.0 and 1.0.1. 1.0.0 only supports TLS1.0. 1.0.1
supports TLS1.1 and 1.2 as well).
Rather than check for OpenSSL w/TLSv1.1+ support at compile time, we
altered Thrift-0.9.0-p11 to compile on all versions, and fail with an
error if an unsupported version is requested at runtime. This patch
is the Impala-side counterpart to that work, and also works around a bug.
The bug is in OpenSSL. When disabling, say, TLS1.1 we pass an option
bitmask to SSL_CTX_set_options() that includes SSL_OP_NO_TLSv1_1. In
OpenSSL 1.0.1 this is defined as 0x10000000L but in 1.0.0 it is not
defined. So to support compilation on all OpenSSL versions, we added
definitions of that constant to Thrift if it was not found at compile
time.
However, in OpenSSL 1.0.0 *another* option for SSL_CTX_set_options() has
the *same* value (0x10000000L). This means that passing
SSL_OP_NO_TLSv1_1 to OpenSSL 1.0.0 actually does something completely
different. This lack of backwards compatibility is the bug.
To work around it, we use the SSLeay() function, which, although
cryptically named, returns the value of OPENSSL_VERSION_NUMBER in the
version of OpenSSL currently linked against (I have tested that it
behaves differently when dynamically linked against different versions
of OpenSSL, but compiled against the same one). We use that method to
tell us which of the TLS protocol versions are actually supported, and
raise errors if they are not.
An alternative approach to doing this inside ThriftClient and
ThriftServer would be to do so in Thrift; this might be a reasonable
future option but for now it is too unwieldy to test a toolchain build
that is linked against different OpenSSL versions. To reduce risk, and
the number of ways things can go wrong, we do all the protocol version
whitelisting in the Impala code.
To simplify the code further, we also remove the values of
SSLProtoVersions that allowed us to restrict the TLS protocol to
exactly *one* value, rather than specify the minimum (e.g. we remove
TLSv1_1 but retain TLSv1_1_plus). The more restrictive options were not
intended for production use, and using them now will raise an error.
Error messages
--------------
Passing a valid, but unsupported version to --ssl_minimum_version:
"TLS (6) version not supported (linked OpenSSL version is 1234567)"
Passing an invalid version to --ssl_minimum_version:
"Unknown TLS version: 'tls_4_2'"
Testing
-------
We assume that tests will be run on the same machine that built Impala,
so still execute slightly different tests depending on the detected
OpenSSL version.
For thrift-server-test, we check to see if a protocol version is
supported at runtime, and use that to determine the expected behaviour
of a test which starts all combinations of clients and server versions.
For the webserver-test, we determine at compile-time the set of
supported TLS protocols, and change the test's expected behaviour based
on that.
Tests have been run when compiling and running against OpenSSL 1.0.0 and
OpenSSL 1.0.1.
Also include corresponding changes to Squeasel from this commit:
https://github.com/henryr/squeasel/commit/eded53
Testing: New webserver and thrift-server tests.
Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
---
M be/src/rpc/thrift-client.cc
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-server.cc
M be/src/rpc/thrift-server.h
M be/src/service/impala-server.cc
M be/src/thirdparty/squeasel/squeasel.c
M be/src/util/webserver-test.cc
M bin/impala-config.sh
8 files changed, 79 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/66/7866/3
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7866/3//COMMIT_MSG
Commit Message:
Line 51: Error messages
> That's a good suggestion, but having thought about it I think I'd prefer no
That sounds fine to me. Also, I didn't realize that the daemons will fail to start anyway, since we abort if the 'be_server' doesn't start up.
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5849: Remove compile-time checks for OpenSSL > 1.0.0
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: No