From b2699c687b44d92b93c08090868e2d4ae7ff3156 Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 27 Sep 2023 15:48:11 +0800 Subject: [PATCH 01/10] fix(sso): support for SSL update && ensure update is atomic 1. support update SSL key and cert files 2. increase connection timeout 3. ensure the update is atomicity, everything will be consistent --- .../src/emqx_dashboard_sso_manager.erl | 118 +++++++++++++----- .../test/emqx_dashboard_sso_ldap_SUITE.erl | 32 +++++ apps/emqx_ldap/src/emqx_ldap.erl | 2 +- 3 files changed, 123 insertions(+), 29 deletions(-) diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl index 9b37c37f4..1d080ac5c 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl @@ -43,7 +43,7 @@ -define(MOD_TAB, emqx_dashboard_sso). -define(MOD_KEY_PATH, [dashboard, sso]). --define(CALL_TIMEOUT, timer:seconds(10)). +-define(CALL_TIMEOUT, timer:seconds(15)). -define(MOD_KEY_PATH(Sub), [dashboard, sso, Sub]). -define(RESOURCE_GROUP, <<"emqx_dashboard_sso">>). -define(DEFAULT_RESOURCE_OPTS, #{ @@ -98,7 +98,7 @@ create_resource(ResourceId, Module, Config) -> Config, ?DEFAULT_RESOURCE_OPTS ), - start_resource_if_enabled(ResourceId, Result, Config, fun clean_when_start_failed/1). + start_resource_if_enabled(ResourceId, Result, Config). update_resource(ResourceId, Module, Config) -> Result = emqx_resource:recreate_local( @@ -133,8 +133,8 @@ init([]) -> start_backend_services(), {ok, #{}}. -handle_call({update_config, Req, NewConf}, _From, State) -> - Result = on_config_update(Req, NewConf), +handle_call({update_config, Req, NewConf, OldConf}, _From, State) -> + Result = on_config_update(Req, NewConf, OldConf), {reply, Result, State}; handle_call(_Request, _From, State) -> Reply = ok, @@ -175,7 +175,7 @@ start_backend_services() -> ?SLOG(error, #{ msg => "start_sso_backend_failed", backend => Backend, - reason => Reason + reason => emqx_utils:redact(Reason) }) end end, @@ -189,17 +189,18 @@ update_config(Backend, UpdateReq) -> ?SLOG(info, #{ msg => "update_sso_successfully", backend => Backend, - result => Result + result => emqx_utils:redact(Result) }), Result; {error, Reason} -> + SafeReason = emqx_utils:redact(Reason), ?SLOG(error, #{ msg => "update_sso_failed", backend => Backend, - reason => Reason + reason => SafeReason }), {error, - case Reason of + case SafeReason of {_Stage, _Mod, Reason2} -> Reason2; _ -> @@ -208,14 +209,14 @@ update_config(Backend, UpdateReq) -> end. pre_config_update(_, {update, _Backend, Config}, _OldConf) -> - {ok, Config}; + maybe_write_certs(Config); pre_config_update(_, {delete, _Backend}, undefined) -> throw(not_exists); pre_config_update(_, {delete, _Backend}, _OldConf) -> {ok, null}. -post_config_update(_, UpdateReq, NewConf, _OldConf, _AppEnvs) -> - call({update_config, UpdateReq, NewConf}). +post_config_update(_, UpdateReq, NewConf, OldConf, _AppEnvs) -> + call({update_config, UpdateReq, NewConf, OldConf}). propagated_post_config_update( ?MOD_KEY_PATH(BackendBin) = Path, _UpdateReq, undefined, OldConf, AppEnvs @@ -236,7 +237,7 @@ propagated_post_config_update( Error end. -on_config_update({update, Backend, _RawConfig}, Config) -> +on_config_update({update, Backend, _RawConfig}, Config, OldConfig) -> Provider = provider(Backend), case lookup(Backend) of undefined -> @@ -247,14 +248,27 @@ on_config_update({update, Backend, _RawConfig}, Config) -> end ); Data -> + %% the steps for updating/recreating a resource are: + %% 1. destroy the old resource + %% 2. create a new resource + %% to keep consistency we need to follow those steps too, + %% however a failed update will not change the config, but will lose the resource + %% hence for consistency and atomicity, we should rollback when the update fails + ets:delete(?MOD_TAB, Backend), + UpdateState = fun(State) -> + ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State}) + end, on_backend_updated( emqx_dashboard_sso:update(Provider, Config, Data#?MOD_TAB.state), - fun(State) -> - ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State}) - end + UpdateState, + rollback( + Backend, + OldConfig, + UpdateState + ) ) end; -on_config_update({delete, Backend}, _NewConf) -> +on_config_update({delete, Backend}, _NewConf, _OldConf) -> case lookup(Backend) of undefined -> {error, not_exists}; @@ -276,12 +290,7 @@ lookup(Backend) -> undefined end. -start_resource_if_enabled(ResourceId, Result, Config) -> - start_resource_if_enabled(ResourceId, Result, Config, undefined). - -start_resource_if_enabled( - ResourceId, {ok, _} = Result, #{enable := true}, CleanWhenStartFailed -) -> +start_resource_if_enabled(ResourceId, {ok, _} = Result, #{enable := true}) -> case emqx_resource:start(ResourceId) of ok -> Result; @@ -292,11 +301,10 @@ start_resource_if_enabled( resource_id => ResourceId, reason => SafeReason }), - erlang:is_function(CleanWhenStartFailed) andalso - CleanWhenStartFailed(ResourceId), + clean_when_start_failed(ResourceId), {error, emqx_dashboard_sso:format(["Start backend failed, Reason: ", SafeReason])} end; -start_resource_if_enabled(_ResourceId, Result, _Config, _) -> +start_resource_if_enabled(_ResourceId, Result, _Config) -> Result. %% ensure the backend creation is atomic, clean the corresponding resource when necessary, @@ -309,14 +317,18 @@ clean_when_start_failed(ResourceId) -> _ = emqx_resource:remove_local(ResourceId), ok. +on_backend_updated(Result, OkFun) -> + on_backend_updated(Result, OkFun, undefined). + %% this first level `ok` is for emqx_config_handler, and the second level is for the caller -on_backend_updated({ok, State} = Ok, Fun) -> +on_backend_updated({ok, State} = Ok, Fun, _ErrFun) -> Fun(State), {ok, Ok}; -on_backend_updated(ok, Fun) -> +on_backend_updated(ok, Fun, _ErrFun) -> Fun(), {ok, ok}; -on_backend_updated(Error, _) -> +on_backend_updated(Error, _, ErrFun) -> + erlang:is_function(ErrFun) andalso ErrFun(Error), Error. bin(A) when is_atom(A) -> atom_to_binary(A, utf8); @@ -331,3 +343,53 @@ add_handler() -> remove_handler() -> ok = emqx_conf:remove_handler(?MOD_KEY_PATH('?')). + +maybe_write_certs(#{<<"backend">> := Backend} = Conf) -> + case + emqx_tls_lib:ensure_ssl_files( + ssl_file_path(Backend), maps:get(<<"ssl">>, Conf, undefined) + ) + of + {ok, SSL} -> + {ok, new_ssl_source(Conf, SSL)}; + {error, Reason} -> + ?SLOG(error, Reason#{msg => "bad_ssl_config"}), + throw({bad_ssl_config, Reason}) + end. + +ssl_file_path(Backend) -> + filename:join(["sso", Backend]). + +new_ssl_source(Source, undefined) -> + Source; +new_ssl_source(Source, SSL) -> + Source#{<<"ssl">> => SSL}. + +rollback(Backend, OldConf, OnSucc) -> + fun(_) -> + try_recreate(Backend, OldConf, OnSucc) + end. + +try_recreate(_Backend, undefined, _OnSucc) -> + ok; +try_recreate(_Backend, #{enable := false}, _OnSucc) -> + ok; +try_recreate(Backend, Config, OnSucc) -> + Provider = provider(Backend), + ?SLOG(info, #{ + msg => "backend_rollback", + backend => Backend, + reason => "update_sso_failed", + config => emqx_utils:redact(Config) + }), + on_backend_updated( + emqx_dashboard_sso:create(Provider, Config), + OnSucc, + fun(Error) -> + ?SLOG(error, #{ + msg => "backend_rollback_failed", + backend => Backend, + reason => emqx_utils:redact(Error) + }) + end + ). diff --git a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl index d85ffff10..1ed21d09e 100644 --- a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl +++ b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl @@ -27,6 +27,7 @@ all() -> t_create_atomicly, t_create, t_update, + t_update_atomicly, t_get, t_login_with_bad, t_first_login, @@ -111,6 +112,37 @@ t_update(_) -> ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)), ok. +%% update fails can rollback able +t_update_atomicly(_) -> + CurrRes = emqx_resource_manager:list_group(?RESOURCE_GROUP), + + Path = uri(["sso", "ldap"]), + ?assertMatch( + {ok, 400, _}, + request( + put, + Path, + ldap_config(#{ + <<"username">> => <<"invalid">>, + <<"enable">> => true, + <<"request_timeout">> => <<"1s">> + }) + ) + ), + + ?assertMatch(#{backend := ldap}, emqx:get_config(?MOD_KEY_PATH, undefined)), + ?assertMatch([_], ets:tab2list(?MOD_TAB)), + ?retry( + _Interval = 5, + _NAttempts = 1000, + begin + Res = emqx_resource_manager:list_group(?RESOURCE_GROUP), + ?assertMatch([_], Res), + ?assertNotMatch(CurrRes, Res) + end + ), + ok. + t_get(_) -> Path = uri(["sso", "ldap"]), {ok, 200, Result} = request(get, Path), diff --git a/apps/emqx_ldap/src/emqx_ldap.erl b/apps/emqx_ldap/src/emqx_ldap.erl index 7b856521c..e7ed01113 100644 --- a/apps/emqx_ldap/src/emqx_ldap.erl +++ b/apps/emqx_ldap/src/emqx_ldap.erl @@ -74,7 +74,7 @@ fields(config) -> {request_timeout, ?HOCON(emqx_schema:timeout_duration_ms(), #{ desc => ?DESC(request_timeout), - default => <<"5s">> + default => <<"10s">> })}, {ssl, ?HOCON(?R_REF(?MODULE, ssl), #{ From 08ad09a68f04ac1678815634e5e7dff2a38f6a4a Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 27 Sep 2023 20:53:10 +0800 Subject: [PATCH 02/10] fix(sso): refactor backen update logic 1. valid config always can update successfully 2. the `running` endpoint only return successfully created backend 3. enhancement of the `/sso` endpoint, and will check is the resource online --- .../src/emqx_dashboard_sso.erl | 4 +- .../src/emqx_dashboard_sso_api.erl | 36 ++- .../src/emqx_dashboard_sso_manager.erl | 255 ++++++++++-------- .../test/emqx_dashboard_sso_ldap_SUITE.erl | 77 +++--- rel/i18n/emqx_dashboard_sso_api.hocon | 12 + 5 files changed, 212 insertions(+), 172 deletions(-) diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl index a7742fc36..2214be06f 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl @@ -26,7 +26,9 @@ backend => atom(), atom() => term() }. --type state() :: #{atom() => term()}. + +%% Note: if a backend has a resource, it must be stored in the state and named resource_id +-type state() :: #{resource_id => binary(), atom() => term()}. -type raw_config() :: #{binary() => term()}. -type config() :: parsed_config() | raw_config(). -type hocon_ref() :: ?R_REF(Module :: atom(), Name :: atom() | binary()). diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl index 6b8d7ef42..b7859948e 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl @@ -133,24 +133,28 @@ schema("/sso/:backend") -> }. fields(backend_status) -> - emqx_dashboard_sso_schema:common_backend_schema(emqx_dashboard_sso:types()). + emqx_dashboard_sso_schema:common_backend_schema(emqx_dashboard_sso:types()) ++ + [ + {running, + mk( + boolean(), #{ + desc => ?DESC(running) + } + )}, + {last_error, + mk( + binary(), #{ + desc => ?DESC(last_error) + } + )} + ]. %%-------------------------------------------------------------------- %% API %%-------------------------------------------------------------------- running(get, _Request) -> - SSO = emqx:get_config(?MOD_KEY_PATH, #{}), - {200, - lists:filtermap( - fun - (#{backend := Backend, enable := true}) -> - {true, Backend}; - (_) -> - false - end, - maps:values(SSO) - )}. + {200, emqx_dashboard_sso_manager:running()}. login(post, #{bindings := #{backend := Backend}, body := Body} = Request) -> case emqx_dashboard_sso_manager:lookup_state(Backend) of @@ -185,8 +189,12 @@ sso(get, _Request) -> SSO = emqx:get_config(?MOD_KEY_PATH, #{}), {200, lists:map( - fun(Backend) -> - maps:with([backend, enable], Backend) + fun(#{backend := Backend, enable := Enable}) -> + Status = emqx_dashboard_sso_manager:get_backend_status(Backend, Enable), + Status#{ + backend => Backend, + enable => Enable + } end, maps:values(SSO) )}. diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl index 1d080ac5c..c1b7ce5b8 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl @@ -24,11 +24,11 @@ -export([ running/0, + get_backend_status/2, lookup_state/1, make_resource_id/1, create_resource/3, - update_resource/3, - call/1 + update_resource/3 ]). -export([ @@ -39,20 +39,21 @@ propagated_post_config_update/5 ]). --import(emqx_dashboard_sso, [provider/1]). +-import(emqx_dashboard_sso, [provider/1, format/1]). -define(MOD_TAB, emqx_dashboard_sso). -define(MOD_KEY_PATH, [dashboard, sso]). --define(CALL_TIMEOUT, timer:seconds(15)). -define(MOD_KEY_PATH(Sub), [dashboard, sso, Sub]). -define(RESOURCE_GROUP, <<"emqx_dashboard_sso">>). +-define(START_ERROR_KEY, start_error). -define(DEFAULT_RESOURCE_OPTS, #{ start_after_created => false }). -record(?MOD_TAB, { backend :: atom(), - state :: map() + state :: map(), + last_error = <<>> :: term() }). %%------------------------------------------------------------------------------ @@ -62,17 +63,36 @@ start_link() -> gen_server:start_link({local, ?MODULE}, ?MODULE, [], []). running() -> - maps:fold( + lists:filtermap( fun - (Type, #{enable := true}, Acc) -> - [Type | Acc]; - (_Type, _Cfg, Acc) -> - Acc + (#?MOD_TAB{backend = Backend, last_error = <<>>}) -> + {true, Backend}; + (_) -> + false end, - [], - emqx:get_config(?MOD_KEY_PATH) + ets:tab2list(?MOD_TAB) ). +get_backend_status(Backend, false) -> + #{ + backend => Backend, + enable => false, + running => false, + last_error => <<>> + }; +get_backend_status(Backend, _) -> + case lookup(Backend) of + undefined -> + #{ + backend => Backend, + enable => true, + running => false, + last_error => <<"resource not found">> + }; + Data -> + maps:merge(#{backend => Backend, enable => true}, do_get_backend_status(Data)) + end. + update(Backend, Config) -> update_config(Backend, {?FUNCTION_NAME, Backend, Config}). delete(Backend) -> @@ -106,14 +126,6 @@ update_resource(ResourceId, Module, Config) -> ), start_resource_if_enabled(ResourceId, Result, Config). -call(Req) -> - try - gen_server:call(?MODULE, Req, ?CALL_TIMEOUT) - catch - exit:{timeout, _} -> - {error, <<"Update backend failed: timeout">>} - end. - %%------------------------------------------------------------------------------ %% gen_server callbacks %%------------------------------------------------------------------------------ @@ -133,9 +145,6 @@ init([]) -> start_backend_services(), {ok, #{}}. -handle_call({update_config, Req, NewConf, OldConf}, _From, State) -> - Result = on_config_update(Req, NewConf, OldConf), - {reply, Result, State}; handle_call(_Request, _From, State) -> Reply = ok, {reply, Reply, State}. @@ -177,35 +186,47 @@ start_backend_services() -> backend => Backend, reason => emqx_utils:redact(Reason) }) - end + end, + record_start_error(Backend, false) end, maps:to_list(Backends) ). update_config(Backend, UpdateReq) -> + OkFun = fun(Result) -> + ?SLOG(info, #{ + msg => "update_sso_successfully", + backend => Backend, + result => emqx_utils:redact(Result) + }), + Result + end, + + ErrFun = fun({error, Reason} = Error) -> + SafeReason = emqx_utils:redact(Reason), + ?SLOG(error, #{ + msg => "update_sso_failed", + backend => Backend, + reason => SafeReason + }), + Error + end, + + %% we always make sure the valid configuration will update successfully, + %% ignore the runtime error during its update case emqx_conf:update(?MOD_KEY_PATH(Backend), UpdateReq, #{override_to => cluster}) of {ok, UpdateResult} -> #{post_config_update := #{?MODULE := Result}} = UpdateResult, - ?SLOG(info, #{ - msg => "update_sso_successfully", - backend => Backend, - result => emqx_utils:redact(Result) - }), - Result; - {error, Reason} -> - SafeReason = emqx_utils:redact(Reason), - ?SLOG(error, #{ - msg => "update_sso_failed", - backend => Backend, - reason => SafeReason - }), - {error, - case SafeReason of - {_Stage, _Mod, Reason2} -> - Reason2; - _ -> - Reason - end} + case Result of + ok -> + OkFun(Result); + {ok, _} -> + OkFun(Result); + {error, _} = Error -> + ErrFun(Error) + end; + {error, _} = Error -> + ErrFun(Error) end. pre_config_update(_, {update, _Backend, Config}, _OldConf) -> @@ -215,8 +236,8 @@ pre_config_update(_, {delete, _Backend}, undefined) -> pre_config_update(_, {delete, _Backend}, _OldConf) -> {ok, null}. -post_config_update(_, UpdateReq, NewConf, OldConf, _AppEnvs) -> - call({update_config, UpdateReq, NewConf, OldConf}). +post_config_update(_, UpdateReq, NewConf, _OldConf, _AppEnvs) -> + {ok, on_config_update(UpdateReq, NewConf)}. propagated_post_config_update( ?MOD_KEY_PATH(BackendBin) = Path, _UpdateReq, undefined, OldConf, AppEnvs @@ -237,44 +258,37 @@ propagated_post_config_update( Error end. -on_config_update({update, Backend, _RawConfig}, Config, OldConfig) -> +on_config_update({update, Backend, _RawConfig}, Config) -> Provider = provider(Backend), case lookup(Backend) of undefined -> on_backend_updated( + Backend, emqx_dashboard_sso:create(Provider, Config), - fun(State) -> - ets:insert(?MOD_TAB, #?MOD_TAB{backend = Backend, state = State}) + fun(State, LastError) -> + ets:insert( + ?MOD_TAB, + #?MOD_TAB{backend = Backend, state = State, last_error = LastError} + ) end ); Data -> - %% the steps for updating/recreating a resource are: - %% 1. destroy the old resource - %% 2. create a new resource - %% to keep consistency we need to follow those steps too, - %% however a failed update will not change the config, but will lose the resource - %% hence for consistency and atomicity, we should rollback when the update fails - ets:delete(?MOD_TAB, Backend), - UpdateState = fun(State) -> - ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State}) - end, on_backend_updated( + Backend, emqx_dashboard_sso:update(Provider, Config, Data#?MOD_TAB.state), - UpdateState, - rollback( - Backend, - OldConfig, - UpdateState - ) + fun(State, LastError) -> + ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State, last_error = LastError}) + end ) end; -on_config_update({delete, Backend}, _NewConf, _OldConf) -> +on_config_update({delete, Backend}, _NewConf) -> case lookup(Backend) of undefined -> {error, not_exists}; Data -> Provider = provider(Backend), on_backend_updated( + Backend, emqx_dashboard_sso:destroy(Provider, Data#?MOD_TAB.state), fun() -> ets:delete(?MOD_TAB, Backend) @@ -290,45 +304,39 @@ lookup(Backend) -> undefined end. +%% to avoid resource leakage the resource start will never affect the update result, +%% so the resource_id will always be recorded start_resource_if_enabled(ResourceId, {ok, _} = Result, #{enable := true}) -> + clear_start_error(), case emqx_resource:start(ResourceId) of ok -> - Result; + ok; {error, Reason} -> SafeReason = emqx_utils:redact(Reason), + mark_start_error(SafeReason), ?SLOG(error, #{ msg => "start_backend_failed", resource_id => ResourceId, reason => SafeReason }), - clean_when_start_failed(ResourceId), - {error, emqx_dashboard_sso:format(["Start backend failed, Reason: ", SafeReason])} - end; + ok + end, + Result; start_resource_if_enabled(_ResourceId, Result, _Config) -> Result. -%% ensure the backend creation is atomic, clean the corresponding resource when necessary, -%% when creating a backend fails, nothing will be inserted into the SSO table, -%% thus the resources created by backend will leakage. -%% Although we can treat start failure as successful, -%% and insert the resource data into the SSO table, -%% it may be strange for users: it succeeds, but can't be used. -clean_when_start_failed(ResourceId) -> - _ = emqx_resource:remove_local(ResourceId), - ok. - -on_backend_updated(Result, OkFun) -> - on_backend_updated(Result, OkFun, undefined). - -%% this first level `ok` is for emqx_config_handler, and the second level is for the caller -on_backend_updated({ok, State} = Ok, Fun, _ErrFun) -> - Fun(State), - {ok, Ok}; -on_backend_updated(ok, Fun, _ErrFun) -> +on_backend_updated(Backend, {ok, State} = Ok, Fun) -> + Fun(State, <<>>), + record_start_error(Backend, true), + Ok; +on_backend_updated(_Backend, {ok, State, LastError} = Ok, Fun) -> + Fun(State, LastError), + Ok; +on_backend_updated(_Backend, ok, Fun) -> Fun(), - {ok, ok}; -on_backend_updated(Error, _, ErrFun) -> - erlang:is_function(ErrFun) andalso ErrFun(Error), + ok; +on_backend_updated(Backend, {error, Reason} = Error, _) -> + update_last_error(Backend, Reason), Error. bin(A) when is_atom(A) -> atom_to_binary(A, utf8); @@ -365,31 +373,44 @@ new_ssl_source(Source, undefined) -> new_ssl_source(Source, SSL) -> Source#{<<"ssl">> => SSL}. -rollback(Backend, OldConf, OnSucc) -> - fun(_) -> - try_recreate(Backend, OldConf, OnSucc) +clear_start_error() -> + mark_start_error(<<>>). + +mark_start_error(Reason) -> + erlang:put(?START_ERROR_KEY, Reason). + +record_start_error(Backend, Force) -> + case erlang:get(?START_ERROR_KEY) of + <<>> when Force -> + update_last_error(Backend, <<>>); + <<>> -> + ok; + Reason -> + update_last_error(Backend, Reason) end. -try_recreate(_Backend, undefined, _OnSucc) -> - ok; -try_recreate(_Backend, #{enable := false}, _OnSucc) -> - ok; -try_recreate(Backend, Config, OnSucc) -> - Provider = provider(Backend), - ?SLOG(info, #{ - msg => "backend_rollback", - backend => Backend, - reason => "update_sso_failed", - config => emqx_utils:redact(Config) - }), - on_backend_updated( - emqx_dashboard_sso:create(Provider, Config), - OnSucc, - fun(Error) -> - ?SLOG(error, #{ - msg => "backend_rollback_failed", - backend => Backend, - reason => emqx_utils:redact(Error) - }) - end - ). +update_last_error(Backend, Reason) -> + ets:update_element(?MOD_TAB, Backend, {#?MOD_TAB.last_error, Reason}). + +do_get_backend_status(#?MOD_TAB{state = #{resource_id := ResourceId}}) -> + case emqx_resource_manager:lookup(ResourceId) of + {ok, _Group, #{status := connected}} -> + #{running => true, last_error => <<>>}; + {ok, _Group, #{status := Status}} -> + #{ + running => false, + last_error => format([<<"Resource not valid, status: ">>, Status]) + }; + {error, not_found} -> + #{ + running => false, + last_error => <<"Resource not found">> + } + end; +do_get_backend_status(#?MOD_TAB{last_error = <<>>}) -> + #{running => true, last_error => <<>>}; +do_get_backend_status(#?MOD_TAB{last_error = LastError}) -> + #{ + running => false, + last_error => format([LastError]) + }. diff --git a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl index 1ed21d09e..7882e17f2 100644 --- a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl +++ b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl @@ -24,14 +24,14 @@ all() -> [ - t_create_atomicly, + t_bad_create, t_create, t_update, - t_update_atomicly, t_get, t_login_with_bad, t_first_login, t_next_login, + t_bad_update, t_delete ]. @@ -61,10 +61,10 @@ end_per_testcase(Case, _) -> end, ok. -t_create_atomicly(_) -> +t_bad_create(_) -> Path = uri(["sso", "ldap"]), ?assertMatch( - {ok, 400, _}, + {ok, 200, _}, request( put, Path, @@ -75,12 +75,18 @@ t_create_atomicly(_) -> }) ) ), - ?assertEqual(undefined, emqx:get_config(?MOD_KEY_PATH, undefined)), - ?assertEqual([], ets:tab2list(?MOD_TAB)), + ?assertMatch(#{backend := ldap}, emqx:get_config(?MOD_KEY_PATH, undefined)), + check_running([]), + ?assertMatch( + [#{backend := <<"ldap">>, enable := true, running := false, last_error := _}], get_sso() + ), + + emqx_dashboard_sso_manager:delete(ldap), + ?retry( - _Interval = 1000, - _NAttempts = 5, - ?assertEqual([], emqx_resource_manager:list_group(?RESOURCE_GROUP)) + _Interval = 500, + _NAttempts = 10, + ?assertMatch([], emqx_resource_manager:list_group(?RESOURCE_GROUP)) ), ok. @@ -112,37 +118,6 @@ t_update(_) -> ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)), ok. -%% update fails can rollback able -t_update_atomicly(_) -> - CurrRes = emqx_resource_manager:list_group(?RESOURCE_GROUP), - - Path = uri(["sso", "ldap"]), - ?assertMatch( - {ok, 400, _}, - request( - put, - Path, - ldap_config(#{ - <<"username">> => <<"invalid">>, - <<"enable">> => true, - <<"request_timeout">> => <<"1s">> - }) - ) - ), - - ?assertMatch(#{backend := ldap}, emqx:get_config(?MOD_KEY_PATH, undefined)), - ?assertMatch([_], ets:tab2list(?MOD_TAB)), - ?retry( - _Interval = 5, - _NAttempts = 1000, - begin - Res = emqx_resource_manager:list_group(?RESOURCE_GROUP), - ?assertMatch([_], Res), - ?assertNotMatch(CurrRes, Res) - end - ), - ok. - t_get(_) -> Path = uri(["sso", "ldap"]), {ok, 200, Result} = request(get, Path), @@ -190,6 +165,28 @@ t_next_login(_) -> ?assertMatch(#{license := _, token := _}, decode_json(Result)), ok. +t_bad_update(_) -> + Path = uri(["sso", "ldap"]), + ?assertMatch( + {ok, 200, _}, + request( + put, + Path, + ldap_config(#{ + <<"username">> => <<"invalid">>, + <<"enable">> => true, + <<"request_timeout">> => <<"1s">> + }) + ) + ), + ?assertMatch(#{backend := ldap}, emqx:get_config(?MOD_KEY_PATH, undefined)), + check_running([]), + ?assertMatch( + [#{backend := <<"ldap">>, enable := true, running := false, last_error := _}], get_sso() + ), + + ok. + t_delete(_) -> Path = uri(["sso", "ldap"]), ?assertMatch({ok, 204, _}, request(delete, Path)), diff --git a/rel/i18n/emqx_dashboard_sso_api.hocon b/rel/i18n/emqx_dashboard_sso_api.hocon index a7ce1f97e..b65142366 100644 --- a/rel/i18n/emqx_dashboard_sso_api.hocon +++ b/rel/i18n/emqx_dashboard_sso_api.hocon @@ -51,4 +51,16 @@ backend_name.desc: backend_name.label: """Backend Name""" +running.desc: +"""Is the backend running""" + +running.label: +"""Running""" + +last_error.desc: +"""Last error of this backend""" + +last_error.label: +"""Last Error""" + } From 66d21070073127934ce416e4fbf059e94ef97361 Mon Sep 17 00:00:00 2001 From: firest Date: Thu, 28 Sep 2023 00:09:09 +0800 Subject: [PATCH 03/10] fix(sso): refactor update logic --- .../src/emqx_dashboard_sso_api.erl | 2 - .../src/emqx_dashboard_sso_manager.erl | 129 +++++++----------- 2 files changed, 53 insertions(+), 78 deletions(-) diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl index b7859948e..c023ace51 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_api.erl @@ -271,8 +271,6 @@ handle_backend_update_result(ok, _) -> 204; handle_backend_update_result({error, not_exists}, _) -> {404, #{code => ?BACKEND_NOT_FOUND, message => <<"Backend not found">>}}; -handle_backend_update_result({error, already_exists}, _) -> - {400, #{code => ?BAD_REQUEST, message => <<"Backend already exists">>}}; handle_backend_update_result({error, failed_to_load_metadata}, _) -> {400, #{code => ?BAD_REQUEST, message => <<"Failed to load metadata">>}}; handle_backend_update_result({error, Reason}, _) when is_binary(Reason) -> diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl index c1b7ce5b8..a3512ac2f 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl @@ -24,8 +24,8 @@ -export([ running/0, - get_backend_status/2, lookup_state/1, + get_backend_status/2, make_resource_id/1, create_resource/3, update_resource/3 @@ -45,7 +45,7 @@ -define(MOD_KEY_PATH, [dashboard, sso]). -define(MOD_KEY_PATH(Sub), [dashboard, sso, Sub]). -define(RESOURCE_GROUP, <<"emqx_dashboard_sso">>). --define(START_ERROR_KEY, start_error). +-define(NO_ERROR, <<>>). -define(DEFAULT_RESOURCE_OPTS, #{ start_after_created => false }). @@ -53,7 +53,7 @@ -record(?MOD_TAB, { backend :: atom(), state :: map(), - last_error = <<>> :: term() + last_error = ?NO_ERROR :: term() }). %%------------------------------------------------------------------------------ @@ -65,7 +65,7 @@ start_link() -> running() -> lists:filtermap( fun - (#?MOD_TAB{backend = Backend, last_error = <<>>}) -> + (#?MOD_TAB{backend = Backend, last_error = ?NO_ERROR}) -> {true, Backend}; (_) -> false @@ -78,7 +78,7 @@ get_backend_status(Backend, false) -> backend => Backend, enable => false, running => false, - last_error => <<>> + last_error => ?NO_ERROR }; get_backend_status(Backend, _) -> case lookup(Backend) of @@ -87,7 +87,7 @@ get_backend_status(Backend, _) -> backend => Backend, enable => true, running => false, - last_error => <<"resource not found">> + last_error => <<"Resource not found">> }; Data -> maps:merge(#{backend => Backend, enable => true}, do_get_backend_status(Data)) @@ -179,54 +179,41 @@ start_backend_services() -> msg => "start_sso_backend_successfully", backend => Backend }), - ets:insert(?MOD_TAB, #?MOD_TAB{backend = Backend, state = State}); + update_state(Backend, State); {error, Reason} -> + SafeReason = emqx_utils:redact(Reason), + update_last_error(Backend, SafeReason), ?SLOG(error, #{ msg => "start_sso_backend_failed", backend => Backend, - reason => emqx_utils:redact(Reason) + reason => SafeReason }) - end, - record_start_error(Backend, false) + end end, maps:to_list(Backends) ). update_config(Backend, UpdateReq) -> - OkFun = fun(Result) -> - ?SLOG(info, #{ - msg => "update_sso_successfully", - backend => Backend, - result => emqx_utils:redact(Result) - }), - Result - end, - - ErrFun = fun({error, Reason} = Error) -> - SafeReason = emqx_utils:redact(Reason), - ?SLOG(error, #{ - msg => "update_sso_failed", - backend => Backend, - reason => SafeReason - }), - Error - end, - %% we always make sure the valid configuration will update successfully, %% ignore the runtime error during its update case emqx_conf:update(?MOD_KEY_PATH(Backend), UpdateReq, #{override_to => cluster}) of - {ok, UpdateResult} -> - #{post_config_update := #{?MODULE := Result}} = UpdateResult, - case Result of - ok -> - OkFun(Result); - {ok, _} -> - OkFun(Result); - {error, _} = Error -> - ErrFun(Error) + {ok, _UpdateResult} -> + case lookup(Backend) of + undefined -> + ok; + #?MOD_TAB{state = State, last_error = ?NO_ERROR} -> + {ok, State}; + Data -> + {error, Data#?MOD_TAB.last_error} end; - {error, _} = Error -> - ErrFun(Error) + {error, Reason} = Error -> + SafeReason = emqx_utils:redact(Reason), + ?SLOG(error, #{ + msg => "update_sso_failed", + backend => Backend, + reason => SafeReason + }), + Error end. pre_config_update(_, {update, _Backend, Config}, _OldConf) -> @@ -237,7 +224,8 @@ pre_config_update(_, {delete, _Backend}, _OldConf) -> {ok, null}. post_config_update(_, UpdateReq, NewConf, _OldConf, _AppEnvs) -> - {ok, on_config_update(UpdateReq, NewConf)}. + _ = on_config_update(UpdateReq, NewConf), + ok. propagated_post_config_update( ?MOD_KEY_PATH(BackendBin) = Path, _UpdateReq, undefined, OldConf, AppEnvs @@ -265,26 +253,23 @@ on_config_update({update, Backend, _RawConfig}, Config) -> on_backend_updated( Backend, emqx_dashboard_sso:create(Provider, Config), - fun(State, LastError) -> - ets:insert( - ?MOD_TAB, - #?MOD_TAB{backend = Backend, state = State, last_error = LastError} - ) + fun(State) -> + update_state(Backend, State) end ); Data -> on_backend_updated( Backend, emqx_dashboard_sso:update(Provider, Config, Data#?MOD_TAB.state), - fun(State, LastError) -> - ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State, last_error = LastError}) + fun(State) -> + update_state(Backend, State) end ) end; on_config_update({delete, Backend}, _NewConf) -> case lookup(Backend) of undefined -> - {error, not_exists}; + on_backend_updated(Backend, {error, not_exists}, undefined); Data -> Provider = provider(Backend), on_backend_updated( @@ -306,31 +291,26 @@ lookup(Backend) -> %% to avoid resource leakage the resource start will never affect the update result, %% so the resource_id will always be recorded -start_resource_if_enabled(ResourceId, {ok, _} = Result, #{enable := true}) -> - clear_start_error(), +start_resource_if_enabled(ResourceId, {ok, _} = Result, #{enable := true, backend := Backend}) -> case emqx_resource:start(ResourceId) of ok -> ok; {error, Reason} -> SafeReason = emqx_utils:redact(Reason), - mark_start_error(SafeReason), ?SLOG(error, #{ msg => "start_backend_failed", resource_id => ResourceId, reason => SafeReason }), + update_last_error(Backend, SafeReason), ok end, Result; start_resource_if_enabled(_ResourceId, Result, _Config) -> Result. -on_backend_updated(Backend, {ok, State} = Ok, Fun) -> - Fun(State, <<>>), - record_start_error(Backend, true), - Ok; -on_backend_updated(_Backend, {ok, State, LastError} = Ok, Fun) -> - Fun(State, LastError), +on_backend_updated(_Backend, {ok, State} = Ok, Fun) -> + Fun(State), Ok; on_backend_updated(_Backend, ok, Fun) -> Fun(), @@ -373,29 +353,26 @@ new_ssl_source(Source, undefined) -> new_ssl_source(Source, SSL) -> Source#{<<"ssl">> => SSL}. -clear_start_error() -> - mark_start_error(<<>>). +update_state(Backend, State) -> + Data = ensure_backend_data(Backend), + ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State}). -mark_start_error(Reason) -> - erlang:put(?START_ERROR_KEY, Reason). +update_last_error(Backend, LastError) -> + Data = ensure_backend_data(Backend), + ets:insert(?MOD_TAB, Data#?MOD_TAB{last_error = LastError}). -record_start_error(Backend, Force) -> - case erlang:get(?START_ERROR_KEY) of - <<>> when Force -> - update_last_error(Backend, <<>>); - <<>> -> - ok; - Reason -> - update_last_error(Backend, Reason) +ensure_backend_data(Backend) -> + case ets:lookup(?MOD_TAB, Backend) of + [Data] -> + Data; + [] -> + #?MOD_TAB{backend = Backend} end. -update_last_error(Backend, Reason) -> - ets:update_element(?MOD_TAB, Backend, {#?MOD_TAB.last_error, Reason}). - do_get_backend_status(#?MOD_TAB{state = #{resource_id := ResourceId}}) -> case emqx_resource_manager:lookup(ResourceId) of {ok, _Group, #{status := connected}} -> - #{running => true, last_error => <<>>}; + #{running => true, last_error => ?NO_ERROR}; {ok, _Group, #{status := Status}} -> #{ running => false, @@ -407,8 +384,8 @@ do_get_backend_status(#?MOD_TAB{state = #{resource_id := ResourceId}}) -> last_error => <<"Resource not found">> } end; -do_get_backend_status(#?MOD_TAB{last_error = <<>>}) -> - #{running => true, last_error => <<>>}; +do_get_backend_status(#?MOD_TAB{last_error = ?NO_ERROR}) -> + #{running => true, last_error => ?NO_ERROR}; do_get_backend_status(#?MOD_TAB{last_error = LastError}) -> #{ running => false, From c8cbbff044d81d637c50aff6b1d80785f0ea71a8 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 21:26:47 +0200 Subject: [PATCH 04/10] fix(logger): no need for special handling of empty string when formating json logs, there is no need to handle empty strings special, already covered by unicode handling --- apps/emqx/src/emqx_logger_jsonfmt.erl | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/emqx/src/emqx_logger_jsonfmt.erl b/apps/emqx/src/emqx_logger_jsonfmt.erl index c6c04f70d..cb867793e 100644 --- a/apps/emqx/src/emqx_logger_jsonfmt.erl +++ b/apps/emqx/src/emqx_logger_jsonfmt.erl @@ -224,10 +224,6 @@ best_effort_json_obj(Map, Config) -> do_format_msg("~p", [Map], Config) end. -json([], _) -> - ""; -json(<<"">>, _) -> - "\"\""; json(A, _) when is_atom(A) -> atom_to_binary(A, utf8); json(I, _) when is_integer(I) -> I; json(F, _) when is_float(F) -> F; From 45caa3bf0199dca011ccd6fb23c77b2d354b0be0 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 21:27:59 +0200 Subject: [PATCH 05/10] fix(sso): make sp_private_key sensitive so it will not be logged --- apps/emqx_utils/src/emqx_utils.erl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/apps/emqx_utils/src/emqx_utils.erl b/apps/emqx_utils/src/emqx_utils.erl index 0682a9b4d..5c766a388 100644 --- a/apps/emqx_utils/src/emqx_utils.erl +++ b/apps/emqx_utils/src/emqx_utils.erl @@ -645,6 +645,7 @@ try_to_existing_atom(Convert, Data, Encoding) -> _:Reason -> {error, Reason} end. +%% NOTE: keep alphabetical order is_sensitive_key(aws_secret_access_key) -> true; is_sensitive_key("aws_secret_access_key") -> true; is_sensitive_key(<<"aws_secret_access_key">>) -> true; @@ -663,6 +664,8 @@ is_sensitive_key(<<"secret_key">>) -> true; is_sensitive_key(security_token) -> true; is_sensitive_key("security_token") -> true; is_sensitive_key(<<"security_token">>) -> true; +is_sensitive_key(sp_private_key) -> true; +is_sensitive_key(<<"sp_private_key">>) -> true; is_sensitive_key(token) -> true; is_sensitive_key("token") -> true; is_sensitive_key(<<"token">>) -> true; From bb49914fd61231bd83c67e10d0545f5741f2db00 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 22:32:26 +0200 Subject: [PATCH 06/10] fix(sso): add convet_certs callback for sso backends must convert certs in pre_config_update so the cert path refernces are stored in raw config, otherwise the files might get gc:ed --- .../src/emqx_dashboard_sso.erl | 11 +++++- .../src/emqx_dashboard_sso_ldap.erl | 21 +++++++++- .../src/emqx_dashboard_sso_manager.erl | 25 +++--------- .../src/emqx_dashboard_sso_saml.erl | 38 ++++++++++++------- 4 files changed, 60 insertions(+), 35 deletions(-) diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl index 2214be06f..971465587 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso.erl @@ -13,7 +13,8 @@ create/2, update/3, destroy/2, - login/3 + login/3, + convert_certs/3 ]). -export([types/0, modules/0, provider/1, backends/0, format/1]). @@ -45,6 +46,11 @@ | {redirect, tuple()} | {error, Reason :: term()}. +-callback convert_certs( + Dir :: file:filename_all(), + config() +) -> config(). + %%------------------------------------------------------------------------------ %% Callback Interface %%------------------------------------------------------------------------------ @@ -68,6 +74,9 @@ destroy(Mod, State) -> login(Mod, Req, State) -> Mod:login(Req, State). +convert_certs(Mod, Dir, Config) -> + Mod:convert_certs(Dir, Config). + %%------------------------------------------------------------------------------ %% API %%------------------------------------------------------------------------------ diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl index bea8ef7c6..83a57cf7b 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl @@ -22,7 +22,8 @@ login/2, create/1, update/2, - destroy/1 + destroy/1, + convert_certs/2 ]). %%------------------------------------------------------------------------------ @@ -163,3 +164,21 @@ ensure_user_exists(Username) -> Error end end. + +convert_certs(Dir, Conf) -> + case + emqx_tls_lib:ensure_ssl_files( + Dir, maps:get(<<"ssl">>, Conf, undefined) + ) + of + {ok, SSL} -> + new_ssl_source(Conf, SSL); + {error, Reason} -> + ?SLOG(error, Reason#{msg => "bad_ssl_config"}), + throw({bad_ssl_config, Reason}) + end. + +new_ssl_source(Source, undefined) -> + Source; +new_ssl_source(Source, SSL) -> + Source#{<<"ssl">> => SSL}. diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl index a3512ac2f..473881cf9 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl @@ -52,7 +52,7 @@ -record(?MOD_TAB, { backend :: atom(), - state :: map(), + state :: undefined | map(), last_error = ?NO_ERROR :: term() }). @@ -217,7 +217,7 @@ update_config(Backend, UpdateReq) -> end. pre_config_update(_, {update, _Backend, Config}, _OldConf) -> - maybe_write_certs(Config); + {ok, maybe_write_certs(Config)}; pre_config_update(_, {delete, _Backend}, undefined) -> throw(not_exists); pre_config_update(_, {delete, _Backend}, _OldConf) -> @@ -333,26 +333,13 @@ remove_handler() -> ok = emqx_conf:remove_handler(?MOD_KEY_PATH('?')). maybe_write_certs(#{<<"backend">> := Backend} = Conf) -> - case - emqx_tls_lib:ensure_ssl_files( - ssl_file_path(Backend), maps:get(<<"ssl">>, Conf, undefined) - ) - of - {ok, SSL} -> - {ok, new_ssl_source(Conf, SSL)}; - {error, Reason} -> - ?SLOG(error, Reason#{msg => "bad_ssl_config"}), - throw({bad_ssl_config, Reason}) - end. + Dir = certs_path(Backend), + Provider = provider(Backend), + emqx_dashboard_sso:convert_certs(Provider, Dir, Conf). -ssl_file_path(Backend) -> +certs_path(Backend) -> filename:join(["sso", Backend]). -new_ssl_source(Source, undefined) -> - Source; -new_ssl_source(Source, SSL) -> - Source#{<<"ssl">> => SSL}. - update_state(Backend, State) -> Data = ensure_backend_data(Backend), ets:insert(?MOD_TAB, Data#?MOD_TAB{state = State}). diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_saml.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_saml.erl index fc2cadfe6..2549c1334 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_saml.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_saml.erl @@ -22,7 +22,8 @@ -export([ create/1, update/2, - destroy/1 + destroy/1, + convert_certs/2 ]). -export([login/2, callback/2]). @@ -102,7 +103,7 @@ desc(_) -> create(#{sp_sign_request := true} = Config) -> try - do_create(ensure_cert_and_key(Config)) + do_create(Config) catch Kind:Error -> Msg = failed_to_ensure_cert_and_key, @@ -150,10 +151,31 @@ callback(_Req = #{body := Body}, #{sp := SP, dashboard_addr := DashboardAddr} = {error, iolist_to_binary(Reason)} end. +convert_certs( + Dir, + #{<<"sp_sign_request">> := true, <<"sp_public_key">> := Cert, <<"sp_private_key">> := Key} = + Conf +) -> + case + emqx_tls_lib:ensure_ssl_files( + Dir, #{enable => ture, certfile => Cert, keyfile => Key}, #{} + ) + of + {ok, #{certfile := CertPath, keyfile := KeyPath}} -> + Conf#{<<"sp_public_key">> => bin(CertPath), <<"sp_private_key">> => bin(KeyPath)}; + {error, Reason} -> + ?SLOG(error, #{msg => "failed_to_save_sp_sign_keys", reason => Reason}), + throw("Failed to save sp signing key(s)") + end; +convert_certs(_Dir, Conf) -> + Conf. + %%------------------------------------------------------------------------------ %% Internal functions %%------------------------------------------------------------------------------ +bin(X) -> iolist_to_binary(X). + do_create( #{ dashboard_addr := DashboardAddr, @@ -222,18 +244,6 @@ gen_redirect_response(DashboardAddr, Username) -> %% Helpers functions %%------------------------------------------------------------------------------ -ensure_cert_and_key(#{sp_public_key := Cert, sp_private_key := Key} = Config) -> - case - emqx_tls_lib:ensure_ssl_files( - ?DIR, #{enable => ture, certfile => Cert, keyfile => Key}, #{} - ) - of - {ok, #{certfile := CertPath, keyfile := KeyPath} = _NSSL} -> - Config#{sp_public_key => CertPath, sp_private_key => KeyPath}; - {error, #{which_options := KeyPath}} -> - error({missing_key, lists:flatten(KeyPath)}) - end. - %% TODO: unify with emqx_dashboard_sso_manager:ensure_user_exists/1 ensure_user_exists(Username) -> case emqx_dashboard_admin:lookup_user(saml, Username) of From 34367fc4ec08d599b62f0b194603c3e81a88bdb0 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 23:06:14 +0200 Subject: [PATCH 07/10] fix(audit_log): pretty print shell args --- apps/emqx_machine/src/emqx_restricted_shell.erl | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/apps/emqx_machine/src/emqx_restricted_shell.erl b/apps/emqx_machine/src/emqx_restricted_shell.erl index ea6d7d7ab..115fa478f 100644 --- a/apps/emqx_machine/src/emqx_restricted_shell.erl +++ b/apps/emqx_machine/src/emqx_restricted_shell.erl @@ -104,7 +104,7 @@ max_heap_size_warning(MF, Args) -> msg => "shell_process_exceed_max_heap_size", current_heap_size => HeapSize, function => MF, - args => Args, + args => pp_args(Args), max_heap_size => ?MAX_HEAP_SIZE }) end. @@ -115,21 +115,30 @@ log(IsAllow, MF, Args) -> ?AUDIT(warning, "from_remote_console", #{ time => logger:timestamp(), function => MF, - args => Args, + args => pp_args(Args), permission => IsAllow }), to_console(IsAllow, MF, Args). to_console(prohibited, MF, Args) -> warning("DANGEROUS FUNCTION: FORBIDDEN IN SHELL!!!!!", []), - ?SLOG(error, #{msg => "execute_function_in_shell_prohibited", function => MF, args => Args}); + ?SLOG(error, #{ + msg => "execute_function_in_shell_prohibited", + function => MF, + args => pp_args(Args) + }); to_console(exempted, MF, Args) -> limit_warning(MF, Args), ?SLOG(error, #{ - msg => "execute_dangerous_function_in_shell_exempted", function => MF, args => Args + msg => "execute_dangerous_function_in_shell_exempted", + function => MF, + args => pp_args(Args) }); to_console(ok, MF, Args) -> limit_warning(MF, Args). warning(Format, Args) -> io:format(?RED_BG ++ Format ++ ?RESET ++ "~n", Args). + +pp_args(Args) -> + iolist_to_binary(io_lib:format("~0p", [Args])). From 6f7a4344dc1fcc3015a3cea8976bb8d207eb109c Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 23:08:26 +0200 Subject: [PATCH 08/10] fix: do not gc sso saml SP singing keys --- apps/emqx/src/emqx_tls_certfile_gc.erl | 5 ++++- apps/emqx/src/emqx_tls_lib.erl | 8 +++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/apps/emqx/src/emqx_tls_certfile_gc.erl b/apps/emqx/src/emqx_tls_certfile_gc.erl index 9e2e98b7f..ccac02471 100644 --- a/apps/emqx/src/emqx_tls_certfile_gc.erl +++ b/apps/emqx/src/emqx_tls_certfile_gc.erl @@ -271,9 +271,12 @@ find_config_references(Root) -> is_file_reference(Stack) -> lists:any( fun(KP) -> lists:prefix(lists:reverse(KP), Stack) end, - emqx_tls_lib:ssl_file_conf_keypaths() + conf_keypaths() ). +conf_keypaths() -> + emqx_tls_lib:ssl_file_conf_keypaths(). + mk_fileref(AbsPath) -> case emqx_utils_fs:read_info(AbsPath) of {ok, Info} -> diff --git a/apps/emqx/src/emqx_tls_lib.erl b/apps/emqx/src/emqx_tls_lib.erl index 9113bd5e6..0b9bfe805 100644 --- a/apps/emqx/src/emqx_tls_lib.erl +++ b/apps/emqx/src/emqx_tls_lib.erl @@ -50,11 +50,17 @@ -define(IS_FALSE(Val), ((Val =:= false) orelse (Val =:= <<"false">>))). -define(SSL_FILE_OPT_PATHS, [ + %% common ssl options [<<"keyfile">>], [<<"certfile">>], [<<"cacertfile">>], - [<<"ocsp">>, <<"issuer_pem">>] + %% OCSP + [<<"ocsp">>, <<"issuer_pem">>], + %% SSO + [<<"sp_public_key">>], + [<<"sp_private_key">>] ]). + -define(SSL_FILE_OPT_PATHS_A, [ [keyfile], [certfile], From afdda107af07a08b1d2b6c069df73d36697d4230 Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Wed, 27 Sep 2023 23:38:55 +0200 Subject: [PATCH 09/10] fix(logger): json format log encode binary list as string array --- apps/emqx/src/emqx_logger_jsonfmt.erl | 29 +++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/apps/emqx/src/emqx_logger_jsonfmt.erl b/apps/emqx/src/emqx_logger_jsonfmt.erl index cb867793e..0f04ee28c 100644 --- a/apps/emqx/src/emqx_logger_jsonfmt.erl +++ b/apps/emqx/src/emqx_logger_jsonfmt.erl @@ -235,12 +235,18 @@ json(B, Config) when is_binary(B) -> json(M, Config) when is_list(M), is_tuple(hd(M)), tuple_size(hd(M)) =:= 2 -> best_effort_json_obj(M, Config); json(L, Config) when is_list(L) -> - try unicode:characters_to_binary(L, utf8) of - B when is_binary(B) -> B; - _ -> [json(I, Config) || I <- L] - catch - _:_ -> - [json(I, Config) || I <- L] + case lists:all(fun erlang:is_binary/1, L) of + true -> + %% string array + L; + false -> + try unicode:characters_to_binary(L, utf8) of + B when is_binary(B) -> B; + _ -> [json(I, Config) || I <- L] + catch + _:_ -> + [json(I, Config) || I <- L] + end end; json(Map, Config) when is_map(Map) -> best_effort_json_obj(Map, Config); @@ -458,4 +464,15 @@ chars_limit_applied_on_format_result_test() -> ?assertEqual(Limit, size(LongStr1)), ok. +string_array_test() -> + Array = #{<<"arr">> => [<<"a">>, <<"b">>]}, + Encoded = emqx_utils_json:encode(json(Array, config())), + ?assertEqual(Array, emqx_utils_json:decode(Encoded)). + +iolist_test() -> + Iolist = #{iolist => ["a", ["b"]]}, + Concat = #{<<"iolist">> => <<"ab">>}, + Encoded = emqx_utils_json:encode(json(Iolist, config())), + ?assertEqual(Concat, emqx_utils_json:decode(Encoded)). + -endif. From 9dee2dc31e623989a510f4145975f0460fe51f26 Mon Sep 17 00:00:00 2001 From: firest Date: Thu, 28 Sep 2023 08:56:16 +0800 Subject: [PATCH 10/10] fix(sso): clear last error first before update && fix the `running` --- .../src/emqx_dashboard_sso_manager.erl | 15 ++++++++++++--- .../test/emqx_dashboard_sso_ldap_SUITE.erl | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl index 473881cf9..ebcfd84cf 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_manager.erl @@ -63,14 +63,22 @@ start_link() -> gen_server:start_link({local, ?MODULE}, ?MODULE, [], []). running() -> + SSO = emqx:get_config(?MOD_KEY_PATH, #{}), lists:filtermap( fun - (#?MOD_TAB{backend = Backend, last_error = ?NO_ERROR}) -> - {true, Backend}; + (#{backend := Backend, enable := true}) -> + case lookup(Backend) of + undefined -> + false; + #?MOD_TAB{last_error = ?NO_ERROR} -> + {true, Backend}; + _ -> + false + end; (_) -> false end, - ets:tab2list(?MOD_TAB) + maps:values(SSO) ). get_backend_status(Backend, false) -> @@ -258,6 +266,7 @@ on_config_update({update, Backend, _RawConfig}, Config) -> end ); Data -> + update_last_error(Backend, ?NO_ERROR), on_backend_updated( Backend, emqx_dashboard_sso:update(Provider, Config, Data#?MOD_TAB.state), diff --git a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl index 7882e17f2..048a4923a 100644 --- a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl +++ b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl @@ -64,7 +64,7 @@ end_per_testcase(Case, _) -> t_bad_create(_) -> Path = uri(["sso", "ldap"]), ?assertMatch( - {ok, 200, _}, + {ok, 400, _}, request( put, Path, @@ -168,7 +168,7 @@ t_next_login(_) -> t_bad_update(_) -> Path = uri(["sso", "ldap"]), ?assertMatch( - {ok, 200, _}, + {ok, 400, _}, request( put, Path,