Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201709010504.v8154O6x009314@ip-10-146-233-104.ec2.internal>
Date: Fri, 1 Sep 2017 05:04:24 +0000
From: "Henry Robinson (Code Review)" <gerrit@cloudera.org>
To: Bharath Vissapragada <bharathv@cloudera.com>,
        Sailesh Mukil <sailesh@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Lars Volker <lv@cloudera.com>
Reply-To: henry@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5849=3A_Remove_compile-time_checks_for_OpenSSL_=3E_1=2E0=2E0=0A?=
In-Reply-To: <gerrit.1503966809000.I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d@gerrit.cloudera.org>
References: <gerrit.1503966809000.I20c5d39c0c4ae9c5445dd3ee3b175fe337a5728d@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 01 Sep 2017 05:04:31 -0000

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 <henry@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>