You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jeking3 <gi...@git.apache.org> on 2017/02/19 01:48:17 UTC
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/1197
THRIFT-4084: Add a SSL/TLS negotiation check to crosstest to verify SSLv3 is not active and that TLSv1.0 through 1.2 are accepted
Fixed the following server implementations to properly disable SSLv3 when using default settings:
go, nodejs, perl, py3
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift THRIFT-4084
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1197.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1197
----
commit 608329ba5cba941c67205f6e453b1173b6cd9c21
Author: James E. King, III <ji...@simplivity.com>
Date: 2017-02-12T22:53:05Z
THRIFT-4084: Add a SSL/TLS negotiation check to crosstest to verify SSLv3 is not active and that TLSv1.0 through 1.2 are accepted.
Client: go, nodejs, perl, python
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101907113
--- Diff: test/tests.json ---
@@ -606,5 +606,23 @@
"compact"
],
"workdir": "rs/bin"
+ },
+ {
+ "name": "secure",
+ "client": {
+ "command": [
+ "test_secure.bash"
+ ]
+ },
+ "transports": [
+ "buffered"
+ ],
+ "sockets": [
+ "ip-ssl"
+ ],
+ "protocols": [
+ "binary"
+ ],
+ "workdir": "secure"
--- End diff --
What do you think about moving them to test/features/tests.json ? Since it's not really a language binding.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101909034
--- Diff: lib/py/src/transport/sslcompat.py ---
@@ -66,6 +66,14 @@ def legacy_validate_callback(cert, hostname):
--- End diff --
In the Ubuntu 14.04 LTS docker image, Python is version 2.7.6. This means it doesn't have SSLContext. This code detects that condition and sets a single version that it can use. I am actually going to change the default to TLSv1.2 for security purposes here. Any language that cannot properly modify the openssl options to negotiate TLSv1.0 "or higher" should be forced to TLSv1.2. From what I can tell, csharp, d, and py2 (in the build image) lack the necessary code/knowledge to allow for a SSLv23 handshake and then autonegotiate TLSv1.0 "or higher".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101914893
--- Diff: lib/py/src/transport/sslcompat.py ---
@@ -66,6 +66,14 @@ def legacy_validate_callback(cert, hostname):
--- End diff --
I backed out my changes to lib/py and will re-test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101908692
--- Diff: lib/go/thrift/ssl_socket.go ---
@@ -20,152 +20,155 @@
package thrift
--- End diff --
Sorry, I'm using the geany editor and I have it set to use 2 character spacing to match the rest of the codebase (C++ mostly). I'll undo the whitespace changes here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101908015
--- Diff: lib/go/thrift/ssl_socket.go ---
@@ -20,152 +20,155 @@
package thrift
--- End diff --
See https://github.com/golang/go/commit/014f3dcc837cb6789076cff4fccaa3bd221f823e
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1197
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101914898
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -247,6 +247,11 @@ exports.createConnection = function(host, port, options) {
};
exports.createSSLConnection = function(host, port, options) {
+ if (!('secureProtocol' in options) && !('secureOptions' in options)) {
+ options.secureProtocol = "SSLv23_method";
+ options.secureOptions = 0x03000000;
--- End diff --
I made this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101914676
--- Diff: test/tests.json ---
@@ -606,5 +606,23 @@
"compact"
],
"workdir": "rs/bin"
+ },
+ {
+ "name": "secure",
+ "client": {
+ "command": [
+ "test_secure.bash"
+ ]
+ },
+ "transports": [
+ "buffered"
+ ],
+ "sockets": [
+ "ip-ssl"
+ ],
+ "protocols": [
+ "binary"
+ ],
+ "workdir": "secure"
--- End diff --
I do not see where crossfeature is executed during Travis CI. Which build job runs it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101915787
--- Diff: test/secure/test_secure.bash ---
@@ -0,0 +1,69 @@
+#!/bin/bash
+
+#
+# Checks various desired attributes in SSL/TLS implementations.
+#
+
+THRIFTHOST=localhost
+THRIFTPORT=9090
+
+while [[ $# -ge 1 ]]; do
+ arg="$1"
+ argIN=(${arg//=/ })
+
+ case ${argIN[0]} in
+ -h|--host)
+ THRIFTHOST=${argIN[1]}
+ shift # past argument
+ ;;
+ -p|--port)
+ THRIFTPORT=${argIN[1]}
+ shift # past argument
+ ;;
+ *)
+ # unknown option ignored
+ ;;
+ esac
+
+ shift # past argument or value
+done
+
+#
+# Negotiation Test Expectations
+#
+
+declare -A EXPECT_NEGOTIATE
+EXPECT_NEGOTIATE[ssl3]=0
+EXPECT_NEGOTIATE[tls1]=1
+EXPECT_NEGOTIATE[tls1_1]=1
+EXPECT_NEGOTIATE[tls1_2]=1
--- End diff --
I moved these into features/ as you requested, split them apart, and made the TLS check for now pass if any of the TLSv1.1, TLSv1.1, or TLSv1.2 is possible. For "py" only TLSv1.0 is possible, as with csharp.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101908742
--- Diff: test/tests.json ---
@@ -606,5 +606,23 @@
"compact"
],
"workdir": "rs/bin"
+ },
+ {
+ "name": "secure",
+ "client": {
+ "command": [
+ "test_secure.bash"
+ ]
+ },
+ "transports": [
+ "buffered"
+ ],
+ "sockets": [
+ "ip-ssl"
+ ],
+ "protocols": [
+ "binary"
+ ],
+ "workdir": "secure"
--- End diff --
I'm not certain how that is used and whether it runs as part of Travis CI. Is there any more documentation for the test/features area?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101907637
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -247,6 +247,11 @@ exports.createConnection = function(host, port, options) {
};
exports.createSSLConnection = function(host, port, options) {
+ if (!('secureProtocol' in options) && !('secureOptions' in options)) {
+ options.secureProtocol = "SSLv23_method";
+ options.secureOptions = 0x03000000;
--- End diff --
Would be more readable with `constants.SSL_OP_NO_SSLv2 | constants.SSL_OP_NO_SSLv3` after requiring 'constants' at the top...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101909066
--- Diff: test/secure/test_secure.bash ---
@@ -0,0 +1,69 @@
+#!/bin/bash
+
+#
+# Checks various desired attributes in SSL/TLS implementations.
+#
+
+THRIFTHOST=localhost
+THRIFTPORT=9090
+
+while [[ $# -ge 1 ]]; do
+ arg="$1"
+ argIN=(${arg//=/ })
+
+ case ${argIN[0]} in
+ -h|--host)
+ THRIFTHOST=${argIN[1]}
+ shift # past argument
+ ;;
+ -p|--port)
+ THRIFTPORT=${argIN[1]}
+ shift # past argument
+ ;;
+ *)
+ # unknown option ignored
+ ;;
+ esac
+
+ shift # past argument or value
+done
+
+#
+# Negotiation Test Expectations
+#
+
+declare -A EXPECT_NEGOTIATE
+EXPECT_NEGOTIATE[ssl3]=0
+EXPECT_NEGOTIATE[tls1]=1
+EXPECT_NEGOTIATE[tls1_1]=1
+EXPECT_NEGOTIATE[tls1_2]=1
--- End diff --
What I'm thinking here is that nothing should allow for SSLv3 as you suggested, however it would be acceptable for csharp and d languages to only allow TLSv1.2, since they lack the ability to specify TLSv1.0 "or later". So I can relax this test to say, "Accept either TLSv1.0 or later, or TLSv1.2 alone, but never allow SSLv3". Really I could just simplify the test down to checking that SSLv3 is not allowed and TLSv1.2 works at this point.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101909373
--- Diff: test/secure/test_secure.bash ---
@@ -0,0 +1,69 @@
+#!/bin/bash
+
+#
+# Checks various desired attributes in SSL/TLS implementations.
+#
+
+THRIFTHOST=localhost
+THRIFTPORT=9090
+
+while [[ $# -ge 1 ]]; do
+ arg="$1"
+ argIN=(${arg//=/ })
+
+ case ${argIN[0]} in
+ -h|--host)
+ THRIFTHOST=${argIN[1]}
+ shift # past argument
+ ;;
+ -p|--port)
+ THRIFTPORT=${argIN[1]}
+ shift # past argument
+ ;;
+ *)
+ # unknown option ignored
+ ;;
+ esac
+
+ shift # past argument or value
+done
+
+#
+# Negotiation Test Expectations
+#
+
+declare -A EXPECT_NEGOTIATE
+EXPECT_NEGOTIATE[ssl3]=0
+EXPECT_NEGOTIATE[tls1]=1
+EXPECT_NEGOTIATE[tls1_1]=1
+EXPECT_NEGOTIATE[tls1_2]=1
--- End diff --
Sounds reasonable to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101907094
--- Diff: lib/go/thrift/ssl_socket.go ---
@@ -20,152 +20,155 @@
package thrift
--- End diff --
We cannot replace indentation with spaces because, like it or not, tabs are the right indentation in Go world.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101907745
--- Diff: test/secure/test_secure.bash ---
@@ -0,0 +1,69 @@
+#!/bin/bash
+
+#
+# Checks various desired attributes in SSL/TLS implementations.
+#
+
+THRIFTHOST=localhost
+THRIFTPORT=9090
+
+while [[ $# -ge 1 ]]; do
+ arg="$1"
+ argIN=(${arg//=/ })
+
+ case ${argIN[0]} in
+ -h|--host)
+ THRIFTHOST=${argIN[1]}
+ shift # past argument
+ ;;
+ -p|--port)
+ THRIFTPORT=${argIN[1]}
+ shift # past argument
+ ;;
+ *)
+ # unknown option ignored
+ ;;
+ esac
+
+ shift # past argument or value
+done
+
+#
+# Negotiation Test Expectations
+#
+
+declare -A EXPECT_NEGOTIATE
+EXPECT_NEGOTIATE[ssl3]=0
+EXPECT_NEGOTIATE[tls1]=1
+EXPECT_NEGOTIATE[tls1_1]=1
+EXPECT_NEGOTIATE[tls1_2]=1
--- End diff --
Maybe split to multiple `tests.json` entries for no-ssl3 (and 2 ?) and tls 1.x tests ?
We wouldn't want any of the former in known_failures.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101909371
--- Diff: test/tests.json ---
@@ -606,5 +606,23 @@
"compact"
],
"workdir": "rs/bin"
+ },
+ {
+ "name": "secure",
+ "client": {
+ "command": [
+ "test_secure.bash"
+ ]
+ },
+ "transports": [
+ "buffered"
+ ],
+ "sockets": [
+ "ip-ssl"
+ ],
+ "protocols": [
+ "binary"
+ ],
+ "workdir": "secure"
--- End diff --
Simply moving the same JSON entry (with relative file path adjustment) might get this included to `make crossfeature`.
In that case, known_failure file is also in `features/known_...`.
Otherwise we can leave it as is and plan to relocate later.
I guess I'll need to put some information to `test/README.md` or create `test/features/README.md`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101907319
--- Diff: lib/py/src/transport/sslcompat.py ---
@@ -66,6 +66,14 @@ def legacy_validate_callback(cert, hostname):
--- End diff --
Is it to introduce backports.ssl as an optional dependency for older Pythons ?
While it would make sense, there are some problematic details here.
I recommend doing it in a separate patch if you think it's worth it.
python2-secure is being listed in known_failures so this wouldn't affect test results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101909372
--- Diff: lib/py/src/transport/sslcompat.py ---
@@ -66,6 +66,14 @@ def legacy_validate_callback(cert, hostname):
--- End diff --
As far as I can see, this does not change any of our docker image behaviors for python:
SSLv23 with NO_SSL* is being used for python 3 and debian python 2.
About defaulting to TLSv1.2, Python < 2.7.9 and C# with .NET 3.5 compat do not seem to have TLS 1.1 and 1.2 at all.
The highest versions listed in doc is TLSv1.0.
For C#, once dropping .NET 3.5, it would be able to support TLS 1.0, 1.1 and 1.2 at once because SslProtocols is an explicit "flags" enum that can be combined with pipe like `Tls12 | Tls11 | Tls`.
Though I agree that it's not possible to do forward compatible "SSLv23 minus SSL 2 and 3".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1197: THRIFT-4084: Add a SSL/TLS negotiation check to c...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1197#discussion_r101914903
--- Diff: test/tests.json ---
@@ -606,5 +606,23 @@
"compact"
],
"workdir": "rs/bin"
+ },
+ {
+ "name": "secure",
+ "client": {
+ "command": [
+ "test_secure.bash"
+ ]
+ },
+ "transports": [
+ "buffered"
+ ],
+ "sockets": [
+ "ip-ssl"
+ ],
+ "protocols": [
+ "binary"
+ ],
+ "workdir": "secure"
--- End diff --
I found it. I will move it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1197: THRIFT-4084: Add a SSL/TLS negotiation check to crossfea...
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1197
@nsuke this is ready for another review as all the code comments were addressed and we have a clean CI build.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---