Repository: thrift Updated Branches: refs/heads/master 239233afb -> 06190874c
THRIFT-4084: Add a SSL/TLS negotiation check to crossfeature to verify SSLv3 is not active and that at least one of TLSv1.0 through 1.2 are accepted. Client: csharp, d, go, nodejs, perl This closes #1197 Project: http://git-wip-us.apache.org/repos/asf/thrift/repo Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/06190874 Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/06190874 Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/06190874 Branch: refs/heads/master Commit: 06190874c8ba8f3a0c7ae83a59965d56c205e080 Parents: 239233a Author: James E. King, III <jk...@apache.org> Authored: Mon Feb 20 08:52:11 2017 -0500 Committer: James E. King, III <jk...@apache.org> Committed: Mon Feb 20 08:52:11 2017 -0500 ---------------------------------------------------------------------- .../cpp/src/thrift/generate/t_cpp_generator.cc | 2 +- configure.ac | 6 +- lib/csharp/src/Transport/TTLSServerSocket.cs | 4 +- lib/d/src/thrift/transport/ssl.d | 4 +- lib/go/thrift/ssl_server_socket.go | 5 +- lib/go/thrift/ssl_socket.go | 3 + lib/nodejs/lib/thrift/connection.js | 8 ++- lib/nodejs/lib/thrift/server.js | 6 ++ lib/perl/lib/Thrift/SSLServerSocket.pm | 6 +- lib/perl/lib/Thrift/SSLSocket.pm | 6 +- test/cpp/src/TestClient.cpp | 15 ++++- test/features/Makefile.am | 2 + test/features/nosslv3.sh | 52 ++++++++++++++ test/features/tests.json | 22 ++++++ test/features/tls.sh | 71 ++++++++++++++++++++ test/known_failures_Linux.json | 10 +-- test/tests.json | 9 ++- 17 files changed, 205 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/compiler/cpp/src/thrift/generate/t_cpp_generator.cc ---------------------------------------------------------------------- diff --git a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc index 869d802..c7eec48 100644 --- a/compiler/cpp/src/thrift/generate/t_cpp_generator.cc +++ b/compiler/cpp/src/thrift/generate/t_cpp_generator.cc @@ -4452,4 +4452,4 @@ THRIFT_REGISTER_GENERATOR( " include_prefix: Use full include paths in generated files.\n" " moveable_types: Generate move constructors and assignment operators.\n" " no_ostream_operators:\n" - " Omit generation of ostream definitions.\n"); + " Omit generation of ostream definitions.\n") http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/configure.ac ---------------------------------------------------------------------- diff --git a/configure.ac b/configure.ac index a889f75..bb25495 100755 --- a/configure.ac +++ b/configure.ac @@ -911,8 +911,8 @@ if test "$have_cpp" = "yes" ; then echo "C++ Library:" echo " Build TZlibTransport ...... : $have_zlib" echo " Build TNonblockingServer .. : $have_libevent" - echo " Build TQTcpServer (Qt4) .... : $have_qt" - echo " Build TQTcpServer (Qt5) .... : $have_qt5" + echo " Build TQTcpServer (Qt4) ... : $have_qt" + echo " Build TQTcpServer (Qt5) ... : $have_qt5" fi if test "$have_java" = "yes" ; then echo @@ -1004,7 +1004,7 @@ fi if test "$have_lua" = "yes" ; then echo echo "Lua Library:" - echo " Using Lua .............. : $LUA" + echo " Using Lua ................. : $LUA" fi if test "$have_rs" = "yes" ; then echo http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/csharp/src/Transport/TTLSServerSocket.cs ---------------------------------------------------------------------- diff --git a/lib/csharp/src/Transport/TTLSServerSocket.cs b/lib/csharp/src/Transport/TTLSServerSocket.cs index 86a4494..d51c217 100644 --- a/lib/csharp/src/Transport/TTLSServerSocket.cs +++ b/lib/csharp/src/Transport/TTLSServerSocket.cs @@ -108,7 +108,7 @@ namespace Thrift.Transport X509Certificate2 certificate, RemoteCertificateValidationCallback clientCertValidator = null, LocalCertificateSelectionCallback localCertificateSelectionCallback = null, - // TODO: Enable Tls1 and Tls2 (TLS 1.1 and 1.2) by default once we start using .NET 4.5+. + // TODO: Enable Tls11 and Tls12 (TLS 1.1 and 1.2) by default once we start using .NET 4.5+. SslProtocols sslProtocols = SslProtocols.Tls) { if (!certificate.HasPrivateKey) @@ -126,7 +126,7 @@ namespace Thrift.Transport try { // Create server socket - this.server = TSocketVersionizer.CreateTcpListener(port); + this.server = TSocketVersionizer.CreateTcpListener(port); this.server.Server.NoDelay = true; } catch (Exception) http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/d/src/thrift/transport/ssl.d ---------------------------------------------------------------------- diff --git a/lib/d/src/thrift/transport/ssl.d b/lib/d/src/thrift/transport/ssl.d index a78a2ed..fbcb6ee 100644 --- a/lib/d/src/thrift/transport/ssl.d +++ b/lib/d/src/thrift/transport/ssl.d @@ -249,7 +249,9 @@ class TSSLContext { } count_++; - ctx_ = SSL_CTX_new(TLSv1_method()); + ctx_ = SSL_CTX_new(SSLv23_method()); + SSL_CTX_set_options(ctx_, SSL_OP_NO_SSLv2); + SSL_CTX_set_options(ctx_, SSL_OP_NO_SSLv3); // THRIFT-3164 enforce(ctx_, getSSLException("SSL_CTX_new")); SSL_CTX_set_mode(ctx_, SSL_MODE_AUTO_RETRY); } http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/go/thrift/ssl_server_socket.go ---------------------------------------------------------------------- diff --git a/lib/go/thrift/ssl_server_socket.go b/lib/go/thrift/ssl_server_socket.go index 58f859b..907afca 100644 --- a/lib/go/thrift/ssl_server_socket.go +++ b/lib/go/thrift/ssl_server_socket.go @@ -20,9 +20,9 @@ package thrift import ( + "crypto/tls" "net" "time" - "crypto/tls" ) type TSSLServerSocket struct { @@ -38,6 +38,9 @@ func NewTSSLServerSocket(listenAddr string, cfg *tls.Config) (*TSSLServerSocket, } func NewTSSLServerSocketTimeout(listenAddr string, cfg *tls.Config, clientTimeout time.Duration) (*TSSLServerSocket, error) { + if cfg.MinVersion == 0 { + cfg.MinVersion = tls.VersionTLS10 + } addr, err := net.ResolveTCPAddr("tcp", listenAddr) if err != nil { return nil, err http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/go/thrift/ssl_socket.go ---------------------------------------------------------------------- diff --git a/lib/go/thrift/ssl_socket.go b/lib/go/thrift/ssl_socket.go index 04d3850..2395986 100644 --- a/lib/go/thrift/ssl_socket.go +++ b/lib/go/thrift/ssl_socket.go @@ -48,6 +48,9 @@ func NewTSSLSocket(hostPort string, cfg *tls.Config) (*TSSLSocket, error) { // NewTSSLSocketTimeout creates a net.Conn-backed TTransport, given a host and port // it also accepts a tls Configuration and a timeout as a time.Duration func NewTSSLSocketTimeout(hostPort string, cfg *tls.Config, timeout time.Duration) (*TSSLSocket, error) { + if cfg.MinVersion == 0 { + cfg.MinVersion = tls.VersionTLS10 + } return &TSSLSocket{hostPort: hostPort, timeout: timeout, cfg: cfg}, nil } http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/nodejs/lib/thrift/connection.js ---------------------------------------------------------------------- diff --git a/lib/nodejs/lib/thrift/connection.js b/lib/nodejs/lib/thrift/connection.js index 575d516..608e552 100644 --- a/lib/nodejs/lib/thrift/connection.js +++ b/lib/nodejs/lib/thrift/connection.js @@ -18,6 +18,7 @@ */ var util = require('util'); var EventEmitter = require("events").EventEmitter; +var constants = require('constants'); var net = require('net'); var tls = require('tls'); var thrift = require('./thrift'); @@ -221,7 +222,7 @@ Connection.prototype.connection_gone = function () { this.retry_timer = setTimeout(function () { if (self._debug) { console.log("Retrying connection..."); - } + } self.retry_totaltime += self.retry_delay; @@ -247,6 +248,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 = constants.SSL_OP_NO_SSLv2 | constants.SSL_OP_NO_SSLv3; + } + var stream = tls.connect(port, host, options); var connection = new Connection(stream, options); connection.host = host; http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/nodejs/lib/thrift/server.js ---------------------------------------------------------------------- diff --git a/lib/nodejs/lib/thrift/server.js b/lib/nodejs/lib/thrift/server.js index 921bb86..e124acc 100644 --- a/lib/nodejs/lib/thrift/server.js +++ b/lib/nodejs/lib/thrift/server.js @@ -16,6 +16,8 @@ * specific language governing permissions and limitations * under the License. */ + +var constants = require('constants'); var net = require('net'); var tls = require('tls'); @@ -87,6 +89,10 @@ exports.createMultiplexServer = function(processor, options) { } if (options && options.tls) { + if (!('secureProtocol' in options.tls) && !('secureOptions' in options.tls)) { + options.tls.secureProtocol = "SSLv23_method"; + options.tls.secureOptions = constants.SSL_OP_NO_SSLv2 | constants.SSL_OP_NO_SSLv3; + } return tls.createServer(options.tls, serverImpl); } else { return net.createServer(serverImpl); http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/perl/lib/Thrift/SSLServerSocket.pm ---------------------------------------------------------------------- diff --git a/lib/perl/lib/Thrift/SSLServerSocket.pm b/lib/perl/lib/Thrift/SSLServerSocket.pm index e885ede..a8dfa56 100644 --- a/lib/perl/lib/Thrift/SSLServerSocket.pm +++ b/lib/perl/lib/Thrift/SSLServerSocket.pm @@ -60,13 +60,15 @@ sub __listen Proto => 'tcp', ReuseAddr => 1}; + my $verify = IO::Socket::SSL::SSL_VERIFY_PEER | IO::Socket::SSL::SSL_VERIFY_FAIL_IF_NO_PEER_CERT | IO::Socket::SSL::SSL_VERIFY_CLIENT_ONCE; + $opts->{SSL_ca_file} = $self->{ca} if defined $self->{ca}; $opts->{SSL_cert_file} = $self->{cert} if defined $self->{cert}; $opts->{SSL_cipher_list} = $self->{ciphers} if defined $self->{ciphers}; $opts->{SSL_key_file} = $self->{key} if defined $self->{key}; $opts->{SSL_use_cert} = (defined $self->{cert}) ? 1 : 0; - $opts->{SSL_verify_mode} = (defined $self->{ca}) ? IO::Socket::SSL::SSL_VERIFY_PEER : IO::Socket::SSL::SSL_VERIFY_NONE; - $opts->{SSL_version} = (defined $self->{version}) ? $self->{version} : 'SSLv23:!SSLv2:!SSLv3'; + $opts->{SSL_verify_mode} = (defined $self->{ca}) ? $verify : IO::Socket::SSL::SSL_VERIFY_NONE; + $opts->{SSL_version} = (defined $self->{version}) ? $self->{version} : 'SSLv23:!SSLv3:!SSLv2'; return IO::Socket::SSL->new(%$opts); } http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/lib/perl/lib/Thrift/SSLSocket.pm ---------------------------------------------------------------------- diff --git a/lib/perl/lib/Thrift/SSLSocket.pm b/lib/perl/lib/Thrift/SSLSocket.pm index 046692e..99a4107 100644 --- a/lib/perl/lib/Thrift/SSLSocket.pm +++ b/lib/perl/lib/Thrift/SSLSocket.pm @@ -71,13 +71,15 @@ sub __open Proto => 'tcp', Timeout => $self->{sendTimeout} / 1000}; + my $verify = IO::Socket::SSL::SSL_VERIFY_PEER | IO::Socket::SSL::SSL_VERIFY_FAIL_IF_NO_PEER_CERT | IO::Socket::SSL::SSL_VERIFY_CLIENT_ONCE; + $opts->{SSL_ca_file} = $self->{ca} if defined $self->{ca}; $opts->{SSL_cert_file} = $self->{cert} if defined $self->{cert}; $opts->{SSL_cipher_list} = $self->{ciphers} if defined $self->{ciphers}; $opts->{SSL_key_file} = $self->{key} if defined $self->{key}; $opts->{SSL_use_cert} = (defined $self->{cert}) ? 1 : 0; - $opts->{SSL_verify_mode} = (defined $self->{ca}) ? IO::Socket::SSL::SSL_VERIFY_PEER : IO::Socket::SSL::SSL_VERIFY_NONE; - $opts->{SSL_version} = (defined $self->{version}) ? $self->{version} : 'SSLv23:!SSLv2:!SSLv3'; + $opts->{SSL_verify_mode} = (defined $self->{ca}) ? $verify : IO::Socket::SSL::SSL_VERIFY_NONE; + $opts->{SSL_version} = (defined $self->{version}) ? $self->{version} : 'SSLv23:!SSLv3:!SSLv2'; return IO::Socket::SSL->new(%$opts); } http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/cpp/src/TestClient.cpp ---------------------------------------------------------------------- diff --git a/test/cpp/src/TestClient.cpp b/test/cpp/src/TestClient.cpp index da20b89..a918bfb 100644 --- a/test/cpp/src/TestClient.cpp +++ b/test/cpp/src/TestClient.cpp @@ -136,8 +136,11 @@ int main(int argc, char** argv) { int ERR_EXCEPTIONS = 8; int ERR_UNKNOWN = 64; - string testDir = boost::filesystem::system_complete(argv[0]).parent_path().parent_path().parent_path().string(); - string pemPath = testDir + "/keys/CA.pem"; + string testDir = boost::filesystem::system_complete(argv[0]).parent_path().parent_path().parent_path().string(); + string caPath = testDir + "/keys/CA.pem"; + string certPath = testDir + "/keys/client.crt"; + string keyPath = testDir + "/keys/client.key"; + #if _WIN32 transport::TWinsockSingleton::create(); #endif @@ -232,9 +235,15 @@ int main(int argc, char** argv) { boost::shared_ptr<TSSLSocketFactory> factory; if (ssl) { + cout << "Client Certificate File: " << certPath << endl; + cout << "Client Key File: " << keyPath << endl; + cout << "CA File: " << caPath << endl; + factory = boost::shared_ptr<TSSLSocketFactory>(new TSSLSocketFactory()); factory->ciphers("ALL:!ADH:!LOW:!EXP:!MD5:@STRENGTH"); - factory->loadTrustedCertificates(pemPath.c_str()); + factory->loadTrustedCertificates(caPath.c_str()); + factory->loadCertificate(certPath.c_str()); + factory->loadPrivateKey(keyPath.c_str()); factory->authenticate(true); socket = factory->createSocket(host, port); } else { http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/features/Makefile.am ---------------------------------------------------------------------- diff --git a/test/features/Makefile.am b/test/features/Makefile.am index f8d6538..337d789 100644 --- a/test/features/Makefile.am +++ b/test/features/Makefile.am @@ -21,8 +21,10 @@ EXTRA_DIST = \ index.html \ known_failures_Linux.json \ Makefile.am \ + nosslv3.sh \ string_limit.py \ tests.json \ theader_binary.py \ setup.cfg \ + tls.sh \ util.py http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/features/nosslv3.sh ---------------------------------------------------------------------- diff --git a/test/features/nosslv3.sh b/test/features/nosslv3.sh new file mode 100755 index 0000000..e550d51 --- /dev/null +++ b/test/features/nosslv3.sh @@ -0,0 +1,52 @@ +#!/bin/bash + +# +# Checks to make sure SSLv3 is not allowed by a server. +# + +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 + +function nosslv3 +{ + local nego + local negodenied + + # echo "openssl s_client -connect $THRIFTHOST:$THRIFTPORT -CAfile ../keys/CA.pem -ssl3 2>&1 < /dev/null" + nego=$(openssl s_client -connect $THRIFTHOST:$THRIFTPORT -CAfile ../keys/CA.pem -ssl3 2>&1 < /dev/null) + negodenied=$? + + if [[ $negodenied -ne 0 ]]; then + echo "[pass] SSLv3 negotiation disabled" + echo $nego + return 0 + fi + + echo "[fail] SSLv3 negotiation enabled! stdout:" + echo $nego + return 1 +} + +nosslv3 +exit $? http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/features/tests.json ---------------------------------------------------------------------- diff --git a/test/features/tests.json b/test/features/tests.json index cfcb4b6..3ab3b68 100644 --- a/test/features/tests.json +++ b/test/features/tests.json @@ -90,5 +90,27 @@ "transports": ["buffered"], "sockets": ["ip"], "workdir": "features" + }, + { + "name": "nosslv3", + "comment": "check to make sure SSLv3 is not supported", + "command": [ + "nosslv3.sh" + ], + "protocols": ["binary"], + "transports": ["buffered"], + "sockets": ["ip-ssl"], + "workdir": "features" + }, + { + "name": "tls", + "comment": "check to make sure TLSv1.0 or later is supported", + "command": [ + "tls.sh" + ], + "protocols": ["binary"], + "transports": ["buffered"], + "sockets": ["ip-ssl"], + "workdir": "features" } ] http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/features/tls.sh ---------------------------------------------------------------------- diff --git a/test/features/tls.sh b/test/features/tls.sh new file mode 100755 index 0000000..dada6ab --- /dev/null +++ b/test/features/tls.sh @@ -0,0 +1,71 @@ +#!/bin/bash + +# +# Checks to make sure TLSv1.0 or later is allowed by a server. +# + +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 + +declare -A EXPECT_NEGOTIATE +EXPECT_NEGOTIATE[tls1]=1 +EXPECT_NEGOTIATE[tls1_1]=1 +EXPECT_NEGOTIATE[tls1_2]=1 + +failures=0 + +function tls +{ + for PROTO in "${!EXPECT_NEGOTIATE[@]}"; do + + local nego + local negodenied + local res + + echo "openssl s_client -connect $THRIFTHOST:$THRIFTPORT -CAfile ../keys/CA.pem -$PROTO 2>&1 < /dev/null" + nego=$(openssl s_client -connect $THRIFTHOST:$THRIFTPORT -CAfile ../keys/CA.pem -$PROTO 2>&1 < /dev/null) + negodenied=$? + echo "result of command: $negodenied" + + res="enabled"; if [[ ${EXPECT_NEGOTIATE[$PROTO]} -eq 0 ]]; then res="disabled"; fi + + if [[ $negodenied -ne ${EXPECT_NEGOTIATE[$PROTO]} ]]; then + echo "$PROTO negotiation allowed" + else + echo "[warn] $PROTO negotiation did not work" + echo $nego + ((failures++)) + fi + done +} + +tls + +if [[ $failures -eq 3 ]]; then + echo "[fail] At least one of TLSv1.0, TLSv1.1, or TLSv1.2 needs to work, but does not" + exit $failures +fi + +echo "[pass] At least one of TLSv1.0, TLSv1.1, or TLSv1.2 worked" +exit 0 http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/known_failures_Linux.json ---------------------------------------------------------------------- diff --git a/test/known_failures_Linux.json b/test/known_failures_Linux.json index d60997d..5ca6d4e 100644 --- a/test/known_failures_Linux.json +++ b/test/known_failures_Linux.json @@ -47,6 +47,9 @@ "csharp-d_binary_buffered-ip-ssl", "csharp-d_compact_buffered-ip-ssl", "csharp-d_json_buffered-ip-ssl", + "csharp-d_binary_framed-ip-ssl", + "csharp-d_compact_framed-ip-ssl", + "csharp-d_json_framed-ip-ssl", "csharp-erl_binary_buffered-ip-ssl", "csharp-erl_binary_framed-ip-ssl", "csharp-erl_compact_buffered-ip-ssl", @@ -105,6 +108,7 @@ "d-cpp_json_http-ip-ssl", "d-d_binary_http-ip", "d-d_compact_http-ip", + "d-d_json_http-ip", "d-dart_binary_framed-ip", "d-dart_binary_http-ip", "d-dart_compact_framed-ip", @@ -220,9 +224,5 @@ "hs-py3_json_framed-ip", "java-d_compact_buffered-ip", "java-d_compact_buffered-ip-ssl", - "java-d_compact_framed-ip", - "perl-erl_binary_buffered-ip-ssl", - "perl-erl_binary_framed-ip-ssl", - "perl-perl_binary_buffered-ip-ssl", - "perl-perl_binary_framed-ip-ssl" + "java-d_compact_framed-ip" ] http://git-wip-us.apache.org/repos/asf/thrift/blob/06190874/test/tests.json ---------------------------------------------------------------------- diff --git a/test/tests.json b/test/tests.json index e228928..f1d6a47 100644 --- a/test/tests.json +++ b/test/tests.json @@ -121,8 +121,8 @@ "framed:fastframed" ], "sockets": [ - "ip-ssl", - "ip" + "ip", + "ip-ssl" ], "protocols": [ "compact", @@ -415,7 +415,7 @@ "-I../../lib/perl/lib/", "TestClient.pl", "--ca=../keys/CA.pem", - "--cert=../keys/client.pem", + "--cert=../keys/client.crt", "--key=../keys/client.key" ] }, @@ -425,8 +425,7 @@ "-Igen-perl/", "-I../../lib/perl/lib/", "TestServer.pl", - "--ca=../keys/CA.pem", - "--cert=../keys/server.pem", + "--cert=../keys/server.crt", "--key=../keys/server.key" ] },