From 3c0fd2520128a69a7d5826e18a2b834fb2bcef3d Mon Sep 17 00:00:00 2001 From: Binbin Date: Wed, 13 Dec 2023 21:28:13 +0800 Subject: [PATCH] Redact ACL username information and mark *-key-file-pass configs as sensitive (#12860) In #11489, we consider acl username to be sensitive information, and consider the ACL GETUSER a sensitive command and remove it from redis-cli historyfile. This PR redact username information in ACL GETUSER and ACL DELUSER from SLOWLOG, and also remove ACL DELUSER from redis-cli historyfile. This PR also mark tls-key-file-pass and tls-client-key-file-pass as sensitive config, will redact it from SLOWLOG and also remove them from redis-cli historyfile. --- src/acl.c | 7 +++++++ src/config.c | 4 ++-- src/redis-cli.c | 7 +++++-- tests/unit/slowlog.tcl | 25 +++++++++++++++++++------ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/src/acl.c b/src/acl.c index 1d65faf69..58a9a3972 100644 --- a/src/acl.c +++ b/src/acl.c @@ -2837,6 +2837,10 @@ void aclCommand(client *c) { } return; } else if (!strcasecmp(sub,"deluser") && c->argc >= 3) { + /* Initially redact all the arguments to not leak any information + * about the users. */ + for (int j = 2; j < c->argc; j++) redactClientCommandArgument(c, j); + int deleted = 0; for (int j = 2; j < c->argc; j++) { sds username = c->argv[j]->ptr; @@ -2859,6 +2863,9 @@ void aclCommand(client *c) { } addReplyLongLong(c,deleted); } else if (!strcasecmp(sub,"getuser") && c->argc == 3) { + /* Redact the username to not leak any information about the user. */ + redactClientCommandArgument(c, 2); + user *u = ACLGetUserByName(c->argv[2]->ptr,sdslen(c->argv[2]->ptr)); if (u == NULL) { addReplyNull(c); diff --git a/src/config.c b/src/config.c index 3231b2442..b152a8fa5 100644 --- a/src/config.c +++ b/src/config.c @@ -3244,10 +3244,10 @@ standardConfig static_configs[] = { createBoolConfig("tls-session-caching", NULL, MODIFIABLE_CONFIG, server.tls_ctx_config.session_caching, 1, NULL, applyTlsCfg), createStringConfig("tls-cert-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.cert_file, NULL, NULL, applyTlsCfg), createStringConfig("tls-key-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.key_file, NULL, NULL, applyTlsCfg), - createStringConfig("tls-key-file-pass", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.key_file_pass, NULL, NULL, applyTlsCfg), + createStringConfig("tls-key-file-pass", NULL, MODIFIABLE_CONFIG | SENSITIVE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.key_file_pass, NULL, NULL, applyTlsCfg), createStringConfig("tls-client-cert-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.client_cert_file, NULL, NULL, applyTlsCfg), createStringConfig("tls-client-key-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.client_key_file, NULL, NULL, applyTlsCfg), - createStringConfig("tls-client-key-file-pass", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.client_key_file_pass, NULL, NULL, applyTlsCfg), + createStringConfig("tls-client-key-file-pass", NULL, MODIFIABLE_CONFIG | SENSITIVE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.client_key_file_pass, NULL, NULL, applyTlsCfg), createStringConfig("tls-dh-params-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.dh_params_file, NULL, NULL, applyTlsCfg), createStringConfig("tls-ca-cert-file", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.ca_cert_file, NULL, NULL, applyTlsCfg), createStringConfig("tls-ca-cert-dir", NULL, VOLATILE_CONFIG | MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, server.tls_ctx_config.ca_cert_dir, NULL, NULL, applyTlsCfg), diff --git a/src/redis-cli.c b/src/redis-cli.c index 96d667c86..f18961eaf 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -3266,8 +3266,8 @@ void cliLoadPreferences(void) { /* Some commands can include sensitive information and shouldn't be put in the * history file. Currently these commands are include: * - AUTH - * - ACL SETUSER, ACL GETUSER - * - CONFIG SET masterauth/masteruser/requirepass + * - ACL DELUSER, ACL SETUSER, ACL GETUSER + * - CONFIG SET masterauth/masteruser/tls-key-file-pass/tls-client-key-file-pass/requirepass * - HELLO with [AUTH username password] * - MIGRATE with [AUTH password] or [AUTH2 username password] * - SENTINEL CONFIG SET sentinel-pass password, SENTINEL CONFIG SET sentinel-user username @@ -3277,6 +3277,7 @@ static int isSensitiveCommand(int argc, char **argv) { return 1; } else if (argc > 1 && !strcasecmp(argv[0],"acl") && ( + !strcasecmp(argv[1],"deluser") || !strcasecmp(argv[1],"setuser") || !strcasecmp(argv[1],"getuser"))) { @@ -3287,6 +3288,8 @@ static int isSensitiveCommand(int argc, char **argv) { for (int j = 2; j < argc; j = j+2) { if (!strcasecmp(argv[j],"masterauth") || !strcasecmp(argv[j],"masteruser") || + !strcasecmp(argv[j],"tls-key-file-pass") || + !strcasecmp(argv[j],"tls-client-key-file-pass") || !strcasecmp(argv[j],"requirepass")) { return 1; } diff --git a/tests/unit/slowlog.tcl b/tests/unit/slowlog.tcl index 3c547b924..a5e8862d7 100644 --- a/tests/unit/slowlog.tcl +++ b/tests/unit/slowlog.tcl @@ -24,7 +24,7 @@ start_server {tags {"slowlog"} overrides {slowlog-log-slower-than 1000000}} { } {10} test {SLOWLOG - GET optional argument to limit output len works} { - + assert_equal 5 [llength [r slowlog get 5]] assert_equal 10 [llength [r slowlog get -1]] assert_equal 10 [llength [r slowlog get 20]] @@ -50,22 +50,35 @@ start_server {tags {"slowlog"} overrides {slowlog-log-slower-than 1000000}} { } {} {needs:debug} test {SLOWLOG - Certain commands are omitted that contain sensitive information} { + r config set slowlog-max-len 100 r config set slowlog-log-slower-than 0 r slowlog reset catch {r acl setuser "slowlog test user" +get +set} _ + r config set masteruser "" r config set masterauth "" + r config set requirepass "" + r config set tls-key-file-pass "" + r config set tls-client-key-file-pass "" r acl setuser slowlog-test-user +get +set + r acl getuser slowlog-test-user + r acl deluser slowlog-test-user non-existing-user r config set slowlog-log-slower-than 0 r config set slowlog-log-slower-than -1 - set slowlog_resp [r slowlog get] + set slowlog_resp [r slowlog get -1] # Make sure normal configs work, but the two sensitive # commands are omitted or redacted - assert_equal 5 [llength $slowlog_resp] - assert_equal {slowlog reset} [lindex [lindex $slowlog_resp 4] 3] + assert_equal 11 [llength $slowlog_resp] + assert_equal {slowlog reset} [lindex [lindex $slowlog_resp 10] 3] + assert_equal {acl setuser (redacted) (redacted) (redacted)} [lindex [lindex $slowlog_resp 9] 3] + assert_equal {config set masteruser (redacted)} [lindex [lindex $slowlog_resp 8] 3] + assert_equal {config set masterauth (redacted)} [lindex [lindex $slowlog_resp 7] 3] + assert_equal {config set requirepass (redacted)} [lindex [lindex $slowlog_resp 6] 3] + assert_equal {config set tls-key-file-pass (redacted)} [lindex [lindex $slowlog_resp 5] 3] + assert_equal {config set tls-client-key-file-pass (redacted)} [lindex [lindex $slowlog_resp 4] 3] assert_equal {acl setuser (redacted) (redacted) (redacted)} [lindex [lindex $slowlog_resp 3] 3] - assert_equal {config set masterauth (redacted)} [lindex [lindex $slowlog_resp 2] 3] - assert_equal {acl setuser (redacted) (redacted) (redacted)} [lindex [lindex $slowlog_resp 1] 3] + assert_equal {acl getuser (redacted)} [lindex [lindex $slowlog_resp 2] 3] + assert_equal {acl deluser (redacted) (redacted)} [lindex [lindex $slowlog_resp 1] 3] assert_equal {config set slowlog-log-slower-than 0} [lindex [lindex $slowlog_resp 0] 3] } {} {needs:repl}