[v3] qga: add guest-set-user-password command

Add a new 'guest-set-user-password' command for changing the password
of guest OS user accounts. This command is needed to enable OpenStack
to support its API for changing the admin password of guests running
on KVM/QEMU. It is not practical to provide a command at the QEMU
level explicitly targetting administrator account password change
only, since different guest OS have different names for the admin
account. While UNIX systems use 'root', Windows systems typically
use 'Administrator' and even that can be renamed. Higher level apps
like OpenStack have the ability to figure out the correct admin
account name since they have info that QEMU/libvirt do not.

The command accepts either the clear text password string, encoded
in base64 to make it 8-bit safe in JSON:

$ echo -n "123456" | base64
MTIzNDU2
$ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
   '{ "execute": "guest-set-user-password",
      "arguments": { "crypted": false,
                     "username": "root",
                     "password": "MTIzNDU2" } }'
  {"return":{}}

Or a password that has already been run though a crypt(3) like
algorithm appropriate for the guest, again then base64 encoded:

$ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
JDYkb...snip...YT2Ey
$ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
   '{ "execute": "guest-set-user-password",
      "arguments": { "crypted": true,
                     "username": "root",
                     "password": "JDYkb...snip...YT2Ey" } }'

NB windows support is desirable, but not implemented in this
patch.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qga/commands-posix.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qga/commands-win32.c |   9 +++++
 qga/qapi-schema.json |  27 +++++++++++++
 3 files changed, 146 insertions(+)

In v3:

 - Renamed from guest-set-admin-password to guest-set-user-password
 - Added 'username' argument
 - Require 'password' to be base64 encoded

On Wed, Feb 11, 2015 at 11:26:12AM +0000, Daniel P. Berrange wrote:
> Add a new 'guest-set-user-password' command for changing the password
> of guest OS user accounts. This command is needed to enable OpenStack
> to support its API for changing the admin password of guests running
> on KVM/QEMU. It is not practical to provide a command at the QEMU
> level explicitly targetting administrator account password change
> only, since different guest OS have different names for the admin
> account. While UNIX systems use 'root', Windows systems typically
> use 'Administrator' and even that can be renamed. Higher level apps
> like OpenStack have the ability to figure out the correct admin
> account name since they have info that QEMU/libvirt do not.
> 
> The command accepts either the clear text password string, encoded
> in base64 to make it 8-bit safe in JSON:
> 
> $ echo -n "123456" | base64
> MTIzNDU2
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>    '{ "execute": "guest-set-user-password",
>       "arguments": { "crypted": false,
>                      "username": "root",
>                      "password": "MTIzNDU2" } }'
>   {"return":{}}
> 
> Or a password that has already been run though a crypt(3) like
> algorithm appropriate for the guest, again then base64 encoded:
> 
> $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> JDYkb...snip...YT2Ey
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>    '{ "execute": "guest-set-user-password",
>       "arguments": { "crypted": true,
>                      "username": "root",
>                      "password": "JDYkb...snip...YT2Ey" } }'
> 

So how would it look like for user "Daniel P. Berrangé" (with accent
aigu :), or "Роман Каган" (my name in cyrillic letters)?  What I'm
trying to say is that if we assume localized usernames we may have
trouble with character encoding.

> +    passwd_path = g_find_program_in_path("chpasswd");
> +
> +    if (!passwd_path) {
> +        error_setg(errp, "cannot find 'passwd' program in PATH");

Minor nitpick: s/passwd/chpasswd/

The patch looks technically correct; however I'm not sold on the
approach, which assumes a responsibility split between the upper
management layer, which is supposed to know the guest OS, the username,
the encryption scheme, and the guest agent which takes care of the
os-specific command to actually change the password.  I still tend to
like more the scheme with the management layer having all the necessary
information, including the command to change the password and the proper
way to call it, and using guest-exec to perform it.

IMO the question is how low the bar is to extend the qga protocol with
yet another general-purpose (i.e. not virtual machine-specific) OS
management task.  I'd rather see it out of scope for qga.  Instead, such
an upper management layer, if necessary, would bring its own agent, with
qga acting as a transport.  This way e.g. OpenStack would be able to
uniformly change admin passwords also in ESXi, Parallels Server, LXC,
OpenVz, physical servers, etc.

Roman.

On Thu, Feb 12, 2015 at 04:21:09PM +0300, Roman Kagan wrote:
> On Wed, Feb 11, 2015 at 11:26:12AM +0000, Daniel P. Berrange wrote:
> > Add a new 'guest-set-user-password' command for changing the password
> > of guest OS user accounts. This command is needed to enable OpenStack
> > to support its API for changing the admin password of guests running
> > on KVM/QEMU. It is not practical to provide a command at the QEMU
> > level explicitly targetting administrator account password change
> > only, since different guest OS have different names for the admin
> > account. While UNIX systems use 'root', Windows systems typically
> > use 'Administrator' and even that can be renamed. Higher level apps
> > like OpenStack have the ability to figure out the correct admin
> > account name since they have info that QEMU/libvirt do not.
> > 
> > The command accepts either the clear text password string, encoded
> > in base64 to make it 8-bit safe in JSON:
> > 
> > $ echo -n "123456" | base64
> > MTIzNDU2
> > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >    '{ "execute": "guest-set-user-password",
> >       "arguments": { "crypted": false,
> >                      "username": "root",
> >                      "password": "MTIzNDU2" } }'
> >   {"return":{}}
> > 
> > Or a password that has already been run though a crypt(3) like
> > algorithm appropriate for the guest, again then base64 encoded:
> > 
> > $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> > JDYkb...snip...YT2Ey
> > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >    '{ "execute": "guest-set-user-password",
> >       "arguments": { "crypted": true,
> >                      "username": "root",
> >                      "password": "JDYkb...snip...YT2Ey" } }'
> > 
> 
> So how would it look like for user "Daniel P. Berrangé" (with accent
> aigu :), or "Роман Каган" (my name in cyrillic letters)?  What I'm
> trying to say is that if we assume localized usernames we may have
> trouble with character encoding.

The JSON string typed accepts UTF-8 encoding strings, so you can
have any of those localized names.  If the API/command that the
agent invokes doesn't use UTF-8 (unlikely) then the agent would
do a charset conversion as needed.

> > +    passwd_path = g_find_program_in_path("chpasswd");
> > +
> > +    if (!passwd_path) {
> > +        error_setg(errp, "cannot find 'passwd' program in PATH");
> 
> Minor nitpick: s/passwd/chpasswd/
> 
> The patch looks technically correct; however I'm not sold on the
> approach, which assumes a responsibility split between the upper
> management layer, which is supposed to know the guest OS, the username,
> the encryption scheme, and the guest agent which takes care of the
> os-specific command to actually change the password.  I still tend to
> like more the scheme with the management layer having all the necessary
> information, including the command to change the password and the proper
> way to call it, and using guest-exec to perform it.

guest-exec is not something that is generally supportable. By virtue
of the fact that it is designed to allow arbitrary command execution,
you can't provide security policy out of the box that confines its
in any meaningfully secure way. By having a dedicated qapi comand for
this, we can write security policy that allows precisely the command
needed, no more, no less. This commands provides a simple mechanism
for the feature, while the user or application can define its policy
for using it.

> IMO the question is how low the bar is to extend the qga protocol with
> yet another general-purpose (i.e. not virtual machine-specific) OS
> management task.  I'd rather see it out of scope for qga.  Instead, such
> an upper management layer, if necessary, would bring its own agent, with
> qga acting as a transport.  This way e.g. OpenStack would be able to
> uniformly change admin passwords also in ESXi, Parallels Server, LXC,
> OpenVz, physical servers, etc.

This proposed feature is a generally useful for regaining access to a
guest for which you've lost login access. Putting it in an application
specific agent results in every app managing QEMU reinventing the
wheel which is just madness. We've already got QEMU guest agent and
SPICE guest agent, adding a 3rd openstack agent is not a desirable
path to take. For the cross hypervisor portability problem we already
have libvirt, so again pushing it up into openstack just makes life
worse for non-openstack apps which want this.

Regards,
Daniel

On 11/02/15 14:26, Daniel P. Berrange wrote:
> Add a new 'guest-set-user-password' command for changing the password
> of guest OS user accounts. This command is needed to enable OpenStack
> to support its API for changing the admin password of guests running
> on KVM/QEMU. It is not practical to provide a command at the QEMU
> level explicitly targetting administrator account password change
> only, since different guest OS have different names for the admin
> account. While UNIX systems use 'root', Windows systems typically
> use 'Administrator' and even that can be renamed. Higher level apps
> like OpenStack have the ability to figure out the correct admin
> account name since they have info that QEMU/libvirt do not.
>
> The command accepts either the clear text password string, encoded
> in base64 to make it 8-bit safe in JSON:
>
> $ echo -n "123456" | base64
> MTIzNDU2
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>     '{ "execute": "guest-set-user-password",
>        "arguments": { "crypted": false,
>                       "username": "root",
>                       "password": "MTIzNDU2" } }'
>    {"return":{}}
>
> Or a password that has already been run though a crypt(3) like
> algorithm appropriate for the guest, again then base64 encoded:
>
> $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> JDYkb...snip...YT2Ey
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>     '{ "execute": "guest-set-user-password",
>        "arguments": { "crypted": true,
>                       "username": "root",
>                       "password": "JDYkb...snip...YT2Ey" } }'
>
> NB windows support is desirable, but not implemented in this
> patch.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>   qga/commands-posix.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   qga/commands-win32.c |   9 +++++
>   qga/qapi-schema.json |  27 +++++++++++++
>   3 files changed, 146 insertions(+)
>
> In v3:
>
>   - Renamed from guest-set-admin-password to guest-set-user-password
>   - Added 'username' argument
>   - Require 'password' to be base64 encoded
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f6f3e3c..57409d0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1875,6 +1875,108 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>       return processed;
>   }
>   
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    Error *local_err = NULL;
> +    char *passwd_path = NULL;
> +    pid_t pid;
> +    int status;
> +    int datafd[2] = { -1, -1 };
> +    char *rawpasswddata = NULL;
> +    size_t rawpasswdlen;
> +    char *chpasswddata = NULL;
> +    size_t chpasswdlen;
> +
> +    rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
> +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> +    rawpasswddata[rawpasswdlen] = '\0';
> +
> +    if (strchr(rawpasswddata, '\n')) {
> +        error_setg(errp, "forbidden characters in raw password");
> +        goto out;
> +    }
> +
> +    if (strchr(username, '\n') ||
> +        strchr(username, ':')) {
> +        error_setg(errp, "forbidden characters in username");
> +        goto out;
> +    }
> +
> +    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> +    chpasswdlen = strlen(chpasswddata);
> +
> +    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], chpasswddata, chpasswdlen) != chpasswdlen) {
> +        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 user password");
> +        goto out;
> +    }
> +
> +out:
> +    g_free(chpasswddata);
> +    g_free(rawpasswddata);
> +    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 +2012,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>       return -1;
>   }
>   
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 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..11876f6 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -446,6 +446,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>       return -1;
>   }
>   
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +}
> +
If we have decided that there is no crypted password in Windows, may be 
we should cross out this argument (with #define) or just take crypted: 
false as default.
>   /* add unsupported commands to the blacklist */
>   GList *ga_command_blacklist_init(GList *blacklist)
>   {
> @@ -454,6 +462,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
>           "guest-file-write", "guest-file-seek", "guest-file-flush",
>           "guest-suspend-hybrid", "guest-network-get-interfaces",
>           "guest-get-vcpus", "guest-set-vcpus",
> +        "guest-set-user-password",
>           "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
>           "guest-fstrim", NULL};
>       char **p = (char **)list_unsupported;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 376e79f..5117b65 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -738,3 +738,30 @@
>   ##
>   { 'command': 'guest-get-fsinfo',
>     'returns': ['GuestFilesystemInfo'] }
> +
> +##
> +# @guest-set-user-password
> +#
> +# @username: the user account whose password to change
> +# @password: the new password entry string, base64 encoded
> +# @crypted: true if password is already crypt()d, false if raw
> +#
> +# If the @crypted flag is true, it is the caller's 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.
> +#
> +# Note all guest operating systems will support use of the
> +# @crypted flag, as they may require the clear-text password
> +#
> +# The @password parameter must always be base64 encoded before
> +# transmission, even if already crypt()d, to ensure it is 8-bit
> +# safe when passed as JSON.
> +#
> +# Returns: Nothing on success.
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-set-user-password',
> +  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
Everything seems fine.

Quoting Daniel P. Berrange (2015-02-12 08:05:56)
> On Thu, Feb 12, 2015 at 04:21:09PM +0300, Roman Kagan wrote:
> > On Wed, Feb 11, 2015 at 11:26:12AM +0000, Daniel P. Berrange wrote:
> > > Add a new 'guest-set-user-password' command for changing the password
> > > of guest OS user accounts. This command is needed to enable OpenStack
> > > to support its API for changing the admin password of guests running
> > > on KVM/QEMU. It is not practical to provide a command at the QEMU
> > > level explicitly targetting administrator account password change
> > > only, since different guest OS have different names for the admin
> > > account. While UNIX systems use 'root', Windows systems typically
> > > use 'Administrator' and even that can be renamed. Higher level apps
> > > like OpenStack have the ability to figure out the correct admin
> > > account name since they have info that QEMU/libvirt do not.
> > > 
> > > The command accepts either the clear text password string, encoded
> > > in base64 to make it 8-bit safe in JSON:
> > > 
> > > $ echo -n "123456" | base64
> > > MTIzNDU2
> > > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> > >    '{ "execute": "guest-set-user-password",
> > >       "arguments": { "crypted": false,
> > >                      "username": "root",
> > >                      "password": "MTIzNDU2" } }'
> > >   {"return":{}}
> > > 
> > > Or a password that has already been run though a crypt(3) like
> > > algorithm appropriate for the guest, again then base64 encoded:
> > > 
> > > $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> > > JDYkb...snip...YT2Ey
> > > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> > >    '{ "execute": "guest-set-user-password",
> > >       "arguments": { "crypted": true,
> > >                      "username": "root",
> > >                      "password": "JDYkb...snip...YT2Ey" } }'
> > > 
> > 
> > So how would it look like for user "Daniel P. Berrangé" (with accent
> > aigu :), or "Роман Каган" (my name in cyrillic letters)?  What I'm
> > trying to say is that if we assume localized usernames we may have
> > trouble with character encoding.
> 
> The JSON string typed accepts UTF-8 encoding strings, so you can
> have any of those localized names.  If the API/command that the
> agent invokes doesn't use UTF-8 (unlikely) then the agent would
> do a charset conversion as needed.
> 
> > > +    passwd_path = g_find_program_in_path("chpasswd");
> > > +
> > > +    if (!passwd_path) {
> > > +        error_setg(errp, "cannot find 'passwd' program in PATH");
> > 
> > Minor nitpick: s/passwd/chpasswd/
> > 
> > The patch looks technically correct; however I'm not sold on the
> > approach, which assumes a responsibility split between the upper
> > management layer, which is supposed to know the guest OS, the username,
> > the encryption scheme, and the guest agent which takes care of the
> > os-specific command to actually change the password.  I still tend to
> > like more the scheme with the management layer having all the necessary
> > information, including the command to change the password and the proper
> > way to call it, and using guest-exec to perform it.
> 
> guest-exec is not something that is generally supportable. By virtue
> of the fact that it is designed to allow arbitrary command execution,
> you can't provide security policy out of the box that confines its
> in any meaningfully secure way. By having a dedicated qapi comand for
> this, we can write security policy that allows precisely the command
> needed, no more, no less. This commands provides a simple mechanism
> for the feature, while the user or application can define its policy
> for using it.
> 
> > IMO the question is how low the bar is to extend the qga protocol with
> > yet another general-purpose (i.e. not virtual machine-specific) OS
> > management task.  I'd rather see it out of scope for qga.  Instead, such
> > an upper management layer, if necessary, would bring its own agent, with
> > qga acting as a transport.  This way e.g. OpenStack would be able to
> > uniformly change admin passwords also in ESXi, Parallels Server, LXC,
> > OpenVz, physical servers, etc.
> 
> This proposed feature is a generally useful for regaining access to a
> guest for which you've lost login access. Putting it in an application
> specific agent results in every app managing QEMU reinventing the
> wheel which is just madness. We've already got QEMU guest agent and
> SPICE guest agent, adding a 3rd openstack agent is not a desirable
> path to take. For the cross hypervisor portability problem we already
> have libvirt, so again pushing it up into openstack just makes life
> worse for non-openstack apps which want this.

I agree. It's true that we don't want QGA to become too closely
tied/integrated with high-level management tasks (since they tend to be
very environment/implementation specific), but if a feature proves
to be generally useful across the board then a good argument can be made
for pushing it down into a lower-level agent like QGA that a simpler
"out of the box" solution can potentially benefit from as well: e.g.
node-level management like libvirt, kimchi, virt-manager, etc.

The interesting stuff like SSO, package-management, etc, is what should be
left to a more integrated agent (or the general-purpose/transport aspects
of QGA like guest-file-*/guest-exec*).

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

Quoting Olga Krishtal (2015-02-13 06:26:09)
> On 11/02/15 14:26, Daniel P. Berrange wrote:
> > Add a new 'guest-set-user-password' command for changing the password
> > of guest OS user accounts. This command is needed to enable OpenStack
> > to support its API for changing the admin password of guests running
> > on KVM/QEMU. It is not practical to provide a command at the QEMU
> > level explicitly targetting administrator account password change
> > only, since different guest OS have different names for the admin
> > account. While UNIX systems use 'root', Windows systems typically
> > use 'Administrator' and even that can be renamed. Higher level apps
> > like OpenStack have the ability to figure out the correct admin
> > account name since they have info that QEMU/libvirt do not.
> >
> > The command accepts either the clear text password string, encoded
> > in base64 to make it 8-bit safe in JSON:
> >
> > $ echo -n "123456" | base64
> > MTIzNDU2
> > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >     '{ "execute": "guest-set-user-password",
> >        "arguments": { "crypted": false,
> >                       "username": "root",
> >                       "password": "MTIzNDU2" } }'
> >    {"return":{}}
> >
> > Or a password that has already been run though a crypt(3) like
> > algorithm appropriate for the guest, again then base64 encoded:
> >
> > $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> > JDYkb...snip...YT2Ey
> > $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
> >     '{ "execute": "guest-set-user-password",
> >        "arguments": { "crypted": true,
> >                       "username": "root",
> >                       "password": "JDYkb...snip...YT2Ey" } }'
> >
> > NB windows support is desirable, but not implemented in this
> > patch.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >   qga/commands-posix.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   qga/commands-win32.c |   9 +++++
> >   qga/qapi-schema.json |  27 +++++++++++++
> >   3 files changed, 146 insertions(+)
> >
> > In v3:
> >
> >   - Renamed from guest-set-admin-password to guest-set-user-password
> >   - Added 'username' argument
> >   - Require 'password' to be base64 encoded
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index f6f3e3c..57409d0 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1875,6 +1875,108 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> >       return processed;
> >   }
> >   
> > +void qmp_guest_set_user_password(const char *username,
> > +                                 const char *password,
> > +                                 bool crypted,
> > +                                 Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    char *passwd_path = NULL;
> > +    pid_t pid;
> > +    int status;
> > +    int datafd[2] = { -1, -1 };
> > +    char *rawpasswddata = NULL;
> > +    size_t rawpasswdlen;
> > +    char *chpasswddata = NULL;
> > +    size_t chpasswdlen;
> > +
> > +    rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
> > +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> > +    rawpasswddata[rawpasswdlen] = '\0';
> > +
> > +    if (strchr(rawpasswddata, '\n')) {
> > +        error_setg(errp, "forbidden characters in raw password");
> > +        goto out;
> > +    }
> > +
> > +    if (strchr(username, '\n') ||
> > +        strchr(username, ':')) {
> > +        error_setg(errp, "forbidden characters in username");
> > +        goto out;
> > +    }
> > +
> > +    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> > +    chpasswdlen = strlen(chpasswddata);
> > +
> > +    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], chpasswddata, chpasswdlen) != chpasswdlen) {
> > +        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 user password");
> > +        goto out;
> > +    }
> > +
> > +out:
> > +    g_free(chpasswddata);
> > +    g_free(rawpasswddata);
> > +    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 +2012,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> >       return -1;
> >   }
> >   
> > +void qmp_guest_set_user_password(const char *username,
> > +                                 const char *password,
> > +                                 bool crypted,
> > +                                 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..11876f6 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> > @@ -446,6 +446,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> >       return -1;
> >   }
> >   
> > +void qmp_guest_set_user_password(const char *username,
> > +                                 const char *password,
> > +                                 bool crypted,
> > +                                 Error **errp)
> > +{
> > +    error_set(errp, QERR_UNSUPPORTED);
> > +}
> > +
> If we have decided that there is no crypted password in Windows, may be 
> we should cross out this argument (with #define) or just take crypted: 
> false as default.

Agreed, but I think we can wait to document that limitation once a w32
implementation exists.

> >   /* add unsupported commands to the blacklist */
> >   GList *ga_command_blacklist_init(GList *blacklist)
> >   {
> > @@ -454,6 +462,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
> >           "guest-file-write", "guest-file-seek", "guest-file-flush",
> >           "guest-suspend-hybrid", "guest-network-get-interfaces",
> >           "guest-get-vcpus", "guest-set-vcpus",
> > +        "guest-set-user-password",
> >           "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
> >           "guest-fstrim", NULL};
> >       char **p = (char **)list_unsupported;
> > diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> > index 376e79f..5117b65 100644
> > --- a/qga/qapi-schema.json
> > +++ b/qga/qapi-schema.json
> > @@ -738,3 +738,30 @@
> >   ##
> >   { 'command': 'guest-get-fsinfo',
> >     'returns': ['GuestFilesystemInfo'] }
> > +
> > +##
> > +# @guest-set-user-password
> > +#
> > +# @username: the user account whose password to change
> > +# @password: the new password entry string, base64 encoded
> > +# @crypted: true if password is already crypt()d, false if raw
> > +#
> > +# If the @crypted flag is true, it is the caller's 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.
> > +#
> > +# Note all guest operating systems will support use of the
> > +# @crypted flag, as they may require the clear-text password
> > +#
> > +# The @password parameter must always be base64 encoded before
> > +# transmission, even if already crypt()d, to ensure it is 8-bit
> > +# safe when passed as JSON.
> > +#
> > +# Returns: Nothing on success.
> > +#
> > +# Since 2.3
> > +##
> > +{ 'command': 'guest-set-user-password',
> > +  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
> Everything seems fine.

Quoting Daniel P. Berrange (2015-02-11 05:26:12)
> Add a new 'guest-set-user-password' command for changing the password
> of guest OS user accounts. This command is needed to enable OpenStack
> to support its API for changing the admin password of guests running
> on KVM/QEMU. It is not practical to provide a command at the QEMU
> level explicitly targetting administrator account password change
> only, since different guest OS have different names for the admin
> account. While UNIX systems use 'root', Windows systems typically
> use 'Administrator' and even that can be renamed. Higher level apps
> like OpenStack have the ability to figure out the correct admin
> account name since they have info that QEMU/libvirt do not.
> 
> The command accepts either the clear text password string, encoded
> in base64 to make it 8-bit safe in JSON:
> 
> $ echo -n "123456" | base64
> MTIzNDU2
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>    '{ "execute": "guest-set-user-password",
>       "arguments": { "crypted": false,
>                      "username": "root",
>                      "password": "MTIzNDU2" } }'
>   {"return":{}}
> 
> Or a password that has already been run though a crypt(3) like
> algorithm appropriate for the guest, again then base64 encoded:
> 
> $ echo -n '$6$n01A2Tau$e...snip...DfMOP7of9AJ1I8q0' | base64
> JDYkb...snip...YT2Ey
> $ virsh -c qemu:///system  qemu-agent-command f21x86_64 \
>    '{ "execute": "guest-set-user-password",
>       "arguments": { "crypted": true,
>                      "username": "root",
>                      "password": "JDYkb...snip...YT2Ey" } }'
> 
> NB windows support is desirable, but not implemented in this
> patch.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Thanks, applied to QGA tree:

https://github.com/mdroth/qemu/commits/qga

> ---
>  qga/commands-posix.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c |   9 +++++
>  qga/qapi-schema.json |  27 +++++++++++++
>  3 files changed, 146 insertions(+)
> 
> In v3:
> 
>  - Renamed from guest-set-admin-password to guest-set-user-password
>  - Added 'username' argument
>  - Require 'password' to be base64 encoded
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f6f3e3c..57409d0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1875,6 +1875,108 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return processed;
>  }
> 
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    Error *local_err = NULL;
> +    char *passwd_path = NULL;
> +    pid_t pid;
> +    int status;
> +    int datafd[2] = { -1, -1 };
> +    char *rawpasswddata = NULL;
> +    size_t rawpasswdlen;
> +    char *chpasswddata = NULL;
> +    size_t chpasswdlen;
> +
> +    rawpasswddata = (char *)g_base64_decode(password, &rawpasswdlen);
> +    rawpasswddata = g_renew(char, rawpasswddata, rawpasswdlen + 1);
> +    rawpasswddata[rawpasswdlen] = '\0';
> +
> +    if (strchr(rawpasswddata, '\n')) {
> +        error_setg(errp, "forbidden characters in raw password");
> +        goto out;
> +    }
> +
> +    if (strchr(username, '\n') ||
> +        strchr(username, ':')) {
> +        error_setg(errp, "forbidden characters in username");
> +        goto out;
> +    }
> +
> +    chpasswddata = g_strdup_printf("%s:%s\n", username, rawpasswddata);
> +    chpasswdlen = strlen(chpasswddata);
> +
> +    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], chpasswddata, chpasswdlen) != chpasswdlen) {
> +        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 user password");
> +        goto out;
> +    }
> +
> +out:
> +    g_free(chpasswddata);
> +    g_free(rawpasswddata);
> +    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 +2012,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return -1;
>  }
> 
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 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..11876f6 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -446,6 +446,14 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
>      return -1;
>  }
> 
> +void qmp_guest_set_user_password(const char *username,
> +                                 const char *password,
> +                                 bool crypted,
> +                                 Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +}
> +
>  /* add unsupported commands to the blacklist */
>  GList *ga_command_blacklist_init(GList *blacklist)
>  {
> @@ -454,6 +462,7 @@ GList *ga_command_blacklist_init(GList *blacklist)
>          "guest-file-write", "guest-file-seek", "guest-file-flush",
>          "guest-suspend-hybrid", "guest-network-get-interfaces",
>          "guest-get-vcpus", "guest-set-vcpus",
> +        "guest-set-user-password",
>          "guest-fsfreeze-freeze-list", "guest-get-fsinfo",
>          "guest-fstrim", NULL};
>      char **p = (char **)list_unsupported;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 376e79f..5117b65 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -738,3 +738,30 @@
>  ##
>  { 'command': 'guest-get-fsinfo',
>    'returns': ['GuestFilesystemInfo'] }
> +
> +##
> +# @guest-set-user-password
> +#
> +# @username: the user account whose password to change
> +# @password: the new password entry string, base64 encoded
> +# @crypted: true if password is already crypt()d, false if raw
> +#
> +# If the @crypted flag is true, it is the caller's 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.
> +#
> +# Note all guest operating systems will support use of the
> +# @crypted flag, as they may require the clear-text password
> +#
> +# The @password parameter must always be base64 encoded before
> +# transmission, even if already crypt()d, to ensure it is 8-bit
> +# safe when passed as JSON.
> +#
> +# Returns: Nothing on success.
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-set-user-password',
> +  'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
> -- 
> 2.1.0