Comment # 3 on bug 1226083 from Wolfgang Frisch

OBS project: server:database/valkey
Upstream: https://github.com/valkey-io/valkey
Original upstream: https://github.com/redis/redis

We concluded our initial review. Due to the size and complexity, a full audit
of valkey was not feasible at this point. We focused our review on a few key
points:

- differences from the original redis
- sysctl settings
- systemd unit
- file system permissions
- default config


### Differences from the original Redis
valkey-7.2.4 is an exact copy of redis-7.2.4. The first differences arise with
(this) release 7.2.5. We went through the complete diff and found no obvious
security-related changes. Most changes are just debranding and API
compatibility fixes.

There were a few commits missing in valkey-7.2.5 (compared to redis-7.2.5):

- Missing fix for: https://github.com/redis/redis/issues/12864
- Missing unit tests for: https://github.com/redis/redis/pull/13004
- Missing bug fix:
https://github.com/redis/redis/commit/b3aaa0a1362d229ba1ecd44629655f76c77304ec
- Missing bug fix:
https://github.com/redis/redis/commit/bad33f8738b4be5f58c4439a0c78312e4afbe432
- Missing bug fix:
https://github.com/redis/redis/commit/1e8dc1da0de0add40d56fcbab9b64a3c5e61b6dd

As far as we can tell, none of this is security-related. OK from a security
PoV.


### sysctl settings
The two sysctl settings are identical to Redis.

It's a bit unfortunate that `vm.overcommit_memory=1` is set system-wide just by
installing this package. As it was already noted by mgerstner in another bug,
it would be an improvement
to just set this sysctl option in a systemd unit, when valkey is actually
started. If valkey ran as root, it would be simple with
```
ExecStartPre=sysctl vm.overcommit_memory=1
```
However, since the valkey service (correctly) drops its privileges, this would
have to be implemented in a separate systemd unit.

Not a problem from a security PoV though. OK from our side.


### systemd unit
Upstream's systemd unit lacks systemd hardenings and doesn't drop privileges by
default but fortunately the (open)SUSE package ships its own much improved
systemd. Good!

I tested a few additional hardenings with no apparent problems:

```
--- valkey@.service.orig        2024-06-11 11:13:59.169323953 +0200
+++ valkey@.service     2024-06-06 20:26:20.000000000 +0200
@@ -8,8 +8,6 @@
 User=valkey
 Group=valkey
 PrivateTmp=true
-# added automatically, for details please see
-# https://en.opensuse.org/openSUSE:Security_Features#Systemd_hardening_effort
 ProtectSystem=full
 ProtectHome=true
 PrivateDevices=true
@@ -20,7 +18,11 @@
 ProtectKernelLogs=true
 ProtectControlGroups=true
 RestrictRealtime=true
-# end of automatic additions 
+MemoryDenyWriteExecute=true
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6
+RestrictNamespaces=true
+RestrictSUIDSGID=true
+
 PIDFile=/run/valkey/%i.pid
 ExecStart=/usr/bin/val
```

```
--- valkey-sentinel@.service.orig       2024-06-11 11:15:04.960116608 +0200
+++ valkey-sentinel@.service    2024-06-11 11:15:35.650486709 +0200
@@ -8,8 +8,6 @@
 User=valkey
 Group=valkey
 PrivateTmp=true
-# added automatically, for details please see
-# https://en.opensuse.org/openSUSE:Security_Features#Systemd_hardening_effort
 ProtectSystem=full
 ProtectHome=true
 PrivateDevices=true
@@ -20,7 +18,11 @@
 ProtectKernelLogs=true
 ProtectControlGroups=true
 RestrictRealtime=true
-# end of automatic additions 
+MemoryDenyWriteExecute=true
+RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6
+RestrictNamespaces=true
+RestrictSUIDSGID=true
+
 ReadWritePaths=/etc/valkey
 PIDFile=/run/valkey/sentinel-%i.pid
 ExecStart=/usr/bin/valkey-sentinel /etc/valkey/sentinel-%i.conf
```

It would be great if you could include these changes, even though they're not
strictly necessary. We will whitelist the package either way.


### File system permissions
No problems here. OK.


### Default config
Valkey's default config is well-documented with sane defaults.
- Binds to localhost only by default. Good!
- In all cases where the user is asked to create files or directories, correct
  file system permissions are mentioned. Good!


## Summary
We might conduct an in-depth audit in the future, but for now we can safely
whitelist this package.