Merge pull request #9763 from zmstone/0114-fix-crash-when-password-undefined

0114 fix crash when password undefined

apps/emqx/src/emqx_channel.erl

@@ -1427,7 +1427,6 @@ interval(will_timer, #channel{will_msg = WillMsg}) ->
 
 -spec terminate(any(), channel()) -> ok.
 terminate(_, #channel{conn_state = idle} = _Channel) ->
-    ?tp(channel_terminated, #{channel => _Channel}),
     ok;
 terminate(normal, Channel) ->
     run_terminate_hook(normal, Channel);
@@ -1460,10 +1459,8 @@ persist_if_session(#channel{session = Session} = Channel) ->
     end.
 
 run_terminate_hook(_Reason, #channel{session = undefined} = _Channel) ->
-    ?tp(channel_terminated, #{channel => _Channel}),
     ok;
 run_terminate_hook(Reason, #channel{clientinfo = ClientInfo, session = Session} = _Channel) ->
-    ?tp(channel_terminated, #{channel => _Channel}),
     emqx_session:terminate(ClientInfo, Reason, Session).
 
 %%--------------------------------------------------------------------

apps/emqx/src/emqx_passwd.erl

@@ -57,22 +57,27 @@
 %% APIs
 %%--------------------------------------------------------------------
 
--spec check_pass(hash_params(), password_hash(), password()) -> boolean().
-check_pass({pbkdf2, MacFun, Salt, Iterations, DKLength}, PasswordHash, Password) ->
+-spec check_pass(hash_params(), password_hash(), password() | undefined) -> boolean().
+check_pass(_Algo, _Hash, undefined) ->
+    false;
+check_pass(Algo, Hash, Password) ->
+    do_check_pass(Algo, Hash, Password).
+
+do_check_pass({pbkdf2, MacFun, Salt, Iterations, DKLength}, PasswordHash, Password) ->
     case pbkdf2(MacFun, Password, Salt, Iterations, DKLength) of
         {ok, HashPasswd} ->
             compare_secure(hex(HashPasswd), PasswordHash);
         {error, _Reason} ->
             false
     end;
-check_pass({bcrypt, Salt}, PasswordHash, Password) ->
+do_check_pass({bcrypt, Salt}, PasswordHash, Password) ->
     case bcrypt:hashpw(Password, Salt) of
         {ok, HashPasswd} ->
             compare_secure(list_to_binary(HashPasswd), PasswordHash);
         {error, _Reason} ->
             false
     end;
-check_pass({_SimpleHash, _Salt, _SaltPosition} = HashParams, PasswordHash, Password) ->
+do_check_pass({_SimpleHash, _Salt, _SaltPosition} = HashParams, PasswordHash, Password) ->
     Hash = hash(HashParams, Password),
     compare_secure(Hash, PasswordHash).
 

apps/emqx/test/emqx_frame_SUITE.erl

@@ -670,6 +670,42 @@ t_invalid_clientid(_) ->
         emqx_frame:parse(<<16, 15, 0, 6, 77, 81, 73, 115, 100, 112, 3, 0, 0, 0, 1, 0, 0>>)
     ).
 
+%% for regression: `password` must be `undefined`
+t_undefined_password(_) ->
+    Payload = <<16, 19, 0, 4, 77, 81, 84, 84, 4, 130, 0, 60, 0, 2, 97, 49, 0, 3, 97, 97, 97>>,
+    {ok, Packet, <<>>, {none, _}} = emqx_frame:parse(Payload),
+    Password = undefined,
+    ?assertEqual(
+        #mqtt_packet{
+            header = #mqtt_packet_header{
+                type = 1,
+                dup = false,
+                qos = 0,
+                retain = false
+            },
+            variable = #mqtt_packet_connect{
+                proto_name = <<"MQTT">>,
+                proto_ver = 4,
+                is_bridge = false,
+                clean_start = true,
+                will_flag = false,
+                will_qos = 0,
+                will_retain = false,
+                keepalive = 60,
+                properties = #{},
+                clientid = <<"a1">>,
+                will_props = #{},
+                will_topic = undefined,
+                will_payload = undefined,
+                username = <<"aaa">>,
+                password = Password
+            },
+            payload = undefined
+        },
+        Packet
+    ),
+    ok.
+
 parse_serialize(Packet) ->
     parse_serialize(Packet, #{strict_mode => true}).
 

apps/emqx_authn/test/emqx_authn_SUITE.erl

@@ -97,21 +97,76 @@ t_will_message_connection_denied(Config) when is_list(Config) ->
         {will_topic, <<"lwt">>},
         {will_payload, <<"should not be published">>}
     ]),
-    snabbkaffe:start_trace(),
-    ?wait_async_action(
-        {error, _} = emqtt:connect(Publisher),
-        #{?snk_kind := channel_terminated}
-    ),
-    snabbkaffe:stop(),
-
+    Ref = monitor(process, Publisher),
+    {error, _} = emqtt:connect(Publisher),
+    receive
+        {'DOWN', Ref, process, Publisher, Reason} ->
+            ?assertEqual({shutdown, unauthorized_client}, Reason)
+    after 2000 ->
+        error(timeout)
+    end,
     receive
         {publish, #{
             topic := <<"lwt">>,
             payload := <<"should not be published">>
         }} ->
             ct:fail("should not publish will message")
-    after 0 ->
+    after 1000 ->
         ok
     end,
-
     ok.
+
+%% With auth enabled, send CONNECT without password field,
+%% expect CONNACK with reason_code=5 and socket close
+t_password_undefined({init, Config}) ->
+    emqx_common_test_helpers:start_apps([emqx_conf, emqx_authn]),
+    AuthnConfig = #{
+        <<"mechanism">> => <<"password_based">>,
+        <<"backend">> => <<"built_in_database">>,
+        <<"user_id_type">> => <<"clientid">>
+    },
+    Chain = 'mqtt:global',
+    emqx:update_config(
+        [authentication],
+        {create_authenticator, Chain, AuthnConfig}
+    ),
+    Config;
+t_password_undefined({'end', _Config}) ->
+    emqx:update_config(
+        [authentication],
+        {delete_authenticator, 'mqtt:global', <<"password_based:built_in_database">>}
+    ),
+    emqx_common_test_helpers:stop_apps([emqx_authn, emqx_conf]),
+    ok;
+t_password_undefined(Config) when is_list(Config) ->
+    Payload = <<16, 19, 0, 4, 77, 81, 84, 84, 4, 130, 0, 60, 0, 2, 97, 49, 0, 3, 97, 97, 97>>,
+    {ok, Sock} = gen_tcp:connect("localhost", 1883, [binary, {active, true}]),
+    gen_tcp:send(Sock, Payload),
+    receive
+        {tcp, Sock, Bytes} ->
+            Resp = parse(iolist_to_binary(Bytes)),
+            ?assertMatch(
+                #mqtt_packet{
+                    header = #mqtt_packet_header{type = ?CONNACK},
+                    variable = #mqtt_packet_connack{
+                        ack_flags = 0,
+                        reason_code = ?CONNACK_AUTH
+                    },
+                    payload = undefined
+                },
+                Resp
+            )
+    after 2000 ->
+        error(timeout)
+    end,
+    receive
+        {tcp_closed, Sock} ->
+            ok
+    after 2000 ->
+        error(timeout)
+    end,
+    ok.
+
+parse(Bytes) ->
+    {ok, Frame, <<>>, {none, _}} = emqx_frame:parse(Bytes),
+    Frame.

changes/v5.0.15/fix-9763.en.md

@@ -0,0 +1 @@
+Fix an authentication exception when password is not provided

changes/v5.0.15/fix-9763.zh.md

@@ -0,0 +1 @@
+修复客户端没有提供密码时的一个异常