Merge pull request #11790 from savonarola/1018-improve-redis-cmd-validation-in-auth

feat(auth): improve redis command parsing and validation

apps/emqx_auth_redis/src/emqx_auth_redis_validations.erl

@@ -0,0 +1,71 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2020-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_auth_redis_validations).
+
+-export([
+    validate_command/2
+]).
+
+validate_command([], _Command) ->
+    ok;
+validate_command([Validation | Rest], Command) ->
+    case validate(Validation, Command) of
+        ok ->
+            validate_command(Rest, Command);
+        {error, _} = Error ->
+            Error
+    end.
+
+validate(not_empty, []) ->
+    {error, empty_command};
+validate(not_empty, _) ->
+    ok;
+validate({command_name, AllowedNames}, [Name | _]) ->
+    IsAllowed = lists:any(
+        fun(AllowedName) ->
+            string:equal(AllowedName, Name, true, none)
+        end,
+        AllowedNames
+    ),
+    case IsAllowed of
+        true ->
+            ok;
+        false ->
+            {error, {invalid_command_name, Name}}
+    end;
+validate({command_name, _}, _) ->
+    {error, invalid_command_name};
+validate({allowed_fields, AllowedFields}, [_CmdName, _CmdKey | Args]) ->
+    Unknown = lists:filter(fun(Arg) -> not lists:member(Arg, AllowedFields) end, Args),
+    case Unknown of
+        [] ->
+            ok;
+        _ ->
+            {error, {unknown_fields, Unknown}}
+    end;
+validate({allowed_fields, _}, _) ->
+    ok;
+validate({required_field_one_of, Required}, [_CmdName, _CmdKey | Args]) ->
+    HasRequired = lists:any(fun(Field) -> lists:member(Field, Args) end, Required),
+    case HasRequired of
+        true ->
+            ok;
+        false ->
+            {error, {missing_required_field, Required}}
+    end;
+validate({required_field_one_of, Required}, _) ->
+    {error, {missing_required_field, Required}}.

apps/emqx_auth_redis/src/emqx_authn_redis.erl

@@ -118,54 +118,51 @@ authenticate(
 
 parse_config(
     #{
-        cmd := Cmd,
+        cmd := CmdStr,
         password_hash_algorithm := Algorithm
     } = Config
 ) ->
-    try
+    case parse_cmd(CmdStr) of
-        NCmd = parse_cmd(Cmd),
+        {ok, Cmd} ->
-        ok = emqx_authn_password_hashing:init(Algorithm),
+            ok = emqx_authn_password_hashing:init(Algorithm),
-        ok = emqx_authn_utils:ensure_apps_started(Algorithm),
+            ok = emqx_authn_utils:ensure_apps_started(Algorithm),
-        State = maps:with([password_hash_algorithm, salt_position], Config),
+            State = maps:with([password_hash_algorithm, salt_position], Config),
-        {Config, State#{cmd => NCmd}}
+            {Config, State#{cmd => Cmd}};
-    catch
+        {error, _} = Error ->
-        error:{unsupported_cmd, _Cmd} ->
+            Error
-            {error, {unsupported_cmd, Cmd}};
-        error:missing_password_hash ->
-            {error, missing_password_hash};
-        error:{unsupported_fields, Fields} ->
-            {error, {unsupported_fields, Fields}}
     end.
 
-%% Only support HGET and HMGET
+parse_cmd(CmdStr) ->
-parse_cmd(Cmd) ->
+    case emqx_redis_command:split(CmdStr) of
-    case string:tokens(Cmd, " ") of
+        {ok, Cmd} ->
-        [Command, Key, Field | Fields] when Command =:= "HGET" orelse Command =:= "HMGET" ->
+            case validate_cmd(Cmd) of
-            NFields = [Field | Fields],
+                ok ->
-            check_fields(NFields),
+                    [CommandName, Key | Fields] = Cmd,
-            KeyTemplate = emqx_authn_utils:parse_str(list_to_binary(Key)),
+                    {ok, {CommandName, emqx_authn_utils:parse_str(Key), Fields}};
-            {Command, KeyTemplate, NFields};
+                {error, _} = Error ->
-        _ ->
+                    Error
-            error({unsupported_cmd, Cmd})
+            end;
+        {error, _} = Error ->
+            Error
     end.
 
-check_fields(Fields) ->
+validate_cmd(Cmd) ->
-    HasPassHash = lists:member("password_hash", Fields) orelse lists:member("password", Fields),
+    emqx_auth_redis_validations:validate_command(
-    KnownFields = ["password_hash", "password", "salt", "is_superuser"],
+        [
-    UnknownFields = [F || F <- Fields, not lists:member(F, KnownFields)],
+            not_empty,
-
+            {command_name, [<<"hget">>, <<"hmget">>]},
-    case {HasPassHash, UnknownFields} of
+            {allowed_fields, [<<"password_hash">>, <<"password">>, <<"salt">>, <<"is_superuser">>]},
-        {true, []} -> ok;
+            {required_field_one_of, [<<"password_hash">>, <<"password">>]}
-        {true, _} -> error({unsupported_fields, UnknownFields});
+        ],
-        {false, _} -> error(missing_password_hash)
+        Cmd
-    end.
+    ).
 
 merge(Fields, Value) when not is_list(Value) ->
     merge(Fields, [Value]);
 merge(Fields, Values) ->
     maps:from_list(
         [
-            {list_to_binary(K), V}
+            {K, V}
          || {K, V} <- lists:zip(Fields, Values), V =/= undefined
         ]
     ).

apps/emqx_auth_redis/src/emqx_authn_redis_schema.erl

@@ -85,7 +85,7 @@ common_fields() ->
         {password_hash_algorithm, fun emqx_authn_password_hashing:type_ro/1}
     ] ++ emqx_authn_schema:common_fields().
 
-cmd(type) -> string();
+cmd(type) -> binary();
 cmd(desc) -> ?DESC(?FUNCTION_NAME);
 cmd(required) -> true;
 cmd(_) -> undefined.

apps/emqx_auth_redis/src/emqx_authz_redis.erl

@@ -47,15 +47,13 @@ description() ->
     "AuthZ with Redis".
 
 create(#{cmd := CmdStr} = Source) ->
-    Cmd = tokens(CmdStr),
+    CmdTemplate = parse_cmd(CmdStr),
     ResourceId = emqx_authz_utils:make_resource_id(?MODULE),
-    CmdTemplate = emqx_authz_utils:parse_deep(Cmd, ?PLACEHOLDERS),
     {ok, _Data} = emqx_authz_utils:create_resource(ResourceId, emqx_redis, Source),
     Source#{annotations => #{id => ResourceId}, cmd_template => CmdTemplate}.
 
 update(#{cmd := CmdStr} = Source) ->
-    Cmd = tokens(CmdStr),
+    CmdTemplate = parse_cmd(CmdStr),
-    CmdTemplate = emqx_authz_utils:parse_deep(Cmd, ?PLACEHOLDERS),
     case emqx_authz_utils:update_resource(emqx_redis, Source) of
         {error, Reason} ->
             error({load_config_error, Reason});
@@ -131,9 +129,28 @@ compile_rule(RuleBin, TopicFilterRaw) ->
             error(Reason)
     end.
 
-tokens(Query) ->
+parse_cmd(Query) ->
-    Tokens = binary:split(Query, <<" ">>, [global]),
+    case emqx_redis_command:split(Query) of
-    [Token || Token <- Tokens, size(Token) > 0].
+        {ok, Cmd} ->
+            ok = validate_cmd(Cmd),
+            emqx_authz_utils:parse_deep(Cmd, ?PLACEHOLDERS);
+        {error, Reason} ->
+            error({invalid_redis_cmd, Reason, Query})
+    end.
+
+validate_cmd(Cmd) ->
+    case
+        emqx_auth_redis_validations:validate_command(
+            [
+                not_empty,
+                {command_name, [<<"hmget">>, <<"hgetall">>]}
+            ],
+            Cmd
+        )
+    of
+        ok -> ok;
+        {error, Reason} -> error({invalid_redis_cmd, Reason, Cmd})
+    end.
 
 parse_rule(<<"publish">>) ->
     #{<<"action">> => <<"publish">>};

apps/emqx_auth_redis/test/emqx_authn_redis_SUITE.erl

@@ -336,7 +336,22 @@ user_seeds() ->
             config_params => #{},
             result => {ok, #{is_superuser => true}}
         },
-
+        #{
+            data => #{
+                password_hash => <<"plainsalt">>,
+                salt => <<"salt">>,
+                is_superuser => <<"1">>
+            },
+            credentials => #{
+                username => <<"plain">>,
+                password => <<"plain">>
+            },
+            key => <<"mqtt_user:plain">>,
+            config_params => #{
+                <<"cmd">> => <<"HmGeT mqtt_user:${username} password_hash salt is_superuser">>
+            },
+            result => {ok, #{is_superuser => true}}
+        },
         #{
             data => #{
                 password_hash => <<"9b4d0c43d206d48279e69b9ad7132e22">>,

apps/emqx_auth_redis/test/emqx_authz_redis_SUITE.erl

@@ -112,7 +112,9 @@ t_create_invalid_config(_Config) ->
     ).
 
 t_redis_error(_Config) ->
-    ok = setup_config(#{<<"cmd">> => <<"INVALID COMMAND">>}),
+    q([<<"SET">>, <<"notahash">>, <<"stringvalue">>]),
+
+    ok = setup_config(#{<<"cmd">> => <<"HGETALL notahash">>}),
 
     ClientInfo = emqx_authz_test_lib:base_client_info(),
 
@@ -121,6 +123,24 @@ t_redis_error(_Config) ->
         emqx_access_control:authorize(ClientInfo, ?AUTHZ_SUBSCRIBE, <<"a">>)
     ).
 
+t_invalid_command(_Config) ->
+    Config = raw_redis_authz_config(),
+
+    ?assertMatch(
+        {error, _},
+        emqx_authz:update(?CMD_REPLACE, [Config#{<<"cmd">> => <<"HGET key">>}])
+    ),
+
+    ?assertMatch(
+        {ok, _},
+        emqx_authz:update(?CMD_REPLACE, [Config#{<<"cmd">> => <<"HGETALL key">>}])
+    ),
+
+    ?assertMatch(
+        {error, _},
+        emqx_authz:update({?CMD_REPLACE, redis}, Config#{<<"cmd">> => <<"HGET key">>})
+    ).
+
 %%------------------------------------------------------------------------------
 %% Cases
 %%------------------------------------------------------------------------------

apps/emqx_redis/src/emqx_redis_command.erl

@@ -0,0 +1,129 @@
+%%--------------------------------------------------------------------
+%% 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 `split/1` function reimplements the one used by Redis itself for `redis-cli`.
+%% See `sdssplitargs` function, https://github.com/redis/redis/blob/unstable/src/sds.c.
+
+-module(emqx_redis_command).
+
+-export([split/1]).
+
+-define(CH_SPACE, 32).
+-define(CH_N, 10).
+-define(CH_R, 13).
+-define(CH_T, 9).
+-define(CH_B, 8).
+-define(CH_A, 11).
+
+-define(IS_CH_HEX_DIGIT(C),
+    (((C >= $a) andalso (C =< $f)) orelse
+        ((C >= $A) andalso (C =< $F)) orelse
+        ((C >= $0) andalso (C =< $9)))
+).
+-define(IS_CH_SPACE(C),
+    (C =:= ?CH_SPACE orelse
+        C =:= ?CH_N orelse
+        C =:= ?CH_R orelse
+        C =:= ?CH_T orelse
+        C =:= ?CH_B orelse
+        C =:= ?CH_A)
+).
+
+split(Line) when is_binary(Line) ->
+    case split(binary_to_list(Line)) of
+        {ok, Args} ->
+            {ok, [list_to_binary(Arg) || Arg <- Args]};
+        {error, _} = Error ->
+            Error
+    end;
+split(Line) ->
+    split(Line, []).
+
+split([], Acc) ->
+    {ok, lists:reverse(Acc)};
+split([C | Rest] = Line, Acc) ->
+    case ?IS_CH_SPACE(C) of
+        true -> split(Rest, Acc);
+        false -> split_noq([], Line, Acc)
+    end.
+
+hex_digit_to_int(C) when (C >= $a) andalso (C =< $f) -> 10 + C - $a;
+hex_digit_to_int(C) when (C >= $A) andalso (C =< $F) -> 10 + C - $A;
+hex_digit_to_int(C) when (C >= $0) andalso (C =< $9) -> C - $0.
+
+maybe_special_char($n) -> ?CH_N;
+maybe_special_char($r) -> ?CH_R;
+maybe_special_char($t) -> ?CH_T;
+maybe_special_char($b) -> ?CH_B;
+maybe_special_char($a) -> ?CH_A;
+maybe_special_char(C) -> C.
+
+%% Inside double quotes
+split_inq(CurAcc, Line, Acc) ->
+    case Line of
+        [$\\, $x, HD1, HD2 | LineRest] when ?IS_CH_HEX_DIGIT(HD1) andalso ?IS_CH_HEX_DIGIT(HD2) ->
+            C = hex_digit_to_int(HD1) * 16 + hex_digit_to_int(HD2),
+            NewCurAcc = [C | CurAcc],
+            split_inq(NewCurAcc, LineRest, Acc);
+        [$\\, SC | LineRest] ->
+            C = maybe_special_char(SC),
+            NewCurAcc = [C | CurAcc],
+            split_inq(NewCurAcc, LineRest, Acc);
+        [$", C | _] when not ?IS_CH_SPACE(C) ->
+            {error, trailing_after_quote};
+        [$" | LineRest] ->
+            split(LineRest, [lists:reverse(CurAcc) | Acc]);
+        [] ->
+            {error, unterminated_quote};
+        [C | LineRest] ->
+            NewCurAcc = [C | CurAcc],
+            split_inq(NewCurAcc, LineRest, Acc)
+    end.
+
+%% Inside single quotes
+split_insq(CurAcc, Line, Acc) ->
+    case Line of
+        [$\\, $' | LineRest] ->
+            NewCurAcc = [$' | CurAcc],
+            split_insq(NewCurAcc, LineRest, Acc);
+        [$', C | _] when not ?IS_CH_SPACE(C) ->
+            {error, trailing_after_single_quote};
+        [$' | LineRest] ->
+            split(LineRest, [lists:reverse(CurAcc) | Acc]);
+        [] ->
+            {error, unterminated_single_quote};
+        [C | LineRest] ->
+            NewCurAcc = [C | CurAcc],
+            split_insq(NewCurAcc, LineRest, Acc)
+    end.
+
+%% Outside quotes
+split_noq(CurAcc, Line, Acc) ->
+    case Line of
+        [C | LineRest] when
+            ?IS_CH_SPACE(C); C =:= ?CH_N; C =:= ?CH_R; C =:= ?CH_T
+        ->
+            split(LineRest, [lists:reverse(CurAcc) | Acc]);
+        [] ->
+            split([], [lists:reverse(CurAcc) | Acc]);
+        [$' | LineRest] ->
+            split_insq(CurAcc, LineRest, Acc);
+        [$" | LineRest] ->
+            split_inq(CurAcc, LineRest, Acc);
+        [C | LineRest] ->
+            NewCurAcc = [C | CurAcc],
+            split_noq(NewCurAcc, LineRest, Acc)
+    end.

apps/emqx_redis/test/emqx_redis_SUITE.erl

@@ -1,17 +1,17 @@
-% %%--------------------------------------------------------------------
+%%--------------------------------------------------------------------
-% %% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved.
+%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved.
-% %%
+%%
-% %% Licensed under the Apache License, Version 2.0 (the "License");
+%% Licensed under the Apache License, Version 2.0 (the "License");
-% %% you may not use this file except in compliance with the License.
+%% you may not use this file except in compliance with the License.
-% %% You may obtain a copy of the License at
+%% You may obtain a copy of the License at
-% %% http://www.apache.org/licenses/LICENSE-2.0
+%% http://www.apache.org/licenses/LICENSE-2.0
-% %%
+%%
-% %% Unless required by applicable law or agreed to in writing, software
+%% Unless required by applicable law or agreed to in writing, software
-% %% distributed under the License is distributed on an "AS IS" BASIS,
+%% distributed under the License is distributed on an "AS IS" BASIS,
-% %% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-% %% See the License for the specific language governing permissions and
+%% See the License for the specific language governing permissions and
-% %% limitations under the License.
+%% limitations under the License.
-% %%--------------------------------------------------------------------
+%%--------------------------------------------------------------------
 
 -module(emqx_redis_SUITE).
 
@@ -190,9 +190,9 @@ perform_lifecycle_check(ResourceId, InitialConfig, RedisCommand) ->
     % Should not even be able to get the resource data out of ets now unlike just stopping.
     ?assertEqual({error, not_found}, emqx_resource:get_instance(ResourceId)).
 
-% %%------------------------------------------------------------------------------
+%%------------------------------------------------------------------------------
-% %% Helpers
+%% Helpers
-% %%------------------------------------------------------------------------------
+%%------------------------------------------------------------------------------
 
 redis_config_single() ->
     redis_config_base("single", "server").

apps/emqx_redis/test/emqx_redis_command_SUITE.erl

@@ -0,0 +1,76 @@
+%%--------------------------------------------------------------------
+%% Copyright (c) 2020-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_redis_command_SUITE).
+
+-compile(nowarn_export_all).
+-compile(export_all).
+
+-include_lib("eunit/include/eunit.hrl").
+
+all() ->
+    emqx_common_test_helpers:all(?MODULE).
+
+t_split_ok(_Config) ->
+    ?assertEqual(
+        {ok, [<<"ab">>, <<"cd">>, <<"ef">>]},
+        emqx_redis_command:split(<<" \"ab\" 'cd' ef ">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"ab">>, <<"cd">>, <<"ef">>]},
+        emqx_redis_command:split(<<" ab\tcd ef">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"abc'd">>, <<"ef">>]},
+        emqx_redis_command:split(<<"ab\"c'd\" ef">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"abc\"d">>, <<"ef">>]},
+        emqx_redis_command:split(<<"ab'c\"d' ef">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"IJK">>, <<"\\x49\\x4a\\x4B">>]},
+        emqx_redis_command:split(<<"\"\\x49\\x4a\\x4B\" \\x49\\x4a\\x4B">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"x\t\n\r\b\v">>]},
+        emqx_redis_command:split(<<"\"\\x\\t\\n\\r\\b\\a\"">>)
+    ),
+    ?assertEqual(
+        {ok, [<<"abc\'d">>, <<"ef">>]},
+        emqx_redis_command:split(<<"'abc\\'d' ef">>)
+    ),
+    ?assertEqual(
+        {ok, [<<>>, <<>>]},
+        emqx_redis_command:split(<<" '' \"\" ">>)
+    ).
+
+t_split_error(_Config) ->
+    ?assertEqual(
+        {error, trailing_after_quote},
+        emqx_redis_command:split(<<"\"a\"b">>)
+    ),
+    ?assertEqual(
+        {error, unterminated_quote},
+        emqx_redis_command:split(<<"\"ab">>)
+    ),
+    ?assertEqual(
+        {error, trailing_after_single_quote},
+        emqx_redis_command:split(<<"'a'b'c">>)
+    ),
+    ?assertEqual(
+        {error, unterminated_single_quote},
+        emqx_redis_command:split(<<"'ab">>)
+    ).

apps/emqx_redis/test/props/prop_emqx_redis_command.erl

@@ -0,0 +1,31 @@
+%%--------------------------------------------------------------------
+%% 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(prop_emqx_redis_command).
+
+-include_lib("proper/include/proper.hrl").
+
+%%--------------------------------------------------------------------
+%% Properties
+%%--------------------------------------------------------------------
+
+prop_split() ->
+    ?FORALL(
+        Cmd,
+        binary(),
+        %% Should terminate and not crash
+        is_tuple(emqx_redis_command:split(Cmd))
+    ).

apps/emqx_telemetry/test/emqx_telemetry_SUITE.erl

@@ -738,7 +738,7 @@ create_authn(ChainName, redis) ->
             backend => redis,
             enable => true,
             user_id_type => username,
-            cmd => "HMGET mqtt_user:${username} password_hash salt is_superuser",
+            cmd => <<"HMGET mqtt_user:${username} password_hash salt is_superuser">>,
             password_hash_algorithm => #{
                 name => plain,
                 salt_position => suffix

changes/ce/feat-11790.en.md

@@ -0,0 +1,3 @@
+Added validation of Redis commands configured in Redis authorization source.
+Also, improved Redis command parsing in authentication and authorization
+so that it is `redis-cli` compatible and supports quoted arguments.