From 2262bf508edbc5b7d87930d561162e4cd6090f23 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Mon, 27 Sep 2021 14:50:22 +0800 Subject: [PATCH] chore(authn): improve code --- apps/emqx/src/emqx_authentication.erl | 2 +- apps/emqx_authn/src/emqx_authn_utils.erl | 26 +++++++++- .../src/simple_authn/emqx_authn_http.erl | 7 ++- .../emqx_authn_jwks_connector.erl | 11 +++-- .../src/simple_authn/emqx_authn_mongodb.erl | 10 ++-- .../src/simple_authn/emqx_authn_mysql.erl | 45 +++++++++-------- .../src/simple_authn/emqx_authn_pgsql.erl | 28 +++-------- .../src/simple_authn/emqx_authn_redis.erl | 48 +++++++------------ 8 files changed, 89 insertions(+), 88 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 341103781..3b408723c 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -310,7 +310,7 @@ do_authenticate([#authenticator{id = ID, provider = Provider, state = State} | M {stop, Result} catch Class:Reason:Stacktrace -> - ?SLOG(warning, #{msg => "an unexpected error in authentication", + ?SLOG(warning, #{msg => "unexpected_error_in_authentication", class => Class, reason => Reason, stacktrace => Stacktrace, diff --git a/apps/emqx_authn/src/emqx_authn_utils.erl b/apps/emqx_authn/src/emqx_authn_utils.erl index c0ba8a549..4784c91c7 100644 --- a/apps/emqx_authn/src/emqx_authn_utils.erl +++ b/apps/emqx_authn/src/emqx_authn_utils.erl @@ -18,6 +18,8 @@ -export([ replace_placeholders/2 , replace_placeholder/2 + , check_password/3 + , is_superuser/1 , hash/4 , gen_salt/0 , bin/1 @@ -55,6 +57,28 @@ replace_placeholder(<<"${cert-common-name}">>, Credential) -> replace_placeholder(Constant, _) -> Constant. +check_password(undefined, _Selected, _State) -> + {error, bad_username_or_password}; +check_password(Password, + #{<<"password_hash">> := Hash}, + #{password_hash_algorithm := bcrypt}) -> + case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of + true -> ok; + false -> {error, bad_username_or_password} + end; +check_password(Password, + #{<<"password_hash">> := Hash} = Selected, + #{password_hash_algorithm := Algorithm, + salt_position := SaltPosition}) -> + Salt = maps:get(<<"salt">>, Selected, <<>>), + case Hash =:= hash(Algorithm, Password, Salt, SaltPosition) of + true -> ok; + false -> {error, bad_username_or_password} + end. + +is_superuser(Selected) -> + #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}. + hash(Algorithm, Password, Salt, prefix) -> emqx_passwd:hash(Algorithm, <>); hash(Algorithm, Password, Salt, suffix) -> @@ -75,4 +99,4 @@ bin(X) -> X. convert_to_sql_param(undefined) -> null; convert_to_sql_param(V) -> - bin(V). \ No newline at end of file + bin(V). diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl index 8659605c4..536cd544a 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -165,11 +165,14 @@ authenticate(Credential, #{'_unique' := Unique, {ok, NBody} -> %% TODO: Return by user property {ok, #{is_superuser => maps:get(<<"is_superuser">>, NBody, false), - user_property => NBody}}; + user_property => NBody}}; {error, _Reason} -> {ok, #{is_superuser => false}} end; - {error, _Reason} -> + {error, Reason} -> + ?SLOG(error, #{msg => "http_server_query_failed", + resource => Unique, + reason => Reason}), ignore end. diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl index 6ec57a4de..89bc565c6 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_jwks_connector.erl @@ -94,7 +94,7 @@ handle_info({http, {RequestID, Result}}, State1 = State0#{request_id := undefined}, case Result of {error, Reason} -> - ?SLOG(warning, #{msg => "failed to request jwks endpoint", + ?SLOG(warning, #{msg => "failed_to_request_jwks_endpoint", endpoint => Endpoint, reason => Reason}), State1; @@ -104,7 +104,7 @@ handle_info({http, {RequestID, Result}}, {_, JWKs} = JWKS#jose_jwk.keys, State1#{jwks := JWKs} catch _:_ -> - ?SLOG(warning, #{msg => "invalid jwks returned from jwks endpoint", + ?SLOG(warning, #{msg => "invalid_jwks_returned", endpoint => Endpoint, body => Body}), State1 @@ -140,11 +140,14 @@ handle_options(#{endpoint := Endpoint, refresh_jwks(#{endpoint := Endpoint, ssl_opts := SSLOpts} = State) -> - HTTPOpts = [{timeout, 5000}, {connect_timeout, 5000}, {ssl, SSLOpts}], + HTTPOpts = [ {timeout, 5000} + , {connect_timeout, 5000} + , {ssl, SSLOpts} + ], NState = case httpc:request(get, {Endpoint, [{"Accept", "application/json"}]}, HTTPOpts, [{body_format, binary}, {sync, false}, {receiver, self()}]) of {error, Reason} -> - ?SLOG(warning, #{msg => "failed to request jwks endpoint", + ?SLOG(warning, #{msg => "failed_to_request_jwks_endpoint", endpoint => Endpoint, reason => Reason}), State; diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index 4ae98d07d..e2459ffe8 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -146,8 +146,8 @@ authenticate(#{password := Password} = Credential, case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of undefined -> ignore; {error, Reason} -> - ?SLOG(error, #{msg => "query failed", - unique => Unique, + ?SLOG(error, #{msg => "mongodb_query_failed", + resource => Unique, reason => Reason}), ignore; Doc -> @@ -155,10 +155,10 @@ authenticate(#{password := Password} = Credential, ok -> {ok, #{is_superuser => is_superuser(Doc, State)}}; {error, {cannot_find_password_hash_field, PasswordHashField}} -> - ?SLOG(error, #{msg => "can't find password hash field", - unique => Unique, + ?SLOG(error, #{msg => "cannot_find_password_hash_field", + resource => Unique, password_hash_field => PasswordHashField}), - {error, bad_username_or_password}; + ignore; {error, Reason} -> {error, Reason} end diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl index 2f614a249..a838d216d 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -119,13 +119,16 @@ authenticate(#{password := Password} = Credential, {ok, _Columns, []} -> ignore; {ok, Columns, Rows} -> Selected = maps:from_list(lists:zip(Columns, Rows)), - case check_password(Password, Selected, State) of + case emqx_authn_utils:check_password(Password, Selected, State) of ok -> - {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; + {ok, emqx_authn_utils:is_superuser(Selected)}; {error, Reason} -> {error, Reason} end; - {error, _Reason} -> + {error, Reason} -> + ?SLOG(error, #{msg => "mysql_query_failed", + resource => Unique, + reason => Reason}), ignore end. @@ -137,24 +140,24 @@ destroy(#{'_unique' := Unique}) -> %% Internal functions %%------------------------------------------------------------------------------ -check_password(undefined, _Selected, _State) -> - {error, bad_username_or_password}; -check_password(Password, - #{<<"password_hash">> := Hash}, - #{password_hash_algorithm := bcrypt}) -> - case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of - true -> ok; - false -> {error, bad_username_or_password} - end; -check_password(Password, - #{<<"password_hash">> := Hash} = Selected, - #{password_hash_algorithm := Algorithm, - salt_position := SaltPosition}) -> - Salt = maps:get(<<"salt">>, Selected, <<>>), - case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of - true -> ok; - false -> {error, bad_username_or_password} - end. +% check_password(undefined, _Selected, _State) -> +% {error, bad_username_or_password}; +% check_password(Password, +% #{<<"password_hash">> := Hash}, +% #{password_hash_algorithm := bcrypt}) -> +% case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of +% true -> ok; +% false -> {error, bad_username_or_password} +% end; +% check_password(Password, +% #{<<"password_hash">> := Hash} = Selected, +% #{password_hash_algorithm := Algorithm, +% salt_position := SaltPosition}) -> +% Salt = maps:get(<<"salt">>, Selected, <<>>), +% case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of +% true -> ok; +% false -> {error, bad_username_or_password} +% end. %% TODO: Support prepare parse_query(Query) -> diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl index d626e2dab..99b83844b 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -109,13 +109,16 @@ authenticate(#{password := Password} = Credential, {ok, Columns, Rows} -> NColumns = [Name || #column{name = Name} <- Columns], Selected = maps:from_list(lists:zip(NColumns, Rows)), - case check_password(Password, Selected, State) of + case emqx_authn_utils:check_password(Password, Selected, State) of ok -> - {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; + {ok, emqx_authn_utils:is_superuser(Selected)}; {error, Reason} -> {error, Reason} end; - {error, _Reason} -> + {error, Reason} -> + ?SLOG(error, #{msg => "postgresql_query_failed", + resource => Unique, + reason => Reason}), ignore end. @@ -127,25 +130,6 @@ destroy(#{'_unique' := Unique}) -> %% Internal functions %%------------------------------------------------------------------------------ -check_password(undefined, _Selected, _State) -> - {error, bad_username_or_password}; -check_password(Password, - #{<<"password_hash">> := Hash}, - #{password_hash_algorithm := bcrypt}) -> - case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of - true -> ok; - false -> {error, bad_username_or_password} - end; -check_password(Password, - #{<<"password_hash">> := Hash} = Selected, - #{password_hash_algorithm := Algorithm, - salt_position := SaltPosition}) -> - Salt = maps:get(<<"salt">>, Selected, <<>>), - case Hash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of - true -> ok; - false -> {error, bad_username_or_password} - end. - %% TODO: Support prepare parse_query(Query) -> case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl index 572ae9a2c..9b8dbefbf 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -130,16 +130,22 @@ authenticate(#{password := Password} = Credential, NKey = binary_to_list(iolist_to_binary(replace_placeholders(Key, Credential))), case emqx_resource:query(Unique, {cmd, [Command, NKey | Fields]}) of {ok, Values} -> - Selected = merge(Fields, Values), - case check_password(Password, Selected, State) of - ok -> - {ok, #{is_superuser => maps:get("is_superuser", Selected, false)}}; - {error, Reason} -> - {error, Reason} + case merge(Fields, Values) of + #{<<"password_hash">> := _} = Selected -> + case emqx_authn_utils:check_password(Password, Selected, State) of + ok -> + {ok, emqx_authn_utils:is_superuser(Selected)}; + {error, Reason} -> + {error, Reason} + end; + _ -> + ?SLOG(error, #{msg => "cannot_find_password_hash_field", + resource => Unique}), + ignore end; {error, Reason} -> - ?SLOG(error, #{msg => "query failed", - unique => Unique, + ?SLOG(error, #{msg => "redis_query_failed", + resource => Unique, reason => Reason}), ignore end. @@ -205,27 +211,5 @@ merge(Fields, Value) when not is_list(Value) -> merge(Fields, [Value]); merge(Fields, Values) -> maps:from_list( - lists:filter(fun({_, V}) -> - V =/= undefined - end, lists:zip(Fields, Values))). - -check_password(undefined, _Selected, _State) -> - {error, bad_username_or_password}; -check_password(Password, - #{"password_hash" := PasswordHash}, - #{password_hash_algorithm := bcrypt}) -> - case {ok, PasswordHash} =:= bcrypt:hashpw(Password, PasswordHash) of - true -> ok; - false -> {error, bad_username_or_password} - end; -check_password(Password, - #{"password_hash" := PasswordHash} = Selected, - #{password_hash_algorithm := Algorithm, - salt_position := SaltPosition}) -> - Salt = maps:get("salt", Selected, <<>>), - case PasswordHash =:= emqx_authn_utils:hash(Algorithm, Password, Salt, SaltPosition) of - true -> ok; - false -> {error, bad_username_or_password} - end; -check_password(_Password, _Selected, _State) -> - ignore. + [{list_to_binary(K), V} + || {K, V} <- lists:zip(Fields, Values), V =/= undefined]).