Merge pull request #10556 from sstrigler/wrap_auth_headers

fix(emqx_connector_http): wrap and unwrap auth headers

apps/emqx_connector/src/emqx_connector_http.erl

@@ -473,7 +473,7 @@ preprocess_request(
         method => emqx_plugin_libs_rule:preproc_tmpl(to_bin(Method)),
         path => emqx_plugin_libs_rule:preproc_tmpl(Path),
         body => maybe_preproc_tmpl(body, Req),
-        headers => preproc_headers(Headers),
+        headers => wrap_auth_header(preproc_headers(Headers)),
         request_timeout => maps:get(request_timeout, Req, 30000),
         max_retries => maps:get(max_retries, Req, 2)
     }.
@@ -503,6 +503,36 @@ preproc_headers(Headers) when is_list(Headers) ->
         Headers
     ).
 
+wrap_auth_header(Headers) ->
+    lists:map(fun maybe_wrap_auth_header/1, Headers).
+
+maybe_wrap_auth_header({[{str, Key}] = StrKey, Val}) ->
+    {_, MaybeWrapped} = maybe_wrap_auth_header({Key, Val}),
+    {StrKey, MaybeWrapped};
+maybe_wrap_auth_header({Key, Val} = Header) when
+    is_binary(Key), (size(Key) =:= 19 orelse size(Key) =:= 13)
+->
+    %% We check the size of potential keys in the guard above and consider only
+    %% those that match the number of characters of either "Authorization" or
+    %% "Proxy-Authorization".
+    case try_bin_to_lower(Key) of
+        <<"authorization">> ->
+            {Key, emqx_secret:wrap(Val)};
+        <<"proxy-authorization">> ->
+            {Key, emqx_secret:wrap(Val)};
+        _Other ->
+            Header
+    end;
+maybe_wrap_auth_header(Header) ->
+    Header.
+
+try_bin_to_lower(Bin) ->
+    try iolist_to_binary(string:lowercase(Bin)) of
+        LowercaseBin -> LowercaseBin
+    catch
+        _:_ -> Bin
+    end.
+
 maybe_preproc_tmpl(Key, Conf) ->
     case maps:get(Key, Conf, undefined) of
         undefined -> undefined;
@@ -537,7 +567,7 @@ proc_headers(HeaderTks, Msg) ->
         fun({K, V}) ->
             {
                 emqx_plugin_libs_rule:proc_tmpl(K, Msg),
-                emqx_plugin_libs_rule:proc_tmpl(V, Msg)
+                emqx_plugin_libs_rule:proc_tmpl(emqx_secret:unwrap(V), Msg)
             }
         end,
         HeaderTks
@@ -628,21 +658,13 @@ is_sensitive_key([{str, StringKey}]) ->
 is_sensitive_key(Atom) when is_atom(Atom) ->
     is_sensitive_key(erlang:atom_to_binary(Atom));
 is_sensitive_key(Bin) when is_binary(Bin), (size(Bin) =:= 19 orelse size(Bin) =:= 13) ->
-    try
-        %% This is wrapped in a try-catch since we don't know that Bin is a
-        %% valid string so string:lowercase/1 might throw an exception.
-        %%
-        %% We want to convert this to lowercase since the http header fields
-        %% are case insensitive, which means that a user of the Webhook bridge
-        %% can write this field name in many different ways.
-        LowercaseBin = iolist_to_binary(string:lowercase(Bin)),
-        case LowercaseBin of
-            <<"authorization">> -> true;
-            <<"proxy-authorization">> -> true;
-            _ -> false
-        end
-    catch
-        _:_ -> false
+    %% We want to convert this to lowercase since the http header fields
+    %% are case insensitive, which means that a user of the Webhook bridge
+    %% can write this field name in many different ways.
+    case try_bin_to_lower(Bin) of
+        <<"authorization">> -> true;
+        <<"proxy-authorization">> -> true;
+        _ -> false
     end;
 is_sensitive_key(_) ->
     false.

apps/emqx_connector/test/emqx_connector_http_tests.erl

@@ -0,0 +1,90 @@
+%%--------------------------------------------------------------------
+%% 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_connector_http_tests).
+
+-include_lib("eunit/include/eunit.hrl").
+
+-define(MY_SECRET, <<"my_precious">>).
+
+wrap_auth_headers_test_() ->
+    {setup,
+        fun() ->
+            meck:expect(ehttpc_sup, start_pool, 2, {ok, foo}),
+            meck:expect(ehttpc, request, fun(_, _, Req, _, _) -> {ok, 200, Req} end),
+            meck:expect(ehttpc_pool, pick_worker, 1, self()),
+            [ehttpc_sup, ehttpc, ehttpc_pool]
+        end,
+        fun meck:unload/1, fun(_) ->
+            Config = #{
+                base_url => #{
+                    scheme => http,
+                    host => "localhost",
+                    port => 18083,
+                    path => "/status"
+                },
+                connect_timeout => 1000,
+                pool_type => random,
+                pool_size => 1,
+                request => #{
+                    method => get,
+                    path => "/status",
+                    headers => auth_headers()
+                }
+            },
+            {ok, #{request := #{headers := Headers}} = State} = emqx_connector_http:on_start(
+                <<"test">>, Config
+            ),
+            {ok, 200, Req} = emqx_connector_http:on_query(foo, {send_message, #{}}, State),
+            Tests =
+                [
+                    ?_assert(is_wrapped(V))
+                 || H <- Headers, is_tuple({K, V} = H), is_auth_header(untmpl(K))
+                ],
+            [
+                ?_assertEqual(4, length(Tests)),
+                ?_assert(is_unwrapped_headers(element(2, Req)))
+                | Tests
+            ]
+        end}.
+
+auth_headers() ->
+    [
+        {<<"Authorization">>, ?MY_SECRET},
+        {<<"authorization">>, ?MY_SECRET},
+        {<<"Proxy-Authorization">>, ?MY_SECRET},
+        {<<"proxy-authorization">>, ?MY_SECRET},
+        {<<"X-Custom-Header">>, <<"foobar">>}
+    ].
+
+is_auth_header(<<"Authorization">>) -> true;
+is_auth_header(<<"Proxy-Authorization">>) -> true;
+is_auth_header(<<"authorization">>) -> true;
+is_auth_header(<<"proxy-authorization">>) -> true;
+is_auth_header(_Other) -> false.
+
+is_wrapped(Secret) when is_function(Secret) ->
+    untmpl(emqx_secret:unwrap(Secret)) =:= ?MY_SECRET;
+is_wrapped(_Other) ->
+    false.
+
+untmpl([{_, V} | _]) -> V.
+
+is_unwrapped_headers(Headers) ->
+    lists:all(fun is_unwrapped_header/1, Headers).
+
+is_unwrapped_header({_, V}) when is_function(V) -> false;
+is_unwrapped_header({_, [{str, _V}]}) -> throw(unexpected_tmpl_token);
+is_unwrapped_header(_) -> true.

changes/ce/fix-10556.en.md

@@ -0,0 +1 @@
+Wrap potentially sensitive data in `emqx_connector_http` if `Authorization` headers are being passed at initialization.