Repository: twisted/ldaptor

Issue:
Proxy may keep connection to proxied server open if client disconnects immediately
Hello! We're developing an LDAP proxy application based on ldaptor which has worked pretty well so far, so thanks for the great library! We recently stumbled upon a small bug which can be reproduced as follows:

* Use the current ldaptor master (7a51037ccf3ebe387766a3a91cf9ae79a3a3393d)
* Run the exemplary logging proxy code from [the docs](https://ldaptor.readthedocs.io/en/latest/cookbook/ldap-proxy.html), proxying any LDAP server (AD at ``10.0.1.161:7389`` in our case)
* Use netcat to perform a simple "health check": ``netcat -z localhost 10389``. The ``-z`` flag is [described](https://www.irongeek.com/i.php?page=backtrack-3-man/netcat) as follows:  _In connect mode it means that as soon as the port is open it is immediately shutdown and closed._
* Even though netcat has disconnected, ``netstat -nto`` reports a connection from the LDAP proxy to the proxied server in the ESTABLISHED state:
```
tcp        0      0 10.1.0.4:60478          10.0.1.161:7389         ESTABLISHED off (0.00/0/0)
```
This socket is only closed when the LDAP proxy is shut down. This is unfortunate because some appliances may perform periodical "health checks" against the LDAP proxy, similar to the netcat call above ([see here](https://community.privacyidea.org/t/too-many-established-tcp-connections-from-the-ldap-proxy/718)). If every such health check persists one connection to the proxied server, the LDAP proxy will eventually run out of open files. 

I believe this is due to a race condition:
* When netcat connects to the proxy, ``connectionMade`` is called, which initiates a connection to the proxied server. It uses a deferred to invoke ``_connectedToProxiedServer`` once the connection has been established: https://github.com/twisted/ldaptor/blob/7a51037ccf3ebe387766a3a91cf9ae79a3a3393d/ldaptor/protocols/ldap/proxybase.py#L40
* netcat closes the connection to the proxy, so ``connectionLost`` is called. However, as netcat does so *immediately*, the above deferred's callback has not been called yet, so ``self.client`` is still None.
* After that, ``_connectedToProxiedServer`` is finally called. Because of that, the connection to the proxied server is never closed.

I'll come up with a small PR to fix this, though it may not be the optimal solution as I'm not that familiar with the ldaptor codebase :)

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=h1) Report
> Merging [#106](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=desc) into [master](https://codecov.io/gh/twisted/ldaptor/commit/7a51037ccf3ebe387766a3a91cf9ae79a3a3393d?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/twisted/ldaptor/pull/106/graphs/tree.svg?token=rXfSDCH1f3&src=pr&width=650&height=150)](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   88.81%   88.84%   +0.03%     
==========================================
  Files          71       71              
  Lines        9905     9914       +9     
  Branches     1018     1019       +1     
==========================================
+ Hits         8797     8808      +11     
  Misses        914      914              
+ Partials      194      192       -2
```


| [Impacted Files](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ldaptor/protocols/ldap/proxybase.py](https://codecov.io/gh/twisted/ldaptor/pull/106/diff?src=pr&el=tree#diff-bGRhcHRvci9wcm90b2NvbHMvbGRhcC9wcm94eWJhc2UucHk=) | `60% <100%> (+1.54%)` | :arrow_up: |
| [ldaptor/test/test\_proxybase.py](https://codecov.io/gh/twisted/ldaptor/pull/106/diff?src=pr&el=tree#diff-bGRhcHRvci90ZXN0L3Rlc3RfcHJveHliYXNlLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [ldaptor/ldiftree.py](https://codecov.io/gh/twisted/ldaptor/pull/106/diff?src=pr&el=tree#diff-bGRhcHRvci9sZGlmdHJlZS5weQ==) | `89.89% <0%> (+0.36%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=footer). Last update [7a51037...87d5ccc](https://codecov.io/gh/twisted/ldaptor/pull/106?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

cwaldbieser: I'm thinking that if the connection is closed by the client, any pending requests it may have sent should be discarded.  Having a pending request shouldn't stop the proxy from closing the connection.  So I'd suggest changing your correction slightly:

            if not self.connected:
                # Client is no longer connected.  Proxy shouldn't be either!
                self.client.transport.loseConnection()
                self.client = None
		self.queuedRequests = []
            else:
                self._processBacklog()

Thanks for catching that!
fredreichbier: Hi Carl, thanks for having a look and your comment! Discarding all queued requests makes sense, so changed the code accordingly. :)
cwaldbieser: Looks good to me.  I ran a functional test using the ldaptor server example and the proxy example, and it still behaved the way I expect.  I'll merge the PR.

Patch:
diff --git a/docs/source/NEWS.rst b/docs/source/NEWS.rst
index 5744869..5a25d34 100644
--- a/docs/source/NEWS.rst
+++ b/docs/source/NEWS.rst
@@ -32,6 +32,7 @@ Bugfixes
 ^^^^^^^^
 
 - DN matching is now case insensitive.
+- Proxies now terminate the connection to the proxied server in case a client immediately closes the connection.
 
 
 Release 16.0 (2016-06-07)
diff --git a/ldaptor/protocols/ldap/proxybase.py b/ldaptor/protocols/ldap/proxybase.py
index ac5fe23..908481a 100755
--- a/ldaptor/protocols/ldap/proxybase.py
+++ b/ldaptor/protocols/ldap/proxybase.py
@@ -62,7 +62,13 @@ class ProxyBase(ldapserver.BaseLDAPServer):
             return d
         else:
             self.client = proto
-            self._processBacklog()
+            if not self.connected:
+                # Client no longer connected, proxy shouldn't be either
+                self.client.transport.loseConnection()
+                self.client = None
+                self.queuedRequests = []
+            else:
+                self._processBacklog()
 
     def _establishedTLS(self, proto):
         """


Test patch:
diff --git a/ldaptor/test/test_proxybase.py b/ldaptor/test/test_proxybase.py
index 34420eb..7667252 100644
--- a/ldaptor/test/test_proxybase.py
+++ b/ldaptor/test/test_proxybase.py
@@ -247,3 +247,23 @@ class ProxyBase(unittest.TestCase):
         self.assertEqual(
             server.transport.value(),
             str(pureldap.LDAPMessage(pureldap.LDAPBindResponse(resultCode=52), id=4)))
+
+    def test_health_check_closes_connection_to_proxied_server(self):
+        """
+        When the client disconnects immediately and before the connection to the proxied server has
+        been established, the proxy terminates the connection to the proxied server.
+        Messages sent by the client are discarded.
+        """
+        request = pureldap.LDAPBindRequest()
+        message = pureldap.LDAPMessage(request, id=4)
+        server = self.createServer()
+        # Send a message, message is queued
+        server.dataReceived(str(message))
+        self.assertEqual(len(server.queuedRequests), 1)
+        self.assertEqual(server.queuedRequests[0][0], request)
+        # Lose connection, message is discarded
+        server.connectionLost(error.ConnectionDone)
+        server.reactor.advance(1)
+        self.assertIsNone(server.client)
+        self.assertFalse(server.clientTestDriver.connected)
+        self.assertEqual(server.queuedRequests, [])
\ No newline at end of file