node-ldapjs
node-ldapjs copied to clipboard
ldapjs
LDAP Connection error
Hello,
I do my connection like this
const client = ldap.createClient({
url: `ldap://url:389`,
reconnect: true
});
client.bind(_this.options.genericEmail, _this.options.genericPassword, err => {
if (err) {
callback(err);
}
});
And unbind after a search.
client.unbind( err => {
if (err) {
console.log(err);
}
});
But after a while on the development server, if it tries to connect again, I get this error and breaks the node server. I am running this application on a kubernetes environment.
events.js:167
--
| throw er; // Unhandled 'error' event
| ^
|
| Error: read ECONNRESET
| at TCP.onStreamRead (internal/stream_base_commons.js:111:27)
| Emitted 'error' event at:
| at Socket.onSocketError (/usr/src/app/node_modules/ldapjs/lib/client/client.js:1169:12)
| at Socket.emit (events.js:182:13)
| at emitErrorNT (internal/streams/destroy.js:82:8)
| at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
| at process._tickCallback (internal/process/next_tick.js:63:19)
Please help me fix this error.
Thank you.
I'm not 100% sure this is your issue based off what you posted, but looks similar to when the client is left open and connection is reset due to inactivity.
The client is connected the minute it's created, I think it's a poor decision in this lib. If you don't destroy / disconnect the client you will get an error from the connection timing out.
The client code is documented saying it can be reused and auto-reconnect, however in my expierence this has not worked so well especially with Active Directory servers.
If the auto-reconnection is working for you then disconnecting the client might be a good option. If not you can create a new client for each bind request.
Inside client.bind() fn -> the arguments are bindDN and bindCredentials, instead u were passing generic email and password. the first parameter bindDN pass that first.
You can bind using the UPN, which in many companies is the users email address.
I think this was just an improper choice of words by the lib devs to only mention DN. The if statement just checks if it's a string or an instance of DN, if so it passes it along.
I did try
client.destroy();
But I still get the error on production machines.
Many thanks, Karthik
Are you listening for errors on the client? If so can you post that error message?
All i get is this below message.
events.js:167
--
| throw er; // Unhandled 'error' event
| ^
|
| Error: read ECONNRESET
| at TCP.onStreamRead (internal/stream_base_commons.js:111:27)
| Emitted 'error' event at:
| at Socket.onSocketError (/usr/src/app/node_modules/ldapjs/lib/client/client.js:1169:12)
| at Socket.emit (events.js:182:13)
| at emitErrorNT (internal/streams/destroy.js:82:8)
| at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
| at process._tickCallback (internal/process/next_tick.js:63:19)
| [nodemon] app crashed - waiting for file changes before starting...
Any solution for this error. I am still getting this on production. Thank you,
Could you share a full example of code that I can test?
Are you only looking to auth users and and get groups or are you doing something more advanced.
No I am just getting User Groups. Let me share the code that I am using.
this.adSuffix = "OU=Employees,OU=<Company> Users,DC=<company>,DC=com";
const searchOptions = {
scope: "sub",
filter: `(cn=${_this.options.getACLForUser})`,
attributes: 'memberOf' // just get memberOf to optimise.
};
// Create client and bind to AD
const client = ldap.createClient({
url: `ldap://ds.company.com:389`,
reconnect: true,
idleTimeout: 259200000
});
client.bind(_this.options.genericEmail, _this.options.genericPassword, err => {
if (err) {
callback(err);
}
});
client.search(_this.adSuffix, searchOptions, (err, res) => {
if (err) {
callback(err);
} else {
let memberOf = [];
res.on('error', err => {
callback(err);
});
res.on('searchEntry', entry => {
memberOf = entry.object.memberOf;
});
res.on('end', result => {
callback(null, memberOf);
client.destroy();
});
}
});
This gives me the below error and crashes the server. I placed forever to run on crash currently. But would prefer better solution.
We have a Java application and it connects to same ldap source with similar parameters. And that application runs without crashing.
events.js:167
--
| throw er; // Unhandled 'error' event
| ^
|
| Error: read ECONNRESET
| at TCP.onStreamRead (internal/stream_base_commons.js:111:27)
| Emitted 'error' event at:
| at Socket.onSocketError (/usr/src/app/node_modules/ldapjs/lib/client/client.js:1169:12)
| at Socket.emit (events.js:182:13)
| at emitErrorNT (internal/streams/destroy.js:82:8)
| at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
| at process._tickCallback (internal/process/next_tick.js:63:19)
| [nodemon] app crashed - waiting for file changes before starting...
I think this is the timeout error, I can't test your code right now since my desktop is updating to F29 but should be able to later today.
In the meantime you can add a listener for the errors, so they hopefully won't crash your application. Here is an example of listening for errors on the client.
// Return errors
client.on('error', error => {
// Do something with the error
});
I'm able to reproduce your issue if I declare the client globally once and attempt to keep reusing it. The connection is reset by Active Directory after some time, this is a similar issue I faced in the past and I never had enough time to dig into ldapjs to see if it's an issue in the lib or not.
The way I worked around this was simply wrapping ldapjs up into a function, so the entire things is destroyed / garbage collected after the execution.
Keeping the majority of your code here is an example, I tried to add capital words liek ADDED or MODIFIED around areas I changed with some notes.
const ldap = require('ldapjs');
// ADDED - I wasn't sure how you had this setup, so I made a simple function to wrap the code in
function queryForUsers(options, callback) {
this.options = options;
this.adSuffix = "OU=Users,DC=domain,DC=local";
const searchOptions = {
scope: "sub",
filter: `(userPrincipalName=${this.options.genericEmail})`, // ALTERED - Changed to UPN since this was easier for my AD query
attributes: 'memberOf' // just get memberOf to optimise.
};
// Create client and bind to AD
const client = ldap.createClient({
url: `ldap://domain.local`, // REMOVED - 389 port is set by the ldap protocal at front instead of ldaps
reconnect: true,
idleTimeout: 3000 // ALTERED - To make testing this easier
});
// ADDED - Listen for errors
client.on('error', error => {
callback(err);
});
client.bind(this.options.genericEmail, this.options.genericPassword, err => {
if (err) {
callback(err);
return;
}
// MOVED - Inside bind, only want to query if bind was sucess
client.search(this.adSuffix, searchOptions, (err, res) => {
if (err) {
callback(err);
} else {
let memberOf = [];
res.on('error', err => {
callback(err);
});
res.on('searchEntry', entry => {
memberOf = entry.object.memberOf;
});
res.on('end', result => {
callback(null, memberOf);
client.destroy();
});
};
});
});
}
// Simple testing options obj
const options = {
genericEmail: '[email protected]',
genericPassword: 'PASSWORD',
};
// To log the call back events for testing
function callBack(err, res) {
if(err) {
console.log('=== ERROR! ===');
console.error(err);
return;
}
console.log('=== Located Data ===');
console.log(res);
};
// Query for the user
queryForUsers(options, callBack);
If you want some more examples check out this wrapper I made a while ago, it needs to be updated / cleaned up. I plan to work on that some day soon, but you know how it goes with getting busy doing other things. It could still be useful just to see an example of how I made it work. https://github.com/tastypackets/node-ad-tools/blob/master/lib/ActiveDirectory.js
As a side note, I'll try and see if I can find out what is going on in ldapjs. When I initially started working with ldapjs and making that wrapper I wasn't as familiar with NodeJS and it's TLS implementation, now that I'm a little more familiar with it I may be able to spot what is going on when I have some time to look around this lib.
So what's going on is the lib, ldapjs, isn't handling the connection rest. The RST response from LDAP server, but it does emit it on the error event. If you don't want to destroy the client as mentioned above on every single query you will need to handle that error event yourself.
I think the ldapjs lib needs to be updated to handle this event, especially since the author decided to connect the client to the server as soon as you create the client. If the author at least didn't do that it would be okay to pass it up on the error and not handle it, but I think it's adding unneeded complexity to the lib / confusion unless someone spends time reading it themselves.
The problem is, all other open PRs never have been merged. Even if I submit a patch there is probably a 90% chance it will never get merged unless the author returns to this lib. I think for now the best things you can do is jsut handle that event yourself.
Knowing this if I have time one day I'll go back to my own wrapper and update it to handle the event, so it doesn't need to recreate the client every time it's called to reduce the number of connection to the server in the event that many users login at once.
EDIT: Added the link to the GitHub issue that explains why / when this changed. https://github.com/nodejs/node-v0.x-archive/issues/1776
I spent a little time doing a few more tests and I wonder if there is an issue with the queue / freezing in the lib, I have found some odd behavior on the socket when sending many requests at once. I don't have enough examples or knowledge about the queue in the libe right now to say for sure.
I think eventually some time needs to be spent writing some additional unit tests, especially since this lib has aged out a lot since it was first created.
Honestly though, if you have the option to switch packages you should look at ldapts. It's what I have been monitoring and planning to switch to if sometime soon.
It's created by someone else here who was unable to get there updates merged with this lib do to author inactivity.
https://github.com/ldapts/ldapts
@tastypackets Thank you so much for detailed testing and explanation. Looks like switching to ldapts is the best option, and looks like no one maintains this code base. I will leave this open if at all someone wants to pick it up in the future. I would love to contribute, but my knowledge in LDAP is very very limited.
No problem, I plan to switch to it as well once I have had time to thoroughly test it. As soon as I do I'm removing this repo from everything. 😄
@tastypackets if you have time to come back to this I would be very willing to look at a PR. If so, please create the PR against the next branch (if it still exists). I have taken up maintainership of this module and am trying to get some much needed fixes merged in before a new release.
haii, i am also facing same error,but i cannot switch my packages. can any one please tell how to solve the problem.
haii, i am also facing same error,but i cannot switch my packages. can any one please tell how to solve the problem.
@srikanth9010 check if you are doing unbind at every success as well as error scenarios [callback]. During testing it is quiet common to validate LDAP with invalid password to check if the validation happens, in which err occurs during bind() and get into callback(err) and mostly we miss to unbind during callback because of which timeout happens. Handling this scenario helped me fix the issue.
As some folks suggest switching to ldapts: Is somebody able to verify, that this issue does not occur with ldapts?
And out of curiosity @jsumners: Wouldn't it be valid for this package to join forces with the TypeScript implementation?
@beevelop we welcome all contributors here. The TypeScript implementation was started when this project had less active maintainers governing it.
today i receive this problem with ldapjs, the recomendation yet is change ldapjs to ldapts? because i put createClient into a function and the error it disappeared.
👋
On February 22, 2023, we released version 3 of this library. As a result, we are closing this issue/pull request.
Please see issue #839 for more information, including how to proceed if you feel this closure is in error.