[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()
>
>