[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 5: I don't think that was the intention. https://gerrit.cloudera.org/#/c/11530/ tries to re-enable them. -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 26 Sep 2018 23:05:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 5: Was this intended to disable these tests on Ubuntu 16.04? That's what I'm seeing. -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 26 Sep 2018 15:56:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 4: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 31 May 2018 05:29:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite a bit. Our RHEL7 machines come equipped with Python 2.7.5. Looking at these comments, that means that we'll be unable to create a 'SSLContext' but be able to explicitly specify ciphers: https://github.com/apache/thrift/blob/88591e32e710a0524327153c8b629d5b461e35e0/lib/py/src/transport/TSSLSocket.py#L37-L41 # SSLContext is not available for Python < 2.7.9 _has_ssl_context = sys.hexversion >= 0x020709F0 # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 If we cannot create a 'SSLContext', then we cannot use TLSv1.2 and have to use TLSv1: https://github.com/apache/thrift/blob/88591e32e710a0524327153c8b629d5b461e35e0/lib/py/src/transport/TSSLSocket.py#L48-L49 # For python >= 2.7.9, use latest TLS that both client and server # supports. # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. # For python < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is # unavailable. _default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ ssl.PROTOCOL_TLSv1 Our custom cluster test forces the server to use TLSv1.2 and also forces a specific cipher: https://github.com/apache/impala/blob/2f22a6f67ff363a0832a7ceee5d0020c8fd9b15a/tests/custom_cluster/test_client_ssl.py#L118-L119 So this combination of configuration values causes a failure in RHEL7 because we only allow a specific cipher which works with TLSv1.2, but the client cannot use TLSv1.2 due to the Python version as mentioned above. We've not noticed these failures on older-than-RHEL7-systems since the OpenSSL versions on those systems don't support TLSv1.2. (< OpenSSL 1.0.1) To fix this, we need to change the Python version on RHEL 7 to be >= Python 2.7.9. This patch skips the test if an older version of Python than 2.7.9 is detected. Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Reviewed-on: http://gerrit.cloudera.org:8080/10529 Reviewed-by: Sailesh Mukil Tested-by: Impala Public Jenkins --- M tests/custom_cluster/test_client_ssl.py 1 file changed, 6 insertions(+), 6 deletions(-) Approvals: Sailesh Mukil: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 4: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2571/ -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 31 May 2018 02:17:38 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 4: Code-Review+2 (5 comments) Thanks for the reviews! Upgrading to +2 since 2 reviewers +1'd it. http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@9 PS3, Line 9: When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite > To be super clear: we used to ignore this silently but now TSSLSocket.py br I explained this in a previous top level comment. http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@13 PS3, Line 13: https://github.com/apache/thrift/blob/88591e32e710a0524327153c8b629d5b461e35e0/lib/py/src/transport/TSSLSocket.py#L37-L41 > You can press "y" in the github UI to get code a permalink UI. Or specify a Done http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py File tests/custom_cluster/test_client_ssl.py: http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@36 PS3, Line 36: HAS_LEGACY_OPENSSL = getattr(ssl, "OPENSSL_VERSION_NUMBER", None) > It's super confusing that you'd change a constant on line 43. Can you just Done http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@45 PS3, Line 45: > If you want, you can use "getattr" with a default to avoid this. Done http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@132 PS3, Line 132: statestored_args=SSL_WILDCARD_ARGS, > Consider just inlining Done -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Thu, 31 May 2018 02:17:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Hello Michael Brown, Philip Zeyliger, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10529 to look at the new patch set (#4). Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite a bit. Our RHEL7 machines come equipped with Python 2.7.5. Looking at these comments, that means that we'll be unable to create a 'SSLContext' but be able to explicitly specify ciphers: https://github.com/apache/thrift/blob/88591e32e710a0524327153c8b629d5b461e35e0/lib/py/src/transport/TSSLSocket.py#L37-L41 # SSLContext is not available for Python < 2.7.9 _has_ssl_context = sys.hexversion >= 0x020709F0 # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 If we cannot create a 'SSLContext', then we cannot use TLSv1.2 and have to use TLSv1: https://github.com/apache/thrift/blob/88591e32e710a0524327153c8b629d5b461e35e0/lib/py/src/transport/TSSLSocket.py#L48-L49 # For python >= 2.7.9, use latest TLS that both client and server # supports. # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. # For python < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is # unavailable. _default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ ssl.PROTOCOL_TLSv1 Our custom cluster test forces the server to use TLSv1.2 and also forces a specific cipher: https://github.com/apache/impala/blob/2f22a6f67ff363a0832a7ceee5d0020c8fd9b15a/tests/custom_cluster/test_client_ssl.py#L118-L119 So this combination of configuration values causes a failure in RHEL7 because we only allow a specific cipher which works with TLSv1.2, but the client cannot use TLSv1.2 due to the Python version as mentioned above. We've not noticed these failures on older-than-RHEL7-systems since the OpenSSL versions on those systems don't support TLSv1.2. (< OpenSSL 1.0.1) To fix this, we need to change the Python version on RHEL 7 to be >= Python 2.7.9. This patch skips the test if an older version of Python than 2.7.9 is detected. Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 --- M tests/custom_cluster/test_client_ssl.py 1 file changed, 6 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/10529/4 -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 3: Code-Review+1 ...pending Phil's comments, which are fair. The Python version comparison looks like the accepted idiom (Impyla does it, for example) -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Michael Brown Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 30 May 2018 23:58:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 3: > > (5 comments) > > > > My main question is to confirm that we understand why the Python > > upgrade broke this. Before it was also broken, but we were > silently > > ignoring that? > > > > The rest is python nits, which you can ignore if you'd like. > > I did some more digging to answer the questions and this is what > I've come up with (I've posted this in the JIRA as well, but > pasting here for better context): > > I missed a detail which was that this test never ran on RHEL6 due > to all our RHEL6 machines having OpenSSL 1.0.0 which doesn't > support TLSv1.2, causing them to be skipped. > > On RHEL7, this used to work before the Thrift upgrade because the > old Thrift cpp library (0.9.0) was somehow accepting TLSv1 > connections even though we explicitly set TLSv1.2 on the server. > I'm unable to figure out why that was happening, and it looks like > a bug, but I'll keep looking. It could be a bug in the Python 'ssl' > library, or the Thrift 0.9.0 python library, or the Thrift 0.9.0 > CPP library, or even OpenSSL. > > In Thrift 0.9.3, we explicitly select TLSv1.2 if that's what the > user specified which fixes the above mentioned bug. Our test caught > this bug, since the client side doesn't support TLSv1.2 unless we > are equipped with Python 2.7.9 or up. > > As for a weaker test, we already run test_ssl() which is a weaker > test as it doesn't enforce any ciphers or TLS versions which allows > the client and server to negotiate a protocol that they're both > aware of. > > So to summarize, things weren't working as expected before and we > were passing the test when it should have been failing, but the > upgrade fixed the original problem and caused the test to start > failing. > > Carry +1. Thanks for the review! I'll address the nits in a separate patchset. -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 30 May 2018 22:32:58 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 3: Code-Review+1 > (5 comments) > > My main question is to confirm that we understand why the Python > upgrade broke this. Before it was also broken, but we were silently > ignoring that? > > The rest is python nits, which you can ignore if you'd like. I did some more digging to answer the questions and this is what I've come up with (I've posted this in the JIRA as well, but pasting here for better context): I missed a detail which was that this test never ran on RHEL6 due to all our RHEL6 machines having OpenSSL 1.0.0 which doesn't support TLSv1.2, causing them to be skipped. On RHEL7, this used to work before the Thrift upgrade because the old Thrift cpp library (0.9.0) was somehow accepting TLSv1 connections even though we explicitly set TLSv1.2 on the server. I'm unable to figure out why that was happening, and it looks like a bug, but I'll keep looking. It could be a bug in the Python 'ssl' library, or the Thrift 0.9.0 python library, or the Thrift 0.9.0 CPP library, or even OpenSSL. In Thrift 0.9.3, we explicitly select TLSv1.2 if that's what the user specified which fixes the above mentioned bug. Our test caught this bug, since the client side doesn't support TLSv1.2 unless we are equipped with Python 2.7.9 or up. As for a weaker test, we already run test_ssl() which is a weaker test as it doesn't enforce any ciphers or TLS versions which allows the client and server to negotiate a protocol that they're both aware of. So to summarize, things weren't working as expected before and we were passing the test when it should have been failing, but the upgrade fixed the original problem and caused the test to start failing. Carry +1. Thanks for the review! -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 30 May 2018 22:32:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 3: Code-Review+1 (5 comments) My main question is to confirm that we understand why the Python upgrade broke this. Before it was also broken, but we were silently ignoring that? The rest is python nits, which you can ignore if you'd like. http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@9 PS3, Line 9: When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite To be super clear: we used to ignore this silently but now TSSLSocket.py broke this? i.e., why is it that the test started failing after the Thrift change... http://gerrit.cloudera.org:8080/#/c/10529/3//COMMIT_MSG@13 PS3, Line 13: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L37-L41 You can press "y" in the github UI to get code a permalink UI. Or specify a tag instead of "master". http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py File tests/custom_cluster/test_client_ssl.py: http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@36 PS3, Line 36: HAS_TLSV12_INCOMPATIBLE_PYTHON = True It's super confusing that you'd change a constant on line 43. Can you just put line 43 here? The same comment sort of applies to the other stuff, but you've not changed it. http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@45 PS3, Line 45: # Old ssl module versions don't even have OPENSSL_VERSION_NUMBER as a member. If you want, you can use "getattr" with a default to avoid this. e.g.: getattr(ssl, "OPENSSL_VERSION_NUMBER", None) Feel free to ignore since this was pre-existing. http://gerrit.cloudera.org:8080/#/c/10529/3/tests/custom_cluster/test_client_ssl.py@132 PS3, Line 132: @pytest.mark.skipif(HAS_TLSV12_INCOMPATIBLE_PYTHON, reason=SKIP_TLSV12_MSG) Consider just inlining @pytest.mark.skipif(sys.version_info < REQUIRED_MIN_PYTHON_VERSION_FOR_TLSV12, reason="Python version too old to allow Thrift client to use TLSv1.2") -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Wed, 30 May 2018 18:51:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 2: (2 comments) > (2 comments) > > I wrote a comment on the JIRA upstream. > > I think the easier thing to do is to check Python version and skip > if you're using a too-old Python, with an explanation. Possibly add > a second copy of the test with the flag changes you suggest. > > But, regardless, I still don't quite understand whether the test > was just failing silently before or something fundamentally broke. I just realized that the test wasn't running before at all since all non-RHEL7 systems that we use for testing don't have OpenSSL 1.0.1+, which is the minimum required version for TLSv1.2. I updated the patch to skip this specific test if we don't have an up-to-date Python that supports the client using TLSv1.2. http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG@42 PS2, Line 42: which does not force the use of specific ciphers, so we get away without > What doesn't force the user of ciphers? Actually, I've found a flaw in my reasoning, I didn't realize but this test was being skipped on older-than-RHEL7-systems due to them having older versions of OpenSSL, that don't support TLSv1.2. So, the ciphers had nothing to do with it and I've removed this para. http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG@49 PS2, Line 49: least unblock our builds while we can upgrade the AMIs for RHEL7. This > RHEL7 just doesn't ship a newer Python based on my quick check with Docker. Makes sense. I've skipped the test unless a TLSv1.2 compatible version of Python is detected. -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Sailesh Mukil Gerrit-Comment-Date: Tue, 29 May 2018 22:55:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Hello Philip Zeyliger, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10529 to look at the new patch set (#3). Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite a bit. Our RHEL7 machines come equipped with Python 2.7.5. Looking at these comments, that means that we'll be unable to create a 'SSLContext' but be able to explicitly specify ciphers: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L37-L41 # SSLContext is not available for Python < 2.7.9 _has_ssl_context = sys.hexversion >= 0x020709F0 # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 If we cannot create a 'SSLContext', then we cannot use TLSv1.2 and have to use TLSv1: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L48-L49 # For python >= 2.7.9, use latest TLS that both client and server # supports. # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. # For python < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is # unavailable. _default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ ssl.PROTOCOL_TLSv1 Our custom cluster test forces the server to use TLSv1.2 and also forces a specific cipher: https://github.com/apache/impala/blob/master/tests/custom_cluster/test_client_ssl.py#L118-L119 So this combination of configuration values causes a failure in RHEL7 because we only allow a specific cipher which works with TLSv1.2, but the client cannot use TLSv1.2 due to the Python version as mentioned above. We've not noticed these failures on older-than-RHEL7-systems since the OpenSSL versions on those systems don't support TLSv1.2. (< OpenSSL 1.0.1) To fix this, we need to change the Python version on RHEL 7 to be >= Python 2.7.9. This patch skips the test if an older version of Python than 2.7.9 is detected. Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 --- M tests/custom_cluster/test_client_ssl.py 1 file changed, 6 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/10529/3 -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. Patch Set 2: (2 comments) I wrote a comment on the JIRA upstream. I think the easier thing to do is to check Python version and skip if you're using a too-old Python, with an explanation. Possibly add a second copy of the test with the flag changes you suggest. But, regardless, I still don't quite understand whether the test was just failing silently before or something fundamentally broke. http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG@42 PS2, Line 42: which does not force the use of specific ciphers, so we get away without What doesn't force the user of ciphers? http://gerrit.cloudera.org:8080/#/c/10529/2//COMMIT_MSG@49 PS2, Line 49: least unblock our builds while we can upgrade the AMIs for RHEL7. This RHEL7 just doesn't ship a newer Python based on my quick check with Docker. I don't think this will get resolved this way. -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Tue, 29 May 2018 19:50:02 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10529 ) Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite a bit. Our RHEL7 machines come equipped with Python 2.7.5. Looking at these comments, that means that we'll be unable to create a 'SSLContext' but be able to explicitly specify ciphers: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L37-L41 # SSLContext is not available for Python < 2.7.9 _has_ssl_context = sys.hexversion >= 0x020709F0 # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 If we cannot create a 'SSLContext', then we cannot use TLSv1.2 and have to use TLSv1: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L48-L49 # For python >= 2.7.9, use latest TLS that both client and server # supports. # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. # For python < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is # unavailable. _default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ ssl.PROTOCOL_TLSv1 Our custom cluster test forces the server to use TLSv1.2 and also forces a specific cipher: https://github.com/apache/impala/blob/master/tests/custom_cluster/test_client_ssl.py#L118-L119 So this combination of configuration values causes a failure in RHEL7 because we only allow a specific cipher which works with TLSv1.2, but the client cannot use TLSv1.2 due to the Python version as mentioned above. On systems lower than RHEL7, the machines come equipped with Python 2.6.6, which does not force the use of specific ciphers, so we get away without a failure. To fix this, we either need to change the Python version on RHEL 7 to be >= Python 2.7.9, or reduce the 'test_client_ssl' limitation to run TLSv1. The second option is the quickest, although not ideal, but it should at least unblock our builds while we can upgrade the AMIs for RHEL7. This patch does just that. Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 --- M tests/custom_cluster/test_client_ssl.py 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/10529/2 -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 2 Gerrit-Owner: Sailesh Mukil
[Impala-ASF-CR] IMPALA-6990: TestClientSsl.test tls v12 failing due to Python SSL error
Sailesh Mukil has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10529 Change subject: IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error .. IMPALA-6990: TestClientSsl.test_tls_v12 failing due to Python SSL error Spent some more time looking at this and found that 'requests' wasn't the culprit. When we upgraded to thrift-0.9.3, the TSSLSocket.py logic changed quite a bit. Our RHEL7 machines come equipped with Python 2.7.5. Looking at these comments, that means that we'll be unable to create a 'SSLContext' but able to explicitly specify ciphers: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L37-L41 # SSLContext is not available for Python < 2.7.9 _has_ssl_context = sys.hexversion >= 0x020709F0 # ciphers argument is not available for Python < 2.7.0 _has_ciphers = sys.hexversion >= 0x020700F0 If we cannot create a 'SSLContext', then we cannot use TLSv1.2 and have to use TLSv1: https://github.com/apache/thrift/blob/master/lib/py/src/transport/TSSLSocket.py#L48-L49 # For python >= 2.7.9, use latest TLS that both client and server # supports. # SSL 2.0 and 3.0 are disabled via ssl.OP_NO_SSLv2 and ssl.OP_NO_SSLv3. # For python < 2.7.9, use TLS 1.0 since TLSv1_X nor OP_NO_SSLvX is # unavailable. _default_protocol = ssl.PROTOCOL_SSLv23 if _has_ssl_context else \ ssl.PROTOCOL_TLSv1 Our custom cluster test forces the server to use TLSv1.2 and also forces a specific cipher: https://github.com/apache/impala/blob/master/tests/custom_cluster/test_client_ssl.py#L118-L119 So this combination of configuration values causes a failure in RHEL7 because we only allow a specific cipher which works with TLSv1.2, but the client cannot use TLSv1.2 due to the Python version as mentioned above. On systems lower than RHEL7, the machines come equipped with Python 2.6.6, which does not force the use of specific ciphers, so we get away without a failure. To fix this, we either need to change the Python version on RHEL 7 to be >= Python 2.7.9, or reduce the 'test_client_ssl' limitation to run TLSv1. The second option is the quickest, although not ideal, but it should at least unblock our builds while we can upgrade the AMIs for RHEL7. This patch does just that. Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 --- M tests/custom_cluster/test_client_ssl.py 1 file changed, 1 insertion(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/10529/1 -- To view, visit http://gerrit.cloudera.org:8080/10529 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I92c66ecaeb94b0c83ee6f1396c082709c21b3187 Gerrit-Change-Number: 10529 Gerrit-PatchSet: 1 Gerrit-Owner: Sailesh Mukil
