From 0aeb2cd77ff33088e2c2a90de3fc5c8f98dfc0c4 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Fri, 10 May 2024 12:27:04 +0200 Subject: [PATCH 1/4] fix: listener crash if access_rules config option is incorrect Previously when changing the access_rules configuration option of a listener to something that was not a valid rule, the listener would crash. This has now been fixed by the addition of a configuration validator that checks the access_rules field. Additionally, a configuration option converter has been added to the access_rules field so that one can specify several rules in a single string by using "," (comma) as separator. Fixes: https://emqx.atlassian.net/browse/EMQX-12315 --- apps/emqx/src/emqx_schema.erl | 46 ++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index b4ba08bbc..9b3450a83 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -1781,7 +1781,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", @@ -1802,6 +1804,48 @@ mqtt_listener(Bind) -> )} ] ++ emqx_schema_hooks:injection_point('mqtt.listener'). +access_rules_converter(AccessRules) -> + DeepRules = + lists:foldr( + fun(Rule, Acc) -> + Rules = re:split(Rule, <<"\\s*,\\s*">>, [{return, binary}]), + [Rules | 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) + end, + false + catch + _:_ -> true + end. + base_listener(Bind) -> [ {"enable", From 2bed5894e301a464398feefe601a37e4472ed3a1 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Fri, 10 May 2024 14:21:43 +0200 Subject: [PATCH 2/4] test: add test cases for listeners access_rules validation and split --- apps/emqx/src/emqx_schema.erl | 5 +- .../emqx/test/emqx_listeners_update_SUITE.erl | 62 +++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 9b3450a83..62287697d 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -1808,8 +1808,9 @@ access_rules_converter(AccessRules) -> DeepRules = lists:foldr( fun(Rule, Acc) -> - Rules = re:split(Rule, <<"\\s*,\\s*">>, [{return, binary}]), - [Rules | Acc] + Rules0 = re:split(Rule, <<"\\s*,\\s*">>, [{return, binary}]), + Rules1 = [string:trim(R) || R <- Rules0], + [Rules1 | Acc] end, [], AccessRules 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), From 0d053a6897bdab01dbbfd25eabcfb3ba241fb2e0 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Fri, 10 May 2024 14:29:35 +0200 Subject: [PATCH 3/4] docs: add change log entry --- changes/ce/fix-13012.en.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/ce/fix-13012.en.md 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"). From b13fa37771daf22880fe2dd91d08532857f6e2c0 Mon Sep 17 00:00:00 2001 From: Kjell Winblad Date: Fri, 10 May 2024 15:51:28 +0200 Subject: [PATCH 4/4] fix: dialyzer warning --- apps/emqx/src/emqx_schema.erl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/emqx/src/emqx_schema.erl b/apps/emqx/src/emqx_schema.erl index 62287697d..3706fdfcf 100644 --- a/apps/emqx/src/emqx_schema.erl +++ b/apps/emqx/src/emqx_schema.erl @@ -1840,7 +1840,8 @@ is_invalid_rule(S) -> ok; _ -> %% should not crash - _ = esockd_cidr:parse(CIDR, true) + _ = esockd_cidr:parse(CIDR, true), + ok end, false catch