From d467289bb2b756530615ac1c8478958ee34d336d Mon Sep 17 00:00:00 2001 From: JimMoen Date: Fri, 20 Oct 2023 18:16:33 +0800 Subject: [PATCH 1/2] fix: avoid duplicated apikey from data import --- apps/emqx/test/emqx_common_test_http.erl | 8 +- .../src/emqx_mgmt_api_api_keys.erl | 1 + apps/emqx_management/src/emqx_mgmt_auth.erl | 96 +++++++++++++------ 3 files changed, 76 insertions(+), 29 deletions(-) diff --git a/apps/emqx/test/emqx_common_test_http.erl b/apps/emqx/test/emqx_common_test_http.erl index 7f50db92b..5a3286fee 100644 --- a/apps/emqx/test/emqx_common_test_http.erl +++ b/apps/emqx/test/emqx_common_test_http.erl @@ -31,6 +31,7 @@ ]). -define(DEFAULT_APP_ID, <<"default_appid">>). +-define(DEFAULT_APP_KEY, <<"default_app_key">>). -define(DEFAULT_APP_SECRET, <<"default_app_secret">>). request_api(Method, Url, Auth) -> @@ -90,7 +91,12 @@ create_default_app() -> Now = erlang:system_time(second), ExpiredAt = Now + timer:minutes(10), emqx_mgmt_auth:create( - ?DEFAULT_APP_ID, ?DEFAULT_APP_SECRET, true, ExpiredAt, <<"default app key for test">> + ?DEFAULT_APP_ID, + ?DEFAULT_APP_KEY, + ?DEFAULT_APP_SECRET, + true, + ExpiredAt, + <<"default app key for test">> ). delete_default_app() -> diff --git a/apps/emqx_management/src/emqx_mgmt_api_api_keys.erl b/apps/emqx_management/src/emqx_mgmt_api_api_keys.erl index 78bbef540..2aa74f2d5 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_api_keys.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_api_keys.erl @@ -192,6 +192,7 @@ api_key(post, #{body := App}) -> } = App, ExpiredAt = ensure_expired_at(App), Desc = unicode:characters_to_binary(Desc0, unicode), + %% create api_key with random api_key and api_secret from Dashboard case emqx_mgmt_auth:create(Name, Enable, ExpiredAt, Desc) of {ok, NewApp} -> {200, emqx_mgmt_auth:format(NewApp)}; diff --git a/apps/emqx_management/src/emqx_mgmt_auth.erl b/apps/emqx_management/src/emqx_mgmt_auth.erl index 3d32afc19..bce417dc4 100644 --- a/apps/emqx_management/src/emqx_mgmt_auth.erl +++ b/apps/emqx_management/src/emqx_mgmt_auth.erl @@ -43,12 +43,12 @@ -export([ do_update/4, do_delete/1, - do_create_app/3, - do_force_create_app/3 + do_create_app/1, + do_force_create_app/1 ]). -ifdef(TEST). --export([create/5]). +-export([create/6]). -endif. -define(APP, emqx_app). @@ -63,6 +63,8 @@ created_at = 0 :: integer() | '_' }). +-define(DEFAULT_HASH_LEN, 16). + mnesia(boot) -> ok = mria:create_table(?APP, [ {type, set}, @@ -97,11 +99,12 @@ init_bootstrap_file() -> create(Name, Enable, ExpiredAt, Desc) -> ApiSecret = generate_api_secret(), - create(Name, ApiSecret, Enable, ExpiredAt, Desc). + ApiKey = generate_unique_api_key(Name), + create(Name, ApiKey, ApiSecret, Enable, ExpiredAt, Desc). -create(Name, ApiSecret, Enable, ExpiredAt, Desc) -> +create(Name, ApiKey, ApiSecret, Enable, ExpiredAt, Desc) -> case mnesia:table_info(?APP, size) < 100 of - true -> create_app(Name, ApiSecret, Enable, ExpiredAt, Desc); + true -> create_app(Name, ApiKey, ApiSecret, Enable, ExpiredAt, Desc); false -> {error, "Maximum ApiKey"} end. @@ -202,7 +205,7 @@ to_map(#?APP{name = N, api_key = K, enable = E, expired_at = ET, created_at = CT is_expired(undefined) -> false; is_expired(ExpiredTime) -> ExpiredTime < erlang:system_time(second). -create_app(Name, ApiSecret, Enable, ExpiredAt, Desc) -> +create_app(Name, ApiKey, ApiSecret, Enable, ExpiredAt, Desc) -> App = #?APP{ name = Name, @@ -211,7 +214,7 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc) -> desc = Desc, created_at = erlang:system_time(second), api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), - api_key = list_to_binary(emqx_utils:gen_id(16)) + api_key = ApiKey }, case create_app(App) of {ok, Res} -> @@ -220,13 +223,13 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc) -> Error end. -create_app(App = #?APP{api_key = ApiKey, name = Name}) -> - trans(fun ?MODULE:do_create_app/3, [App, ApiKey, Name]). +create_app(App) -> + trans(fun ?MODULE:do_create_app/1, [App]). -force_create_app(NamePrefix, App = #?APP{api_key = ApiKey}) -> - trans(fun ?MODULE:do_force_create_app/3, [App, ApiKey, NamePrefix]). +force_create_app(App) -> + trans(fun ?MODULE:do_force_create_app/1, [App]). -do_create_app(App, ApiKey, Name) -> +do_create_app(App = #?APP{api_key = ApiKey, name = Name}) -> case mnesia:read(?APP, Name) of [_] -> mnesia:abort(name_already_existed); @@ -240,21 +243,56 @@ do_create_app(App, ApiKey, Name) -> end end. -do_force_create_app(App, ApiKey, NamePrefix) -> +do_force_create_app(App) -> + _ = maybe_cleanup_api_key(App), + ok = mnesia:write(App). + +maybe_cleanup_api_key(#?APP{name = Name, api_key = ApiKey}) -> case mnesia:match_object(?APP, #?APP{api_key = ApiKey, _ = '_'}, read) of [] -> - NewName = generate_unique_name(NamePrefix), - ok = mnesia:write(App#?APP{name = NewName}); + ok; [#?APP{name = Name}] -> - ok = mnesia:write(App#?APP{name = Name}) + ?SLOG(debug, #{ + msg => "same_apikey_detected", + info => <<"The last `KEY:SECRET` in bootstrap file will be used.">> + }), + ok; + [_App1] -> + ?SLOG(info, #{ + msg => "update_apikey_name_from_old_version", + info => <<"Update ApiKey name with new name rule, more information: xxx">> + }), + ok; + Existed -> + %% Duplicated or upgraded from old version: + %% Which `Name` and `ApiKey` are not related in old version. + %% So delete it/(them) and write a new record with a name strongly related to the apikey. + %% The apikeys generated from the file do not have names. + %% Generate a name for the apikey from the apikey itself by rule: + %% Use `from_bootstrap_file_` as the prefix, and the first 16 digits of the + %% sha512 hexadecimal value of the `ApiKey` as the suffix to form the name of the apikey. + %% e.g. The name of the apikey: `example-api-key:secret_xxxx` is `from_bootstrap_file_53280fb165b6cd37` + ?SLOG(info, #{ + msg => "duplicated_apikey_detected", + info => <<"Delete duplicated apikeys and write a new one from bootstrap file">> + }), + _ = lists:map( + fun(#?APP{name = N}) -> ok = mnesia:delete({?APP, N}) end, Existed + ), + ok end. -generate_unique_name(NamePrefix) -> - New = list_to_binary(NamePrefix ++ emqx_utils:gen_id(16)), - case mnesia:read(?APP, New) of - [] -> New; - _ -> generate_unique_name(NamePrefix) - end. +hash_string_from_seed(Seed, PrefixLen) -> + <> = crypto:hash(sha512, Seed), + list_to_binary(string:slice(io_lib:format("~128.16.0b", [Integer]), 0, PrefixLen)). + +%% Form Dashboard API Key pannel, only `Name` provided for users +generate_unique_api_key(Name) -> + hash_string_from_seed(Name, ?DEFAULT_HASH_LEN). + +%% Form BootStrap File, only `ApiKey` provided from file, no `Name` +generate_unique_name(NamePrefix, ApiKey) -> + <>. trans(Fun, Args) -> case mria:transaction(?COMMON_SHARD, Fun, Args) of @@ -300,22 +338,24 @@ init_bootstrap_file(File, Dev, MP) -> end. -define(BOOTSTRAP_TAG, <<"Bootstrapped From File">>). +-define(FROM_BOOTSTRAP_FILE_PREFIX, <<"from_bootstrap_file_">>). 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]]} -> + {match, [[ApiKey, ApiSecret]]} -> App = #?APP{ + name = generate_unique_name(?FROM_BOOTSTRAP_FILE_PREFIX, ApiKey), + api_key = ApiKey, + api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), enable = true, - expired_at = infinity, desc = ?BOOTSTRAP_TAG, created_at = erlang:system_time(second), - api_secret_hash = emqx_dashboard_admin:hash(ApiSecret), - api_key = AppKey + expired_at = infinity }, - case force_create_app("from_bootstrap_file_", App) of + case force_create_app(App) of {ok, ok} -> add_bootstrap_file(File, Dev, MP, Line + 1); {error, Reason} -> From e6576951ef7acae7022bc8e68a585b10c1ac8976 Mon Sep 17 00:00:00 2001 From: JimMoen Date: Fri, 20 Oct 2023 22:31:19 +0800 Subject: [PATCH 2/2] test: cleanup duplicated apikey with different name --- apps/emqx_management/src/emqx_mgmt_auth.erl | 2 + .../test/emqx_mgmt_api_api_keys_SUITE.erl | 104 ++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/apps/emqx_management/src/emqx_mgmt_auth.erl b/apps/emqx_management/src/emqx_mgmt_auth.erl index bce417dc4..afa8a8e2e 100644 --- a/apps/emqx_management/src/emqx_mgmt_auth.erl +++ b/apps/emqx_management/src/emqx_mgmt_auth.erl @@ -14,6 +14,7 @@ %% limitations under the License. %%-------------------------------------------------------------------- -module(emqx_mgmt_auth). +-include_lib("emqx_mgmt.hrl"). -include_lib("emqx/include/emqx.hrl"). -include_lib("emqx/include/logger.hrl"). @@ -49,6 +50,7 @@ -ifdef(TEST). -export([create/6]). +-export([trans/2, force_create_app/1]). -endif. -define(APP, emqx_app). 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 2a78f76fc..72e0c5218 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 @@ -19,6 +19,19 @@ -compile(nowarn_export_all). -include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). + +-define(APP, emqx_app). + +-record(?APP, { + name = <<>> :: binary() | '_', + api_key = <<>> :: binary() | '_', + api_secret_hash = <<>> :: binary() | '_', + enable = true :: boolean() | '_', + desc = <<>> :: binary() | '_', + expired_at = 0 :: integer() | undefined | infinity | '_', + created_at = 0 :: integer() | '_' +}). all() -> [{group, parallel}, {group, sequence}]. suite() -> [{timetrap, {minutes, 1}}]. @@ -72,6 +85,97 @@ t_bootstrap_file(_) -> update_file(<<>>), ok. +t_bootstrap_file_override(_) -> + TestPath = <<"/api/v5/status">>, + Bin = + <<"test-1:secret-1\ntest-1:duplicated-secret-1\ntest-2:secret-2\ntest-2:duplicated-secret-2">>, + File = "./bootstrap_api_keys.txt", + ok = file:write_file(File, Bin), + update_file(File), + + ?assertEqual(ok, emqx_mgmt_auth:init_bootstrap_file()), + + MatchFun = fun(ApiKey) -> mnesia:match_object(#?APP{api_key = ApiKey, _ = '_'}) end, + ?assertMatch( + {ok, [ + #?APP{ + name = <<"from_bootstrap_file_18926f94712af04e">>, + api_key = <<"test-1">> + } + ]}, + emqx_mgmt_auth:trans(MatchFun, [<<"test-1">>]) + ), + ?assertEqual(ok, emqx_mgmt_auth:authorize(TestPath, <<"test-1">>, <<"duplicated-secret-1">>)), + + ?assertMatch( + {ok, [ + #?APP{ + name = <<"from_bootstrap_file_de1c28a2e610e734">>, + api_key = <<"test-2">> + } + ]}, + emqx_mgmt_auth:trans(MatchFun, [<<"test-2">>]) + ), + ?assertEqual(ok, emqx_mgmt_auth:authorize(TestPath, <<"test-2">>, <<"duplicated-secret-2">>)), + ok. + +t_bootstrap_file_dup_override(_) -> + TestPath = <<"/api/v5/status">>, + TestApiKey = <<"test-1">>, + Bin = <<"test-1:secret-1">>, + File = "./bootstrap_api_keys.txt", + ok = file:write_file(File, Bin), + update_file(File), + ?assertEqual(ok, emqx_mgmt_auth:init_bootstrap_file()), + + SameAppWithDiffName = #?APP{ + name = <<"name-1">>, + api_key = <<"test-1">>, + api_secret_hash = emqx_dashboard_admin:hash(<<"duplicated-secret-1">>), + enable = true, + desc = <<"dup api key">>, + created_at = erlang:system_time(second), + expired_at = infinity + }, + WriteFun = fun(App) -> mnesia:write(App) end, + MatchFun = fun(ApiKey) -> mnesia:match_object(#?APP{api_key = ApiKey, _ = '_'}) end, + + ?assertEqual({ok, ok}, emqx_mgmt_auth:trans(WriteFun, [SameAppWithDiffName])), + %% as erlang term order + ?assertMatch( + {ok, [ + #?APP{ + name = <<"name-1">>, + api_key = <<"test-1">> + }, + #?APP{ + name = <<"from_bootstrap_file_18926f94712af04e">>, + api_key = <<"test-1">> + } + ]}, + emqx_mgmt_auth:trans(MatchFun, [TestApiKey]) + ), + + update_file(File), + + %% Similar to loading bootstrap file at node startup + %% the duplicated apikey in mnesia will be cleaned up + ?assertEqual(ok, emqx_mgmt_auth:init_bootstrap_file()), + ?assertMatch( + {ok, [ + #?APP{ + name = <<"from_bootstrap_file_18926f94712af04e">>, + api_key = <<"test-1">> + } + ]}, + emqx_mgmt_auth:trans(MatchFun, [<<"test-1">>]) + ), + + %% the last apikey in bootstrap file will override the all in mnesia and the previous one(s) in bootstrap file + ?assertEqual(ok, emqx_mgmt_auth:authorize(TestPath, <<"test-1">>, <<"secret-1">>)), + + ok. + update_file(File) -> ?assertMatch({ok, _}, emqx:update_config([<<"api_key">>], #{<<"bootstrap_file">> => File})).