Describe the bug

Running Sentinel, as soon as an automatic config rewrite is performed, the file permissions of sentinel.conf are set to 0644 and redis:redis (user/group under which sentinel is running), irrespective of the original file permissions and ignoring UMask setting of systemd service unit. This exposes sensitive data by setting the file world-readable, e.g. requirepass/auth-pass.

To reproduce

  • Creating /etc/redis/sentinel.conf with intended config parameters
  • Setting ownership to root:redis (i.e. user under which sentinel is invoked)
  • Setting file permissions to 0660, to allow sentinel to write the file
  • Setting directory ownership and permissions of /etc/redis/ to root:redis and 0770
  • Start redis-sentinel

prepared environment:

drwxrwx---  2 root redis 4.0K Jan 20 12:23 .
drwxr-xr-x 69 root root  4.0K Jan 19 16:27 ..
-rw-r-----  1 root redis 2.2K Jan 19 18:55 redis.conf
-rw-rw----  1 root redis  599 Jan 20 12:23 sentinel.conf

after starting redis-sentinel, the config is rewritten (# Generated by CONFIG REWRITE is added and some parameters) AND permissions are altered:

drwxrwx---  2 root  redis 4.0K Jan 20 12:23 .
drwxr-xr-x 69 root  root  4.0K Jan 19 16:27 ..
-rw-r-----  1 root  redis 2.2K Jan 19 18:55 redis.conf
-rw-r--r--  1 redis redis  883 Jan 20 12:23 sentinel.conf

Expected behavior

The config rewrite is not supposed to alter file ownership nor permissions, especially not to insecure world-readable, putting sensitive data at risk.

Additional information

Running on Debian 10, with recent redis-sentinel 6.0.9-2~bpo10+1 from buster-backports using Debian's systemd unit, i.e. User=redis, Group=redis, UMask=0007

Comment From: yossigo

@tokred Thanks for reporting this! This seems to have been introduced due to a new (safer) way of rewriting configuration files. In the past the file was simply overwritten (same inode), but now Redis creates a new temporary files and then renames it over the old file.

It will be impossible to guarantee that file ownership is retained, but this should definitely be fixed to at least honor the umask.

Comment From: tokred

@yossigo, thank you for confirmation and looking forward to the fix.

Frankly, I find sentinel's approach of the config file getting rewritten less than ideal in the first place. It a) requires write permissions for the process' uid, b) breaks configuration deployments like Ansible (no idempotency of repeated runs as the deployed config will be altered locally), and c) kind of mixes static configuration with dynamic state under /etc. No personal criticism, just constructive feedback. :)

Regards

Comment From: yossigo

@tokred I generally agree about the need to better separate static configuration and runtime state, but that's water under the bridge now. You may still be able to achieve idempotency if you create a separate pristine config file and a "working config" which is copied elsewhere.