haraka / haraka-plugin-ldap

Developing LDAP plugins for Haraka
https://www.npmjs.com/package/haraka-plugin-ldap
MIT License
1 stars 4 forks source link

Invalid Credentials leave LDAP server to resource exhaustion #12

Closed flippipe closed 2 months ago

flippipe commented 2 months ago

Describe the bug

When a user authentication fails in LDAP server, TCP connection is not closed, leading to resource exhaustion on LDAP server.

invalid_leave_connection_open

Expected behavior

After the return of response of InvalidCredentials, the connection should be disconnected.

fix_invalid_close_connection

As it occurs when the authentication is successful

valid_close_connection

Steps To Reproduce

Do hundreds of invalid authentications

System Info

Haraka Haraka.js — Version: 2.8.23
Node v6.14.3
OS Linux 3.10.0-862.el7.x86_64 #1 SMP Fri Apr 20 16:44:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
openssl OpenSSL 1.0.2k-fips 26 Jan 2017

Additional information

I've fixed this issue in my environment placing an unbind before function return.

exports._verify_user = function (userdn, passwd, cb, connection) {
    // code removed for brevity
    pool._create_client(function (err, client) {
        if (err) { return onError(err); }
        client.bind(userdn, passwd, function(err) {
            if (err) {
                connection.logdebug('Login failed, could not bind ' + util.inspect(userdn) + ': ' + util.inspect(err));
                // added unbind
                client.unbind();
                return cb(false);
            }
            else {
                client.unbind();
                return cb(true);
            }
        });
    });
};

But the same logic error, is still present in latest version

https://github.com/haraka/haraka-plugin-ldap/blob/1aecf4b167fa016e59b41cb5f7ea59aba85ec874/authn.js#L18:L26

From ldapjs documentation

Note that unbind operation is not an opposite operation for bind. Unbinding results in disconnecting the client regardless of whether a bind operation was performed.
msimerson commented 2 months ago

Can we expect a PR?

flippipe commented 2 months ago

I do not know how node works to be comfortable to know if this is the right way to do it. I hope someone with more expertise do it the right way.

msimerson commented 2 months ago

It looks okay to me. I've merged this change and released a new version. Thanks for the report.