From 0f2f5fbbe0f733caa3bf6486af65f8f9339110ee Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Sat, 14 Jan 2023 13:51:26 +0100 Subject: [PATCH] fix(authn): no exception when password is 'undefined' --- apps/emqx/src/emqx_channel.erl | 3 - apps/emqx/src/emqx_passwd.erl | 13 ++-- apps/emqx/test/emqx_frame_SUITE.erl | 36 +++++++++++ apps/emqx_authn/test/emqx_authn_SUITE.erl | 73 ++++++++++++++++++++--- changes/{ => v5.0.14}/refactor-9653.en.md | 0 changes/{ => v5.0.14}/refactor-9653.zh.md | 0 changes/v5.0.15/fix-9763.en.md | 1 + changes/v5.0.15/fix-9763.zh.md | 1 + 8 files changed, 111 insertions(+), 16 deletions(-) rename changes/{ => v5.0.14}/refactor-9653.en.md (100%) rename changes/{ => v5.0.14}/refactor-9653.zh.md (100%) create mode 100644 changes/v5.0.15/fix-9763.en.md create mode 100644 changes/v5.0.15/fix-9763.zh.md diff --git a/apps/emqx/src/emqx_channel.erl b/apps/emqx/src/emqx_channel.erl index b6be52c6e..e82adc786 100644 --- a/apps/emqx/src/emqx_channel.erl +++ b/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). %%-------------------------------------------------------------------- diff --git a/apps/emqx/src/emqx_passwd.erl b/apps/emqx/src/emqx_passwd.erl index 0d264f45f..dc940645b 100644 --- a/apps/emqx/src/emqx_passwd.erl +++ b/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). diff --git a/apps/emqx/test/emqx_frame_SUITE.erl b/apps/emqx/test/emqx_frame_SUITE.erl index 2a8d1bc39..23e8972e9 100644 --- a/apps/emqx/test/emqx_frame_SUITE.erl +++ b/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}). diff --git a/apps/emqx_authn/test/emqx_authn_SUITE.erl b/apps/emqx_authn/test/emqx_authn_SUITE.erl index 3017c3346..6b24f7231 100644 --- a/apps/emqx_authn/test/emqx_authn_SUITE.erl +++ b/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. diff --git a/changes/refactor-9653.en.md b/changes/v5.0.14/refactor-9653.en.md similarity index 100% rename from changes/refactor-9653.en.md rename to changes/v5.0.14/refactor-9653.en.md diff --git a/changes/refactor-9653.zh.md b/changes/v5.0.14/refactor-9653.zh.md similarity index 100% rename from changes/refactor-9653.zh.md rename to changes/v5.0.14/refactor-9653.zh.md diff --git a/changes/v5.0.15/fix-9763.en.md b/changes/v5.0.15/fix-9763.en.md new file mode 100644 index 000000000..8c07a3d5d --- /dev/null +++ b/changes/v5.0.15/fix-9763.en.md @@ -0,0 +1 @@ +Fix an authentication exception when password is not provided diff --git a/changes/v5.0.15/fix-9763.zh.md b/changes/v5.0.15/fix-9763.zh.md new file mode 100644 index 000000000..8548a363e --- /dev/null +++ b/changes/v5.0.15/fix-9763.zh.md @@ -0,0 +1 @@ +修复客户端没有提供密码时的一个异常