[PATCH v2 3/6] virsh: Expose OpenSSH authorized key file mgmt APIs

[Peter Krempa]

On Mon, Nov 16, 2020 at 13:21:00 +0100, Michal Privoznik wrote:
> The new virsh commands are:
> 
>   get-user-sshkeys
>   set-user-sshkeys
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  docs/manpages/virsh.rst |  37 +++++++++
>  tools/virsh-domain.c    | 167 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 204 insertions(+)
> 
> diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
> index bfd26e3120..18cee358fd 100644
> --- a/docs/manpages/virsh.rst
> +++ b/docs/manpages/virsh.rst
> @@ -2636,6 +2636,21 @@ When *--timestamp* is used, a human-readable timestamp will be printed
>  before the event.
>  
>  
> +get-user-sshkeys
> +----------------
> +
> +**Syntax:**
> +
> +::
> +
> +   get-user-sshkeys domain user
> +
> +Print SSH authorized keys for given *user* in the guest *domain*. Please note,
> +that an entry in the file has internal structure as defined by *sshd(8)* and
> +virsh/libvirt does handle keys as opaque strings, i.e. does not interpret
> +them.
> +
> +
>  guest-agent-timeout
>  -------------------
>  
> @@ -4004,6 +4019,28 @@ For QEMU/KVM, this requires the guest agent to be configured
>  and running.
>  
>  
> +set-user-sshkeys
> +----------------
> +
> +**Syntax:**
> +
> +::
> +
> +   set-user-sshkeys domain user [--file FILE] [{--append | --remove}]
> +
> +Replace the contents of *user*'s SSH authorized keys file in the guest *domain*
> +with keys read from optional *FILE*. In the *FILE* keys must be on separate
> +lines and each line must follow authorized keys format as defined by *sshd(8)*.
> +If no *FILE* is specified then the guest authorized keys file is cleared out.

IMO this is a very dangerous mode of operation. The default at least for
virsh should be the same as if --apend was specified when no flag is
used. That way you won't accidentally delete or overwrite keys if you
don't mean to. Any of the more destructive modes should have an explicit
flag.



> +If *--append* is specified, then the guest authorized keys file content is not
> +replaced and new keys from *FILE* are just appended.
> +
> +If *--remove* is specified, then instead of adding any new keys then keys read
> +from *FILE* are removed from the authorized keys file. It is not considered an
> +error if the key does not exist in the file.
> +
> +
>  setmaxmem
>  ---------
>  
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 12b35c037d..2a775fd277 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c

[...]

> +static bool
> +cmdGetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom = NULL;
> +    const char *user;
> +    char **keys = NULL;
> +    int nkeys = 0;
> +    size_t i;
> +    const unsigned int flags = 0;
> +    vshTablePtr table = NULL;
> +    bool ret = false;
> +
> +    if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "user", &user) < 0)
> +        goto cleanup;
> +
> +    nkeys = virDomainAuthorizedSSHKeysGet(dom, user, &keys, flags);
> +    if (nkeys < 0)
> +        goto cleanup;
> +
> +    if (!(table = vshTableNew(_("key"), NULL)))

vshTable seems to be a bit overkill for printing just strings on a line.

> +        goto cleanup;
> +
> +    for (i = 0; i < nkeys; i++) {
> +        if (vshTableRowAppend(table, keys[i], NULL) < 0)
> +            goto cleanup;
> +    }
> +
> +    vshTablePrintToStdout(table, ctl);
> +
> +    ret = true;
> + cleanup:
> +    vshTableFree(table);
> +    if (nkeys > 0)
> +        virStringListFreeCount(keys, nkeys);
> +    virshDomainFree(dom);
> +    return ret;
> +}
> +
> +
> +/*
> + * "set-user-sshkeys" command
> + */
> +static const vshCmdInfo info_set_user_sshkeys[] = {
> +    {.name = "help",
> +     .data = N_("manipulate authorized SSH keys file for given user (via agent)")
> +    },
> +    {.name = "desc",
> +     .data = N_("Append, reset or remove specified key from the authorized "
> +                "keys file for given user")
> +    },
> +    {.name = NULL}
> +};
> +
> +static const vshCmdOptDef opts_set_user_sshkeys[] = {
> +    VIRSH_COMMON_OPT_DOMAIN_FULL(VIR_CONNECT_LIST_DOMAINS_ACTIVE),
> +    {.name = "user",
> +     .type = VSH_OT_DATA,
> +     .flags = VSH_OFLAG_REQ,
> +     .help = N_("user to set authorized keys for"),
> +    },
> +    {.name = "file",
> +     .type = VSH_OT_STRING,
> +     .help = N_("optional file to read keys from"),
> +    },
> +    {.name = "append",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("append keys to the authorized keys file"),
> +    },
> +    {.name = "remove",
> +     .type = VSH_OT_BOOL,
> +     .help = N_("remove keys from the authorized keys file"),
> +    },
> +    {.name = NULL}
> +};
> +
> +static bool
> +cmdSetUserSSHKeys(vshControl *ctl, const vshCmd *cmd)
> +{
> +    virDomainPtr dom = NULL;
> +    const char *user;
> +    const char *from;
> +    g_autofree char *buffer = NULL;
> +    VIR_AUTOSTRINGLIST keys = NULL;
> +    int nkeys = 0;
> +    unsigned int flags = 0;
> +    bool ret = false;
> +
> +    VSH_REQUIRE_OPTION("append", "file");
> +    VSH_REQUIRE_OPTION("remove", "file");
> +
> +    if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> +        return false;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "user", &user) < 0)
> +        goto cleanup;
> +
> +    if (vshCommandOptStringReq(ctl, cmd, "file", &from) < 0)
> +        goto cleanup;
> +
> +    if (vshCommandOptBool(cmd, "append"))
> +        flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
> +    if (vshCommandOptBool(cmd, "remove"))
> +        flags |= VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
> +
> +    if (from) {
> +        if (virFileReadAll(from, VSH_MAX_XML_FILE, &buffer) < 0) {
> +            vshReportError(ctl);

We usually use vshSaveLibvirtHelperError in such situations.


> +            goto cleanup;
> +        }
> +
> +        if (!(keys = virStringSplit(buffer, "\n", -1)))
> +            goto cleanup;
> +
> +        nkeys = virStringListLength((const char **) keys);
> +    }
> +
> +    if (virDomainAuthorizedSSHKeysSet(dom, user,
> +                                      (const char **) keys, nkeys, flags) < 0) {
> +        goto cleanup;
> +    }
> +
> +    ret = true;
> + cleanup:
> +    virshDomainFree(dom);
> +    return ret;
> +}