Merge pull request #6532 from zhongwencool/fix-banned-bad-peerhost-crash

fix(banned): crash by bad peerhost; add banned http API tests

apps/emqx/src/emqx_banned.erl

@@ -37,7 +37,6 @@
         , info/1
         , format/1
         , parse/1
-        , to_timestamp/1
         ]).
 
 %% gen_server callbacks
@@ -53,6 +52,11 @@
 
 -define(BANNED_TAB, ?MODULE).
 
+-ifdef(TEST).
+-compile(export_all).
+-compile(nowarn_export_all).
+-endif.
+
 %%--------------------------------------------------------------------
 %% Mnesia bootstrap
 %%--------------------------------------------------------------------
@@ -106,32 +110,36 @@ format(#banned{who = Who0,
     }.
 
 parse(Params) ->
-    Who    = pares_who(Params),
+    case pares_who(Params) of
+        {error, Reason} -> {error, Reason};
+        Who  ->
             By     = maps:get(<<"by">>, Params, <<"mgmt_api">>),
             Reason = maps:get(<<"reason">>, Params, <<"">>),
-    At     = parse_time(maps:get(<<"at">>, Params, undefined), erlang:system_time(second)),
+            At     = maps:get(<<"at">>, Params, erlang:system_time(second)),
-    Until  = parse_time(maps:get(<<"until">>, Params, undefined), At + 5 * 60),
+            Until  = maps:get(<<"until">>, Params, At + 5 * 60),
+            case Until > erlang:system_time(second) of
+                true ->
                     #banned{
                         who    = Who,
                         by     = By,
                         reason = Reason,
                         at     = At,
                         until  = Until
-    }.
+                    };
-
+                false ->
+                    {error, "already_expired"}
+            end
+    end.
 pares_who(#{as := As, who := Who}) ->
     pares_who(#{<<"as">> => As, <<"who">> => Who});
 pares_who(#{<<"as">> := peerhost, <<"who">> := Peerhost0}) ->
-    {ok, Peerhost} = inet:parse_address(binary_to_list(Peerhost0)),
+    case inet:parse_address(binary_to_list(Peerhost0)) of
-    {peerhost, Peerhost};
+        {ok, Peerhost} -> {peerhost, Peerhost};
+        {error, einval} -> {error, "bad peerhost"}
+    end;
 pares_who(#{<<"as">> := As, <<"who">> := Who}) ->
     {As, Who}.
 
-parse_time(undefined, Default) ->
-    Default;
-parse_time(Rfc3339, _Default) ->
-    to_timestamp(Rfc3339).
-
 maybe_format_host({peerhost, Host}) ->
     AddrBinary = list_to_binary(inet:ntoa(Host)),
     {peerhost, AddrBinary};
@@ -141,11 +149,6 @@ maybe_format_host({As, Who}) ->
 to_rfc3339(Timestamp) ->
     list_to_binary(calendar:system_time_to_rfc3339(Timestamp, [{unit, second}])).
 
-to_timestamp(Rfc3339) when is_binary(Rfc3339) ->
-    to_timestamp(binary_to_list(Rfc3339));
-to_timestamp(Rfc3339) ->
-    calendar:rfc3339_to_system_time(Rfc3339, [{unit, second}]).
-
 -spec(create(emqx_types:banned() | map()) ->
     {ok, emqx_types:banned()} | {error, {already_exist, emqx_types:banned()}}).
 create(#{who    := Who,
@@ -168,10 +171,11 @@ create(Banned = #banned{who = Who})  ->
             mria:dirty_write(?BANNED_TAB, Banned),
             {ok, Banned};
         [OldBanned = #banned{until = Until}] ->
-            case Until < erlang:system_time(second) of
+            %% Don't support shorten or extend the until time by overwrite.
-                true ->
+            %% We don't support update api yet, user must delete then create new one.
-                    {error, {already_exist, OldBanned}};
+            case Until > erlang:system_time(second) of
-                false ->
+                true -> {error, {already_exist, OldBanned}};
+                false -> %% overwrite expired one is ok.
                     mria:dirty_write(?BANNED_TAB, Banned),
                     {ok, Banned}
             end

apps/emqx/test/emqx_banned_SUITE.erl

@@ -39,9 +39,13 @@ t_add_delete(_) ->
                      by = <<"banned suite">>,
                      reason = <<"test">>,
                      at = erlang:system_time(second),
-                     until = erlang:system_time(second) + 1000
+                     until = erlang:system_time(second) + 1
                     },
     {ok, _} = emqx_banned:create(Banned),
+    {error, {already_exist, Banned}} = emqx_banned:create(Banned),
+    ?assertEqual(1, emqx_banned:info(size)),
+    {error, {already_exist, Banned}} =
+        emqx_banned:create(Banned#banned{until = erlang:system_time(second) + 100}),
     ?assertEqual(1, emqx_banned:info(size)),
 
     ok = emqx_banned:delete({clientid, <<"TestClient">>}),
@@ -68,10 +72,14 @@ t_check(_) ->
                     username => <<"user">>,
                     peerhost => {127,0,0,1}
                    },
+    ClientInfo5 = #{},
+    ClientInfo6 = #{clientid => <<"client1">>},
     ?assert(emqx_banned:check(ClientInfo1)),
     ?assert(emqx_banned:check(ClientInfo2)),
     ?assert(emqx_banned:check(ClientInfo3)),
     ?assertNot(emqx_banned:check(ClientInfo4)),
+    ?assertNot(emqx_banned:check(ClientInfo5)),
+    ?assertNot(emqx_banned:check(ClientInfo6)),
     ok = emqx_banned:delete({clientid, <<"BannedClient">>}),
     ok = emqx_banned:delete({username, <<"BannedUser">>}),
     ok = emqx_banned:delete({peerhost, {192,168,0,1}}),
@@ -83,8 +91,10 @@ t_check(_) ->
 
 t_unused(_) ->
     {ok, Banned} = emqx_banned:start_link(),
-    {ok, _} = emqx_banned:create(#banned{who = {clientid, <<"BannedClient">>},
+    {ok, _} = emqx_banned:create(#banned{who = {clientid, <<"BannedClient1">>},
         until = erlang:system_time(second)}),
+    {ok, _} = emqx_banned:create(#banned{who = {clientid, <<"BannedClient2">>},
+        until = erlang:system_time(second) - 1}),
     ?assertEqual(ignored, gen_server:call(Banned, unexpected_req)),
     ?assertEqual(ok, gen_server:cast(Banned, unexpected_msg)),
     ?assertEqual(ok, Banned ! ok),

apps/emqx_management/src/emqx_mgmt_api_banned.erl

@@ -105,7 +105,7 @@ fields(ban) ->
             desc => <<"Client info as banned type">>,
             nullable => false,
             example => <<"Badass坏"/utf8>>})},
-        {by, hoconsc:mk(binary(), #{
+        {by, hoconsc:mk(emqx_schema:unicode_binary(), #{
             desc => <<"Commander">>,
             nullable => true,
             example => <<"mgmt_api">>})},
@@ -113,15 +113,13 @@ fields(ban) ->
             desc => <<"Banned reason">>,
             nullable => true,
             example => <<"Too many requests">>})},
-        {at, hoconsc:mk(binary(), #{
+        {at, hoconsc:mk(emqx_schema:rfc3339_system_time(), #{
             desc => <<"Create banned time, rfc3339, now if not specified">>,
             nullable => true,
-            validator => fun is_rfc3339/1,
             example => <<"2021-10-25T21:48:47+08:00">>})},
-        {until, hoconsc:mk(binary(), #{
+        {until, hoconsc:mk(emqx_schema:rfc3339_system_time(), #{
             desc => <<"Cancel banned time, rfc3339, now + 5 minute if not specified">>,
             nullable => true,
-            validator => fun is_rfc3339/1,
             example => <<"2021-10-25T21:53:47+08:00">>})
         }
     ];
@@ -130,22 +128,19 @@ fields(meta) ->
         emqx_dashboard_swagger:fields(limit) ++
         [{count, hoconsc:mk(integer(), #{example => 1})}].
 
-is_rfc3339(Time) ->
-    try
-        emqx_banned:to_timestamp(Time),
-        ok
-    catch _:_ -> {error, Time}
-    end.
-
 banned(get, #{query_string := Params}) ->
     Response = emqx_mgmt_api:paginate(?TAB, Params, ?FORMAT_FUN),
     {200, Response};
 banned(post, #{body := Body}) ->
-    case emqx_banned:create(emqx_banned:parse(Body)) of
+    case emqx_banned:parse(Body) of
-        {ok, Banned} ->
+        {error, Reason} ->
-            {200, format(Banned)};
+            {400, #{code => 'PARAMS_ERROR', message => list_to_binary(Reason)}};
+        Ban ->
+            case emqx_banned:create(Ban) of
+                {ok, Banned} -> {200, format(Banned)};
                 {error, {already_exist, Old}} ->
                     {400, #{code => 'ALREADY_EXISTED', message => format(Old)}}
+            end
     end.
 
 delete_banned(delete, #{bindings := Params}) ->

apps/emqx_management/test/emqx_mgmt_banned_api_SUITE.erl

@@ -0,0 +1,144 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2020-2021 EMQ Technologies Co., Ltd. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%--------------------------------------------------------------------
+-module(emqx_mgmt_banned_api_SUITE).
+
+-compile(export_all).
+-compile(nowarn_export_all).
+
+-include_lib("eunit/include/eunit.hrl").
+
+all() ->
+    emqx_common_test_helpers:all(?MODULE).
+
+init_per_suite(Config) ->
+    emqx_mgmt_api_test_util:init_suite(),
+    Config.
+
+end_per_suite(_) ->
+    emqx_mgmt_api_test_util:end_suite().
+
+t_create(_Config) ->
+    Now = erlang:system_time(second),
+    At = emqx_banned:to_rfc3339(Now),
+    Until = emqx_banned:to_rfc3339(Now + 3),
+    ClientId = <<"TestClient测试"/utf8>>,
+    By = <<"banned suite测试组"/utf8>>,
+    Reason = <<"test测试"/utf8>>,
+    As = <<"clientid">>,
+    ClientIdBanned = #{
+        as => As,
+        who => ClientId,
+        by => By,
+        reason => Reason,
+        at => At,
+        until => Until
+    },
+    {ok, ClientIdBannedRes} = create_banned(ClientIdBanned),
+    ?assertEqual(#{<<"as">> => As,
+        <<"at">> => At,
+        <<"by">> => By,
+        <<"reason">> => Reason,
+        <<"until">> => Until,
+        <<"who">> => ClientId
+        }, ClientIdBannedRes),
+    PeerHost = <<"192.168.2.13">>,
+    PeerHostBanned = #{
+        as => <<"peerhost">>,
+        who => PeerHost,
+        by => By,
+        reason => Reason,
+        at => At,
+        until => Until
+    },
+    {ok, PeerHostBannedRes} = create_banned(PeerHostBanned),
+    ?assertEqual(#{<<"as">> => <<"peerhost">>,
+        <<"at">> => At,
+        <<"by">> => By,
+        <<"reason">> => Reason,
+        <<"until">> => Until,
+        <<"who">> => PeerHost
+    }, PeerHostBannedRes),
+    {ok, #{<<"data">> := List}} = list_banned(),
+    Bans = lists:sort(lists:map(fun(#{<<"who">> := W, <<"as">> := A}) ->  {A, W} end, List)),
+    ?assertEqual([{<<"clientid">>, ClientId}, {<<"peerhost">>, PeerHost}], Bans),
+    ok.
+
+t_create_failed(_Config) ->
+    Now = erlang:system_time(second),
+    At = emqx_banned:to_rfc3339(Now),
+    Until = emqx_banned:to_rfc3339(Now + 10),
+    Who = <<"BadHost"/utf8>>,
+    By = <<"banned suite测试组"/utf8>>,
+    Reason = <<"test测试"/utf8>>,
+    As = <<"peerhost">>,
+    BadPeerHost = #{
+        as => As,
+        who => Who,
+        by => By,
+        reason => Reason,
+        at => At,
+        until => Until
+    },
+    BadRequest = {error, {"HTTP/1.1", 400, "Bad Request"}},
+    ?assertEqual(BadRequest, create_banned(BadPeerHost)),
+    Expired = BadPeerHost#{until => emqx_banned:to_rfc3339(Now - 1),
+        who => <<"127.0.0.1">>},
+    ?assertEqual(BadRequest, create_banned(Expired)),
+    ok.
+
+t_delete(_Config) ->
+    Now = erlang:system_time(second),
+    At = emqx_banned:to_rfc3339(Now),
+    Until = emqx_banned:to_rfc3339(Now + 3),
+    Who = <<"TestClient-"/utf8>>,
+    By = <<"banned suite 中"/utf8>>,
+    Reason = <<"test测试"/utf8>>,
+    As = <<"clientid">>,
+    Banned = #{
+        as => clientid,
+        who => Who,
+        by => By,
+        reason => Reason,
+        at => At,
+        until => Until
+    },
+    {ok, _} = create_banned(Banned),
+    ?assertMatch({ok, _}, delete_banned(binary_to_list(As), binary_to_list(Who))),
+    ?assertMatch({error,{"HTTP/1.1",404,"Not Found"}},
+        delete_banned(binary_to_list(As), binary_to_list(Who))),
+    ok.
+
+list_banned() ->
+    Path = emqx_mgmt_api_test_util:api_path(["banned"]),
+    case emqx_mgmt_api_test_util:request_api(get, Path) of
+        {ok, Apps} -> {ok, emqx_json:decode(Apps, [return_maps])};
+        Error -> Error
+    end.
+
+create_banned(Banned) ->
+    AuthHeader = emqx_mgmt_api_test_util:auth_header_(),
+    Path = emqx_mgmt_api_test_util:api_path(["banned"]),
+    case emqx_mgmt_api_test_util:request_api(post, Path, "", AuthHeader, Banned) of
+        {ok, Res} -> {ok, emqx_json:decode(Res, [return_maps])};
+        Error -> Error
+    end.
+
+delete_banned(As, Who) ->
+    DeletePath = emqx_mgmt_api_test_util:api_path(["banned", As, Who]),
+    emqx_mgmt_api_test_util:request_api(delete, DeletePath).
+
+to_rfc3339(Sec) ->
+    list_to_binary(calendar:system_time_to_rfc3339(Sec)).