Merge pull request #11540 from thalesmg/fix-cert-path-utf8-r52-20230829

fix(bridge): validate bridge name before attempting to convert certificates

apps/emqx_bridge/src/emqx_bridge.erl

@@ -55,6 +55,7 @@
 ]).
 
 -export([config_key_path/0]).
+-export([validate_bridge_name/1]).
 
 %% exported for `emqx_telemetry'
 -export([get_basic_usage_info/0]).
@@ -96,6 +97,9 @@
 
 -define(ROOT_KEY, bridges).
 
+%% See `hocon_tconf`
+-define(MAP_KEY_RE, <<"^[A-Za-z0-9]+[A-Za-z0-9-_]*$">>).
+
 load() ->
     Bridges = emqx:get_config([?ROOT_KEY], #{}),
     lists:foreach(
@@ -580,3 +584,19 @@ get_basic_usage_info() ->
         _:_ ->
             InitialAcc
     end.
+
+validate_bridge_name(BridgeName0) ->
+    BridgeName = to_bin(BridgeName0),
+    case re:run(BridgeName, ?MAP_KEY_RE, [{capture, none}]) of
+        match ->
+            ok;
+        nomatch ->
+            {error, #{
+                kind => validation_error,
+                reason => bad_bridge_name,
+                value => BridgeName
+            }}
+    end.
+
+to_bin(A) when is_atom(A) -> atom_to_binary(A, utf8);
+to_bin(B) when is_binary(B) -> B.

apps/emqx_bridge/src/emqx_bridge_api.erl

@@ -609,6 +609,8 @@ create_or_update_bridge(BridgeType, BridgeName, Conf, HttpStatusCode) ->
     case emqx_bridge:create(BridgeType, BridgeName, Conf) of
         {ok, _} ->
             lookup_from_all_nodes(BridgeType, BridgeName, HttpStatusCode);
+        {error, {pre_config_update, _HandlerMod, Reason}} when is_map(Reason) ->
+            ?BAD_REQUEST(map_to_json(redact(Reason)));
         {error, Reason} when is_map(Reason) ->
             ?BAD_REQUEST(map_to_json(redact(Reason)))
     end.

apps/emqx_bridge/src/emqx_bridge_app.erl

@@ -62,11 +62,16 @@ pre_config_update(_, {Oper, _Type, _Name}, OldConfig) ->
     %% to save the 'enable' to the config files
     {ok, OldConfig#{<<"enable">> => operation_to_enable(Oper)}};
 pre_config_update(Path, Conf, _OldConfig) when is_map(Conf) ->
+    case validate_bridge_name(Path) of
+        ok ->
             case emqx_connector_ssl:convert_certs(filename:join(Path), Conf) of
                 {error, Reason} ->
                     {error, Reason};
                 {ok, ConfNew} ->
                     {ok, ConfNew}
+            end;
+        Error ->
+            Error
     end.
 
 post_config_update([bridges, BridgeType, BridgeName], '$remove', _, _OldConf, _AppEnvs) ->
@@ -97,3 +102,12 @@ post_config_update([bridges, BridgeType, BridgeName], _Req, NewConf, OldConf, _A
 %% internal functions
 operation_to_enable(disable) -> false;
 operation_to_enable(enable) -> true.
+
+validate_bridge_name(Path) ->
+    [RootKey] = emqx_bridge:config_key_path(),
+    case Path of
+        [RootKey, _BridgeType, BridgeName] ->
+            emqx_bridge:validate_bridge_name(BridgeName);
+        _ ->
+            ok
+    end.

apps/emqx_bridge/test/emqx_bridge_SUITE.erl

@@ -26,19 +26,20 @@ all() ->
     emqx_common_test_helpers:all(?MODULE).
 
 init_per_suite(Config) ->
-    _ = application:load(emqx_conf),
-    %% to avoid inter-suite dependencies
-    application:stop(emqx_connector),
-    ok = emqx_common_test_helpers:start_apps([emqx, emqx_bridge]),
-    Config.
-
-end_per_suite(_Config) ->
-    emqx_common_test_helpers:stop_apps([
+    Apps = emqx_cth_suite:start(
+        [
             emqx,
-        emqx_bridge,
-        emqx_resource,
-        emqx_connector
-    ]).
+            emqx_conf,
+            emqx_bridge
+        ],
+        #{work_dir => ?config(priv_dir, Config)}
+    ),
+    [{apps, Apps} | Config].
+
+end_per_suite(Config) ->
+    Apps = ?config(apps, Config),
+    ok = emqx_cth_suite:stop(Apps),
+    ok.
 
 init_per_testcase(t_get_basic_usage_info_1, Config) ->
     {ok, _} = emqx_cluster_rpc:start_link(node(), emqx_cluster_rpc, 1000),
@@ -180,6 +181,31 @@ t_update_ssl_conf(Config) ->
     ?assertMatch({error, enoent}, file:list_dir(CertDir)),
     ok.
 
+t_create_with_bad_name(_Config) ->
+    Path = [bridges, mqtt, 'test_哈哈'],
+    Conf = #{
+        <<"bridge_mode">> => false,
+        <<"clean_start">> => true,
+        <<"keepalive">> => <<"60s">>,
+        <<"proto_ver">> => <<"v4">>,
+        <<"server">> => <<"127.0.0.1:1883">>,
+        <<"ssl">> =>
+            #{
+                %% needed to trigger pre_config_update
+                <<"certfile">> => cert_file("certfile"),
+                <<"enable">> => true
+            }
+    },
+    ?assertMatch(
+        {error,
+            {pre_config_update, emqx_bridge_app, #{
+                reason := bad_bridge_name,
+                kind := validation_error
+            }}},
+        emqx:update_config(Path, Conf)
+    ),
+    ok.
+
 data_file(Name) ->
     Dir = code:lib_dir(emqx_bridge, test),
     {ok, Bin} = file:read_file(filename:join([Dir, "data", Name])),

apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl

@@ -1335,6 +1335,35 @@ t_cluster_later_join_metrics(Config) ->
     ),
     ok.
 
+t_create_with_bad_name(Config) ->
+    Port = ?config(port, Config),
+    URL1 = ?URL(Port, "path1"),
+    Name = <<"test_哈哈">>,
+    BadBridgeParams =
+        emqx_utils_maps:deep_merge(
+            ?HTTP_BRIDGE(URL1, Name),
+            #{
+                <<"ssl">> =>
+                    #{
+                        <<"enable">> => true,
+                        <<"certfile">> => cert_file("certfile")
+                    }
+            }
+        ),
+    {ok, 400, #{
+        <<"code">> := <<"BAD_REQUEST">>,
+        <<"message">> := Msg0
+    }} =
+        request_json(
+            post,
+            uri(["bridges"]),
+            BadBridgeParams,
+            Config
+        ),
+    Msg = emqx_utils_json:decode(Msg0, [return_maps]),
+    ?assertMatch(#{<<"reason">> := <<"bad_bridge_name">>}, Msg),
+    ok.
+
 validate_resource_request_ttl(single, Timeout, Name) ->
     SentData = #{payload => <<"Hello EMQX">>, timestamp => 1668602148000},
     BridgeID = emqx_bridge_resource:bridge_id(?BRIDGE_TYPE_HTTP, Name),
@@ -1418,3 +1447,11 @@ str(S) when is_binary(S) -> binary_to_list(S).
 
 json(B) when is_binary(B) ->
     emqx_utils_json:decode(B, [return_maps]).
+
+data_file(Name) ->
+    Dir = code:lib_dir(emqx_bridge, test),
+    {ok, Bin} = file:read_file(filename:join([Dir, "data", Name])),
+    Bin.
+
+cert_file(Name) ->
+    data_file(filename:join(["certs", Name])).

changes/ce/fix-11540.en.md

@@ -0,0 +1 @@
+Improved HTTP response when attempting to create a bridge with an invalid name.