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
 #
@@ -487,22 +495,22 @@ def self.result2string(code) #:nodoc:
   #   The :start_tls like the :simple_tls encryption method also encrypts all
   #   communcations with the LDAP server. With the exception that it operates
   #   over the standard TCP port.
-  #  
+  #
   #   In order to verify certificates and enable other TLS options, the
   #   :tls_options hash can be passed alongside :simple_tls or :start_tls.
   #   This hash contains any options that can be passed to
   #   OpenSSL::SSL::SSLContext#set_params(). The most common options passed
   #   should be OpenSSL::SSL::SSLContext::DEFAULT_PARAMS, or the :ca_file option,
   #   which contains a path to a Certificate Authority file (PEM-encoded).
-  #  
+  #
   #   Example for a default setup without custom settings:
   #     {
   #       :method => :simple_tls,
   #       :tls_options => OpenSSL::SSL::SSLContext::DEFAULT_PARAMS
   #     }
-  #  
+  #
   #   Example for specifying a CA-File and only allowing TLSv1.1 connections:
-  #  
+  #
   #     {
   #       :method => :start_tls,
   #       :tls_options => { :ca_file => "/etc/cafile.pem", :ssl_version => "TLSv1_1" }
@@ -524,6 +532,7 @@ def initialize(args = {})
     @base = args[:base] || DefaultTreebase
     @force_no_page = args[:force_no_page] || DefaultForceNoPage
     @encryption = args[:encryption] # may be nil
+    @connect_timeout = args[:connect_timeout]
 
     if pr = @auth[:password] and pr.respond_to?(:call)
       @auth[:password] = pr.call
@@ -587,7 +596,7 @@ def authenticate(username, password)
   # additional capabilities are added, more configuration values will be
   # added here.
   #
-  # This method is deprecated. 
+  # This method is deprecated.
   #
   def encryption(args)
     warn "Deprecation warning: please give :encryption option as a Hash to Net::LDAP.new"
@@ -1247,8 +1256,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 \