Use Socket.tcp instead of TCPSocket.new to provide socket timeouts

This patch prevents LDAP connections to hang up for an eccessive amount of time
and instead returns earlier in case of failures (e.g., packets dropped).

A new option is now exposed through Net::LDAP:
- connect_timeout: sets a timeout for socket#connect (defaults to 1s)

It also provides an integration test to validate the new behaviour (#244)

Author: astratto

lib/net/ldap.rb

@@ -79,6 +79,14 @@ class LDAP
 #
 #  p ldap.get_operation_result
 #
+# === Setting connect timeout
+#
+# By default, Net::LDAP uses TCP sockets with a connection timeout of 5 seconds.
+#
+# This value can be tweaked passing the :connect_timeout parameter.
+# i.e.
+#  ldap = Net::LDAP.new ...,
+#                       :connect_timeout => 3
 #
 # == A Brief Introduction to LDAP
 #
@@ -482,6 +490,7 @@ def initialize(args = {})
     @auth = args[:auth] || DefaultAuth
     @base = args[:base] || DefaultTreebase
     @force_no_page = args[:force_no_page] || DefaultForceNoPage
+    @connect_timeout = args[:connect_timeout]
     encryption args[:encryption] # may be nil
 
     if pr = @auth[:password] and pr.respond_to?(:call)
@@ -1242,8 +1251,9 @@ def new_connection
       :port                    => @port,
       :hosts                   => @hosts,
       :encryption              => @encryption,
-      :instrumentation_service => @instrumentation_service
-  rescue Errno::ECONNREFUSED, Net::LDAP::ConnectionRefusedError => e
+      :instrumentation_service => @instrumentation_service,
+      :connect_timeout         => @connect_timeout
+  rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::LDAP::ConnectionRefusedError => e
     @result = {
       :resultCode   => 52,
       :errorMessage => ResultStrings[ResultCodeUnavailable]

lib/net/ldap/connection.rb

@@ -3,6 +3,9 @@
 class Net::LDAP::Connection #:nodoc:
   include Net::LDAP::Instrumentation
 
+  # Seconds before failing for socket connect timeout
+  DefaultConnectTimeout = 5
+
   LdapVersion = 3
   MaxSaslChallenges = 10
 
@@ -31,10 +34,14 @@ def open_connection(server)
     hosts = server[:hosts]
     encryption = server[:encryption]
 
+    socket_opts = {
+      connect_timeout: server[:connect_timeout] || DefaultConnectTimeout
+    }
+
     errors = []
     hosts.each do |host, port|
       begin
-        prepare_socket(server.merge(socket: TCPSocket.new(host, port)))
+        prepare_socket(server.merge(socket: Socket.tcp(host, port, socket_opts)))
         return
       rescue Net::LDAP::Error, SocketError, SystemCallError,
              OpenSSL::SSL::SSLError => e

script/install-openldap

@@ -109,4 +109,7 @@ chgrp ssl-cert /etc/ssl/private/ldap01_slapd_key.pem
 chmod g+r /etc/ssl/private/ldap01_slapd_key.pem
 chmod o-r /etc/ssl/private/ldap01_slapd_key.pem
 
+# Drop packets on a secondary port used to specific timeout tests
+iptables -A OUTPUT -p tcp -j DROP --dport 8389
+
 service slapd restart

test/integration/test_bind.rb

@@ -5,6 +5,14 @@ def test_bind_success
     assert @ldap.bind(method: :simple, username: "uid=user1,ou=People,dc=rubyldap,dc=com", password: "passworD1"), @ldap.get_operation_result.inspect
   end
 
+  def test_bind_timeout
+    @ldap.port = 8389
+    error = assert_raise Net::LDAP::Error do
+      @ldap.bind(method: :simple, username: "uid=user1,ou=People,dc=rubyldap,dc=com", password: "passworD1")
+    end
+    assert_equal('Connection timed out - user specified timeout', error.message)
+  end
+
   def test_bind_anonymous_fail
     refute @ldap.bind(method: :simple, username: "uid=user1,ou=People,dc=rubyldap,dc=com", password: ""), @ldap.get_operation_result.inspect
 

test/test_auth_adapter.rb

@@ -2,7 +2,8 @@
 
 class TestAuthAdapter < Test::Unit::TestCase
   def test_undefined_auth_adapter
-    flexmock(TCPSocket).should_receive(:new).ordered.with('ldap.example.com', 379).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.with('ldap.example.com', 379, { connect_timeout: 5 }).once.and_return(nil)
+
     conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379)
     assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do
       conn.bind(method: :foo)

test/test_ldap_connection.rb

@@ -15,8 +15,8 @@ def test_list_of_hosts_with_first_host_successful
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     Net::LDAP::Connection.new(:hosts => hosts)
   end
 
@@ -26,9 +26,9 @@ def test_list_of_hosts_with_first_host_failure
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_return(nil)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     Net::LDAP::Connection.new(:hosts => hosts)
   end
 
@@ -38,17 +38,17 @@ def test_list_of_hosts_with_all_hosts_failure
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[2]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 5 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 5 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[2], { connect_timeout: 5 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     assert_raise Net::LDAP::ConnectionError do
       Net::LDAP::Connection.new(:hosts => hosts)
     end
   end
 
   def test_result_for_connection_failed_is_set
-    flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED)
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
 
     ldap_client = Net::LDAP.new(host: '127.0.0.1', port: 12345)
 
@@ -67,14 +67,14 @@ def test_unresponsive_host
   end
 
   def test_blocked_port
-    flexmock(TCPSocket).should_receive(:new).and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).and_raise(SocketError)
     assert_raise Net::LDAP::Error do
       Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
     end
   end
 
   def test_connection_refused
-    flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED)
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
     stderr = capture_stderr do
       assert_raise Net::LDAP::ConnectionRefusedError do
         Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
@@ -83,9 +83,18 @@ def test_connection_refused
     assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n",  stderr)
   end
 
+  def test_connection_timedout
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ETIMEDOUT)
+    stderr = capture_stderr do
+      assert_raise Net::LDAP::Error do
+        Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+      end
+    end
+  end
+
   def test_raises_unknown_exceptions
     error = Class.new(StandardError)
-    flexmock(TCPSocket).should_receive(:new).and_raise(error)
+    flexmock(Socket).should_receive(:tcp).and_raise(error)
     assert_raise error do
       Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
     end
@@ -328,7 +337,7 @@ class TestLDAPConnectionErrors < Test::Unit::TestCase
   def setup
     @tcp_socket = flexmock(:connection)
     @tcp_socket.should_receive(:write)
-    flexmock(TCPSocket).should_receive(:new).and_return(@tcp_socket)
+    flexmock(Socket).should_receive(:tcp).and_return(@tcp_socket)
     @connection = Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
   end
 
@@ -357,7 +366,7 @@ class TestLDAPConnectionInstrumentation < Test::Unit::TestCase
   def setup
     @tcp_socket = flexmock(:connection)
     @tcp_socket.should_receive(:write)
-    flexmock(TCPSocket).should_receive(:new).and_return(@tcp_socket)
+    flexmock(Socket).should_receive(:tcp).and_return(@tcp_socket)
 
     @service = MockInstrumentationService.new
     @connection = Net::LDAP::Connection.new \