Re: [Qemu-devel] [PATCH v2] qga: add guest-set-admin-password command

[Daniel P. Berrange]

On Wed, Feb 04, 2015 at 04:25:47PM +0300, Olga Krishtal wrote:
> On 12/01/15 18:58, Daniel P. Berrange wrote:
> >Add a new 'guest-set-admin-password' command for changing the
> >root/administrator password. This command is needed to allow
> >OpenStack to support its API for changing the admin password
> >on a running guest.
> >
> >Accepts either the raw password string:
> >
> >$ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >    '{ "execute": "guest-set-admin-password", "arguments":
> >      { "crypted": false, "password": "12345678" } }'
> >   {"return":{}}
> >
> >Or a pre-encrypted string (recommended)
> >
> >$ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >    '{ "execute": "guest-set-admin-password", "arguments":
> >      { "crypted": true, "password":
> >         "$6$T9O/j/aGPrE...snip....rQoRN4F0.GG0MPjNUNyml." } }'
> >
> >NB windows support is desirable, but not implemented in this
> >patch.
> >
> >Signed-off-by: Daniel P. Berrange <address@hidden>
> >---
> >  qga/commands-posix.c | 90 
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qga/commands-win32.c |  6 ++++
> >  qga/qapi-schema.json | 19 +++++++++++
> >  3 files changed, 115 insertions(+)
> >
> >diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> >index f6f3e3c..4887889 100644
> >--- a/qga/commands-posix.c
> >+++ b/qga/commands-posix.c
> >@@ -1875,6 +1875,90 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList 
> >*vcpus, Error **errp)
> >      return processed;
> >  }
> >+void qmp_guest_set_admin_password(bool crypted, const char *password,
> >+                                  Error **errp)
> >+{
> >+    Error *local_err = NULL;
> >+    char *passwd_path = NULL;
> >+    pid_t pid;
> >+    int status;
> >+    int datafd[2] = { -1, -1 };
> >+    char *acctpw = g_strdup_printf("root:%s\n", password);
> >+    size_t acctpwlen = strlen(acctpw);
> >+
> >+    if (strchr(password, '\n')) {
> >+        error_setg(errp, "forbidden characters in new password");
> >+        goto out;
> >+    }
> >+
> >+    passwd_path = g_find_program_in_path("chpasswd");
> >+
> >+    if (!passwd_path) {
> >+        error_setg(errp, "cannot find 'passwd' program in PATH");
> >+        goto out;
> >+    }
> >+
> >+    if (pipe(datafd) < 0) {
> >+        error_setg(errp, "cannot create pipe FDs");
> >+        goto out;
> >+    }
> >+
> >+    pid = fork();
> >+    if (pid == 0) {
> >+        close(datafd[1]);
> >+        /* child */
> >+        setsid();
> >+        dup2(datafd[0], 0);
> >+        reopen_fd_to_null(1);
> >+        reopen_fd_to_null(2);
> >+
> >+        if (crypted) {
> >+            execle(passwd_path, "chpasswd", "-e", NULL, environ);
> >+        } else {
> >+            execle(passwd_path, "chpasswd", NULL, environ);
> >+        }
> >+        _exit(EXIT_FAILURE);
> >+    } else if (pid < 0) {
> >+        error_setg_errno(errp, errno, "failed to create child process");
> >+        goto out;
> >+    }
> >+    close(datafd[0]);
> >+    datafd[0] = -1;
> >+
> >+    if (qemu_write_full(datafd[1], acctpw, acctpwlen) != acctpwlen) {
> >+        error_setg_errno(errp, errno, "cannot write new account password");
> >+        goto out;
> >+    }
> >+    close(datafd[1]);
> >+    datafd[1] = -1;
> >+
> >+    ga_wait_child(pid, &status, &local_err);
> >+    if (local_err) {
> >+        error_propagate(errp, local_err);
> >+        goto out;
> >+    }
> >+
> >+    if (!WIFEXITED(status)) {
> >+        error_setg(errp, "child process has terminated abnormally");
> >+        goto out;
> >+    }
> >+
> >+    if (WEXITSTATUS(status)) {
> >+        error_setg(errp, "child process has failed to set admin password");
> >+        goto out;
> >+    }
> >+
> >+out:
> >+    g_free(acctpw);
> >+    g_free(passwd_path);
> >+    if (datafd[0] != -1) {
> >+        close(datafd[0]);
> >+    }
> >+    if (datafd[1] != -1) {
> >+        close(datafd[1]);
> >+    }
> >+}
> >+
> >  #else /* defined(__linux__) */
> >  void qmp_guest_suspend_disk(Error **errp)
> >@@ -1910,6 +1994,12 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList 
> >*vcpus, Error **errp)
> >      return -1;
> >  }
> >+void qmp_guest_set_admin_password(bool crypted, const char *password,
> >+                                  Error **errp)
> >+{
> >+    error_set(errp, QERR_UNSUPPORTED);
> >+}
> >+
> >  #endif
> >  #if !defined(CONFIG_FSFREEZE)
> >diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> >index 3bcbeae..56854d5 100644
> >--- a/qga/commands-win32.c
> >+++ b/qga/commands-win32.c
> >@@ -446,6 +446,12 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList 
> >*vcpus, Error **errp)
> >      return -1;
> >  }
> >+void qmp_guest_set_admin_password(bool crypted, const char *password,
> >+                                  Error **errp)
> >+{
> >+    error_set(errp, QERR_UNSUPPORTED);
> >+}
> >+
> >  /* add unsupported commands to the blacklist */
> >  GList *ga_command_blacklist_init(GList *blacklist)
> >  {
> >diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> >index 376e79f..25118e2 100644
> >--- a/qga/qapi-schema.json
> >+++ b/qga/qapi-schema.json
> >@@ -738,3 +738,22 @@
> >  ##
> >  { 'command': 'guest-get-fsinfo',
> >    'returns': ['GuestFilesystemInfo'] }
> >+
> >+##
> >+# @guest-set-admin-password
> >+#
> >+# @crypted: true if password is already crypt()d, false if raw
> >+# @password: the new password entry
> >+#
> >+# If the @crypted flag is true, it is the callers responsibility
> >+# to ensure the correct crypt() encryption scheme is used. This
> >+# command does not attempt to interpret or report on the encryption
> >+# scheme. Refer to the documentation of the guest operating system
> >+# in question to determine what is supported.
> >+#
> >+# Returns: Nothing on success.
> >+#
> >+# Since 2.3
> >+##
> >+{ 'command': 'guest-set-admin-password',
> >+  'data': { 'crypted': 'bool', 'password': 'str' } }
> While implementing such functionality for Windows NT we can suffer from
> particular problem:
> -The password must be passed to WinApi function as a plain text, so we would
> need entire the encryption mechanism if we used ctypted: true

In that scenario, I'd suggest the windows impl of the command simply report
an error if the flag was crypted==true. QEMU shouldn't try to interpret the
data in any way IMHO, so that'd rule out decryption of any kind. In any
case, 'crypt' as a concept is all about doing a one-way hash so you can't
decrypt regardless.

Regards,
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 :|