New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

user.present with hash_password: True detects change on every state.apply/highstate #45939

Closed
andygabby opened this Issue Feb 9, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@andygabby

andygabby commented Feb 9, 2018

Description of Issue/Question

user.present with hash_password: True detects change on every highstate. The password is successfully set to what was intended, but upon subsequent highstates, salt always thinks it needs to change the root password again.

Setup

# salt/root_pw.sls
root:
  user.present:
    - password: {{ salt['pillar.get']('root_pw') }}
    - hash_password: True
# pillar/root_pw.sls
root_pw: supersecret

Steps to Reproduce Issue

Run state multiple times against same node. The password is set successfully without error, but always detects a change is necessary.

Versions Report

Minions and masters all running same version. Tested on 2017.7.2 and 2016.11.2

Salt Version:
           Salt: 2017.7.2
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.1
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Aug  4 2017, 00:39:18)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.4.1708 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-693.17.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.4.1708 Core

Also not working on:

Salt Version:
           Salt: 2016.11.2
 
Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 1.5
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.5 (default, Aug  4 2017, 00:39:18)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.4.1708 Core
        machine: x86_64
        release: 3.10.0-693.17.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.4.1708 Core
@garethgreenaway

This comment has been minimized.

Member

garethgreenaway commented Feb 9, 2018

@andygabby Thanks for the report. Looking at the code for this I can see why it's continuously attempting to set the password. When hash_password is set to True, the state will call shadow.gen_password using the provided password. If you try running that module function manually you'll see that the hash it generates each time is different. If you set the enforce_password option to False then it the password will not be updated.

@andygabby

This comment has been minimized.

andygabby commented Feb 9, 2018

@garethgreenaway Thanks for the reply! I noticed the same about shadow.gen_password. Setting enforce_password: False kind of defeats the purpose of changing the pillar for pushing password updates.

shadow.gen_password can take a crypt_salt argument so that if passed the same value each time, would give you a consistent hash of the same password.

salt-call shadow.gen_password supersecret crypt_salt=salty

Perhaps the user state/module could take crypt_salt as an argument to pass to shadow.gen_password so a consistent salt value could be used to hash the password?

For some of us using encrypted pillars, there are use cases where it may not be desirable or needed to gen a password, hash it, then encrypt the hash for the pillar

Thanks!

@tigpas

This comment has been minimized.

tigpas commented Apr 6, 2018

Hi,

I have the same problem and tried to find a solution: If salt get plain passwd and the current password with correct hash function is stored on the system, salt can compare it by combining the plain password and the stored salt. If the result between this new hash and the stored hash, salt can set a new password. I think, all needed information are available.

I tried to extend salt to do it.

diff --git a/salt/modules/shadow.py b/salt/modules/shadow.py
index 71e6749..6942750 100644
--- a/salt/modules/shadow.py
+++ b/salt/modules/shadow.py
@@ -81,6 +81,25 @@ def info(name):
             'expire': ''}
     return ret
 
+def pwsalt(name, algorithm='sha512'):
+    '''
+    Return the password salt for the specified user.
+
+    If algorithm not correct, it will return None.
+
+    algorithm
+        Supported value: sha512
+
+    .. code-block:: bash
+
+        salt '*' shadow.pwsalt root sha512
+    '''
+
+    pre_info = info(name)
+    if algorithm == 'sha512' and pre_info['passwd'].startswith('$6$'):
+        return pre_info['passwd'].split('$')[2]
+
+    return None
 
 def set_inactdays(name, inactdays):
     '''
diff --git a/salt/states/user.py b/salt/states/user.py
index 691300a..8bef264 100644
--- a/salt/states/user.py
+++ b/salt/states/user.py
@@ -401,9 +401,16 @@ def present(name,
     # First check if a password is set. If password is set, check if
     # hash_password is True, then hash it.
     if password and hash_password:
+        log.debug('Hashing clear password with old salt only for compare')
+        passwordoldsalt = __salt__['shadow.pwsalt'](name)
+        comparepassword = __salt__['shadow.gen_password'](password, passwordoldsalt)
+
         log.debug('Hashing a clear text password')
         password = __salt__['shadow.gen_password'](password)
 
+    elif password:
+        comparepassword = password
+
     if fullname is not None:
         fullname = sdecode(fullname)
     if roomnumber is not None:
@@ -474,7 +481,7 @@ def present(name,
                        remove_groups,
                        home,
                        createhome,
-                       password,
+                       comparepassword,
                        enforce_password,
                        empty_password,
                        shell,

Sorry for my bad python skils, but maybe it's for anyone usable.

This diff is working for me on this system in this folder:

root@server-bento-debian-9:/usr/lib/python2.7/dist-packages# salt-call -V
Salt Version:
           Salt: 2018.3.0
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Nov 24 2017, 17:33:09)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1
 
System Versions:
           dist: debian 9.4 
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-6-amd64
         system: Linux
        version: debian 9.4 

eliasp added a commit to eliasp/salt that referenced this issue Apr 18, 2018

@tigpas

This comment has been minimized.

tigpas commented Apr 19, 2018

@eliasp Your solution looks good for me, but you will reuse the same password salt if you change the password. I would expect from states.user.present that a new salt is generated if the password is changed. I think, that can become a security issue.

I think, it's better to reuse the salt only for compare the passwords
tigpas@5066c0c

@cachedout cachedout closed this in 5451ab6 Jul 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment