From afada8913d876ddb2d8673a422d04b8927bdc222 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Fri, 21 Feb 2020 18:01:21 -0500 Subject: [PATCH 01/11] outline expectations --- spec/mongo/socket/ssl_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index 57c0769dbe..4a08aa6ff8 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -581,6 +581,13 @@ end it 'fails' do + # This test provides a second level client certificate to the + # server *without* providing the intermediate certificate. + # If the server performs certificate verification, it will + # reject the connection (seen from the driver as a SocketError) + # and the test will succeed. If the server does not perform + # certificate verification, it will accept the connection, + # no SocketError will be raised and the test will fail. connection expect do connection.connect! From 048aa72d6a474e35c61e4799b808f75282d23244 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Fri, 21 Feb 2020 18:17:00 -0500 Subject: [PATCH 02/11] try testing via mlaunch to get server to verify client certificates --- .evergreen/config.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.evergreen/config.yml b/.evergreen/config.yml index 838d14137b..f054c7ef8b 100644 --- a/.evergreen/config.yml +++ b/.evergreen/config.yml @@ -729,10 +729,10 @@ buildvariants: ruby: "jruby-9.2" mongodb-version: "4.2" topology: "*" - os: rhel70 + os: ubuntu1604 display_name: "${mongodb-version} ${topology} ${auth-and-ssl} ${ruby}" tasks: - - name: "test" + - name: "test-mlaunch" - matrix_name: "jruby-noauth" @@ -741,10 +741,10 @@ buildvariants: ruby: "jruby-9.2" mongodb-version: "2.6" topology: "*" - os: rhel70 + os: ubuntu1604 display_name: "${mongodb-version} ${topology} ${auth-and-ssl} ${ruby}" tasks: - - name: "test" + - name: "test-mlaunch" - matrix_name: "zlib-auth" From 8447ccef5f6e82973d972a0ca1a7c2521a66be83 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sat, 22 Feb 2020 13:37:02 -0500 Subject: [PATCH 03/11] rename options to ssl_options --- spec/mongo/socket/ssl_spec.rb | 78 +++++++++++++++++------------------ 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index 4a08aa6ff8..d1b76e82e9 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -20,10 +20,10 @@ end let(:socket) do - resolver.socket(socket_timeout, options) + resolver.socket(socket_timeout, ssl_options) end - let(:options) do + let(:ssl_options) do SpecConfig.instance.ssl_options end @@ -82,7 +82,7 @@ context 'when a certificate and key are provided as strings' do - let(:options) do + let(:ssl_options) do { :ssl => true, :ssl_cert_string => cert_string, @@ -99,7 +99,7 @@ context 'when certificate and an encrypted key are provided as strings' do require_local_tls - let(:options) do + let(:ssl_options) do { :ssl => true, :ssl_cert_string => cert_string, @@ -116,7 +116,7 @@ context 'when a certificate and key are provided as objects' do - let(:options) do + let(:ssl_options) do { :ssl => true, :ssl_cert_object => cert_object, @@ -132,7 +132,7 @@ context 'when the certificate is specified using both a file and a PEM-encoded string' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_cert_string => 'This is a random string, not a PEM-encoded certificate' ) @@ -146,7 +146,7 @@ context 'when the certificate is specified using both a file and an object' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_cert_object => 'This is a string, not a certificate' ) @@ -160,7 +160,7 @@ context 'when the certificate is specified using both a PEM-encoded string and an object' do - let(:options) do + let(:ssl_options) do { :ssl => true, :ssl_cert_string => cert_string, @@ -178,7 +178,7 @@ context 'when the key is specified using both a file and a PEM-encoded string' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_key_string => 'This is a normal string, not a PEM-encoded key' ) @@ -192,7 +192,7 @@ context 'when the key is specified using both a file and an object' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_cert_object => 'This is a string, not a key' ) @@ -206,7 +206,7 @@ context 'when the key is specified using both a PEM-encoded string and an object' do - let(:options) do + let(:ssl_options) do { :ssl => true, :ssl_cert => SpecConfig.instance.client_cert_path, @@ -224,7 +224,7 @@ context 'when a certificate is passed, but it is not of the right type' do - let(:options) do + let(:ssl_options) do cert = "This is a string, not an X.509 Certificate" { :ssl => true, @@ -256,7 +256,7 @@ host_name, 30, ::Socket::PF_INET, - options.merge(ssl_verify: false, ssl_verify_hostname: true) + ssl_options.merge(ssl_verify: false, ssl_verify_hostname: true) ) rescue => e error = e @@ -276,7 +276,7 @@ host_name, 30, ::Socket::PF_INET, - options.merge(ssl_verify: false, ssl_verify_hostname: false) + ssl_options.merge(ssl_verify: false, ssl_verify_hostname: false) ) }.not_to raise_error end @@ -288,7 +288,7 @@ context 'when a key is passed, but it is not of the right type' do - let(:options) do + let(:ssl_options) do key = "This is a string not a key" { :ssl => true, @@ -315,7 +315,7 @@ context 'when a key is passed, but it is not of the right type' do - let(:options) do + let(:ssl_options) do key = "This is a string not a key" { :ssl => true, @@ -349,7 +349,7 @@ context 'when a bad certificate is provided' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_key => COMMAND_MONITORING_TESTS.first ) @@ -384,7 +384,7 @@ context 'as a path to a file' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => SpecConfig.instance.local_ca_cert_path, :ssl_verify => true @@ -398,7 +398,7 @@ context 'as a string containing the PEM-encoded certificate' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert_string => ca_cert_string, :ssl_verify => true @@ -411,7 +411,7 @@ end context 'as an array of Certificate objects' do - let(:options) do + let(:ssl_options) do cert = [OpenSSL::X509::Certificate.new(ca_cert_string)] super().merge( :ssl_ca_cert_object => cert, @@ -426,7 +426,7 @@ context 'both as a file and a PEM-encoded parameter' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => SpecConfig.instance.local_ca_cert_path, :ssl_ca_cert_string => 'This is a string, not a certificate', @@ -442,7 +442,7 @@ context 'both as a file and as object parameter' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => SpecConfig.instance.local_ca_cert_path, :ssl_ca_cert_object => 'This is a string, not an array of certificates', @@ -457,7 +457,7 @@ context 'both as a PEM-encoded string and as object parameter' do - let(:options) do + let(:ssl_options) do cert = File.read(SpecConfig.instance.local_ca_cert_path) super().merge( :ssl_ca_cert_string => cert, @@ -480,11 +480,11 @@ end let(:connection) do - Mongo::Server::Connection.new(server, options.merge(socket_timeout: 2)) + Mongo::Server::Connection.new(server, ssl_options.merge(socket_timeout: 2)) end context 'as a file' do - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl: true, ssl_cert: SpecConfig.instance.client_cert_path, @@ -511,11 +511,11 @@ end let(:connection) do - Mongo::Server::Connection.new(server, options.merge(socket_timeout: 2)) + Mongo::Server::Connection.new(server, ssl_options.merge(socket_timeout: 2)) end context 'as a file' do - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl: true, ssl_cert: SpecConfig.instance.client_cert_path, @@ -537,7 +537,7 @@ context 'when a CA certificate is not provided' do require_local_tls - let(:options) do + let(:ssl_options) do super().merge( :ssl_verify => true ) @@ -566,12 +566,12 @@ end let(:connection) do - Mongo::Server::Connection.new(server, options.merge(socket_timeout: 2)) + Mongo::Server::Connection.new(server, ssl_options.merge(socket_timeout: 2)) end context 'as a path to a file' do context 'standalone' do - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl_cert: SpecConfig.instance.second_level_cert_path, ssl_key: SpecConfig.instance.second_level_key_path, @@ -600,7 +600,7 @@ # https://github.com/jruby/jruby-openssl/issues/181 only_mri - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl: true, ssl_cert: SpecConfig.instance.second_level_cert_bundle_path, @@ -621,7 +621,7 @@ context 'as a string' do context 'standalone' do - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl_cert: nil, ssl_cert_string: File.read(SpecConfig.instance.second_level_cert_path), @@ -645,7 +645,7 @@ # https://github.com/jruby/jruby-openssl/issues/181 only_mri - let(:options) do + let(:ssl_options) do SpecConfig.instance.test_options.merge( ssl: true, ssl_cert: nil, @@ -675,11 +675,11 @@ end let(:connection) do - Mongo::Server::Connection.new(server, options.merge(socket_timeout: 2)) + Mongo::Server::Connection.new(server, ssl_options.merge(socket_timeout: 2)) end - let(:options) do - SpecConfig.instance.test_options.merge( + let(:ssl_options) do + SpecConfig.instance.ssl_options.merge( ssl: true, ssl_cert: SpecConfig.instance.client_pem_path, ssl_key: SpecConfig.instance.client_pem_path, @@ -699,7 +699,7 @@ context 'when ssl_verify is not specified' do require_local_tls - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => SpecConfig.instance.local_ca_cert_path ).tap { |options| options.delete(:ssl_verify) } @@ -713,7 +713,7 @@ context 'when ssl_verify is true' do require_local_tls - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => SpecConfig.instance.local_ca_cert_path, :ssl_verify => true @@ -727,7 +727,7 @@ context 'when ssl_verify is false' do - let(:options) do + let(:ssl_options) do super().merge( :ssl_ca_cert => 'invalid', :ssl_verify => false From f6d12868b5c815782dd4b58205610aa1b42e2f29 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sat, 22 Feb 2020 16:56:26 -0500 Subject: [PATCH 04/11] set timeout on the dns resolution for good measure --- spec/mongo/socket/ssl_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index d1b76e82e9..131cc46bcf 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -20,7 +20,7 @@ end let(:socket) do - resolver.socket(socket_timeout, ssl_options) + resolver.socket(socket_timeout, ssl_options, connect_timeout: 2.4) end let(:ssl_options) do From 1365d48650a2b59235a677a9f8d5d6559f6f0f87 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sat, 22 Feb 2020 16:56:44 -0500 Subject: [PATCH 05/11] RUBY-2140 add a timeout on socket liveness check --- lib/mongo/socket.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/mongo/socket.rb b/lib/mongo/socket.rb index 2867c5979d..f99cae2c1e 100644 --- a/lib/mongo/socket.rb +++ b/lib/mongo/socket.rb @@ -63,7 +63,16 @@ class Socket def alive? sock_arr = [ @socket ] if Kernel::select(sock_arr, nil, sock_arr, 0) - eof? + # The eof? call is supposed to return immediately since select + # indicated the socket is readable. However, if @socket is an SSL + # socket, eof? can block anyway - see RUBY-2140. + begin + Timeout.timeout(0.1) do + eof? + end + rescue ::Timeout::Error + true + end else true end From 24d27396340e6cea646ac329be684667db875c31 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sat, 22 Feb 2020 18:14:17 -0500 Subject: [PATCH 06/11] add an actual invalid cert test and skip invalid key test on jruby --- spec/mongo/socket/ssl_spec.rb | 44 ++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index 131cc46bcf..59ac48c04c 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -347,13 +347,7 @@ end end - context 'when a bad certificate is provided' do - - let(:ssl_options) do - super().merge( - :ssl_key => COMMAND_MONITORING_TESTS.first - ) - end + context 'when a bad certificate/key is provided' do let(:expected_exception) do if SpecConfig.instance.jruby? @@ -372,10 +366,38 @@ end end - it 'raises an exception' do - expect do - socket - end.to raise_exception(expected_exception) + shared_examples_for 'raises an exception' do + it 'raises an exception' do + expect do + socket + end.to raise_exception(expected_exception) + end + end + + context 'when a bad certificate is provided' do + + let(:ssl_options) do + super().merge( + :ssl_cert => COMMAND_MONITORING_TESTS.first, + :ssl_key => nil, + ) + end + + it_behaves_like 'raises an exception' + end + + context 'when a bad key is provided' do + # On JRuby the key does not appear to be parsed + only_mri + + let(:ssl_options) do + super().merge( + :ssl_cert => nil, + :ssl_key => COMMAND_MONITORING_TESTS.first, + ) + end + + it_behaves_like 'raises an exception' end end From 2747697d11d37c41b424c82e50e79ea34c7fa3f8 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sun, 23 Feb 2020 02:17:00 -0500 Subject: [PATCH 07/11] reorganize invalid cert/key tests --- spec/mongo/socket/ssl_spec.rb | 99 +++++++++++++++++++++----------- spec/support/lite_constraints.rb | 8 +++ 2 files changed, 72 insertions(+), 35 deletions(-) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index 59ac48c04c..a797ff0b87 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -349,55 +349,84 @@ context 'when a bad certificate/key is provided' do - let(:expected_exception) do - if SpecConfig.instance.jruby? - # java.lang.ClassCastException: org.bouncycastle.asn1.DERApplicationSpecific cannot be cast to org.bouncycastle.asn1.ASN1Sequence - # https://github.com/jruby/jruby-openssl/issues/171 - Exception - else - # mri - if RUBY_VERSION >= '2.4.0' - # OpenSSL::PKey::PKeyError: Could not parse PKey: no start line - OpenSSL::OpenSSLError - else - # ArgumentError: Could not parse PKey: no start line - ArgumentError - end - end - end - shared_examples_for 'raises an exception' do it 'raises an exception' do expect do socket - end.to raise_exception(expected_exception) + end.to raise_exception(*expected_exception) end end - context 'when a bad certificate is provided' do + context 'mri' do + only_mri - let(:ssl_options) do - super().merge( - :ssl_cert => COMMAND_MONITORING_TESTS.first, - :ssl_key => nil, - ) + context 'when a bad certificate is provided' do + + let(:expected_exception) do + if RUBY_VERSION >= '2.4.0' + # OpenSSL::X509::CertificateError: nested asn1 error + [OpenSSL::OpenSSLError, /asn1 error/i] + else + [ArgumentError, /asn1 error/i] + end + end + + let(:ssl_options) do + super().merge( + :ssl_cert => COMMAND_MONITORING_TESTS.first, + :ssl_key => nil, + ) + end + + it_behaves_like 'raises an exception' end - it_behaves_like 'raises an exception' - end + context 'when a bad key is provided' do - context 'when a bad key is provided' do - # On JRuby the key does not appear to be parsed - only_mri + let(:expected_exception) do + if RUBY_VERSION >= '2.4.0' + # OpenSSL::PKey::PKeyError: Could not parse PKey: no start line + [OpenSSL::OpenSSLError, /Could not parse PKey/] + else + # ArgumentError: Could not parse PKey: no start line + [ArgumentError, /Could not parse PKey/] + end + end - let(:ssl_options) do - super().merge( - :ssl_cert => nil, - :ssl_key => COMMAND_MONITORING_TESTS.first, - ) + let(:ssl_options) do + super().merge( + :ssl_cert => nil, + :ssl_key => COMMAND_MONITORING_TESTS.first, + ) + end + + it_behaves_like 'raises an exception' end + end - it_behaves_like 'raises an exception' + context 'jruby' do + require_jruby + + # On JRuby the key does not appear to be parsed, therefore only + # specifying the bad certificate produces an error. + + context 'when a bad certificate is provided' do + + let(:ssl_options) do + super().merge( + :ssl_cert => COMMAND_MONITORING_TESTS.first, + :ssl_key => nil, + ) + end + + let(:expected_exception) do + # java.lang.ClassCastException: org.bouncycastle.asn1.DERApplicationSpecific cannot be cast to org.bouncycastle.asn1.ASN1Sequence + # OpenSSL::X509::CertificateError: parsing issue: malformed PEM data: no header found + [OpenSSL::OpenSSLError, /malformed pem data/i] + end + + it_behaves_like 'raises an exception' + end end end diff --git a/spec/support/lite_constraints.rb b/spec/support/lite_constraints.rb index e7082c91c4..d758284aae 100644 --- a/spec/support/lite_constraints.rb +++ b/spec/support/lite_constraints.rb @@ -8,6 +8,14 @@ def only_mri end end + def require_jruby + before(:all) do + unless BSON::Environment.jruby? + skip "JRuby required, we have #{SpecConfig.instance.platform}" + end + end + end + # This is for marking tests that fail on jruby that should # in principle work (as opposed to being fundamentally incompatible # with jruby). From 64005182de730f62c241f5848222b8a8179c286d Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sun, 23 Feb 2020 02:37:03 -0500 Subject: [PATCH 08/11] all rubies fail the same way for certs it seems --- spec/mongo/socket/ssl_spec.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/spec/mongo/socket/ssl_spec.rb b/spec/mongo/socket/ssl_spec.rb index a797ff0b87..d3ee14c239 100644 --- a/spec/mongo/socket/ssl_spec.rb +++ b/spec/mongo/socket/ssl_spec.rb @@ -363,12 +363,8 @@ context 'when a bad certificate is provided' do let(:expected_exception) do - if RUBY_VERSION >= '2.4.0' - # OpenSSL::X509::CertificateError: nested asn1 error - [OpenSSL::OpenSSLError, /asn1 error/i] - else - [ArgumentError, /asn1 error/i] - end + # OpenSSL::X509::CertificateError: nested asn1 error + [OpenSSL::OpenSSLError, /asn1 error/i] end let(:ssl_options) do From cb4c9708df229966cb9a8c66c4135d5105fe760d Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sun, 23 Feb 2020 03:25:20 -0500 Subject: [PATCH 09/11] try to fix conversation spec failures on jruby --- spec/mongo/auth/scram/conversation_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/mongo/auth/scram/conversation_spec.rb b/spec/mongo/auth/scram/conversation_spec.rb index ee6f8982f4..7769ffccda 100644 --- a/spec/mongo/auth/scram/conversation_spec.rb +++ b/spec/mongo/auth/scram/conversation_spec.rb @@ -35,6 +35,8 @@ end describe '#start' do + # Test uses global assertions + clean_slate let(:query) do conversation.start(nil) From 2380e66af892ee68825dc013330122c54a446dad Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sun, 23 Feb 2020 03:36:35 -0500 Subject: [PATCH 10/11] fix step down tests when an arbiter is present in replica set --- spec/integration/retryable_errors_spec.rb | 4 ++-- spec/support/cluster_tools.rb | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/integration/retryable_errors_spec.rb b/spec/integration/retryable_errors_spec.rb index 511848494c..fce3d35533 100644 --- a/spec/integration/retryable_errors_spec.rb +++ b/spec/integration/retryable_errors_spec.rb @@ -24,7 +24,7 @@ end after do - ClusterTools.instance.direct_client_for_each_server do |client| + ClusterTools.instance.direct_client_for_each_data_bearing_server do |client| client.use(:admin).database.command(clear_fail_point_command) end end @@ -62,7 +62,7 @@ server.monitor.stop! end - ClusterTools.instance.direct_client_for_each_server do |client| + ClusterTools.instance.direct_client_for_each_data_bearing_server do |client| client.use(:admin).database.command(fail_point_command) end end diff --git a/spec/support/cluster_tools.rb b/spec/support/cluster_tools.rb index fdbdd0ee0a..899b9e2994 100644 --- a/spec/support/cluster_tools.rb +++ b/spec/support/cluster_tools.rb @@ -58,7 +58,7 @@ def set_election_handoff(value) raise ArgumentError, 'Value must be true or false' end - direct_client_for_each_server do |client| + direct_client_for_each_data_bearing_server do |client| client.use(:admin).database.command(setParameter: 1, enableElectionHandoff: value) end end @@ -147,7 +147,7 @@ def change_primary existing_primary_address = existing_primary.address target = admin_client.cluster.servers_list.detect do |server| - server.address != existing_primary_address + !server.arbiter? && server.address != existing_primary_address end cfg = get_rs_config @@ -279,6 +279,7 @@ def unfreeze_server(address) def unfreeze_all admin_client.cluster.servers_list.each do |server| + next if server.arbiter? client = direct_client(server.address) # Primary refuses to be unfrozen with this message: # cannot freeze node when primary or running for election. state: Primary (95) @@ -353,8 +354,9 @@ def each_server(&block) admin_client.cluster.servers_list.each(&block) end - def direct_client_for_each_server(&block) + def direct_client_for_each_data_bearing_server(&block) each_server do |server| + next if server.arbiter? yield direct_client(server.address) end end From 95a8d35476470c0051cae8088d5f48e212470501 Mon Sep 17 00:00:00 2001 From: Oleg Pudeyev Date: Sun, 23 Feb 2020 03:42:58 -0500 Subject: [PATCH 11/11] set auth options to fix the test on jruby w/auth configuration --- spec/mongo/collection_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/mongo/collection_spec.rb b/spec/mongo/collection_spec.rb index 8e68b0992f..4a0f7a6d30 100644 --- a/spec/mongo/collection_spec.rb +++ b/spec/mongo/collection_spec.rb @@ -278,9 +278,9 @@ require_topology :replica_set let(:client_options) do - { + SpecConfig.instance.auth_options.merge( read: { mode: :primary_preferred }, - } + ) end let(:subscriber) { EventSubscriber.new }