Merge ~ahasenack/ubuntu/+source/frr:kinetic-frr-logging-2 into ubuntu/+source/frr:ubuntu/devel

[Andreas Hasenack]

This MP fixes the logging issue caused by the fact that our rsyslog runs unprivileged, whereas debian's runs as root.

PPA: https://launchpad.net/~ahasenack/+archive/ubuntu/frrr-syslog-approach-2

There are a few ways to address this, and all may clash with local configuration changes done by the administrator. Even debian's way. What I did here was to keep the logs in the same place as originally intended.

Some background. I can think of a few reasons why daemons would log directly to /var/log/<file>.log, or use a subdirectory like /var/log/<daemon>/{file1,file2,...}.log:
- direct logging without syslog: if the daemon can't create files in /var/log, then it's common to create /var/log/<daemon>/ with the correct non-root privileges so the daemon can write to that subdirectory
- multiple file: instead of polluting /var/log directly with scattered files, in such cases it's also common to have a subdirectory where multiple files can be written to. This applies to both cases of using rsyslog or direct logging

frr's default setup from debian is neither: it uses rsyslog, but writes one single log file to /var/log/frr/frr.log. So there shouldn't really be a need for a subdirectory. Maybe some time ago each daemon was logging to its own file in there.

Therefore, my solution is to fix the permissions of /var/log/frr and /var/log/frr/*, including in logrotate, to keep the same behavior. This adds delta to debian.

In frr.postinst, the debian maintainer is resorting to a pattern that I haven't seen before, which is to check if the log files have posix ACLs. Maybe this happened before in the package history. I kept the same behavior when adjusting /var/log/frr to our needs. One could be tempted to use a function for that procedure, since it's used twice, but I think this way the delta is a bit clearer and easier to apply to future merges.

Clashes with local administrator changes
Keeping in mind that the default configuration ships with syslog logging (/etc/frr/frr.conf), and already chowns (in the debian case) those logging files to frr:.
- If an admin changes syslog to file, and writes to the same /var/log/frr/ place, our postinst script will change that to syslog:adm, probably preventing the frr daemon from logging. Should I detect that in postinst (syslog not being used), and then refrain from changing permissions? Sounds doable. Same check should be done in the logrotate script. The check would be something like "grep ^[[:space:]]*syslog /etc/frr/frr.conf". I just fear trying to be too smart here and making things worse in some scenario.
- If an admin changes the above in the debian case, then it would still work, as in both cases (syslog and file) the log ownership is frr:.

That's it. Let me know what you think about the extra "if there is no ^syslog in frr.conf, don't do any chowning in postinst".

[Robie Basak (racb)]

I like this approach. It seems to me that if we default to using rsyslog to write logs, then the ownership of the file should be that of rsyslog, so this also seems correct. Using the same ownership as what the daemon _would_ use were it configured to take over logging directly might make it easier to make that change, but it seems sloppy to me. If we have to maintain this delta indefinitely because Ubuntu chooses to stop rsyslog running as root by default, then so be it.

I also think this makes sense as a general pattern of what to do in other packages that use rsyslog.

I'm not keen on the "make permissions be correct on first install" code in the postinst being conflated with the "change permissions because we're upgrading from a previously different situation" code. In particular, I prefer the latter to always be guarded and apply only on a version comparison. But I see you've used the same pattern as what was already there, and I agree that this is preferable.

review: Approve

[Evren Yurtesen (eyurtese-g)]

Hi Andreas,

Would it not be better to not force specific ownerships in `frr.logrotate`? I mean this line:

create 640 syslog adm

If this line is missing, it will default to correct username which is syslog. (as rsyslogd is running as syslog)

The folder /var/log/frr could have 0770 permissions and ownership should be `frr:adm`. The default /etc/rsyslog.conf configuration should cause files to be created with `syslog:adm` ownership.

I believe this way, perhaps a `delta` can be avoided. Debian already has accepted similar changes for `tomcat9` package as the directory is owned by `tomcat:adm` with 2770 permissions.

Thanks,
Evren

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: ahasenack, racb
Uploaders: ahasenack, racb
MP auto-approved

review: Approve

[Andreas Hasenack (ahasenack)]

Hi Evren,

thanks for your suggestion. If I understand it correctly, you are suggesting to ask debian to change /var/log/frr from frr:frr 0750 to frr:adm 0770? Or 2770?

I think for debian the "create" logrotate config would still be needed, or else the new frr.log file would be created with rsyslog's defaults there which are root:adm (from /etc/rsyslog.conf), even with the sgid bit set in /var/log/frr.

[Andreas Hasenack (ahasenack)]

Debian could then specify the ownership in the omfile output module in /etc/rsyslog.d/45-frr.conf, but that would trigger an error in Ubuntu when rsyslog tries to chown the file and can't.

[Andreas Hasenack (ahasenack)]

Perhaps, untested:
a) change /var/log/frr (directory) to frr:adm 0770 (or 2770, to cope with the case where frr's logging is changed from syslog to file)

b) change logrotate to "create frr:adm 0660"

c) postinst chowns frr:adm, chmod 0660 the log files.

This would allow both ubuntu and debian to keep logging in /var/log/frr via rsyslog, at the cost of changing ownership of log files once again in the history of the package (from frr:frr to frr:adm in debian, and no log to frr:adm in ubuntu)

Is the above what you more or less meant?

diff --git a/debian/changelog b/debian/changelog
index 27f82a0..2a36559 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+frr (8.1-1ubuntu2) kinetic; urgency=medium
+
+  * Fix logging with Ubuntu's unprivileged rsyslog (LP: #1958162):
+    - d/frr.postinst: change log files ownership
+    - d/frr.logrotate: change rotated log file ownership
+
+ -- Andreas Hasenack <[e-mail]>  Thu, 09 Jun 2022 12:35:58 -0300
+
 frr (8.1-1ubuntu1) jammy; urgency=medium
 
   * SECURITY UPDATE: overflow via input packet length
diff --git a/debian/frr.logrotate b/debian/frr.logrotate
index a56a908..f49da46 100644
--- a/debian/frr.logrotate
+++ b/debian/frr.logrotate
@@ -4,7 +4,7 @@
         missingok
         compress
         rotate 14
-        create 640 frr frrvty
+        create 640 syslog adm
 
         postrotate
             pid=$(lsof -t -a -c /syslog/ /var/log/frr/* 2>/dev/null)
diff --git a/debian/frr.postinst b/debian/frr.postinst
index 505ff8e..382edcb 100644
--- a/debian/frr.postinst
+++ b/debian/frr.postinst
@@ -32,7 +32,6 @@ quaggagid=`id -g quagga 2>/dev/null || echo 0`
 
 find \
 	/etc/frr \
-	/var/log/frr \
 		\( -uid 0 -o -uid $quaggauid \) -a \
 		\( -gid 0 -o -gid $quaggauid \) | \
 	while read filename; do
@@ -48,6 +47,30 @@ find \
 	fi
 done
 
+# fix logging for Ubuntu, which does not run rsyslog as root (LP: #1958162),
+# and upgrades from quagga (what the block above used to do also for /var/log)
+
+# frr user was created above, this really shouldn't fail
+frruid=`getent passwd frr | cut -d : -f 3`
+frrgid=`getent group frr | cut -d : -f 3`
+
+find \
+    /var/log/frr \
+        \( -uid 0 -o -uid $quaggauid -o -uid $frruid \) -a \
+        \( -gid 0 -o -gid $quaggauid -o -gid $frrgid \) | \
+    while read filename; do
+
+    # don't chown anything that has ACLs (but don't fail if we don't
+    # have getfacl)
+    if { getfacl -c "$filename" 2>/dev/null || true; } \
+        | egrep -q -v '^((user|group|other)::|$)'; then
+        :
+    else
+        chown syslog:adm "$filename"
+        chmod o-rwx "$filename"
+    fi
+done
+
 # fix misconfigured vtysh.conf & frr.conf ownership caused by config save
 # mishandling in earlier FRR (and Quagga) versions
 find /etc/frr -maxdepth 1 \( -name vtysh.conf -o -name frr.conf \) \