Merge pull request #6009 from tigercl/fix/authn

fix(authn): fix handling of query result

apps/emqx/src/emqx_authentication.erl

@@ -495,7 +495,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}),

apps/emqx/test/emqx_authentication_SUITE.erl

@@ -138,11 +138,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)),

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.

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, <<Salt/binary, Password/binary>>);
 hash(Algorithm, Password, Salt, suffix) ->
     emqx_passwd:hash(Algorithm, <<Password/binary, Salt/binary>>).
+
+to_list(L) when is_list(L) -> L;
+to_list(L) when is_binary(L) -> binary_to_list(L);
+to_list(X) -> X.

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} ->
+        {ok, Columns, [Row | _]} ->
-            Selected = maps:from_list(lists:zip(Columns, Rows)),
+            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)};

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.
 
@@ -106,10 +115,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, erlang:tuple_to_list(Row))),
-            Selected = maps:from_list(lists:zip(NColumns, NRows)),
             case emqx_authn_utils:check_password(Password, Selected, State) of
                 ok ->
                     {ok, emqx_authn_utils:is_superuser(Selected)};
@@ -138,7 +146,7 @@ parse_query(Query) ->
             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 ->

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}).
+

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}).
+