[libvirt] [PATCH 11/17] virt-login-shell: honour the -c option to launch commands

[John Ferlan]

On 04/14/2016 11:22 AM, Daniel P. Berrange wrote:
> The virt-login-shell program is supposed to look like a
> regular shell to clients. Login services like sshd
> expect the shell to accept a '-c cmdstring' argument to
> specify a command to launch instead of presenting an
> interactive prompt.
> 
> We can implement this by simply passing the '-c cmdstring'
> data straight through to the real shell we use. This does
> not open any security holes, since the command is not run
> until we're inside the container namespaces. This allows
> scp to work for users with virt-login-shell.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  tools/virt-login-shell.c | 70 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
> index ec759dc..34a8836 100644
> --- a/tools/virt-login-shell.c
> +++ b/tools/virt-login-shell.c
> @@ -100,20 +100,19 @@ static int virLoginShellAllowedUser(virConfPtr conf,
>      return ret;
>  }
>  
> -static char **virLoginShellGetShellArgv(virConfPtr conf)
> +static int virLoginShellGetShellArgv(virConfPtr conf,
> +                                     char ***retshargv,
> +                                     size_t *retshargvlen)
>  {
>      size_t i;
> +    size_t len;
>      char **shargv = NULL;
> -    virConfValuePtr p;
> +    virConfValuePtr p, pp;
>  
>      p = virConfGetValue(conf, "shell");
> -    if (!p)
> -        return virStringSplit("/bin/sh -l", " ", 3);
> -
> -    if (p->type == VIR_CONF_LIST) {
> -        size_t len;
> -        virConfValuePtr pp;
> -
> +    if (!p) {
> +        len = 2; /* /bin/sh -l */
> +    } else if (p->type == VIR_CONF_LIST) {
>          /* Calc length and check items */
>          for (len = 0, pp = p->list; pp; len++, pp = pp->next) {
>              if (pp->type != VIR_CONF_STRING) {
> @@ -122,18 +121,41 @@ static char **virLoginShellGetShellArgv(virConfPtr conf)
>                  goto error;
>              }
>          }
> +    } else {
> +        virReportSystemError(EINVAL, "%s",
> +                             _("shell must be a list of strings"));
> +        goto error;
> +    }
>  
> -        if (VIR_ALLOC_N(shargv, len + 1) < 0)
> +    len++; /* NULL terminator */
> +
> +    if (VIR_ALLOC_N(shargv, len) < 0)
> +        goto error;
> +
> +    i = 0;
> +    if (!p) {
> +        if (VIR_STRDUP(shargv[i++], "/bin/sh") < 0)
>              goto error;
> -        for (i = 0, pp = p->list; pp; i++, pp = pp->next) {
> -            if (VIR_STRDUP(shargv[i], pp->str) < 0)
> +        if (VIR_STRDUP(shargv[i++], "-l") < 0)
> +            goto error;
> +    } else if (p->type == VIR_CONF_LIST) {
> +        for (pp = p->list; pp; pp = pp->next) {
> +            if (VIR_STRDUP(shargv[i++], pp->str) < 0)
>                  goto error;
>          }
>      }
> -    return shargv;
> +
> +    shargv[i] = NULL;
> +
> +    *retshargvlen = i;
> +    *retshargv = shargv;
> +
> +    return 0;
>   error:
> +    *retshargv = NULL;
> +    *retshargvlen = 0;
>      virStringFreeList(shargv);
> -    return NULL;
> +    return -1;
>  }
>  
>  static char *progname;

Right in here somewhere there's some 'usage()' that should probably be
updated.

> @@ -177,6 +199,7 @@ main(int argc, char **argv)
>      gid_t gid = getgid();
>      char *name = NULL;
>      char **shargv = NULL;
> +    size_t shargvlen = 0;
>      virSecurityModelPtr secmodel = NULL;
>      virSecurityLabelPtr seclabel = NULL;
>      virDomainPtr dom = NULL;
> @@ -190,6 +213,7 @@ main(int argc, char **argv)
>      int *fdlist = NULL;
>      int openmax;
>      size_t i;
> +    const char *cmdstr = NULL;
>  
>      struct option opt[] = {
>          {"help", no_argument, NULL, 'h'},

Should this add a "-c" w/ required_argument?

I also see the "V" lists "optional_argument", but that's not part of the
"hV" below, so shouldn't it be no_argument?

ACK with the changes -

John

> @@ -220,7 +244,7 @@ main(int argc, char **argv)
>          return ret;
>      }
>  
> -    while ((arg = getopt_long(argc, argv, "hV", opt, &longindex)) != -1) {
> +    while ((arg = getopt_long(argc, argv, "hVc:", opt, &longindex)) != -1) {
>          switch (arg) {
>          case 'h':
>              usage();
> @@ -230,6 +254,10 @@ main(int argc, char **argv)
>              show_version();
>              exit(EXIT_SUCCESS);
>  
> +        case 'c':
> +            cmdstr = optarg;
> +            break;
> +
>          case '?':
>          default:
>              usage();
> @@ -265,7 +293,7 @@ main(int argc, char **argv)
>      if (virLoginShellAllowedUser(conf, name, groups) < 0)
>          goto cleanup;
>  
> -    if (!(shargv = virLoginShellGetShellArgv(conf)))
> +    if (virLoginShellGetShellArgv(conf, &shargv, &shargvlen) < 0)
>          goto cleanup;
>  
>      conn = virConnectOpen("lxc:///");
> @@ -318,6 +346,16 @@ main(int argc, char **argv)
>          goto cleanup;
>      }
>  
> +    if (cmdstr) {
> +        if (VIR_REALLOC_N(shargv, shargvlen + 3) < 0)
> +            goto cleanup;
> +        if (VIR_STRDUP(shargv[shargvlen++], "-c") < 0)
> +            goto cleanup;
> +        if (VIR_STRDUP(shargv[shargvlen++], cmdstr) < 0)
> +            goto cleanup;
> +        shargv[shargvlen] = NULL;
> +    }
> +
>      /* A fork is required to create new process in correct pid namespace.  */
>      if ((cpid = virFork()) < 0)
>          goto cleanup;
>