search

vesse / node-ldapauth-fork

Simple node.js module to authenticate against an LDAP server
Other
127 stars 79 forks source link

bcrypt cache performance #76

Open travisghansen opened 5 years ago

travisghansen commented 5 years ago

Thanks for the library! Using this in a new project and was surprised to see authenticate taking as much time as it did when I had cache enabled. I had 17 parallel jobs executing which added about 1.5 - 2.0 seconds vs my own auth cache stored in redis. Digging through the code left me at 1 spot, bcrypt.compareSync() (I didn't trace all this so it could be something else).

Would it be possible to use a different hash? Would it be possible to use the native c++ module and backfill with the pure js? Would it be possible to simply hash the username/password jointly as the cache key (using something faster than bcrypt) and the mere existence of the cache entry means it's legit?

Thanks!

vesse commented 5 years ago

True that - I've never given it a thought since this was just one feature in the original library. I'd guess the point has been to reduce load on the LDAP server instead of speedier authentication. This is a difficult thing though because one of the points of bcrypt, afaik, is that it takes time so brute force attacks are harder and thus I'm not very open to changing to "weaker" algorithm. One thing that comes to mind is to allow providing a cache implementation so it'd be up to the programmer to take care of possible security concerns.

travisghansen commented 5 years ago

@vesse Fair enough. Seems like we could still migrate over to optionally allow the native module at a minimum.

The brute force thought seems mostly irrelevant as implemented from what I can tell. The salt is generated once per instance (this._salt = bcrypt.genSaltSync();) and is where you specify the rounds so if the same client instance is handling subsequent invocations it seems mostly moot. Additionally, an attacker trying to brute force something via a code path that is more efficient (ie: cached) seems unlikely and counter productive (to an actor with bad intentions).

An alternative is if you made the cache key an md5 of username/password jointly (this is essentially what I'm doing outside the client myself currently). Then to hit the bcrypt code path at all they'd need to actually have legitimate credentials anyhow (which would render the subsequent check entirely unnecessary anyway).

As it is I am cache'ing outside the module as you suggested. A second layer wouldn't hurt though :)

Food for thought I guess.