From e31840d943ffb1e11ba28d4ad9cb9fb7e9179905 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Thu, 23 Sep 2021 16:40:33 +0800 Subject: [PATCH 1/6] fix(authn): add timeout option for mysql connector --- apps/emqx/src/emqx_authentication.erl | 8 +++- .../src/simple_authn/emqx_authn_http.erl | 34 +++++++--------- .../src/simple_authn/emqx_authn_mongodb.erl | 40 ++++++++----------- .../src/simple_authn/emqx_authn_mysql.erl | 30 ++++++-------- .../src/simple_authn/emqx_authn_pgsql.erl | 32 ++++++--------- .../src/simple_authn/emqx_authn_redis.erl | 30 ++++++-------- .../src/emqx_connector_mysql.erl | 10 +++-- .../src/emqx_connector_pgsql.erl | 4 +- 8 files changed, 82 insertions(+), 106 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 4200190ac..40af0e7b7 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -303,8 +303,8 @@ authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthRe do_authenticate([], _) -> {stop, {error, not_authorized}}; -do_authenticate([#authenticator{provider = Provider, state = State} | More], Credential) -> - case Provider:authenticate(Credential, State) of +do_authenticate([#authenticator{provider = Provider, state = #{'_unique' := Unique} = State} | More], Credential) -> + try Provider:authenticate(Credential, State) of ignore -> do_authenticate(More, Credential); Result -> @@ -314,6 +314,10 @@ do_authenticate([#authenticator{provider = Provider, state = State} | More], Cre %% {continue, AuthData, AuthCache} %% {error, Reason} {stop, Result} + catch + error:Reason:Stacktrace -> + ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, {Reason, Stacktrace}]), + do_authenticate(More, Credential) end. %%------------------------------------------------------------------------------ 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 2fa29d2df..77a8a94c8 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -156,26 +156,20 @@ authenticate(#{auth_method := _}, _) -> authenticate(Credential, #{'_unique' := Unique, method := Method, request_timeout := RequestTimeout} = State) -> - try - Request = generate_request(Credential, State), - case emqx_resource:query(Unique, {Method, Request, RequestTimeout}) of - {ok, 204, _Headers} -> {ok, #{is_superuser => false}}; - {ok, 200, Headers, Body} -> - ContentType = proplists:get_value(<<"content-type">>, Headers, <<"application/json">>), - case safely_parse_body(ContentType, Body) of - {ok, NBody} -> - %% TODO: Return by user property - {ok, #{is_superuser => maps:get(<<"is_superuser">>, NBody, false), - user_property => NBody}}; - {error, _Reason} -> - {ok, #{is_superuser => false}} - end; - {error, _Reason} -> - ignore - end - catch - error:Reason -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, Reason]), + Request = generate_request(Credential, State), + case emqx_resource:query(Unique, {Method, Request, RequestTimeout}) of + {ok, 204, _Headers} -> {ok, #{is_superuser => false}}; + {ok, 200, Headers, Body} -> + ContentType = proplists:get_value(<<"content-type">>, Headers, <<"application/json">>), + case safely_parse_body(ContentType, Body) of + {ok, NBody} -> + %% TODO: Return by user property + {ok, #{is_superuser => maps:get(<<"is_superuser">>, NBody, false), + user_property => NBody}}; + {error, _Reason} -> + {ok, #{is_superuser => false}} + end; + {error, _Reason} -> ignore end. 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 5ad148009..d348a439c 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -141,29 +141,23 @@ authenticate(#{password := Password} = Credential, , selector := Selector0 , '_unique' := Unique } = State) -> - try - Selector1 = replace_placeholders(Selector0, Credential), - Selector2 = normalize_selector(Selector1), - case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of - undefined -> ignore; - {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), - ignore; - Doc -> - case check_password(Password, Doc, State) of - ok -> - {ok, #{is_superuser => is_superuser(Doc, State)}}; - {error, {cannot_find_password_hash_field, PasswordHashField}} -> - ?LOG(error, "['~s'] Can't find password hash field: ~s", [Unique, PasswordHashField]), - {error, bad_username_or_password}; - {error, Reason} -> - {error, Reason} - end - end - catch - error:Error -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, Error]), - ignore + Selector1 = replace_placeholders(Selector0, Credential), + Selector2 = normalize_selector(Selector1), + case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of + undefined -> ignore; + {error, Reason} -> + ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), + ignore; + Doc -> + case check_password(Password, Doc, State) of + ok -> + {ok, #{is_superuser => is_superuser(Doc, State)}}; + {error, {cannot_find_password_hash_field, PasswordHashField}} -> + ?LOG(error, "['~s'] Can't find password hash field: ~s", [Unique, PasswordHashField]), + {error, bad_username_or_password}; + {error, Reason} -> + {error, Reason} + end end. destroy(#{'_unique' := Unique}) -> 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 87c61da1e..2f614a249 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -114,24 +114,18 @@ authenticate(#{password := Password} = Credential, query := Query, query_timeout := Timeout, '_unique' := Unique} = State) -> - try - Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), - case emqx_resource:query(Unique, {sql, Query, Params, Timeout}) of - {ok, _Columns, []} -> ignore; - {ok, Columns, Rows} -> - Selected = maps:from_list(lists:zip(Columns, Rows)), - case check_password(Password, Selected, State) of - ok -> - {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; - {error, Reason} -> - {error, Reason} - end; - {error, _Reason} -> - ignore - end - catch - error:Error -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, Error]), + Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), + case emqx_resource:query(Unique, {sql, Query, Params, Timeout}) of + {ok, _Columns, []} -> ignore; + {ok, Columns, Rows} -> + Selected = maps:from_list(lists:zip(Columns, Rows)), + case check_password(Password, Selected, State) of + ok -> + {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; + {error, Reason} -> + {error, Reason} + end; + {error, _Reason} -> ignore end. 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 940c50519..78632610e 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -103,25 +103,19 @@ authenticate(#{password := Password} = Credential, #{query := Query, placeholders := PlaceHolders, '_unique' := Unique} = State) -> - try - Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), - case emqx_resource:query(Unique, {sql, Query, Params}) of - {ok, _Columns, []} -> ignore; - {ok, Columns, Rows} -> - NColumns = [Name || #column{name = Name} <- Columns], - Selected = maps:from_list(lists:zip(NColumns, Rows)), - case check_password(Password, Selected, State) of - ok -> - {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; - {error, Reason} -> - {error, Reason} - end; - {error, _Reason} -> - ignore - end - catch - error:Error -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, Error]), + Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), + case emqx_resource:query(Unique, {sql, Query, Params}) of + {ok, _Columns, []} -> ignore; + {ok, Columns, Rows} -> + NColumns = [Name || #column{name = Name} <- Columns], + Selected = maps:from_list(lists:zip(NColumns, Rows)), + case check_password(Password, Selected, State) of + ok -> + {ok, #{is_superuser => maps:get(<<"is_superuser">>, Selected, false)}}; + {error, Reason} -> + {error, Reason} + end; + {error, _Reason} -> ignore end. 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 5926740a8..5d73f9410 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -127,24 +127,18 @@ authenticate(#{password := Password} = Credential, #{ query := {Command, Key, Fields} , '_unique' := Unique } = State) -> - try - 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} - end; - {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), - ignore - end - catch - error:{cannot_get_variable, Placeholder} -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, {cannot_get_variable, Placeholder}]), + 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} + end; + {error, Reason} -> + ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), ignore end. diff --git a/apps/emqx_connector/src/emqx_connector_mysql.erl b/apps/emqx_connector/src/emqx_connector_mysql.erl index 9dc194c55..0cd051512 100644 --- a/apps/emqx_connector/src/emqx_connector_mysql.erl +++ b/apps/emqx_connector/src/emqx_connector_mysql.erl @@ -76,11 +76,13 @@ on_stop(InstId, #{poolname := PoolName}) -> logger:info("stopping mysql connector: ~p", [InstId]), emqx_plugin_libs_pool:stop_pool(PoolName). -on_query(InstId, {sql, SQL}, AfterQuery, #{poolname := PoolName} = State) -> - on_query(InstId, {sql, SQL, []}, AfterQuery, #{poolname := PoolName} = State); -on_query(InstId, {sql, SQL, Params}, AfterQuery, #{poolname := PoolName} = State) -> +on_query(InstId, {sql, SQL}, AfterQuery, #{poolname := _PoolName} = State) -> + on_query(InstId, {sql, SQL, [], default_timeout}, AfterQuery, State); +on_query(InstId, {sql, SQL, Params}, AfterQuery, #{poolname := _PoolName} = State) -> + on_query(InstId, {sql, SQL, Params, default_timeout}, AfterQuery, State); +on_query(InstId, {sql, SQL, Params, Timeout}, AfterQuery, #{poolname := PoolName} = State) -> logger:debug("mysql connector ~p received sql query: ~p, at state: ~p", [InstId, SQL, State]), - case Result = ecpool:pick_and_do(PoolName, {mysql, query, [SQL, Params]}, no_handover) of + case Result = ecpool:pick_and_do(PoolName, {mysql, query, [SQL, Params, Timeout]}, no_handover) of {error, Reason} -> logger:debug("mysql connector ~p do sql query failed, sql: ~p, reason: ~p", [InstId, SQL, Reason]), emqx_resource:query_failed(AfterQuery); diff --git a/apps/emqx_connector/src/emqx_connector_pgsql.erl b/apps/emqx_connector/src/emqx_connector_pgsql.erl index 8472c661e..31e954b3c 100644 --- a/apps/emqx_connector/src/emqx_connector_pgsql.erl +++ b/apps/emqx_connector/src/emqx_connector_pgsql.erl @@ -76,8 +76,8 @@ on_stop(InstId, #{poolname := PoolName}) -> logger:info("stopping postgresql connector: ~p", [InstId]), emqx_plugin_libs_pool:stop_pool(PoolName). -on_query(InstId, {sql, SQL}, AfterQuery, #{poolname := PoolName} = State) -> - on_query(InstId, {sql, SQL, []}, AfterQuery, #{poolname := PoolName} = State); +on_query(InstId, {sql, SQL}, AfterQuery, #{poolname := _PoolName} = State) -> + on_query(InstId, {sql, SQL, []}, AfterQuery, State); on_query(InstId, {sql, SQL, Params}, AfterQuery, #{poolname := PoolName} = State) -> logger:debug("postgresql connector ~p received sql query: ~p, at state: ~p", [InstId, SQL, State]), case Result = ecpool:pick_and_do(PoolName, {?MODULE, query, [SQL, Params]}, no_handover) of From 11bdfcb8f0a3f8e0451a49c28fe0750913636fbc Mon Sep 17 00:00:00 2001 From: zhouzb Date: Fri, 24 Sep 2021 17:04:20 +0800 Subject: [PATCH 2/6] fix(authn): fix bugs in http and pgsql authn --- apps/emqx_authn/src/simple_authn/emqx_authn_http.erl | 4 ++-- apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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 77a8a94c8..363963f96 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -188,8 +188,8 @@ check_url(URL) -> end. check_body(Body) -> - lists:any(fun({_, V}) -> - not is_binary(V) + lists:all(fun({_, V}) -> + is_binary(V) end, maps:to_list(Body)). default_headers() -> 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 78632610e..d626e2dab 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -150,7 +150,7 @@ check_password(Password, parse_query(Query) -> case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of {match, Captured} -> - PlaceHolders = [PlaceHolder || PlaceHolder <- Captured], + PlaceHolders = [PlaceHolder || [PlaceHolder] <- Captured], Replacements = ["$" ++ integer_to_list(I) || I <- lists:seq(1, length(Captured))], NQuery = lists:foldl(fun({PlaceHolder, Replacement}, Query0) -> re:replace(Query0, <<"'\\", PlaceHolder/binary, "'">>, Replacement, [{return, binary}]) From 6051ea92eb683610e3045dbb0eb569a5102d22c0 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Fri, 24 Sep 2021 17:24:04 +0800 Subject: [PATCH 3/6] chore(authn): improve log --- apps/emqx/src/emqx_authentication.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 40af0e7b7..3e98fb548 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -303,7 +303,7 @@ authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthRe do_authenticate([], _) -> {stop, {error, not_authorized}}; -do_authenticate([#authenticator{provider = Provider, state = #{'_unique' := Unique} = State} | More], Credential) -> +do_authenticate([#authenticator{id = ID, provider = Provider, state = State} | More], Credential) -> try Provider:authenticate(Credential, State) of ignore -> do_authenticate(More, Credential); @@ -316,7 +316,7 @@ do_authenticate([#authenticator{provider = Provider, state = #{'_unique' := Uniq {stop, Result} catch error:Reason:Stacktrace -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [Unique, {Reason, Stacktrace}]), + ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [ID, {Reason, Stacktrace}]), do_authenticate(More, Credential) end. From 096e85dc14fc1d041ae611cc87d39a959cbafd90 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Sun, 26 Sep 2021 11:31:19 +0800 Subject: [PATCH 4/6] chore(authn): improve log and fix disabled authentication still working --- apps/emqx/src/emqx_authentication.erl | 53 ++++++++++++------- .../src/simple_authn/emqx_authn_http.erl | 6 +-- .../emqx_authn_jwks_connector.erl | 12 +++-- .../src/simple_authn/emqx_authn_mongodb.erl | 8 ++- .../src/simple_authn/emqx_authn_redis.erl | 4 +- 5 files changed, 56 insertions(+), 27 deletions(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 3e98fb548..341103781 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -289,16 +289,10 @@ check_config(Config) -> %%------------------------------------------------------------------------------ authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthResult) -> - case ets:lookup(?CHAINS_TAB, Listener) of - [#chain{authenticators = Authenticators}] when Authenticators =/= [] -> - do_authenticate(Authenticators, Credential); - _ -> - case ets:lookup(?CHAINS_TAB, global_chain(Protocol)) of - [#chain{authenticators = Authenticators}] when Authenticators =/= [] -> - do_authenticate(Authenticators, Credential); - _ -> - ignore - end + Authenticators = get_authenticators(Listener, global_chain(Protocol)), + case get_enabled(Authenticators) of + [] -> ignore; + NAuthenticators -> do_authenticate(NAuthenticators, Credential) end. do_authenticate([], _) -> @@ -315,11 +309,31 @@ do_authenticate([#authenticator{id = ID, provider = Provider, state = State} | M %% {error, Reason} {stop, Result} catch - error:Reason:Stacktrace -> - ?LOG(warning, "The following error occurred in '~s' during authentication: ~p", [ID, {Reason, Stacktrace}]), + Class:Reason:Stacktrace -> + ?SLOG(warning, #{msg => "an unexpected error in authentication", + class => Class, + reason => Reason, + stacktrace => Stacktrace, + authenticator => ID}), do_authenticate(More, Credential) end. +get_authenticators(Listener, Global) -> + case ets:lookup(?CHAINS_TAB, Listener) of + [#chain{authenticators = Authenticators}] -> + Authenticators; + _ -> + case ets:lookup(?CHAINS_TAB, Global) of + [#chain{authenticators = Authenticators}] -> + Authenticators; + _ -> + [] + end + end. + +get_enabled(Authenticators) -> + [Authenticator || Authenticator <- Authenticators, Authenticator#authenticator.enable =:= true]. + %%------------------------------------------------------------------------------ %% APIs %%------------------------------------------------------------------------------ @@ -335,7 +349,9 @@ initialize_authentication(ChainName, AuthenticatorsConfig) -> {ok, _} -> ok; {error, Reason} -> - ?LOG(error, "Failed to create authenticator '~s': ~p", [generate_id(AuthenticatorConfig), Reason]) + ?SLOG(error, #{msg => "failed to create authenticator", + reason => Reason, + authenticator => generate_id(AuthenticatorConfig)}) end end, CheckedConfig). @@ -540,7 +556,7 @@ handle_call({create_authenticator, ChainName, Config}, _From, #{providers := Pro false -> case do_create_authenticator(ChainName, AuthenticatorID, Config, Providers) of {ok, Authenticator} -> - NAuthenticators = Authenticators ++ [Authenticator], + NAuthenticators = Authenticators ++ [Authenticator#authenticator{enable = maps:get(enable, Config)}], true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NAuthenticators}), {ok, serialize_authenticator(Authenticator)}; {error, Reason} -> @@ -579,7 +595,8 @@ handle_call({update_authenticator, ChainName, AuthenticatorID, Config}, _From, S Unique = unique(ChainName, AuthenticatorID, Version), case Provider:update(Config#{'_unique' => Unique}, ST) of {ok, NewST} -> - NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST)}, + NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST), + enable = maps:get(enable, Config)}, NewAuthenticators = replace_authenticator(AuthenticatorID, NewAuthenticator, Authenticators), true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NewAuthenticators}), {ok, serialize_authenticator(NewAuthenticator)}; @@ -633,15 +650,15 @@ handle_call({list_users, ChainName, AuthenticatorID}, _From, State) -> reply(Reply, State); handle_call(Req, _From, State) -> - ?LOG(error, "Unexpected call: ~p", [Req]), + ?SLOG(error, #{msg => "unexpected call", req => Req}), {reply, ignored, State}. handle_cast(Req, State) -> - ?LOG(error, "Unexpected case: ~p", [Req]), + ?SLOG(error, #{msg => "unexpected cast", req => Req}), {noreply, State}. handle_info(Info, State) -> - ?LOG(error, "Unexpected info: ~p", [Info]), + ?SLOG(error, #{msg => "unexpected info", info => Info}), {noreply, State}. terminate(_Reason, _State) -> 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 363963f96..8659605c4 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_http.erl @@ -188,9 +188,9 @@ check_url(URL) -> end. check_body(Body) -> - lists:all(fun({_, V}) -> - is_binary(V) - end, maps:to_list(Body)). + maps:fold(fun(_K, _V, false) -> false; + (_K, V, true) -> is_binary(V) + end, true, Body). default_headers() -> maps:put(<<"content-type">>, 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 d6e977be6..6ec57a4de 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,9 @@ handle_info({http, {RequestID, Result}}, State1 = State0#{request_id := undefined}, case Result of {error, Reason} -> - ?LOG(error, "Failed to request jwks endpoint(~s): ~p", [Endpoint, Reason]), + ?SLOG(warning, #{msg => "failed to request jwks endpoint", + endpoint => Endpoint, + reason => Reason}), State1; {_StatusLine, _Headers, Body} -> try @@ -102,7 +104,9 @@ handle_info({http, {RequestID, Result}}, {_, JWKs} = JWKS#jose_jwk.keys, State1#{jwks := JWKs} catch _:_ -> - ?LOG(error, "Invalid jwks returned from jwks endpoint(~s): ~p~n", [Endpoint, Body]), + ?SLOG(warning, #{msg => "invalid jwks returned from jwks endpoint", + endpoint => Endpoint, + body => Body}), State1 end end; @@ -140,7 +144,9 @@ refresh_jwks(#{endpoint := Endpoint, NState = case httpc:request(get, {Endpoint, [{"Accept", "application/json"}]}, HTTPOpts, [{body_format, binary}, {sync, false}, {receiver, self()}]) of {error, Reason} -> - ?LOG(error, "Failed to request jwks endpoint(~s): ~p", [Endpoint, Reason]), + ?SLOG(warning, #{msg => "failed to request jwks endpoint", + endpoint => Endpoint, + reason => Reason}), State; {ok, RequestID} -> State#{request_id := RequestID} 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 d348a439c..4ae98d07d 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -146,14 +146,18 @@ authenticate(#{password := Password} = Credential, case emqx_resource:query(Unique, {find_one, Collection, Selector2, #{}}) of undefined -> ignore; {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), + ?SLOG(error, #{msg => "query failed", + unique => Unique, + reason => Reason}), ignore; Doc -> case check_password(Password, Doc, State) of ok -> {ok, #{is_superuser => is_superuser(Doc, State)}}; {error, {cannot_find_password_hash_field, PasswordHashField}} -> - ?LOG(error, "['~s'] Can't find password hash field: ~s", [Unique, PasswordHashField]), + ?SLOG(error, #{msg => "can't find password hash field", + unique => Unique, + password_hash_field => PasswordHashField}), {error, bad_username_or_password}; {error, Reason} -> {error, Reason} 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 5d73f9410..572ae9a2c 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl @@ -138,7 +138,9 @@ authenticate(#{password := Password} = Credential, {error, Reason} end; {error, Reason} -> - ?LOG(error, "['~s'] Query failed: ~p", [Unique, Reason]), + ?SLOG(error, #{msg => "query failed", + unique => Unique, + reason => Reason}), ignore end. From 2262bf508edbc5b7d87930d561162e4cd6090f23 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Mon, 27 Sep 2021 14:50:22 +0800 Subject: [PATCH 5/6] 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]). From 352c87a5869422a5cb83de3aa4b2af68ee6fe06e Mon Sep 17 00:00:00 2001 From: zhouzb Date: Tue, 28 Sep 2021 09:22:59 +0800 Subject: [PATCH 6/6] chore(authn): delete useless func --- .../src/simple_authn/emqx_authn_mysql.erl | 19 ------------------- 1 file changed, 19 deletions(-) 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 a838d216d..d15dff6e1 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -140,25 +140,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