fix(audit): make emqx eval command auditable

apps/emqx_ctl/src/emqx_ctl.erl

@@ -44,6 +44,10 @@
     usage/2
 ]).
 
+-export([
+    eval_erl/1
+]).
+
 %% Exports mainly for test cases
 -export([
     format/2,
@@ -119,7 +123,7 @@ run_command(Cmd, Args) when is_atom(Cmd) ->
     Start = erlang:monotonic_time(),
     Result =
         case lookup_command(Cmd) of
-            [{Mod, Fun}] ->
+            {ok, {Mod, Fun}} ->
                 try
                     apply(Mod, Fun, [Args])
                 catch
@@ -127,13 +131,15 @@ run_command(Cmd, Args) when is_atom(Cmd) ->
                         ?LOG_ERROR(#{
                             msg => "ctl_command_crashed",
                             stacktrace => Stacktrace,
-                            reason => Reason
+                            reason => Reason,
+                            module => Mod,
+                            function => Fun
                         }),
                         {error, Reason}
                 end;
-            Error ->
+            {error, Reason} ->
                 help(),
-                Error
+                {error, Reason}
         end,
     Duration = erlang:convert_time_unit(erlang:monotonic_time() - Start, native, millisecond),
 
@@ -144,12 +150,22 @@ run_command(Cmd, Args) when is_atom(Cmd) ->
     ),
     Result.
 
--spec lookup_command(cmd()) -> [{module(), atom()}] | {error, any()}.
+-spec lookup_command(cmd()) -> {module(), atom()} | {error, any()}.
+lookup_command(eval_erl) ->
+    %% So far 'emqx ctl eval_erl Expr' is a undocumented hidden command.
+    %% For backward compatibility,
+    %% the documented command 'emqx eval Expr' has the expression parsed
+    %% in the remsh node (nodetool).
+    %%
+    %% 'eval_erl' is added for two purposes
+    %% 1. 'emqx eval Expr' can be audited
+    %% 2. 'emqx ctl eval_erl Expr' simplifies the scripting part
+    {ok, {?MODULE, eval_erl}};
 lookup_command(Cmd) when is_atom(Cmd) ->
     case is_initialized() of
         true ->
             case ets:match(?CMD_TAB, {{'_', Cmd}, '$1', '_'}) of
-                [El] -> El;
+                [[{M, F}]] -> {ok, {M, F}};
                 [] -> {error, cmd_not_found}
             end;
         false ->
@@ -319,7 +335,7 @@ audit_log(Level, From, Log) ->
     case lookup_command(audit) of
         {error, _} ->
             ignore;
-        [{Mod, Fun}] ->
+        {ok, {Mod, Fun}} ->
             try
                 apply(Mod, Fun, [Level, From, Log])
             catch
@@ -339,3 +355,23 @@ audit_level({ok, _}, Duration) when Duration >= ?TOO_SLOW -> warning;
 audit_level(ok, _Duration) -> info;
 audit_level({ok, _}, _Duration) -> info;
 audit_level(_, _) -> error.
+
+eval_erl([Parsed | _] = Expr) when is_tuple(Parsed) ->
+    eval_expr(Expr);
+eval_erl([String]) ->
+    % convenience to users, if they forgot a trailing
+    % '.' add it for them.
+    Normalized =
+        case lists:reverse(String) of
+            [$. | _] -> String;
+            R -> lists:reverse([$. | R])
+        end,
+    % then scan and parse the string
+    {ok, Scanned, _} = erl_scan:string(Normalized),
+    {ok, Parsed} = erl_parse:parse_exprs(Scanned),
+    {ok, Value} = eval_expr(Parsed),
+    print("~p~n", [Value]).
+
+eval_expr(Parsed) ->
+    {value, Value, _} = erl_eval:exprs(Parsed, []),
+    {ok, Value}.

apps/emqx_ctl/test/emqx_ctl_SUITE.erl

@@ -40,8 +40,8 @@ t_reg_unreg_command(_) ->
         fun(_CtlSrv) ->
             emqx_ctl:register_command(cmd1, {?MODULE, cmd1_fun}),
             emqx_ctl:register_command(cmd2, {?MODULE, cmd2_fun}),
-            ?assertEqual([{?MODULE, cmd1_fun}], emqx_ctl:lookup_command(cmd1)),
-            ?assertEqual([{?MODULE, cmd2_fun}], emqx_ctl:lookup_command(cmd2)),
+            ?assertEqual({?MODULE, cmd1_fun}, lookup_command(cmd1)),
+            ?assertEqual({?MODULE, cmd2_fun}, lookup_command(cmd2)),
             ?assertEqual(
                 [{cmd1, ?MODULE, cmd1_fun}, {cmd2, ?MODULE, cmd2_fun}],
                 emqx_ctl:get_commands()
@@ -49,8 +49,8 @@ t_reg_unreg_command(_) ->
             emqx_ctl:unregister_command(cmd1),
             emqx_ctl:unregister_command(cmd2),
             ct:sleep(100),
-            ?assertEqual({error, cmd_not_found}, emqx_ctl:lookup_command(cmd1)),
-            ?assertEqual({error, cmd_not_found}, emqx_ctl:lookup_command(cmd2)),
+            ?assertEqual({error, cmd_not_found}, lookup_command(cmd1)),
+            ?assertEqual({error, cmd_not_found}, lookup_command(cmd2)),
             ?assertEqual([], emqx_ctl:get_commands())
         end
     ).
@@ -79,6 +79,12 @@ t_print(_) ->
     ?assertEqual("~!@#$%^&*()", emqx_ctl:print("~ts", [<<"~!@#$%^&*()">>])),
     unmock_print().
 
+t_eval_erl(_) ->
+    mock_print(),
+    Expected = atom_to_list(node()) ++ "\n",
+    ?assertEqual(Expected, emqx_ctl:run_command(["eval_erl", "node()"])),
+    unmock_print().
+
 t_usage(_) ->
     CmdParams1 = "emqx_cmd_1 param1 param2",
     CmdDescr1 = "emqx_cmd_1 is a test command means nothing",
@@ -129,3 +135,9 @@ mock_print() ->
 
 unmock_print() ->
     meck:unload(emqx_ctl).
+
+lookup_command(Cmd) ->
+    case emqx_ctl:lookup_command(Cmd) of
+        {ok, {Mod, Fun}} -> {Mod, Fun};
+        Error -> Error
+    end.

bin/nodetool

@@ -142,8 +142,8 @@ do(Args) ->
         ["eval" | ListOfArgs] ->
             Parsed = parse_eval_args(ListOfArgs),
             % and evaluate it on the remote node
-            case rpc:call(TargetNode, erl_eval, exprs, [Parsed, [] ]) of
-                {value, Value, _} ->
+            case rpc:call(TargetNode, emqx_ctl, eval_erl, [Parsed]) of
+                {ok, Value} ->
                     io:format("~p~n",[Value]);
                 {badrpc, Reason} ->
                     io:format("RPC to ~p failed: ~p~n", [TargetNode, Reason]),

dev

@@ -37,6 +37,7 @@ COMMANDS:
   ctl:    Equivalent to 'emqx ctl'.
           ctl command arguments should be passed after '--'
           e.g. $0 ctl -- help
+  eval:   Evaluate an Erlang expression
 
 OPTIONS:
   -p|--profile:      emqx | emqx-enterprise, defaults to 'PROFILE' env.
@@ -83,6 +84,10 @@ case "${1:-novalue}" in
         COMMAND='ctl'
         shift
         ;;
+    eval)
+        COMMAND='eval'
+        shift
+        ;;
     help)
         usage
         exit 0
@@ -425,14 +430,22 @@ remsh() {
         $EPMD_ARGS
 }
 
+# evaluate erlang expression in remsh node
+eval_remsh_erl() {
+    local tmpnode erl_code
+    tmpnode="$(gen_tmp_node_name)"
+    erl_code="$1"
+    # shellcheck disable=SC2086 # need to expand EMQD_ARGS
+    erl -name "$tmpnode" -setcookie "$COOKIE" -hidden -noshell $EPMD_ARGS -eval "$erl_code" 2>&1
+}
+
 ctl() {
     if [ -z "${PASSTHROUGH_ARGS:-}" ]; then
         logerr "Need at least one argument for ctl command"
         logerr "e.g. $0 ctl -- help"
         exit 1
     fi
-    local tmpnode args rpc_code output result
-    tmpnode="$(gen_tmp_node_name)"
+    local args rpc_code output result
     args="$(make_erlang_args "${PASSTHROUGH_ARGS[@]}")"
     rpc_code="
         case rpc:call('$EMQX_NODE_NAME', emqx_ctl, run_command, [[$args]]) of
@@ -443,8 +456,7 @@ ctl() {
             init:stop(1)
         end"
     set +e
-    # shellcheck disable=SC2086
-    output="$(erl -name "$tmpnode" -setcookie "$COOKIE" -hidden -noshell $EPMD_ARGS -eval "$rpc_code" 2>&1)"
+    output="$(eval_remsh_erl "$rpc_code")"
     result=$?
     if [ $result -eq 0 ]; then
         echo -e "$output"
@@ -464,4 +476,8 @@ case "$COMMAND" in
     ctl)
         ctl
         ;;
+    eval)
+        PASSTHROUGH_ARGS=('eval_erl' "${PASSTHROUGH_ARGS[@]}")
+        ctl
+        ;;
 esac