[PATCH 0/2] test/py: fix test_efi_secboot/conftest.py

[PATCH 0/2] test/py: fix test_efi_secboot/conftest.py

[Heinrich Schuchardt]

The Python tests for secure booting have some issues that stop them from
being executed by non-root.

With this patch series a non-root user can execute the tests if he is
member of the sudoers group.

The test preparations relies on mounting a disk image. Non-root users can
only mount fuse file-systems and fusefat is in bad shape. So currently
changing the tests to run as non-sudo user seems infeasible.

Heinrich Schuchardt (2):
  test/py: efi_secboot should not assume sbin is in the path
  test/py: fix test_efi_secboot/conftest.py

 test/py/tests/test_efi_secboot/conftest.py | 34 +++++-----------------
 1 file changed, 8 insertions(+), 26 deletions(-)

--
2.26.1

[PATCH 1/2] test/py: efi_secboot should not assume sbin is in the path

[Heinrich Schuchardt]

For non-root users /sbin is typically not in the path. So use absolute
paths to mkfs.vfat and sgdisk.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/py/tests/test_efi_secboot/conftest.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index e542fef6e8..40cdf15bf2 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -48,12 +48,12 @@ def efi_boot_env(request, u_boot_config):
         # create a disk/partition
         check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
                             % (image_path, image_size), shell=True)
-        check_call('sgdisk %s -n 1:0:+%dMiB'
+        check_call('/sbin/sgdisk %s -n 1:0:+%dMiB'
                             % (image_path, part_size), shell=True)
         # create a file system
         check_call('dd if=/dev/zero of=%s.tmp bs=1MiB count=%d'
                             % (image_path, part_size), shell=True)
-        check_call('mkfs -t %s %s.tmp' % (fs_type, image_path), shell=True)
+        check_call('/sbin/mkfs.%s %s.tmp' % (fs_type, image_path), shell=True)
         check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
                             % (image_path, image_path, 1), shell=True)
         check_call('rm %s.tmp' % image_path, shell=True)
--
2.26.1

[PATCH 2/2] test/py: fix test_efi_secboot/conftest.py

[Heinrich Schuchardt]

If udisksctl is present
test/py/tests/test_efi_secboot/conftest.py
fails because the disk image is never mounted.

Normal users can only mount fuse file systems. Unfortunately fusefat is
still in an experimental state and seems not to work here correctly.

So as we have to be root or use the sudo command anyway delete all coding
referring to udisksctl.

--

We should not use mount point /mnt as this directory or one of its
sub-directories might already be in use as active mount points. Instead
create a new directory in the build root as mount point.

--

Remove debug print statements that have been commented out. print without
parentheses is anyway invalid in Python 3. And pytest anyway filters out
the output if there is no exception reported.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 test/py/tests/test_efi_secboot/conftest.py | 30 +++++-----------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 40cdf15bf2..93d308cf0d 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -43,7 +43,8 @@ def efi_boot_env(request, u_boot_config):
         HELLO_PATH = u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi'

     try:
-        non_root = tool_is_in_path('udisksctl')
+        mnt_point = u_boot_config.persistent_data_dir + '/mnt_efisecure'
+        check_call('mkdir -p {}'.format(mnt_point), shell=True)

         # create a disk/partition
         check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
@@ -57,25 +58,11 @@ def efi_boot_env(request, u_boot_config):
         check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
                             % (image_path, image_path, 1), shell=True)
         check_call('rm %s.tmp' % image_path, shell=True)
-        if non_root:
-            out_data = check_output('udisksctl loop-setup -f %s -o %d'
-                                % (image_path, 1048576), shell=True).decode()
-            m = re.search('(?<= as )(.*)\.', out_data)
-            loop_dev = m.group(1)
-            # print 'loop device is: %s' % loop_dev
-            out_data = check_output('udisksctl info -b %s'
-                                % loop_dev, shell=True).decode()
-            m = re.search('MountPoints:[ \t]+(.*)', out_data)
-            mnt_point = m.group(1)
-        else:
-            loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
+        loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
                                 % (part_size, image_path), shell=True).decode()
-            mnt_point = '/mnt'
-            check_output('sudo mount -t %s -o umask=000 %s %s'
+        check_output('sudo mount -t %s -o umask=000 %s %s'
                                 % (fs_type, loop_dev, mnt_point), shell=True)

-        # print 'mount point is: %s' % mnt_point
-
         # suffix
         # *.key: RSA private key in PEM
         # *.crt: X509 certificate (self-signed) in PEM
@@ -134,13 +121,8 @@ def efi_boot_env(request, u_boot_config):
                             % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
                             shell=True)

-        if non_root:
-            check_call('udisksctl unmount -b %s' % loop_dev, shell=True)
-            # not needed
-            # check_call('udisksctl loop-delete -b %s' % loop_dev, shell=True)
-        else:
-            check_call('sudo umount %s' % loop_dev, shell=True)
-            check_call('sudo losetup -d %s' % loop_dev, shell=True)
+        check_call('sudo umount %s' % loop_dev, shell=True)
+        check_call('sudo losetup -d %s' % loop_dev, shell=True)

     except CalledProcessError as e:
         pytest.skip('Setup failed: %s' % e.cmd)
--
2.26.1

[PATCH 1/2] test/py: efi_secboot should not assume sbin is in the path

[AKASHI Takahiro]

Heinrich,

On Wed, Apr 22, 2020 at 05:52:54PM +0200, Heinrich Schuchardt wrote:
> For non-root users /sbin is typically not in the path. So use absolute
> paths to mkfs.vfat and sgdisk.

As Stephen pointed out somewhere else else before, you should
set $PATH properly instead of using an absolute path directly.

Thanks,
-Takahiro Akashi



> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/py/tests/test_efi_secboot/conftest.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> index e542fef6e8..40cdf15bf2 100644
> --- a/test/py/tests/test_efi_secboot/conftest.py
> +++ b/test/py/tests/test_efi_secboot/conftest.py
> @@ -48,12 +48,12 @@ def efi_boot_env(request, u_boot_config):
>          # create a disk/partition
>          check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
>                              % (image_path, image_size), shell=True)
> -        check_call('sgdisk %s -n 1:0:+%dMiB'
> +        check_call('/sbin/sgdisk %s -n 1:0:+%dMiB'
>                              % (image_path, part_size), shell=True)
>          # create a file system
>          check_call('dd if=/dev/zero of=%s.tmp bs=1MiB count=%d'
>                              % (image_path, part_size), shell=True)
> -        check_call('mkfs -t %s %s.tmp' % (fs_type, image_path), shell=True)
> +        check_call('/sbin/mkfs.%s %s.tmp' % (fs_type, image_path), shell=True)
>          check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
>                              % (image_path, image_path, 1), shell=True)
>          check_call('rm %s.tmp' % image_path, shell=True)
> --
> 2.26.1
> 

[PATCH 2/2] test/py: fix test_efi_secboot/conftest.py

[AKASHI Takahiro]

Heinrich,

On Wed, Apr 22, 2020 at 05:52:55PM +0200, Heinrich Schuchardt wrote:
> If udisksctl is present
> test/py/tests/test_efi_secboot/conftest.py
> fails because the disk image is never mounted.
> 
> Normal users can only mount fuse file systems. Unfortunately fusefat is
> still in an experimental state and seems not to work here correctly.

I haven't confirmed that fuse/fat is not stable, but

> So as we have to be root or use the sudo command anyway delete all coding
> referring to udisksctl.

I don't mind non-root path being deleted as it was used when Travis CI
didn't work with "sudo".

> --
> 
> We should not use mount point /mnt as this directory or one of its
> sub-directories might already be in use as active mount points. Instead
> create a new directory in the build root as mount point.
> 
> --
> 
> Remove debug print statements that have been commented out. print without
> parentheses is anyway invalid in Python 3. And pytest anyway filters out
> the output if there is no exception reported.

'printing a mount point' was mostly useful for debugging
non-root path.

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/py/tests/test_efi_secboot/conftest.py | 30 +++++-----------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> index 40cdf15bf2..93d308cf0d 100644
> --- a/test/py/tests/test_efi_secboot/conftest.py
> +++ b/test/py/tests/test_efi_secboot/conftest.py
> @@ -43,7 +43,8 @@ def efi_boot_env(request, u_boot_config):
>          HELLO_PATH = u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi'
> 
>      try:
> -        non_root = tool_is_in_path('udisksctl')
> +        mnt_point = u_boot_config.persistent_data_dir + '/mnt_efisecure'
> +        check_call('mkdir -p {}'.format(mnt_point), shell=True)

For consistency, it would be better to use "%" formatting as elsewhere
in the file.

Thanks,
-Takahiro Akashi

> 
>          # create a disk/partition
>          check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
> @@ -57,25 +58,11 @@ def efi_boot_env(request, u_boot_config):
>          check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
>                              % (image_path, image_path, 1), shell=True)
>          check_call('rm %s.tmp' % image_path, shell=True)
> -        if non_root:
> -            out_data = check_output('udisksctl loop-setup -f %s -o %d'
> -                                % (image_path, 1048576), shell=True).decode()
> -            m = re.search('(?<= as )(.*)\.', out_data)
> -            loop_dev = m.group(1)
> -            # print 'loop device is: %s' % loop_dev
> -            out_data = check_output('udisksctl info -b %s'
> -                                % loop_dev, shell=True).decode()
> -            m = re.search('MountPoints:[ \t]+(.*)', out_data)
> -            mnt_point = m.group(1)
> -        else:
> -            loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
> +        loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
>                                  % (part_size, image_path), shell=True).decode()
> -            mnt_point = '/mnt'
> -            check_output('sudo mount -t %s -o umask=000 %s %s'
> +        check_output('sudo mount -t %s -o umask=000 %s %s'
>                                  % (fs_type, loop_dev, mnt_point), shell=True)
> 
> -        # print 'mount point is: %s' % mnt_point
> -
>          # suffix
>          # *.key: RSA private key in PEM
>          # *.crt: X509 certificate (self-signed) in PEM
> @@ -134,13 +121,8 @@ def efi_boot_env(request, u_boot_config):
>                              % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
>                              shell=True)
> 
> -        if non_root:
> -            check_call('udisksctl unmount -b %s' % loop_dev, shell=True)
> -            # not needed
> -            # check_call('udisksctl loop-delete -b %s' % loop_dev, shell=True)
> -        else:
> -            check_call('sudo umount %s' % loop_dev, shell=True)
> -            check_call('sudo losetup -d %s' % loop_dev, shell=True)
> +        check_call('sudo umount %s' % loop_dev, shell=True)
> +        check_call('sudo losetup -d %s' % loop_dev, shell=True)
> 
>      except CalledProcessError as e:
>          pytest.skip('Setup failed: %s' % e.cmd)
> --
> 2.26.1
> 

[PATCH 2/2] test/py: fix test_efi_secboot/conftest.py

[Heinrich Schuchardt]

On 23.04.20 02:15, AKASHI Takahiro wrote:
> Heinrich,
>
> On Wed, Apr 22, 2020 at 05:52:55PM +0200, Heinrich Schuchardt wrote:
>> If udisksctl is present
>> test/py/tests/test_efi_secboot/conftest.py
>> fails because the disk image is never mounted.
>>
>> Normal users can only mount fuse file systems. Unfortunately fusefat is
>> still in an experimental state and seems not to work here correctly.
>
> I haven't confirmed that fuse/fat is not stable, but
>
>> So as we have to be root or use the sudo command anyway delete all coding
>> referring to udisksctl.
>
> I don't mind non-root path being deleted as it was used when Travis CI
> didn't work with "sudo".
>
>> --
>>
>> We should not use mount point /mnt as this directory or one of its
>> sub-directories might already be in use as active mount points. Instead
>> create a new directory in the build root as mount point.
>>
>> --
>>
>> Remove debug print statements that have been commented out. print without
>> parentheses is anyway invalid in Python 3. And pytest anyway filters out
>> the output if there is no exception reported.
>
> 'printing a mount point' was mostly useful for debugging
> non-root path.

The code in your comments was invalid: We are using Python 3. In Python
3 print() is a function.

>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  test/py/tests/test_efi_secboot/conftest.py | 30 +++++-----------------
>>  1 file changed, 6 insertions(+), 24 deletions(-)
>>
>> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
>> index 40cdf15bf2..93d308cf0d 100644
>> --- a/test/py/tests/test_efi_secboot/conftest.py
>> +++ b/test/py/tests/test_efi_secboot/conftest.py
>> @@ -43,7 +43,8 @@ def efi_boot_env(request, u_boot_config):
>>          HELLO_PATH = u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi'
>>
>>      try:
>> -        non_root = tool_is_in_path('udisksctl')
>> +        mnt_point = u_boot_config.persistent_data_dir + '/mnt_efisecure'
>> +        check_call('mkdir -p {}'.format(mnt_point), shell=True)
>
> For consistency, it would be better to use "%" formatting as elsewhere
> in the file.

https://docs.python.org/release/3.0/library/stdtypes.html#old-string-formatting-operations

explicitly says:

"The formatting operations described here are obsolete and may go away
in future versions of Python. Use the new String Formatting in new code."

As using % was deprecated 12 years ago it would be great if you could
rework your code to use current Python 3.

Best regards

Heinrich

>
> Thanks,
> -Takahiro Akashi
>
>>
>>          # create a disk/partition
>>          check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
>> @@ -57,25 +58,11 @@ def efi_boot_env(request, u_boot_config):
>>          check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
>>                              % (image_path, image_path, 1), shell=True)
>>          check_call('rm %s.tmp' % image_path, shell=True)
>> -        if non_root:
>> -            out_data = check_output('udisksctl loop-setup -f %s -o %d'
>> -                                % (image_path, 1048576), shell=True).decode()
>> -            m = re.search('(?<= as )(.*)\.', out_data)
>> -            loop_dev = m.group(1)
>> -            # print 'loop device is: %s' % loop_dev
>> -            out_data = check_output('udisksctl info -b %s'
>> -                                % loop_dev, shell=True).decode()
>> -            m = re.search('MountPoints:[ \t]+(.*)', out_data)
>> -            mnt_point = m.group(1)
>> -        else:
>> -            loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
>> +        loop_dev = check_output('sudo losetup -o 1MiB --sizelimit %dMiB --show -f %s | tr -d "\n"'
>>                                  % (part_size, image_path), shell=True).decode()
>> -            mnt_point = '/mnt'
>> -            check_output('sudo mount -t %s -o umask=000 %s %s'
>> +        check_output('sudo mount -t %s -o umask=000 %s %s'
>>                                  % (fs_type, loop_dev, mnt_point), shell=True)
>>
>> -        # print 'mount point is: %s' % mnt_point
>> -
>>          # suffix
>>          # *.key: RSA private key in PEM
>>          # *.crt: X509 certificate (self-signed) in PEM
>> @@ -134,13 +121,8 @@ def efi_boot_env(request, u_boot_config):
>>                              % (mnt_point, EFITOOLS_PATH, EFITOOLS_PATH),
>>                              shell=True)
>>
>> -        if non_root:
>> -            check_call('udisksctl unmount -b %s' % loop_dev, shell=True)
>> -            # not needed
>> -            # check_call('udisksctl loop-delete -b %s' % loop_dev, shell=True)
>> -        else:
>> -            check_call('sudo umount %s' % loop_dev, shell=True)
>> -            check_call('sudo losetup -d %s' % loop_dev, shell=True)
>> +        check_call('sudo umount %s' % loop_dev, shell=True)
>> +        check_call('sudo losetup -d %s' % loop_dev, shell=True)
>>
>>      except CalledProcessError as e:
>>          pytest.skip('Setup failed: %s' % e.cmd)
>> --
>> 2.26.1
>>

[PATCH 1/2] test/py: efi_secboot should not assume sbin is in the path

[Heinrich Schuchardt]

On 22.04.20 17:52, Heinrich Schuchardt wrote:
> For non-root users /sbin is typically not in the path. So use absolute
> paths to mkfs.vfat and sgdisk.

Ilias pointed me to virt-make-fs (Debian package libguestfs-tools). With
this command the creation of file systems would become much easier.

Best regards

Heinrich

>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  test/py/tests/test_efi_secboot/conftest.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> index e542fef6e8..40cdf15bf2 100644
> --- a/test/py/tests/test_efi_secboot/conftest.py
> +++ b/test/py/tests/test_efi_secboot/conftest.py
> @@ -48,12 +48,12 @@ def efi_boot_env(request, u_boot_config):
>          # create a disk/partition
>          check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
>                              % (image_path, image_size), shell=True)
> -        check_call('sgdisk %s -n 1:0:+%dMiB'
> +        check_call('/sbin/sgdisk %s -n 1:0:+%dMiB'
>                              % (image_path, part_size), shell=True)
>          # create a file system
>          check_call('dd if=/dev/zero of=%s.tmp bs=1MiB count=%d'
>                              % (image_path, part_size), shell=True)
> -        check_call('mkfs -t %s %s.tmp' % (fs_type, image_path), shell=True)
> +        check_call('/sbin/mkfs.%s %s.tmp' % (fs_type, image_path), shell=True)
>          check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
>                              % (image_path, image_path, 1), shell=True)
>          check_call('rm %s.tmp' % image_path, shell=True)
> --
> 2.26.1
>

[PATCH 1/2] test/py: efi_secboot should not assume sbin is in the path

[AKASHI Takahiro]

On Fri, May 15, 2020 at 05:39:31PM +0200, Heinrich Schuchardt wrote:
> On 22.04.20 17:52, Heinrich Schuchardt wrote:
> > For non-root users /sbin is typically not in the path. So use absolute
> > paths to mkfs.vfat and sgdisk.
> 
> Ilias pointed me to virt-make-fs (Debian package libguestfs-tools). With
> this command the creation of file systems would become much easier.

Okay, and are you going to revert this change and submit another patch?

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  test/py/tests/test_efi_secboot/conftest.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> > index e542fef6e8..40cdf15bf2 100644
> > --- a/test/py/tests/test_efi_secboot/conftest.py
> > +++ b/test/py/tests/test_efi_secboot/conftest.py
> > @@ -48,12 +48,12 @@ def efi_boot_env(request, u_boot_config):
> >          # create a disk/partition
> >          check_call('dd if=/dev/zero of=%s bs=1MiB count=%d'
> >                              % (image_path, image_size), shell=True)
> > -        check_call('sgdisk %s -n 1:0:+%dMiB'
> > +        check_call('/sbin/sgdisk %s -n 1:0:+%dMiB'
> >                              % (image_path, part_size), shell=True)
> >          # create a file system
> >          check_call('dd if=/dev/zero of=%s.tmp bs=1MiB count=%d'
> >                              % (image_path, part_size), shell=True)
> > -        check_call('mkfs -t %s %s.tmp' % (fs_type, image_path), shell=True)
> > +        check_call('/sbin/mkfs.%s %s.tmp' % (fs_type, image_path), shell=True)
> >          check_call('dd if=%s.tmp of=%s bs=1MiB seek=1 count=%d conv=notrunc'
> >                              % (image_path, image_path, 1), shell=True)
> >          check_call('rm %s.tmp' % image_path, shell=True)
> > --
> > 2.26.1
> >
> 
>