[lxc-devel] [PATCH] handle unprivileged user calls more gracefully
S.Çağlar Onur
caglar at 10ur.org
Sat Jan 18 00:27:03 UTC 2014
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.
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.
> 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 :)
>> ---
>> 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>
More information about the lxc-devel
mailing list