From d30b52f0f9ea08a2f74aea4628d140ab8c48f638 Mon Sep 17 00:00:00 2001 From: zmstone Date: Thu, 25 Apr 2024 14:04:50 +0200 Subject: [PATCH 1/2] docs: refine acl.conf comments --- apps/emqx_auth/etc/acl.conf | 137 +++++++++++++++++++++++++++++------- 1 file changed, 113 insertions(+), 24 deletions(-) diff --git a/apps/emqx_auth/etc/acl.conf b/apps/emqx_auth/etc/acl.conf index dbeec6852..b23714558 100644 --- a/apps/emqx_auth/etc/acl.conf +++ b/apps/emqx_auth/etc/acl.conf @@ -1,27 +1,4 @@ -%%-------------------------------------------------------------------- -%% -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). -%% -%% -type(action() :: subscribe | publish | all). -%% -%% -type(topic_filters() :: string()). -%% -%% -type(topics() :: [topic_filters() | {eq, topic_filters()}]). -%% -%% -type(permission() :: allow | deny). -%% -%% -type(rule() :: {permission(), who(), action(), topics()} | {permission(), all}). -%%-------------------------------------------------------------------- +%%-------------- Default ACL rules ------------------------------------------------------- {allow, {username, {re, "^dashboard$"}}, subscribe, ["$SYS/#"]}. @@ -29,4 +6,116 @@ {deny, all, subscribe, ["$SYS/#", {eq, "#"}]}. +%% NOTE: change to {deny, all} in production! {allow, all}. + +%% See docs below +%% +%% ------------ The formal spec ---------------------------------------------------------- +%% +%% -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. +%% -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 ------------------------------------------------- +%% +%% 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} +%% + +%% 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 `.` +%% +%% NOTE: When deploy to production, the last rule should be changed to {deny, all}. +%% +%% 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. +%% +%% 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. From 01923147a20500a2767122ca25aead86cc975d3d Mon Sep 17 00:00:00 2001 From: zmstone Date: Thu, 25 Apr 2024 16:20:47 +0200 Subject: [PATCH 2/2] fix(variform and authz): do not initialize empty client_attrs field when client_attrs_init expression renders to empty string, do not initialize the attribute. also fixed an ACL error: a template render failure for a topic would stop the ACL checks for the following topics if more than one topic is configured. --- apps/emqx/src/emqx_channel.erl | 9 ++++ apps/emqx_auth/etc/acl.conf | 4 +- .../src/emqx_authz/emqx_authz_rule.erl | 18 +++++++- .../test/emqx_authz/emqx_authz_SUITE.erl | 41 ++++++++++++++++++- 4 files changed, 68 insertions(+), 4 deletions(-) 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.