[libvirt PATCH] qemu: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys

[Michal Privoznik]

On 11/7/20 10:12 AM, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 
> In QEMU 5.2, the guest agent learned to manipulate a user
> ~/.ssh/authorized_keys. Bind the JSON API to libvirt.
> 
> https://wiki.qemu.org/ChangeLog/5.2#Guest_agent
> 
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1888537
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>   src/qemu/qemu_agent.c | 158 ++++++++++++++++++++++++++++++++++++++++++
>   src/qemu/qemu_agent.h |  26 +++++++
>   tests/qemuagenttest.c |  80 +++++++++++++++++++++
>   3 files changed, 264 insertions(+)
> 

While you get bonus points for introducing tests we're still missing 
public APIs. And virsh commands.

> diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
> index 7fbb4a9431..75e7fea9e4 100644
> --- a/src/qemu/qemu_agent.c
> +++ b/src/qemu/qemu_agent.c
> @@ -2496,3 +2496,161 @@ qemuAgentSetResponseTimeout(qemuAgentPtr agent,
>   {
>       agent->timeout = timeout;
>   }
> +
> +void qemuAgentSSHAuthorizedKeyFree(qemuAgentSSHAuthorizedKeyPtr key)
> +{
> +    if (!key)
> +        return;
> +
> +    g_free(key->key);
> +    g_free(key);

I wonder if we need to wrap this string into a struct. At least here on 
internal APIs level looks like we don't - we can change that anytime. 
And the public API - well, I don't think we need to break down the key 
string into its individual members, do we? I mean, "options, keytype, 
base64-encoded key, comment". The public API can accept just a single 
string and let sshd interpret it later.

> +}
> +
> +/* Returns: 0 on success
> + *          -2 when agent command is not supported by the agent and
> + *             'report_unsupported' is false (libvirt error is not reported)
> + *          -1 otherwise (libvirt error is reported)
> + */
> +int qemuAgentSSHGetAuthorizedKeys(qemuAgentPtr agent,
> +                                  const char *user,
> +                                  qemuAgentSSHAuthorizedKeyPtr **keys,
> +                                  bool report_unsupported)

I don't think we need to suppress CommandNotFound type of messages. Some 
wrappers have it because they are called from qemuDomainGetGuestInfo() 
which has a logic where if no specific type was requested then all 
available types are fetched (user list, os info, timezone, hostname, FS 
info).

> +{
> +    g_autoptr(virJSONValue) cmd = NULL;
> +    g_autoptr(virJSONValue) reply = NULL;
> +    virJSONValuePtr data = NULL;
> +    size_t ndata;
> +    size_t i;
> +    int rc;
> +    qemuAgentSSHAuthorizedKeyPtr *keys_ret = NULL;
> +
> +    if (!(cmd = qemuAgentMakeCommand("guest-ssh-get-authorized-keys",
> +                                     "s:username", user,
> +                                      NULL)))
> +        return -1;
> +
> +    if ((rc = qemuAgentCommandFull(agent, cmd, &reply, agent->timeout,
> +                                   report_unsupported)) < 0)
> +        return rc;
> +
> +    if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("qemu agent didn't return an array of keys"));
> +        return -1;
> +    }
> +    ndata = virJSONValueArraySize(data);
> +
> +    keys_ret = g_new0(qemuAgentSSHAuthorizedKeyPtr, ndata);
> +
> +    for (i = 0; i < ndata; i++) {
> +        virJSONValuePtr entry = virJSONValueArrayGet(data, i);
> +
> +        if (!entry) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("array element missing in guest-ssh-get-authorized-keys return "
> +                             "value"));
> +            goto cleanup;
> +        }
> +
> +        keys_ret[i] = g_new0(qemuAgentSSHAuthorizedKey, 1);
> +        keys_ret[i]->key = g_strdup(virJSONValueGetString(entry));
> +    }
> +
> +    *keys = g_steal_pointer(&keys_ret);
> +    return ndata;
> +
> + cleanup:

Technically, this should be 'error' because it's used only in case of 
failure ;-)

> +    if (keys_ret) {
> +        for (i = 0; i < ndata; i++)
> +            qemuAgentSSHAuthorizedKeyFree(keys_ret[i]);
> +        g_free(keys_ret);
> +    }
> +    return -1;
> +}
> +
> +static virJSONValuePtr
> +makeJSONArrayFromKeys(qemuAgentSSHAuthorizedKeyPtr *keys,
> +                      size_t nkeys)

If we'd go with plain strings then we could use 
qemuAgentMakeStringsArray() instead.

> +{
> +    g_autoptr(virJSONValue) jkeys = NULL;
> +    size_t i;
> +
> +    jkeys = virJSONValueNewArray();
> +
> +    for (i = 0; i < nkeys; i++) {
> +        qemuAgentSSHAuthorizedKeyPtr k = keys[i];
> +
> +        if (virJSONValueArrayAppendString(jkeys, k->key) < 0)
> +            return NULL;
> +    }
> +
> +    return g_steal_pointer(&jkeys);
> +}

I'm stopping my review here. The wrappers are okay, but we really need 
the public API and RPC first. I can work on that if you don't feel like it.

Michal