10:22 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
THis patch fixes all of Eric's and Daniels comments.
[PATCH] virt-login-shell joins users into lxc container.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
=7zpw
-----END PGP SIGNATURE-----
10:52 a.m.
On Fri, Aug 02, 2013 at 11:22:07AM -0400, Daniel J Walsh wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
THis patch fixes all of Eric's and Daniels comments.
[PATCH] virt-login-shell joins users into lxc container.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
=7zpw
-----END PGP SIGNATURE-----
>From
01c7ab48e720f34c2aa891a8fa07812b1c66c316 Mon Sep 17 00:00:00 2001
From: Dan Walsh <dwalsh(a)redhat.com>
Date: Fri, 28 Jun 2013 13:50:58 -0400
Subject: [PATCH] virt-login-shell joins users into lxc container.
Openshift wants to have their gears stuck into a container when they login
to the system. virt-login-shell will join a running gear with the username of
the person running it, or attempt to start the container if it is not running.
(Currently containers do not exist if they are not running, so I can not test
this feature. But the code is there).
This tool needs to be setuid since joining a container (nsjoin) requires privs.
The root user is not allowed to execute this command. When this tool is
run by a normal user it will only join the "users" container.
Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
are allowed to join containers using this tool. By default no users are allowed.
---
.gitignore | 1 +
libvirt.spec.in | 3 +
po/POTFILES.in | 1 +
tools/Makefile.am | 30 +++-
tools/virt-login-shell.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
tools/virt-login-shell.conf | 26 ++++
tools/virt-login-shell.pod | 62 ++++++++
7 files changed, 472 insertions(+), 1 deletion(-)
create mode 100644 tools/virt-login-shell.c
create mode 100644 tools/virt-login-shell.conf
create mode 100644 tools/virt-login-shell.pod
ACK,
looks good to me now.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
10:47 a.m.
On Fri, Aug 02, 2013 at 04:52:52PM +0100, Daniel P. Berrange wrote:
On Fri, Aug 02, 2013 at 11:22:07AM -0400, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> THis patch fixes all of Eric's and Daniels comments.
>
> [PATCH] virt-login-shell joins users into lxc container.
FYI, I pushed this now
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.14 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
> BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
> =7zpw
> -----END PGP SIGNATURE-----
> >From 01c7ab48e720f34c2aa891a8fa07812b1c66c316 Mon Sep 17 00:00:00 2001
> From: Dan Walsh <dwalsh(a)redhat.com>
> Date: Fri, 28 Jun 2013 13:50:58 -0400
> Subject: [PATCH] virt-login-shell joins users into lxc container.
>
> Openshift wants to have their gears stuck into a container when they login
> to the system. virt-login-shell will join a running gear with the username of
> the person running it, or attempt to start the container if it is not running.
> (Currently containers do not exist if they are not running, so I can not test
> this feature. But the code is there).
>
> This tool needs to be setuid since joining a container (nsjoin) requires privs.
> The root user is not allowed to execute this command. When this tool is
> run by a normal user it will only join the "users" container.
>
> Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
> are allowed to join containers using this tool. By default no users are allowed.
> ---
> .gitignore | 1 +
> libvirt.spec.in | 3 +
> po/POTFILES.in | 1 +
> tools/Makefile.am | 30 +++-
> tools/virt-login-shell.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
> tools/virt-login-shell.conf | 26 ++++
> tools/virt-login-shell.pod | 62 ++++++++
> 7 files changed, 472 insertions(+), 1 deletion(-)
> create mode 100644 tools/virt-login-shell.c
> create mode 100644 tools/virt-login-shell.conf
> create mode 100644 tools/virt-login-shell.pod
ACK,
looks good to me now. 
5:27 p.m.
On 08/02/2013 09:22 AM, Daniel J Walsh wrote:
Subject: [PATCH] virt-login-shell joins users into lxc container.
Openshift wants to have their gears stuck into a container when they login
to the system. virt-login-shell will join a running gear with the username of
the person running it, or attempt to start the container if it is not running.
(Currently containers do not exist if they are not running, so I can not test
this feature. But the code is there).
This tool needs to be setuid since joining a container (nsjoin) requires privs.
The root user is not allowed to execute this command. When this tool is
run by a normal user it will only join the "users" container.
Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf
are allowed to join containers using this tool. By default no users are allowed.
---
.gitignore | 1 +
libvirt.spec.in | 3 +
po/POTFILES.in | 1 +
tools/Makefile.am | 30 +++-
tools/virt-login-shell.c | 350 ++++++++++++++++++++++++++++++++++++++++++++
tools/virt-login-shell.conf | 26 ++++
tools/virt-login-shell.pod | 62 ++++++++
7 files changed, 472 insertions(+), 1 deletion(-)
create mode 100644 tools/virt-login-shell.c
create mode 100644 tools/virt-login-shell.conf
create mode 100644 tools/virt-login-shell.pod
Already committed, but the following items are worth cleaning up in a
followup patch:
@@ -128,6 +133,24 @@ virt_host_validate_CFLAGS = \
$(COVERAGE_CFLAGS) \
$(NULL)
+virt_login_shell_SOURCES = \
+ virt-login-shell.conf \
.conf files should not be part of _SOURCES; it is already correctly
listed under conf_DATA, so I think you can just delete this line.
+
+static ssize_t nfdlist = 0;
+static int *fdlist = NULL;
Static variables are automatically 0-initialized without needing
something explicit. gcc can optimize this into .bss, but not all
compilers do that.
+static const char *conf_file = SYSCONFDIR
"/libvirt/virt-login-shell.conf";
+
+static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom)
Unintentional 2 spaces.
+static int virLoginShellAllowedUser(virConfPtr conf,
+ const char *name,
+ gid_t *groups)
+{
+ /*
+ If string begins with a % this indicates a linux group.
+ Check to see if the user is in the Linux Group.
+ */
+ if (pp->str[0] == '%') {
+ ptr = &pp->str[1];
+ if (!ptr)
This conditional is dead code (ptr is always non-NULL at this point).
Did you mean to check if (!*ptr) instead? And if so, you should
probably warn that "%" is an invalid string, rather than silently
ignoring it.
+ }
+ virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"),
name, conf_file);
Awkward grammar; I'd suggest s/as an/in/:
"%s not listed in allowed_users in %s"
+
+static char **virLoginShellGetShellArgv(virConfPtr conf)
+{
+ size_t i;
+ char **shargv=NULL;
+ virConfValuePtr p;
+
+ p = virConfGetValue(conf, "shell");
+ if (!p)
+ return virStringSplit("/bin/sh -l", " ", 3);
+
+ if (p && p->type == VIR_CONF_LIST) {
Dead conditional on the left of &&; you only get here if p is non-NULL.
+
+ if (VIR_ALLOC_N(shargv, len + 1) < 0)
+ goto error;
+ for (i = 0, pp = p->list; pp; i++, pp = pp->next) {
+ if (VIR_STRDUP(shargv[i], pp->str) < 0)
+ goto error;
+ }
+ shargv[len] = NULL;
Dead assignment; VIR_ALLOC_N guaranteed that shargv[len] starts life as
NULL.
+static void
+usage(void)
+{
+ fprintf(stdout, _("\n"
+ "%s is a privileged program that allows non root users
\n"
Outputting trailing whitespace to stdout is evil.
+ "specified in %s to join a Linux
container \n"
+ "with a matching user name and launch a shell. \n"
Two more instances.
+
+ struct option opt[] = {
+ {"help", no_argument, NULL, 'h'},
+ {NULL, 0, NULL, 0}
I still think all programs should have a --version option.
+
+ /* The only option we support is help
+ */
+ while ((arg = getopt_long(argc, argv, "h", opt, &longindex)) != -1) {
+ switch (arg) {
+ case 'h':
+ usage();
+ exit(EXIT_SUCCESS);
+ break;
+ }
+ }
That may be so, but the way you used getopt_long, you will print a
message and then plow onwards anyways. It is better to have a case '?'
that calls usage(); exit(EXIT_FAILURE);, so that if we later add a new
option, and a user calls this program without knowing whether the
version they are using was built before or after the addition of the new
option, then trying the new option will gracefully fail instead of
printing a warning and then behaving as if the option had not been
specified...
+
+ if (argc > optind) {
+ virReportSystemError(EINVAL, _("%s takes no options"), progname);
+ errno = EINVAL;
+ goto cleanup;
...especially since that's the behavior you chose for unknown non-options.
+
+ if (!virDomainIsActive(dom) && virDomainCreate(dom)) {
+ virErrorPtr last_error;
+ last_error = virGetLastError();
+ if (last_error->code != VIR_ERR_OPERATION_INVALID) {
+ virReportSystemError(last_error->code,_("Can't create %s
container: %s"), name, virGetLastErrorMessage());
Missing a space after ','.
Long line; please wrap to 80 columns.
+ if (ccpid == 0) {
+ if (chdir(homedir) < 0) {
+ virReportSystemError(errno, _("Unable chdir(%s)"), homedir);
+ return EXIT_FAILURE;
+ }
+ if (execv(shargv[0], (char *const*) shargv) < 0) {
+ virReportSystemError(errno, _("Unable exec shell %s"),
shargv[0]);
+ return -errno;
This ends up being status 255; but it is more typical to use status 127
on execv() failure.
+++ b/tools/virt-login-shell.pod
@@ -0,0 +1,62 @@
+=head1 NAME
+
+virt-login-shell - tool to execute a shell within a container matching the users name
+
+=head1 SYNOPSIS
+
+B<virt-login-shell>
+
+=head1 DESCRIPTION
+
+The B<virt-login-shell> program is setuid shell that is used to join
s/setuid/a setuid/
+an LXC container that matches the users name. If the container is
not
s/users/user's/
+running virt-login-shell will attempt to start the container.
s/running/running,/
+virt-sandbox-shell is not allowed to be run by root. Normal users
will get
+added to a container that matches their username, if it exists. And they are
+configured in /etc/libvirt/virt-login-shell.conf.
grammar:
s/if it exists. And/if it exists, and if/
+
+By default no users are allowed to user virt-login-shell, if you want to allow
s/to user/to use/
+certain users to use virt-login-shell, you need to modify the
allowed_users variable in /etc/libvirt/virt-login-shell.conf.
Long line; please wrap to 80 columns. (Didn't I make some of these
comments against your earlier version? I'm not fond of having review
comments ignored, only to have to make them again)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:51 a.m.
On 08/02/2013 11:22 AM, Daniel J Walsh wrote:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
THis patch fixes all of Eric's and Daniels comments.
[PATCH] virt-login-shell joins users into lxc container.
The overnight run of Coverity found two issues with this module. I've
cut-n-pasted the relevant portions of the error/traces:
#1: DEADCODE: virLoginShellAllowedUser()
86 if (pp->str[0] == '%') {
(1) Event assignment: Assigning: "ptr" = "pp->str + 1".
Also see events: [notnull][dead_error_condition][dead_error_line]
87 ptr = &pp->str[1];
(2) Event notnull: At condition "ptr", the value of "ptr" cannot be
NULL.
(3) Event dead_error_condition: The condition "!ptr" cannot be true.
Also see events: [assignment][dead_error_line]
88 if (!ptr)
(4) Event dead_error_line: Execution cannot reach this statement "continue;".
Also see events: [assignment][notnull][dead_error_condition]
89 continue;
Did you perhaps mean (!*ptr) (eg, the contents not the value?)
#2: USE_AFTER_FREE: main()
252 if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
253 goto cleanup;
254
(18) Event freed_arg: "virLoginShellAllowedUser(virConfPtr, char const *, gid_t
*)" frees "groups". [details]
(19) Event cond_false: Condition "virLoginShellAllowedUser(conf, name, groups) <
0", taking false branch
Also see events: [double_free]
255 if (virLoginShellAllowedUser(conf, name, groups) < 0)
256 goto cleanup;
257
Which is true:
...
61
62 static int virLoginShellAllowedUser(virConfPtr conf,
63 const char *name,
64 gid_t *groups)
65 {
...
(6) Event label: Reached label "cleanup"
110 cleanup:
111 VIR_FREE(gname);
(7) Event freed_arg: "virFree(void *)" frees parameter "groups".
[details]
112 VIR_FREE(groups);
113 return ret;
114 }
...
Then back in main()
338
(24) Event label: Reached label "cleanup"
339 cleanup:
340 virConfFree(conf);
341 virLoginShellFini(conn, dom);
342 virStringFreeList(shargv);
343 VIR_FREE(name);
344 VIR_FREE(homedir);
345 VIR_FREE(seclabel);
346 VIR_FREE(secmodel);
(25) Event double_free: Calling "virFree(void *)" frees pointer
"groups" which has already been freed. [details]
Also see events: [freed_arg]
347 VIR_FREE(groups);
Not quite sure how you want to approach fixing the second error. There are
two VIR_FREE(groups) in virLoginShellAllowedUser() and both would need to
be handled properly. Additionally code in main() seems to perhaps use
groups() after the call - so freeing it could have other consequences too.
John
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
=7zpw
-----END PGP SIGNATURE-----
6:02 a.m.
On Fri, Aug 09, 2013 at 06:51:25AM -0400, John Ferlan wrote:
On 08/02/2013 11:22 AM, Daniel J Walsh wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> THis patch fixes all of Eric's and Daniels comments.
>
> [PATCH] virt-login-shell joins users into lxc container.
Yes, that should have been !*ptr.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.14 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlH7zp8ACgkQrlYvE4MpobNx3gCbBtxw7T4fzIfHSyEEKKyjojXR
> BUUAoOToptiTOi+RC6Bdcp+zvg/xzfRh
> =7zpw
> -----END PGP SIGNATURE-----
>
>
>
The overnight run of Coverity found two issues with this module. I've
cut-n-pasted the relevant portions of the error/traces:
#1: DEADCODE: virLoginShellAllowedUser()
86 if (pp->str[0] == '%') {
(1) Event assignment: Assigning: "ptr" = "pp->str + 1".
Also see events: [notnull][dead_error_condition][dead_error_line]
87 ptr = &pp->str[1];
(2) Event notnull: At condition "ptr", the value of "ptr" cannot be
NULL.
(3) Event dead_error_condition: The condition "!ptr" cannot be true.
Also see events: [assignment][dead_error_line]
88 if (!ptr)
(4) Event dead_error_line: Execution cannot reach this statement "continue;".
Also see events: [assignment][notnull][dead_error_condition]
89 continue;
Did you perhaps mean (!*ptr) (eg, the contents not the value?) #2: USE_AFTER_FREE: main()
252 if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
253 goto cleanup;
254
(18) Event freed_arg: "virLoginShellAllowedUser(virConfPtr, char const *, gid_t
*)" frees "groups". [details]
(19) Event cond_false: Condition "virLoginShellAllowedUser(conf, name, groups) <
0", taking false branch
Also see events: [double_free]
255 if (virLoginShellAllowedUser(conf, name, groups) < 0)
256 goto cleanup;
257
Which is true:
...
61
62 static int virLoginShellAllowedUser(virConfPtr conf,
63 const char *name,
64 gid_t *groups)
65 {
...
(6) Event label: Reached label "cleanup"
110 cleanup:
111 VIR_FREE(gname);
(7) Event freed_arg: "virFree(void *)" frees parameter "groups".
[details]
112 VIR_FREE(groups);
113 return ret;
114 }
...
Then back in main()
338
(24) Event label: Reached label "cleanup"
339 cleanup:
340 virConfFree(conf);
341 virLoginShellFini(conn, dom);
342 virStringFreeList(shargv);
343 VIR_FREE(name);
344 VIR_FREE(homedir);
345 VIR_FREE(seclabel);
346 VIR_FREE(secmodel);
(25) Event double_free: Calling "virFree(void *)" frees pointer
"groups" which has already been freed. [details]
Also see events: [freed_arg]
347 VIR_FREE(groups);
Not quite sure how you want to approach fixing the second error. There are
two VIR_FREE(groups) in virLoginShellAllowedUser() and both would need to
be handled properly. Additionally code in main() seems to perhaps use
groups() after the call - so freeing it could have other consequences too.
The virLoginShellAllowedUser() method has absolutely no business free'ing
the 'groups' parameter it is given. That pointer is owned by the caller
who will free it when needed.
I've sent a patch which should fix both these issues.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:29 a.m.
On 08/09/2013 04:51 AM, John Ferlan wrote:
88 if (!ptr)
(4) Event dead_error_line: Execution cannot reach this statement "continue;".
Also see events: [assignment][notnull][dead_error_condition]
89 continue;
Did you perhaps mean (!*ptr) (eg, the contents not the value?)
Ha - I caught that one earlier in the day by manual code inspection :)
https://www.redhat.com/archives/libvir-list/2013-August/msg00398.html
I also caught other instances of dead code not listed by Coverity,
although with less impact.
#2: USE_AFTER_FREE: main()
But I missed that one, so Coverity is still proving useful.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4123
days inactive
4130
days old
6 comments
4 participants
participants (4)
-
Daniel J Walsh -
Daniel P. Berrange -
Eric Blake -
John Ferlan