[libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually

Martin Kletzander posted 1 patch 5 years, 7 months ago
Failed in applying to current master (apply log)
guests/lcitool                          | 29 +------------------------
guests/playbooks/update/tasks/users.yml |  2 +-
2 files changed, 2 insertions(+), 29 deletions(-)
[libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually
Posted by Martin Kletzander 5 years, 7 months ago
Since version 1.9 ansible supports password_hash filter that can do that for us.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
 guests/lcitool                          | 29 +------------------------
 guests/playbooks/update/tasks/users.yml |  2 +-
 2 files changed, 2 insertions(+), 29 deletions(-)

diff --git a/guests/lcitool b/guests/lcitool
index 2901a92c507b..ad1eee288620 100755
--- a/guests/lcitool
+++ b/guests/lcitool
@@ -151,34 +151,7 @@ class Config:
         return vault_pass_file
 
     def get_root_password_file(self):
-        root_pass_file = self._get_config_file("root-password")
-        root_hash_file = self._get_config_file(".root-password.hash")
-
-        try:
-            with open(root_pass_file, "r") as infile:
-                root_pass = infile.readline().strip()
-        except Exception:
-            raise Error(
-                "Missing or invalid root password file ({})".format(
-                    root_pass_file,
-                )
-            )
-
-        # The hash will be different every time we run, but that doesn't
-        # matter - it will still validate the correct root password
-        root_hash = crypt.crypt(root_pass, Util.mksalt())
-
-        try:
-            with open(root_hash_file, "w") as infile:
-                infile.write("{}\n".format(root_hash))
-        except Exception:
-            raise Error(
-                "Can't write hashed root password file ({})".format(
-                    root_hash_file,
-                )
-            )
-
-        return root_hash_file
+        return self._get_config_file("root-password")
 
 
 class Inventory:
diff --git a/guests/playbooks/update/tasks/users.yml b/guests/playbooks/update/tasks/users.yml
index ec7f798a9c00..0a930d6c382c 100644
--- a/guests/playbooks/update/tasks/users.yml
+++ b/guests/playbooks/update/tasks/users.yml
@@ -2,7 +2,7 @@
 - name: 'root: Set password'
   user:
     name: root
-    password: '{{ lookup("file", root_password_file) }}'
+    password: '{{ lookup("file", root_password_file)|password_hash("sha512") }}'
     shell: '{{ bash }}'
 
 - name: 'root: Configure ssh access'
-- 
2.18.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually
Posted by Andrea Bolognani 5 years, 7 months ago
On Tue, 2018-09-04 at 10:49 +0200, Martin Kletzander wrote:

s/manually/ourselves/ in the subject.

[...]
>      def get_root_password_file(self):
> -        root_pass_file = self._get_config_file("root-password")
> -        root_hash_file = self._get_config_file(".root-password.hash")
> -
> -        try:
> -            with open(root_pass_file, "r") as infile:
> -                root_pass = infile.readline().strip()
> -        except Exception:
> -            raise Error(
> -                "Missing or invalid root password file ({})".format(
> -                    root_pass_file,
> -                )
> -            )
> -
> -        # The hash will be different every time we run, but that doesn't
> -        # matter - it will still validate the correct root password
> -        root_hash = crypt.crypt(root_pass, Util.mksalt())
> -
> -        try:
> -            with open(root_hash_file, "w") as infile:
> -                infile.write("{}\n".format(root_hash))
> -        except Exception:
> -            raise Error(
> -                "Can't write hashed root password file ({})".format(
> -                    root_hash_file,
> -                )
> -            )
> -
> -        return root_hash_file
> +        return self._get_config_file("root-password")

This is a really nice improvement overall, but we can't quite get
rid of the entire function: we still need to try and open the file,
or at least stat() it, like we do in get_vault_password_file(), so
that we can error out early instead of having Ansible bail out on
us really late in the game.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually
Posted by Martin Kletzander 5 years, 7 months ago
On Tue, Sep 04, 2018 at 01:53:45PM +0200, Andrea Bolognani wrote:
>On Tue, 2018-09-04 at 10:49 +0200, Martin Kletzander wrote:
>
>s/manually/ourselves/ in the subject.
>
>[...]
>>      def get_root_password_file(self):
>> -        root_pass_file = self._get_config_file("root-password")
>> -        root_hash_file = self._get_config_file(".root-password.hash")
>> -
>> -        try:
>> -            with open(root_pass_file, "r") as infile:
>> -                root_pass = infile.readline().strip()
>> -        except Exception:
>> -            raise Error(
>> -                "Missing or invalid root password file ({})".format(
>> -                    root_pass_file,
>> -                )
>> -            )
>> -
>> -        # The hash will be different every time we run, but that doesn't
>> -        # matter - it will still validate the correct root password
>> -        root_hash = crypt.crypt(root_pass, Util.mksalt())
>> -
>> -        try:
>> -            with open(root_hash_file, "w") as infile:
>> -                infile.write("{}\n".format(root_hash))
>> -        except Exception:
>> -            raise Error(
>> -                "Can't write hashed root password file ({})".format(
>> -                    root_hash_file,
>> -                )
>> -            )
>> -
>> -        return root_hash_file
>> +        return self._get_config_file("root-password")
>
>This is a really nice improvement overall, but we can't quite get
>rid of the entire function: we still need to try and open the file,
>or at least stat() it, like we do in get_vault_password_file(), so
>that we can error out early instead of having Ansible bail out on
>us really late in the game.
>

So what you had in mind is something like the following squashed in?

diff --git i/guests/lcitool w/guests/lcitool
index 609c73c43dbc..2ac98ea69030 100755
--- i/guests/lcitool
+++ w/guests/lcitool
@@ -151,7 +151,22 @@ class Config:
         return vault_pass_file

     def get_root_password_file(self):
-        return self._get_config_file("root-password")
+        root_pass_file = None
+
+        root_pass_file = self._get_config_file("root-password")
+
+        try:
+            with open(root_pass_file, "r") as infile:
+                if not infile.readline().strip():
+                    raise ValueError
+        except Exception:
+            raise Error(
+                "Missing or invalid root password file ({})".format(
+                    root_pass_file,
+                )
+            )
+
+        return root_pass_file


 class Inventory:
--

Or we could have the check in ansible itself, but that would be a bigger change
and the codebase is not prepared for that.

TLTTIRN,
Martin
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually
Posted by Andrea Bolognani 5 years, 7 months ago
On Wed, 2018-09-05 at 11:39 +0200, Martin Kletzander wrote:
> On Tue, Sep 04, 2018 at 01:53:45PM +0200, Andrea Bolognani wrote:
> > On Tue, 2018-09-04 at 10:49 +0200, Martin Kletzander wrote:
> > 
> > s/manually/ourselves/ in the subject.

[...]
> > This is a really nice improvement overall, but we can't quite get
> > rid of the entire function: we still need to try and open the file,
> > or at least stat() it, like we do in get_vault_password_file(), so
> > that we can error out early instead of having Ansible bail out on
> > us really late in the game.
> 
> So what you had in mind is something like the following squashed in?

Pretty much exactly, yes :)

[...]
>      def get_root_password_file(self):
> -        return self._get_config_file("root-password")
> +        root_pass_file = None
> +

These two lines can be avoided.

> +        root_pass_file = self._get_config_file("root-password")
> +
> +        try:
> +            with open(root_pass_file, "r") as infile:
> +                if not infile.readline().strip():
> +                    raise ValueError

This introduces a *slight* semantic change, in that we won't
accept an empty root password anymore. I'd argue it's probably
for the best anyway, so let's just go ahead with it.

> Or we could have the check in ansible itself, but that would be a bigger change
> and the codebase is not prepared for that.

It shouldn't be *too* hard, and we could also take the
opportunity to fix the long-standing issue of 'update' not
working correctly if ~/.ssh/id_rsa.pub doesn't exist.

But we can worry about that another day; in the meantime, if
you fix the two nits pointed out above you can have my

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list