From 7a8a5926ab91a8475d9b49c96df2ea904772f2f0 Mon Sep 17 00:00:00 2001 From: firest Date: Tue, 24 Oct 2023 19:01:12 +0800 Subject: [PATCH 1/4] fix(rbac): adjust the role names --- apps/emqx/test/emqx_common_test_http.erl | 2 +- apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl | 6 +++--- apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/apps/emqx/test/emqx_common_test_http.erl b/apps/emqx/test/emqx_common_test_http.erl index 83cf02019..67473363e 100644 --- a/apps/emqx/test/emqx_common_test_http.erl +++ b/apps/emqx/test/emqx_common_test_http.erl @@ -34,7 +34,7 @@ -define(DEFAULT_APP_SECRET, <<"default_app_secret">>). %% from emqx_dashboard/include/emqx_dashboard_rbac.hrl --define(ROLE_API_SUPERUSER, <<"api_administrator">>). +-define(ROLE_API_SUPERUSER, <<"administrator">>). request_api(Method, Url, Auth) -> request_api(Method, Url, [], Auth, []). diff --git a/apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl b/apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl index 8f49464a4..386ae8bea 100644 --- a/apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl +++ b/apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl @@ -25,9 +25,9 @@ -define(ROLE_SUPERUSER, <<"administrator">>). -define(ROLE_DEFAULT, ?ROLE_SUPERUSER). --define(ROLE_API_VIEWER, <<"api_viewer">>). --define(ROLE_API_SUPERUSER, <<"api_administrator">>). --define(ROLE_API_PUBLISHER, <<"api_publisher">>). +-define(ROLE_API_VIEWER, <<"viewer">>). +-define(ROLE_API_SUPERUSER, <<"administrator">>). +-define(ROLE_API_PUBLISHER, <<"publisher">>). -define(ROLE_API_DEFAULT, ?ROLE_API_SUPERUSER). -endif. diff --git a/apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl index c98ccf676..c384b55e8 100644 --- a/apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl @@ -23,7 +23,7 @@ -include_lib("snabbkaffe/include/snabbkaffe.hrl"). -define(ROLE_SUPERUSER, <<"administrator">>). --define(ROLE_API_SUPERUSER, <<"api_administrator">>). +-define(ROLE_API_SUPERUSER, <<"administrator">>). -define(BOOTSTRAP_BACKUP, "emqx-export-test-bootstrap-ce.tar.gz"). all() -> From e175c213a19f0e48d2a8cdadf0bbfd4fe7add57c Mon Sep 17 00:00:00 2001 From: firest Date: Tue, 24 Oct 2023 22:28:53 +0800 Subject: [PATCH 2/4] fix(rbac): for compatibility with old data schema, extend the existing field as extra --- .../src/emqx_dashboard_rbac.erl | 8 +- apps/emqx_management/src/emqx_mgmt_auth.erl | 104 +++++------------- 2 files changed, 34 insertions(+), 78 deletions(-) diff --git a/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl b/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl index bde3be6e6..cd38540dd 100644 --- a/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl +++ b/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl @@ -59,12 +59,12 @@ valid_role(Type, Role) -> %% =================================================================== check_rbac(?ROLE_SUPERUSER, _, _, _) -> true; -check_rbac(?ROLE_API_SUPERUSER, _, _, _) -> - true; +%%check_rbac(?ROLE_API_SUPERUSER, _, _, _) -> +%% true; check_rbac(?ROLE_VIEWER, <<"GET">>, _, _) -> true; -check_rbac(?ROLE_API_VIEWER, <<"GET">>, _, _) -> - true; +%%check_rbac(?ROLE_API_VIEWER, <<"GET">>, _, _) -> +%% true; check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish">>, _) -> true; check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish/bulk">>, _) -> diff --git a/apps/emqx_management/src/emqx_mgmt_auth.erl b/apps/emqx_management/src/emqx_mgmt_auth.erl index 216fc636b..f0d92ece7 100644 --- a/apps/emqx_management/src/emqx_mgmt_auth.erl +++ b/apps/emqx_management/src/emqx_mgmt_auth.erl @@ -38,7 +38,7 @@ -export([authorize/4]). -export([post_config_update/5]). --export([backup_tables/0, validate_mnesia_backup/1, migrate_mnesia_backup/1]). +-export([backup_tables/0]). %% Internal exports (RPC) -export([ @@ -53,18 +53,17 @@ -endif. -define(APP, emqx_app). --type api_user_role() :: binary(). -record(?APP, { name = <<>> :: binary() | '_', api_key = <<>> :: binary() | '_', api_secret_hash = <<>> :: binary() | '_', enable = true :: boolean() | '_', - desc = <<>> :: binary() | '_', + %% Since v5.4.0 the `desc` has changed to `extra` + %% desc = <<>> :: binary() | '_', + extra = #{} :: binary() | map() | '_', expired_at = 0 :: integer() | undefined | infinity | '_', - created_at = 0 :: integer() | '_', - role = ?ROLE_DEFAULT :: api_user_role() | '_', - extra = #{} :: map() | '_' + created_at = 0 :: integer() | '_' }). mnesia(boot) -> @@ -75,8 +74,7 @@ mnesia(boot) -> {storage, disc_copies}, {record_name, ?APP}, {attributes, Fields} - ]), - maybe_migrate_table(Fields). + ]). %%-------------------------------------------------------------------- %% Data backup @@ -84,35 +82,6 @@ mnesia(boot) -> backup_tables() -> [?APP]. -validate_mnesia_backup({schema, _Tab, CreateList} = Schema) -> - case emqx_mgmt_data_backup:default_validate_mnesia_backup(Schema) of - ok -> - {ok, over}; - _ -> - case proplists:get_value(attributes, CreateList) of - [name, api_key, api_secret_hash, enable, desc, expired_at, created_at] -> - {ok, migrate}; - Fields -> - {error, {unknow_fields, Fields}} - end - end; -validate_mnesia_backup(_Other) -> - ok. - -migrate_mnesia_backup({schema, Tab, CreateList}) -> - case proplists:get_value(attributes, CreateList) of - [name, api_key, api_secret_hash, enable, desc, expired_at, created_at] = Fields -> - NewFields = Fields ++ [role, extra], - CreateList2 = lists:keyreplace( - attributes, 1, CreateList, {attributes, NewFields} - ), - {ok, {schema, Tab, CreateList2}}; - Fields -> - {error, {unknow_fields, Fields}} - end; -migrate_mnesia_backup(Data) -> - {ok, do_table_migrate(Data)}. - post_config_update([api_key], _Req, NewConf, _OldConf, _AppEnvs) -> #{bootstrap_file := File} = NewConf, case init_bootstrap_file(File) of @@ -158,13 +127,13 @@ do_update(Name, Enable, ExpiredAt, Desc, Role) -> case mnesia:read(?APP, Name, write) of [] -> mnesia:abort(not_found); - [App0 = #?APP{enable = Enable0, desc = Desc0}] -> + [App0 = #?APP{enable = Enable0, extra = Extra0}] -> + #{desc := Desc0} = Extra = normalize_extra(Extra0), App = App0#?APP{ expired_at = ExpiredAt, enable = ensure_not_undefined(Enable, Enable0), - desc = ensure_not_undefined(Desc, Desc0), - role = Role + extra = Extra#{desc := ensure_not_undefined(Desc, Desc0), role := Role} }, ok = mnesia:write(App), to_map(App) @@ -220,10 +189,10 @@ find_by_api_key(ApiKey) -> case mria:ro_transaction(?COMMON_SHARD, Fun) of {atomic, [ #?APP{ - api_secret_hash = SecretHash, enable = Enable, expired_at = ExpiredAt, role = Role + api_secret_hash = SecretHash, enable = Enable, expired_at = ExpiredAt, extra = Extra } ]} -> - {ok, Enable, ExpiredAt, SecretHash, Role}; + {ok, Enable, ExpiredAt, SecretHash, get_role(Extra)}; _ -> {error, "not_found"} end. @@ -234,15 +203,16 @@ ensure_not_undefined(New, _Old) -> New. to_map(Apps) when is_list(Apps) -> [to_map(App) || App <- Apps]; to_map(#?APP{ - name = N, api_key = K, enable = E, expired_at = ET, created_at = CT, desc = D, role = Role + name = N, api_key = K, enable = E, expired_at = ET, created_at = CT, extra = Extra0 }) -> + #{role := Role, desc := Desc} = normalize_extra(Extra0), #{ name => N, api_key => K, enable => E, expired_at => ET, created_at => CT, - desc => D, + desc => Desc, expired => is_expired(ET), role => Role }. @@ -256,11 +226,10 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc, Role) -> name = Name, enable = Enable, expired_at = ExpiredAt, - desc = Desc, + extra = #{desc => Desc, role => Role}, created_at = erlang:system_time(second), api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), - api_key = list_to_binary(emqx_utils:gen_id(16)), - role = Role + api_key = list_to_binary(emqx_utils:gen_id(16)) }, case create_app(App) of {ok, Res} -> @@ -269,7 +238,7 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc, Role) -> Error end. -create_app(App = #?APP{api_key = ApiKey, name = Name, role = Role}) -> +create_app(App = #?APP{api_key = ApiKey, name = Name, extra = #{role := Role}}) -> case valid_role(Role) of ok -> trans(fun ?MODULE:do_create_app/3, [App, ApiKey, Name]); @@ -364,7 +333,7 @@ add_bootstrap_file(File, Dev, MP, Line) -> #?APP{ enable = true, expired_at = infinity, - desc = ?BOOTSTRAP_TAG, + extra = #{desc => ?BOOTSTRAP_TAG, role => ?ROLE_API_DEFAULT}, created_at = erlang:system_time(second), api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), api_key = AppKey @@ -395,6 +364,18 @@ add_bootstrap_file(File, Dev, MP, Line) -> throw(#{file => File, line => Line, reason => Reason}) end. +get_role(#{role := Role}) -> + Role; +%% Before v5.4.0, +%% the field in the position of the `extra` is `desc` which is a binary for description +get_role(_Desc) -> + ?ROLE_API_DEFAULT. + +normalize_extra(Map) when is_map(Map) -> + Map; +normalize_extra(Desc) -> + #{desc => Desc, role => ?ROLE_API_DEFAULT}. + -if(?EMQX_RELEASE_EDITION == ee). check_rbac(Req, ApiKey, Role) -> case emqx_dashboard_rbac:check_rbac(Req, ApiKey, Role) of @@ -424,28 +405,3 @@ valid_role(_) -> {error, <<"Role does not exist">>}. -endif. - -maybe_migrate_table(Fields) -> - case mnesia:table_info(?APP, attributes) =:= Fields of - true -> - ok; - false -> - TransFun = fun do_table_migrate/1, - {atomic, ok} = mnesia:transform_table(?APP, TransFun, Fields, ?APP), - ok - end. - -do_table_migrate({?APP, Name, Key, Hash, Enable, Desc, ExpiredAt, CreatedAt}) -> - #?APP{ - name = Name, - api_key = Key, - api_secret_hash = Hash, - enable = Enable, - desc = Desc, - expired_at = ExpiredAt, - created_at = CreatedAt, - role = ?ROLE_API_DEFAULT, - extra = #{} - }; -do_table_migrate(#?APP{} = App) -> - App. From ec4147963311064c6dd365446f89da3758cbb5d3 Mon Sep 17 00:00:00 2001 From: firest Date: Tue, 24 Oct 2023 23:03:18 +0800 Subject: [PATCH 3/4] feat(rbac): supports setting role in API bootstrap file --- apps/emqx_management/src/emqx_mgmt_auth.erl | 26 ++++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_auth.erl b/apps/emqx_management/src/emqx_mgmt_auth.erl index f0d92ece7..bdb5d97fa 100644 --- a/apps/emqx_management/src/emqx_mgmt_auth.erl +++ b/apps/emqx_management/src/emqx_mgmt_auth.erl @@ -297,7 +297,7 @@ init_bootstrap_file(<<>>) -> init_bootstrap_file(File) -> case file:open(File, [read, binary]) of {ok, Dev} -> - {ok, MP} = re:compile(<<"(\.+):(\.+$)">>, [ungreedy]), + {ok, MP} = re:compile(<<"(\.+):(\.+)(?::(\.+))?$">>, [ungreedy]), init_bootstrap_file(File, Dev, MP); {error, Reason0} -> Reason = emqx_utils:explain_posix(Reason0), @@ -327,13 +327,13 @@ init_bootstrap_file(File, Dev, MP) -> add_bootstrap_file(File, Dev, MP, Line) -> case file:read_line(Dev) of {ok, Bin} -> - case re:run(Bin, MP, [global, {capture, all_but_first, binary}]) of - {match, [[AppKey, ApiSecret]]} -> + case parse_bootstrap_line(Bin, MP) of + {ok, [AppKey, ApiSecret, Role]} -> App = #?APP{ enable = true, expired_at = infinity, - extra = #{desc => ?BOOTSTRAP_TAG, role => ?ROLE_API_DEFAULT}, + extra = #{desc => ?BOOTSTRAP_TAG, role => Role}, created_at = erlang:system_time(second), api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), api_key = AppKey @@ -344,8 +344,7 @@ add_bootstrap_file(File, Dev, MP, Line) -> {error, Reason} -> throw(#{file => File, line => Line, content => Bin, reason => Reason}) end; - _ -> - Reason = "invalid_format", + {error, Reason} -> ?SLOG( error, #{ @@ -364,6 +363,21 @@ add_bootstrap_file(File, Dev, MP, Line) -> throw(#{file => File, line => Line, reason => Reason}) end. +parse_bootstrap_line(Bin, MP) -> + case re:run(Bin, MP, [global, {capture, all_but_first, binary}]) of + {match, [[_AppKey, _ApiSecret] = Args]} -> + {ok, Args ++ [?ROLE_API_DEFAULT]}; + {match, [[_AppKey, _ApiSecret, Role] = Args]} -> + case valid_role(Role) of + ok -> + {ok, Args}; + _Error -> + {error, {"invalid_role", Role}} + end; + _ -> + {error, "invalid_format"} + end. + get_role(#{role := Role}) -> Role; %% Before v5.4.0, From 4ba34f8f3eaa0756296e5fbd1f11074d47fb8fc5 Mon Sep 17 00:00:00 2001 From: firest Date: Wed, 25 Oct 2023 12:02:40 +0800 Subject: [PATCH 4/4] chore(rbac): fix CI errors & update change --- .../src/emqx_dashboard_rbac.erl | 4 - apps/emqx_management/src/emqx_mgmt_auth.erl | 18 +++- .../test/emqx_mgmt_api_api_keys_SUITE.erl | 88 ++++++++++++++++++- changes/ee/feat-11811.en.md | 5 ++ rel/i18n/emqx_mgmt_api_key_schema.hocon | 7 +- 5 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 changes/ee/feat-11811.en.md diff --git a/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl b/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl index cd38540dd..7b8ffef02 100644 --- a/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl +++ b/apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl @@ -59,12 +59,8 @@ valid_role(Type, Role) -> %% =================================================================== check_rbac(?ROLE_SUPERUSER, _, _, _) -> true; -%%check_rbac(?ROLE_API_SUPERUSER, _, _, _) -> -%% true; check_rbac(?ROLE_VIEWER, <<"GET">>, _, _) -> true; -%%check_rbac(?ROLE_API_VIEWER, <<"GET">>, _, _) -> -%% true; check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish">>, _) -> true; check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish/bulk">>, _) -> diff --git a/apps/emqx_management/src/emqx_mgmt_auth.erl b/apps/emqx_management/src/emqx_mgmt_auth.erl index bdb5d97fa..fa48ccfea 100644 --- a/apps/emqx_management/src/emqx_mgmt_auth.erl +++ b/apps/emqx_management/src/emqx_mgmt_auth.erl @@ -38,7 +38,7 @@ -export([authorize/4]). -export([post_config_update/5]). --export([backup_tables/0]). +-export([backup_tables/0, validate_mnesia_backup/1]). %% Internal exports (RPC) -export([ @@ -82,6 +82,22 @@ mnesia(boot) -> backup_tables() -> [?APP]. +validate_mnesia_backup({schema, _Tab, CreateList} = Schema) -> + case emqx_mgmt_data_backup:default_validate_mnesia_backup(Schema) of + ok -> + ok; + _ -> + case proplists:get_value(attributes, CreateList) of + %% Since v5.4.0 the `desc` has changed to `extra` + [name, api_key, api_secret_hash, enable, desc, expired_at, created_at] -> + ok; + Fields -> + {error, {unknow_fields, Fields}} + end + end; +validate_mnesia_backup(_Other) -> + ok. + post_config_update([api_key], _Req, NewConf, _OldConf, _AppEnvs) -> #{bootstrap_file := File} = NewConf, case init_bootstrap_file(File) of diff --git a/apps/emqx_management/test/emqx_mgmt_api_api_keys_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_api_keys_SUITE.erl index 8243b18ff..bebf8e338 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_api_keys_SUITE.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_api_keys_SUITE.erl @@ -39,7 +39,7 @@ groups() -> [ {parallel, [parallel], [t_create, t_update, t_delete, t_authorize, t_create_unexpired_app]}, {parallel, [parallel], ?EE_CASES}, - {sequence, [], [t_bootstrap_file, t_create_failed]} + {sequence, [], [t_bootstrap_file, t_bootstrap_file_with_role, t_create_failed]} ]. init_per_suite(Config) -> @@ -86,6 +86,92 @@ t_bootstrap_file(_) -> update_file(<<>>), ok. +-if(?EMQX_RELEASE_EDITION == ee). +t_bootstrap_file_with_role(_) -> + Search = fun(Name) -> + lists:search( + fun(#{api_key := AppName}) -> + AppName =:= Name + end, + emqx_mgmt_auth:list() + ) + end, + + Bin = <<"role-1:role-1:viewer\nrole-2:role-2:administrator\nrole-3:role-3">>, + File = "./bootstrap_api_keys.txt", + ok = file:write_file(File, Bin), + update_file(File), + + ?assertMatch( + {value, #{api_key := <<"role-1">>, role := <<"viewer">>}}, + Search(<<"role-1">>) + ), + + ?assertMatch( + {value, #{api_key := <<"role-2">>, role := <<"administrator">>}}, + Search(<<"role-2">>) + ), + + ?assertMatch( + {value, #{api_key := <<"role-3">>, role := <<"administrator">>}}, + Search(<<"role-3">>) + ), + + %% bad role + BadBin = <<"role-4:secret-11:bad\n">>, + ok = file:write_file(File, BadBin), + update_file(File), + ?assertEqual( + false, + Search(<<"role-4">>) + ), + ok. +-else. +t_bootstrap_file_with_role(_) -> + Search = fun(Name) -> + lists:search( + fun(#{api_key := AppName}) -> + AppName =:= Name + end, + emqx_mgmt_auth:list() + ) + end, + + Bin = <<"role-1:role-1:administrator\nrole-2:role-2">>, + File = "./bootstrap_api_keys.txt", + ok = file:write_file(File, Bin), + update_file(File), + + ?assertMatch( + {value, #{api_key := <<"role-1">>, role := <<"administrator">>}}, + Search(<<"role-1">>) + ), + + ?assertMatch( + {value, #{api_key := <<"role-2">>, role := <<"administrator">>}}, + Search(<<"role-2">>) + ), + + %% only administrator + OtherRoleBin = <<"role-3:role-3:viewer\n">>, + ok = file:write_file(File, OtherRoleBin), + update_file(File), + ?assertEqual( + false, + Search(<<"role-3">>) + ), + + %% bad role + BadBin = <<"role-4:secret-11:bad\n">>, + ok = file:write_file(File, BadBin), + update_file(File), + ?assertEqual( + false, + Search(<<"role-4">>) + ), + ok. +-endif. + auth_authorize(Path, Key, Secret) -> FakePath = erlang:list_to_binary(emqx_dashboard_swagger:relative_uri("/fake")), FakeReq = #{method => <<"GET">>, path => FakePath}, diff --git a/changes/ee/feat-11811.en.md b/changes/ee/feat-11811.en.md new file mode 100644 index 000000000..91b9e90aa --- /dev/null +++ b/changes/ee/feat-11811.en.md @@ -0,0 +1,5 @@ +Improve the format for the REST API key bootstrap file to support initialize key with a role. + +The new form is:`api_key:api_secret:role`. + +`role` is optional and its default value is `administrator`. diff --git a/rel/i18n/emqx_mgmt_api_key_schema.hocon b/rel/i18n/emqx_mgmt_api_key_schema.hocon index 811ab8a98..e72682747 100644 --- a/rel/i18n/emqx_mgmt_api_key_schema.hocon +++ b/rel/i18n/emqx_mgmt_api_key_schema.hocon @@ -9,8 +9,11 @@ api_key.label: bootstrap_file.desc: """The bootstrap file provides API keys for EMQX. EMQX will load these keys on startup to authorize API requests. -It contains key-value pairs in the format:`api_key:api_secret`. -Each line specifies an API key and its associated secret.""" +It contains colon-separated values in the format: `api_key:api_secret:role`. +Each line specifies an API key and its associated secret, and the role of this key. +The 'role' part should be the pre-defined access scope group name, +for example, `administrator` or `viewer`. +The 'role' is introduced in 5.4, to be backward compatible, if it is missing, the key is implicitly granted `administrator` role.""" bootstrap_file.label: """Initialize api_key file."""