Re: [libvirt PATCH] qemu: add qemuAgentSSH{Add, Remove, Get}AuthorizedKeys
Hi On Mon, Nov 9, 2020 at 4:44 PM Michal Privoznik wrote: > > 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. I am on holiday this week and perhaps a few more days. If you have some free time, feel free to update the patch. Otherwise, it will wait a bit longer. thanks
Re: [libvirt PATCH] qemu: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
On 11/7/20 10:12 AM, marcandre.lur...@redhat.com wrote:
From: Marc-André Lureau
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
---
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, , 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(_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();
+}
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
[libvirt PATCH] qemu: add qemuAgentSSH{Add,Remove,Get}AuthorizedKeys
From: Marc-André Lureau
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
---
src/qemu/qemu_agent.c | 158 ++
src/qemu/qemu_agent.h | 26 +++
tests/qemuagenttest.c | 80 +
3 files changed, 264 insertions(+)
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);
+}
+
+/* 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)
+{
+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, , 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(_ret);
+return ndata;
+
+ cleanup:
+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)
+{
+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();
+}
+
+/* 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 qemuAgentSSHAddAuthorizedKeys(qemuAgentPtr agent,
+ const char *user,
+ qemuAgentSSHAuthorizedKeyPtr *keys,
+ size_t nkeys,
+ bool reset,
+ bool report_unsupported)
+{
+g_autoptr(virJSONValue) cmd = NULL;
+g_autoptr(virJSONValue) reply = NULL;
+g_autoptr(virJSONValue) jkeys = NULL;
+int rc;
+
+jkeys = makeJSONArrayFromKeys(keys, nkeys);
+if (jkeys == NULL)
+return -1;
+
+if (!(cmd = qemuAgentMakeCommand("guest-ssh-add-authorized-keys",
+ "s:username", user,
+ "a:keys", ,
+ "b:reset", reset,
+ NULL)))
+return -1;
+
+if ((rc = qemuAgentCommandFull(agent, cmd, , agent->timeout,
+ report_unsupported)) < 0)
+return rc;
+
+return 0;
+}
+
+/* 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
