From 1c2f9321d193974a514c20ca2a675e6c88a767b3 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 24 Oct 2023 14:45:04 +0700 Subject: [PATCH 1/4] feat(emqx): add file-sourced generic secrets These secrets follow the same `emqx_secret` convention of 0-arity functions. Also provide a simple HOCON schema module for use in application schemas. --- apps/emqx/src/emqx_schema_secret.erl | 107 ++++++++++++++++++ apps/emqx/src/emqx_secret.erl | 27 ++++- apps/emqx/test/emqx_secret_tests.erl | 69 +++++++++++ .../src/emqx_dashboard_swagger.erl | 3 + 4 files changed, 205 insertions(+), 1 deletion(-) create mode 100644 apps/emqx/src/emqx_schema_secret.erl create mode 100644 apps/emqx/test/emqx_secret_tests.erl diff --git a/apps/emqx/src/emqx_schema_secret.erl b/apps/emqx/src/emqx_schema_secret.erl new file mode 100644 index 000000000..aa2cbcc84 --- /dev/null +++ b/apps/emqx/src/emqx_schema_secret.erl @@ -0,0 +1,107 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +%% @doc HOCON schema that defines _secret_ concept. +-module(emqx_schema_secret). + +-include_lib("emqx/include/logger.hrl"). +-include_lib("typerefl/include/types.hrl"). + +-export([mk/1]). + +%% HOCON Schema API +-export([convert_secret/2]). + +%% Target of `emqx_secret:wrap/3` +-export([load/1]). + +%% @doc Secret value. +-type t() :: binary(). + +%% @doc Source of the secret value. +%% * "file://...": file path to a file containing secret value. +%% * other binaries: secret value itself. +-type source() :: iodata(). + +-type secret() :: binary() | function(). +-reflect_type([secret/0]). + +-define(SCHEMA, #{ + required => false, + format => <<"password">>, + sensitive => true, + converter => fun ?MODULE:convert_secret/2 +}). + +-dialyzer({nowarn_function, source/1}). + +%% + +-spec mk(#{atom() => _}) -> hocon_schema:field_schema(). +mk(Overrides = #{}) -> + hoconsc:mk(secret(), maps:merge(?SCHEMA, Overrides)). + +convert_secret(undefined, #{}) -> + undefined; +convert_secret(Secret, #{make_serializable := true}) -> + unicode:characters_to_binary(source(Secret)); +convert_secret(Secret, #{}) when is_function(Secret, 0) -> + Secret; +convert_secret(Secret, #{}) when is_integer(Secret) -> + wrap(integer_to_binary(Secret)); +convert_secret(Secret, #{}) -> + try unicode:characters_to_binary(Secret) of + String when is_binary(String) -> + wrap(String); + {error, _, _} -> + throw(invalid_string) + catch + error:_ -> + throw(invalid_type) + end. + +-spec wrap(source()) -> emqx_secret:t(t()). +wrap(Source) -> + try + _Secret = load(Source), + emqx_secret:wrap(?MODULE, load, Source) + catch + error:Reason -> + % NOTE: This should be a term serializable as JSON value. + throw(emqx_utils:format(Reason)) + end. + +-spec source(emqx_secret:t(t())) -> source(). +source(Secret) when is_function(Secret) -> + emqx_secret:term(Secret); +source(Secret) -> + Secret. + +%% + +-spec load(source()) -> t(). +load(<<"file://", Filename/binary>>) -> + load_file(Filename); +load(Secret) -> + Secret. + +load_file(Filename) -> + case file:read_file(Filename) of + {ok, Secret} -> + string:trim(Secret, trailing, [$\n]); + {error, Reason} -> + error({inaccessible_secret_file, Reason}, [Filename]) + end. diff --git a/apps/emqx/src/emqx_secret.erl b/apps/emqx/src/emqx_secret.erl index 72c4f3c08..ad0194201 100644 --- a/apps/emqx/src/emqx_secret.erl +++ b/apps/emqx/src/emqx_secret.erl @@ -19,7 +19,7 @@ -module(emqx_secret). %% API: --export([wrap/1, unwrap/1]). +-export([wrap/1, wrap/3, unwrap/1, term/1]). -export_type([t/1]). @@ -29,13 +29,38 @@ %% API funcions %%================================================================================ +%% @doc Wrap a term in a secret closure. +%% This effectively hides the term from any term formatting / printing code. +-spec wrap(T) -> t(T). wrap(Term) -> fun() -> Term end. +%% @doc Wrap a function call over a term in a secret closure. +%% This is slightly more flexible form of `wrap/1` with the same basic purpose. +-spec wrap(module(), atom(), _Term) -> t(_). +wrap(Module, Function, Term) -> + fun() -> + apply(Module, Function, [Term]) + end. + +%% @doc Unwrap a secret closure, revealing the secret. +%% This is either `Term` or `Module:Function(Term)` depending on how it was wrapped. +-spec unwrap(t(T)) -> T. unwrap(Term) when is_function(Term, 0) -> %% Handle potentially nested funs unwrap(Term()); unwrap(Term) -> Term. + +%% @doc Inspect the term wrapped in a secret closure. +-spec term(t(_)) -> _Term. +term(Wrap) when is_function(Wrap, 0) -> + case erlang:fun_info(Wrap, module) of + {module, ?MODULE} -> + {env, Env} = erlang:fun_info(Wrap, env), + lists:last(Env); + _ -> + error(badarg, [Wrap]) + end. diff --git a/apps/emqx/test/emqx_secret_tests.erl b/apps/emqx/test/emqx_secret_tests.erl new file mode 100644 index 000000000..ab0866c10 --- /dev/null +++ b/apps/emqx/test/emqx_secret_tests.erl @@ -0,0 +1,69 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_secret_tests). + +-export([ident/1]). + +-include_lib("eunit/include/eunit.hrl"). + +wrap_unwrap_test() -> + ?assertEqual( + 42, + emqx_secret:unwrap(emqx_secret:wrap(42)) + ). + +unwrap_immediate_test() -> + ?assertEqual( + 42, + emqx_secret:unwrap(42) + ). + +wrap_unwrap_external_test() -> + ?assertEqual( + ident({foo, bar}), + emqx_secret:unwrap(emqx_secret:wrap(?MODULE, ident, {foo, bar})) + ). + +wrap_unwrap_transform_test() -> + ?assertEqual( + <<"this_was_an_atom">>, + emqx_secret:unwrap(emqx_secret:wrap(erlang, atom_to_binary, this_was_an_atom)) + ). + +wrap_term_test() -> + ?assertEqual( + 42, + emqx_secret:term(emqx_secret:wrap(42)) + ). + +wrap_external_term_test() -> + ?assertEqual( + this_was_an_atom, + emqx_secret:term(emqx_secret:wrap(erlang, atom_to_binary, this_was_an_atom)) + ). + +external_fun_term_error_test() -> + Term = {foo, bar}, + ?assertError( + badarg, + emqx_secret:term(fun() -> Term end) + ). + +%% + +ident(X) -> + X. diff --git a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl index 75e93fdd1..f1759fb2d 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_swagger.erl @@ -908,6 +908,9 @@ typename_to_spec("port_number()", _Mod) -> range("1..65535"); typename_to_spec("secret_access_key()", _Mod) -> #{type => string, example => <<"TW8dPwmjpjJJuLW....">>}; +typename_to_spec("secret()", _Mod) -> + %% TODO: ideally, this should be dispatched to the module that defines this type + #{type => string, example => <<"R4ND0M/S∃CЯ∃T"/utf8>>}; typename_to_spec(Name, Mod) -> try_convert_to_spec(Name, Mod, [ fun try_remote_module_type/2, From 52f4519eebfc2a4df8790ffeb211d8fe66bf3c21 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Tue, 24 Oct 2023 14:52:25 +0700 Subject: [PATCH 2/4] feat(mqttbridge): support file-sourced secrets as passwords --- .../src/emqx_bridge_mqtt_connector.erl | 10 +- .../src/emqx_bridge_mqtt_connector_schema.erl | 8 +- .../test/emqx_bridge_mqtt_SUITE.erl | 183 +++++++++++------- .../test/emqx_bridge_mqtt_SUITE_data/password | 1 + 4 files changed, 126 insertions(+), 76 deletions(-) create mode 100644 apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE_data/password diff --git a/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector.erl b/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector.erl index eb81c4b6e..ff5f0c2f2 100644 --- a/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector.erl +++ b/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector.erl @@ -326,7 +326,7 @@ mk_client_opts( ], Config ), - Options#{ + mk_client_opt_password(Options#{ hosts => [HostPort], clientid => clientid(ResourceId, ClientScope, Config), connect_timeout => 30, @@ -334,7 +334,13 @@ mk_client_opts( force_ping => true, ssl => EnableSsl, ssl_opts => maps:to_list(maps:remove(enable, Ssl)) - }. + }). + +mk_client_opt_password(Options = #{password := Secret}) -> + %% TODO: Teach `emqtt` to accept 0-arity closures as passwords. + Options#{password := emqx_secret:unwrap(Secret)}; +mk_client_opt_password(Options) -> + Options. ms_to_s(Ms) -> erlang:ceil(Ms / 1000). diff --git a/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector_schema.erl b/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector_schema.erl index 1dc3ca5f8..eb298c5ff 100644 --- a/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector_schema.erl +++ b/apps/emqx_bridge_mqtt/src/emqx_bridge_mqtt_connector_schema.erl @@ -99,13 +99,9 @@ fields("server_configs") -> } )}, {password, - mk( - binary(), + emqx_schema_secret:mk( #{ - format => <<"password">>, - sensitive => true, - desc => ?DESC("password"), - converter => fun emqx_schema:password_converter/2 + desc => ?DESC("password") } )}, {clean_start, diff --git a/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl index bc0f2450a..6b5cc86da 100644 --- a/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl +++ b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl @@ -21,13 +21,15 @@ -import(emqx_dashboard_api_test_helpers, [request/4, uri/1]). -include("emqx/include/emqx.hrl"). +-include("emqx/include/emqx_hooks.hrl"). +-include("emqx/include/asserts.hrl"). -include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). -include_lib("snabbkaffe/include/snabbkaffe.hrl"). %% output functions -export([inspect/3]). --define(BRIDGE_CONF_DEFAULT, <<"bridges: {}">>). -define(TYPE_MQTT, <<"mqtt">>). -define(BRIDGE_NAME_INGRESS, <<"ingress_mqtt_bridge">>). -define(BRIDGE_NAME_EGRESS, <<"egress_mqtt_bridge">>). @@ -38,14 +40,18 @@ -define(EGRESS_REMOTE_TOPIC, "egress_remote_topic"). -define(EGRESS_LOCAL_TOPIC, "egress_local_topic"). --define(SERVER_CONF(Username), #{ +-define(SERVER_CONF, #{ + <<"type">> => ?TYPE_MQTT, <<"server">> => <<"127.0.0.1:1883">>, - <<"username">> => Username, - <<"password">> => <<"">>, <<"proto_ver">> => <<"v4">>, <<"ssl">> => #{<<"enable">> => false} }). +-define(SERVER_CONF(Username, Password), (?SERVER_CONF)#{ + <<"username">> => Username, + <<"password">> => Password +}). + -define(INGRESS_CONF, #{ <<"remote">> => #{ <<"topic">> => <>, @@ -129,43 +135,32 @@ suite() -> [{timetrap, {seconds, 30}}]. init_per_suite(Config) -> - _ = application:load(emqx_conf), - ok = emqx_common_test_helpers:start_apps( + Apps = emqx_cth_suite:start( [ - emqx_rule_engine, + emqx_conf, emqx_bridge, + emqx_rule_engine, emqx_bridge_mqtt, - emqx_dashboard + {emqx_dashboard, + "dashboard {" + "\n listeners.http { bind = 18083 }" + "\n default_username = connector_admin" + "\n default_password = public" + "\n }"} ], - fun set_special_configs/1 + #{work_dir => emqx_cth_suite:work_dir(Config)} ), - ok = emqx_common_test_helpers:load_config( - emqx_rule_engine_schema, - <<"rule_engine {rules {}}">> - ), - ok = emqx_common_test_helpers:load_config(emqx_bridge_schema, ?BRIDGE_CONF_DEFAULT), - Config. + [{suite_apps, Apps} | Config]. -end_per_suite(_Config) -> - emqx_common_test_helpers:stop_apps([ - emqx_dashboard, - emqx_bridge_mqtt, - emqx_bridge, - emqx_rule_engine - ]), - ok. - -set_special_configs(emqx_dashboard) -> - emqx_dashboard_api_test_helpers:set_default_config(<<"connector_admin">>); -set_special_configs(_) -> - ok. +end_per_suite(Config) -> + emqx_cth_suite:stop(?config(suite_apps, Config)). init_per_testcase(_, Config) -> - {ok, _} = emqx_cluster_rpc:start_link(node(), emqx_cluster_rpc, 1000), ok = snabbkaffe:start_trace(), Config. end_per_testcase(_, _Config) -> + ok = unhook_authenticate(), clear_resources(), snabbkaffe:stop(), ok. @@ -187,14 +182,84 @@ clear_resources() -> %%------------------------------------------------------------------------------ %% Testcases %%------------------------------------------------------------------------------ + +t_conf_bridge_authn_anonymous(_) -> + ok = hook_authenticate(), + {ok, 201, _Bridge} = request( + post, + uri(["bridges"]), + ?SERVER_CONF#{ + <<"name">> => <<"t_conf_bridge_anonymous">>, + <<"ingress">> => ?INGRESS_CONF#{<<"pool_size">> => 1} + } + ), + ?assertReceive( + {authenticate, #{username := undefined, password := undefined}} + ). + +t_conf_bridge_authn_password(_) -> + Username1 = <<"user1">>, + Password1 = <<"from-here">>, + ok = hook_authenticate(), + {ok, 201, _Bridge1} = request( + post, + uri(["bridges"]), + ?SERVER_CONF(Username1, Password1)#{ + <<"name">> => <<"t_conf_bridge_authn_password">>, + <<"ingress">> => ?INGRESS_CONF#{<<"pool_size">> => 1} + } + ), + ?assertReceive( + {authenticate, #{username := Username1, password := Password1}} + ). + +t_conf_bridge_authn_passfile(Config) -> + DataDir = ?config(data_dir, Config), + Username2 = <<"user2">>, + PasswordFilename = filename:join(DataDir, "password"), + Password2 = <<"from-there">>, + ok = hook_authenticate(), + {ok, 201, _Bridge2} = request( + post, + uri(["bridges"]), + ?SERVER_CONF(Username2, iolist_to_binary(["file://", PasswordFilename]))#{ + <<"name">> => <<"t_conf_bridge_authn_passfile">>, + <<"ingress">> => ?INGRESS_CONF#{<<"pool_size">> => 1} + } + ), + ?assertReceive( + {authenticate, #{username := Username2, password := Password2}} + ), + {ok, 400, #{<<"message">> := Message}} = request_json( + post, + uri(["bridges"]), + ?SERVER_CONF(<<>>, <<"file://im/pretty/sure/theres/no/such/file">>)#{ + <<"name">> => <<"t_conf_bridge_authn_no_passfile">> + } + ), + ?assertMatch( + #{<<"reason">> := <<"{inaccessible_secret_file,enoent}">>}, + emqx_utils_json:decode(Message) + ). + +hook_authenticate() -> + emqx_hooks:add('client.authenticate', {?MODULE, authenticate, [self()]}, ?HP_HIGHEST). + +unhook_authenticate() -> + emqx_hooks:del('client.authenticate', {?MODULE, authenticate}). + +authenticate(Credential, _, TestRunnerPid) -> + _ = TestRunnerPid ! {authenticate, Credential}, + ignore. + +%%------------------------------------------------------------------------------ + t_mqtt_conn_bridge_ingress(_) -> - User1 = <<"user1">>, %% create an MQTT bridge, using POST {ok, 201, Bridge} = request( post, uri(["bridges"]), - ServerConf = ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ServerConf = ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_INGRESS, <<"ingress">> => ?INGRESS_CONF } @@ -249,7 +314,6 @@ t_mqtt_conn_bridge_ingress(_) -> ok. t_mqtt_conn_bridge_ingress_full_context(_Config) -> - User1 = <<"user1">>, IngressConf = emqx_utils_maps:deep_merge( ?INGRESS_CONF, @@ -258,8 +322,7 @@ t_mqtt_conn_bridge_ingress_full_context(_Config) -> {ok, 201, _Bridge} = request( post, uri(["bridges"]), - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_INGRESS, <<"ingress">> => IngressConf } @@ -297,8 +360,7 @@ t_mqtt_conn_bridge_ingress_shared_subscription(_) -> Ns = lists:seq(1, 10), BridgeName = atom_to_binary(?FUNCTION_NAME), BridgeID = create_bridge( - ?SERVER_CONF(<<>>)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => BridgeName, <<"ingress">> => #{ <<"pool_size">> => PoolSize, @@ -337,8 +399,7 @@ t_mqtt_conn_bridge_ingress_shared_subscription(_) -> t_mqtt_egress_bridge_ignores_clean_start(_) -> BridgeName = atom_to_binary(?FUNCTION_NAME), BridgeID = create_bridge( - ?SERVER_CONF(<<"user1">>)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => BridgeName, <<"egress">> => ?EGRESS_CONF, <<"clean_start">> => false @@ -366,8 +427,7 @@ t_mqtt_egress_bridge_ignores_clean_start(_) -> t_mqtt_conn_bridge_ingress_downgrades_qos_2(_) -> BridgeName = atom_to_binary(?FUNCTION_NAME), BridgeID = create_bridge( - ?SERVER_CONF(<<"user1">>)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => BridgeName, <<"ingress">> => emqx_utils_maps:deep_merge( ?INGRESS_CONF, @@ -392,9 +452,8 @@ t_mqtt_conn_bridge_ingress_downgrades_qos_2(_) -> ok. t_mqtt_conn_bridge_ingress_no_payload_template(_) -> - User1 = <<"user1">>, BridgeIDIngress = create_bridge( - ?SERVER_CONF(User1)#{ + ?SERVER_CONF#{ <<"type">> => ?TYPE_MQTT, <<"name">> => ?BRIDGE_NAME_INGRESS, <<"ingress">> => ?INGRESS_CONF_NO_PAYLOAD_TEMPLATE @@ -428,10 +487,8 @@ t_mqtt_conn_bridge_ingress_no_payload_template(_) -> t_mqtt_conn_bridge_egress(_) -> %% then we add a mqtt connector, using POST - User1 = <<"user1">>, BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF } @@ -473,11 +530,8 @@ t_mqtt_conn_bridge_egress(_) -> t_mqtt_conn_bridge_egress_no_payload_template(_) -> %% then we add a mqtt connector, using POST - User1 = <<"user1">>, - BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF_NO_PAYLOAD_TEMPLATE } @@ -520,11 +574,9 @@ t_mqtt_conn_bridge_egress_no_payload_template(_) -> ok. t_egress_custom_clientid_prefix(_Config) -> - User1 = <<"user1">>, BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ + ?SERVER_CONF#{ <<"clientid_prefix">> => <<"my-custom-prefix">>, - <<"type">> => ?TYPE_MQTT, <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF } @@ -545,17 +597,14 @@ t_egress_custom_clientid_prefix(_Config) -> ok. t_mqtt_conn_bridge_ingress_and_egress(_) -> - User1 = <<"user1">>, BridgeIDIngress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_INGRESS, <<"ingress">> => ?INGRESS_CONF } ), BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF } @@ -627,8 +676,7 @@ t_mqtt_conn_bridge_ingress_and_egress(_) -> t_ingress_mqtt_bridge_with_rules(_) -> BridgeIDIngress = create_bridge( - ?SERVER_CONF(<<"user1">>)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_INGRESS, <<"ingress">> => ?INGRESS_CONF } @@ -712,8 +760,7 @@ t_ingress_mqtt_bridge_with_rules(_) -> t_egress_mqtt_bridge_with_rules(_) -> BridgeIDEgress = create_bridge( - ?SERVER_CONF(<<"user1">>)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF } @@ -789,10 +836,8 @@ t_egress_mqtt_bridge_with_rules(_) -> t_mqtt_conn_bridge_egress_reconnect(_) -> %% then we add a mqtt connector, using POST - User1 = <<"user1">>, BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF, <<"resource_opts">> => #{ @@ -897,10 +942,8 @@ t_mqtt_conn_bridge_egress_reconnect(_) -> ok. t_mqtt_conn_bridge_egress_async_reconnect(_) -> - User1 = <<"user1">>, BridgeIDEgress = create_bridge( - ?SERVER_CONF(User1)#{ - <<"type">> => ?TYPE_MQTT, + ?SERVER_CONF#{ <<"name">> => ?BRIDGE_NAME_EGRESS, <<"egress">> => ?EGRESS_CONF, <<"resource_opts">> => #{ @@ -1018,5 +1061,9 @@ request_bridge_metrics(BridgeID) -> {ok, 200, BridgeMetrics} = request(get, uri(["bridges", BridgeID, "metrics"]), []), emqx_utils_json:decode(BridgeMetrics). +request_json(Method, Url, Body) -> + {ok, Code, Response} = request(Method, Url, Body), + {ok, Code, emqx_utils_json:decode(Response)}. + request(Method, Url, Body) -> request(<<"connector_admin">>, Method, Url, Body). diff --git a/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE_data/password b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE_data/password new file mode 100644 index 000000000..d68418fda --- /dev/null +++ b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE_data/password @@ -0,0 +1 @@ +from-there From 44b4205561d63ac0440d776d040dba2d2c647de8 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Thu, 26 Oct 2023 14:37:14 +0700 Subject: [PATCH 3/4] fix(secret): do not treat missing file secrets as config error They are intended to be used mostly in the context of resources, which have their own feedback mechanism: statuses, retries, etc. Also turn the error into a throw exception, so that it can be interpreted as a regular error condition, for example by the resource manager. --- apps/emqx/src/emqx_schema_secret.erl | 15 ++++++-------- .../test/emqx_bridge_mqtt_SUITE.erl | 20 ++++++++++--------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/apps/emqx/src/emqx_schema_secret.erl b/apps/emqx/src/emqx_schema_secret.erl index aa2cbcc84..865d0eac9 100644 --- a/apps/emqx/src/emqx_schema_secret.erl +++ b/apps/emqx/src/emqx_schema_secret.erl @@ -75,14 +75,7 @@ convert_secret(Secret, #{}) -> -spec wrap(source()) -> emqx_secret:t(t()). wrap(Source) -> - try - _Secret = load(Source), - emqx_secret:wrap(?MODULE, load, Source) - catch - error:Reason -> - % NOTE: This should be a term serializable as JSON value. - throw(emqx_utils:format(Reason)) - end. + emqx_secret:wrap(?MODULE, load, Source). -spec source(emqx_secret:t(t())) -> source(). source(Secret) when is_function(Secret) -> @@ -103,5 +96,9 @@ load_file(Filename) -> {ok, Secret} -> string:trim(Secret, trailing, [$\n]); {error, Reason} -> - error({inaccessible_secret_file, Reason}, [Filename]) + throw(#{ + msg => failed_to_read_secret_file, + path => Filename, + reason => emqx_utils:explain_posix(Reason) + }) end. diff --git a/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl index 6b5cc86da..1776ae236 100644 --- a/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl +++ b/apps/emqx_bridge_mqtt/test/emqx_bridge_mqtt_SUITE.erl @@ -230,16 +230,18 @@ t_conf_bridge_authn_passfile(Config) -> ?assertReceive( {authenticate, #{username := Username2, password := Password2}} ), - {ok, 400, #{<<"message">> := Message}} = request_json( - post, - uri(["bridges"]), - ?SERVER_CONF(<<>>, <<"file://im/pretty/sure/theres/no/such/file">>)#{ - <<"name">> => <<"t_conf_bridge_authn_no_passfile">> - } - ), ?assertMatch( - #{<<"reason">> := <<"{inaccessible_secret_file,enoent}">>}, - emqx_utils_json:decode(Message) + {ok, 201, #{ + <<"status">> := <<"disconnected">>, + <<"status_reason">> := <<"#{msg => failed_to_read_secret_file", _/bytes>> + }}, + request_json( + post, + uri(["bridges"]), + ?SERVER_CONF(<<>>, <<"file://im/pretty/sure/theres/no/such/file">>)#{ + <<"name">> => <<"t_conf_bridge_authn_no_passfile">> + } + ) ). hook_authenticate() -> From d278486416d002d4ecc34f160c04c26c2c03f8d8 Mon Sep 17 00:00:00 2001 From: Andrew Mayorov Date: Mon, 30 Oct 2023 21:48:42 +0700 Subject: [PATCH 4/4] fix(secret): dedicate a specific loader module for file secrets To make code employing `emqx_secret` easier to follow. --- apps/emqx/src/emqx_schema_secret.erl | 33 +++++--------------- apps/emqx/src/emqx_secret.erl | 14 +++++---- apps/emqx/src/emqx_secret_loader.erl | 42 ++++++++++++++++++++++++++ apps/emqx/test/emqx_secret_tests.erl | 45 ++++++++++++++++------------ 4 files changed, 84 insertions(+), 50 deletions(-) create mode 100644 apps/emqx/src/emqx_secret_loader.erl diff --git a/apps/emqx/src/emqx_schema_secret.erl b/apps/emqx/src/emqx_schema_secret.erl index 865d0eac9..635285ce7 100644 --- a/apps/emqx/src/emqx_schema_secret.erl +++ b/apps/emqx/src/emqx_schema_secret.erl @@ -25,9 +25,6 @@ %% HOCON Schema API -export([convert_secret/2]). -%% Target of `emqx_secret:wrap/3` --export([load/1]). - %% @doc Secret value. -type t() :: binary(). @@ -74,31 +71,15 @@ convert_secret(Secret, #{}) -> end. -spec wrap(source()) -> emqx_secret:t(t()). -wrap(Source) -> - emqx_secret:wrap(?MODULE, load, Source). +wrap(<<"file://", Filename/binary>>) -> + emqx_secret:wrap_load({file, Filename}); +wrap(Secret) -> + emqx_secret:wrap(Secret). -spec source(emqx_secret:t(t())) -> source(). source(Secret) when is_function(Secret) -> - emqx_secret:term(Secret); + source(emqx_secret:term(Secret)); +source({file, Filename}) -> + <<"file://", Filename/binary>>; source(Secret) -> Secret. - -%% - --spec load(source()) -> t(). -load(<<"file://", Filename/binary>>) -> - load_file(Filename); -load(Secret) -> - Secret. - -load_file(Filename) -> - case file:read_file(Filename) of - {ok, Secret} -> - string:trim(Secret, trailing, [$\n]); - {error, Reason} -> - throw(#{ - msg => failed_to_read_secret_file, - path => Filename, - reason => emqx_utils:explain_posix(Reason) - }) - end. diff --git a/apps/emqx/src/emqx_secret.erl b/apps/emqx/src/emqx_secret.erl index ad0194201..dfbfa488e 100644 --- a/apps/emqx/src/emqx_secret.erl +++ b/apps/emqx/src/emqx_secret.erl @@ -19,12 +19,16 @@ -module(emqx_secret). %% API: --export([wrap/1, wrap/3, unwrap/1, term/1]). +-export([wrap/1, wrap_load/1, unwrap/1, term/1]). -export_type([t/1]). -opaque t(T) :: T | fun(() -> t(T)). +%% Secret loader module. +%% Any changes related to processing of secrets should be made there. +-define(LOADER, emqx_secret_loader). + %%================================================================================ %% API funcions %%================================================================================ @@ -37,12 +41,12 @@ wrap(Term) -> Term end. -%% @doc Wrap a function call over a term in a secret closure. +%% @doc Wrap a loader function call over a term in a secret closure. %% This is slightly more flexible form of `wrap/1` with the same basic purpose. --spec wrap(module(), atom(), _Term) -> t(_). -wrap(Module, Function, Term) -> +-spec wrap_load(emqx_secret_loader:source()) -> t(_). +wrap_load(Source) -> fun() -> - apply(Module, Function, [Term]) + apply(?LOADER, load, [Source]) end. %% @doc Unwrap a secret closure, revealing the secret. diff --git a/apps/emqx/src/emqx_secret_loader.erl b/apps/emqx/src/emqx_secret_loader.erl new file mode 100644 index 000000000..2e99587bf --- /dev/null +++ b/apps/emqx/src/emqx_secret_loader.erl @@ -0,0 +1,42 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_secret_loader). + +%% API +-export([load/1]). +-export([file/1]). + +-export_type([source/0]). + +-type source() :: {file, file:filename_all()}. + +-spec load(source()) -> binary() | no_return(). +load({file, Filename}) -> + file(Filename). + +-spec file(file:filename_all()) -> binary() | no_return(). +file(Filename) -> + case file:read_file(Filename) of + {ok, Secret} -> + string:trim(Secret, trailing); + {error, Reason} -> + throw(#{ + msg => failed_to_read_secret_file, + path => Filename, + reason => emqx_utils:explain_posix(Reason) + }) + end. diff --git a/apps/emqx/test/emqx_secret_tests.erl b/apps/emqx/test/emqx_secret_tests.erl index ab0866c10..cd6588c83 100644 --- a/apps/emqx/test/emqx_secret_tests.erl +++ b/apps/emqx/test/emqx_secret_tests.erl @@ -16,8 +16,6 @@ -module(emqx_secret_tests). --export([ident/1]). - -include_lib("eunit/include/eunit.hrl"). wrap_unwrap_test() -> @@ -32,16 +30,30 @@ unwrap_immediate_test() -> emqx_secret:unwrap(42) ). -wrap_unwrap_external_test() -> +wrap_unwrap_load_test_() -> + Secret = <<"foobaz">>, + { + setup, + fun() -> write_temp_file(Secret) end, + fun(Filename) -> file:delete(Filename) end, + fun(Filename) -> + ?_assertEqual( + Secret, + emqx_secret:unwrap(emqx_secret:wrap_load({file, Filename})) + ) + end + }. + +wrap_load_term_test() -> ?assertEqual( - ident({foo, bar}), - emqx_secret:unwrap(emqx_secret:wrap(?MODULE, ident, {foo, bar})) + {file, "no/such/file/i/swear"}, + emqx_secret:term(emqx_secret:wrap_load({file, "no/such/file/i/swear"})) ). -wrap_unwrap_transform_test() -> - ?assertEqual( - <<"this_was_an_atom">>, - emqx_secret:unwrap(emqx_secret:wrap(erlang, atom_to_binary, this_was_an_atom)) +wrap_unwrap_missing_file_test() -> + ?assertThrow( + #{msg := failed_to_read_secret_file, reason := "No such file or directory"}, + emqx_secret:unwrap(emqx_secret:wrap_load({file, "no/such/file/i/swear"})) ). wrap_term_test() -> @@ -50,12 +62,6 @@ wrap_term_test() -> emqx_secret:term(emqx_secret:wrap(42)) ). -wrap_external_term_test() -> - ?assertEqual( - this_was_an_atom, - emqx_secret:term(emqx_secret:wrap(erlang, atom_to_binary, this_was_an_atom)) - ). - external_fun_term_error_test() -> Term = {foo, bar}, ?assertError( @@ -63,7 +69,8 @@ external_fun_term_error_test() -> emqx_secret:term(fun() -> Term end) ). -%% - -ident(X) -> - X. +write_temp_file(Bytes) -> + Ts = erlang:system_time(millisecond), + Filename = filename:join("/tmp", ?MODULE_STRING ++ integer_to_list(-Ts)), + ok = file:write_file(Filename, Bytes), + Filename.