Merge pull request #31162 from isbm/isbm-md5-to-sha1

Remove MD5 digest from everywhere and default to SHA256

conf/master

@@ -465,10 +465,13 @@
 #default_top: base
 
 # The hash_type is the hash to use when discovering the hash of a file on
-# the master server. The default is md5, but sha1, sha224, sha256, sha384
+# the master server. The default is md5 but sha1, sha224, sha256, sha384
 # and sha512 are also supported.
 #
-# Prior to changing this value, the master should be stopped and all Salt
+# WARNING: While md5 is supported, do not use it due to the high chance
+# of possible collisions and thus security breach.
+#
+# Prior to changing this value, the master should be stopped and all Salt 
 # caches should be cleared.
 #hash_type: md5
 

conf/minion

@@ -444,12 +444,14 @@
 #fileserver_limit_traversal: False
 
 # The hash_type is the hash to use when discovering the hash of a file in
-# the local fileserver. The default is md5, but sha1, sha224, sha256, sha384
-# and sha512 are also supported.
+# the local fileserver. The default is sha256, sha224, sha384 and sha512 are also supported.
+#
+# WARNING: While md5 and sha1 are also supported, do not use it due to the high chance
+# of possible collisions and thus security breach.
 #
 # Warning: Prior to changing this value, the minion should be stopped and all
 # Salt caches should be cleared.
-#hash_type: md5
+#hash_type: sha256
 
 # The Salt pillar is searched for locally if file_client is set to local. If
 # this is the case, and pillar data is defined, then the pillar_roots need to

conf/proxy

@@ -419,12 +419,15 @@
 #fileserver_limit_traversal: False
 
 # The hash_type is the hash to use when discovering the hash of a file in
-# the local fileserver. The default is md5, but sha1, sha224, sha256, sha384
-# and sha512 are also supported.
+# the local fileserver. The default is sha256 but sha224, sha384 and sha512
+# are also supported.
+#
+# WARNING: While md5 and sha1 are also supported, do not use it due to the high chance
+# of possible collisions and thus security breach.
 #
 # Warning: Prior to changing this value, the minion should be stopped and all
 # Salt caches should be cleared.
-#hash_type: md5
+#hash_type: sha256
 
 # The Salt pillar is searched for locally if file_client is set to local. If
 # this is the case, and pillar data is defined, then the pillar_roots need to

salt/cli/daemons.py

@@ -58,7 +58,50 @@
 logger = salt.log.setup.logging.getLogger(__name__)
 
 
-class Master(parsers.MasterOptionParser):
+class DaemonsMixin(object):  # pylint: disable=no-init
+    '''
+    Uses the same functions for all daemons
+    '''
+    def verify_hash_type(self):
+        '''
+        Verify and display a nag-messsage to the log if vulnerable hash-type is used.
+
+        :return:
+        '''
+        if self.config['hash_type'].lower() in ['md5', 'sha1']:
+            logger.warning('IMPORTANT: Do not use {h_type} hashing algorithm! Please set "hash_type" to '
+                           'SHA256 in Salt {d_name} config!'.format(
+                h_type=self.config['hash_type'], d_name=self.__class__.__name__))
+
+    def start_log_info(self):
+        '''
+        Say daemon starting.
+
+        :return:
+        '''
+        logger.info('The Salt {d_name} is starting up'.format(d_name=self.__class__.__name__))
+
+    def shutdown_log_info(self):
+        '''
+        Say daemon shutting down.
+
+        :return:
+        '''
+        logger.info('The Salt {d_name} is shut down'.format(d_name=self.__class__.__name__))
+
+    def environment_failure(self, error):
+        '''
+        Log environment failure for the daemon and exit with the error code.
+
+        :param error:
+        :return:
+        '''
+        logger.exception('Failed to create environment for {d_name}: {reason}'.format(
+            d_name=self.__class__.__name__, reason=error.message))
+        sys.exit(error.errno)
+
+
+class Master(parsers.MasterOptionParser, DaemonsMixin):  # pylint: disable=no-init
     '''
     Creates a master server
     '''
@@ -114,8 +157,7 @@ def prepare(self):
                 for syndic_file in os.listdir(self.config['syndic_dir']):
                     os.remove(os.path.join(self.config['syndic_dir'], syndic_file))
         except OSError as err:
-            logger.exception('Failed to prepare salt environment')
-            sys.exit(err.errno)
+            self.environment_failure(err)
 
         self.setup_logfile_logger()
         verify_log(self.config)
@@ -153,17 +195,18 @@ def start(self):
         '''
         self.prepare()
         if check_user(self.config['user']):
-            logger.info('The salt master is starting up')
+            self.verify_hash_type()
+            self.start_log_info()
             self.master.start()
 
     def shutdown(self):
         '''
         If sub-classed, run any shutdown operations on this method.
         '''
-        logger.info('The salt master is shut down')
+        self.shutdown_log_info()
 
 
-class Minion(parsers.MinionOptionParser):  # pylint: disable=no-init
+class Minion(parsers.MinionOptionParser, DaemonsMixin):  # pylint: disable=no-init
     '''
     Create a minion server
     '''
@@ -226,8 +269,7 @@ def prepare(self):
                     verify_files([logfile], self.config['user'])
                     os.umask(current_umask)
         except OSError as err:
-            logger.exception('Failed to prepare salt environment')
-            sys.exit(err.errno)
+            self.environment_failure(err)
 
         self.setup_logfile_logger()
         verify_log(self.config)
@@ -273,7 +315,8 @@ def start(self):
         try:
             self.prepare()
             if check_user(self.config['user']):
-                logger.info('The salt minion is starting up')
+                self.verify_hash_type()
+                self.start_log_info()
                 self.minion.tune_in()
         finally:
             self.shutdown()
@@ -310,10 +353,10 @@ def shutdown(self):
         '''
         If sub-classed, run any shutdown operations on this method.
         '''
-        logger.info('The salt minion is shut down')
+        self.shutdown_log_info()
 
 
-class ProxyMinion(parsers.ProxyMinionOptionParser):  # pylint: disable=no-init
+class ProxyMinion(parsers.ProxyMinionOptionParser, DaemonsMixin):  # pylint: disable=no-init
     '''
     Create a proxy minion server
     '''
@@ -388,8 +431,7 @@ def prepare(self):
                     os.umask(current_umask)
 
         except OSError as err:
-            logger.exception('Failed to prepare salt environment')
-            sys.exit(err.errno)
+            self.environment_failure(err)
 
         self.setup_logfile_logger()
         verify_log(self.config)
@@ -431,7 +473,8 @@ def start(self):
         try:
             self.prepare()
             if check_user(self.config['user']):
-                logger.info('The proxy minion is starting up')
+                self.verify_hash_type()
+                self.start_log_info()
                 self.minion.tune_in()
         except (KeyboardInterrupt, SaltSystemExit) as exc:
             logger.warn('Stopping the Salt Proxy Minion')
@@ -449,10 +492,10 @@ def shutdown(self):
         if hasattr(self, 'minion') and 'proxymodule' in self.minion.opts:
             proxy_fn = self.minion.opts['proxymodule'].loaded_base_name + '.shutdown'
             self.minion.opts['proxymodule'][proxy_fn](self.minion.opts)
-        logger.info('The proxy minion is shut down')
+        self.shutdown_log_info()
 
 
-class Syndic(parsers.SyndicOptionParser):
+class Syndic(parsers.SyndicOptionParser, DaemonsMixin):  # pylint: disable=no-init
     '''
     Create a syndic server
     '''
@@ -488,8 +531,7 @@ def prepare(self):
                     verify_files([logfile], self.config['user'])
                     os.umask(current_umask)
         except OSError as err:
-            logger.exception('Failed to prepare salt environment')
-            sys.exit(err.errno)
+            self.environment_failure(err)
 
         self.setup_logfile_logger()
         verify_log(self.config)
@@ -521,7 +563,8 @@ def start(self):
         '''
         self.prepare()
         if check_user(self.config['user']):
-            logger.info('The salt syndic is starting up')
+            self.verify_hash_type()
+            self.start_log_info()
             try:
                 self.syndic.tune_in()
             except KeyboardInterrupt:
@@ -532,4 +575,4 @@ def shutdown(self):
         '''
         If sub-classed, run any shutdown operations on this method.
         '''
-        logger.info('The salt syndic is shut down')
+        self.shutdown_log_info()

salt/crypt.py

@@ -559,11 +559,11 @@ def sign_in(self, timeout=60, safe=True, tries=1):
         if self.opts.get('syndic_master', False):  # Is syndic
             syndic_finger = self.opts.get('syndic_finger', self.opts.get('master_finger', False))
             if syndic_finger:
-                if salt.utils.pem_finger(m_pub_fn) != syndic_finger:
+                if salt.utils.pem_finger(m_pub_fn, sum_type=self.opts['hash_type']) != syndic_finger:
                     self._finger_fail(syndic_finger, m_pub_fn)
         else:
             if self.opts.get('master_finger', False):
-                if salt.utils.pem_finger(m_pub_fn) != self.opts['master_finger']:
+                if salt.utils.pem_finger(m_pub_fn, sum_type=self.opts['hash_type']) != self.opts['master_finger']:
                     self._finger_fail(self.opts['master_finger'], m_pub_fn)
         auth['publish_port'] = payload['publish_port']
         raise tornado.gen.Return(auth)

salt/modules/key.py

@@ -21,10 +21,8 @@ def finger():
 
         salt '*' key.finger
     '''
-    return salt.utils.pem_finger(
-            os.path.join(__opts__['pki_dir'], 'minion.pub'),
-            sum_type=__opts__['hash_type']
-            )
+    return salt.utils.pem_finger(os.path.join(__opts__['pki_dir'], 'minion.pub'),
+                                 sum_type=__opts__.get('hash_type', 'md5'))
 
 
 def finger_master():
@@ -37,7 +35,5 @@ def finger_master():
 
         salt '*' key.finger_master
     '''
-    return salt.utils.pem_finger(
-            os.path.join(__opts__['pki_dir'], 'minion_master.pub'),
-            sum_type=__opts__['hash_type']
-            )
+    return salt.utils.pem_finger(os.path.join(__opts__['pki_dir'], 'minion_master.pub'),
+                                 sum_type=__opts__.get('hash_type', 'md5'))

salt/modules/tomcat.py

@@ -631,7 +631,7 @@ def deploy_war(war,
 
 def passwd(passwd,
            user='',
-           alg='md5',
+           alg='sha1',
            realm=None):
     '''
     This function replaces the $CATALINA_HOME/bin/digest.sh script
@@ -646,23 +646,15 @@ def passwd(passwd,
         salt '*' tomcat.passwd secret tomcat sha1
         salt '*' tomcat.passwd secret tomcat sha1 'Protected Realm'
     '''
-    if alg == 'md5':
-        m = hashlib.md5()
-    elif alg == 'sha1':
-        m = hashlib.sha1()
-    else:
-        return False
-
-    if realm:
-        m.update('{0}:{1}:{2}'.format(
-            user,
-            realm,
-            passwd,
-            ))
-    else:
-        m.update(passwd)
+    # Shouldn't it be SHA265 instead of SHA1?
+    digest = hasattr(hashlib, alg) and getattr(hashlib, alg) or None
+    if digest:
+        if realm:
+            digest.update('{0}:{1}:{2}'.format(user, realm, passwd,))
+        else:
+            digest.update(passwd)
 
-    return m.hexdigest()
+    return digest and digest.hexdigest() or False
 
 
 # Non-Manager functions

salt/modules/win_file.py

@@ -842,7 +842,7 @@ def chgrp(path, group):
     return None
 
 
-def stats(path, hash_type='md5', follow_symlinks=True):
+def stats(path, hash_type='sha256', follow_symlinks=True):
     '''
     Return a dict containing the stats for a given file
 

salt/utils/__init__.py

@@ -858,10 +858,10 @@ def path_join(*parts):
     ))
 
 
-def pem_finger(path=None, key=None, sum_type='md5'):
+def pem_finger(path=None, key=None, sum_type='sha256'):
     '''
     Pass in either a raw pem string, or the path on disk to the location of a
-    pem file, and the type of cryptographic hash to use. The default is md5.
+    pem file, and the type of cryptographic hash to use. The default is SHA256.
     The fingerprint of the pem will be returned.
 
     If neither a key nor a path are passed in, a blank string will be returned.
@@ -1979,7 +1979,7 @@ def safe_walk(top, topdown=True, onerror=None, followlinks=True, _seen=None):
         yield top, dirs, nondirs
 
 
-def get_hash(path, form='md5', chunk_size=65536):
+def get_hash(path, form='sha256', chunk_size=65536):
     '''
     Get the hash sum of a file
 
@@ -1989,10 +1989,10 @@ def get_hash(path, form='md5', chunk_size=65536):
             ``get_sum`` cannot really be trusted since it is vulnerable to
             collisions: ``get_sum(..., 'xyz') == 'Hash xyz not supported'``
     '''
-    try:
-        hash_type = getattr(hashlib, form)
-    except (AttributeError, TypeError):
+    hash_type = hasattr(hashlib, form) and getattr(hashlib, form) or None
+    if hash_type is None:
         raise ValueError('Invalid hash type: {0}'.format(form))
+
     with salt.utils.fopen(path, 'rb') as ifile:
         hash_obj = hash_type()
         # read the file in in chunks, not the entire file

salt/utils/cloud.py

@@ -2465,6 +2465,7 @@ def init_cachedir(base=None):
 
 def request_minion_cachedir(
         minion_id,
+        opts=None,
         fingerprint='',
         pubkey=None,
         provider=None,
@@ -2484,7 +2485,7 @@ def request_minion_cachedir(
 
     if not fingerprint:
         if pubkey is not None:
-            fingerprint = salt.utils.pem_finger(key=pubkey)
+            fingerprint = salt.utils.pem_finger(key=pubkey, sum_type=(opts and opts.get('hash_type') or 'sha256'))
 
     init_cachedir(base)