Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(authz): use extensible map format for actions in authz rules #11132

Merged
merged 5 commits into from Jul 10, 2023

Conversation

savonarola
Copy link
Contributor

@savonarola savonarola commented Jun 23, 2023

Fixes EMQX-9766

Problem

We have to control whether a client can write retain messages or subscribe/publish using certain QoS levels.

Current model

In authz, we have

  • subjects — "who" makes a request/action
  • permissions — resolution whether to allow an action or not, allow and deny
  • resources — "what" is allowed. We have resources split into topic and action (publish, subscribe, all)

New model

The current model is not very extensible. We have hotwired primitive types for all resources. We need to extend the model to support more complex resources.

Since the topic is considered to be more or less "constant" data-like structure (e.g. different clients may subscribe the same topic in a different manner), we choose to extend the action part of the resource.
This also more or less reflects the MQTT protocol itself, where the QoS/Retain are a part of the PUBLISH/SUBSCRIBE packet: different clients subscribe differently to the same topic.

Internal model

Internally we extend the signature of the emqx_access_control:authorize/3 callback.
From

-type pubsub() :: publish | subscribe.
...
-spec authorize(emqx_types:clientinfo(), emqx_types:pubsub(), emqx_types:topic()) ->
    allow | deny.

To

-type pubsub() ::
    #{action_type := subscribe, qos := qos()}
    | #{action_type := publish, qos := qos(), retain := boolean()}.
...
-spec authorize(emqx_types:clientinfo(), emqx_types:pubsub(), emqx_types:topic()) ->
    allow | deny.

Thus, we make emqx_types:pubsub() extensible if needed.

We update all the entry points (including gateways) to pass the new extended action to the authorize/3 function.

External model

That is, how the user will configure the authorization rules.

We now explicitly differentiate between three types of rules:

  • raw rules: rules obtained as plain data structures from a database or API, having the format like:
#{
    <<"permission">> => <<"allow">>,
    <<"topic">> => <<"a/b/c">>,
    <<"action">> => <<"subscribe">>,
    <<"retain">> => <<"true">>,
    <<"qos">> => <<"1,2">>
}
%% or
#{
    <<"permission">> => <<"allow">>,
    <<"topic">> => [<<"a/b/c">>, <<"d">>],
    <<"action">> => <<"subscribe">>,
    <<"retain">> => <<"true">>,
    <<"qos">> => [1, 2]
}
...

We have a module emqx_authz_rule_raw to convert between raw and dsl rules.

  • dsl rules: rules described with Erlang data structures. They are used by mnesia and file backends.
    We use emqx_authz_rule to convert from dsl to compiled rules.
  • compiled rules: slightly converter dsl rules, used for final matching.

We refactor the backends to utilize the common parser and validator of raw rules.

Test refactoring

Previous test implementation was not very flexible and was not able to test the new model.
The problem was that the test both "inserted" the test data into a database and then also "queried" the database to check the results. However, many bugs may come from how the data is inserted into the database in the wild, because this is the part that is more likely to be customized by the user.

So we refactor the tests to be more like authn tests, where for each test we have its own storage schema and inserted data.
Example:

...
#{
    name => qos_retain_in_query_result_as_integer,
    setup => [
        "CREATE TABLE acl(username VARCHAR(255), topic VARCHAR(255), "
        "permission VARCHAR(255), action VARCHAR(255),"
        "qos_i VARCHAR(255), retain_i VARCHAR(255))",

        "INSERT INTO acl(username, topic, permission, action, qos_i, retain_i)"
        " VALUES('username', 't1', 'allow', 'publish', 1, 1)"
    ],
    query =>
        "SELECT permission, action, topic, qos_i as qos, retain_i as retain"
        " FROM acl WHERE username = ${username}",
    client_info => #{
        username => <<"username">>
    },
    checks => [
        {allow, ?PUBLISH_ACTION(1, true), <<"t1">>},
        {deny, ?PUBLISH_ACTION(1, false), <<"t1">>},
        {deny, ?PUBLISH_ACTION(0, true), <<"t1">>}
    ]
},
...

Here we explicitly check that retain flag stored as an integer is handled correctly.

Summary

🤖 Generated by Copilot at d49e101

This pull request enhances the authorization logic and features of the EMQ X broker by adding support for QoS and retain properties in the publish action, and by refactoring and improving the code quality and compatibility of the authorization modules and test suites. It also introduces a new placeholder for the retain flag, a new module for parsing and formatting raw rules, and some new macros and types for the action conditions. The pull request affects several files in the apps/emqx_authz and apps/emqx directories.

PR Checklist

  • Added tests for the changes
  • Changed lines covered in coverage report
  • Change log has been added to changes/{ce,ee}/(feat|perf|fix)-<PR-id>.en.md files
  • For internal contributor: there is a jira ticket to track this change
  • If there should be document changes, a PR to emqx-docs.git is sent, or a jira ticket is created to follow up: EMQX-10404
  • Schema changes are backward compatible

@savonarola savonarola force-pushed the 0531-authz-qos-retain branch 11 times, most recently from d7ded9c to 6711ed5 Compare June 28, 2023 09:39
@coveralls
Copy link
Collaborator

coveralls commented Jun 28, 2023

Pull Request Test Coverage Report for Build 5488638862

  • 246 of 255 (96.47%) changed or added relevant lines in 22 files are covered.
  • 28 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.1%) to 82.384%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/emqx_authz/src/emqx_authz.erl 1 2 50.0%
apps/emqx_authz/src/emqx_authz_mongodb.erl 7 8 87.5%
apps/emqx_authz/src/emqx_authz_mysql.erl 9 10 90.0%
apps/emqx_authz/src/emqx_authz_postgresql.erl 9 10 90.0%
apps/emqx_authz/src/emqx_authz_redis.erl 18 19 94.74%
apps/emqx_authz/src/emqx_authz_rule.erl 42 43 97.67%
apps/emqx_authz/src/emqx_authz_rule_raw.erl 68 69 98.55%
apps/emqx_gateway_coap/src/emqx_coap_pubsub_handler.erl 22 24 91.67%
Files with Coverage Reduction New Missed Lines %
apps/emqx_conf/src/emqx_conf_schema.erl 1 94.74%
apps/emqx_ft/src/emqx_ft_storage_exporter_fs.erl 1 80.43%
apps/emqx_gateway_exproto/src/emqx_exproto_gcli.erl 1 64.15%
apps/emqx/src/emqx_banned.erl 1 92.54%
apps/emqx_utils/src/emqx_utils.erl 1 82.3%
apps/emqx_gateway_mqttsn/src/emqx_mqttsn_frame.erl 2 64.04%
apps/emqx/src/emqx_channel.erl 2 87.69%
apps/emqx_oracle/src/emqx_oracle.erl 4 80.0%
apps/emqx/src/emqx_reason_codes.erl 15 86.03%
Totals Coverage Status
Change from base Build 5488419108: 0.1%
Covered Lines: 31062
Relevant Lines: 37704

💛 - Coveralls

@savonarola savonarola marked this pull request as ready for review June 28, 2023 10:48
@savonarola savonarola force-pushed the 0531-authz-qos-retain branch 4 times, most recently from 2855c7a to f88272d Compare July 4, 2023 13:55
Makefile Outdated Show resolved Hide resolved
thalesmg
thalesmg previously approved these changes Jul 5, 2023
Copy link
Contributor

@thalesmg thalesmg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still haven't gone through the tests, but looks good! 🍻

apps/emqx/src/emqx_channel.erl Outdated Show resolved Hide resolved
apps/emqx_authz/include/emqx_authz.hrl Outdated Show resolved Hide resolved
apps/emqx_authz/src/emqx_authz.erl Outdated Show resolved Hide resolved
apps/emqx_authz/src/emqx_authz_rule.erl Outdated Show resolved Hide resolved
@savonarola savonarola merged commit 12e237c into emqx:master Jul 10, 2023
138 checks passed
@savonarola savonarola deleted the 0531-authz-qos-retain branch July 10, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants