diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 7f2839f35..4727ae3c8 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -1789,7 +1789,9 @@ mqtt_listener(Bind) -> hoconsc:array(string()), #{ desc => ?DESC(mqtt_listener_access_rules), - default => [<<"allow all">>] + default => [<<"allow all">>], + converter => fun access_rules_converter/1, + validator => fun access_rules_validator/1 } )}, {"proxy_protocol", @@ -1810,6 +1812,50 @@ mqtt_listener(Bind) -> )} ] ++ emqx_schema_hooks:injection_point('mqtt.listener'). +access_rules_converter(AccessRules) -> + DeepRules = + lists:foldr( + fun(Rule, Acc) -> + Rules0 = re:split(Rule, <<"\\s*,\\s*">>, [{return, binary}]), + Rules1 = [string:trim(R) || R <- Rules0], + [Rules1 | Acc] + end, + [], + AccessRules + ), + [unicode:characters_to_list(RuleBin) || RuleBin <- lists:flatten(DeepRules)]. + +access_rules_validator(AccessRules) -> + InvalidRules = [Rule || Rule <- AccessRules, is_invalid_rule(Rule)], + case InvalidRules of + [] -> + ok; + _ -> + MsgStr = io_lib:format("invalid_rule(s): ~ts", [string:join(InvalidRules, ", ")]), + MsgBin = unicode:characters_to_binary(MsgStr), + {error, MsgBin} + end. + +is_invalid_rule(S) -> + try + [Action, CIDR] = string:tokens(S, " "), + case Action of + "allow" -> ok; + "deny" -> ok + end, + case CIDR of + "all" -> + ok; + _ -> + %% should not crash + _ = esockd_cidr:parse(CIDR, true), + ok + end, + false + catch + _:_ -> true + end. + base_listener(Bind) -> [ {"enable", diff --git a/apps/emqx/test/emqx_listeners_update_SUITE.erl b/apps/emqx/test/emqx_listeners_update_SUITE.erl index 8c72b853e..824d5fd2f 100644 --- a/apps/emqx/test/emqx_listeners_update_SUITE.erl +++ b/apps/emqx/test/emqx_listeners_update_SUITE.erl @@ -115,6 +115,68 @@ t_update_conf(_Conf) -> ?assert(is_running('wss:default')), ok. +t_update_conf_validate_access_rules(_Conf) -> + Raw = emqx:get_raw_config(?LISTENERS), + RawCorrectConf1 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], Raw, ["allow all"] + ), + ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, RawCorrectConf1)), + RawCorrectConf2 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], Raw, ["deny all"] + ), + ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, RawCorrectConf2)), + RawCorrectConf3 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], Raw, ["allow 10.0.1.0/24"] + ), + ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, RawCorrectConf3)), + RawIncorrectConf1 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], Raw, ["xxx all"] + ), + ?assertMatch( + {error, #{ + reason := <<"invalid_rule(s): xxx all">>, + value := ["xxx all"], + path := "listeners.tcp.default.access_rules", + kind := validation_error, + matched_type := "emqx:mqtt_tcp_listener" + }}, + emqx:update_config(?LISTENERS, RawIncorrectConf1) + ), + RawIncorrectConf2 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], Raw, ["allow xxx"] + ), + ?assertMatch( + {error, #{ + reason := <<"invalid_rule(s): allow xxx">>, + value := ["allow xxx"], + path := "listeners.tcp.default.access_rules", + kind := validation_error, + matched_type := "emqx:mqtt_tcp_listener" + }}, + emqx:update_config(?LISTENERS, RawIncorrectConf2) + ), + ok. + +t_update_conf_access_rules_split(_Conf) -> + Raw = emqx:get_raw_config(?LISTENERS), + Raw1 = emqx_utils_maps:deep_put( + [<<"tcp">>, <<"default">>, <<"access_rules">>], + Raw, + [" allow all , deny all , allow 10.0.1.0/24 "] + ), + ?assertMatch({ok, _}, emqx:update_config(?LISTENERS, Raw1)), + ?assertMatch( + #{ + tcp := #{ + default := #{ + access_rules := ["allow all", "deny all", "allow 10.0.1.0/24"] + } + } + }, + emqx:get_config(?LISTENERS) + ), + ok. + t_update_tcp_keepalive_conf(_Conf) -> Keepalive = <<"240,30,5">>, KeepaliveStr = binary_to_list(Keepalive), diff --git a/changes/ce/fix-13012.en.md b/changes/ce/fix-13012.en.md new file mode 100644 index 000000000..ef87837ee --- /dev/null +++ b/changes/ce/fix-13012.en.md @@ -0,0 +1,4 @@ +The MQTT listerners config option `access_rules` has been improved in the following ways: + +* The listener no longer crash with an incomprehensible error message if a non-valid access rule is configured. Instead a configuration error is generated. +* One can now add several rules in a single string by separating them by comma (for example, "allow 10.0.1.0/24, deny all").