diff --git a/apps/emqx/test/emqx_common_test_helpers.erl b/apps/emqx/test/emqx_common_test_helpers.erl index 40e9ca5fc..15869fb36 100644 --- a/apps/emqx/test/emqx_common_test_helpers.erl +++ b/apps/emqx/test/emqx_common_test_helpers.erl @@ -238,6 +238,8 @@ render_and_load_app_config(App, Opts) -> end. do_render_app_config(App, Schema, ConfigFile, Opts) -> + %% copy acl_conf must run before read_schema_configs + copy_acl_conf(), Vars = mustache_vars(App, Opts), RenderedConfigFile = render_config_file(ConfigFile, Vars), read_schema_configs(Schema, RenderedConfigFile), @@ -497,6 +499,16 @@ copy_certs(emqx_conf, Dest0) -> copy_certs(_, _) -> ok. +copy_acl_conf() -> + Dest = filename:join([code:lib_dir(emqx), "etc/acl.conf"]), + case code:lib_dir(emqx_authz) of + {error, bad_name} -> + (not filelib:is_regular(Dest)) andalso file:write_file(Dest, <<"">>); + _ -> + {ok, _} = file:copy(deps_path(emqx_authz, "etc/acl.conf"), Dest) + end, + ok. + load_config(SchemaModule, Config) -> ConfigBin = case is_map(Config) of diff --git a/apps/emqx_authz/src/emqx_authz.app.src b/apps/emqx_authz/src/emqx_authz.app.src index dd0325694..4856008e6 100644 --- a/apps/emqx_authz/src/emqx_authz.app.src +++ b/apps/emqx_authz/src/emqx_authz.app.src @@ -1,7 +1,7 @@ %% -*- mode: erlang -*- {application, emqx_authz, [ {description, "An OTP application"}, - {vsn, "0.1.19"}, + {vsn, "0.1.20"}, {registered, []}, {mod, {emqx_authz_app, []}}, {applications, [ diff --git a/apps/emqx_authz/src/emqx_authz.erl b/apps/emqx_authz/src/emqx_authz.erl index 682ad7f2e..c7db65992 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,14 @@ 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(), + %% Always check if the rules are valid before writing to the file + %% If the rules are invalid, the old file will be kept + 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 +514,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 +536,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(TmpPath, Rules), + {ok, _} = emqx_authz_file:validate(TmpPath), + 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 4adada182..d6e5a3eb2 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..317395a45 100644 --- a/apps/emqx_authz/src/emqx_authz_file.erl +++ b/apps/emqx_authz/src/emqx_authz_file.erl @@ -33,13 +33,14 @@ update/1, destroy/1, authorize/4, + validate/1, read_file/1 ]). description() -> "AuthZ with static rules". -create(#{path := Path0} = Source) -> +validate(Path0) -> Path = filename(Path0), Rules = case file:consult(Path) of @@ -54,8 +55,12 @@ 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, + {ok, Rules}. + +create(#{path := Path} = Source) -> + {ok, Rules} = validate(Path), Source#{annotations => #{rules => Rules}}. update(#{path := _Path} = Source) -> 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/src/emqx_authz_schema.erl b/apps/emqx_authz/src/emqx_authz_schema.erl index a2a7c6b52..3b318c39f 100644 --- a/apps/emqx_authz/src/emqx_authz_schema.erl +++ b/apps/emqx_authz/src/emqx_authz_schema.erl @@ -78,7 +78,17 @@ fields("authorization") -> authz_fields(); fields(file) -> authz_common_fields(file) ++ - [{path, ?HOCON(string(), #{required => true, desc => ?DESC(path)})}]; + [ + {path, + ?HOCON( + string(), + #{ + required => true, + validator => fun(Path) -> element(1, emqx_authz_file:validate(Path)) end, + desc => ?DESC(path) + } + )} + ]; fields(http_get) -> authz_common_fields(http) ++ http_common_fields() ++ @@ -496,7 +506,7 @@ authz_fields() -> %% doc_lift is force a root level reference instead of nesting sub-structs extra => #{doc_lift => true}, %% it is recommended to configure authz sources from dashboard - %% hance the importance level for config is low + %% hence the importance level for config is low importance => ?IMPORTANCE_LOW } )} diff --git a/apps/emqx_authz/test/emqx_authz_SUITE.erl b/apps/emqx_authz/test/emqx_authz_SUITE.erl index 84b1d903e..9c1b7fd51 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,40 @@ 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}) -> + File = emqx_authz:acl_conf_file(), + {ok, Bin1} = file:read_file(File), + ?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)), + %% Check file content not changed if update failed + {ok, Bin2} = file:read_file(File), + ?assertEqual(Bin1, Bin2) + 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/apps/emqx_conf/test/emqx_conf_schema_tests.erl b/apps/emqx_conf/test/emqx_conf_schema_tests.erl index 79fe30293..06784e32d 100644 --- a/apps/emqx_conf/test/emqx_conf_schema_tests.erl +++ b/apps/emqx_conf/test/emqx_conf_schema_tests.erl @@ -23,6 +23,7 @@ """). array_nodes_test() -> + ensure_acl_conf(), ExpectNodes = ['emqx1@127.0.0.1', 'emqx2@127.0.0.1'], lists:foreach( fun(Nodes) -> @@ -79,6 +80,7 @@ array_nodes_test() -> ). authn_validations_test() -> + ensure_acl_conf(), BaseConf = to_bin(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"]), OKHttps = to_bin(?BASE_AUTHN_ARRAY, [post, true, <<"https://127.0.0.1:8080">>]), @@ -128,6 +130,7 @@ authn_validations_test() -> ). listeners_test() -> + ensure_acl_conf(), BaseConf = to_bin(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"]), Conf = <>, @@ -198,6 +201,7 @@ listeners_test() -> ok. doc_gen_test() -> + ensure_acl_conf(), %% the json file too large to encode. { timeout, @@ -220,3 +224,11 @@ doc_gen_test() -> to_bin(Format, Args) -> iolist_to_binary(io_lib:format(Format, Args)). + +ensure_acl_conf() -> + File = emqx_schema:naive_env_interpolation(<<"${EMQX_ETC_DIR}/acl.conf">>), + ok = filelib:ensure_dir(filename:dirname(File)), + case filelib:is_regular(File) of + true -> ok; + false -> file:write_file(File, <<"">>) + end. 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.