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

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

Quoting Daniel P. Berrange (2014-12-15 06:47:46)
> 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 <berrange@redhat.com>
> ---
>  qga/commands-posix.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-win32.c |  6 ++++
>  qga/qapi-schema.json | 13 ++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index f6f3e3c..3c3998a 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -1875,6 +1875,89 @@ 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);
> +
> +    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 (write(datafd[1], acctpw, strlen(acctpw)) != strlen(acctpw)) {
> +        error_setg(errp, "cannot write new account password");
> +        goto out;
> +    }

We should probably retry on EINTR, and for cases where -1 is returned I
think error_setg_errno() would be useful.

> +    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 suspend");

"... has failed to set admin password" or somesuch

> +        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 +1993,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..202d3be 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -738,3 +738,16 @@
>  ##
>  { 'command': 'guest-get-fsinfo',
>    'returns': ['GuestFilesystemInfo'] }
> +
> +##
> +# @guest-set-admin-password
> +#
> +# @crypted: true if password is already crypt()d, false if raw

Should we have some sort of note about what sort of encryption
scheme is expected, or a way to query for it?

> +# @password: the new password entry
> +#
> +# Returns: Nothing on success.
> +#
> +# Since 2.3
> +##
> +{ 'command': 'guest-set-admin-password',
> +  'data': { 'crypted': 'bool', 'password': 'str' } }
> -- 
> 2.1.0