[lxc-devel] [PATCH] handle unprivileged user calls more gracefully
Stéphane Graber
stgraber at ubuntu.com
Sat Jan 18 00:34:11 UTC 2014
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.
>
> >> ---
> >> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140117/0de83062/attachment.pgp>
More information about the lxc-devel
mailing list