Merge pull request #9496 from savonarola/fix-auth-chain

Fix `emqx_authentication` hook cooperation with other hooks
2022-12-09 14:48:18 +03:00 · 2022-12-09 14:48:18 +03:00 · a26964291d
parent ecb2f85746 cd0ae62995
commit a26964291d
5 changed files with 157 additions and 27 deletions
--- 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').
--- 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).
--- 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}).

--- 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).
--- 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)。