diff --git a/src/acl.c b/src/acl.c index f48fb405e..aecd0629b 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1873,6 +1873,10 @@ void addACLLogEntry(client *c, int reason, int argpos, sds username) { void aclCommand(client *c) { char *sub = c->argv[1]->ptr; if (!strcasecmp(sub,"setuser") && c->argc >= 3) { + /* Consider information about passwords or permissions + * to be sensitive, which will be the arguments for this + * subcommand. */ + preventCommandLogging(c); sds username = c->argv[2]->ptr; /* Check username validity. */ if (ACLStringHasSpaces(username,sdslen(username))) { diff --git a/src/blocked.c b/src/blocked.c index 09e17213c..2009c9a24 100644 --- a/src/blocked.c +++ b/src/blocked.c @@ -106,12 +106,20 @@ void blockClient(client *c, int btype) { void updateStatsOnUnblock(client *c, long blocked_us, long reply_us){ const ustime_t total_cmd_duration = c->duration + blocked_us + reply_us; c->lastcmd->microseconds += total_cmd_duration; + /* Log the command into the Slow log if needed. */ - if (!(c->lastcmd->flags & CMD_SKIP_SLOWLOG)) { + if (!(c->lastcmd->flags & CMD_SKIP_SLOWLOG) && + !(c->flags & CLIENT_PREVENT_LOGGING)) + { slowlogPushEntryIfNeeded(c,c->argv,c->argc,total_cmd_duration); /* Log the reply duration event. */ latencyAddSampleIfNeeded("command-unblocking",reply_us/1000); } + + /* Always clear the prevent logging field now. */ + if (c->flags & CLIENT_PREVENT_LOGGING) { + c->flags &= ~CLIENT_PREVENT_LOGGING; + } } /* This function is called in the beforeSleep() function of the event loop diff --git a/src/config.c b/src/config.c index 2f4a13ae8..2db13820c 100644 --- a/src/config.c +++ b/src/config.c @@ -241,11 +241,16 @@ typedef struct typeInterface { typedef struct standardConfig { const char *name; /* The user visible name of this config */ const char *alias; /* An alias that can also be used for this config */ - const int modifiable; /* Can this value be updated by CONFIG SET? */ + const unsigned int flags; /* Flags for this specific config */ typeInterface interface; /* The function pointers that define the type interface */ typeData data; /* The type specific data exposed used by the interface */ } standardConfig; +#define MODIFIABLE_CONFIG 0 /* This is the implied default for a standard + * config, which is mutable. */ +#define IMMUTABLE_CONFIG (1ULL<<0) /* Can this value only be set at startup? */ +#define SENSITIVE_CONFIG (1ULL<<1) /* Does this value contain sensitive information */ + standardConfig configs[]; /*----------------------------------------------------------------------------- @@ -712,9 +717,13 @@ void configSetCommand(client *c) { /* Iterate the configs that are standard */ for (standardConfig *config = configs; config->name != NULL; config++) { - if(config->modifiable && (!strcasecmp(c->argv[2]->ptr,config->name) || + if (!(config->flags & IMMUTABLE_CONFIG) && + (!strcasecmp(c->argv[2]->ptr,config->name) || (config->alias && !strcasecmp(c->argv[2]->ptr,config->alias)))) { + if (config->flags & SENSITIVE_CONFIG) { + preventCommandLogging(c); + } if (!config->interface.set(config->data,o->ptr,1,&errstr)) { goto badfmt; } @@ -1709,13 +1718,10 @@ int rewriteConfig(char *path, int force_all) { #define LOADBUF_SIZE 256 static char loadbuf[LOADBUF_SIZE]; -#define MODIFIABLE_CONFIG 1 -#define IMMUTABLE_CONFIG 0 - -#define embedCommonConfig(config_name, config_alias, is_modifiable) \ +#define embedCommonConfig(config_name, config_alias, config_flags) \ .name = (config_name), \ .alias = (config_alias), \ - .modifiable = (is_modifiable), + .flags = (config_flags), #define embedConfigInterface(initfn, setfn, getfn, rewritefn) .interface = { \ .init = (initfn), \ @@ -1766,8 +1772,8 @@ static void boolConfigRewrite(typeData data, const char *name, struct rewriteCon rewriteConfigYesNoOption(state, name,*(data.yesno.config), data.yesno.default_value); } -#define createBoolConfig(name, alias, modifiable, config_addr, default, is_valid, update) { \ - embedCommonConfig(name, alias, modifiable) \ +#define createBoolConfig(name, alias, flags, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, flags) \ embedConfigInterface(boolConfigInit, boolConfigSet, boolConfigGet, boolConfigRewrite) \ .data.yesno = { \ .config = &(config_addr), \ @@ -1839,8 +1845,8 @@ static void sdsConfigRewrite(typeData data, const char *name, struct rewriteConf #define ALLOW_EMPTY_STRING 0 #define EMPTY_STRING_IS_NULL 1 -#define createStringConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \ - embedCommonConfig(name, alias, modifiable) \ +#define createStringConfig(name, alias, flags, empty_to_null, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, flags) \ embedConfigInterface(stringConfigInit, stringConfigSet, stringConfigGet, stringConfigRewrite) \ .data.string = { \ .config = &(config_addr), \ @@ -1851,8 +1857,8 @@ static void sdsConfigRewrite(typeData data, const char *name, struct rewriteConf } \ } -#define createSDSConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \ - embedCommonConfig(name, alias, modifiable) \ +#define createSDSConfig(name, alias, flags, empty_to_null, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, flags) \ embedConfigInterface(sdsConfigInit, sdsConfigSet, sdsConfigGet, sdsConfigRewrite) \ .data.sds = { \ .config = &(config_addr), \ @@ -1907,8 +1913,8 @@ static void enumConfigRewrite(typeData data, const char *name, struct rewriteCon rewriteConfigEnumOption(state, name,*(data.enumd.config), data.enumd.enum_value, data.enumd.default_value); } -#define createEnumConfig(name, alias, modifiable, enum, config_addr, default, is_valid, update) { \ - embedCommonConfig(name, alias, modifiable) \ +#define createEnumConfig(name, alias, flags, enum, config_addr, default, is_valid, update) { \ + embedCommonConfig(name, alias, flags) \ embedConfigInterface(enumConfigInit, enumConfigSet, enumConfigGet, enumConfigRewrite) \ .data.enumd = { \ .config = &(config_addr), \ @@ -2061,8 +2067,8 @@ static void numericConfigRewrite(typeData data, const char *name, struct rewrite #define INTEGER_CONFIG 0 #define MEMORY_CONFIG 1 -#define embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) { \ - embedCommonConfig(name, alias, modifiable) \ +#define embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) { \ + embedCommonConfig(name, alias, flags) \ embedConfigInterface(numericConfigInit, numericConfigSet, numericConfigGet, numericConfigRewrite) \ .data.numeric = { \ .lower_bound = (lower), \ @@ -2072,71 +2078,71 @@ static void numericConfigRewrite(typeData data, const char *name, struct rewrite .update_fn = (update), \ .is_memory = (memory), -#define createIntConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createIntConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_INT, \ .config.i = &(config_addr) \ } \ } -#define createUIntConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createUIntConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_UINT, \ .config.ui = &(config_addr) \ } \ } -#define createLongConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createLongConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_LONG, \ .config.l = &(config_addr) \ } \ } -#define createULongConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createULongConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_ULONG, \ .config.ul = &(config_addr) \ } \ } -#define createLongLongConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createLongLongConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_LONG_LONG, \ .config.ll = &(config_addr) \ } \ } -#define createULongLongConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createULongLongConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_ULONG_LONG, \ .config.ull = &(config_addr) \ } \ } -#define createSizeTConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createSizeTConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_SIZE_T, \ .config.st = &(config_addr) \ } \ } -#define createSSizeTConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createSSizeTConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_SSIZE_T, \ .config.sst = &(config_addr) \ } \ } -#define createTimeTConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createTimeTConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_TIME_T, \ .config.tt = &(config_addr) \ } \ } -#define createOffTConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ - embedCommonNumericalConfig(name, alias, modifiable, lower, upper, config_addr, default, memory, is_valid, update) \ +#define createOffTConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ + embedCommonNumericalConfig(name, alias, flags, lower, upper, config_addr, default, memory, is_valid, update) \ .numeric_type = NUMERIC_TYPE_OFF_T, \ .config.ot = &(config_addr) \ } \ @@ -2429,7 +2435,7 @@ standardConfig configs[] = { createStringConfig("unixsocket", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.unixsocket, NULL, NULL, NULL), createStringConfig("pidfile", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.pidfile, NULL, NULL, NULL), createStringConfig("replica-announce-ip", "slave-announce-ip", MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.slave_announce_ip, NULL, NULL, NULL), - createStringConfig("masteruser", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masteruser, NULL, NULL, NULL), + createStringConfig("masteruser", NULL, MODIFIABLE_CONFIG | SENSITIVE_CONFIG, EMPTY_STRING_IS_NULL, server.masteruser, NULL, NULL, NULL), createStringConfig("cluster-announce-ip", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.cluster_announce_ip, NULL, NULL, NULL), createStringConfig("syslog-ident", NULL, IMMUTABLE_CONFIG, ALLOW_EMPTY_STRING, server.syslog_ident, "redis", NULL, NULL), createStringConfig("dbfilename", NULL, MODIFIABLE_CONFIG, ALLOW_EMPTY_STRING, server.rdb_filename, "dump.rdb", isValidDBfilename, NULL), @@ -2442,8 +2448,8 @@ standardConfig configs[] = { createStringConfig("proc-title-template", NULL, MODIFIABLE_CONFIG, ALLOW_EMPTY_STRING, server.proc_title_template, CONFIG_DEFAULT_PROC_TITLE_TEMPLATE, isValidProcTitleTemplate, updateProcTitleTemplate), /* SDS Configs */ - createSDSConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), - createSDSConfig("requirepass", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.requirepass, NULL, NULL, updateRequirePass), + createSDSConfig("masterauth", NULL, MODIFIABLE_CONFIG | SENSITIVE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL), + createSDSConfig("requirepass", NULL, MODIFIABLE_CONFIG | SENSITIVE_CONFIG, EMPTY_STRING_IS_NULL, server.requirepass, NULL, NULL, updateRequirePass), /* Enum Configs */ createEnumConfig("supervised", NULL, IMMUTABLE_CONFIG, supervised_mode_enum, server.supervised_mode, SUPERVISED_NONE, NULL, NULL), diff --git a/src/networking.c b/src/networking.c index 50e4b71bc..7d184bae7 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2941,6 +2941,7 @@ void helloCommand(client *c) { int moreargs = (c->argc-1) - j; const char *opt = c->argv[j]->ptr; if (!strcasecmp(opt,"AUTH") && moreargs >= 2) { + preventCommandLogging(c); if (ACLAuthenticateUser(c, c->argv[j+1], c->argv[j+2]) == C_ERR) { addReplyError(c,"-WRONGPASS invalid username-password pair or user is disabled."); return; diff --git a/src/server.c b/src/server.c index 5cfb13bbb..f535b525b 100644 --- a/src/server.c +++ b/src/server.c @@ -901,7 +901,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"hello",helloCommand,-1, - "no-auth no-script fast no-monitor ok-loading ok-stale no-slowlog @connection", + "no-auth no-script fast no-monitor ok-loading ok-stale @connection", 0,NULL,0,0,0,0,0,0}, /* EVAL can modify the dataset, however it is not flagged as a write @@ -1091,7 +1091,7 @@ struct redisCommand redisCommandTable[] = { 0,NULL,0,0,0,0,0,0}, {"acl",aclCommand,-2, - "admin no-script no-slowlog ok-loading ok-stale", + "admin no-script ok-loading ok-stale", 0,NULL,0,0,0,0,0,0}, {"stralgo",stralgoCommand,-2, @@ -3619,6 +3619,12 @@ void preventCommandPropagation(client *c) { c->flags |= CLIENT_PREVENT_PROP; } +/* Avoid logging any information about this client's arguments + * since they contain sensitive information. */ +void preventCommandLogging(client *c) { + c->flags |= CLIENT_PREVENT_LOGGING; +} + /* AOF specific version of preventCommandPropagation(). */ void preventCommandAOF(client *c) { c->flags |= CLIENT_PREVENT_AOF_PROP; @@ -3731,6 +3737,13 @@ void call(client *c, int flags) { server.lua_caller->flags |= CLIENT_FORCE_AOF; } + /* Some commands may contain sensitive data that should + * not be available in the slowlog. */ + if ((c->flags & CLIENT_PREVENT_LOGGING) && !(c->flags & CLIENT_BLOCKED)) { + c->flags &= ~CLIENT_PREVENT_LOGGING; + flags &= ~CMD_CALL_SLOWLOG; + } + /* Log the command into the Slow log if needed, and populate the * per-command statistics that we show in INFO commandstats. */ if (flags & CMD_CALL_SLOWLOG && !(c->cmd->flags & CMD_SKIP_SLOWLOG)) { diff --git a/src/server.h b/src/server.h index 3126db8f0..81395f4bb 100644 --- a/src/server.h +++ b/src/server.h @@ -274,6 +274,7 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT]; and AOF client */ #define CLIENT_REPL_RDBONLY (1ULL<<42) /* This client is a replica that only wants RDB without replication buffer. */ +#define CLIENT_PREVENT_LOGGING (1ULL<<43) /* Prevent logging of command to slowlog */ /* Client block type (btype field in client structure) * if CLIENT_BLOCKED flag is set. */ @@ -2196,6 +2197,7 @@ void redisOpArrayInit(redisOpArray *oa); void redisOpArrayFree(redisOpArray *oa); void forceCommandPropagation(client *c, int flags); void preventCommandPropagation(client *c); +void preventCommandLogging(client *c); void preventCommandAOF(client *c); void preventCommandReplication(client *c); int prepareForShutdown(int flags); diff --git a/tests/unit/slowlog.tcl b/tests/unit/slowlog.tcl index e782682e4..4cbf6d8f0 100644 --- a/tests/unit/slowlog.tcl +++ b/tests/unit/slowlog.tcl @@ -41,6 +41,22 @@ start_server {tags {"slowlog"} overrides {slowlog-log-slower-than 1000000}} { assert_equal {foobar} [lindex $e 5] } + test {SLOWLOG - Certain commands are omitted that contain sensitive information} { + r config set slowlog-log-slower-than 0 + r slowlog reset + r config set masterauth "" + r acl setuser slowlog-test-user + r config set slowlog-log-slower-than 0 + r config set slowlog-log-slower-than 10000 + set slowlog_resp [r slowlog get] + + # Make sure normal configs work, but the two sensitive + # commands are omitted + assert_equal 2 [llength $slowlog_resp] + assert_equal {slowlog reset} [lindex [lindex [r slowlog get] 1] 3] + assert_equal {config set slowlog-log-slower-than 0} [lindex [lindex [r slowlog get] 0] 3] + } + test {SLOWLOG - Rewritten commands are logged as their original command} { r config set slowlog-log-slower-than 0