rnewson commented on code in PR #4140:
URL: https://github.com/apache/couchdb/pull/4140#discussion_r942157270
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -308,15 +309,18 @@ cookie_authentication_handler(#httpd{mochi_req =
MochiReq} = Req, AuthModule) ->
{ok, UserProps, _AuthCtx} ->
UserSalt = couch_util:get_value(<<"salt">>,
UserProps, <<"">>),
FullSecret = <<Secret/binary, UserSalt/binary>>,
- ExpectedHash = couch_util:hmac(sha, FullSecret,
User ++ ":" ++ TimeStr),
Hash = ?l2b(HashStr),
+ VerifyHash = fun(HashAlg) ->
+ Hmac = couch_util:hmac(HashAlg, FullSecret,
User ++ ":" ++ TimeStr),
+ couch_passwords:verify(Hmac, Hash)
+ end,
Timeout =
chttpd_util:get_chttpd_auth_config_integer(
"timeout", 600
),
couch_log:debug("timeout ~p", [Timeout]),
case (catch erlang:list_to_integer(TimeStr, 16)) of
TimeStamp when CurrentTime < TimeStamp +
Timeout ->
- case couch_passwords:verify(ExpectedHash,
Hash) of
+ case lists:any(VerifyHash, HashAlgorithms)
of
Review Comment:
nice.
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -695,3 +700,33 @@ authentication_warning(#httpd{mochi_req = Req}, User) ->
"~p: Authentication failed for user ~s from ~s",
[?MODULE, User, Peer]
).
+
+verify_hash_names(HashAlgorithms, SupportedHashFun) ->
+ verify_hash_names(HashAlgorithms, SupportedHashFun, []).
+verify_hash_names([], _, HashNames) ->
+ HashNames;
+verify_hash_names([H | T], SupportedHashFun, HashNames) ->
+ try
+ HashAtom = binary_to_existing_atom(H),
+ Result =
+ case lists:member(HashAtom, SupportedHashFun) of
+ true -> HashNames ++ [HashAtom];
+ false -> HashNames
+ end,
+ verify_hash_names(T, SupportedHashFun, Result)
+ catch
+ _:_ ->
+ couch_log:warning("~p: Hash algorithm ~s is not available.",
[?MODULE, H]),
+ verify_hash_names(T, SupportedHashFun, HashNames)
+ end.
+
+-spec get_config_hash_algorithms() -> list(atom()).
+get_config_hash_algorithms() ->
+ SupportedHashAlgorithms = crypto:supports(hashs),
+ HashAlgorithmsStr = chttpd_util:get_chttpd_auth_config("hash_algorithms",
"sha256, sha"),
+ HashAlgorithms = re:split(HashAlgorithmsStr, "\\s*,\\s*", [trim, {return,
binary}]),
+ VerifiedHashNames = verify_hash_names(HashAlgorithms,
SupportedHashAlgorithms),
+ if
+ length(VerifiedHashNames) == 0 -> [sha256];
Review Comment:
it's more common to compare against the empty list `[]`.
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -695,3 +700,33 @@ authentication_warning(#httpd{mochi_req = Req}, User) ->
"~p: Authentication failed for user ~s from ~s",
[?MODULE, User, Peer]
).
+
+verify_hash_names(HashAlgorithms, SupportedHashFun) ->
+ verify_hash_names(HashAlgorithms, SupportedHashFun, []).
+verify_hash_names([], _, HashNames) ->
+ HashNames;
+verify_hash_names([H | T], SupportedHashFun, HashNames) ->
+ try
+ HashAtom = binary_to_existing_atom(H),
+ Result =
+ case lists:member(HashAtom, SupportedHashFun) of
+ true -> HashNames ++ [HashAtom];
+ false -> HashNames
+ end,
+ verify_hash_names(T, SupportedHashFun, Result)
+ catch
+ _:_ ->
+ couch_log:warning("~p: Hash algorithm ~s is not available.",
[?MODULE, H]),
Review Comment:
suggest 'valid' instead of 'available'.
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -695,3 +700,33 @@ authentication_warning(#httpd{mochi_req = Req}, User) ->
"~p: Authentication failed for user ~s from ~s",
[?MODULE, User, Peer]
).
+
+verify_hash_names(HashAlgorithms, SupportedHashFun) ->
+ verify_hash_names(HashAlgorithms, SupportedHashFun, []).
+verify_hash_names([], _, HashNames) ->
+ HashNames;
+verify_hash_names([H | T], SupportedHashFun, HashNames) ->
+ try
+ HashAtom = binary_to_existing_atom(H),
+ Result =
+ case lists:member(HashAtom, SupportedHashFun) of
+ true -> HashNames ++ [HashAtom];
+ false -> HashNames
+ end,
+ verify_hash_names(T, SupportedHashFun, Result)
+ catch
+ _:_ ->
Review Comment:
catch only the error you expect `error:badarg`
##########
src/couch/src/couch_httpd_auth.erl:
##########
@@ -695,3 +700,33 @@ authentication_warning(#httpd{mochi_req = Req}, User) ->
"~p: Authentication failed for user ~s from ~s",
[?MODULE, User, Peer]
).
+
+verify_hash_names(HashAlgorithms, SupportedHashFun) ->
+ verify_hash_names(HashAlgorithms, SupportedHashFun, []).
+verify_hash_names([], _, HashNames) ->
+ HashNames;
+verify_hash_names([H | T], SupportedHashFun, HashNames) ->
+ try
+ HashAtom = binary_to_existing_atom(H),
+ Result =
+ case lists:member(HashAtom, SupportedHashFun) of
+ true -> HashNames ++ [HashAtom];
Review Comment:
while it's unlikely a problem here, with very short lists, we try to avoid
`++`. since order doesn't matter I think `[HashAtom | HashNames]` is preferable.
--
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]
