[virt-tools-list] [virt-manager PATCH 1/2] unattended: Read the passwords from a file

[Pavel Hrdina]

On Tue, Jul 02, 2019 at 09:21:44PM +0200, Fabiano Fidêncio wrote:
> Let's not expose the user/root password in the CLI and, instead, let's
> rely on a file passed by the admin and read the password from there.
> 
> 'CVE-2019-10183' has been assigned to the virt-install --unattended
> admin-password=xxx disclosure issue.
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
>  man/virt-install.pod                  | 14 ++++++++++----
>  tests/cli-test-xml/admin-password.txt |  1 +
>  tests/cli-test-xml/user-password.txt  |  3 +++
>  tests/clitest.py                      | 18 +++++++++--------
>  virtinst/cli.py                       |  4 ++--
>  virtinst/install/unattended.py        | 28 +++++++++++++++++----------
>  6 files changed, 44 insertions(+), 24 deletions(-)
>  create mode 100644 tests/cli-test-xml/admin-password.txt
>  create mode 100644 tests/cli-test-xml/user-password.txt
> 
> diff --git a/man/virt-install.pod b/man/virt-install.pod
> index d8bd4127..0e655fef 100644
> --- a/man/virt-install.pod
> +++ b/man/virt-install.pod
> @@ -612,13 +612,19 @@ Choose which libosinfo unattended profile to use. Most distros have
>  a 'desktop' and a 'jeos' profile. virt-install will default to 'desktop'
>  if this is unspecified.
>  
> -=item B<admin-password=>
> +=item B<admin-password-file=>
>  
> -Set the VM OS admin/root password
> +A file used to set the VM OS admin/root password from. This option can
> +be used either as "admin-password-file=/path/to/passowrd-file" or as
> +"admin-password-file=/dev/fd/n", being n the file descriptor of the
> +password-file.

Typo fixed in my local branch.

> -=item B<user-password=>
> +=item B<user-password-file=>
>  
> -Set the VM user password. The username is your current host username
> +A file used to set the VM user password. This option can be used either as
> +"user-password-file=/path/to/password-file" or as
> +"user-password-file=/dev/fd/n", being n the file descriptor of the
> +password-file. The username is your current host username.
>  
>  =item B<product-key=>
>  
> diff --git a/tests/cli-test-xml/admin-password.txt b/tests/cli-test-xml/admin-password.txt
> new file mode 100644
> index 00000000..323fae03
> --- /dev/null
> +++ b/tests/cli-test-xml/admin-password.txt
> @@ -0,0 +1 @@
> +foobar
> diff --git a/tests/cli-test-xml/user-password.txt b/tests/cli-test-xml/user-password.txt
> new file mode 100644
> index 00000000..625999ba
> --- /dev/null
> +++ b/tests/cli-test-xml/user-password.txt
> @@ -0,0 +1,3 @@
> +blah
> +     
> +        

Are these random white spaces intentional?  There are two lines and
both with different number of white spaces.  If not I can fix it before
pushing.

[...]

> diff --git a/virtinst/install/unattended.py b/virtinst/install/unattended.py
> index ea3f9066..4f311296 100644
> --- a/virtinst/install/unattended.py
> +++ b/virtinst/install/unattended.py
> @@ -39,23 +39,21 @@ def _make_installconfig(script, osobj, unattended_data, arch, hostname, url):
>  
>      # Set user-password.
>      # In case it's required and not passed, just raise a RuntimeError.
> -    if script.requires_user_password() and not unattended_data.user_password:
> +    if (script.requires_user_password() and
> +        not unattended_data.get_user_password()):
>          raise RuntimeError(
>              _("%s requires the user-password to be set.") %
>              osobj.name)
> -    config.set_user_password(
> -        unattended_data.user_password if unattended_data.user_password
> -        else "")
> +    config.set_user_password(unattended_data.get_user_password() or "")
>  
>      # Set the admin-password:
>      # In case it's required and not passed, just raise a RuntimeError.
> -    if script.requires_admin_password() and not unattended_data.admin_password:
> +    if (script.requires_admin_password() and
> +        not unattended_data.get_admin_password()):
>          raise RuntimeError(
>              _("%s requires the admin-password to be set.") %
>              osobj.name)
> -    config.set_admin_password(
> -        unattended_data.admin_password if unattended_data.admin_password
> -        else "")
> +    config.set_admin_password(unattended_data.get_admin_password() or "")
>  
>      # Set the target disk.
>      # virtiodisk is the preferred way, in case it's supported, otherwise
> @@ -205,10 +203,20 @@ class OSInstallScript:
>  
>  class UnattendedData():
>      profile = None
> -    admin_password = None
> -    user_password = None
> +    admin_password_file = None
> +    user_password_file = None
>      product_key = None
>  
> +    def get_user_password(self):
> +        if self.user_password_file:
> +            with open(self.user_password_file) as pwd:
> +                return pwd.read().rstrip("\n\r")
> +
> +    def get_admin_password(self):
> +        if self.admin_password_file:
> +            with open(self.admin_password_file) as pwd:
> +                return pwd.read().rstrip("\n\r")
> +

I guess this is related to my question above, we will strip trailing
lines, everything else is considered as a valid password including
other white space characters?

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20190703/4cc37b79/attachment.sig>