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

[Pavel Hrdina]

On Wed, Jul 03, 2019 at 12:20:23PM +0200, Fabiano Fidêncio wrote:
> 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. :-)

I'm not sure this works the way you intend it to work, for the example
above if you try this in python3:

    tmp = "blah\n      \n        "
    print(tmp.rstrip("\n\r"))

will print the same string and the new lines with white spaces will be
part of the password, is that intended?

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/8601138f/attachment.sig>