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

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

[Po-Hsu Lin]

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
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=""
+	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
-        # RPM packages drop log rotation information into this directory
-        include /etc/logrotate.d
         /var/log/tst_largelogfile {
+            $permission
             rotate 5
             size=2k
         }
-- 
2.25.1


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

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

[Petr Vorel]

> 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

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

[Po-Hsu Lin]

On Thu, Jun 20, 2024 at 12:31 AM Petr Vorel <pvorel@suse.cz> wrote:

> > 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?
>
> Hello,
thanks for the detailed feedback,
yes I think it's better to add this regardless the system, I will explain
later.


> > 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.
>
The su here is not for running the stat command. It's for composing the "su"
directive required for the logrotate.

I think we can drop the os-release detection, by looking into this test more
closely I noticed that there is a similar commit 3d7bc899bd
("commands/logrotate_tests.sh: Fix missing syslog group on some systems")
for test1.

Since it's been there for a while, I think this works for different OS.

I will make an identical change, but utilize setup() as you suggested to
prevent duplicating codes.
Thank you
Po-Hsu


> > +     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