Re: [Qemu-devel] [PATCH v3] qga: add guest-set-user-password command

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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.