Merge pull request #13012 from kjellwinblad/kjell/fix_incorrect_config_crash/EMQX-12315

fix: listener crash if access_rules config option is incorrect

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",

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),

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").