[libcamera-devel] [PATCH 2/3] test: log/process: check CAP_SYS_ADMIN in test init

[You-Sheng Yang]

On 2020-07-27 07:47, Laurent Pinchart wrote:
> Hi again,
> 
> On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote:
>> Hi You-Sheng,
>>
>> Thank you for the patch.
>>
>> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote:
>>> While these tests may be executed as normal user at build time,
>>> unshare() call will fail and so are tests log_process and process_test.
>>> This change checks if one is granted with necessary capabilities so that
>>> we don't fail the build unexpectedly.
>>>
>>> Signed-off-by: You-Sheng Yang <vicamo.yang at canonical.com>
>>> ---
>>>  test/log/log_process.cpp      | 20 ++++++++++++++++++++
>>>  test/log/meson.build          |  2 +-
>>>  test/meson.build              |  2 ++
>>>  test/process/meson.build      |  2 +-
>>>  test/process/process_test.cpp | 23 +++++++++++++++++++++++
>>>  5 files changed, 47 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/test/log/log_process.cpp b/test/log/log_process.cpp
>>> index d46d5e3..876da22 100644
>>> --- a/test/log/log_process.cpp
>>> +++ b/test/log/log_process.cpp
>>> @@ -9,6 +9,7 @@
>>>  #include <iostream>
>>>  #include <random>
>>>  #include <string.h>
>>> +#include <sys/capability.h>
>>>  #include <sys/stat.h>
>>>  #include <sys/types.h>
>>>  #include <unistd.h>
>>> @@ -55,6 +56,25 @@ class LogProcessTest : public Test
>>>  protected:
>>>  	int init()
>>>  	{
>>> +		int ret = TestPass;
>>> +
>>> +		cap_t caps = cap_get_proc();
>>> +		if (caps == NULL) {
>>> +			cerr << "failed to check process capabilities" << endl;
>>> +			return TestFail;
>>> +		}
>>> +
>>> +		/* Check required permissions: CAP_SYS_ADMIN: unshare */
>>> +		cap_flag_value_t fv;
>>> +		if ((cap_get_flag(caps, CAP_SYS_ADMIN, CAP_EFFECTIVE, &fv) < 0) || (fv != CAP_SET)) {
>>> +			cerr << "skip due to insufficient capability" << endl;
>>> +			ret = TestSkip;
>>> +		}
>>
>> Would it make sense to add this as a helper function to the base Test
>> class ?

Will do. But probably after having a conclusion below.

>>> +
>>> +		cap_free(caps);
>>> +		if (ret != TestPass)
>>> +			return ret;
>>> +
>>>  		random_device random;
>>>  		num_ = random();
>>>  		logPath_ = "/tmp/libcamera.worker.test." +
>>> diff --git a/test/log/meson.build b/test/log/meson.build
>>> index 8cd664e..000f980 100644
>>> --- a/test/log/meson.build
>>> +++ b/test/log/meson.build
>>> @@ -7,7 +7,7 @@ log_test = [
>>>  
>>>  foreach t : log_test
>>>      exe = executable(t[0], t[1],
>>> -                     dependencies : libcamera_dep,
>>> +                     dependencies : [libcamera_dep, libcap],
>>>                       link_with : test_libraries,
>>>                       include_directories : test_includes_internal)
>>>  
>>> diff --git a/test/meson.build b/test/meson.build
>>> index f41d6e7..b4db328 100644
>>> --- a/test/meson.build
>>> +++ b/test/meson.build
>>> @@ -1,5 +1,7 @@
>>>  # SPDX-License-Identifier: CC0-1.0
>>>  
>>> +libcap = dependency('libcap', required : true)
>>
>> 'true' is the default value for 'required', you can omit it. However,
>> I'd like to keep the dependency optional, as we try to also support
>> resource-constrainted embedded systems (based on musl or uclibc for
>> instance, and/or without udev).
>>
>> I have an idea how to do that, I'll try to submit a patch shortly.
> 
> Actually, thinking about it some more, would it make sense to instead
> condition the call to unshare() to CAP_SYS_ADMIN in the
> Process:isolate() class ? Or turn it into a non-fatal error ?

It's about API design, so your opinions matter most.

I didn't have much idea about the rational behind the unshare() call
inside libcamera::Process, but I'm really suspect the necessity of it as
part of a, at least looks like, generic API. It implicitly adds a
constrain that any process tries to create a subprocess in libcamera
using libcamera::Process, its child process must be either executed by
root or have CAP_SYS_ADMIN. This doesn't really sound a good idea for
me, especially when I believe one should really build a multimedia
library to run as a normal user as possible.

Anyway, the only user of this API in libcamera is ipa_proxy_linux, you
could have put unshare() into ipa_proxy_linux itself. This way you could
install some selinux/apparmor rules to grant such permission to this
executable explicitly. But again, is that really necessary? Is
ipa_proxy_linux really has to own its own network and uid namespace?

> Could you maybe elaborate a little bit on the failure this patch is
> trying to solve ? I haven't seen any such failure, how can they be
> reproduced ?

Please see https://gitlab.com/vicamo/libcamera/-/jobs/650449281

21/55 libcamera:process / process_test                 FAIL
0.01s (exit status 255 or signal 127 SIGinvalid)
--- command ---
10:26:15
/builds/vicamo/libcamera/debian/output/libcamera-0~git20200722+d929555/obj-x86_64-linux-gnu/test/process/process_test
--- stderr ---

I was trying to fix debian packaging and to have a daily build based on
master tip.

You-Sheng Yang

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200727/3cee0c7f/attachment.sig>