Merge pull request #8963 from savonarola/improve-interpolation

fix(auth): use empty strings for absent placeholder values

.gitignore

@@ -68,4 +68,3 @@ 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

@@ -8,6 +8,7 @@
 
 * Check ACLs for last will testament topic before publishing the message. [#8930](https://github.com/emqx/emqx/pull/8930)
 * Fix GET /listeners API crash When some nodes still in initial configuration. [#9002](https://github.com/emqx/emqx/pull/9002)
+* Fix empty variable interpolation in authentication and authorization. Placeholders for undefined variables are rendered now as empty strings and do not cause errors anymore. [#8963](https://github.com/emqx/emqx/pull/8963)
 
 # 5.0.8
 

apps/emqx_authn/include/emqx_authn.hrl

@@ -38,8 +38,4 @@
 
 -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_utils.erl

@@ -34,8 +34,7 @@
     ensure_apps_started/1,
     cleanup_resources/0,
     make_resource_id/1,
-    without_password/1,
+    without_password/1
-    with_successful_render/2
 ]).
 
 -define(AUTHN_PLACEHOLDERS, [
@@ -137,18 +136,6 @@ 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};
@@ -230,15 +217,15 @@ without_password(Credential, [Name | Rest]) ->
             without_password(Credential, Rest)
     end.
 
-handle_var({var, Name}, undefined) ->
+handle_var({var, _Name}, undefined) ->
-    error({cannot_get_variable, Name});
+    <<>>;
 handle_var({var, <<"peerhost">>}, PeerHost) ->
     emqx_placeholder:bin(inet:ntoa(PeerHost));
 handle_var(_, Value) ->
     emqx_placeholder:bin(Value).
 
-handle_sql_var({var, Name}, undefined) ->
+handle_sql_var({var, _Name}, undefined) ->
-    error({cannot_get_variable, Name});
+    <<>>;
 handle_sql_var({var, <<"peerhost">>}, PeerHost) ->
     emqx_placeholder:bin(inet:ntoa(PeerHost));
 handle_sql_var(_, Value) ->

apps/emqx_authn/src/simple_authn/emqx_authn_http.erl

@@ -187,8 +187,6 @@ 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", #{
@@ -207,9 +205,7 @@ 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,8 +162,6 @@ 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 ->
@@ -192,9 +190,7 @@ authenticate(
                 {error, Reason} ->
                     {error, Reason}
             end
-            end
+    end.
-        end
-    ).
 
 %%------------------------------------------------------------------------------
 %% Internal functions

apps/emqx_authn/src/simple_authn/emqx_authn_mysql.erl

@@ -113,8 +113,6 @@ 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, []} ->
@@ -140,9 +138,7 @@ authenticate(
                 reason => Reason
             }),
             ignore
-            end
+    end.
-        end
-    ).
 
 parse_config(
     #{

apps/emqx_authn/src/simple_authn/emqx_authn_pgsql.erl

@@ -115,8 +115,6 @@ 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, []} ->
@@ -141,9 +139,7 @@ authenticate(
                 reason => Reason
             }),
             ignore
-            end
+    end.
-        end
-    ).
 
 parse_config(
     #{

apps/emqx_authn/src/simple_authn/emqx_authn_redis.erl

@@ -133,8 +133,6 @@ 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
@@ -161,9 +159,7 @@ authenticate(
                 reason => Reason
             }),
             ignore
-            end
+    end.
-        end
-    ).
 
 %%------------------------------------------------------------------------------
 %% Internal functions

apps/emqx_authn/test/emqx_authn_http_SUITE.erl

@@ -166,6 +166,49 @@ test_user_auth(#{
         ?GLOBAL
     ).
 
+t_no_value_for_placeholder(_Config) ->
+    Handler = fun(Req0, State) ->
+        {ok, RawBody, Req1} = cowboy_req:read_body(Req0),
+        #{
+            <<"cert_subject">> := <<"">>,
+            <<"cert_common_name">> := <<"">>
+        } = jiffy:decode(RawBody, [return_maps]),
+        Req = cowboy_req:reply(
+            200,
+            #{<<"content-type">> => <<"application/json">>},
+            jiffy:encode(#{result => allow, is_superuser => false}),
+            Req1
+        ),
+        {ok, Req, State}
+    end,
+
+    SpecificConfgParams = #{
+        <<"method">> => <<"post">>,
+        <<"headers">> => #{<<"content-type">> => <<"application/json">>},
+        <<"body">> => #{
+            <<"cert_subject">> => ?PH_CERT_SUBJECT,
+            <<"cert_common_name">> => ?PH_CERT_CN_NAME
+        }
+    },
+
+    AuthConfig = maps:merge(raw_http_auth_config(), SpecificConfgParams),
+
+    {ok, _} = emqx:update_config(
+        ?PATH,
+        {create_authenticator, ?GLOBAL, AuthConfig}
+    ),
+
+    ok = emqx_authn_http_test_server:set_handler(Handler),
+
+    Credentials = maps:without([cert_subject, cert_common_name], ?CREDENTIALS),
+
+    ?assertMatch({ok, _}, emqx_access_control:authenticate(Credentials)),
+
+    emqx_authn_test_lib:delete_authenticators(
+        [authentication],
+        ?GLOBAL
+    ).
+
 t_destroy(_Config) ->
     AuthConfig = raw_http_auth_config(),
 
@@ -247,27 +290,6 @@ 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(
@@ -431,26 +453,6 @@ 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,20 +288,6 @@ 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,20 +258,6 @@ 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,20 +320,6 @@ 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

@@ -292,20 +292,6 @@ 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.erl

@@ -391,14 +391,6 @@ 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) ->
-    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) ->
-    error({cannot_get_variable, Name});
+    <<>>;
 handle_sql_var({var, <<"peerhost">>}, IpAddr) ->
     inet_parse:ntoa(IpAddr);
 handle_sql_var(_Name, Value) ->

apps/emqx_authz/test/emqx_authz_http_SUITE.erl

@@ -364,6 +364,46 @@ t_placeholder_and_body(_Config) ->
         emqx_access_control:authorize(ClientInfo, publish, <<"t">>)
     ).
 
+t_no_value_for_placeholder(_Config) ->
+    ok = setup_handler_and_config(
+        fun(Req0, State) ->
+            ?assertEqual(
+                <<"/authz/users/">>,
+                cowboy_req:path(Req0)
+            ),
+
+            {ok, RawBody, Req1} = cowboy_req:read_body(Req0),
+
+            ?assertMatch(
+                #{
+                    <<"mountpoint">> := <<"[]">>
+                },
+                jiffy:decode(RawBody, [return_maps])
+            ),
+            {ok, ?AUTHZ_HTTP_RESP(allow, Req1), State}
+        end,
+        #{
+            <<"method">> => <<"post">>,
+            <<"body">> => #{
+                <<"mountpoint">> => <<"[${mountpoint}]">>
+            }
+        }
+    ),
+
+    ClientInfo = #{
+        clientid => <<"client id">>,
+        username => <<"user name">>,
+        peerhost => {127, 0, 0, 1},
+        protocol => <<"MQTT">>,
+        zone => default,
+        listener => {tcp, default}
+    },
+
+    ?assertEqual(
+        allow,
+        emqx_access_control:authorize(ClientInfo, publish, <<"t">>)
+    ).
+
 t_create_replace(_Config) ->
     ClientInfo = #{
         clientid => <<"clientid">>,