gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Add configuration hide-user-data-from-log to hide user data from server logs (#877)

Browse Source 
Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

---------

Signed-off-by: Amit Nagler <anagler123@gmail.com>
This commit is contained in:
Amit Nagler 2024-09-02 19:50:36 +03:00 committed by GitHub
parent 5693fe4664
commit 5fdb47c2e2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 74 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/acl.c
View File

@ -2678,7 +2678,7 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username
/* if we have a real client from the network, use it (could be missing on module timers) */
client *realclient = server.current_client ? server.current_client : c;
le->cinfo = catClientInfoString(sdsempty(), realclient);
le->cinfo = catClientInfoString(sdsempty(), realclient, 0);
le->context = context;
/* Try to match this entry with past ones, to see if we can just

1
src/config.c
View File

@ -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, 1, NULL, NULL),
/* String Configs */
createStringConfig("aclfile", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.acl_filename, "", NULL, NULL),

26
src/debug.c
View File

@ -1041,6 +1041,16 @@ __attribute__((noinline)) void _serverAssert(const char *estr, const char *file,
bugReportEnd(0, 0);
}
/* Checks if the argument at the given index should be redacted from logs. */
int shouldRedactArg(const client *c, int idx) {
serverAssert(idx < c->argc);
/* 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) {
int j;
char conninfo[CONN_INFO_LEN];
@ -1051,6 +1061,10 @@ 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 (shouldRedactArg(c, j)) {
serverLog(LL_WARNING, "client->argv[%d]: %zu bytes ", j, sdslen((sds)c->argv[j]->ptr));
continue;
}
char buf[128];
char *arg;
@ -1251,6 +1265,10 @@ static void *getAndSetMcontextEip(ucontext_t *uc, void *eip) {
VALKEY_NO_SANITIZE("address")
void logStackContent(void **sp) {
if (server.hide_user_data_from_log) {
serverLog(LL_NOTICE, "hide-user-data-from-log is on, skip logging stack content to avoid spilling user data.");
return;
}
int i;
for (i = 15; i >= 0; i--) {
unsigned long addr = (unsigned long)sp + i;
@ -1826,7 +1844,7 @@ void logServerInfo(void) {
}
serverLogRaw(LL_WARNING | LL_RAW, infostring);
serverLogRaw(LL_WARNING | LL_RAW, "\n------ CLIENT LIST OUTPUT ------\n");
clients = getAllClientsInfoString(-1);
clients = getAllClientsInfoString(-1, server.hide_user_data_from_log);
serverLogRaw(LL_WARNING | LL_RAW, clients);
sdsfree(infostring);
sdsfree(clients);
@ -1861,11 +1879,15 @@ void logCurrentClient(client *cc, const char *title) {
int j;
serverLog(LL_WARNING | LL_RAW, "\n------ %s CLIENT INFO ------\n", title);
client = catClientInfoString(sdsempty(), cc);
client = catClientInfoString(sdsempty(), cc, server.hide_user_data_from_log);
serverLog(LL_WARNING | LL_RAW, "%s\n", client);
sdsfree(client);
serverLog(LL_WARNING | LL_RAW, "argc: '%d'\n", cc->argc);
for (j = 0; j < cc->argc; j++) {
if (shouldRedactArg(cc, j)) {
serverLog(LL_WARNING | LL_RAW, "client->argv[%d]: %zu bytes ", j, sdslen((sds)cc->argv[j]->ptr));
continue;
}
robj *decoded;
decoded = getDecodedObject(cc->argv[j]);
sds repr = sdscatrepr(sdsempty(), decoded->ptr, min(sdslen(decoded->ptr), 128));

31
src/networking.c
View File

@ -1253,7 +1253,7 @@ void AddReplyFromClient(client *dst, client *src) {
* for some reason the output limits don't reach the same decision (maybe
* they changed) */
if (src->flag.close_asap) {
sds client = catClientInfoString(sdsempty(), dst);
sds client = catClientInfoString(sdsempty(), dst, server.hide_user_data_from_log);
freeClientAsync(dst);
serverLog(LL_WARNING, "Client %s scheduled to be closed ASAP for overcoming of output buffer limits.", client);
sdsfree(client);
@ -1810,7 +1810,7 @@ void logInvalidUseAndFreeClientAsync(client *c, const char *fmt, ...) {
sds info = sdscatvprintf(sdsempty(), fmt, ap);
va_end(ap);
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_WARNING, "%s, disconnecting it: %s", info, client);
sdsfree(info);
@ -2246,7 +2246,7 @@ void sendReplyToClient(connection *conn) {
}
void handleQbLimitReached(client *c) {
sds ci = catClientInfoString(sdsempty(), c), bytes = sdsempty();
sds ci = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log), bytes = sdsempty();
bytes = sdscatrepr(bytes, c->querybuf, 64);
serverLog(LL_WARNING, "Closing client that reached max query buffer length: %s (qbuf initial bytes: %s)", ci,
bytes);
@ -2276,7 +2276,7 @@ int handleReadResult(client *c) {
}
} else if (c->nread == 0) {
if (server.verbosity <= LL_VERBOSE) {
sds info = catClientInfoString(sdsempty(), c);
sds info = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_VERBOSE, "Client closed connection %s", info);
sdsfree(info);
}
@ -2679,7 +2679,7 @@ void processInlineBuffer(client *c) {
#define PROTO_DUMP_LEN 128
static void setProtocolError(const char *errstr, client *c) {
if (server.verbosity <= LL_VERBOSE || c->flag.primary) {
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
/* Sample some protocol to given an idea about what was inside. */
char buf[256];
@ -3260,7 +3260,7 @@ int isClientConnIpV6(client *c) {
/* Concatenate a string representing the state of a client in a human
* readable format, into the sds string 's'. */
sds catClientInfoString(sds s, client *client) {
sds catClientInfoString(sds s, client *client, int hide_user_data) {
if (!server.crashed) waitForClientIO(client);
char flags[17], events[3], conninfo[CONN_INFO_LEN], *p;
@ -3307,14 +3307,13 @@ sds catClientInfoString(sds s, client *client) {
replBufBlock *cur = listNodeValue(client->ref_repl_buf_node);
used_blocks_of_repl_buf = last->id - cur->id + 1;
}
/* 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", client->name ? (char*)client->name->ptr : "",
" name=%s", hide_user_data ? "*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,
@ -3336,7 +3335,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", client->user ? client->user->name : "(superuser)",
" user=%s", hide_user_data ? "*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 : "",
@ -3348,7 +3347,7 @@ sds catClientInfoString(sds s, client *client) {
return ret;
}
sds getAllClientsInfoString(int type) {
sds getAllClientsInfoString(int type, int hide_user_data) {
listNode *ln;
listIter li;
client *client;
@ -3358,7 +3357,7 @@ sds getAllClientsInfoString(int type) {
while ((ln = listNext(&li)) != NULL) {
client = listNodeValue(ln);
if (type != -1 && getClientType(client) != type) continue;
o = catClientInfoString(o, client);
o = catClientInfoString(o, client, hide_user_data);
o = sdscatlen(o, "\n", 1);
}
return o;
@ -3554,7 +3553,7 @@ NULL
addReplyLongLong(c, c->id);
} else if (!strcasecmp(c->argv[1]->ptr, "info") && c->argc == 2) {
/* CLIENT INFO */
sds o = catClientInfoString(sdsempty(), c);
sds o = catClientInfoString(sdsempty(), c, 0);
o = sdscatlen(o, "\n", 1);
addReplyVerbatim(c, o, sdslen(o), "txt");
sdsfree(o);
@ -3579,7 +3578,7 @@ NULL
}
client *cl = lookupClientByID(cid);
if (cl) {
o = catClientInfoString(o, cl);
o = catClientInfoString(o, cl, 0);
o = sdscatlen(o, "\n", 1);
}
}
@ -3588,7 +3587,7 @@ NULL
return;
}
if (!o) o = getAllClientsInfoString(type);
if (!o) o = getAllClientsInfoString(type, 0);
addReplyVerbatim(c, o, sdslen(o), "txt");
sdsfree(o);
} else if (!strcasecmp(c->argv[1]->ptr, "reply") && c->argc == 3) {
@ -4425,7 +4424,7 @@ int closeClientOnOutputBufferLimitReached(client *c, int async) {
(c->flag.close_asap && !(c->flag.protected_rdb_channel)))
return 0;
if (checkClientOutputBufferLimits(c)) {
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
/* Remove RDB connection protection on COB overrun */
if (async || c->flag.protected_rdb_channel) {
@ -4759,7 +4758,7 @@ void evictClients(void) {
listNode *ln = listNext(&bucket_iter);
if (ln) {
client *c = ln->value;
sds ci = catClientInfoString(sdsempty(), c);
sds ci = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_NOTICE, "Evicting client: %s", ci);
freeClient(c);
sdsfree(ci);

21
src/replication.c
View File

@ -614,6 +614,11 @@ 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) {
serverLog(LL_NOTICE,
"hide-user-data-from-log is on, skip logging backlog content to avoid spilling user data.");
return;
}
size_t dumplen = 256;
if (server.repl_backlog->histlen < (long long)dumplen) dumplen = server.repl_backlog->histlen;
@ -645,11 +650,13 @@ 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) {
printf("%zu:", buflen);
for (size_t j = 0; j < buflen; j++) {
printf("%c", isprint(buf[j]) ? buf[j] : '.');
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] : '.');
}
printf("\n");
}
printf("\n");
}
/* There must be replication backlog if having attached replicas. */
@ -1043,7 +1050,7 @@ void syncCommand(client *c) {
} else {
replicationUnsetPrimary();
}
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_NOTICE, "PRIMARY MODE enabled (failover request from '%s')", client);
sdsfree(client);
} else {
@ -3877,7 +3884,7 @@ void replicaofCommand(client *c) {
if (!strcasecmp(c->argv[1]->ptr, "no") && !strcasecmp(c->argv[2]->ptr, "one")) {
if (server.primary_host) {
replicationUnsetPrimary();
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_NOTICE, "PRIMARY MODE enabled (user request from '%s')", client);
sdsfree(client);
}
@ -3906,7 +3913,7 @@ void replicaofCommand(client *c) {
/* There was no previous primary or the user specified a different one,
* we can continue. */
replicationSetPrimary(c->argv[1]->ptr, port, 0);
sds client = catClientInfoString(sdsempty(), c);
sds client = catClientInfoString(sdsempty(), c, server.hide_user_data_from_log);
serverLog(LL_NOTICE, "REPLICAOF %s:%d enabled (user request from '%s')", server.primary_host,
server.primary_port, client);
sdsfree(client);

17
src/server.h
View File

@ -1868,12 +1868,13 @@ struct valkeyServer {
durationStats duration_stats[EL_DURATION_TYPE_NUM];
/* Configuration */
int verbosity; /* Loglevel verbosity */
int maxidletime; /* Client timeout in seconds */
int tcpkeepalive; /* Set SO_KEEPALIVE if non-zero. */
int active_expire_enabled; /* Can be disabled for testing purposes. */
int active_expire_effort; /* From 1 (default) to 10, active effort. */
int lazy_expire_disabled; /* If > 0, don't trigger lazy expire */
int verbosity; /* Loglevel verbosity */
int hide_user_data_from_log; /* Hide or redact user data, or data that may contain user data, from the log. */
int maxidletime; /* Client timeout in seconds */
int tcpkeepalive; /* Set SO_KEEPALIVE if non-zero. */
int active_expire_enabled; /* Can be disabled for testing purposes. */
int active_expire_effort; /* From 1 (default) to 10, active effort. */
int lazy_expire_disabled; /* If > 0, don't trigger lazy expire */
int active_defrag_enabled;
int sanitize_dump_payload; /* Enables deep sanitization for ziplist and listpack in RDB and RESTORE. */
int skip_checksum_validation; /* Disable checksum validation for RDB and RESTORE payload. */
@ -2838,8 +2839,8 @@ void *dupClientReplyValue(void *o);
char *getClientPeerId(client *client);
char *getClientSockName(client *client);
int isClientConnIpV6(client *c);
sds catClientInfoString(sds s, client *client);
sds getAllClientsInfoString(int type);
sds catClientInfoString(sds s, client *client, int hide_user_data);
sds getAllClientsInfoString(int type, int hide_user_data);
int clientSetName(client *c, robj *name, const char **err);
void rewriteClientCommandVector(client *c, int argc, ...);
void rewriteClientCommandArgument(client *c, int i, robj *newval);

2
tests/assets/default.conf
View File

@ -37,3 +37,5 @@ propagation-error-behavior panic
shutdown-on-sigterm force
enable-debug-assert yes
hide-user-data-from-log no

8
valkey.conf
View File

@ -387,6 +387,14 @@ databases 16
# ASCII art logo in startup logs by setting the following option to yes.
always-show-logo no
# User data, including keys, values, client names, and ACL usernames, can be
# logged as part of assertions and other error cases. To prevent sensitive user
# information, such as PII, from being recorded in the server log file, this
# user data is hidden from the log 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 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
# the process name as executed by setting the following to no.