Merge pull request #11706 from zmstone/0928-fix-ldap-sso-logging-level

0928 fix ldap sso logging level and reject muti-match results

.ci/docker-compose-file/docker-compose-ldap.yaml

@@ -6,8 +6,6 @@ services:
     build:
       context: ../..
       dockerfile: .ci/docker-compose-file/openldap/Dockerfile
-      args:
-        LDAP_TAG: ${LDAP_TAG}
     image: openldap
     #ports:
     #  - 389:389

.ci/docker-compose-file/openldap/Dockerfile

@@ -1,13 +1,4 @@
-FROM buildpack-deps:bookworm
+FROM docker.io/zmstone/openldap:2.5.16
-
-ARG LDAP_TAG=2.5.16
-
-RUN apt-get update && apt-get install -y groff groff-base
-RUN wget https://www.openldap.org/software/download/OpenLDAP/openldap-release/openldap-${LDAP_TAG}.tgz \
-    && tar xvzf openldap-${LDAP_TAG}.tgz \
-    && cd openldap-${LDAP_TAG} \
-    && ./configure && make depend && make && make install \
-    && cd .. && rm -rf  openldap-${LDAP_TAG}
 
 COPY .ci/docker-compose-file/openldap/slapd.conf /usr/local/etc/openldap/slapd.conf
 COPY apps/emqx_ldap/test/data/emqx.io.ldif /usr/local/etc/openldap/schema/emqx.io.ldif

.ci/docker-compose-file/openldap/slapd.conf

@@ -1,14 +1,13 @@
 include         /usr/local/etc/openldap/schema/core.schema
 include         /usr/local/etc/openldap/schema/cosine.schema
 include         /usr/local/etc/openldap/schema/inetorgperson.schema
-include         /usr/local/etc/openldap/schema/ppolicy.schema
 include         /usr/local/etc/openldap/schema/emqx.schema
 
 TLSCACertificateFile  /usr/local/etc/openldap/cacert.pem
 TLSCertificateFile    /usr/local/etc/openldap/cert.pem
 TLSCertificateKeyFile /usr/local/etc/openldap/key.pem
 
-database bdb
+database mdb
 suffix "dc=emqx,dc=io"
 rootdn "cn=root,dc=emqx,dc=io"
 rootpw {SSHA}eoF7NhNrejVYYyGHqnt+MdKNBh4r1w3W

apps/emqx/include/emqx_release.hrl

@@ -32,10 +32,10 @@
 %% `apps/emqx/src/bpapi/README.md'
 
 %% Opensource edition
--define(EMQX_RELEASE_CE, "5.2.1").
+-define(EMQX_RELEASE_CE, "5.3.0").
 
 %% Enterprise edition
--define(EMQX_RELEASE_EE, "5.3.0-alpha.2").
+-define(EMQX_RELEASE_EE, "5.3.0-rc.1").
 
 %% The HTTP API version
 -define(EMQX_API_VERSION, "5.0").

apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl

@@ -124,15 +124,17 @@ login(
     of
         {ok, []} ->
             {error, user_not_found};
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             case
                 emqx_resource:simple_sync_query(
                     ResourceId,
                     {bind, Entry#eldap_entry.object_name, Sign}
                 )
             of
-                ok ->
+                {ok, #{result := ok}} ->
                     ensure_user_exists(Username);
+                {ok, #{result := 'invalidCredentials'} = Reason} ->
+                    {error, Reason};
                 {error, _} = Error ->
                     Error
             end;

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

@@ -249,7 +249,7 @@ do_ldap_query(
     #{pool_name := PoolName} = State
 ) ->
     LogMeta = #{connector => InstId, search => SearchOptions, state => emqx_utils:redact(State)},
-    ?TRACE("QUERY", "ldap_connector_received", LogMeta),
+    ?TRACE("QUERY", "ldap_connector_received_query", LogMeta),
     case
         ecpool:pick_and_do(
             PoolName,
@@ -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, {unrecoverable_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,15 +95,21 @@ authenticate(
     of
         {ok, []} ->
             ignore;
-        {ok, [Entry | _]} ->
+        {ok, [Entry]} ->
             case
                 emqx_resource:simple_sync_query(
                     ResourceId,
                     {bind, Entry#eldap_entry.object_name, Credential}
                 )
             of
-                ok ->
+                {ok, #{result := ok}} ->
                     {ok, #{is_superuser => false}};
+                {ok, #{result := 'invalidCredentials'}} ->
+                    ?TRACE_AUTHN_PROVIDER(error, "ldap_bind_failed", #{
+                        resource => ResourceId,
+                        reason => 'invalidCredentials'
+                    }),
+                    {error, bad_username_or_password};
                 {error, Reason} ->
                     ?TRACE_AUTHN_PROVIDER(error, "ldap_bind_failed", #{
                         resource => 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_ldap/src/emqx_ldap_bind_worker.erl

@@ -80,7 +80,9 @@ on_query(
                 ldap_connector_query_return,
                 #{result => ok}
             ),
-            ok;
+            {ok, #{result => ok}};
+        {error, 'invalidCredentials'} ->
+            {ok, #{result => 'invalidCredentials'}};
         {error, Reason} ->
             ?SLOG(
                 error,

apps/emqx_ldap/test/data/emqx.io.ldif

@@ -13,6 +13,12 @@ objectClass: top
 objectclass:organizationalUnit
 ou:testdevice
 
+# create dashboard.emqx.io
+dn:ou=dashboard,dc=emqx,dc=io
+objectClass: top
+objectclass:organizationalUnit
+ou:dashboard
+
 # create user admin
 dn:uid=admin,ou=testdevice,dc=emqx,dc=io
 objectClass: top
@@ -150,3 +156,23 @@ objectClass: mqttSecurity
 uid: mqttuser0007
 isSuperuser: TRUE
 userPassword: {SHA}axpQGbl00j3jvOG058y313ocnBk=
+
+## Try to test with base DN 'ou=dashboard,dc=emqx,dc=io'
+## with a filter ugroup=group1
+## this should return 2 users in the query and fail the test
+
+## echo -n "viewer1" | sha1sum | cut -d' ' -f1 | xxd -r -p | base64
+dn:uid=viewer1,ou=dashboard,dc=emqx,dc=io
+objectClass: top
+objectClass: dashboardUser
+uid: viewer1
+ugroup: group1
+userPassword: {SHA}I/LgVpQ6joiHifK7pZEQ1+0AUlg=
+
+## echo -n "viewer2" | sha1sum | cut -d' ' -f1 | xxd -r -p | base64
+dn:uid=viewer2,ou=dashboard,dc=emqx,dc=io
+objectClass: top
+objectClass: dashboardUser
+uid: viewer2
+ugroup: group1
+userPassword: {SHA}SR0qZpf8pYKKAbn6ILFvX91JuQg=

apps/emqx_ldap/test/data/emqx.schema

@@ -35,10 +35,11 @@ attributetype ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.4.4 NAME ( 'mqttAccountName' 'ma
 	SYNTAX 1.3.6.1.4.1.1466.115.121.1.15
 	USAGE userApplications )
 
-
+attributetype ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.5.1 NAME 'ugroup'
-objectclass ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.4 NAME 'mqttUser'
+	EQUALITY caseIgnoreMatch
-	AUXILIARY
+	SUBSTR caseIgnoreSubstringsMatch
-	MAY ( mqttPublishTopic $ mqttSubscriptionTopic $ mqttPubSubTopic $ mqttAccountName $ isSuperuser) )
+	SYNTAX 1.3.6.1.4.1.1466.115.121.1.15
+	USAGE userApplications )
 
 objectclass ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.2 NAME 'mqttDevice'
 	SUP top
@@ -50,3 +51,13 @@ objectclass ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.3 NAME 'mqttSecurity'
 	SUP top
 	AUXILIARY
 	MUST ( userPassword ) )
+
+objectclass ( 1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.4 NAME 'mqttUser'
+	AUXILIARY
+	MAY ( mqttPublishTopic $ mqttSubscriptionTopic $ mqttPubSubTopic $ mqttAccountName $ isSuperuser ) )
+
+objectclass (1.3.6.1.4.1.11.2.53.2.2.3.1.2.3.5 NAME 'dashboardUser'
+	SUP top
+    STRUCTURAL
+	MUST ( uid $ userPassword )
+    MAY ( ugroup ))

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};

deploy/charts/emqx-enterprise/Chart.yaml

@@ -14,8 +14,8 @@ type: application
 
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
-version: 5.3.0-alpha.2
+version: 5.3.0-rc.1
 
 # This is the version number of the application being deployed. This version number should be
 # incremented each time you make changes to the application.
-appVersion: 5.3.0-alpha.2
+appVersion: 5.3.0-rc.1

deploy/charts/emqx/Chart.yaml

@@ -14,8 +14,8 @@ type: application
 
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
-version: 5.2.1
+version: 5.3.0
 
 # This is the version number of the application being deployed. This version number should be
 # incremented each time you make changes to the application.
-appVersion: 5.2.1
+appVersion: 5.3.0