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

[Fabiano Fidêncio]

On Wed, Jul 3, 2019 at 10:53 AM Pavel Hrdina <phrdina at redhat.com> wrote:
>
> 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.

Those are intentional ...

>
> [...]
>
> > 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?

We'll strip trailing lines, everything else can be considered a valid
password, even if it includes whitespaces in the end. :-)

>
> Pavel