Send signals as Icinga user in safe-reload and logrotate

[julianbrost]

In contrast to the regular kill binary, icinga2 internal signal drops permissions before sending the signal. This is important as the PID file can be written by the Icinga user, dropping the permissions prevents that user from using this to send signals to processes it is not supposed to signal.

SIGUSR1 wasn't among the list of signals supported by icinga2 internal signal, so it is added there.

Important

Mention in changelog:
The logrotate config is installed in /etc and may not be updated by a package update in case it was modified.

Tests

• Installed from this branch, new CMake substitutions worked correctly.
• systemctl reload icinga2 (and also calling /usr/lib/icinga2/safe-reload) still successfully reload Icinga 2.
• Expanded command from the logrotate config worked when manually executing it (triggered the "Received USR1 signal, reopening application logs." log message).
• icinga2 internal signal indeed drops privileges (more of a sanity check):

  # strace -e getuid,setuid,kill icinga2 internal signal --sig SIGHUP --pid 1
  --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=2562755, si_uid=0, si_status=0, si_utime=0, si_stime=0} ---
  getuid()                                = 0
  setuid(961)                             = 0
  kill(1, SIGHUP)                         = -1 EPERM (Operation not permitted)
  +++ exited with 255 +++

fixes #10527

[Al2Klimov]

Be aware that this is a config file which we've currently told the package manager not to override by %config(noreplace).

[julianbrost]

Unfortunate, but not much we can do about it except mentioning it in the changelog. I've added a corresponding hint to the PR description.

[Al2Klimov]

That's not true! We can at least consider %config, i.e w/o (noreplace).

[julianbrost]

If the file was modified, there was probably a reason for doing so. Just overwriting it would likely break something.

[Al2Klimov]

And just keeping it leaves a security hole(?) for all who don't read the changelog, but modified stuff. Also, %config makes a backup of your modifications.

[Al2Klimov]

@dgoetz:

• This is a valid reason for %config
• Needs proper upgrade log, though (breaking changes awareness)
• Also %config must be kept long enough, as users may skip a version

[julianbrost]

Feedback from the reporter:

I just had a look. I guess the interesting part now is what icinga2 internal signal --sig <signal> --pid <pid> does. Judging from the strace output it's a lot. Many threads are created, sockets are created, network interface information is retrieved via Netlink, file descriptors in /proc/self/fd are inspected, one thread sends SIGRT_1 to another thread.

Besides all that, all threads also seem to call setuid(), setgid() and setgroups(). icinga2 and icingacmd group ownership is retained. Only then the desired signal is sent to the target <pid>. So I guess it is okay. I couldn't see any dangerous file operations in the trace.

[Al2Klimov]

Discuss #10530 (comment) with Eric (in the next core status meeting).

RPM

performs its .rpmsave/.rpmnew dance only if both the package maintainer and the admin change a config file: https://serverfault.com/a/672640?utm_source=chatgpt.com

Given that the issue here is security-related, I consider a plain %config reasonable, especially as it will (backup and) replace the edited file (warning the user) only once.

Yes, only once. I mean, just look at the file's history: https://github.com/Icinga/icinga2/commits/master/etc/logrotate.d/icinga2.cmake

Spoiler:

We can also revert it in a year or similar.

Debian

always asks the user, offering a diff: https://raphaelhertzog.com/2010/09/21/debian-conffile-configuration-file-managed-by-dpkg/?utm_source=chatgpt.com

Just pressing Enter, for simply keep my changes, is tempting, though.

Both of them

provide post-scripts, so we could replace /bin/kill -USR1 with sed(1). A good workaround IMAO.

Eric also mentioned a(n Icinga 2) runtime check (that the file is sane).