Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814: URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378395364 ## 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). Review Comment: Would it simplify naming a bit if we just used ?MODULE in this module and `couch_passwords_cache` in `couch_primary_sup`? We already know it's an LRU based on the ets_lru insert/lookup_d calls below. -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378393624
##
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, [], []).
Review Comment:
Do we use this call? This module doesn't look like a gen_server?
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378384582
##
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:
Not sure if the prf, iterations and keylen would look better as macros, to
stand out at the top as parameters of this module. If you prefer them as is,
that's fine too, it's more concise perhaps.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378382278
##
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) ->
Review Comment:
Instead of just specs, which are great, it might make sense to also assert
them as guards?
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378380253
##
src/couch/src/couch_passwords.erl:
##
@@ -69,106 +68,74 @@ get_unhashed_admins() ->
({_User, "-pbkdf2-" ++ _}) ->
% already hashed
false;
+({_User, "-pbkdf2:sha-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha224-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha256-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha384-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha512-" ++ _}) ->
+% already hashed
+false;
({_User, _ClearPassword}) ->
true
end,
config:get("admins")
).
-%% Current scheme, much stronger.
--spec pbkdf2(binary(), binary(), integer()) -> binary().
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(Password, Salt, Iterations) ->
+pbkdf2(sha, Password, Salt, Iterations).
+
+pbkdf2(PRF, Password, Salt, Iterations) when is_atom(PRF) ->
+#{size := Size} = crypto:hash_info(PRF),
+pbkdf2(PRF, Password, Salt, Iterations, Size);
+pbkdf2(Password, Salt, Iterations, KeyLen) ->
+pbkdf2(sha, Password, Salt, Iterations, KeyLen).
+
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
->
-{ok, Result} = pbkdf2(Password, Salt, Iterations, ?SHA1_OUTPUT_LENGTH),
-Result;
-pbkdf2(Password, Salt, Iterations) when
+DerivedKey = fast_pbkdf2:pbkdf2(PRF, Password, Salt, Iterations, KeyLen),
+couch_util:to_hex_bin(DerivedKey);
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
->
Msg = io_lib:format("Password value of '~p' is invalid.", [Password]),
throw({forbidden, Msg});
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
Review Comment:
Add `is_integer(KeyLen)`, as above.
If these are all lower level private functions and it's too repetitive to
add guard to them all, it could make sense to just check the types and ranges a
bit higher at the external API's level (whichever looks shorter or nicer).
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378380590
##
src/couch/src/couch_passwords.erl:
##
@@ -69,106 +68,74 @@ get_unhashed_admins() ->
({_User, "-pbkdf2-" ++ _}) ->
% already hashed
false;
+({_User, "-pbkdf2:sha-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha224-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha256-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha384-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha512-" ++ _}) ->
+% already hashed
+false;
({_User, _ClearPassword}) ->
true
end,
config:get("admins")
).
-%% Current scheme, much stronger.
--spec pbkdf2(binary(), binary(), integer()) -> binary().
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(Password, Salt, Iterations) ->
+pbkdf2(sha, Password, Salt, Iterations).
+
+pbkdf2(PRF, Password, Salt, Iterations) when is_atom(PRF) ->
+#{size := Size} = crypto:hash_info(PRF),
+pbkdf2(PRF, Password, Salt, Iterations, Size);
+pbkdf2(Password, Salt, Iterations, KeyLen) ->
+pbkdf2(sha, Password, Salt, Iterations, KeyLen).
+
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
->
-{ok, Result} = pbkdf2(Password, Salt, Iterations, ?SHA1_OUTPUT_LENGTH),
-Result;
-pbkdf2(Password, Salt, Iterations) when
+DerivedKey = fast_pbkdf2:pbkdf2(PRF, Password, Salt, Iterations, KeyLen),
+couch_util:to_hex_bin(DerivedKey);
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
Review Comment:
Let's assert KeyLen is an integer.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378380253
##
src/couch/src/couch_passwords.erl:
##
@@ -69,106 +68,74 @@ get_unhashed_admins() ->
({_User, "-pbkdf2-" ++ _}) ->
% already hashed
false;
+({_User, "-pbkdf2:sha-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha224-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha256-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha384-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha512-" ++ _}) ->
+% already hashed
+false;
({_User, _ClearPassword}) ->
true
end,
config:get("admins")
).
-%% Current scheme, much stronger.
--spec pbkdf2(binary(), binary(), integer()) -> binary().
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(Password, Salt, Iterations) ->
+pbkdf2(sha, Password, Salt, Iterations).
+
+pbkdf2(PRF, Password, Salt, Iterations) when is_atom(PRF) ->
+#{size := Size} = crypto:hash_info(PRF),
+pbkdf2(PRF, Password, Salt, Iterations, Size);
+pbkdf2(Password, Salt, Iterations, KeyLen) ->
+pbkdf2(sha, Password, Salt, Iterations, KeyLen).
+
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
->
-{ok, Result} = pbkdf2(Password, Salt, Iterations, ?SHA1_OUTPUT_LENGTH),
-Result;
-pbkdf2(Password, Salt, Iterations) when
+DerivedKey = fast_pbkdf2:pbkdf2(PRF, Password, Salt, Iterations, KeyLen),
+couch_util:to_hex_bin(DerivedKey);
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
->
Msg = io_lib:format("Password value of '~p' is invalid.", [Password]),
throw({forbidden, Msg});
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
Review Comment:
Let's assert KeyLen is an integer, too
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378379988
##
src/couch/src/couch_passwords.erl:
##
@@ -69,106 +68,74 @@ get_unhashed_admins() ->
({_User, "-pbkdf2-" ++ _}) ->
% already hashed
false;
+({_User, "-pbkdf2:sha-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha224-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha256-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha384-" ++ _}) ->
+% already hashed
+false;
+({_User, "-pbkdf2:sha512-" ++ _}) ->
+% already hashed
+false;
({_User, _ClearPassword}) ->
true
end,
config:get("admins")
).
-%% Current scheme, much stronger.
--spec pbkdf2(binary(), binary(), integer()) -> binary().
-pbkdf2(Password, Salt, Iterations) when
+pbkdf2(Password, Salt, Iterations) ->
+pbkdf2(sha, Password, Salt, Iterations).
+
+pbkdf2(PRF, Password, Salt, Iterations) when is_atom(PRF) ->
+#{size := Size} = crypto:hash_info(PRF),
+pbkdf2(PRF, Password, Salt, Iterations, Size);
+pbkdf2(Password, Salt, Iterations, KeyLen) ->
+pbkdf2(sha, Password, Salt, Iterations, KeyLen).
+
+pbkdf2(PRF, Password, Salt, Iterations, KeyLen) when
+is_atom(PRF),
is_binary(Password),
is_binary(Salt),
is_integer(Iterations),
-Iterations > 0
+Iterations > 0,
+KeyLen > 0
Review Comment:
If we check a value like `KeyLen` for `> 0` it's usually a good practice to
always assert it's an integer as well.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378379078
##
src/couch/src/couch_password_hasher.erl:
##
@@ -45,29 +66,95 @@ start_link() ->
init(_Args) ->
hash_admin_passwords(true),
-{ok, #state{}}.
+?IN_PROGRESS_ETS = ets:new(?IN_PROGRESS_ETS, [named_table,
{read_concurrency, true}]),
+{ok, start_worker_loop(#state{})}.
handle_call(Msg, _From, #state{} = State) ->
{stop, {invalid_call, Msg}, {invalid_call, Msg}, State}.
-handle_cast({hash_passwords, Persist}, State) ->
-hash_admin_passwords(Persist),
+handle_cast({upgrade_password_hash, AuthModule, UserName, Password,
UserProps}, State) ->
+case ets:insert_new(?IN_PROGRESS_ETS, {{AuthModule, UserName}}) of
+true ->
+State#state.worker_pid !
+{upgrade_password_hash, AuthModule, UserName, Password,
UserProps};
+false ->
+ok
+end,
+{noreply, State};
+handle_cast({hash_admin_passwords, Persist}, State) ->
+hash_admin_passwords_int(Persist),
{noreply, State};
handle_cast(Msg, State) ->
{stop, {invalid_cast, Msg}, State}.
+handle_info({done, AuthModule, UserName}, State) ->
+ets:delete(?IN_PROGRESS_ETS, {AuthModule, UserName}),
+{noreply, State};
+handle_info({'DOWN', MonRef, _, _, normal}, #state{worker_mon = MonRef} =
State) ->
+ets:delete_all_objects(?IN_PROGRESS_ETS),
+{noreply, start_worker_loop(State)};
+handle_info(Msg, State) ->
+{stop, {invalid_info, Msg}, State}.
+
code_change(_OldVsn, #state{} = State, _Extra) ->
{ok, State}.
%%%===
%%% Internal functions
%%%===
-hash_admin_passwords(Persist) ->
+hash_admin_passwords_int(Persist) ->
lists:foreach(
fun({User, ClearPassword}) ->
HashedPassword =
couch_passwords:hash_admin_password(ClearPassword),
config:set("admins", User, ?b2l(HashedPassword), Persist)
end,
couch_passwords:get_unhashed_admins()
).
+
+is_admin(UserProps) ->
+Roles = couch_util:get_value(<<"roles">>, UserProps, []),
+lists:member(<<"_admin">>, Roles).
+
+needs_upgrade(UserProps) ->
+CurrentScheme = couch_util:get_value(<<"password_scheme">>, UserProps,
<<"simple">>),
+TargetScheme = ?l2b(chttpd_util:get_chttpd_auth_config("password_scheme",
"pbkdf2")),
+CurrentPRF = couch_util:get_value(<<"pbkdf2_prf">>, UserProps, <<"sha">>),
+TargetPRF = ?l2b(chttpd_util:get_chttpd_auth_config("pbkdf2_prf",
"sha256")),
+CurrentIterations = couch_util:to_integer(
+couch_util:get_value(<<"iterations">>, UserProps, "10")
+),
+TargetIterations = chttpd_util:get_chttpd_auth_config_integer(
+"iterations", 10
+),
+if
+CurrentScheme == TargetScheme andalso TargetScheme == <<"simple">>
andalso
+CurrentIterations == TargetIterations ->
+false;
+CurrentScheme == TargetScheme andalso TargetScheme == <<"pbkdf2">>
andalso
+CurrentPRF == TargetPRF andalso CurrentIterations ==
TargetIterations ->
+false;
+true ->
+true
+end.
+
+in_progress(AuthModule, UserName) ->
+ets:member(?IN_PROGRESS_ETS, {AuthModule, UserName}).
+
+start_worker_loop(State) ->
+{WorkerPid, WorkerMon} = spawn_monitor(?MODULE, worker_loop, [self()]),
Review Comment:
`spawn_monitor` will keep the worker up even after its parent crashes.
Should we `spawn_link` it instead and then in a `terminate/2` function to
`unlink` and `kill` it?
If we expect it to crash often we could set the parent to trap exits and
then react to an `{'EXIT', ...}` to log an error, cleanup the table and restart
it again.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378376532
##
src/couch/src/couch_password_hasher.erl:
##
@@ -45,29 +66,95 @@ start_link() ->
init(_Args) ->
hash_admin_passwords(true),
-{ok, #state{}}.
+?IN_PROGRESS_ETS = ets:new(?IN_PROGRESS_ETS, [named_table,
{read_concurrency, true}]),
+{ok, start_worker_loop(#state{})}.
handle_call(Msg, _From, #state{} = State) ->
{stop, {invalid_call, Msg}, {invalid_call, Msg}, State}.
-handle_cast({hash_passwords, Persist}, State) ->
-hash_admin_passwords(Persist),
+handle_cast({upgrade_password_hash, AuthModule, UserName, Password,
UserProps}, State) ->
+case ets:insert_new(?IN_PROGRESS_ETS, {{AuthModule, UserName}}) of
+true ->
+State#state.worker_pid !
+{upgrade_password_hash, AuthModule, UserName, Password,
UserProps};
+false ->
+ok
+end,
+{noreply, State};
+handle_cast({hash_admin_passwords, Persist}, State) ->
+hash_admin_passwords_int(Persist),
{noreply, State};
handle_cast(Msg, State) ->
{stop, {invalid_cast, Msg}, State}.
+handle_info({done, AuthModule, UserName}, State) ->
+ets:delete(?IN_PROGRESS_ETS, {AuthModule, UserName}),
+{noreply, State};
+handle_info({'DOWN', MonRef, _, _, normal}, #state{worker_mon = MonRef} =
State) ->
+ets:delete_all_objects(?IN_PROGRESS_ETS),
+{noreply, start_worker_loop(State)};
Review Comment:
It doesn't seem like the worker would ever exit `normal`. As soon as
`receive` expression returns, recurse right back to the top. We expect it to
run as long as password hasher gen_server is up.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378374291
##
src/couch/src/couch_password_hasher.erl:
##
@@ -21,20 +21,41 @@
init/1,
handle_call/3,
handle_cast/2,
+handle_info/2,
code_change/3
]).
--export([hash/1]).
+-export([maybe_upgrade_password_hash/4, hash_admin_passwords/1]).
--record(state, {}).
+-export([worker_loop/1]).
+
+-define(IN_PROGRESS_ETS, couch_password_hasher_in_progress).
+
+-record(state, {
+worker_pid,
+worker_mon
+}).
%%%===
%%% Public functions
%%%===
--spec hash(Persist :: boolean()) -> Reply :: term().
-hash(Persist) ->
-gen_server:cast(?MODULE, {hash_passwords, Persist}).
+maybe_upgrade_password_hash(AuthModule, UserName, Password, UserProps) ->
+IsAdmin = is_admin(UserProps),
+NeedsUpgrade = needs_upgrade(UserProps),
+InProgress = in_progress(AuthModule, UserName),
+if
+not IsAdmin andalso NeedsUpgrade andalso not InProgress ->
Review Comment:
We upgrade user doc passwords but not admins. Would it make sense to have a
way to upgrade those too?
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
nickva commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1788364777 @pgj excellent find! > Given that the other issue was fixed by unconditionally applying a shim on top of the ICU string comparison call, we should do the same here? @nickva what do you think? It could work. The previous fix was easy enough as it was an isolated element -- a single marker. This one might involve doing some string wrangling in C, which is always fun, and by fun I mean dangerous, of course ;-) I guess we could extend the current check to first see if the string ends in `\uF` and handle that special case. -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378329308
##
src/couch/src/couch_password_hasher.erl:
##
@@ -45,29 +66,95 @@ start_link() ->
init(_Args) ->
hash_admin_passwords(true),
-{ok, #state{}}.
+?IN_PROGRESS_ETS = ets:new(?IN_PROGRESS_ETS, [named_table,
{read_concurrency, true}]),
+{ok, start_worker_loop(#state{})}.
handle_call(Msg, _From, #state{} = State) ->
{stop, {invalid_call, Msg}, {invalid_call, Msg}, State}.
-handle_cast({hash_passwords, Persist}, State) ->
-hash_admin_passwords(Persist),
+handle_cast({upgrade_password_hash, AuthModule, UserName, Password,
UserProps}, State) ->
+case ets:insert_new(?IN_PROGRESS_ETS, {{AuthModule, UserName}}) of
+true ->
+State#state.worker_pid !
+{upgrade_password_hash, AuthModule, UserName, Password,
UserProps};
+false ->
+ok
+end,
+{noreply, State};
+handle_cast({hash_admin_passwords, Persist}, State) ->
+hash_admin_passwords_int(Persist),
{noreply, State};
handle_cast(Msg, State) ->
{stop, {invalid_cast, Msg}, State}.
+handle_info({done, AuthModule, UserName}, State) ->
+ets:delete(?IN_PROGRESS_ETS, {AuthModule, UserName}),
+{noreply, State};
+handle_info({'DOWN', MonRef, _, _, normal}, #state{worker_mon = MonRef} =
State) ->
+ets:delete_all_objects(?IN_PROGRESS_ETS),
+{noreply, start_worker_loop(State)};
+handle_info(Msg, State) ->
+{stop, {invalid_info, Msg}, State}.
+
code_change(_OldVsn, #state{} = State, _Extra) ->
{ok, State}.
%%%===
%%% Internal functions
%%%===
-hash_admin_passwords(Persist) ->
+hash_admin_passwords_int(Persist) ->
lists:foreach(
fun({User, ClearPassword}) ->
HashedPassword =
couch_passwords:hash_admin_password(ClearPassword),
config:set("admins", User, ?b2l(HashedPassword), Persist)
end,
couch_passwords:get_unhashed_admins()
).
+
+is_admin(UserProps) ->
+Roles = couch_util:get_value(<<"roles">>, UserProps, []),
+lists:member(<<"_admin">>, Roles).
+
+needs_upgrade(UserProps) ->
+CurrentScheme = couch_util:get_value(<<"password_scheme">>, UserProps,
<<"simple">>),
+TargetScheme = ?l2b(chttpd_util:get_chttpd_auth_config("password_scheme",
"pbkdf2")),
+CurrentPRF = couch_util:get_value(<<"pbkdf2_prf">>, UserProps, <<"sha">>),
+TargetPRF = ?l2b(chttpd_util:get_chttpd_auth_config("pbkdf2_prf",
"sha256")),
+CurrentIterations = couch_util:to_integer(
+couch_util:get_value(<<"iterations">>, UserProps, "10")
+),
+TargetIterations = chttpd_util:get_chttpd_auth_config_integer(
+"iterations", 10
+),
+if
+CurrentScheme == TargetScheme andalso TargetScheme == <<"simple">>
andalso
+CurrentIterations == TargetIterations ->
+false;
+CurrentScheme == TargetScheme andalso TargetScheme == <<"pbkdf2">>
andalso
+CurrentPRF == TargetPRF andalso CurrentIterations ==
TargetIterations ->
+false;
+true ->
+true
+end.
Review Comment:
minor style nit: another way to structure it could be something like
```erlang
case {TargetScheme, TargetIterations, TargetPRF} of
{CurrentScheme, CurrentIterations, _} when CurrentScheme == <<"simple">>
-> false;
{CurrentScheme, CurrentIterations, CurrentPRF} when CurrentScheme ==
<<"pbkdf2">> -> false;
{_, _, _} -> true
end
```
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
pgj commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1788169233 Yeah, basically [the lack of support for `0x` in older ICU versions](https://www.unicode.org/reports/tr35/tr35-collation.html#tailored_noncharacter_weights) strikes again here, similar to what happened in #3491. As the standard says: _U+: This code point is tailored to have a primary weight higher than all other characters. This allows the reliable specification of a range, such as “Sch” ≤ X ≤ “Sch\u”, to include all strings starting with "sch" or equivalent._ Given that the other issue was fixed by unconditionally applying a shim on top of the ICU string comparison call, we should do the same here? @nickva what do you think? -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
rnewson commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378048823
##
src/couch/src/couch_users_db.erl:
##
@@ -63,6 +64,21 @@ before_doc_update(Doc, Db, _UpdateType) ->
save_doc(#doc{body = {Body}} = Doc) ->
%% Support both schemes to smooth migration from legacy scheme
Scheme = chttpd_util:get_chttpd_auth_config("password_scheme", "pbkdf2"),
+
+% We preserve the salt value if requested (for a hashing upgrade,
typically)
+% in order to avoid conflicts if multiple nodes try to upgrade at the same
time
+% and to avoid invalidating existing session cookies (since the password
did not
+% change).
+PreserveSalt = couch_util:get_value(?PRESERVE_SALT, Body, false),
+Salt =
+if
+PreserveSalt ->
+% use existing salt, if present.
+couch_util:get_value(?SALT, Body, couch_uuids:random());
+true ->
+couch_uuids:random()
Review Comment:
sure
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Decouple offline hash strength from online [couchdb]
nickva commented on code in PR #4814:
URL: https://github.com/apache/couchdb/pull/4814#discussion_r1378046612
##
src/couch/src/couch_users_db.erl:
##
@@ -63,6 +64,21 @@ before_doc_update(Doc, Db, _UpdateType) ->
save_doc(#doc{body = {Body}} = Doc) ->
%% Support both schemes to smooth migration from legacy scheme
Scheme = chttpd_util:get_chttpd_auth_config("password_scheme", "pbkdf2"),
+
+% We preserve the salt value if requested (for a hashing upgrade,
typically)
+% in order to avoid conflicts if multiple nodes try to upgrade at the same
time
+% and to avoid invalidating existing session cookies (since the password
did not
+% change).
+PreserveSalt = couch_util:get_value(?PRESERVE_SALT, Body, false),
+Salt =
+if
+PreserveSalt ->
+% use existing salt, if present.
+couch_util:get_value(?SALT, Body, couch_uuids:random());
+true ->
+couch_uuids:random()
Review Comment:
Minor nit: `if true -> ... ; true -> ... end` even if correct will look
confusing. A simple `case` might be more clear instead?
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [I] Need to limit access to Fauxton UI to localhost, but maintain remote API access. [couchdb]
rnewson commented on issue #4827: URL: https://github.com/apache/couchdb/issues/4827#issuecomment-1787623697 To what end? Fauxton is just calling the API, it has no special privileges. -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[I] Need to limit access to Fauxton UI to localhost, but maintain remote API access. [couchdb]
estoT1 opened a new issue, #4827: URL: https://github.com/apache/couchdb/issues/4827 Need to limit access to Fauxton UI to localhost, but maintain remote API access. Do I understand correctly that the Fauxton UI cannot be disabled or limited to localhost separately from the other APIs? Other than messing with the share/www directory or using a front end proxy to route remote API requests to localhost bind address? Thank you in advance. -- 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: notifications-unsubscr...@couchdb.apache.org.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
pgj commented on PR #4810:
URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1787413943
It is the derived string upper limit that does not work. Consider the
following:
```erlang
> Lower = <<"A">>.
<<"A">>.
> Upper = <>.
<<"A\377">>.
```
On CentOS 7:
```erlang
> couch_ejson_compare:get_icu_version().
{50,2,0,0}
> couch_ejson_compare:get_collator_version().
{58,0,6,50}
> mango_json:cmp(Lower, <<"AUS">>).
-1
> mango_json:cmp(<<"AUS">>, Upper).
1
```
On anywhere else (for example, locally):
```erlang
> couch_ejson_compare:get_icu_version().
{73,2,0,0}
> couch_ejson_compare:get_collator_version().
{153,120,0,0}
> mango_json:cmp(Lower, <<"AUS">>).
-1
> mango_json:cmp(<<"AUS">>, Upper).
-1
```
The `mango_json:cmp/2` function should return `-1` for every case. That
signals the "less than" result, which should hold for all. Apparently, `icu`
50 disagrees (as @nickva suspected) hence no documents are returned for the
range.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
big-r81 commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1787391987 @pgj can you test on your local CentOS vm the state before the PR? -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
pgj commented on PR #4810:
URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1787188105
> it definitely smells like a collation issue and likely an issue with the
special characters used in the tests
In my understanding, the two failing test cases do not feature any special
characters. Neither the selectors nor the documents. C.f. "begins with `A`"
vs. { `"AUS"`, `"AND"`, `"CAN"`, `"DEN"`, `"ETH"` }.
--
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: notifications-unsubscr...@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
big-r81 commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1786846495 @willholley np, only a reminder. We will add this review-branch-protection in the future. At the moment, we will get the "green" merge button, if the CI runs through... The failure didn't show up in the PR checks, because CentOS and other OSes are only called in the full CI run. For PRs, we only test a linux distro (debian) with different erlang version (24,25,26). -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
willholley commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1786790838 curious that the failure didn't show in the PR checks - it definitely smells like a collation issue and likely an issue with the special characters used in the tests rather than the implementation of `$beginsWith` -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
willholley commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1786788940 @big-r81 oops on the premature merge - I'm too used to working with repos with branch protection enabled in GH and didn't think to manually check for the +1. -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] mango: add $beginsWith operator [couchdb]
nickva commented on PR #4810: URL: https://github.com/apache/couchdb/pull/4810#issuecomment-1786501544 > It is not flaky, these errors happen every time -- but only on CentOS 7. Oh very interesting, definitely worth investigating. One thing that immediately jumps to mind is that CentOS 7 has an older libicu library which may have different collation rules. Any end/start (min/max) marker perhaps might collate differently there. -- 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: notifications-unsubscr...@couchdb.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
