From 7311132d499b248b5855aec9620bea1148432c8f Mon Sep 17 00:00:00 2001 From: zhouzb Date: Wed, 27 Oct 2021 09:22:17 +0800 Subject: [PATCH 1/7] fix(authn): fix handling of query result --- apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl | 4 ++-- apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl index 98d515310..d8cc7618a 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl @@ -117,8 +117,8 @@ authenticate(#{password := Password} = Credential, Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), case emqx_resource:query(Unique, {sql, Query, Params, Timeout}) of {ok, _Columns, []} -> ignore; - {ok, Columns, Rows} -> - Selected = maps:from_list(lists:zip(Columns, Rows)), + {ok, Columns, [Row | _]} -> + Selected = maps:from_list(lists:zip(Columns, Row)), case emqx_authn_utils:check_password(Password, Selected, State) of ok -> {ok, emqx_authn_utils:is_superuser(Selected)}; diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl index d1390697a..89c489d6d 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -106,10 +106,9 @@ authenticate(#{password := Password} = Credential, Params = emqx_authn_utils:replace_placeholders(PlaceHolders, Credential), case emqx_resource:query(Unique, {sql, Query, Params}) of {ok, _Columns, []} -> ignore; - {ok, Columns, Rows} -> + {ok, Columns, [Row | _]} -> NColumns = [Name || #column{name = Name} <- Columns], - NRows = [erlang:element(1, Row) || Row <- Rows], - Selected = maps:from_list(lists:zip(NColumns, NRows)), + Selected = maps:from_list(lists:zip(NColumns, erlang:tuple_to_list(Row))), case emqx_authn_utils:check_password(Password, Selected, State) of ok -> {ok, emqx_authn_utils:is_superuser(Selected)}; @@ -135,7 +134,7 @@ destroy(#{'_unique' := Unique}) -> parse_query(Query) -> case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of {match, Captured} -> - PlaceHolders = [PlaceHolder || [PlaceHolder] <- Captured], + PlaceHolders = [PlaceHolder || ["\\" ++ PlaceHolder] <- Captured], Replacements = ["$" ++ integer_to_list(I) || I <- lists:seq(1, length(Captured))], NQuery = lists:foldl(fun({PlaceHolder, Replacement}, Query0) -> re:replace(Query0, PlaceHolder, Replacement, [{return, binary}]) From a712daaebc622cd13c60786f3a348311bf5e816e Mon Sep 17 00:00:00 2001 From: zhouzb Date: Wed, 27 Oct 2021 15:08:02 +0800 Subject: [PATCH 2/7] fix(authn): fix bad list comprehension --- apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl index 89c489d6d..57a8416df 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -134,7 +134,7 @@ destroy(#{'_unique' := Unique}) -> parse_query(Query) -> case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of {match, Captured} -> - PlaceHolders = [PlaceHolder || ["\\" ++ PlaceHolder] <- Captured], + PlaceHolders = ["\\" ++ PlaceHolder || [PlaceHolder] <- Captured], Replacements = ["$" ++ integer_to_list(I) || I <- lists:seq(1, length(Captured))], NQuery = lists:foldl(fun({PlaceHolder, Replacement}, Query0) -> re:replace(Query0, PlaceHolder, Replacement, [{return, binary}]) From 966348db059ef72a8ac4028f0abdd54b15752984 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Fri, 29 Oct 2021 10:12:29 +0800 Subject: [PATCH 3/7] fix(authn): fix version switching error when updating multiple times --- apps/emqx/src/emqx_authentication.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/emqx/src/emqx_authentication.erl b/apps/emqx/src/emqx_authentication.erl index 226d697d8..765437163 100644 --- a/apps/emqx/src/emqx_authentication.erl +++ b/apps/emqx/src/emqx_authentication.erl @@ -489,7 +489,7 @@ handle_call({update_authenticator, ChainName, AuthenticatorID, Config}, _From, S Unique = unique(ChainName, AuthenticatorID, Version), case Provider:update(Config#{'_unique' => Unique}, ST) of {ok, NewST} -> - NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST), + NewAuthenticator = Authenticator#authenticator{state = switch_version(NewST#{version => Version}), enable = maps:get(enable, Config)}, NewAuthenticators = replace_authenticator(AuthenticatorID, NewAuthenticator, Authenticators), true = ets:insert(?CHAINS_TAB, Chain#chain{authenticators = NewAuthenticators}), From c64637ca39bf87264bfd8e1a65037ee9fee8b6c4 Mon Sep 17 00:00:00 2001 From: zhouzb Date: Fri, 29 Oct 2021 14:18:25 +0800 Subject: [PATCH 4/7] test(authn): add test case of version checking --- apps/emqx/test/emqx_authentication_SUITE.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/emqx/test/emqx_authentication_SUITE.erl b/apps/emqx/test/emqx_authentication_SUITE.erl index 62224a87f..8d7201a74 100644 --- a/apps/emqx/test/emqx_authentication_SUITE.erl +++ b/apps/emqx/test/emqx_authentication_SUITE.erl @@ -136,11 +136,11 @@ t_authenticator(Config) when is_list(Config) -> ID1 = <<"password-based:built-in-database">>, % CRUD of authencaticator - ?assertMatch({ok, #{id := ID1, state := #{mark := 1}}}, ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1)), + ?assertMatch({ok, #{id := ID1, state := #{mark := 1, version := <<"2">>}}}, ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1)), ?assertMatch({ok, #{id := ID1}}, ?AUTHN:lookup_authenticator(ChainName, ID1)), ?assertMatch({ok, [#{id := ID1}]}, ?AUTHN:list_authenticators(ChainName)), ?assertEqual({error, {already_exists, {authenticator, ID1}}}, ?AUTHN:create_authenticator(ChainName, AuthenticatorConfig1)), - ?assertMatch({ok, #{id := ID1, state := #{mark := 2}}}, ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)), + ?assertMatch({ok, #{id := ID1, state := #{mark := 2, version := <<"1">>}}}, ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)), ?assertEqual(ok, ?AUTHN:delete_authenticator(ChainName, ID1)), ?assertEqual({error, {not_found, {authenticator, ID1}}}, ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)), ?assertMatch({ok, []}, ?AUTHN:list_authenticators(ChainName)), From 29fb9b33618e85c825bf10cd400add09fdf364fb Mon Sep 17 00:00:00 2001 From: zhouzb Date: Mon, 1 Nov 2021 18:49:13 +0800 Subject: [PATCH 5/7] fix(authn): fix bad type of hash --- apps/emqx_authn/src/emqx_authn_utils.erl | 6 +++++- apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/apps/emqx_authn/src/emqx_authn_utils.erl b/apps/emqx_authn/src/emqx_authn_utils.erl index 4784c91c7..4aa7f550e 100644 --- a/apps/emqx_authn/src/emqx_authn_utils.erl +++ b/apps/emqx_authn/src/emqx_authn_utils.erl @@ -62,7 +62,7 @@ check_password(undefined, _Selected, _State) -> check_password(Password, #{<<"password_hash">> := Hash}, #{password_hash_algorithm := bcrypt}) -> - case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of + case {ok, to_list(Hash)} =:= bcrypt:hashpw(Password, Hash) of true -> ok; false -> {error, bad_username_or_password} end; @@ -100,3 +100,7 @@ convert_to_sql_param(undefined) -> null; convert_to_sql_param(V) -> bin(V). + +to_list(L) when is_list(L) -> L; +to_list(L) when is_binary(L) -> binary_to_list(L); +to_list(X) -> X. diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl index ce5d3d8ee..7ce3f46d0 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl @@ -205,7 +205,7 @@ check_password(Password, undefined -> {error, {cannot_find_password_hash_field, PasswordHashField}}; Hash -> - case {ok, Hash} =:= bcrypt:hashpw(Password, Hash) of + case {ok, to_list(Hash)} =:= bcrypt:hashpw(Password, Hash) of true -> ok; false -> {error, bad_username_or_password} end @@ -238,3 +238,7 @@ hash(Algorithm, Password, Salt, prefix) -> emqx_passwd:hash(Algorithm, <>); hash(Algorithm, Password, Salt, suffix) -> emqx_passwd:hash(Algorithm, <>). + +to_list(L) when is_list(L) -> L; +to_list(L) when is_binary(L) -> binary_to_list(L); +to_list(X) -> X. From ca4bb100ec8b9a99b3595b939f7d45c8e258121c Mon Sep 17 00:00:00 2001 From: zhouzb Date: Thu, 4 Nov 2021 10:01:54 +0800 Subject: [PATCH 6/7] fix(authn): fix bad parsing for postgresql SQL --- .../src/simple_authn/emqx_authn_pgsql.erl | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl index 57a8416df..fa7765542 100644 --- a/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl +++ b/apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl @@ -36,6 +36,11 @@ , destroy/1 ]). +-ifdef(TEST). +-compile(export_all). +-compile(nowarn_export_all). +-endif. + %%------------------------------------------------------------------------------ %% Hocon Schema %%------------------------------------------------------------------------------ @@ -48,7 +53,7 @@ fields(config) -> [ {mechanism, {enum, ['password-based']}} , {backend, {enum, [postgresql]}} , {password_hash_algorithm, fun password_hash_algorithm/1} - , {salt_position, {enum, [prefix, suffix]}} + , {salt_position, fun salt_position/1} , {query, fun query/1} ] ++ emqx_authn_schema:common_fields() ++ emqx_connector_schema_lib:relational_db_fields() @@ -58,6 +63,10 @@ password_hash_algorithm(type) -> {enum, [plain, md5, sha, sha256, sha512, bcrypt password_hash_algorithm(default) -> sha256; password_hash_algorithm(_) -> undefined. +salt_position(type) -> {enum, [prefix, suffix]}; +salt_position(default) -> prefix; +salt_position(_) -> undefined. + query(type) -> string(); query(_) -> undefined. @@ -134,10 +143,10 @@ destroy(#{'_unique' := Unique}) -> parse_query(Query) -> case re:run(Query, ?RE_PLACEHOLDER, [global, {capture, all, binary}]) of {match, Captured} -> - PlaceHolders = ["\\" ++ PlaceHolder || [PlaceHolder] <- Captured], + PlaceHolders = [PlaceHolder || [PlaceHolder] <- Captured], Replacements = ["$" ++ integer_to_list(I) || I <- lists:seq(1, length(Captured))], NQuery = lists:foldl(fun({PlaceHolder, Replacement}, Query0) -> - re:replace(Query0, PlaceHolder, Replacement, [{return, binary}]) + re:replace(Query0, "\\" ++ PlaceHolder, Replacement, [{return, binary}]) end, Query, lists:zip(PlaceHolders, Replacements)), {NQuery, PlaceHolders}; nomatch -> From 48ddd056b5270b8134b3c5f74e7acb91c97d556b Mon Sep 17 00:00:00 2001 From: zhouzb Date: Thu, 4 Nov 2021 10:03:34 +0800 Subject: [PATCH 7/7] test(authn): add test cases for authn --- .../test/emqx_authn_mysql_SUITE.erl | 90 ++++++++++++++++ .../test/emqx_authn_pgsql_SUITE.erl | 101 ++++++++++++++++++ 2 files changed, 191 insertions(+) create mode 100644 apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl create mode 100644 apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl diff --git a/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl new file mode 100644 index 000000000..844f45762 --- /dev/null +++ b/apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl @@ -0,0 +1,90 @@ +%%-------------------------------------------------------------------- +%% 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_authn_mysql_SUITE). + +-compile(nowarn_export_all). +-compile(export_all). + +-include("emqx_authn.hrl"). +-include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). + +all() -> + emqx_common_test_helpers:all(?MODULE). + +groups() -> + []. + +init_per_suite(Config) -> + ok = emqx_common_test_helpers:start_apps([emqx_authn]), + Config. + +end_per_suite(_Config) -> + emqx_common_test_helpers:stop_apps([emqx_authn]), + ok. + +init_per_testcase(t_authn, Config) -> + meck:new(emqx_resource, [non_strict, passthrough, no_history, no_link]), + meck:expect(emqx_resource, create_local, fun(_, _, _) -> {ok, undefined} end), + Config; +init_per_testcase(_, Config) -> + Config. + +end_per_testcase(t_authn, _Config) -> + meck:unload(emqx_resource), + ok; +end_per_testcase(_, _Config) -> + ok. + +%%------------------------------------------------------------------------------ +%% Testcases +%%------------------------------------------------------------------------------ + +t_authn(_) -> + Password = <<"test">>, + Salt = <<"salt">>, + PasswordHash = emqx_authn_utils:hash(sha256, Password, Salt, prefix), + + Config = #{<<"mechanism">> => <<"password-based">>, + <<"backend">> => <<"mysql">>, + <<"server">> => <<"127.0.0.1:3306">>, + <<"database">> => <<"mqtt">>, + <<"query">> => <<"SELECT password_hash, salt FROM users where username = ${mqtt-username} LIMIT 1">> + }, + {ok, _} = update_config([authentication], {create_authenticator, ?GLOBAL, Config}), + + meck:expect(emqx_resource, query, + fun(_, {sql, _, [<<"good">>], _}) -> + {ok, [<<"password_hash">>, <<"salt">>], [[PasswordHash, Salt]]}; + (_, {sql, _, _, _}) -> + {error, this_is_a_fictitious_reason} + end), + + ClientInfo = #{zone => default, + listener => 'tcp:default', + protocol => mqtt, + username => <<"good">>, + password => Password}, + ?assertEqual({ok, #{is_superuser => false}}, emqx_access_control:authenticate(ClientInfo)), + + ClientInfo2 = ClientInfo#{username => <<"bad">>}, + ?assertEqual({error, not_authorized}, emqx_access_control:authenticate(ClientInfo2)), + + ?AUTHN:delete_chain(?GLOBAL). + +update_config(Path, ConfigRequest) -> + emqx:update_config(Path, ConfigRequest, #{rawconf_with_defaults => true}). + diff --git a/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl new file mode 100644 index 000000000..574a11c74 --- /dev/null +++ b/apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl @@ -0,0 +1,101 @@ +%%-------------------------------------------------------------------- +%% 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_authn_pgsql_SUITE). + +-compile(nowarn_export_all). +-compile(export_all). + +-include("emqx_authn.hrl"). +-include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). +-include_lib("epgsql/include/epgsql.hrl"). + +all() -> + emqx_common_test_helpers:all(?MODULE). + +groups() -> + []. + +init_per_suite(Config) -> + ok = emqx_common_test_helpers:start_apps([emqx_authn]), + Config. + +end_per_suite(_Config) -> + emqx_common_test_helpers:stop_apps([emqx_authn]), + ok. + +init_per_testcase(t_authn, Config) -> + meck:new(emqx_resource, [non_strict, passthrough, no_history, no_link]), + meck:expect(emqx_resource, create_local, fun(_, _, _) -> {ok, undefined} end), + Config; +init_per_testcase(_, Config) -> + Config. + +end_per_testcase(t_authn, _Config) -> + meck:unload(emqx_resource), + ok; +end_per_testcase(_, _Config) -> + ok. + +%%------------------------------------------------------------------------------ +%% Testcases +%%------------------------------------------------------------------------------ + +t_parse_query(_) -> + Query1 = <<"${mqtt-username}">>, + ?assertEqual({<<"$1">>, [<<"${mqtt-username}">>]}, emqx_authn_pgsql:parse_query(Query1)), + + Query2 = <<"${mqtt-username}, ${mqtt-clientid}">>, + ?assertEqual({<<"$1, $2">>, [<<"${mqtt-username}">>, <<"${mqtt-clientid}">>]}, emqx_authn_pgsql:parse_query(Query2)), + + Query3 = <<"nomatch">>, + ?assertEqual({<<"nomatch">>, []}, emqx_authn_pgsql:parse_query(Query3)). + +t_authn(_) -> + Password = <<"test">>, + Salt = <<"salt">>, + PasswordHash = emqx_authn_utils:hash(sha256, Password, Salt, prefix), + + Config = #{<<"mechanism">> => <<"password-based">>, + <<"backend">> => <<"postgresql">>, + <<"server">> => <<"127.0.0.1:5432">>, + <<"database">> => <<"mqtt">>, + <<"query">> => <<"SELECT password_hash, salt FROM users where username = ${mqtt-username} LIMIT 1">> + }, + {ok, _} = update_config([authentication], {create_authenticator, ?GLOBAL, Config}), + + meck:expect(emqx_resource, query, + fun(_, {sql, _, [<<"good">>]}) -> + {ok, [#column{name = <<"password_hash">>}, #column{name = <<"salt">>}], [{PasswordHash, Salt}]}; + (_, {sql, _, _}) -> + {error, this_is_a_fictitious_reason} + end), + + ClientInfo = #{zone => default, + listener => 'tcp:default', + protocol => mqtt, + username => <<"good">>, + password => Password}, + ?assertEqual({ok, #{is_superuser => false}}, emqx_access_control:authenticate(ClientInfo)), + + ClientInfo2 = ClientInfo#{username => <<"bad">>}, + ?assertEqual({error, not_authorized}, emqx_access_control:authenticate(ClientInfo2)), + + ?AUTHN:delete_chain(?GLOBAL). + +update_config(Path, ConfigRequest) -> + emqx:update_config(Path, ConfigRequest, #{rawconf_with_defaults => true}). +