[lxc-devel] [PATCH] handle unprivileged user calls more gracefully
S.Çağlar Onur
caglar at 10ur.org
Sat Jan 18 04:35:43 UTC 2014
On Fri, Jan 17, 2014 at 7:34 PM, Stéphane Graber <stgraber at ubuntu.com> wrote:
> On Fri, Jan 17, 2014 at 07:27:03PM -0500, S.Çağlar Onur wrote:
>> Hey Stéphane,
>>
>> On Fri, Jan 17, 2014 at 3:18 PM, Stéphane Graber <stgraber at ubuntu.com> wrote:
>> > On Fri, Jan 17, 2014 at 03:01:46PM -0500, S.Çağlar Onur wrote:
>> >> Return an error if the function is not supposed to be called by an unprivileged user.
>> >> Otherwise those calls fail in the middle of their execution with different reasons.
>> >>
>> >> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
>> >
>> > Hmm, so I have mixed feelings about this. I certainly understand the
>> > reason for wanting this and I was actually considering a similar patch
>> > after seeing lxc-info complain quite a bit about unprivileged
>> > containers.
>> >
>> > However, I think API calls should print an error message indicating that
>> > the requested function isn't currently supported with unprivileged
>> > containers and the individual callers should themselves check whether
>> > they're running unprivileged and show something more relevant to the
>> > user.
>>
>> This is what I'm doing in go bindings but we still need to handle
>> those cases in the C API itself as I believe there will be people
>> directly using it. I can definitely add some error messages to
>> unprivileged cases to make it clear to the caller why the call failed.
>
> Right, the one place I can think of at least is lxc-info, so ideally
> we'd want to have it be unprivilege containers aware at the same time as
> we change the API.
>
>> On the other hand, I see this patch as an intermediate step as I
>> believe some of those functions could be made functional with
>> unprivileged containers as well.
>
> Agreed, at least anything relying on setns should be fixable.
> It's possible/likely that doing the right sequence of setns and remount
> we could make this work, if not, then we'd have to get a kernel patch.
>
> In any case, I think getting working attach by 1.0 is a very worthwhile
> goal as it's the main issue I see day to day when working with
> unprivileged containers.
>
>>
>> > Specifically, my concern with this is that lxc-ls and lxc-info will now
>> > report that a container doesn't have any interface or ip address, which
>> > is just wrong. Instead the user should be told that those are unknown as
>> > lxc-ls is currently doing.
>> >
>> > Also, the condition for am_unpriv seems a bit odd to me as it'd indicate
>> > that I can do:
>> > - p1 = Container("p1")
>> > - p1.start()
>> > - p1.clear_config_item("lxc.id_map")
>> > - p1.get_ips()
>> >
>> > And that'd workaround your test.
>> >
>> > Why can't it simply be: "return getuid() != 0;"?
>> >
>> > And if it ends up being just that, do we really need an am_unpriv(c)
>> > function when we can simply call "if (geteuid())"?
>>
>> I guess we can just call geteuid as you suggested, that am_priv code
>> was already in the code so I just reused it :)
>
> Yeah, I think it'd be easier to just call geteuid() in those cases and
> throwing an ERROR() with a relevant message for the given API call.
>
> In practice we should never see any of those ERROR in the upstream tools
> as they should all be detecting this before the actual API call, so it's
> just a safety net for external users.
OK, v2 should be in the list soon, hopefully addressing those issues,
please let me know what you think.
>>
>> >> ---
>> >> src/lxc/lxccontainer.c | 33 ++++++++++++++++++++++++++++-----
>> >> 1 file changed, 28 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
>> >> index 0bebdff..8d49e94 100644
>> >> --- a/src/lxc/lxccontainer.c
>> >> +++ b/src/lxc/lxccontainer.c
>> >> @@ -64,6 +64,10 @@
>> >>
>> >> lxc_log_define(lxc_container, lxc);
>> >>
>> >> +static bool am_unpriv(struct lxc_container *c) {
>> >> + return c->lxc_conf && geteuid() != 0 && !lxc_list_empty(&c->lxc_conf->id_map);
>> >> +}
>> >> +
>> >> static bool file_exists(const char *f)
>> >> {
>> >> struct stat statbuf;
>> >> @@ -1489,6 +1493,9 @@ static char** lxcapi_get_interfaces(struct lxc_container *c)
>> >> char **interfaces = NULL;
>> >> int old_netns = -1, new_netns = -1;
>> >>
>> >> + if (am_unpriv(c))
>> >> + goto out;
>> >> +
>> >> if (!enter_to_ns(c, &old_netns, &new_netns))
>> >> goto out;
>> >>
>> >> @@ -1538,6 +1545,9 @@ static char** lxcapi_get_ips(struct lxc_container *c, const char* interface, con
>> >> char *address = NULL;
>> >> int old_netns = -1, new_netns = -1;
>> >>
>> >> + if (am_unpriv(c))
>> >> + goto out;
>> >> +
>> >> if (!enter_to_ns(c, &old_netns, &new_netns))
>> >> goto out;
>> >>
>> >> @@ -1818,7 +1828,7 @@ static int lxc_rmdir_onedev_wrapper(void *data)
>> >> static bool lxcapi_destroy(struct lxc_container *c)
>> >> {
>> >> struct bdev *r = NULL;
>> >> - bool bret = false, am_unpriv;
>> >> + bool bret = false;
>> >> int ret;
>> >>
>> >> if (!c || !lxcapi_is_defined(c))
>> >> @@ -1833,14 +1843,12 @@ static bool lxcapi_destroy(struct lxc_container *c)
>> >> goto out;
>> >> }
>> >>
>> >> - am_unpriv = c->lxc_conf && geteuid() != 0 && !lxc_list_empty(&c->lxc_conf->id_map);
>> >> -
>> >> if (c->lxc_conf && has_snapshots(c)) {
>> >> ERROR("container %s has dependent snapshots", c->name);
>> >> goto out;
>> >> }
>> >>
>> >> - if (!am_unpriv && c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
>> >> + if (!am_unpriv(c) && c->lxc_conf->rootfs.path && c->lxc_conf->rootfs.mount) {
>> >> r = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
>> >> if (r) {
>> >> if (r->ops->destroy(r) < 0) {
>> >> @@ -1857,7 +1865,7 @@ static bool lxcapi_destroy(struct lxc_container *c)
>> >> const char *p1 = lxcapi_get_config_path(c);
>> >> char *path = alloca(strlen(p1) + strlen(c->name) + 2);
>> >> sprintf(path, "%s/%s", p1, c->name);
>> >> - if (am_unpriv)
>> >> + if (am_unpriv(c))
>> >> ret = userns_exec_1(c->lxc_conf, lxc_rmdir_onedev_wrapper, path);
>> >> else
>> >> ret = lxc_rmdir_onedev(path);
>> >> @@ -2406,6 +2414,9 @@ static struct lxc_container *lxcapi_clone(struct lxc_container *c, const char *n
>> >> if (!c || !c->is_defined(c))
>> >> return NULL;
>> >>
>> >> + if (am_unpriv(c))
>> >> + return NULL;
>> >> +
>> >> if (container_mem_lock(c))
>> >> return NULL;
>> >>
>> >> @@ -2587,6 +2598,9 @@ static int lxcapi_snapshot(struct lxc_container *c, const char *commentfile)
>> >> struct lxc_container *c2;
>> >> char snappath[MAXPATHLEN], newname[20];
>> >>
>> >> + if (am_unpriv(c))
>> >> + return -1;
>> >> +
>> >> // /var/lib/lxc -> /var/lib/lxcsnaps \0
>> >> ret = snprintf(snappath, MAXPATHLEN, "%ssnaps/%s", c->config_path, c->name);
>> >> if (ret < 0 || ret >= MAXPATHLEN)
>> >> @@ -2802,6 +2816,9 @@ static bool lxcapi_snapshot_restore(struct lxc_container *c, const char *snapnam
>> >> if (!c || !c->name || !c->config_path)
>> >> return false;
>> >>
>> >> + if (am_unpriv(c))
>> >> + return false;
>> >> +
>> >> bdev = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, NULL);
>> >> if (!bdev) {
>> >> ERROR("Failed to find original backing store type");
>> >> @@ -2851,6 +2868,9 @@ static bool lxcapi_snapshot_destroy(struct lxc_container *c, const char *snapnam
>> >> if (!c || !c->name || !c->config_path)
>> >> return false;
>> >>
>> >> + if (am_unpriv(c))
>> >> + return false;
>> >> +
>> >> ret = snprintf(clonelxcpath, MAXPATHLEN, "%ssnaps/%s", c->config_path, c->name);
>> >> if (ret < 0 || ret >= MAXPATHLEN)
>> >> goto err;
>> >> @@ -2888,6 +2908,9 @@ static bool add_remove_device_node(struct lxc_container *c, const char *src_path
>> >> char *directory_path = NULL;
>> >> const char *p;
>> >>
>> >> + if (am_unpriv(c))
>> >> + goto out;
>> >> +
>> >> /* make sure container is running */
>> >> if (!c->is_running(c)) {
>> >> ERROR("container is not running");
>> >> --
>> >> 1.8.3.2
>> >>
>> >> _______________________________________________
>> >> lxc-devel mailing list
>> >> lxc-devel at lists.linuxcontainers.org
>> >> http://lists.linuxcontainers.org/listinfo/lxc-devel
>> >
>> > --
>> > Stéphane Graber
>> > Ubuntu developer
>> > http://www.ubuntu.com
>> >
>> > _______________________________________________
>> > lxc-devel mailing list
>> > lxc-devel at lists.linuxcontainers.org
>> > http://lists.linuxcontainers.org/listinfo/lxc-devel
>> >
>>
>>
>>
>> --
>> S.Çağlar Onur <caglar at 10ur.org>
>> _______________________________________________
>> lxc-devel mailing list
>> lxc-devel at lists.linuxcontainers.org
>> http://lists.linuxcontainers.org/listinfo/lxc-devel
>
> --
> Stéphane Graber
> Ubuntu developer
> http://www.ubuntu.com
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
>
--
S.Çağlar Onur <caglar at 10ur.org>
More information about the lxc-devel
mailing list