Merge pull request #8635 from savonarola/handle-interpolate-errors

chore(authn/authz): better handling of placeholder interpolation errors

.gitignore

@@ -68,3 +68,4 @@ apps/emqx/test/emqx_static_checks_data/master.bpapi
 # rendered configurations
 *.conf.rendered
 lux_logs/
+.ci/docker-compose-file/redis/*.log

CHANGES-5.0.md

@@ -11,6 +11,7 @@
 * The license is now copied to all nodes in the cluster when it's reloaded. [#8598](https://github.com/emqx/emqx/pull/8598)
 * Added a HTTP API to manage licenses. [#8610](https://github.com/emqx/emqx/pull/8610)
 * Updated `/nodes` API node_status from `Running/Stopped` to `running/stopped`. [#8642](https://github.com/emqx/emqx/pull/8642)
+* Improve handling of placeholder interpolation errors [#8635](https://github.com/emqx/emqx/pull/8635)
 * Better logging on unknown object IDs. [#8670](https://github.com/emqx/emqx/pull/8670)
 
 # 5.0.4

apps/emqx_authn/include/emqx_authn.hrl

@@ -38,4 +38,8 @@
 
 -define(RESOURCE_GROUP, <<"emqx_authn">>).
 
+-define(WITH_SUCCESSFUL_RENDER(Code),
+    emqx_authn_utils:with_successful_render(?MODULE, fun() -> Code end)
+).
+
 -endif.

apps/emqx_authn/src/emqx_authn.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_authn, [
     {description, "EMQX Authentication"},
-    {vsn, "0.1.3"},
+    {vsn, "0.1.4"},
     {modules, []},
     {registered, [emqx_authn_sup, emqx_authn_registry]},
     {applications, [kernel, stdlib, emqx_resource, ehttpc, epgsql, mysql, jose]},

apps/emqx_authn/src/emqx_authn_utils.erl

@@ -34,7 +34,8 @@
     ensure_apps_started/1,
     cleanup_resources/0,
     make_resource_id/1,
-    without_password/1
+    without_password/1,
+    with_successful_render/2
 ]).
 
 -define(AUTHN_PLACEHOLDERS, [
@@ -136,6 +137,18 @@ render_sql_params(ParamList, Credential) ->
         #{return => rawlist, var_trans => fun handle_sql_var/2}
     ).
 
+with_successful_render(Provider, Fun) when is_function(Fun, 0) ->
+    try
+        Fun()
+    catch
+        error:{cannot_get_variable, Name} ->
+            ?TRACE_AUTHN(error, "placeholder_interpolation_failed", #{
+                provider => Provider,
+                placeholder => Name
+            }),
+            ignore
+    end.
+
 %% true
 is_superuser(#{<<"is_superuser">> := <<"true">>}) ->
     #{is_superuser => true};

apps/emqx_authn/src/simple_authn/emqx_authn_http.erl

@@ -187,6 +187,8 @@ authenticate(
         request_timeout := RequestTimeout
     } = State
 ) ->
+    ?WITH_SUCCESSFUL_RENDER(
+        begin
             Request = generate_request(Credential, State),
             Response = emqx_resource:query(ResourceId, {Method, Request, RequestTimeout}),
             ?TRACE_AUTHN_PROVIDER("http_response", #{
@@ -205,7 +207,9 @@ authenticate(
                     ignore;
                 {error, _Reason} ->
                     ignore
-    end.
+            end
+        end
+    ).
 
 destroy(#{resource_id := ResourceId}) ->
     _ = emqx_resource:remove_local(ResourceId),

apps/emqx_authn/src/simple_authn/emqx_authn_mongodb.erl

@@ -162,6 +162,8 @@ authenticate(
         resource_id := ResourceId
     } = State
 ) ->
+    ?WITH_SUCCESSFUL_RENDER(
+        begin
             Filter = emqx_authn_utils:render_deep(FilterTemplate, Credential),
             case emqx_resource:query(ResourceId, {find_one, Collection, Filter, #{}}) of
                 undefined ->
@@ -190,7 +192,9 @@ authenticate(
                         {error, Reason} ->
                             {error, Reason}
                     end
-    end.
+            end
+        end
+    ).
 
 %%------------------------------------------------------------------------------
 %% Internal functions

apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl

@@ -113,6 +113,8 @@ authenticate(
         password_hash_algorithm := Algorithm
     }
 ) ->
+    ?WITH_SUCCESSFUL_RENDER(
+        begin
             Params = emqx_authn_utils:render_sql_params(TmplToken, Credential),
             case emqx_resource:query(ResourceId, {prepared_query, ?PREPARE_KEY, Params, Timeout}) of
                 {ok, _Columns, []} ->
@@ -138,7 +140,9 @@ authenticate(
                         reason => Reason
                     }),
                     ignore
-    end.
+            end
+        end
+    ).
 
 parse_config(
     #{

apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl

@@ -115,6 +115,8 @@ authenticate(
         password_hash_algorithm := Algorithm
     }
 ) ->
+    ?WITH_SUCCESSFUL_RENDER(
+        begin
             Params = emqx_authn_utils:render_sql_params(PlaceHolders, Credential),
             case emqx_resource:query(ResourceId, {prepared_query, ResourceId, Params}) of
                 {ok, _Columns, []} ->
@@ -139,7 +141,9 @@ authenticate(
                         reason => Reason
                     }),
                     ignore
-    end.
+            end
+        end
+    ).
 
 parse_config(
     #{

apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl

@@ -133,6 +133,8 @@ authenticate(
         password_hash_algorithm := Algorithm
     }
 ) ->
+    ?WITH_SUCCESSFUL_RENDER(
+        begin
             NKey = emqx_authn_utils:render_str(KeyTemplate, Credential),
             Command = [CommandName, NKey | Fields],
             case emqx_resource:query(ResourceId, {cmd, Command}) of
@@ -159,7 +161,9 @@ authenticate(
                         reason => Reason
                     }),
                     ignore
-    end.
+            end
+        end
+    ).
 
 %%------------------------------------------------------------------------------
 %% Internal functions

apps/emqx_authn/test/emqx_authn_http_SUITE.erl

@@ -247,6 +247,27 @@ t_update(_Config) ->
         emqx_access_control:authenticate(?CREDENTIALS)
     ).
 
+t_interpolation_error(_Config) ->
+    {ok, _} = emqx:update_config(
+        ?PATH,
+        {create_authenticator, ?GLOBAL, raw_http_auth_config()}
+    ),
+
+    Headers = #{<<"content-type">> => <<"application/json">>},
+    Response = ?SERVER_RESPONSE_JSON(allow),
+
+    ok = emqx_authn_http_test_server:set_handler(
+        fun(Req0, State) ->
+            Req = cowboy_req:reply(200, Headers, Response, Req0),
+            {ok, Req, State}
+        end
+    ),
+
+    ?assertMatch(
+        ?EXCEPTION_DENY,
+        emqx_access_control:authenticate(maps:without([username], ?CREDENTIALS))
+    ).
+
 t_is_superuser(_Config) ->
     Config = raw_http_auth_config(),
     {ok, _} = emqx:update_config(
@@ -410,6 +431,26 @@ samples() ->
             result => {ok, #{is_superuser => false, user_property => #{}}}
         },
 
+        %% simple get request, no username
+        #{
+            handler => fun(Req0, State) ->
+                #{
+                    username := <<"plain">>,
+                    password := <<"plain">>
+                } = cowboy_req:match_qs([username, password], Req0),
+
+                Req = cowboy_req:reply(
+                    200,
+                    #{<<"content-type">> => <<"application/json">>},
+                    jiffy:encode(#{result => allow, is_superuser => false}),
+                    Req0
+                ),
+                {ok, Req, State}
+            end,
+            config_params => #{},
+            result => {ok, #{is_superuser => false, user_property => #{}}}
+        },
+
         %% get request with json body response
         #{
             handler => fun(Req0, State) ->

apps/emqx_authn/test/emqx_authn_mongo_SUITE.erl

@@ -288,6 +288,20 @@ raw_mongo_auth_config() ->
 
 user_seeds() ->
     [
+        #{
+            data => #{
+                username => <<"plain">>,
+                password_hash => <<"plainsalt">>,
+                salt => <<"salt">>,
+                is_superuser => <<"1">>
+            },
+            credentials => #{
+                password => <<"plain">>
+            },
+            config_params => #{},
+            result => {error, not_authorized}
+        },
+
         #{
             data => #{
                 username => <<"plain">>,

apps/emqx_authn/test/emqx_authn_mysql_SUITE.erl

@@ -258,6 +258,20 @@ raw_mysql_auth_config() ->
 
 user_seeds() ->
     [
+        #{
+            data => #{
+                username => "plain",
+                password_hash => "plainsalt",
+                salt => "salt",
+                is_superuser_str => "1"
+            },
+            credentials => #{
+                password => <<"plain">>
+            },
+            config_params => #{},
+            result => {error, not_authorized}
+        },
+
         #{
             data => #{
                 username => "plain",

apps/emqx_authn/test/emqx_authn_pgsql_SUITE.erl

@@ -320,6 +320,20 @@ raw_pgsql_auth_config() ->
 
 user_seeds() ->
     [
+        #{
+            data => #{
+                username => "plain",
+                password_hash => "plainsalt",
+                salt => "salt",
+                is_superuser_str => "1"
+            },
+            credentials => #{
+                password => <<"plain">>
+            },
+            config_params => #{},
+            result => {error, not_authorized}
+        },
+
         #{
             data => #{
                 username => "plain",

apps/emqx_authn/test/emqx_authn_redis_SUITE.erl

@@ -280,6 +280,20 @@ raw_redis_auth_config() ->
 
 user_seeds() ->
     [
+        #{
+            data => #{
+                password_hash => <<"plainsalt">>,
+                salt => <<"salt">>,
+                is_superuser => <<"1">>
+            },
+            credentials => #{
+                password => <<"plain">>
+            },
+            key => <<"mqtt_user:plain">>,
+            config_params => #{},
+            result => {error, not_authorized}
+        },
+
         #{
             data => #{
                 password_hash => <<"plainsalt">>,

apps/emqx_authz/src/emqx_authz.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_authz, [
     {description, "An OTP application"},
-    {vsn, "0.1.3"},
+    {vsn, "0.1.4"},
     {registered, []},
     {mod, {emqx_authz_app, []}},
     {applications, [

apps/emqx_authz/src/emqx_authz.erl

@@ -402,6 +402,14 @@ do_authorize(
         Matched ->
             {Matched, Type}
     catch
+        error:{cannot_get_variable, Name} ->
+            emqx_metrics_worker:inc(authz_metrics, Type, nomatch),
+            ?SLOG(warning, #{
+                msg => "placeholder_interpolation_failed",
+                placeholder => Name,
+                authorize_type => Type
+            }),
+            do_authorize(Client, PubSub, Topic, Tail);
         Class:Reason:Stacktrace ->
             emqx_metrics_worker:inc(authz_metrics, Type, nomatch),
             ?SLOG(warning, #{

apps/emqx_authz/src/emqx_authz_utils.erl

@@ -181,15 +181,15 @@ convert_client_var({dn, DN}) -> {cert_subject, DN};
 convert_client_var({protocol, Proto}) -> {proto_name, Proto};
 convert_client_var(Other) -> Other.
 
-handle_var({var, _Name}, undefined) ->
+handle_var({var, Name}, undefined) ->
-    "undefined";
+    error({cannot_get_variable, Name});
 handle_var({var, <<"peerhost">>}, IpAddr) ->
     inet_parse:ntoa(IpAddr);
 handle_var(_Name, Value) ->
     emqx_placeholder:bin(Value).
 
-handle_sql_var({var, _Name}, undefined) ->
+handle_sql_var({var, Name}, undefined) ->
-    "undefined";
+    error({cannot_get_variable, Name});
 handle_sql_var({var, <<"peerhost">>}, IpAddr) ->
     inet_parse:ntoa(IpAddr);
 handle_sql_var(_Name, Value) ->