fix(ldap): do not allow multi-matches to proceed

if ldap query returns more than on match
we should reject the auth request instead of picking
the first one

apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl

@@ -124,7 +124,7 @@ login(
     of
         {ok, []} ->
             {error, user_not_found};
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             case
                 emqx_resource:simple_sync_query(
                     ResourceId,

apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl

@@ -13,8 +13,12 @@
 
 -define(LDAP_HOST, "ldap").
 -define(LDAP_DEFAULT_PORT, 389).
--define(LDAP_USER, <<"mqttuser0001">>).
+-define(LDAP_USER, <<"viewer1">>).
--define(LDAP_USER_PASSWORD, <<"mqttuser0001">>).
+-define(LDAP_USER_PASSWORD, <<"viewer1">>).
+-define(LDAP_BASE_DN, <<"ou=dashboard,dc=emqx,dc=io">>).
+-define(LDAP_FILTER_WITH_UID, <<"(uid=${username})">>).
+%% there are more than one users in this group
+-define(LDAP_FILTER_WITH_GROUP, <<"(ugroup=group1)">>).
 
 -define(MOD_TAB, emqx_dashboard_sso).
 -define(MOD_KEY_PATH, [dashboard, sso, ldap]).
@@ -22,6 +26,7 @@
 
 -import(emqx_mgmt_api_test_util, [request/2, request/3, uri/1, request_api/3]).
 
+%% order matters
 all() ->
     [
         t_bad_create,
@@ -31,6 +36,7 @@ all() ->
         t_login_with_bad,
         t_first_login,
         t_next_login,
+        t_more_than_one_user_matched,
         t_bad_update,
         t_delete
     ].
@@ -46,12 +52,13 @@ end_per_suite(_Config) ->
     [emqx_dashboard_admin:remove_user(Name) || #{username := Name} <- All],
     emqx_mgmt_api_test_util:end_suite([emqx_conf, emqx_dashboard_sso]).
 
-init_per_testcase(_, Config) ->
+init_per_testcase(Case, Config) ->
     {ok, _} = emqx_cluster_rpc:start_link(),
+    ?MODULE:Case({init, Config}),
     Config.
 
-end_per_testcase(Case, _) ->
+end_per_testcase(Case, Config) ->
-    Case =:= t_delete_backend andalso emqx_dashboard_sso_manager:delete(ldap),
+    ?MODULE:Case({'end', Config}),
     case erlang:whereis(node()) of
         undefined ->
             ok;
@@ -61,6 +68,10 @@ end_per_testcase(Case, _) ->
     end,
     ok.
 
+t_bad_create({init, Config}) ->
+    Config;
+t_bad_create({'end', _}) ->
+    ok;
 t_bad_create(_) ->
     Path = uri(["sso", "ldap"]),
     ?assertMatch(
@@ -90,6 +101,10 @@ t_bad_create(_) ->
     ),
     ok.
 
+t_create({init, Config}) ->
+    Config;
+t_create({'end', _Config}) ->
+    ok;
 t_create(_) ->
     check_running([]),
     Path = uri(["sso", "ldap"]),
@@ -109,6 +124,10 @@ t_create(_) ->
     ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)),
     ok.
 
+t_update({init, Config}) ->
+    Config;
+t_update({'end', _Config}) ->
+    ok;
 t_update(_) ->
     Path = uri(["sso", "ldap"]),
     {ok, 200, Result} = request(put, Path, ldap_config(#{<<"enable">> => <<"true">>})),
@@ -118,6 +137,10 @@ t_update(_) ->
     ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)),
     ok.
 
+t_get({init, Config}) ->
+    Config;
+t_get({'end', _Config}) ->
+    ok;
 t_get(_) ->
     Path = uri(["sso", "ldap"]),
     {ok, 200, Result} = request(get, Path),
@@ -127,6 +150,10 @@ t_get(_) ->
     {ok, 400, _} = request(get, NotExists),
     ok.
 
+t_login_with_bad({init, Config}) ->
+    Config;
+t_login_with_bad({'end', _Config}) ->
+    ok;
 t_login_with_bad(_) ->
     Path = uri(["sso", "login", "ldap"]),
     Req = #{
@@ -138,6 +165,10 @@ t_login_with_bad(_) ->
     ?assertMatch(#{code := <<"BAD_USERNAME_OR_PWD">>}, decode_json(Result)),
     ok.
 
+t_first_login({init, Config}) ->
+    Config;
+t_first_login({'end', _Config}) ->
+    ok;
 t_first_login(_) ->
     Path = uri(["sso", "login", "ldap"]),
     Req = #{
@@ -154,6 +185,10 @@ t_first_login(_) ->
     ),
     ok.
 
+t_next_login({init, Config}) ->
+    Config;
+t_next_login({'end', _Config}) ->
+    ok;
 t_next_login(_) ->
     Path = uri(["sso", "login", "ldap"]),
     Req = #{
@@ -165,6 +200,38 @@ t_next_login(_) ->
     ?assertMatch(#{license := _, token := _}, decode_json(Result)),
     ok.
 
+t_more_than_one_user_matched({init, Config}) ->
+    emqx_logger:set_primary_log_level(error),
+    Config;
+t_more_than_one_user_matched({'end', _Config}) ->
+    %% restore default config
+    Path = uri(["sso", "ldap"]),
+    {ok, 200, _} = request(put, Path, ldap_config(#{<<"enable">> => true})),
+    ok;
+t_more_than_one_user_matched(_) ->
+    Path = uri(["sso", "ldap"]),
+    %% change to query with ugroup=group1
+    NewConfig = ldap_config(#{
+        <<"enable">> => true,
+        <<"base_dn">> => ?LDAP_BASE_DN,
+        <<"filter">> => ?LDAP_FILTER_WITH_GROUP
+    }),
+    ?assertMatch({ok, 200, _}, request(put, Path, NewConfig)),
+    check_running([<<"ldap">>]),
+    Path1 = uri(["sso", "login", "ldap"]),
+    Req = #{
+        <<"backend">> => <<"ldap">>,
+        <<"username">> => ?LDAP_USER,
+        <<"password">> => ?LDAP_USER_PASSWORD
+    },
+    {ok, 401, Result} = request(post, Path1, Req),
+    ?assertMatch(#{code := <<"BAD_USERNAME_OR_PWD">>}, decode_json(Result)),
+    ok.
+
+t_bad_update({init, Config}) ->
+    Config;
+t_bad_update({'end', _Config}) ->
+    ok;
 t_bad_update(_) ->
     Path = uri(["sso", "ldap"]),
     ?assertMatch(
@@ -184,9 +251,12 @@ t_bad_update(_) ->
     ?assertMatch(
         [#{backend := <<"ldap">>, enable := true, running := false, last_error := _}], get_sso()
     ),
-
     ok.
 
+t_delete({init, Config}) ->
+    Config;
+t_delete({'end', _Config}) ->
+    ok;
 t_delete(_) ->
     Path = uri(["sso", "ldap"]),
     ?assertMatch({ok, 204, _}, request(delete, Path)),
@@ -214,8 +284,8 @@ ldap_config(Override) ->
             <<"backend">> => <<"ldap">>,
             <<"enable">> => <<"false">>,
             <<"server">> => ldap_server(),
-            <<"base_dn">> => <<"uid=${username},ou=testdevice,dc=emqx,dc=io">>,
+            <<"base_dn">> => ?LDAP_BASE_DN,
-            <<"filter">> => <<"(objectClass=mqttUser)">>,
+            <<"filter">> => ?LDAP_FILTER_WITH_UID,
             <<"username">> => <<"cn=root,dc=emqx,dc=io">>,
             <<"password">> => <<"public">>,
             <<"pool_size">> => 8

apps/emqx_ldap/src/emqx_ldap.erl

@@ -262,7 +262,25 @@ do_ldap_query(
                 ldap_connector_query_return,
                 #{result => Result}
             ),
-            {ok, Result#eldap_search_result.entries};
+            case Result#eldap_search_result.entries of
+                [_] = Entry ->
+                    {ok, Entry};
+                [_ | _] = L ->
+                    %% Accept only a single exact match.
+                    %% Multiple matches likely indicate:
+                    %% 1. A misconfiguration in EMQX, allowing overly broad query conditions.
+                    %% 2. Indistinguishable entries in the LDAP database.
+                    %% Neither scenario should be allowed to proceed.
+                    Msg = "ldap_query_found_more_than_one_match",
+                    ?SLOG(
+                        error,
+                        LogMeta#{
+                            msg => "ldap_query_found_more_than_one_match",
+                            count => length(L)
+                        }
+                    ),
+                    {error, {recoverable_error, Msg}}
+            end;
         {error, 'noSuchObject'} ->
             {ok, []};
         {error, Reason} ->

apps/emqx_ldap/src/emqx_ldap_authn.erl

@@ -131,7 +131,7 @@ authenticate(
     of
         {ok, []} ->
             ignore;
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             is_enabled(Password, Entry, State);
         {error, Reason} ->
             ?TRACE_AUTHN_PROVIDER(error, "ldap_query_failed", #{

apps/emqx_ldap/src/emqx_ldap_authn_bind.erl

@@ -95,7 +95,7 @@ authenticate(
     of
         {ok, []} ->
             ignore;
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             case
                 emqx_resource:simple_sync_query(
                     ResourceId,

apps/emqx_ldap/src/emqx_ldap_authz.erl

@@ -111,11 +111,11 @@ authorize(
     case emqx_resource:simple_sync_query(ResourceID, {query, Client, Attrs, QueryTimeout}) of
         {ok, []} ->
             nomatch;
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             do_authorize(Action, Topic, Attrs, Entry);
         {error, Reason} ->
             ?SLOG(error, #{
-                msg => "query_ldap_error",
+                msg => "ldap_query_failed",
                 reason => emqx_utils:redact(Reason),
                 resource_id => ResourceID
             }),

apps/emqx_management/test/emqx_mgmt_api_test_util.erl

@@ -119,7 +119,7 @@ do_request_api(Method, Request, Opts) ->
     ReturnAll = maps:get(return_all, Opts, false),
     CompatibleMode = maps:get(compatible_mode, Opts, false),
     HttpcReqOpts = maps:get(httpc_req_opts, Opts, []),
-    ct:pal("Method: ~p, Request: ~p, Opts: ~p", [Method, Request, Opts]),
+    ct:pal("~p: ~p~nOpts: ~p", [Method, Request, Opts]),
     case httpc:request(Method, Request, [], HttpcReqOpts) of
         {error, socket_closed_remotely} ->
             {error, socket_closed_remotely};