fix(command): MONITOR command should redact sensitive tokens (#3193)
diff --git a/src/commands/cmd_function.cc b/src/commands/cmd_function.cc
index 73fc24c..3709a74 100644
--- a/src/commands/cmd_function.cc
+++ b/src/commands/cmd_function.cc
@@ -130,7 +130,7 @@
REDIS_REGISTER_COMMANDS(
Function, MakeCmdAttr<CommandFunction>("function", -2, "exclusive no-script", NO_KEY, GenerateFunctionFlags),
- MakeCmdAttr<CommandFCall<>>("fcall", -3, "write no-script", GetScriptEvalKeyRange, GenerateFCallFlags),
- MakeCmdAttr<CommandFCall<true>>("fcall_ro", -3, "read-only no-script", GetScriptEvalKeyRange));
+ MakeCmdAttr<CommandFCall<>>("fcall", -3, "write no-script skip-monitor", GetScriptEvalKeyRange, GenerateFCallFlags),
+ MakeCmdAttr<CommandFCall<true>>("fcall_ro", -3, "read-only no-script skip-monitor", GetScriptEvalKeyRange));
} // namespace redis
diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc
index ea6a37d..e9a05fe 100644
--- a/src/commands/cmd_script.cc
+++ b/src/commands/cmd_script.cc
@@ -134,10 +134,12 @@
}
REDIS_REGISTER_COMMANDS(
- Script, MakeCmdAttr<CommandEval>("eval", -3, "write no-script", GetScriptEvalKeyRange, GenerateEvalFlags),
- MakeCmdAttr<CommandEvalSHA>("evalsha", -3, "write no-script", GetScriptEvalKeyRange, GenerateEvalFlags),
- MakeCmdAttr<CommandEvalRO>("eval_ro", -3, "read-only no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandEvalSHARO>("evalsha_ro", -3, "read-only no-script", GetScriptEvalKeyRange),
- MakeCmdAttr<CommandScript>("script", -2, "exclusive no-script", NO_KEY, GenerateScriptFlags), )
+ Script,
+ MakeCmdAttr<CommandEval>("eval", -3, "write no-script skip-monitor", GetScriptEvalKeyRange, GenerateEvalFlags),
+ MakeCmdAttr<CommandEvalSHA>("evalsha", -3, "write no-script skip-monitor", GetScriptEvalKeyRange,
+ GenerateEvalFlags),
+ MakeCmdAttr<CommandEvalRO>("eval_ro", -3, "read-only no-script skip-monitor", GetScriptEvalKeyRange),
+ MakeCmdAttr<CommandEvalSHARO>("evalsha_ro", -3, "read-only no-script skip-monitor", GetScriptEvalKeyRange),
+ MakeCmdAttr<CommandScript>("script", -2, "exclusive no-script skip-monitor", NO_KEY, GenerateScriptFlags), )
} // namespace redis
diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc
index e90487d..212d44a 100644
--- a/src/commands/cmd_server.cc
+++ b/src/commands/cmd_server.cc
@@ -1555,8 +1555,8 @@
MakeCmdAttr<CommandSelect>("select", 2, "read-only", NO_KEY),
MakeCmdAttr<CommandInfo>("info", -1, "read-only ok-loading", NO_KEY),
MakeCmdAttr<CommandRole>("role", 1, "read-only ok-loading", NO_KEY),
- MakeCmdAttr<CommandConfig>("config", -2, "read-only admin", NO_KEY, GenerateConfigFlag),
- MakeCmdAttr<CommandNamespace>("namespace", -2, "read-only admin", NO_KEY, GenerateNamespaceFlag),
+ MakeCmdAttr<CommandConfig>("config", -2, "read-only admin skip-monitor", NO_KEY, GenerateConfigFlag),
+ MakeCmdAttr<CommandNamespace>("namespace", -2, "read-only admin skip-monitor", NO_KEY, GenerateNamespaceFlag),
MakeCmdAttr<CommandKeys>("keys", 2, "read-only slow", NO_KEY),
MakeCmdAttr<CommandFlushDB>("flushdb", 1, "write no-dbsize-check exclusive", NO_KEY),
MakeCmdAttr<CommandFlushAll>("flushall", 1, "write no-dbsize-check exclusive admin", NO_KEY),
diff --git a/src/commands/commander.h b/src/commands/commander.h
index 61a3b79..7fede57 100644
--- a/src/commands/commander.h
+++ b/src/commands/commander.h
@@ -87,6 +87,8 @@
kCmdAuth = 1ULL << 10,
// "admin" flag, for commands that require admin permission
kCmdAdmin = 1ULL << 11,
+ // "skip-monitor" flag, for commands that should skip monitor feed
+ kCmdSkipMonitor = 1ULL << 12,
};
enum class CommandCategory : uint8_t {
@@ -352,6 +354,8 @@
flags |= kCmdNoMulti;
else if (flag == "no-script")
flags |= kCmdNoScript;
+ else if (flag == "skip-monitor")
+ flags |= kCmdSkipMonitor;
else if (flag == "no-dbsize-check")
flags |= kCmdNoDBSizeCheck;
else if (flag == "slow")
@@ -388,6 +392,7 @@
if (flags & kCmdBlocking) res.emplace_back("blocking");
if (flags & kCmdAuth) res.emplace_back("auth");
if (flags & kCmdAdmin) res.emplace_back("admin");
+ if (flags & kCmdSkipMonitor) res.emplace_back("skip-monitor");
return res;
}
diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc
index 06a1a45..e615486 100644
--- a/src/server/redis_connection.cc
+++ b/src/server/redis_connection.cc
@@ -573,7 +573,9 @@
}
}
- srv_->FeedMonitorConns(this, cmd_tokens);
+ if (!(cmd_flags & redis::kCmdSkipMonitor)) {
+ srv_->FeedMonitorConns(this, cmd_tokens);
+ }
// Break the execution loop when occurring the blocking command like BLPOP or BRPOP,
// it will suspend the connection and wait for the wakeup signal.
diff --git a/src/server/server.cc b/src/server/server.cc
index 6ca4014..04e62fe 100644
--- a/src/server/server.cc
+++ b/src/server/server.cc
@@ -370,13 +370,47 @@
}
}
+std::vector<std::string> Server::RedactSensitiveTokens(const std::vector<std::string> &tokens) {
+ if (tokens.empty()) return tokens;
+ std::string cmd = util::ToLower(tokens[0]);
+ if (cmd != "auth" && cmd != "hello") return tokens;
+
+ std::vector<std::string> redacted_tokens = tokens;
+ if (cmd == "auth" && tokens.size() >= 2) {
+ // AUTH password -> redact password (arg 1)
+ redacted_tokens[1] = "(redacted)";
+ } else if (cmd == "hello" && tokens.size() >= 3) {
+ // HELLO [version] [AUTH [username] password] [SETNAME name]
+ for (size_t i = 1; i < tokens.size(); ++i) {
+ std::string arg = util::ToLower(tokens[i]);
+ if (arg == "auth" && i + 1 < tokens.size()) {
+ size_t remaining_args = tokens.size() - i - 1;
+ if (remaining_args >= 2) {
+ // Check if this follows the pattern AUTH username password
+ // In this case, redact the password (arg i+2)
+ redacted_tokens[i + 2] = "(redacted)";
+ } else if (remaining_args == 1) {
+ // This follows the pattern AUTH password
+ // Redact the password (arg i+1)
+ redacted_tokens[i + 1] = "(redacted)";
+ }
+ break;
+ }
+ }
+ }
+ return redacted_tokens;
+}
+
void Server::FeedMonitorConns(redis::Connection *conn, const std::vector<std::string> &tokens) {
if (monitor_clients_ <= 0) return;
auto now_us = util::GetTimeStampUS();
std::string output =
fmt::format("{}.{} [{} {}]", now_us / 1000000, now_us % 1000000, conn->GetNamespace(), conn->GetAddr());
- for (const auto &token : tokens) {
+
+ auto redacted_tokens = RedactSensitiveTokens(tokens);
+
+ for (const auto &token : redacted_tokens) {
output += " \"";
output += util::EscapeString(token);
output += "\"";
diff --git a/src/server/server.h b/src/server/server.h
index 54f4c2c..7063a66 100644
--- a/src/server/server.h
+++ b/src/server/server.h
@@ -203,6 +203,7 @@
void CleanupExitedSlaves();
bool IsSlave() const { return !master_host_.empty(); }
void FeedMonitorConns(redis::Connection *conn, const std::vector<std::string> &tokens);
+ static std::vector<std::string> RedactSensitiveTokens(const std::vector<std::string> &tokens);
void IncrFetchFileThread() { fetch_file_threads_num_++; }
void DecrFetchFileThread() { fetch_file_threads_num_--; }
int GetFetchFileThreadNum() const { return fetch_file_threads_num_; }
diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc
index ff97d25..c8ddfc6 100644
--- a/src/storage/scripting.cc
+++ b/src/storage/scripting.cc
@@ -884,7 +884,9 @@
return raise_error ? RaiseError(lua) : 1;
}
- srv->FeedMonitorConns(conn, args);
+ if (!(cmd_flags & redis::kCmdSkipMonitor)) {
+ srv->FeedMonitorConns(conn, args);
+ }
RedisProtocolToLuaType(lua, output.data());
return 1;
diff --git a/tests/gocase/unit/introspection/introspection_test.go b/tests/gocase/unit/introspection/introspection_test.go
index 1b17a98..f8b0874 100644
--- a/tests/gocase/unit/introspection/introspection_test.go
+++ b/tests/gocase/unit/introspection/introspection_test.go
@@ -118,17 +118,6 @@
require.Regexp(t, "id=.* addr=.*:.* fd=.* name=.* age=.* idle=.* flags=N namespace=.* qbuf=.* .*obuf=.* cmd=client.*", v)
})
- t.Run("MONITOR can log executed commands", func(t *testing.T) {
- c := srv.NewTCPClient()
- defer func() { require.NoError(t, c.Close()) }()
- require.NoError(t, c.WriteArgs("MONITOR"))
- c.MustRead(t, "+OK")
- require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
- require.NoError(t, rdb.Get(ctx, "foo").Err())
- c.MustMatch(t, ".*set.*foo.*bar.*")
- c.MustMatch(t, ".*get.*foo.*")
- })
-
t.Run("CLIENT GETNAME should return NIL if name is not assigned", func(t *testing.T) {
require.EqualError(t, rdb.ClientGetName(ctx).Err(), redis.Nil.Error())
})
diff --git a/tests/gocase/unit/monitor/monitor_test.go b/tests/gocase/unit/monitor/monitor_test.go
new file mode 100644
index 0000000..a76a84a
--- /dev/null
+++ b/tests/gocase/unit/monitor/monitor_test.go
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+package monitor
+
+import (
+ "context"
+ "testing"
+
+ "github.com/redis/go-redis/v9"
+ "github.com/stretchr/testify/require"
+
+ "github.com/apache/kvrocks/tests/gocase/util"
+)
+
+func TestMonitor(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ t.Run("MONITOR can log executed commands", func(t *testing.T) {
+ c := srv.NewTCPClient()
+ defer func() { require.NoError(t, c.Close()) }()
+ require.NoError(t, c.WriteArgs("MONITOR"))
+ c.MustRead(t, "+OK")
+ require.NoError(t, rdb.Set(ctx, "foo", "bar", 0).Err())
+ require.NoError(t, rdb.Get(ctx, "foo").Err())
+ c.MustMatch(t, ".*hello.*")
+ c.MustMatch(t, ".*set.*foo.*bar.*")
+ c.MustMatch(t, ".*get.*foo.*")
+ })
+
+ t.Run("MONITOR should skip commands with 'skip-monitor' flag", func(t *testing.T) {
+ c := srv.NewTCPClient()
+ defer func() { require.NoError(t, c.Close()) }()
+ require.NoError(t, c.WriteArgs("MONITOR"))
+ c.MustRead(t, "+OK")
+
+ // Execute commands that should be skipped
+ require.NoError(t, rdb.ConfigSet(ctx, "slave-read-only", "yes").Err())
+ require.NoError(t, rdb.ConfigGet(ctx, "slave-read-only").Err())
+
+ require.NoError(t, rdb.Ping(ctx).Err())
+ require.NoError(t, rdb.FlushAll(ctx).Err())
+ require.NoError(t, rdb.Set(ctx, "test", "value", 0).Err())
+ require.NoError(t, rdb.Get(ctx, "test").Err())
+ require.NoError(t, rdb.Info(ctx, "server").Err())
+ c.MustMatch(t, ".*ping.*")
+ c.MustMatch(t, ".*flushall.*")
+ c.MustMatch(t, ".*set.*test.*value.*")
+ c.MustMatch(t, ".*get.*test.*")
+ c.MustMatch(t, ".*info.*server.*")
+ })
+}
+
+func TestMonitorRedactPassword(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{"requirepass": "testpass"})
+ defer srv.Close()
+
+ c := srv.NewTCPClient()
+ defer func() { require.NoError(t, c.Close()) }()
+ require.NoError(t, c.WriteArgs("AUTH", "testpass"))
+ c.MustRead(t, "+OK")
+
+ require.NoError(t, c.WriteArgs("MONITOR"))
+ c.MustRead(t, "+OK")
+
+ rdb := srv.NewClientWithOption(&redis.Options{
+ Password: "testpass",
+ })
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ ctx := context.Background()
+ require.NoError(t, rdb.Do(ctx, "AUTH", "testpass").Err())
+ require.NoError(t, rdb.Do(ctx, "HELLO", "3", "AUTH", "testpass").Err())
+ require.NoError(t, rdb.Do(ctx, "HELLO", "3", "AUTH", "default", "testpass").Err())
+
+ // Client uses HELLO to AUTH, so it will have an extra HELLO command
+ c.MustMatch(t, `.*hello.*3.*auth.*default.*\(redacted\).*`)
+ c.MustMatch(t, `.*AUTH.*\(redacted\).*`)
+ c.MustMatch(t, `.*HELLO.*3.*AUTH.*\(redacted\).*`)
+ c.MustMatch(t, `.*HELLO.*3.*AUTH.*default.*\(redacted\).*`)
+}
