Skip to content

Commit

Permalink
Handle binary safe string for REQUIREPASS and MASTERAUTH directives (r…
Browse files Browse the repository at this point in the history 
…edis#8200)

* Handle binary safe string for REQUIREPASS and MASTERAUTH directives.
  • Loading branch information
@QuChen88
QuChen88 authored Dec 17, 2020
1 parent 0f3e0cb commit f48afb4
Show file tree
Hide file tree
Showing 4 changed files with 148 additions and 27 deletions.
102 changes: 86 additions & 16 deletions src/config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,15 @@ typedef struct stringConfigData {
be stored as a NULL value. */
} stringConfigData;

typedef struct sdsConfigData {
sds *config; /* Pointer to the server config this value is stored in. */
const char *default_value; /* Default value of the config on rewrite. */
int (*is_valid_fn)(sds val, char **err); /* Optional function to check validity of new value (generic doc above) */
int (*update_fn)(sds val, sds prev, char **err); /* Optional function to apply new value at runtime (generic doc above) */
int convert_empty_to_null; /* Boolean indicating if empty SDS strings should
be stored as a NULL value. */
} sdsConfigData;

typedef struct enumConfigData {
int *config; /* The pointer to the server config this value is stored in */
configEnum *enum_value; /* The underlying enum type this data represents */
Expand Down Expand Up @@ -212,6 +221,7 @@ typedef struct numericConfigData {
typedef union typeData {
boolConfigData yesno;
stringConfigData string;
sdsConfigData sds;
enumConfigData enumd;
numericConfigData numeric;
} typeData;
Expand Down Expand Up @@ -512,7 +522,7 @@ void loadServerConfigFromString(char *config) {
}
server.repl_state = REPL_STATE_CONNECT;
} else if (!strcasecmp(argv[0],"requirepass") && argc == 2) {
if (strlen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) {
if (sdslen(argv[1]) > CONFIG_AUTHPASS_MAX_LEN) {
err = "Password is longer than CONFIG_AUTHPASS_MAX_LEN";
goto loaderr;
}
Expand All @@ -524,10 +534,10 @@ void loadServerConfigFromString(char *config) {
sdsfree(server.requirepass);
server.requirepass = NULL;
if (sdslen(argv[1])) {
sds aclop = sdscatprintf(sdsempty(),">%s",argv[1]);
sds aclop = sdscatlen(sdsnew(">"), argv[1], sdslen(argv[1]));
ACLSetUser(DefaultUser,aclop,sdslen(aclop));
sdsfree(aclop);
server.requirepass = sdsnew(argv[1]);
server.requirepass = sdsdup(argv[1]);
} else {
ACLSetUser(DefaultUser,"nopass",-1);
}
Expand Down Expand Up @@ -751,10 +761,10 @@ void configSetCommand(client *c) {
sdsfree(server.requirepass);
server.requirepass = NULL;
if (sdslen(o->ptr)) {
sds aclop = sdscatprintf(sdsempty(),">%s",(char*)o->ptr);
sds aclop = sdscatlen(sdsnew(">"), o->ptr, sdslen(o->ptr));
ACLSetUser(DefaultUser,aclop,sdslen(aclop));
sdsfree(aclop);
server.requirepass = sdsnew(o->ptr);
server.requirepass = sdsdup(o->ptr);
} else {
ACLSetUser(DefaultUser,"nopass",-1);
}
Expand Down Expand Up @@ -1330,6 +1340,28 @@ void rewriteConfigStringOption(struct rewriteConfigState *state, const char *opt
rewriteConfigRewriteLine(state,option,line,force);
}

/* Rewrite a SDS string option. */
void rewriteConfigSdsOption(struct rewriteConfigState *state, const char *option, sds value, const sds defvalue) {
int force = 1;
sds line;

/* If there is no value set, we don't want the SDS option
* to be present in the configuration at all. */
if (value == NULL) {
rewriteConfigMarkAsProcessed(state, option);
return;
}

/* Set force to zero if the value is set to its default. */
if (defvalue && sdscmp(value, defvalue) == 0) force = 0;

line = sdsnew(option);
line = sdscatlen(line, " ", 1);
line = sdscatrepr(line, value, sdslen(value));

rewriteConfigRewriteLine(state, option, line, force);
}

/* Rewrite a numerical (long long range) option. */
void rewriteConfigNumericalOption(struct rewriteConfigState *state, const char *option, long long value, long long defvalue) {
int force = value != defvalue;
Expand Down Expand Up @@ -1802,22 +1834,14 @@ static void boolConfigRewrite(typeData data, const char *name, struct rewriteCon

/* String Configs */
static void stringConfigInit(typeData data) {
if (data.string.convert_empty_to_null) {
*data.string.config = data.string.default_value ? zstrdup(data.string.default_value) : NULL;
} else {
*data.string.config = zstrdup(data.string.default_value);
}
*data.string.config = (data.string.convert_empty_to_null && !data.string.default_value) ? NULL : zstrdup(data.string.default_value);
}

static int stringConfigSet(typeData data, sds value, int update, char **err) {
if (data.string.is_valid_fn && !data.string.is_valid_fn(value, err))
return 0;
char *prev = *data.string.config;
if (data.string.convert_empty_to_null) {
*data.string.config = value[0] ? zstrdup(value) : NULL;
} else {
*data.string.config = zstrdup(value);
}
*data.string.config = (data.string.convert_empty_to_null && !value[0]) ? NULL : zstrdup(value);
if (update && data.string.update_fn && !data.string.update_fn(*data.string.config, prev, err)) {
zfree(*data.string.config);
*data.string.config = prev;
Expand All @@ -1835,6 +1859,38 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC
rewriteConfigStringOption(state, name,*(data.string.config), data.string.default_value);
}

/* SDS Configs */
static void sdsConfigInit(typeData data) {
*data.sds.config = (data.sds.convert_empty_to_null && !data.sds.default_value) ? NULL: sdsnew(data.sds.default_value);
}

static int sdsConfigSet(typeData data, sds value, int update, char **err) {
if (data.sds.is_valid_fn && !data.sds.is_valid_fn(value, err))
return 0;
sds prev = *data.sds.config;
*data.sds.config = (data.sds.convert_empty_to_null && (sdslen(value) == 0)) ? NULL : sdsdup(value);
if (update && data.sds.update_fn && !data.sds.update_fn(*data.sds.config, prev, err)) {
sdsfree(*data.sds.config);
*data.sds.config = prev;
return 0;
}
sdsfree(prev);
return 1;
}

static void sdsConfigGet(client *c, typeData data) {
if (*data.sds.config) {
addReplyBulkSds(c, sdsdup(*data.sds.config));
} else {
addReplyBulkCString(c, "");
}
}

static void sdsConfigRewrite(typeData data, const char *name, struct rewriteConfigState *state) {
rewriteConfigSdsOption(state, name, *(data.sds.config), data.sds.default_value ? sdsnew(data.sds.default_value) : NULL);
}


#define ALLOW_EMPTY_STRING 0
#define EMPTY_STRING_IS_NULL 1

Expand All @@ -1850,6 +1906,18 @@ static void stringConfigRewrite(typeData data, const char *name, struct rewriteC
} \
}

#define createSDSConfig(name, alias, modifiable, empty_to_null, config_addr, default, is_valid, update) { \
embedCommonConfig(name, alias, modifiable) \
embedConfigInterface(sdsConfigInit, sdsConfigSet, sdsConfigGet, sdsConfigRewrite) \
.data.sds = { \
.config = &(config_addr), \
.default_value = (default), \
.is_valid_fn = (is_valid), \
.update_fn = (update), \
.convert_empty_to_null = (empty_to_null), \
} \
}

/* Enum configs */
static void enumConfigInit(typeData data) {
*data.enumd.config = data.enumd.default_value;
Expand Down Expand Up @@ -2349,7 +2417,6 @@ standardConfig configs[] = {
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("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, 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),
Expand All @@ -2359,6 +2426,9 @@ standardConfig configs[] = {
createStringConfig("aof_rewrite_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.aof_rewrite_cpulist, NULL, NULL, NULL),
createStringConfig("bgsave_cpulist", NULL, IMMUTABLE_CONFIG, EMPTY_STRING_IS_NULL, server.bgsave_cpulist, NULL, NULL, NULL),

/* SDS Configs */
createSDSConfig("masterauth", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.masterauth, NULL, NULL, NULL),

/* Enum Configs */
createEnumConfig("supervised", NULL, IMMUTABLE_CONFIG, supervised_mode_enum, server.supervised_mode, SUPERVISED_NONE, NULL, NULL),
createEnumConfig("syslog-facility", NULL, IMMUTABLE_CONFIG, syslog_facility_enum, server.syslog_facility, LOG_LOCAL0, NULL, NULL),
Expand Down
30 changes: 20 additions & 10 deletions src/replication.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1835,6 +1835,7 @@ void readSyncBulkPayload(connection *conn) {
*/
#define SYNC_CMD_READ (1<<0)
#define SYNC_CMD_WRITE (1<<1)
#define SYNC_CMD_WRITE_SDS (1<<2)
#define SYNC_CMD_FULL (SYNC_CMD_READ|SYNC_CMD_WRITE)
char *sendSynchronousCommand(int flags, connection *conn, ...) {

Expand All @@ -1852,8 +1853,13 @@ char *sendSynchronousCommand(int flags, connection *conn, ...) {
while(1) {
arg = va_arg(ap, char*);
if (arg == NULL) break;

cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg);
if (flags & SYNC_CMD_WRITE_SDS) {
cmdargs = sdscatprintf(cmdargs,"$%zu\r\n", sdslen((sds)arg));
cmdargs = sdscatsds(cmdargs, (sds)arg);
cmdargs = sdscat(cmdargs, "\r\n");
} else {
cmdargs = sdscatprintf(cmdargs,"$%zu\r\n%s\r\n",strlen(arg),arg);
}
argslen++;
}

Expand Down Expand Up @@ -2166,14 +2172,18 @@ void syncWithMaster(connection *conn) {

/* AUTH with the master if required. */
if (server.repl_state == REPL_STATE_SEND_AUTH) {
if (server.masteruser && server.masterauth) {
err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",
server.masteruser,server.masterauth,NULL);
if (err) goto write_error;
server.repl_state = REPL_STATE_RECEIVE_AUTH;
return;
} else if (server.masterauth) {
err = sendSynchronousCommand(SYNC_CMD_WRITE,conn,"AUTH",server.masterauth,NULL);
if (server.masterauth) {
sds auth = sdsnew("AUTH");
if (server.masteruser) {
sds masteruser = sdsnew(server.masteruser);
err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth,
masteruser, server.masterauth, NULL);
sdsfree(masteruser);
} else {
err = sendSynchronousCommand(SYNC_CMD_WRITE | SYNC_CMD_WRITE_SDS, conn, auth,
server.masterauth, NULL);
}
sdsfree(auth);
if (err) goto write_error;
server.repl_state = REPL_STATE_RECEIVE_AUTH;
return;
Expand Down
2 changes: 1 addition & 1 deletion src/server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1386,7 +1386,7 @@ struct redisServer {
int repl_diskless_sync_delay; /* Delay to start a diskless repl BGSAVE. */
/* Replication (slave) */
char *masteruser; /* AUTH with this user and masterauth with master */
char *masterauth; /* AUTH with this password with master */
sds masterauth; /* AUTH with this password with master */
char *masterhost; /* Hostname of master */
int masterport; /* Port of master */
int repl_timeout; /* Timeout after N seconds of master idle */
Expand Down
41 changes: 41 additions & 0 deletions tests/unit/auth.tcl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,44 @@ start_server {tags {"auth"} overrides {requirepass foobar}} {
r incr foo
} {101}
}

start_server {tags {"auth_binary_password"}} {
test {AUTH fails when binary password is wrong} {
r config set requirepass "abc\x00def"
catch {r auth abc} err
set _ $err
} {WRONGPASS*}

test {AUTH succeeds when binary password is correct} {
r config set requirepass "abc\x00def"
r auth "abc\x00def"
} {OK}

start_server {tags {"masterauth"}} {
set master [srv -1 client]
set master_host [srv -1 host]
set master_port [srv -1 port]
set slave [srv 0 client]

test {MASTERAUTH test with binary password} {
$master config set requirepass "abc\x00def"

# Configure the replica with masterauth
set loglines [count_log_lines 0]
$slave slaveof $master_host $master_port
$slave config set masterauth "abc"

# Verify replica is not able to sync with master
wait_for_log_messages 0 {"*Unable to AUTH to MASTER*"} $loglines 1000 10
assert_equal {down} [s 0 master_link_status]

# Test replica with the correct masterauth
$slave config set masterauth "abc\x00def"
wait_for_condition 50 100 {
[s 0 master_link_status] eq {up}
} else {
fail "Can't turn the instance into a replica"
}
}
}
}

0 comments on commit f48afb4

Please sign in to comment.