[libvirt] [jenkins-ci PATCH] lcitool: Don't encrypt password manually
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
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
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
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
participants (2)
-
Andrea Bolognani -
Martin Kletzander