nickva commented on code in PR #4814: URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378989463
########## src/couch/src/couch_passwords_cache.erl: ########## @@ -0,0 +1,65 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. +% +% password hashes on disk can take a long time to verify. This is by design, to +% guard against offline attacks. This module adds an in-memory cache for password +% verification to speed up verification after a successful slow verification from +% the hash stored on disk, in order not to transfer the deliberate offline attack +% protection to database requests. +% +% In memory we record a PBKDF_SHA256 derivation using a low number of iterations +% and check against this if present. Entries in couch_passwords_cache expire automatically +% and the maximum number of cached entries is configurable. + +-module(couch_passwords_cache). + +-define(CACHE, couch_passwords_cache_lru). + +-export([start_link/0]). + +% public api +-export([authenticate/4, insert/4]). + +start_link() -> + gen_server:start_link({local, ?MODULE}, ?MODULE, [], []). + +% public functions +-spec authenticate( + AuthModule :: atom(), UserName :: binary(), Password :: binary(), Salt :: binary() +) -> + not_found | boolean(). +authenticate(AuthModule, UserName, Password, Salt) -> + case config:get_boolean("couch_passwords_cache", "enable", true) of + true -> + authenticate_int(AuthModule, UserName, Password, Salt); + false -> + not_found + end. + +authenticate_int(AuthModule, UserName, Password, Salt) -> + % salt in key to ensure entry is not a match if user changes their password + % (as salt is re-randomised in that event) + case ets_lru:lookup_d(?CACHE, {AuthModule, UserName, Salt}) of + not_found -> + not_found; + {ok, ExpectedHash} -> + ActualHash = hash(Password, Salt), + couch_passwords:verify(ExpectedHash, ActualHash) + end. + +-spec insert(AuthModule :: atom(), UserName :: binary(), Password :: binary(), Salt :: binary()) -> + ok. +insert(AuthModule, UserName, Password, Salt) -> + ets_lru:insert(?CACHE, {AuthModule, UserName, Salt}, hash(Password, Salt)). + +hash(Password, Salt) -> + fast_pbkdf2:pbkdf2(sha256, Password, Salt, _Iterations = 5, _KeyLen = 32). Review Comment: Well, it wouldn't be configurable anyway, but the idea is to make them more visible as they are parameters for this cache module. Perhaps with a comment about why iterations is 5 and maybe what threat model we're assuming. If I were auditing the code ` _Iterations = 5` would jump out immediately as a red flag. Since fast_pbkdf2 is an external library let's isolate all the calls to it in `couch_passwords` or other hash utility module if we have one. If my otp pbdkf2 unlocking pr merges, we'd have less places to replace. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
