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 <cag...@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.
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())"?
> ---
> 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@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
signature.asc
Description: Digital signature
_______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel
