shaozi / ldap-authentication

🔐🔐🔐 A simple Nodejs Async LDAP authentication library
BSD 2-Clause "Simplified" License
109 stars 28 forks source link

Random crashes due to double unbind #58

Closed bchen290 closed 5 months ago

bchen290 commented 11 months ago

Hi,

When using this package, we are noticing that our APIs sometimes come down hard due to the following errors. The first error is

/app/node_modules/ldapjs/lib/client/client.js:1102
  socket.removeAllListeners('connect')
         ^

TypeError: Cannot read properties of null (reading 'removeAllListeners')
    at Client._onClose (/app/node_modules/ldapjs/lib/client/client.js:1102:10)
    at Object.onceWrapper (node:events:632:26)
    at Socket.emit (node:events:517:28)
    at TCP.<anonymous> (node:net:350:12)

while the second error is an uncaught exception:

Error: read ECONNRESET
      at TCP.onStreamRead (node:internal/stream_base_commons:217:20)

It was hard to reproduce this error as it happened intermittently but the one way I was able to reproduce it was to spam the API with a ton of auth requests which will eventually take it down with the above errors. We are running the following code block for authentication:

let authenticated = await authenticate({
    ldapOpts: this.ldapOptions,
    verifyUserExists: true,
    usernameAttribute: "sAMAccountName",
    userSearchBase: this.options.ldap.base,
    adminDn: this.options.ldap.adminDn,
    adminPassword: this.options.ldap.adminPassword,
    username: username,
})

and tracing the code path in this package, we call an unbind on res.on("end") in _searchUser and again after _searchUser is finished, in verifyUserExists.

I also dumped the packets from our system and can confirm that we are unbinding twice for every bind and search we do. This normally is fine but one thing I noticed is that a bad request will have a unbind request, then before the unbind request is successfully ACK by the ldap controller, we send another unbind and bind request which gets shutdown by the LDAP controller which causes an ECONNRESET or the socket to not be properly cleaned up.

I tested removing the extra unbind in verifyUserExists locally and running the same test of spamming the API, that seem to resolve the issue. If you need any more information, please let me know but thanks for reading and have a nice day!

shaozi commented 5 months ago

hmm... seems there are excessive unbind() calls. let me remove them