diff --git a/apps/emqx_authz/src/emqx_authz.erl b/apps/emqx_authz/src/emqx_authz.erl index 682ad7f2e..5fffb61d2 100644 --- a/apps/emqx_authz/src/emqx_authz.erl +++ b/apps/emqx_authz/src/emqx_authz.erl @@ -140,7 +140,12 @@ update(Cmd, Sources) -> emqx_authz_utils:update_config(?CONF_KEY_PATH, {Cmd, Sources}). pre_config_update(_, Cmd, Sources) -> - {ok, do_pre_config_update(Cmd, Sources)}. + try do_pre_config_update(Cmd, Sources) of + {error, Reason} -> {error, Reason}; + NSources -> {ok, NSources} + catch + _:Reason -> {error, Reason} + end. do_pre_config_update({?CMD_MOVE, _, _} = Cmd, Sources) -> do_move(Cmd, Sources); @@ -475,11 +480,12 @@ maybe_write_files(#{<<"type">> := <<"file">>} = Source) -> maybe_write_files(NewSource) -> maybe_write_certs(NewSource). -write_acl_file(#{<<"rules">> := Rules} = Source) -> - NRules = check_acl_file_rules(Rules), - Path = ?MODULE:acl_conf_file(), - {ok, _Filename} = write_file(Path, NRules), - maps:without([<<"rules">>], Source#{<<"path">> => Path}). +write_acl_file(#{<<"rules">> := Rules} = Source0) -> + AclPath = ?MODULE:acl_conf_file(), + ok = check_acl_file_rules(AclPath, Rules), + ok = write_file(AclPath, Rules), + Source1 = maps:remove(<<"rules">>, Source0), + maps:put(<<"path">>, AclPath, Source1). %% @doc where the acl.conf file is stored. acl_conf_file() -> @@ -506,7 +512,7 @@ write_file(Filename, Bytes) -> ok = filelib:ensure_dir(Filename), case file:write_file(Filename, Bytes) of ok -> - {ok, iolist_to_binary(Filename)}; + ok; {error, Reason} -> ?SLOG(error, #{filename => Filename, msg => "write_file_error", reason => Reason}), throw(Reason) @@ -528,6 +534,14 @@ get_source_by_type(Type, Sources) -> update_authz_chain(Actions) -> emqx_hooks:put('client.authorize', {?MODULE, authorize, [Actions]}, ?HP_AUTHZ). -check_acl_file_rules(RawRules) -> - %% TODO: make sure the bin rules checked - RawRules. +check_acl_file_rules(Path, Rules) -> + TmpPath = Path ++ ".tmp", + try + ok = write_file(Path, Rules), + #{annotations := #{rules := _}} = emqx_authz_file:create(#{path => Path}), + ok + catch + throw:Reason -> throw(Reason) + after + _ = file:delete(TmpPath) + end. diff --git a/apps/emqx_authz/src/emqx_authz_api_schema.erl b/apps/emqx_authz/src/emqx_authz_api_schema.erl index c89ac5bee..049c84713 100644 --- a/apps/emqx_authz/src/emqx_authz_api_schema.erl +++ b/apps/emqx_authz/src/emqx_authz_api_schema.erl @@ -39,8 +39,10 @@ fields(file) -> type => binary(), required => true, example => - <<"{allow,{username,\"^dashboard?\"},", "subscribe,[\"$SYS/#\"]}.\n", - "{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}.">>, + << + "{allow,{username,{re,\"^dashboard$\"}},subscribe,[\"$SYS/#\"]}.\n", + "{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}." + >>, desc => ?DESC(rules) }} ]; diff --git a/apps/emqx_authz/src/emqx_authz_file.erl b/apps/emqx_authz/src/emqx_authz_file.erl index 54f1775c6..dfd28f0c0 100644 --- a/apps/emqx_authz/src/emqx_authz_file.erl +++ b/apps/emqx_authz/src/emqx_authz_file.erl @@ -54,7 +54,7 @@ create(#{path := Path0} = Source) -> throw(failed_to_read_acl_file); {error, Reason} -> ?SLOG(alert, #{msg => bad_acl_file_content, path => Path, reason => Reason}), - throw(bad_acl_file_content) + throw({bad_acl_file_content, Reason}) end, Source#{annotations => #{rules => Rules}}. diff --git a/apps/emqx_authz/src/emqx_authz_rule.erl b/apps/emqx_authz/src/emqx_authz_rule.erl index bdd0904f7..ec1a8c5de 100644 --- a/apps/emqx_authz/src/emqx_authz_rule.erl +++ b/apps/emqx_authz/src/emqx_authz_rule.erl @@ -68,7 +68,13 @@ compile({Permission, Who, Action, TopicFilters}) when {atom(Permission), compile_who(Who), atom(Action), [ compile_topic(Topic) || Topic <- TopicFilters - ]}. + ]}; +compile({Permission, _Who, _Action, _TopicFilter}) when not ?ALLOW_DENY(Permission) -> + throw({invalid_authorization_permission, Permission}); +compile({_Permission, _Who, Action, _TopicFilter}) when not ?PUBSUB(Action) -> + throw({invalid_authorization_action, Action}); +compile(BadRule) -> + throw({invalid_authorization_rule, BadRule}). compile_who(all) -> all; diff --git a/apps/emqx_authz/test/emqx_authz_SUITE.erl b/apps/emqx_authz/test/emqx_authz_SUITE.erl index 84b1d903e..2c57d807a 100644 --- a/apps/emqx_authz/test/emqx_authz_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_SUITE.erl @@ -155,22 +155,36 @@ set_special_configs(_App) -> <<"ssl">> => #{<<"enable">> => false}, <<"cmd">> => <<"HGETALL mqtt_authz:", ?PH_USERNAME/binary>> }). --define(SOURCE6, #{ + +-define(FILE_SOURCE(Rules), #{ <<"type">> => <<"file">>, <<"enable">> => true, - <<"rules">> => + <<"rules">> => Rules +}). + +-define(SOURCE6, + ?FILE_SOURCE( << "{allow,{username,\"^dashboard?\"},subscribe,[\"$SYS/#\"]}." "\n{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}." >> -}). --define(SOURCE7, #{ + ) +). +-define(SOURCE7, + ?FILE_SOURCE( + << + "{allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}.\n" + "{deny, all}." + >> + ) +). + +-define(BAD_FILE_SOURCE2, #{ <<"type">> => <<"file">>, <<"enable">> => true, <<"rules">> => << - "{allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}.\n" - "{deny, all}." + "{not_allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}." >> }). @@ -178,6 +192,35 @@ set_special_configs(_App) -> %% Testcases %%------------------------------------------------------------------------------ +-define(UPDATE_ERROR(Err), {error, {pre_config_update, emqx_authz, Err}}). + +t_bad_file_source(_) -> + BadContent = ?FILE_SOURCE(<<"{allow,{username,\"bar\"}, publish, [\"test\"]}">>), + BadContentErr = {bad_acl_file_content, {1, erl_parse, ["syntax error before: ", []]}}, + BadRule = ?FILE_SOURCE(<<"{allow,{username,\"bar\"},publish}.">>), + BadRuleErr = {invalid_authorization_rule, {allow, {username, "bar"}, publish}}, + BadPermission = ?FILE_SOURCE(<<"{not_allow,{username,\"bar\"},publish,[\"test\"]}.">>), + BadPermissionErr = {invalid_authorization_permission, not_allow}, + BadAction = ?FILE_SOURCE(<<"{allow,{username,\"bar\"},pubsub,[\"test\"]}.">>), + BadActionErr = {invalid_authorization_action, pubsub}, + lists:foreach( + fun({Source, Error}) -> + ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_REPLACE, [Source])), + ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_PREPEND, Source)), + ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_APPEND, Source)) + end, + [ + {BadContent, BadContentErr}, + {BadRule, BadRuleErr}, + {BadPermission, BadPermissionErr}, + {BadAction, BadActionErr} + ] + ), + ?assertMatch( + [], + emqx_conf:get([authorization, sources], []) + ). + t_update_source(_) -> %% replace all {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE3]), diff --git a/apps/emqx_authz/test/emqx_authz_file_SUITE.erl b/apps/emqx_authz/test/emqx_authz_file_SUITE.erl index 124fe904f..be8907feb 100644 --- a/apps/emqx_authz/test/emqx_authz_file_SUITE.erl +++ b/apps/emqx_authz/test/emqx_authz_file_SUITE.erl @@ -120,7 +120,9 @@ t_superuser(_Config) -> t_invalid_file(_Config) -> ?assertMatch( - {error, bad_acl_file_content}, + {error, + {pre_config_update, emqx_authz, + {bad_acl_file_content, {1, erl_parse, ["syntax error before: ", "term"]}}}}, emqx_authz:update(?CMD_REPLACE, [?RAW_SOURCE#{<<"rules">> => <<"{{invalid term">>}]) ). diff --git a/changes/ce/fix-10742.en.md b/changes/ce/fix-10742.en.md new file mode 100644 index 000000000..cc8232a04 --- /dev/null +++ b/changes/ce/fix-10742.en.md @@ -0,0 +1,2 @@ +Check the correctness of the rules before saving the authorization file source. +Previously, Saving wrong rules could lead to restart failure.