Merge pull request #12931 from zmstone/0425-fix-acl.conf-topic-template-render-fialure-handling

0425 fix acl.conf topic template render fialure handling

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,

apps/emqx_auth/etc/acl.conf

@@ -1,32 +1,123 @@
-%%--------------------------------------------------------------------
-%% -type(ipaddr() :: {ipaddr, string()}).
-%%
-%% -type(ipaddrs() :: {ipaddrs, [string()]}).
-%%
-%% -type(username() :: {user | username, string()} | {user | username, {re, regex()}}).
+%%-------------- Default ACL rules -------------------------------------------------------
+
+{allow, {username, {re, "^dashboard$"}}, subscribe, ["$SYS/#"]}.
+
+{allow, {ipaddr, "127.0.0.1"}, all, ["$SYS/#", "#"]}.
+
+{deny, all, subscribe, ["$SYS/#", {eq, "#"}]}.
+
+{allow, all}.
+%% NOTE! when deploy in production:
+%% - Change the last rule to `{deny, all}.`
+%% - Set config `authorization.no_match = deny`
+
+%% See docs below
 %%
-%% -type(clientid() :: {client | clientid, string()} | {client | clientid, {re, regex()}}).
+%% ------------ The formal spec ----------------------------------------------------------
 %%
-%% -type(who() :: ipaddr() | ipaddrs() | username() | clientid() |
+%% -type ipaddr() :: {ipaddr, string()}.
+%% -type ipaddrs() :: {ipaddrs, [string()]}.
+%% -type username() :: {user | username, string()} | {user | username, {re, regex()}}.
+%% -type clientid() :: {client | clientid, string()} | {client | clientid, {re, regex()}}.
+%% -type who() :: ipaddr() | ipaddrs() | username() | clientid() |
 %%                {'and', [ipaddr() | ipaddrs() | username() | clientid()]} |
 %%                {'or',  [ipaddr() | ipaddrs() | username() | clientid()]} |
-%%                all).
+%%                all.
+%% -type simple_action() :: subscribe | publish | all.
+%% -type complex_action() :: {simple_action(), [{qos, 0..2}, {retain, true|false|all}]}.
+%% -type action() :: simple_action() | complex_action().
+%% -type topic() :: string().
+%% -type topic_filter() :: string().
+%% -type topic_match() :: topic() | topic_filter() | {eq, topic() | topic_filter()}.
+%% -type perm() :: allow | deny.
+%% -type rule() :: {perm(), who(), action(), [topic_match()]} | {perm(), all}.
+
+%%-------------- Viusal aid for the spec -------------------------------------------------
 %%
-%% -type(action() :: subscribe | publish | all).
+%% rule()
+%% ├── {perm(), who(), action(), [topic_match()]}
+%% │    │       │      │          ├── topic() :: string()
+%% │    │       │      │          ├── topic_filter() :: string()
+%% │    │       │      │          └── {eq, topic() | topic_filter()}
+%% │    │       │      │
+%% │    │       │      ├── simple_action()
+%% │    │       │      │   ├── publish
+%% │    │       │      │   ├── subscribe
+%% │    │       │      │   └── all
+%% │    │       │      └── {simple_action(), [{qos,0..2},{retain,true|false|all}]}
+%% │    │       │
+%% │    │       ├── ipaddr()
+%% │    │       │   └── {ipaddr, string()}
+%% │    │       ├── ipaddrs()
+%% │    │       │   └── {ipaddrs, [string()]}
+%% │    │       ├── username()
+%% │    │       │   ├── {user | username, string()}
+%% │    │       │   └── {user | username, {re, regex()}}
+%% │    │       ├── clientid()
+%% │    │       │   ├── {client | clientid, string()}
+%% │    │       │   └── {client | clientid, {re, regex()}}
+%% │    │       ├── {'and', [ipaddr() | ipaddrs() | username() | clientid()]}
+%% │    │       ├── {'or', [ipaddr() | ipaddrs() | username() | clientid()]}
+%% │    │       └── all
+%% │    │
+%% │    ├── allow
+%% │    └── deny
+%% │
+%% └── {perm(), all}
 %%
-%% -type(topic_filters() :: string()).
+
+%% This file defines a set of ACL rules for MQTT client pub/sub authorization.
+%% The content is of Erlang-term format.
+%% Each Erlang-term is a tuple `{...}` terminated by dot `.`
 %%
-%% -type(topics() :: [topic_filters() | {eq, topic_filters()}]).
+%% NOTE: When deploy to production, the last rule should be changed to {deny, all}.
 %%
-%% -type(permission() :: allow | deny).
+%% NOTE: It's a good practice to keep the nubmer of rules small, because in worst case
+%%       scenarios, all rules have to be traversed for each message publish.
 %%
-%% -type(rule() :: {permission(), who(), action(), topics()} | {permission(), all}).
-%%--------------------------------------------------------------------
-
-{allow, {username, {re, "^dashboard$"}}, subscribe, ["$SYS/#"]}.
-
-{allow, {ipaddr, "127.0.0.1"}, all, ["$SYS/#", "#"]}.
-
-{deny, all, subscribe, ["$SYS/#", {eq, "#"}]}.
-
-{allow, all}.
+%% A rule is a 4-element tuple.
+%% For example, `{allow, {username, "Jon"}, subscribe, ["#"]}` allows Jon to subscribe to
+%% any topic they want.
+%%
+%% Below is an explanation:
+%%
+%% - `perm()`: The permission.
+%%   Defines whether this is an `allow` or `deny` rule.
+%%
+%% - `who()`: The MQTT client matching condition.
+%%   - `all`: A rule which applies to all clients.
+%%   - `{ipaddr, IpAddress}`: Matches a client by source IP address. CIDR notation is allowed.
+%%   - `{ipaddrs, [IpAddress]}`: Matches clients by a set of IP addresses. CIDR notation is allowed.
+%%   - `{clientid, ClientID}`: Matches a client by ID.
+%%   - `{username, Username}`: Matches a client by username.
+%%   - `{..., {re, ..}}`: Regular expression to match either clientid or username.
+%%   - `{'and', [...]}`: Combines a list of matching conditions.
+%%   - `{'or', [...]}`: Combines a list of matching conditions.
+%%
+%% - `action()`: Matches publish or subscribe actions (or both).
+%%   Applies the rule to `publish` or `subscribe` actions.
+%%   The special value `all` denotes allowing or denying both `publish` and `subscribe`.
+%%   It can also be associated with `qos` and `retain` flags to match the action with
+%%   more specifics. For example, `{publish, [{qos,0},{retain,false}]}` should only
+%%   match the `publish` action when the message has QoS 0, and without retained flag set.
+%%
+%% - `[topic_match()]`:
+%%   A list of topics, topic-filters, or template rendering to match the topic being
+%%   subscribed to or published.
+%%   For example, `{allow, {username, "Jan"}, publish, ["jan/#"]}` permits Jan to publish
+%%   to any topic matching the wildcard pattern "jan/#".
+%%   A special tuple `{eq, topic_match()}` is useful to allow or deny the specific wildcard
+%%   subscription instead of performing a topic match.
+%%   A `topic_match()` can also contain a placeholder rendered with actual value at runtime,
+%%   for example, `{allow, all, publish, "${clientid}/#"}` allows all clients to publish to
+%%   topics prefixed by their own client ID.
+%%
+%%   Supported placeholders are:
+%%   - `${cn}`: TLS certificate common name.
+%%   - `${clientid}`: The client ID.
+%%   - `${username}`: The username.
+%%   - `${client_attrs.NAME}`: A client attribute named `NAME`, which can be initialized by
+%%     `mqtt.client_attrs_init` config or extended by certain authentication backends.
+%%   NOTE: Placeholder is not rendered as empty string if the referencing value is not
+%%         foud. For example, `${client_attrs.group}/#` is not rendered as `/#` if the
+%%         client does not have a `group` attribute.

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.

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.