From b1ec8ddcfd2cd24fd14791ce7c64ba9dc8bba5be Mon Sep 17 00:00:00 2001 From: naglera Date: Wed, 14 Aug 2024 08:14:13 +0000 Subject: [PATCH] Fix comments config should be boolean leave room to test each argument based on command type Signed-off-by: naglera --- src/config.c | 2 +- src/debug.c | 18 ++++++++++-------- src/networking.c | 5 ++--- src/replication.c | 4 ++-- valkey.conf | 4 ++-- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/config.c b/src/config.c index d7c9fa2122..75977523d0 100644 --- a/src/config.c +++ b/src/config.c @@ -3106,6 +3106,7 @@ standardConfig static_configs[] = { createBoolConfig("extended-redis-compatibility", NULL, MODIFIABLE_CONFIG, server.extended_redis_compat, 0, NULL, updateExtendedRedisCompat), createBoolConfig("enable-debug-assert", NULL, IMMUTABLE_CONFIG | HIDDEN_CONFIG, server.enable_debug_assert, 0, NULL, NULL), createBoolConfig("cluster-slot-stats-enabled", NULL, MODIFIABLE_CONFIG, server.cluster_slot_stats_enabled, 0, NULL, NULL), + createBoolConfig("hide-user-data-from-log", NULL, MODIFIABLE_CONFIG, server.hide_user_data_from_log, 0, NULL, NULL), /* String Configs */ createStringConfig("aclfile", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.acl_filename, "", NULL, NULL), @@ -3198,7 +3199,6 @@ standardConfig static_configs[] = { createIntConfig("watchdog-period", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, 0, INT_MAX, server.watchdog_period, 0, INTEGER_CONFIG, NULL, updateWatchdogPeriod), createIntConfig("shutdown-timeout", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.shutdown_timeout, 10, INTEGER_CONFIG, NULL, NULL), createIntConfig("repl-diskless-sync-max-replicas", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.repl_diskless_sync_max_replicas, 0, INTEGER_CONFIG, NULL, NULL), - createIntConfig("hide-user-data-from-log", NULL, MODIFIABLE_CONFIG, -1, INT_MAX, server.hide_user_data_from_log, 0, INTEGER_CONFIG, NULL, NULL), /* Unsigned int configs */ createUIntConfig("maxclients", NULL, MODIFIABLE_CONFIG, 1, UINT_MAX, server.maxclients, 10000, INTEGER_CONFIG, NULL, updateMaxclients), diff --git a/src/debug.c b/src/debug.c index 57474e1124..fb73f31aa9 100644 --- a/src/debug.c +++ b/src/debug.c @@ -1031,12 +1031,14 @@ __attribute__((noinline)) void _serverAssert(const char *estr, const char *file, bugReportEnd(0, 0); } -/* Checks if logging the provided argument is permitted based on the current - * configuration for hiding user data from logs. */ -int canLogClientArg(const client *c, int idx) { +/* Checks if the argument at the given index should be redacted from logs. */ +int shouldRedactArg(const client *c, int idx) { serverAssert(idx < c->argc); - if (server.hide_user_data_from_log < 0) return 1; - return idx < server.hide_user_data_from_log; + /* Don't redact if the config is disabled */ + if (!server.hide_user_data_from_log) return 0; + /* first_sensitive_arg_idx value should be changed based on the command type. */ + int first_sensitive_arg_idx = 1; + return idx >= first_sensitive_arg_idx; } void _serverAssertPrintClientInfo(const client *c) { @@ -1049,7 +1051,7 @@ void _serverAssertPrintClientInfo(const client *c) { serverLog(LL_WARNING, "client->conn = %s", connGetInfo(c->conn, conninfo, sizeof(conninfo))); serverLog(LL_WARNING, "client->argc = %d", c->argc); for (j = 0; j < c->argc; j++) { - if (!canLogClientArg(c, j)) { + if (!shouldRedactArg(c, j)) { serverLog(LL_WARNING | LL_RAW, "client->argv[%d]: %zu bytes ", j, sdslen((sds)c->argv[j]->ptr)); continue; } @@ -1253,7 +1255,7 @@ static void *getAndSetMcontextEip(ucontext_t *uc, void *eip) { VALKEY_NO_SANITIZE("address") void logStackContent(void **sp) { - if (server.hide_user_data_from_log > 0) { + if (server.hide_user_data_from_log) { serverLog(LL_NOTICE, "hide-user-data-from-log is on, skip logging stack content to avoid spilling PII."); return; } @@ -1872,7 +1874,7 @@ void logCurrentClient(client *cc, const char *title) { sdsfree(client); serverLog(LL_WARNING | LL_RAW, "argc: '%d'\n", cc->argc); for (j = 0; j < cc->argc; j++) { - if (!canLogClientArg(cc, j)) { + if (!shouldRedactArg(cc, j)) { serverLog(LL_WARNING | LL_RAW, "client->argv[%d]: %zu bytes ", j, sdslen((sds)cc->argv[j]->ptr)); continue; } diff --git a/src/networking.c b/src/networking.c index 9f42e792e8..5f3e1953b5 100644 --- a/src/networking.c +++ b/src/networking.c @@ -3277,14 +3277,13 @@ sds catClientInfoString(sds s, client *client) { used_blocks_of_repl_buf = last->id - cur->id + 1; } - int hide_data = server.hide_user_data_from_log > 0; /* clang-format off */ sds ret = sdscatfmt(s, FMTARGS( "id=%U", (unsigned long long) client->id, " addr=%s", getClientPeerId(client), " laddr=%s", getClientSockname(client), " %s", connGetInfo(client->conn, conninfo, sizeof(conninfo)), - " name=%s", hide_data ? "*redacted*" : (client->name ? (char*)client->name->ptr : ""), + " name=%s", server.hide_user_data_from_log ? "*redacted*" : (client->name ? (char*)client->name->ptr : ""), " age=%I", (long long)(commandTimeSnapshot() / 1000 - client->ctime), " idle=%I", (long long)(server.unixtime - client->last_interaction), " flags=%s", flags, @@ -3306,7 +3305,7 @@ sds catClientInfoString(sds s, client *client) { " tot-mem=%U", (unsigned long long) total_mem, " events=%s", events, " cmd=%s", client->lastcmd ? client->lastcmd->fullname : "NULL", - " user=%s", hide_data ? "*redacted*" : (client->user ? client->user->name : "(superuser)"), + " user=%s", server.hide_user_data_from_log ? "*redacted*" : (client->user ? client->user->name : "(superuser)"), " redir=%I", (client->flag.tracking) ? (long long) client->client_tracking_redirection : -1, " resp=%i", client->resp, " lib-name=%s", client->lib_name ? (char*)client->lib_name->ptr : "", diff --git a/src/replication.c b/src/replication.c index e5dfd0959a..d9d5e9cf95 100644 --- a/src/replication.c +++ b/src/replication.c @@ -614,7 +614,7 @@ void replicationFeedReplicas(int dictid, robj **argv, int argc) { void showLatestBacklog(void) { if (server.repl_backlog == NULL) return; if (listLength(server.repl_buffer_blocks) == 0) return; - if (server.hide_user_data_from_log > 0) { + if (server.hide_user_data_from_log) { serverLog(LL_NOTICE, "hide-user-data-from-log is on, skip logging backlog content to avoid spilling user data."); return; } @@ -649,7 +649,7 @@ void replicationFeedStreamFromPrimaryStream(char *buf, size_t buflen) { /* Debugging: this is handy to see the stream sent from primary * to replicas. Disabled with if(0). */ if (0) { - if (server.hide_user_data_from_log < 0) { + if (server.hide_user_data_from_log) { printf("%zu:", buflen); for (size_t j = 0; j < buflen; j++) { printf("%c", isprint(buf[j]) ? buf[j] : '.'); diff --git a/valkey.conf b/valkey.conf index 97b2dc1bf2..65de4d1bc8 100644 --- a/valkey.conf +++ b/valkey.conf @@ -389,8 +389,8 @@ always-show-logo no # To prevent sensitive user information, such as PII, from being recorded in the # server log file, this feature is enabled by default. If you need to log user # data for debugging or troubleshooting purposes, you can disable this feature -# by changing the config value to -1. -hide-user-data-from-log 1 +# by changing the config value to no. +hide-user-data-from-log yes # By default, the server modifies the process title (as seen in 'top' and 'ps') to # provide some runtime information. It is possible to disable this and leave