| Message ID | 20200725122442.1679820-3-vicamo.yang@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + > + 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. > + > subdir('libtest') > > subdir('camera') > diff --git a/test/process/meson.build b/test/process/meson.build > index c215fa7..828c17b 100644 > --- a/test/process/meson.build > +++ b/test/process/meson.build > @@ -6,7 +6,7 @@ process_tests = [ > > foreach t : process_tests > 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/process/process_test.cpp b/test/process/process_test.cpp > index ce0cc7c..ffa2143 100644 > --- a/test/process/process_test.cpp > +++ b/test/process/process_test.cpp > @@ -5,6 +5,8 @@ > * process_test.cpp - Process test > */ > > +#include <sys/capability.h> > + > #include <iostream> > #include <unistd.h> > #include <vector> > @@ -41,6 +43,27 @@ public: > } > > 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; > + } > + > + cap_free(caps); > + return ret; > + } > + > int run() > { > EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
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@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 ? > > > + > > + 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 ? 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 ? > > + > > subdir('libtest') > > > > subdir('camera') > > diff --git a/test/process/meson.build b/test/process/meson.build > > index c215fa7..828c17b 100644 > > --- a/test/process/meson.build > > +++ b/test/process/meson.build > > @@ -6,7 +6,7 @@ process_tests = [ > > > > foreach t : process_tests > > 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/process/process_test.cpp b/test/process/process_test.cpp > > index ce0cc7c..ffa2143 100644 > > --- a/test/process/process_test.cpp > > +++ b/test/process/process_test.cpp > > @@ -5,6 +5,8 @@ > > * process_test.cpp - Process test > > */ > > > > +#include <sys/capability.h> > > + > > #include <iostream> > > #include <unistd.h> > > #include <vector> > > @@ -41,6 +43,27 @@ public: > > } > > > > 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; > > + } > > + > > + cap_free(caps); > > + return ret; > > + } > > + > > int run() > > { > > EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
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@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
Hi You-Sheng, On Mon, Jul 27, 2020 at 11:50:39PM +0800, You-Sheng Yang wrote: > On 2020-07-27 07:47, Laurent Pinchart wrote: > > On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote: > >> 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@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. The Process class is meant to run closed-source image processing algorithm (IPA) modules in a separate, isolated process. The unshare() call is a very first (mockup) step in that direction, and we know more work is needed to achieve a real sandboxing. Now that I think about it, it may be better to instead rely on minijail or firejail instead of reinventing the wheel. > 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? We want to isolate the IPA modules, limiting their access to the system as much as possible. They should only be able to access specific file system directories (in order to load configuration data and write logs), and nothing else (no device access, no network access, ...). Closed-source IPA modules are considered to be untrusted binaries. > > 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 That's lots of failures :-S The process test has been part of our test suite for a long time, and it's not run as root or with CAP_SYS_ADMIN. As far as I can tell, we've never noticed any issue with unshare() failing. I'm not sure what's different in your environment. We can also consider dropping the unshare() call for now, as it's only a partial implementation of process isolation. We would need to implement that feature down the line though. Wrapping the ipa_proxy_worker with minijail or firejail, or implementing isolation in the worker itself, are two possible candidates. Another option would be to run the proxy worker as a system daemon, but at this point we would like to avoid going down that route if possible. Do you have any recommendation ? > 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.
Hi Laurent, On 2020-07-28 07:58, Laurent Pinchart wrote: > Hi You-Sheng, > > On Mon, Jul 27, 2020 at 11:50:39PM +0800, You-Sheng Yang wrote: >> On 2020-07-27 07:47, Laurent Pinchart wrote: >>> On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote: >>>> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote: >>>>> 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. > > The Process class is meant to run closed-source image processing > algorithm (IPA) modules in a separate, isolated process. The unshare() > call is a very first (mockup) step in that direction, and we know more > work is needed to achieve a real sandboxing. > > Now that I think about it, it may be better to instead rely on minijail > or firejail instead of reinventing the wheel. >> 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? > > We want to isolate the IPA modules, limiting their access to the system > as much as possible. They should only be able to access specific file > system directories (in order to load configuration data and write logs), > and nothing else (no device access, no network access, ...). > Closed-source IPA modules are considered to be untrusted binaries. I understand. But while ipa_linux_proxy is currently integrated into libcamera source, you know and can setup constrains for it correctly. When some other vendor adopts libcamera and creates similar plugin by their own, that may become something blocking their normal function. >>> 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 > > That's lots of failures :-S > > The process test has been part of our test suite for a long time, and > it's not run as root or with CAP_SYS_ADMIN. As far as I can tell, we've > never noticed any issue with unshare() failing. I'm not sure what's > different in your environment. This is executed in a unprivileged docker container. And since unshare() takes CAP_SYS_ADMIN, if that doesn't fail in your setup, it follows either that test has never been enrolled or its executed with CAP_SYS_ADMIN somehow. > We can also consider dropping the unshare() call for now, as it's only a > partial implementation of process isolation. We would need to implement > that feature down the line though. Wrapping the ipa_proxy_worker with > minijail or firejail, or implementing isolation in the worker itself, > are two possible candidates. Another option would be to run the proxy > worker as a system daemon, but at this point we would like to avoid > going down that route if possible. minijail/firejail seem sufficient to me. It's already a separate process. There are many other ways to contain it without touching the source code. > Do you have any recommendation ? Not really. But as an end user, I would really love to see you drop all those incomplete/error-prone EventBlahBlah/Signal/Timer stuff with Boost io_service/process/... since you're already using boost. You-Sheng Yang
Hi You-Sheng, (CC'ing Paul Elder) On Tue, Jul 28, 2020 at 11:39:38AM +0800, You-Sheng Yang wrote: > On 2020-07-28 07:58, Laurent Pinchart wrote: > > On Mon, Jul 27, 2020 at 11:50:39PM +0800, You-Sheng Yang wrote: > >> On 2020-07-27 07:47, Laurent Pinchart wrote: > >>> On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote: > >>>> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote: > >>>>> 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. > > > > The Process class is meant to run closed-source image processing > > algorithm (IPA) modules in a separate, isolated process. The unshare() > > call is a very first (mockup) step in that direction, and we know more > > work is needed to achieve a real sandboxing. > > > > Now that I think about it, it may be better to instead rely on minijail > > or firejail instead of reinventing the wheel. > > > >> 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? > > > > We want to isolate the IPA modules, limiting their access to the system > > as much as possible. They should only be able to access specific file > > system directories (in order to load configuration data and write logs), > > and nothing else (no device access, no network access, ...). > > Closed-source IPA modules are considered to be untrusted binaries. > > I understand. But while ipa_linux_proxy is currently integrated into > libcamera source, you know and can setup constrains for it correctly. > When some other vendor adopts libcamera and creates similar plugin by > their own, that may become something blocking their normal function. > > >>> 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 > > > > That's lots of failures :-S > > > > The process test has been part of our test suite for a long time, and > > it's not run as root or with CAP_SYS_ADMIN. As far as I can tell, we've > > never noticed any issue with unshare() failing. I'm not sure what's > > different in your environment. > > This is executed in a unprivileged docker container. And since unshare() > takes CAP_SYS_ADMIN, if that doesn't fail in your setup, it follows > either that test has never been enrolled or its executed with > CAP_SYS_ADMIN somehow. I've double-checked, and I can run the test successfully, without CAP_SYS_ADMIN. I expect the problem to be caused by calling unshared(CLONE_NEWUSER) within a docker container, which is something I haven't tried. Nonetheless, we need to address this issue. Paul, you're the author of that code, do you think we could just drop the skeleton isolation for now, and bring it back in another form later ? > > We can also consider dropping the unshare() call for now, as it's only a > > partial implementation of process isolation. We would need to implement > > that feature down the line though. Wrapping the ipa_proxy_worker with > > minijail or firejail, or implementing isolation in the worker itself, > > are two possible candidates. Another option would be to run the proxy > > worker as a system daemon, but at this point we would like to avoid > > going down that route if possible. > > minijail/firejail seem sufficient to me. It's already a separate > process. There are many other ways to contain it without touching the > source code. I think that's the direction we'll take. > > Do you have any recommendation ? > > Not really. But as an end user, I would really love to see you drop all > those incomplete/error-prone EventBlahBlah/Signal/Timer stuff with Boost > io_service/process/... since you're already using boost. Boost is only needed for the Raspberry Pi IPA module, for the JSON parser, and it's something we may consider. libcamera aims at supporting resource-constrained embedded systems (as much as we can reasonably do), hence the attempt to limit the number of dependencies. That being said, this decision could be reconsidered later. Regarding timers and event notifiers, I think we'll simply drop them from the public API, and make them internal only, as libcamera's goal isn't to provide a generic implementation of those concepts.
Hello, Sorry for the delay. On Tue, Jul 28, 2020 at 10:39:34PM +0300, Laurent Pinchart wrote: > Hi You-Sheng, > > (CC'ing Paul Elder) > > On Tue, Jul 28, 2020 at 11:39:38AM +0800, You-Sheng Yang wrote: > > On 2020-07-28 07:58, Laurent Pinchart wrote: > > > On Mon, Jul 27, 2020 at 11:50:39PM +0800, You-Sheng Yang wrote: > > >> On 2020-07-27 07:47, Laurent Pinchart wrote: > > >>> On Mon, Jul 27, 2020 at 02:42:11AM +0300, Laurent Pinchart wrote: > > >>>> On Sat, Jul 25, 2020 at 08:24:41PM +0800, You-Sheng Yang wrote: > > >>>>> 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. > > > > > > The Process class is meant to run closed-source image processing > > > algorithm (IPA) modules in a separate, isolated process. The unshare() > > > call is a very first (mockup) step in that direction, and we know more > > > work is needed to achieve a real sandboxing. > > > > > > Now that I think about it, it may be better to instead rely on minijail > > > or firejail instead of reinventing the wheel. > > > > > >> 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? > > > > > > We want to isolate the IPA modules, limiting their access to the system > > > as much as possible. They should only be able to access specific file > > > system directories (in order to load configuration data and write logs), > > > and nothing else (no device access, no network access, ...). > > > Closed-source IPA modules are considered to be untrusted binaries. > > > > I understand. But while ipa_linux_proxy is currently integrated into > > libcamera source, you know and can setup constrains for it correctly. > > When some other vendor adopts libcamera and creates similar plugin by > > their own, that may become something blocking their normal function. > > > > >>> 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 > > > > > > That's lots of failures :-S > > > > > > The process test has been part of our test suite for a long time, and > > > it's not run as root or with CAP_SYS_ADMIN. As far as I can tell, we've > > > never noticed any issue with unshare() failing. I'm not sure what's > > > different in your environment. > > > > This is executed in a unprivileged docker container. And since unshare() > > takes CAP_SYS_ADMIN, if that doesn't fail in your setup, it follows > > either that test has never been enrolled or its executed with > > CAP_SYS_ADMIN somehow. > > I've double-checked, and I can run the test successfully, without > CAP_SYS_ADMIN. I expect the problem to be caused by calling > unshared(CLONE_NEWUSER) within a docker container, which is something I > haven't tried. > > Nonetheless, we need to address this issue. Paul, you're the author of > that code, do you think we could just drop the skeleton isolation for > now, and bring it back in another form later ? Yeah, I think we could just drop the skeleton isolation for now. It's probably better to replace it with minijail (or something similar) instead of reimplementing our own jail. I think I used unshare() just to show how isolation would work/fit in. Paul
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; + } + + 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) + subdir('libtest') subdir('camera') diff --git a/test/process/meson.build b/test/process/meson.build index c215fa7..828c17b 100644 --- a/test/process/meson.build +++ b/test/process/meson.build @@ -6,7 +6,7 @@ process_tests = [ foreach t : process_tests 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/process/process_test.cpp b/test/process/process_test.cpp index ce0cc7c..ffa2143 100644 --- a/test/process/process_test.cpp +++ b/test/process/process_test.cpp @@ -5,6 +5,8 @@ * process_test.cpp - Process test */ +#include <sys/capability.h> + #include <iostream> #include <unistd.h> #include <vector> @@ -41,6 +43,27 @@ public: } 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; + } + + cap_free(caps); + return ret; + } + int run() { EventDispatcher *dispatcher = Thread::current()->eventDispatcher();
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@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(-)