diff --git a/apps/emqx/src/emqx_channel.erl b/apps/emqx/src/emqx_channel.erl index efb5133bc..ec49d165c 100644 --- a/apps/emqx/src/emqx_channel.erl +++ b/apps/emqx/src/emqx_channel.erl @@ -1638,6 +1638,15 @@ initialize_client_attrs(Inits, ClientInfo) -> fun(#{expression := Variform, set_as_attr := Name}, Acc) -> Attrs = maps:get(client_attrs, ClientInfo, #{}), case emqx_variform:render(Variform, ClientInfo) of + {ok, <<>>} -> + ?SLOG( + debug, + #{ + msg => "client_attr_rednered_to_empty_string", + set_as_attr => Name + } + ), + Acc; {ok, Value} -> ?SLOG( debug, diff --git a/apps/emqx_auth/etc/acl.conf b/apps/emqx_auth/etc/acl.conf index b23714558..3cc0ed5b8 100644 --- a/apps/emqx_auth/etc/acl.conf +++ b/apps/emqx_auth/etc/acl.conf @@ -6,8 +6,10 @@ {deny, all, subscribe, ["$SYS/#", {eq, "#"}]}. -%% NOTE: change to {deny, all} in production! {allow, all}. +%% NOTE! when deploy in production: +%% - Change the last rule to `{deny, all}.` +%% - Set config `authorization.no_match = deny` %% See docs below %% diff --git a/apps/emqx_auth/src/emqx_authz/emqx_authz_rule.erl b/apps/emqx_auth/src/emqx_authz/emqx_authz_rule.erl index 9a877b5c8..b3bddada4 100644 --- a/apps/emqx_auth/src/emqx_authz/emqx_authz_rule.erl +++ b/apps/emqx_auth/src/emqx_authz/emqx_authz_rule.erl @@ -351,8 +351,9 @@ match_who(_, _) -> match_topics(_ClientInfo, _Topic, []) -> false; match_topics(ClientInfo, Topic, [{pattern, PatternFilter} | Filters]) -> - TopicFilter = bin(emqx_template:render_strict(PatternFilter, ClientInfo)), - match_topic(emqx_topic:words(Topic), emqx_topic:words(TopicFilter)) orelse + TopicFilter = render_topic(PatternFilter, ClientInfo), + (is_binary(TopicFilter) andalso + match_topic(emqx_topic:words(Topic), emqx_topic:words(TopicFilter))) orelse match_topics(ClientInfo, Topic, Filters); match_topics(ClientInfo, Topic, [TopicFilter | Filters]) -> match_topic(emqx_topic:words(Topic), TopicFilter) orelse @@ -362,3 +363,16 @@ match_topic(Topic, {'eq', TopicFilter}) -> Topic =:= TopicFilter; match_topic(Topic, TopicFilter) -> emqx_topic:match(Topic, TopicFilter). + +render_topic(Topic, ClientInfo) -> + try + bin(emqx_template:render_strict(Topic, ClientInfo)) + catch + error:Reason -> + ?SLOG(debug, #{ + msg => "failed_to_render_topic_template", + template => Topic, + reason => Reason + }), + error + end. diff --git a/apps/emqx_auth/test/emqx_authz/emqx_authz_SUITE.erl b/apps/emqx_auth/test/emqx_authz/emqx_authz_SUITE.erl index 70dd0bbb6..575eb4109 100644 --- a/apps/emqx_auth/test/emqx_authz/emqx_authz_SUITE.erl +++ b/apps/emqx_auth/test/emqx_authz/emqx_authz_SUITE.erl @@ -173,7 +173,16 @@ end_per_testcase(_TestCase, _Config) -> -define(SOURCE_FILE_CLIENT_ATTR, ?SOURCE_FILE( << - "{allow,all,all,[\"${client_attrs.alias}/#\"]}.\n" + "{allow,all,all,[\"${client_attrs.alias}/#\",\"client_attrs_backup\"]}.\n" + "{deny, all}." + >> + ) +). + +-define(SOURCE_FILE_CLIENT_NO_SUCH_ATTR, + ?SOURCE_FILE( + << + "{allow,all,all,[\"${client_attrs.nonexist}/#\",\"client_attrs_backup\"]}.\n" "{deny, all}." >> ) @@ -572,11 +581,41 @@ t_alias_prefix(_Config) -> ?assertMatch({ok, _}, emqtt:connect(C)), ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C, SubTopic)), ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, SubTopicNotAllowed)), + ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, <<"/#">>)), unlink(C), emqtt:stop(C), + NonMatching = <<"clientid_which_has_no_dash">>, + {ok, C2} = emqtt:start_link([{clientid, NonMatching}, {proto_ver, v5}]), + ?assertMatch({ok, _}, emqtt:connect(C2)), + ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C2, <<"client_attrs_backup">>)), + %% assert '${client_attrs.alias}/#' is not rendered as '/#' + ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C2, <<"/#">>)), + unlink(C2), + emqtt:stop(C2), emqx_config:put_zone_conf(default, [mqtt, client_attrs_init], []), ok. +t_non_existing_attr(_Config) -> + {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE_FILE_CLIENT_NO_SUCH_ATTR]), + %% '^.*-(.*)$': extract the suffix after the last '-' + {ok, Compiled} = emqx_variform:compile("concat(regex_extract(clientid,'^.*-(.*)$'))"), + emqx_config:put_zone_conf(default, [mqtt, client_attrs_init], [ + #{ + expression => Compiled, + %% this is intended to be different from 'nonexist' + set_as_attr => <<"existing">> + } + ]), + ClientId = <<"org1-name3">>, + {ok, C} = emqtt:start_link([{clientid, ClientId}, {proto_ver, v5}]), + ?assertMatch({ok, _}, emqtt:connect(C)), + ?assertMatch({ok, _, [?RC_SUCCESS]}, emqtt:subscribe(C, <<"client_attrs_backup">>)), + %% assert '${client_attrs.nonexist}/#' is not rendered as '/#' + ?assertMatch({ok, _, [?RC_NOT_AUTHORIZED]}, emqtt:subscribe(C, <<"/#">>)), + unlink(C), + emqtt:stop(C), + ok. + %% client is allowed by ACL to publish to its LWT topic, is connected, %% and then gets banned and kicked out while connected. Should not %% publish LWT.