Merge pull request #30894 from terminalmage/issue30858

git module/state: Handle identity files more gracefully

salt/modules/git.py

@@ -6,7 +6,6 @@ from __future__ import absolute_import
 
 # Import python libs
 import copy
-import errno
 import logging
 import os
 import re
@@ -155,7 +154,9 @@ def _git_run(command, cwd=None, runas=None, identity=None,
     env = {}
 
     if identity:
-        stderrs = []
+        _salt_cli = __opts__.get('__cli', '')
+        errors = []
+        missing_keys = []
 
         # if the statefile provides multiple identities, they need to be tried
         # (but also allow a string instead of a list)
@@ -165,6 +166,11 @@ def _git_run(command, cwd=None, runas=None, identity=None,
 
         # try each of the identities, independently
         for id_file in identity:
+            if not os.path.isfile(id_file):
+                missing_keys.append(id_file)
+                log.warning('Identity file {0} does not exist'.format(id_file))
+                continue
+
             env = {
                 'GIT_IDENTITY': id_file
             }
@@ -198,6 +204,21 @@ def _git_run(command, cwd=None, runas=None, identity=None,
                 os.chown(tmp_file, __salt__['file.user_to_uid'](runas), -1)
                 env['GIT_SSH'] = tmp_file
 
+            if 'salt-call' not in _salt_cli \
+                    and __salt__['ssh.key_is_encrypted'](id_file):
+                errors.append(
+                    'Identity file {0} is passphrase-protected and cannot be '
+                    'used in a non-interactive command. Using salt-call from '
+                    'the minion will allow a passphrase-protected key to be '
+                    'used.'.format(id_file)
+                )
+                continue
+
+            log.info(
+                'Attempting git authentication using identity file {0}'
+                .format(id_file)
+            )
+
             try:
                 result = __salt__['cmd.run_all'](
                     command,
@@ -213,17 +234,29 @@ def _git_run(command, cwd=None, runas=None, identity=None,
                 if not salt.utils.is_windows() and 'GIT_SSH' in env:
                     os.remove(env['GIT_SSH'])
 
-            # if the command was successful, no need to try additional IDs
+            # If the command was successful, no need to try additional IDs
             if result['retcode'] == 0:
                 return result
             else:
                 stderr = \
                     salt.utils.url.redact_http_basic_auth(result['stderr'])
-                stderrs.append(stderr)
+                errors.append(stderr)
 
-        # we've tried all IDs and still haven't passed, so error out
+        # We've tried all IDs and still haven't passed, so error out
         if failhard:
-            raise CommandExecutionError('\n\n'.join(stderrs))
+            msg = (
+                'Unable to authenticate using identity file:\n\n{0}'.format(
+                    '\n'.join(errors)
+                )
+            )
+            if missing_keys:
+                if errors:
+                    msg += '\n\n'
+                msg += (
+                    'The following identity file(s) were not found: {0}'
+                    .format(', '.join(missing_keys))
+                )
+            raise CommandExecutionError(msg)
         return result
 
     else:
@@ -677,13 +710,19 @@ def clone(cwd,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     https_user
         Set HTTP Basic Auth username. Only accepted for HTTPS URLs.
 
@@ -1369,13 +1408,19 @@ def fetch(cwd,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     ignore_retcode : False
         If ``True``, do not log an error to the minion log if the git command
         returns a nonzero exit status.
@@ -1886,20 +1931,8 @@ def list_worktrees(cwd, stale=False, user=None, **kwargs):
                         break
                     return ret
             except (IOError, OSError) as exc:
-                if exc.errno == errno.ENOENT:
+                # Raise a CommandExecutionError
-                    raise CommandExecutionError(
+                salt.utils.files.process_read_exception(exc, path)
-                        '{0} does not exist'.format(path)
-                    )
-                elif exc.errno == errno.EACCES:
-                    raise CommandExecutionError(
-                        'Permission denied reading from {0}'.format(path)
-                    )
-                else:
-                    raise CommandExecutionError(
-                        'Error {0} encountered reading from {1}: {2}'.format(
-                            exc.errno, path, exc.strerror
-                        )
-                    )
 
         for worktree_name in os.listdir(worktree_root):
             admin_dir = os.path.join(worktree_root, worktree_name)
@@ -2009,13 +2042,19 @@ def ls_remote(cwd=None,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     https_user
         Set HTTP Basic Auth username. Only accepted for HTTPS URLs.
 
@@ -2427,13 +2466,19 @@ def pull(cwd, opts='', user=None, identity=None, ignore_retcode=False):
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     ignore_retcode : False
         If ``True``, do not log an error to the minion log if the git command
         returns a nonzero exit status.
@@ -2507,13 +2552,19 @@ def push(cwd,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     ignore_retcode : False
         If ``True``, do not log an error to the minion log if the git command
         returns a nonzero exit status.
@@ -2703,13 +2754,19 @@ def remote_refs(url,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     https_user
         Set HTTP Basic Auth username. Only accepted for HTTPS URLs.
 
@@ -3275,13 +3332,19 @@ def submodule(cwd,
 
         .. warning::
 
-            Key must be passphraseless to allow for non-interactive login. For
+            Unless Salt is invoked from the minion using ``salt-call``, the
-            greater security with passphraseless private keys, see the
+            key(s) must be passphraseless. For greater security with
-            `sshd(8)`_ manpage for information on securing the keypair from the
+            passphraseless private keys, see the `sshd(8)`_ manpage for
-            remote side in the ``authorized_keys`` file.
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
 
             .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
 
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
+
     ignore_retcode : False
         If ``True``, do not log an error to the minion log if the git command
         returns a nonzero exit status.

salt/modules/ssh.py

@@ -2,23 +2,26 @@
 '''
 Manage client ssh components
 
-.. note:: This module requires the use of MD5 hashing. Certain
+.. note::
-    security audits may not permit the use of MD5. For those cases,
+
-    this module should be disabled or removed.
+    This module requires the use of MD5 hashing. Certain security audits may
+    not permit the use of MD5. For those cases, this module should be disabled
+    or removed.
 '''
 
 from __future__ import absolute_import
 
 # Import python libs
+import binascii
+import hashlib
+import logging
 import os
 import re
-import hashlib
-import binascii
-import logging
 import subprocess
 
 # Import salt libs
 import salt.utils
+import salt.utils.files
 import salt.utils.decorators as decorators
 from salt.exceptions import (
     SaltInvocationError,
@@ -286,7 +289,10 @@ def host_keys(keydir=None, private=True):
     for fn_ in os.listdir(keydir):
         if fn_.startswith('ssh_host_'):
             if fn_.endswith('.pub') is False and private is False:
-                log.info('Skipping private key file {0} as private is set to False'.format(fn_))
+                log.info(
+                    'Skipping private key file {0} as private is set to False'
+                    .format(fn_)
+                )
                 continue
 
             top = fn_.split('.')
@@ -296,11 +302,14 @@ def host_keys(keydir=None, private=True):
                 kname += '.{0}'.format(top[1])
             try:
                 with salt.utils.fopen(os.path.join(keydir, fn_), 'r') as _fh:
-                    # As of RFC 4716 "a key file is a text file, containing a sequence of lines",
+                    # As of RFC 4716 "a key file is a text file, containing a
-                    # although some SSH implementations (e.g. OpenSSH) manage their own format(s).
+                    # sequence of lines", although some SSH implementations
-                    # Please see #20708 for a discussion about how to handle SSH key files in the future
+                    # (e.g. OpenSSH) manage their own format(s).  Please see
+                    # #20708 for a discussion about how to handle SSH key files
+                    # in the future
                     keys[kname] = _fh.readline()
-                    # only read the whole file if it is not in the legacy 1.1 binary format
+                    # only read the whole file if it is not in the legacy 1.1
+                    # binary format
                     if keys[kname] != "SSH PRIVATE KEY FILE FORMAT 1.1\n":
                         keys[kname] += _fh.read()
                     keys[kname] = keys[kname].strip()
@@ -445,7 +454,8 @@ def rm_auth_key_from_file(user,
              saltenv='base',
              env=None):
     '''
-    Remove an authorized key from the specified user's authorized key file, using a file as source
+    Remove an authorized key from the specified user's authorized key file,
+    using a file as source
 
     CLI Example:
 
@@ -1085,9 +1095,9 @@ def user_keys(user=None, pubfile=None, prvfile=None):
         salt '*' ssh.user_keys user=user1 prvfile=False
         salt '*' ssh.user_keys user="['user1','user2'] pubfile=id_rsa.pub prvfile=id_rsa
 
-    As you can see you can tell Salt not to read from the user's private (or public) key file by setting the
+    As you can see you can tell Salt not to read from the user's private (or
-    file path to ``False``. This can be useful to prevent Salt from publishing private data via Salt Mine or
+    public) key file by setting the file path to ``False``. This can be useful
-    others.
+    to prevent Salt from publishing private data via Salt Mine or others.
     '''
     if not user:
         user = __salt__['user.list_users']()
@@ -1111,7 +1121,8 @@ def user_keys(user=None, pubfile=None, prvfile=None):
             userKeys.append(pubfile)
         elif pubfile is not False:
             # Add the default public keys
-            userKeys += ['id_rsa.pub', 'id_dsa.pub', 'id_ecdsa.pub', 'id_ed25519.pub']
+            userKeys += ['id_rsa.pub', 'id_dsa.pub',
+                         'id_ecdsa.pub', 'id_ed25519.pub']
 
         if prvfile:
             userKeys.append(prvfile)
@@ -1188,3 +1199,36 @@ def _hostname_and_port_to_ssh_hostname(hostname, port=DEFAULT_SSH_PORT):
         return hostname
     else:
         return '[{0}]:{1}'.format(hostname, port)
+
+
+def key_is_encrypted(key):
+    '''
+    .. versionadded:: 2015.8.7
+
+    Function to determine whether or not a private key is encrypted with a
+    passphrase.
+
+    Checks key for a ``Proc-Type`` header with ``ENCRYPTED`` in the value. If
+    found, returns ``True``, otherwise returns ``False``.
+
+    CLI Example:
+
+    .. code-block:: bash
+
+        salt '*' ssh.key_is_encrypted /root/id_rsa
+    '''
+    try:
+        with salt.utils.fopen(key, 'r') as fp_:
+            key_data = fp_.read()
+    except (IOError, OSError) as exc:
+        # Raise a CommandExecutionError
+        salt.utils.files.process_read_exception(exc, key)
+
+    is_private_key = re.search(r'BEGIN (?:\w+\s)*PRIVATE KEY', key_data)
+    is_encrypted = 'ENCRYPTED' in key_data
+    del key_data
+
+    if not is_private_key:
+        raise CommandExecutionError('{0} is not a private key'.format(key))
+
+    return is_encrypted

salt/states/git.py

@@ -123,6 +123,20 @@ def _fail(ret, msg, comments=None):
     return ret
 
 
+def _failed_fetch(ret, exc, comments=None):
+    msg = (
+        'Fetch failed. Set \'force_fetch\' to True to force the fetch if the '
+        'failure was due to it being non-fast-forward. Output of the fetch '
+        'command follows:\n\n{0}'.format(_strip_exc(exc))
+    )
+    return _fail(ret, msg, comments)
+
+
+def _failed_submodule_update(ret, exc, comments=None):
+    msg = 'Failed to update submodules: ' + _strip_exc(exc)
+    return _fail(ret, msg, comments)
+
+
 def _not_fast_forward(ret, pre, post, branch, local_branch, comments):
     return _fail(
         ret,
@@ -274,7 +288,42 @@ def latest(name,
         with tags or revision IDs.
 
     identity
-        A path on the minion server to a private key to use over SSH
+        Path to a private key to use for ssh URLs. This can be either a single
+        string, or a list of strings. For example:
+
+        .. code-block:: yaml
+
+            # Single key
+            git@github.com:user/repo.git:
+              git.latest:
+                - user: deployer
+                - identity: /home/deployer/.ssh/id_rsa
+
+            # Two keys
+            git@github.com:user/repo.git:
+              git.latest:
+                - user: deployer
+                - identity:
+                  - /home/deployer/.ssh/id_rsa
+                  - /home/deployer/.ssh/id_rsa_alternate
+
+        If multiple keys are specified, they will be tried one-by-one in order
+        for each git command which needs to authenticate.
+
+        .. warning::
+
+            Unless Salt is invoked from the minion using ``salt-call``, the
+            key(s) must be passphraseless. For greater security with
+            passphraseless private keys, see the `sshd(8)`_ manpage for
+            information on securing the keypair from the remote side in the
+            ``authorized_keys`` file.
+
+            .. _`sshd(8)`: http://www.man7.org/linux/man-pages/man8/sshd.8.html#AUTHORIZED_KEYS_FILE%20FORMAT
+
+        .. versionchanged:: 2015.8.7
+            Salt will no longer attempt to use passphrase-protected keys unless
+            invoked from the minion using ``salt-call``, to prevent blocking
+            waiting for user input.
 
     https_user
         HTTP Basic Auth username for HTTPS (only) clones
@@ -952,18 +1001,7 @@ def latest(name,
                             user=user,
                             identity=identity)
                     except CommandExecutionError as exc:
-                        msg = 'Fetch failed'
+                        return _failed_fetch(ret, exc, comments)
-                        if isinstance(exc, CommandExecutionError):
-                            msg += (
-                                '. Set \'force_fetch\' to True to force '
-                                'the fetch if the failure was due to it '
-                                'bein non-fast-forward. Output of the '
-                                'fetch command follows:\n\n'
-                            )
-                            msg += _strip_exc(exc)
-                        else:
-                            msg += ':\n\n' + str(exc)
-                        return _fail(ret, msg, comments)
                     else:
                         if fetch_changes:
                             comments.append(
@@ -1117,11 +1155,15 @@ def latest(name,
                 # TODO: Figure out how to add submodule update info to
                 # test=True return data, and changes dict.
                 if submodules:
-                    __salt__['git.submodule'](target,
+                    try:
-                                              'update',
+                        __salt__['git.submodule'](
-                                              opts=['--init', '--recursive'],
+                            target,
-                                              user=user,
+                            'update',
-                                              identity=identity)
+                            opts=['--init', '--recursive'],
+                            user=user,
+                            identity=identity)
+                    except CommandExecutionError as exc:
+                        return _failed_submodule_update(ret, exc, comments)
             elif bare:
                 if __opts__['test']:
                     msg = (
@@ -1141,18 +1183,7 @@ def latest(name,
                         user=user,
                         identity=identity)
                 except CommandExecutionError as exc:
-                    msg = 'Fetch failed'
+                    return _failed_fetch(ret, exc, comments)
-                    if isinstance(exc, CommandExecutionError):
-                        msg += (
-                            '. Set \'force_fetch\' to True to force '
-                            'the fetch if the failure was due to it '
-                            'bein non-fast-forward. Output of the '
-                            'fetch command follows:\n\n'
-                        )
-                        msg += _strip_exc(exc)
-                    else:
-                        msg += ':\n\n' + str(exc)
-                    return _fail(ret, msg, comments)
                 else:
                     comments.append(
                         'Bare repository at {0} was fetched{1}'.format(
@@ -1260,13 +1291,18 @@ def latest(name,
             # We're cloning a fresh repo, there is no local branch or revision
             local_branch = local_rev = None
 
-            __salt__['git.clone'](target,
+            try:
-                                  name,
+                __salt__['git.clone'](target,
-                                  user=user,
+                                      name,
-                                  opts=clone_opts,
+                                      user=user,
-                                  identity=identity,
+                                      opts=clone_opts,
-                                  https_user=https_user,
+                                      identity=identity,
-                                  https_pass=https_pass)
+                                      https_user=https_user,
+                                      https_pass=https_pass)
+            except CommandExecutionError as exc:
+                msg = 'Clone failed: {0}'.format(_strip_exc(exc))
+                return _fail(ret, msg, comments)
+
             ret['changes']['new'] = name + ' => ' + target
             comments.append(
                 '{0} cloned to {1}{2}'.format(
@@ -1376,11 +1412,14 @@ def latest(name,
                         comments.append(upstream_action)
 
             if submodules and remote_rev:
-                __salt__['git.submodule'](target,
+                try:
-                                          'update',
+                    __salt__['git.submodule'](target,
-                                          opts=['--init', '--recursive'],
+                                              'update',
-                                          user=user,
+                                              opts=['--init', '--recursive'],
-                                          identity=identity)
+                                              user=user,
+                                              identity=identity)
+                except CommandExecutionError as exc:
+                    return _failed_submodule_update(ret, exc, comments)
 
             try:
                 new_rev = __salt__['git.revision'](

salt/templates/git/ssh-id-wrapper

@@ -1,6 +1,18 @@
-#!/bin/sh
 OPTS="-oStrictHostKeyChecking=no -oPasswordAuthentication=no -oKbdInteractiveAuthentication=no -oChallengeResponseAuthentication=no"
-if [ ! -z "$GIT_IDENTITY" ]; then
+if test -n "$GIT_IDENTITY"; then
     OPTS="$OPTS -i $GIT_IDENTITY"
+    # Check /etc/ssh and /usr/local/etc/ssh for the global ssh_config
+    if test -e "/etc/ssh/ssh_config"; then
+        OVERRIDE_SSH_CONFIG="/etc/ssh/ssh_config"
+    elif test -e "/usr/local/etc/ssh/ssh_config"; then
+        OVERRIDE_SSH_CONFIG="/usr/local/etc/ssh/ssh_config"
+    else
+        OVERRIDE_SSH_CONFIG=""
+    fi
+    # If a global ssh_config was found, add it to the opts. This keeps the
+    # user's ~/.ssh/config from overriding the "-i" param.
+    if test -n "$OVERRIDE_SSH_CONFIG"; then
+        OPTS="$OPTS -F $OVERRIDE_SSH_CONFIG"
+    fi
 fi
 exec ssh $OPTS "$@"

salt/utils/files.py

@@ -112,3 +112,21 @@ def rename(src, dst):
                     )
                 )
         os.rename(src, dst)
+
+
+def process_read_exception(exc, path):
+    '''
+    Common code for raising exceptions when reading a file fails
+    '''
+    if exc.errno == errno.ENOENT:
+        raise CommandExecutionError('{0} does not exist'.format(path))
+    elif exc.errno == errno.EACCES:
+        raise CommandExecutionError(
+            'Permission denied reading from {0}'.format(path)
+        )
+    else:
+        raise CommandExecutionError(
+            'Error {0} encountered reading from {1}: {2}'.format(
+                exc.errno, path, exc.strerror
+            )
+        )