From 5532c7b0a6aba242b13f442f11abdbb10d55fff1 Mon Sep 17 00:00:00 2001 From: firest Date: Thu, 6 Jun 2024 16:22:53 +0800 Subject: [PATCH 1/3] fix(authz_mnesia): add a soft limit in the API for the length of ACL rules --- .../src/emqx_authz_api_mnesia.erl | 98 ++++++++++++++----- .../src/emqx_authz_mnesia_schema.erl | 14 ++- .../test/emqx_authz_api_mnesia_SUITE.erl | 32 +++++- rel/i18n/emqx_authz_mnesia_schema.hocon | 3 + 4 files changed, 121 insertions(+), 26 deletions(-) diff --git a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl index fd073eb84..88cb8ab63 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl +++ b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl @@ -469,8 +469,8 @@ users(get, #{query_string := QueryString}) -> {200, Result} end; users(post, #{body := Body}) when is_list(Body) -> - case ensure_all_not_exists(<<"username">>, username, Body) of - [] -> + case ensure_rules_is_valid(<<"username">>, username, Body) of + ok -> lists:foreach( fun(#{<<"username">> := Username, <<"rules">> := Rules}) -> emqx_authz_mnesia:store_rules({username, Username}, Rules) @@ -478,10 +478,16 @@ users(post, #{body := Body}) when is_list(Body) -> Body ), {204}; - Exists -> + {error, rules_too_long} -> + {400, #{ + code => <<"BAD_REQUEST">>, + message => + <<"The length of rules exceeds the maximum limit.">> + }}; + {error, {already_exists, Exists}} -> {409, #{ code => <<"ALREADY_EXISTS">>, - message => binfmt("Users '~ts' already exist", [binjoin(Exists)]) + message => binfmt("User '~ts' already exist", [binjoin(Exists)]) }} end. @@ -507,8 +513,8 @@ clients(get, #{query_string := QueryString}) -> {200, Result} end; clients(post, #{body := Body}) when is_list(Body) -> - case ensure_all_not_exists(<<"clientid">>, clientid, Body) of - [] -> + case ensure_rules_is_valid(<<"clientid">>, clientid, Body) of + ok -> lists:foreach( fun(#{<<"clientid">> := ClientID, <<"rules">> := Rules}) -> emqx_authz_mnesia:store_rules({clientid, ClientID}, Rules) @@ -516,10 +522,16 @@ clients(post, #{body := Body}) when is_list(Body) -> Body ), {204}; - Exists -> + {error, rules_too_long} -> + {400, #{ + code => <<"BAD_REQUEST">>, + message => + <<"The length of rules exceeds the maximum limit.">> + }}; + {error, {already_exists, Exists}} -> {409, #{ code => <<"ALREADY_EXISTS">>, - message => binfmt("Clients '~ts' already exist", [binjoin(Exists)]) + message => binfmt("Client '~ts' already exist", [binjoin(Exists)]) }} end. @@ -583,8 +595,17 @@ all(get, _) -> }} end; all(post, #{body := #{<<"rules">> := Rules}}) -> - emqx_authz_mnesia:store_rules(all, Rules), - {204}; + case ensure_rules_len(Rules) of + ok -> + emqx_authz_mnesia:store_rules(all, Rules), + {204}; + _ -> + {400, #{ + code => <<"BAD_REQUEST">>, + message => + <<"The length of rules exceeds the maximum limit.">> + }} + end; all(delete, _) -> emqx_authz_mnesia:store_rules(all, []), {204}. @@ -700,24 +721,53 @@ rules_example({ExampleName, ExampleType}) -> } }. -ensure_all_not_exists(Key, Type, Cfgs) -> - lists:foldl( - fun(#{Key := Id}, Acc) -> - case emqx_authz_mnesia:get_rules({Type, Id}) of - not_found -> - Acc; - _ -> - [Id | Acc] - end - end, - [], - Cfgs +ensure_rules_len(Rules) -> + emqx_authz_api_sources:with_source( + ?AUTHZ_TYPE_BIN, + fun(#{<<"max_rules_len">> := MaxLen}) -> + ensure_rules_len(Rules, MaxLen) + end ). +ensure_rules_len(Rules, MaxLen) -> + case erlang:length(Rules) =< MaxLen of + true -> + ok; + _ -> + {error, rules_too_long} + end. + +ensure_rules_is_valid(Key, Type, Cfgs) -> + MaxLen = emqx_authz_api_sources:with_source( + ?AUTHZ_TYPE_BIN, + fun(#{<<"max_rules_len">> := MaxLen}) -> + MaxLen + end + ), + ensure_rules_is_valid(Key, Type, MaxLen, Cfgs). + +ensure_rules_is_valid(Key, Type, MaxLen, [Cfg | Cfgs]) -> + #{Key := Id, <<"rules">> := Rules} = Cfg, + case emqx_authz_mnesia:get_rules({Type, Id}) of + not_found -> + case ensure_rules_len(Rules, MaxLen) of + ok -> + ensure_rules_is_valid(Key, Type, MaxLen, Cfgs); + Error -> + Error + end; + _ -> + {error, {already_exists, Id}} + end; +ensure_rules_is_valid(_Key, _Type, _MaxLen, []) -> + ok. + binjoin([Bin]) -> Bin; -binjoin(Bins) -> - binjoin(Bins, <<>>). +binjoin(Bins) when is_list(Bins) -> + binjoin(Bins, <<>>); +binjoin(Bin) -> + Bin. binjoin([H | T], Acc) -> binjoin(T, <>); diff --git a/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl b/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl index 1924752f3..72b71b39a 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl +++ b/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl @@ -30,12 +30,24 @@ namespace/0 ]). +-define(MAX_RULES_LEN, 100). + namespace() -> "authz". type() -> ?AUTHZ_TYPE. fields(builtin_db) -> - emqx_authz_schema:authz_common_fields(?AUTHZ_TYPE). + emqx_authz_schema:authz_common_fields(?AUTHZ_TYPE) ++ + [ + {max_rules_len, + ?HOCON( + pos_integer(), + #{ + default => ?MAX_RULES_LEN, + desc => ?DESC(max_rules_len) + } + )} + ]. source_refs() -> [?R_REF(builtin_db)]. diff --git a/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl b/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl index cb9875ee2..6eb467d67 100644 --- a/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl +++ b/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl @@ -36,7 +36,7 @@ init_per_suite(Config) -> {emqx_conf, "authorization.cache { enable = false }," "authorization.no_match = deny," - "authorization.sources = [{type = built_in_database}]"}, + "authorization.sources = [{type = built_in_database, max_rules_len = 5}]"}, emqx, emqx_auth, emqx_auth_mnesia, @@ -66,6 +66,14 @@ t_api(_) -> [?USERNAME_RULES_EXAMPLE] ), + %% check length limit + {ok, 400, _} = + request( + post, + uri(["authorization", "sources", "built_in_database", "rules", "users"]), + [dup_rules_example(?USERNAME_RULES_EXAMPLE)] + ), + {ok, 409, _} = request( post, @@ -171,6 +179,13 @@ t_api(_) -> [?CLIENTID_RULES_EXAMPLE] ), + {ok, 400, _} = + request( + post, + uri(["authorization", "sources", "built_in_database", "rules", "clients"]), + [dup_rules_example(?CLIENTID_RULES_EXAMPLE)] + ), + {ok, 409, _} = request( post, @@ -238,6 +253,14 @@ t_api(_) -> uri(["authorization", "sources", "built_in_database", "rules", "all"]), ?ALL_RULES_EXAMPLE ), + + {ok, 400, _} = + request( + post, + uri(["authorization", "sources", "built_in_database", "rules", "all"]), + dup_rules_example(?ALL_RULES_EXAMPLE) + ), + {ok, 200, Request7} = request( get, @@ -491,3 +514,10 @@ replace_parts(Parts, Replacements) -> end, Parts ). + +dup_rules_example(#{username := _, rules := Rules}) -> + #{username => user2, rules => Rules ++ Rules}; +dup_rules_example(#{clientid := _, rules := Rules}) -> + #{clientid => client2, rules => Rules ++ Rules}; +dup_rules_example(#{rules := Rules}) -> + #{rules => Rules ++ Rules}. diff --git a/rel/i18n/emqx_authz_mnesia_schema.hocon b/rel/i18n/emqx_authz_mnesia_schema.hocon index 6face07b9..2c36dac20 100644 --- a/rel/i18n/emqx_authz_mnesia_schema.hocon +++ b/rel/i18n/emqx_authz_mnesia_schema.hocon @@ -6,4 +6,7 @@ builtin_db.desc: builtin_db.label: """Builtin Database""" +max_rules_len.desc: +"""Maximum rule length per client/user. Note that performance may decrease as rule length increases.""" + } From 3ae26c8a547b82a37da8e3c46ea61ae0893c301e Mon Sep 17 00:00:00 2001 From: firest Date: Thu, 6 Jun 2024 16:42:43 +0800 Subject: [PATCH 2/3] chore: update changes --- .../src/emqx_authz_api_mnesia.erl | 16 ++-------------- changes/ce/fix-13196.en.md | 1 + 2 files changed, 3 insertions(+), 14 deletions(-) create mode 100644 changes/ce/fix-13196.en.md diff --git a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl index 88cb8ab63..0cefe929d 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl +++ b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl @@ -487,7 +487,7 @@ users(post, #{body := Body}) when is_list(Body) -> {error, {already_exists, Exists}} -> {409, #{ code => <<"ALREADY_EXISTS">>, - message => binfmt("User '~ts' already exist", [binjoin(Exists)]) + message => binfmt("User '~ts' already exist", [Exists]) }} end. @@ -531,7 +531,7 @@ clients(post, #{body := Body}) when is_list(Body) -> {error, {already_exists, Exists}} -> {409, #{ code => <<"ALREADY_EXISTS">>, - message => binfmt("Client '~ts' already exist", [binjoin(Exists)]) + message => binfmt("Client '~ts' already exist", [Exists]) }} end. @@ -762,16 +762,4 @@ ensure_rules_is_valid(Key, Type, MaxLen, [Cfg | Cfgs]) -> ensure_rules_is_valid(_Key, _Type, _MaxLen, []) -> ok. -binjoin([Bin]) -> - Bin; -binjoin(Bins) when is_list(Bins) -> - binjoin(Bins, <<>>); -binjoin(Bin) -> - Bin. - -binjoin([H | T], Acc) -> - binjoin(T, <>); -binjoin([], Acc) -> - Acc. - binfmt(Fmt, Args) -> iolist_to_binary(io_lib:format(Fmt, Args)). diff --git a/changes/ce/fix-13196.en.md b/changes/ce/fix-13196.en.md new file mode 100644 index 000000000..b0fed0eae --- /dev/null +++ b/changes/ce/fix-13196.en.md @@ -0,0 +1 @@ +In the built-in database of authorization, added a limit for the length of rules per client/user, and the default values is 100. From 171685205775dc8216fc2cd4fe3ee3c9d838134d Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 12 Jun 2024 10:13:43 +0800 Subject: [PATCH 3/3] fix(authz_mnesia): improve field names and changes --- .../src/emqx_authz_api_mnesia.erl | 24 ++++++++++++------- .../src/emqx_authz_mnesia_schema.erl | 8 +++---- .../test/emqx_authz_api_mnesia_SUITE.erl | 2 +- changes/ce/fix-13196.en.md | 2 +- rel/i18n/emqx_authz_mnesia_schema.hocon | 4 ++-- 5 files changed, 23 insertions(+), 17 deletions(-) diff --git a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl index 0cefe929d..7e95a6500 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl +++ b/apps/emqx_auth_mnesia/src/emqx_authz_api_mnesia.erl @@ -478,11 +478,14 @@ users(post, #{body := Body}) when is_list(Body) -> Body ), {204}; - {error, rules_too_long} -> + {error, {Username, too_many_rules}} -> {400, #{ code => <<"BAD_REQUEST">>, message => - <<"The length of rules exceeds the maximum limit.">> + binfmt( + <<"The rules length of User '~ts' exceeds the maximum limit.">>, + [Username] + ) }}; {error, {already_exists, Exists}} -> {409, #{ @@ -522,11 +525,14 @@ clients(post, #{body := Body}) when is_list(Body) -> Body ), {204}; - {error, rules_too_long} -> + {error, {ClientId, too_many_rules}} -> {400, #{ code => <<"BAD_REQUEST">>, message => - <<"The length of rules exceeds the maximum limit.">> + binfmt( + <<"The rules length of Client '~ts' exceeds the maximum limit.">>, + [ClientId] + ) }}; {error, {already_exists, Exists}} -> {409, #{ @@ -724,7 +730,7 @@ rules_example({ExampleName, ExampleType}) -> ensure_rules_len(Rules) -> emqx_authz_api_sources:with_source( ?AUTHZ_TYPE_BIN, - fun(#{<<"max_rules_len">> := MaxLen}) -> + fun(#{<<"max_rules">> := MaxLen}) -> ensure_rules_len(Rules, MaxLen) end ). @@ -734,13 +740,13 @@ ensure_rules_len(Rules, MaxLen) -> true -> ok; _ -> - {error, rules_too_long} + {error, too_many_rules} end. ensure_rules_is_valid(Key, Type, Cfgs) -> MaxLen = emqx_authz_api_sources:with_source( ?AUTHZ_TYPE_BIN, - fun(#{<<"max_rules_len">> := MaxLen}) -> + fun(#{<<"max_rules">> := MaxLen}) -> MaxLen end ), @@ -753,8 +759,8 @@ ensure_rules_is_valid(Key, Type, MaxLen, [Cfg | Cfgs]) -> case ensure_rules_len(Rules, MaxLen) of ok -> ensure_rules_is_valid(Key, Type, MaxLen, Cfgs); - Error -> - Error + {error, Reason} -> + {error, {Id, Reason}} end; _ -> {error, {already_exists, Id}} diff --git a/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl b/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl index 72b71b39a..1076be2f3 100644 --- a/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl +++ b/apps/emqx_auth_mnesia/src/emqx_authz_mnesia_schema.erl @@ -30,7 +30,7 @@ namespace/0 ]). --define(MAX_RULES_LEN, 100). +-define(MAX_RULES, 100). namespace() -> "authz". @@ -39,12 +39,12 @@ type() -> ?AUTHZ_TYPE. fields(builtin_db) -> emqx_authz_schema:authz_common_fields(?AUTHZ_TYPE) ++ [ - {max_rules_len, + {max_rules, ?HOCON( pos_integer(), #{ - default => ?MAX_RULES_LEN, - desc => ?DESC(max_rules_len) + default => ?MAX_RULES, + desc => ?DESC(max_rules) } )} ]. diff --git a/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl b/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl index 6eb467d67..4ba4c7075 100644 --- a/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl +++ b/apps/emqx_auth_mnesia/test/emqx_authz_api_mnesia_SUITE.erl @@ -36,7 +36,7 @@ init_per_suite(Config) -> {emqx_conf, "authorization.cache { enable = false }," "authorization.no_match = deny," - "authorization.sources = [{type = built_in_database, max_rules_len = 5}]"}, + "authorization.sources = [{type = built_in_database, max_rules = 5}]"}, emqx, emqx_auth, emqx_auth_mnesia, diff --git a/changes/ce/fix-13196.en.md b/changes/ce/fix-13196.en.md index b0fed0eae..26950ffc0 100644 --- a/changes/ce/fix-13196.en.md +++ b/changes/ce/fix-13196.en.md @@ -1 +1 @@ -In the built-in database of authorization, added a limit for the length of rules per client/user, and the default values is 100. +In the built-in database of authorization, added a limit for the number of rules per client/user, and the default values is 100. diff --git a/rel/i18n/emqx_authz_mnesia_schema.hocon b/rel/i18n/emqx_authz_mnesia_schema.hocon index 2c36dac20..efc80dd34 100644 --- a/rel/i18n/emqx_authz_mnesia_schema.hocon +++ b/rel/i18n/emqx_authz_mnesia_schema.hocon @@ -6,7 +6,7 @@ builtin_db.desc: builtin_db.label: """Builtin Database""" -max_rules_len.desc: -"""Maximum rule length per client/user. Note that performance may decrease as rule length increases.""" +max_rules.desc: +"""Maximum number of rules per client/user. Note that performance may decrease as number of rules increases.""" }