diff --git a/apps/emqx/src/emqx_access_control.erl b/apps/emqx/src/emqx_access_control.erl index 30d56f257..3fa781e6d 100644 --- a/apps/emqx/src/emqx_access_control.erl +++ b/apps/emqx/src/emqx_access_control.erl @@ -46,16 +46,32 @@ authenticate(Credential) -> NotSuperUser = #{is_superuser => false}, case emqx_authentication:pre_hook_authenticate(Credential) of ok -> + inc_authn_metrics(anonymous), {ok, NotSuperUser}; continue -> - case run_hooks('client.authenticate', [Credential], {ok, #{is_superuser => false}}) of - ok -> + case run_hooks('client.authenticate', [Credential], ignore) of + ignore -> + inc_authn_metrics(anonymous), {ok, NotSuperUser}; + ok -> + inc_authn_metrics(ok), + {ok, NotSuperUser}; + {ok, _AuthResult} = OkResult -> + inc_authn_metrics(ok), + OkResult; + {ok, _AuthResult, _AuthData} = OkResult -> + inc_authn_metrics(ok), + OkResult; + {error, _Reason} = Error -> + inc_authn_metrics(error), + Error; + %% {continue, AuthCache} | {continue, AuthData, AuthCache} Other -> Other end; - Other -> - Other + {error, _Reason} = Error -> + inc_authn_metrics(error), + Error end. %% @doc Check Authorization @@ -134,3 +150,11 @@ inc_authz_metrics(deny) -> emqx_metrics:inc('authorization.deny'); inc_authz_metrics(cache_hit) -> emqx_metrics:inc('authorization.cache_hit'). + +inc_authn_metrics(error) -> + emqx_metrics:inc('authentication.failure'); +inc_authn_metrics(ok) -> + emqx_metrics:inc('authentication.success'); +inc_authn_metrics(anonymous) -> + emqx_metrics:inc('authentication.success.anonymous'), + emqx_metrics:inc('authentication.success'). diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 749f5bfd7..ffce81787 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -228,7 +228,6 @@ when -spec pre_hook_authenticate(emqx_types:clientinfo()) -> ok | continue | {error, not_authorized}. pre_hook_authenticate(#{enable_authn := false}) -> - inc_authenticate_metric('authentication.success.anonymous'), ?TRACE_RESULT("authentication_result", ok, enable_authn_false); pre_hook_authenticate(#{enable_authn := quick_deny_anonymous} = Credential) -> case maps:get(username, Credential, undefined) of @@ -242,29 +241,18 @@ pre_hook_authenticate(#{enable_authn := quick_deny_anonymous} = Credential) -> pre_hook_authenticate(_) -> continue. -authenticate(#{listener := Listener, protocol := Protocol} = Credential, _AuthResult) -> +authenticate(#{listener := Listener, protocol := Protocol} = Credential, AuthResult) -> case get_authenticators(Listener, global_chain(Protocol)) of {ok, ChainName, Authenticators} -> case get_enabled(Authenticators) of [] -> - inc_authenticate_metric('authentication.success.anonymous'), - ?TRACE_RESULT("authentication_result", ignore, empty_chain); + ?TRACE_RESULT("authentication_result", AuthResult, empty_chain); NAuthenticators -> Result = do_authenticate(ChainName, NAuthenticators, Credential), - - case Result of - {stop, {ok, _}} -> - inc_authenticate_metric('authentication.success'); - {stop, {error, _}} -> - inc_authenticate_metric('authentication.failure'); - _ -> - ok - end, ?TRACE_RESULT("authentication_result", Result, chain_result) end; none -> - inc_authenticate_metric('authentication.success.anonymous'), - ?TRACE_RESULT("authentication_result", ignore, no_chain) + ?TRACE_RESULT("authentication_result", AuthResult, no_chain) end. get_authenticators(Listener, Global) -> @@ -649,7 +637,7 @@ handle_create_authenticator(Chain, Config, Providers) -> end. do_authenticate(_ChainName, [], _) -> - {stop, {error, not_authorized}}; + {ok, {error, not_authorized}}; do_authenticate( ChainName, [#authenticator{id = ID} = Authenticator | More], Credential ) -> @@ -673,7 +661,7 @@ do_authenticate( _ -> ok end, - {stop, Result} + {ok, Result} catch Class:Reason:Stacktrace -> ?TRACE_AUTHN(warning, "authenticator_error", #{ @@ -947,9 +935,3 @@ to_list(M) when is_map(M) -> [M]; to_list(L) when is_list(L) -> L. call(Call) -> gen_server:call(?MODULE, Call, infinity). - -inc_authenticate_metric('authentication.success.anonymous' = Metric) -> - emqx_metrics:inc(Metric), - emqx_metrics:inc('authentication.success'); -inc_authenticate_metric(Metric) -> - emqx_metrics:inc(Metric). diff --git a/apps/emqx/test/emqx_authentication_SUITE.erl b/apps/emqx/test/emqx_authentication_SUITE.erl index 61b4b2775..7016a8a00 100644 --- a/apps/emqx/test/emqx_authentication_SUITE.erl +++ b/apps/emqx/test/emqx_authentication_SUITE.erl @@ -22,6 +22,8 @@ -compile(export_all). -compile(nowarn_export_all). +-include_lib("emqx/include/emqx_hooks.hrl"). + -include_lib("common_test/include/ct.hrl"). -include_lib("eunit/include/eunit.hrl"). -include_lib("typerefl/include/types.hrl"). @@ -35,6 +37,20 @@ end)() ). -define(CONF_ROOT, ?EMQX_AUTHENTICATION_CONFIG_ROOT_NAME_ATOM). +-define(NOT_SUPERUSER, #{is_superuser => false}). + +-define(assertAuthSuccessForUser(User), + ?assertMatch( + {ok, _}, + emqx_access_control:authenticate(ClientInfo#{username => atom_to_binary(User)}) + ) +). +-define(assertAuthFailureForUser(User), + ?assertMatch( + {error, _}, + emqx_access_control:authenticate(ClientInfo#{username => atom_to_binary(User)}) + ) +). %%------------------------------------------------------------------------------ %% Hocon Schema @@ -88,9 +104,22 @@ update(_Config, _State) -> authenticate(#{username := <<"good">>}, _State) -> {ok, #{is_superuser => true}}; +authenticate(#{username := <<"ignore">>}, _State) -> + ignore; authenticate(#{username := _}, _State) -> {error, bad_username_or_password}. +hook_authenticate(#{username := <<"hook_user_good">>}, _AuthResult) -> + {ok, {ok, ?NOT_SUPERUSER}}; +hook_authenticate(#{username := <<"hook_user_bad">>}, _AuthResult) -> + {ok, {error, invalid_username}}; +hook_authenticate(#{username := <<"hook_user_finally_good">>}, _AuthResult) -> + {stop, {ok, ?NOT_SUPERUSER}}; +hook_authenticate(#{username := <<"hook_user_finally_bad">>}, _AuthResult) -> + {stop, {error, invalid_username}}; +hook_authenticate(_ClientId, AuthResult) -> + {ok, AuthResult}. + destroy(_State) -> ok. @@ -113,6 +142,10 @@ end_per_testcase(Case, Config) -> _ = ?MODULE:Case({'end', Config}), ok. +%%================================================================================= +%% Testcases +%%================================================================================= + t_chain({'init', Config}) -> Config; t_chain(Config) when is_list(Config) -> @@ -500,6 +533,92 @@ t_convert_certs(Config) when is_list(Config) -> clear_certs(CertsDir, #{<<"ssl">> => NCerts3}), ?assertEqual(false, filelib:is_regular(maps:get(<<"keyfile">>, NCerts3))). +t_combine_authn_and_callback({init, Config}) -> + [ + {listener_id, 'tcp:default'}, + {authn_type, {password_based, built_in_database}} + | Config + ]; +t_combine_authn_and_callback(Config) when is_list(Config) -> + ListenerID = ?config(listener_id), + ClientInfo = #{ + zone => default, + listener => ListenerID, + protocol => mqtt, + password => <<"any">> + }, + + %% no emqx_authentication authenticators, anonymous is allowed + ?assertAuthSuccessForUser(bad), + + AuthNType = ?config(authn_type), + register_provider(AuthNType, ?MODULE), + + AuthenticatorConfig = #{ + mechanism => password_based, + backend => built_in_database, + enable => true + }, + {ok, _} = ?AUTHN:create_authenticator(ListenerID, AuthenticatorConfig), + + %% emqx_authentication alone + ?assertAuthSuccessForUser(good), + ?assertAuthFailureForUser(ignore), + ?assertAuthFailureForUser(bad), + + %% add hook with higher priority + ok = hook(?HP_AUTHN + 1), + + %% for hook unrelataed users everything is the same + ?assertAuthSuccessForUser(good), + ?assertAuthFailureForUser(ignore), + ?assertAuthFailureForUser(bad), + + %% higher-priority hook can permit access with {ok,...}, + %% then emqx_authentication overrides the result + ?assertAuthFailureForUser(hook_user_good), + ?assertAuthFailureForUser(hook_user_bad), + + %% higher-priority hook can permit and return {stop,...}, + %% then emqx_authentication cannot override the result + ?assertAuthSuccessForUser(hook_user_finally_good), + ?assertAuthFailureForUser(hook_user_finally_bad), + + ok = unhook(), + + %% add hook with lower priority + ok = hook(?HP_AUTHN - 1), + + %% for hook unrelataed users + ?assertAuthSuccessForUser(good), + ?assertAuthFailureForUser(bad), + ?assertAuthFailureForUser(ignore), + + %% lower-priority hook can overrride auth result, + %% because emqx_authentication permits/denies with {ok, ...} + ?assertAuthSuccessForUser(hook_user_good), + ?assertAuthFailureForUser(hook_user_bad), + ?assertAuthSuccessForUser(hook_user_finally_good), + ?assertAuthFailureForUser(hook_user_finally_bad), + + ok = unhook(); +t_combine_authn_and_callback({'end', Config}) -> + ?AUTHN:delete_chain(?config(listener_id)), + ?AUTHN:deregister_provider(?config(authn_type)), + ok. + +%%================================================================================= +%% Helpers fns +%%================================================================================= + +hook(Priority) -> + ok = emqx_hooks:put( + 'client.authenticate', {?MODULE, hook_authenticate, []}, Priority + ). + +unhook() -> + ok = emqx_hooks:del('client.authenticate', {?MODULE, hook_authenticate}). + update_config(Path, ConfigRequest) -> emqx:update_config(Path, ConfigRequest, #{rawconf_with_defaults => true}). diff --git a/changes/v5.0.12-en.md b/changes/v5.0.12-en.md index ba785f1f8..b1a643708 100644 --- a/changes/v5.0.12-en.md +++ b/changes/v5.0.12-en.md @@ -35,3 +35,6 @@ - Fixed the `Discovery error: no such service` error occurred during helm chart deployment, resulting in an abnormal discovery of cluster nodes. - Fixed that caused EMQX Helm Chart to fail when modifying some of EMQX's configuration items via environment variables + +- Fix shadowing `'client.authenticate'` callbacks by `emqx_authenticator`. Now `emqx_authenticator` + passes execution to the further callbacks if none of the authenticators matches [#9496](https://github.com/emqx/emqx/pull/9496). diff --git a/changes/v5.0.12-zh.md b/changes/v5.0.12-zh.md index 95d1aed42..4d945d81a 100644 --- a/changes/v5.0.12-zh.md +++ b/changes/v5.0.12-zh.md @@ -35,3 +35,5 @@ - 修复了 EMQX Helm Chart 部署时出现 `Discovery error: no such service` 错误，导致集群节点发现异常。 - 修复了 EMQX Helm Chart 通过环境变量修改部分 EMQX 的配置项时的错误 + +- 通过 `emqx_authenticator` 修复隐藏 `'client.authenticate'` 回调。 现在 `emqx_authenticator` 如果没有任何验证器匹配，则将执行传递给进一步的回调 [#9496](https://github.com/emqx/emqx/pull/9496)。