fix(authn): removed explicit chain creation for security reasons

.github/workflows/run_fvt_tests.yaml

@@ -141,7 +141,9 @@ jobs:
         path: .
     - name: unzip source code
       run: unzip -q source.zip
-
+    - uses: erlef/setup-beam@v1
+        with:
+          otp-version: "24.2"
     - name: Get deps git refs for cache
       id: deps-refs
       run: |

apps/emqx/src/emqx_authentication.erl

@@ -47,7 +47,6 @@
     register_providers/1,
     deregister_provider/1,
     deregister_providers/1,
-    create_chain/1,
     delete_chain/1,
     lookup_chain/1,
     list_chains/0,
@@ -271,7 +270,6 @@ authenticator_id(Config) ->
 initialize_authentication(_, []) ->
     ok;
 initialize_authentication(ChainName, AuthenticatorsConfig) ->
-    _ = create_chain(ChainName),
     CheckedConfig = to_list(AuthenticatorsConfig),
     lists:foreach(
         fun(AuthenticatorConfig) ->
@@ -322,10 +320,6 @@ deregister_providers(AuthNTypes) when is_list(AuthNTypes) ->
 deregister_provider(AuthNType) ->
     deregister_providers([AuthNType]).
 
--spec create_chain(chain_name()) -> {ok, chain()} | {error, term()}.
-create_chain(Name) ->
-    call({create_chain, Name}).
-
 -spec delete_chain(chain_name()) -> ok | {error, term()}.
 delete_chain(Name) ->
     call({delete_chain, Name}).
@@ -451,50 +445,36 @@ handle_call(
     end;
 handle_call({deregister_providers, AuthNTypes}, _From, #{providers := Providers} = State) ->
     reply(ok, State#{providers := maps:without(AuthNTypes, Providers)});
-handle_call({create_chain, Name}, _From, State) ->
+handle_call({delete_chain, ChainName}, _From, State) ->
-    case ets:member(?CHAINS_TAB, Name) of
+    UpdateFun = fun(Chain) ->
-        true ->
+        {_MatchedIDs, NewChain} = do_delete_authenticators(fun(_) -> true end, Chain),
-            reply({error, {already_exists, {chain, Name}}}, State);
+        {ok, ok, NewChain}
-        false ->
+    end,
-            Chain = #chain{
+    Reply = with_chain(ChainName, UpdateFun),
-                name = Name,
+    reply(Reply, maybe_unhook(State));
-                authenticators = []
-            },
-            true = ets:insert(?CHAINS_TAB, Chain),
-            reply({ok, serialize_chain(Chain)}, State)
-    end;
-handle_call({delete_chain, Name}, _From, State) ->
-    case ets:lookup(?CHAINS_TAB, Name) of
-        [] ->
-            reply({error, {not_found, {chain, Name}}}, State);
-        [#chain{} = Chain] ->
-            _MatchedIDs = do_delete_authenticators(fun(_) -> true end, Chain),
-            true = ets:delete(?CHAINS_TAB, Name),
-            reply(ok, maybe_unhook(State))
-    end;
 handle_call({create_authenticator, ChainName, Config}, _From, #{providers := Providers} = State) ->
     UpdateFun = fun(Chain) ->
         handle_create_authenticator(Chain, Config, Providers)
     end,
-    Reply = update_chain(ChainName, UpdateFun),
+    Reply = with_new_chain(ChainName, UpdateFun),
     reply(Reply, maybe_hook(State));
 handle_call({delete_authenticator, ChainName, AuthenticatorID}, _From, State) ->
     UpdateFun = fun(Chain) ->
         handle_delete_authenticator(Chain, AuthenticatorID)
     end,
-    Reply = update_chain(ChainName, UpdateFun),
+    Reply = with_chain(ChainName, UpdateFun),
     reply(Reply, maybe_unhook(State));
 handle_call({update_authenticator, ChainName, AuthenticatorID, Config}, _From, State) ->
     UpdateFun = fun(Chain) ->
         handle_update_authenticator(Chain, AuthenticatorID, Config)
     end,
-    Reply = update_chain(ChainName, UpdateFun),
+    Reply = with_chain(ChainName, UpdateFun),
     reply(Reply, State);
 handle_call({move_authenticator, ChainName, AuthenticatorID, Position}, _From, State) ->
     UpdateFun = fun(Chain) ->
         handle_move_authenticator(Chain, AuthenticatorID, Position)
     end,
-    Reply = update_chain(ChainName, UpdateFun),
+    Reply = with_chain(ChainName, UpdateFun),
     reply(Reply, State);
 handle_call({import_users, ChainName, AuthenticatorID, Filename}, _From, State) ->
     Reply = call_authenticator(ChainName, AuthenticatorID, import_users, [Filename]),
@@ -569,11 +549,9 @@ handle_update_authenticator(Chain, AuthenticatorID, Config) ->
                                 NewAuthenticator,
                                 Authenticators
                             ),
-                            true = ets:insert(
+                            NewChain = Chain#chain{authenticators = NewAuthenticators},
-                                ?CHAINS_TAB,
+                            Result = {ok, serialize_authenticator(NewAuthenticator)},
-                                Chain#chain{authenticators = NewAuthenticators}
+                            {ok, Result, NewChain};
-                            ),
-                            {ok, serialize_authenticator(NewAuthenticator)};
                         {error, Reason} ->
                             {error, Reason}
                     end;
@@ -587,18 +565,18 @@ handle_delete_authenticator(Chain, AuthenticatorID) ->
         ID =:= AuthenticatorID
     end,
     case do_delete_authenticators(MatchFun, Chain) of
-        [] ->
+        {[], _NewChain} ->
             {error, {not_found, {authenticator, AuthenticatorID}}};
-        [AuthenticatorID] ->
+        {[AuthenticatorID], NewChain} ->
-            ok
+            {ok, ok, NewChain}
     end.
 
 handle_move_authenticator(Chain, AuthenticatorID, Position) ->
     #chain{authenticators = Authenticators} = Chain,
     case do_move_authenticator(AuthenticatorID, Authenticators, Position) of
         {ok, NAuthenticators} ->
-            true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NAuthenticators}),
+            NewChain = Chain#chain{authenticators = NAuthenticators},
-            ok;
+            {ok, ok, NewChain};
         {error, Reason} ->
             {error, Reason}
     end.
@@ -616,18 +594,15 @@ handle_create_authenticator(Chain, Config, Providers) ->
                     NAuthenticators =
                         Authenticators ++
                             [Authenticator#authenticator{enable = maps:get(enable, Config)}],
-                    true = ets:insert(
-                        ?CHAINS_TAB,
-                        Chain#chain{authenticators = NAuthenticators}
-                    ),
-
                     ok = emqx_metrics_worker:create_metrics(
                         authn_metrics,
                         metrics_id(Name, AuthenticatorID),
                         [total, success, failed, nomatch],
                         [total]
                     ),
-                    {ok, serialize_authenticator(Authenticator)};
+                    NewChain = Chain#chain{authenticators = NAuthenticators},
+                    Result = {ok, serialize_authenticator(Authenticator)},
+                    {ok, Result, NewChain};
                 {error, Reason} ->
                     {error, Reason}
             end
@@ -675,6 +650,14 @@ do_authenticate(
 reply(Reply, State) ->
     {reply, Reply, State}.
 
+save_chain(#chain{
+    name = Name,
+    authenticators = []
+}) ->
+    ets:delete(?CHAINS_TAB, Name);
+save_chain(#chain{} = Chain) ->
+    ets:insert(?CHAINS_TAB, Chain).
+
 create_chain_table() ->
     try
         _ = ets:new(?CHAINS_TAB, [
@@ -774,14 +757,7 @@ do_delete_authenticators(MatchFun, #chain{name = Name, authenticators = Authenti
         end,
         Matching
     ),
-    true =
+    {MatchingIDs, Chain#chain{authenticators = Others}}.
-        case Others of
-            [] ->
-                ets:delete(?CHAINS_TAB, Name);
-            _ ->
-                ets:insert(?CHAINS_TAB, Chain#chain{authenticators = Others})
-        end,
-    MatchingIDs.
 
 do_destroy_authenticator(#authenticator{provider = Provider, state = State}) ->
     _ = Provider:destroy(State),
@@ -824,21 +800,41 @@ insert(
 insert(Authenticator, [Authenticator0 | More], {Relative, RelatedID}, Acc) ->
     insert(Authenticator, More, {Relative, RelatedID}, [Authenticator0 | Acc]).
 
-update_chain(ChainName, UpdateFun) ->
+with_new_chain(ChainName, Fun) ->
+    case ets:lookup(?CHAINS_TAB, ChainName) of
+        [] ->
+            Chain = #chain{name = ChainName, authenticators = []},
+            do_with_chain(Fun, Chain);
+        [Chain] ->
+            do_with_chain(Fun, Chain)
+    end.
+
+with_chain(ChainName, Fun) ->
     case ets:lookup(?CHAINS_TAB, ChainName) of
         [] ->
             {error, {not_found, {chain, ChainName}}};
         [Chain] ->
+            do_with_chain(Fun, Chain)
+    end.
+
+do_with_chain(Fun, Chain) ->
     try
-                UpdateFun(Chain)
+        case Fun(Chain) of
+            {ok, Result} ->
+                Result;
+            {ok, Result, NewChain} ->
+                save_chain(NewChain),
+                Result;
+            {error, _} = Error ->
+                Error
+        end
     catch
         Class:Reason:Stk ->
             {error, {exception, {Class, Reason, Stk}}}
-            end
     end.
 
 call_authenticator(ChainName, AuthenticatorID, Func, Args) ->
-    UpdateFun =
+    Fun =
         fun(#chain{authenticators = Authenticators}) ->
             case lists:keyfind(AuthenticatorID, #authenticator.id, Authenticators) of
                 false ->
@@ -846,13 +842,13 @@ call_authenticator(ChainName, AuthenticatorID, Func, Args) ->
                 #authenticator{provider = Provider, state = State} ->
                     case erlang:function_exported(Provider, Func, length(Args) + 1) of
                         true ->
-                            erlang:apply(Provider, Func, Args ++ [State]);
+                            {ok, erlang:apply(Provider, Func, Args ++ [State])};
                         false ->
                             {error, unsupported_operation}
                     end
             end
         end,
-    update_chain(ChainName, UpdateFun).
+    with_chain(ChainName, Fun).
 
 serialize_chain(#chain{
     name = Name,

apps/emqx/src/emqx_authentication_config.erl

@@ -140,7 +140,6 @@ post_config_update(_, UpdateReq, NewConfig, OldConfig, AppEnvs) ->
 
 do_post_config_update({create_authenticator, ChainName, Config}, NewConfig, _OldConfig, _AppEnvs) ->
     NConfig = get_authenticator_config(authenticator_id(Config), NewConfig),
-    _ = emqx_authentication:create_chain(ChainName),
     emqx_authentication:create_authenticator(ChainName, NConfig);
 do_post_config_update(
     {delete_authenticator, ChainName, AuthenticatorID},

apps/emqx/test/emqx_authentication_SUITE.erl

@@ -113,20 +113,32 @@ end_per_testcase(Case, Config) ->
     _ = ?MODULE:Case({'end', Config}),
     ok.
 
-t_chain({_, Config}) ->
+t_chain({'init', Config}) ->
     Config;
 t_chain(Config) when is_list(Config) ->
     % CRUD of authentication chain
     ChainName = 'test',
     ?assertMatch({ok, []}, ?AUTHN:list_chains()),
     ?assertMatch({ok, []}, ?AUTHN:list_chain_names()),
-    ?assertMatch({ok, #{name := ChainName, authenticators := []}}, ?AUTHN:create_chain(ChainName)),
+
-    ?assertEqual({error, {already_exists, {chain, ChainName}}}, ?AUTHN:create_chain(ChainName)),
+    %% to create a chain we need create an authenticator
-    ?assertMatch({ok, #{name := ChainName, authenticators := []}}, ?AUTHN:lookup_chain(ChainName)),
+    AuthenticatorConfig = #{
+        mechanism => password_based,
+        backend => built_in_database,
+        enable => true
+    },
+    register_provider({password_based, built_in_database}, ?MODULE),
+    ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig),
+
+    ?assertMatch({ok, #{name := ChainName, authenticators := [_]}}, ?AUTHN:lookup_chain(ChainName)),
     ?assertMatch({ok, [#{name := ChainName}]}, ?AUTHN:list_chains()),
     ?assertEqual({ok, [ChainName]}, ?AUTHN:list_chain_names()),
     ?assertEqual(ok, ?AUTHN:delete_chain(ChainName)),
     ?assertMatch({error, {not_found, {chain, ChainName}}}, ?AUTHN:lookup_chain(ChainName)),
+    ok;
+t_chain({'end', _Config}) ->
+    ?AUTHN:delete_chain(test),
+    ?AUTHN:deregister_providers([{password_based, built_in_database}]),
     ok.
 
 t_authenticator({'init', Config}) ->
@@ -143,13 +155,6 @@ t_authenticator(Config) when is_list(Config) ->
         enable => true
     },
 
-    % Create an authenticator when the authentication chain does not exist
-    ?assertEqual(
-        {error, {not_found, {chain, ChainName}}},
-        ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1)
-    ),
-
-    ?AUTHN:create_chain(ChainName),
     % Create an authenticator when the provider does not exist
 
     ?assertEqual(
@@ -183,11 +188,14 @@ t_authenticator(Config) when is_list(Config) ->
     ?assertEqual(ok, ?AUTHN:delete_authenticator(ChainName, ID1)),
 
     ?assertEqual(
-        {error, {not_found, {authenticator, ID1}}},
+        {error, {not_found, {chain, test}}},
         ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)
     ),
 
-    ?assertMatch({ok, []}, ?AUTHN:list_authenticators(ChainName)),
+    ?assertMatch(
+        {error, {not_found, {chain, ChainName}}},
+        ?AUTHN:list_authenticators(ChainName)
+    ),
 
     % Multiple authenticators exist at the same time
     AuthNType2 = ?config("auth2"),
@@ -253,7 +261,6 @@ t_authenticate(Config) when is_list(Config) ->
         backend => built_in_database,
         enable => true
     },
-    ?AUTHN:create_chain(ListenerID),
     ?assertMatch({ok, _}, ?AUTHN:create_authenticator(ListenerID, AuthenticatorConfig)),
 
     ?assertEqual(
@@ -352,7 +359,7 @@ t_update_config(Config) when is_list(Config) ->
     ),
 
     ?assertEqual(
-        {error, {not_found, {authenticator, ID2}}},
+        {error, {not_found, {chain, Global}}},
         ?AUTHN:lookup_authenticator(Global, ID2)
     ),
 
@@ -427,7 +434,15 @@ t_restart({'init', Config}) ->
 t_restart(Config) when is_list(Config) ->
     ?assertEqual({ok, []}, ?AUTHN:list_chain_names()),
 
-    ?AUTHN:create_chain(test_chain),
+    %% to create a chain we need create an authenticator
+    AuthenticatorConfig = #{
+        mechanism => password_based,
+        backend => built_in_database,
+        enable => true
+    },
+    register_provider({password_based, built_in_database}, ?MODULE),
+    ?AUTHN:create_authenticator(test_chain, AuthenticatorConfig),
+
     ?assertEqual({ok, [test_chain]}, ?AUTHN:list_chain_names()),
 
     ok = supervisor:terminate_child(emqx_authentication_sup, ?AUTHN),
@@ -436,6 +451,7 @@ t_restart(Config) when is_list(Config) ->
     ?assertEqual({ok, [test_chain]}, ?AUTHN:list_chain_names());
 t_restart({'end', _Config}) ->
     ?AUTHN:delete_chain(test_chain),
+    ?AUTHN:deregister_providers([{password_based, built_in_database}]),
     ok.
 
 t_convert_certs({_, Config}) ->

apps/emqx_authn/test/emqx_authn_http_SUITE.erl

@@ -101,7 +101,10 @@ t_create_invalid(_Config) ->
                     throw:Error ->
                         {error, Error}
                 end,
-            {ok, []} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs
     ).

apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl

@@ -95,7 +95,10 @@ t_create_invalid(_Config) ->
                 {create_authenticator, ?GLOBAL, Config}
             ),
 
-            {ok, []} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs
     ).

apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl

@@ -113,7 +113,10 @@ t_create_invalid(_Config) ->
                 {create_authenticator, ?GLOBAL, Config}
             ),
             emqx_authn_test_lib:delete_config(?ResourceID),
-            {ok, _} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs
     ).

apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl

@@ -114,7 +114,10 @@ t_create_invalid(_Config) ->
                 {create_authenticator, ?GLOBAL, Config}
             ),
             emqx_authn_test_lib:delete_config(?ResourceID),
-            {ok, []} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs
     ).

apps/emqx_authn/test/emqx_authn_redis_SUITE.erl

@@ -85,8 +85,10 @@ end_per_suite(_Config) ->
 %%------------------------------------------------------------------------------
 
 t_create(_Config) ->
-    {ok, []} = emqx_authentication:list_authenticators(?GLOBAL),
+    ?assertEqual(
-
+        {error, {not_found, {chain, ?GLOBAL}}},
+        emqx_authentication:list_authenticators(?GLOBAL)
+    ),
     AuthConfig = raw_redis_auth_config(),
     {ok, _} = emqx:update_config(
         ?PATH,
@@ -119,7 +121,10 @@ t_create_invalid(_Config) ->
                 {create_authenticator, ?GLOBAL, Config}
             ),
 
-            {ok, []} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs
     ),
@@ -139,7 +144,10 @@ t_create_invalid(_Config) ->
                 {create_authenticator, ?GLOBAL, Config}
             ),
             emqx_authn_test_lib:delete_config(?ResourceID),
-            {ok, []} = emqx_authentication:list_authenticators(?GLOBAL)
+            ?assertEqual(
+                {error, {not_found, {chain, ?GLOBAL}}},
+                emqx_authentication:list_authenticators(?GLOBAL)
+            )
         end,
         InvalidConfigs1
     ).

apps/emqx_authn/test/emqx_enhanced_authn_scram_mnesia_SUITE.erl

@@ -87,7 +87,10 @@ t_create_invalid(_Config) ->
         {create_authenticator, ?GLOBAL, InvalidConfig}
     ),
 
-    {ok, []} = emqx_authentication:list_authenticators(?GLOBAL).
+    ?assertEqual(
+        {error, {not_found, {chain, ?GLOBAL}}},
+        emqx_authentication:list_authenticators(?GLOBAL)
+    ).
 
 t_authenticate(_Config) ->
     Algorithm = sha512,

apps/emqx_gateway/src/emqx_gateway_insta_sup.erl

@@ -277,8 +277,6 @@ authn_conf(Conf) ->
     maps:get(authentication, Conf, #{enable => false}).
 
 do_create_authn_chain(ChainName, AuthConf) ->
-    case ensure_chain(ChainName) of
-        ok ->
     case emqx_authentication:create_authenticator(ChainName, AuthConf) of
         {ok, _} ->
             ok;
@@ -290,24 +288,6 @@ do_create_authn_chain(ChainName, AuthConf) ->
                 config => AuthConf
             }),
             throw({badauth, Reason})
-            end;
-        {error, Reason} ->
-            ?SLOG(error, #{
-                msg => "failed_to_create_authn_chanin",
-                chain_name => ChainName,
-                reason => Reason
-            }),
-            throw({badauth, Reason})
-    end.
-
-ensure_chain(ChainName) ->
-    case emqx_authentication:create_chain(ChainName) of
-        {ok, _ChainInfo} ->
-            ok;
-        {error, {already_exists, _}} ->
-            ok;
-        {error, Reason} ->
-            {error, Reason}
     end.
 
 do_deinit_authn(Names) ->