Re: [LTP] [PATCH] logrotate: fix permission issue on Ubuntu

[Petr Vorel <pvorel@suse.cz>]

> When running this logrotate test on Ubuntu, the test will fail with:
>   logrotate_tests 2 TINFO: sleep 1 min to wait for rotating logs
>   logrotate_tests 2 TFAIL: [ -f /var/log/tst_largelogfile.1.gz ] failed unexpectedly
>   logrotate_tests 2 TFAIL: Failed to create a compressed file

> Look more closely to what this test is trying to do we will see there
> are two issues here in the tst_largelog.conf:

> 1. "include /etc/logrotate.d"
> This will rotate other log files defined here, since we are just testing
> the log rotation capibility I think there is no need to rotate log files
> other than the one for this test.

> 2. Permission issue on Ubuntu
Is it only Ubuntu or also Debian? Or, wouldn't be better to add permissions
regardless the system?

> Trying to rotate the target file on Ubuntu will cause the following error:
>   error: skipping "/var/log/tst_largelogfile" because parent directory has
>          insecure permissions (It's world writable or writable by group which
>          is not "root") Set "su" directive in config file to tell logrotate
>          which user/group should be used for rotation.

> Solution is to add an extra line with the user and group information of
> the /var/log directory. This change has been limited to Ubuntu to prevent
> causing regression on other OS.

> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
>  testcases/commands/logrotate/logrotate_tests.sh | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

> diff --git a/testcases/commands/logrotate/logrotate_tests.sh b/testcases/commands/logrotate/logrotate_tests.sh
> index 1f3e61294..4506ab5c6 100755
> --- a/testcases/commands/logrotate/logrotate_tests.sh
> +++ b/testcases/commands/logrotate/logrotate_tests.sh
> @@ -90,14 +90,17 @@ test1()

>  test2()
>  {
> +	permission=""
missing keyboard local (it.d be local permission, ="" not needed). But it's
better to put it into setup function to be executed only once (one can run kind
of stress tests with -i1000, permission detection will be needed only once.

> +	if `grep -q 'NAME="Ubuntu"' /etc/os-release 2>/dev/null`; then
`` are not needed. Also grepping ID would be slightly more readable:

> +		permission="su `stat -c "%U %G" /var/log`"
Do we really need to use su? It would have to be installed and configured.
IMHO it should be OK to to run stat without su.

> +	fi


>  	cat >tst_largelog.conf <<-EOF
>          # create new (empty) log files after rotating old ones
>          create
>          # compress the log files
>          compress
> -        # RPM packages drop log rotation information into this directory
> -        include /etc/logrotate.d
>          /var/log/tst_largelogfile {
> +            $permission
>              rotate 5
>              size=2k
>          }

I propose these changes over your patch. Could you please test it?

+++ testcases/commands/logrotate/logrotate_tests.sh
@@ -25,8 +25,18 @@ TST_NEEDS_CMDS="crontab file grep logrotate"
 TST_TESTFUNC=test
 TST_NEEDS_TMPDIR=1
 TST_CNT=2
+TST_SETUP=setup
 TST_CLEANUP=cleanup
 
+PERMISSION=
+
+setup()
+{
+	if grep 'ID=ubuntu' /etc/os-release 2>/dev/null; then
+		PERMISSION="$(stat -c "%U %G" /var/log)"
+	fi
+}
+
 cleanup()
 {
 	(crontab -l | grep -v tst_largelog) | crontab -
@@ -90,17 +100,13 @@ test1()
 
 test2()
 {
-	permission=""
-	if `grep -q 'NAME="Ubuntu"' /etc/os-release 2>/dev/null`; then
-		permission="su `stat -c "%U %G" /var/log`"
-	fi
 	cat >tst_largelog.conf <<-EOF
         # create new (empty) log files after rotating old ones
         create
         # compress the log files
         compress
         /var/log/tst_largelogfile {
-            $permission
+            $PERMISSION
             rotate 5
             size=2k
         }
---

Or, even without condition on PERMISSION.

With these changes, you may add.

Reviewed-by: Petr Vorel <pvorel@suse.cz>

Kind regards,
Petr

+++ testcases/commands/logrotate/logrotate_tests.sh
@@ -21,7 +21,7 @@
 # - Add messages to the logfile until it gets rotated when a re-dittermined size
 #   is reached.
 
-TST_NEEDS_CMDS="crontab file grep logrotate"
+TST_NEEDS_CMDS="crontab file grep logrotate stat"
 TST_TESTFUNC=test
 TST_NEEDS_TMPDIR=1
 TST_CNT=2
@@ -32,9 +32,7 @@ PERMISSION=
 
 setup()
 {
-	if grep 'ID=ubuntu' /etc/os-release 2>/dev/null; then
-		PERMISSION="$(stat -c "%U %G" /var/log)"
-	fi
+	PERMISSION="$(stat -c "%U %G" /var/log)"
 }
 
 cleanup()


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp