Copy git ssh-id-wrapper to /tmp only if necessary (Fixes #10582, #19532)

[mtorromeo]

This adds a check that only copies the ssh wrapper to a temporary location if git is to be run by a specific user and at the same time the predefined wrapper file is not executable by "others" by verifying every path part for the others executable bit.

By doing this the temp file is kept only as a last resort which should work with salt-ssh as per bug #19532, while avoiding a needless copy on /tmp which could be potentially mounted with noexec as per bug #10582.

[mtorromeo]

It would be nice to have this also backported to the stable branches if the fix is good.

Thanks!

[terminalmage]

One small nitpick, though functionally this looks good.

[terminalmage]

EAFP here. We can skip stat-ing the file by doing it like this:

try:
    __salt__['file.remove'](tmp_ssh_wrapper)
    log.debug('Removed ssh wrapper file %s', tmp_ssh_wrapper)
except AttributeError:
    # No wrapper was used
    pass
except (SaltInvocationError, CommandExecutionError) as exc:
    log.warning('Failed to remove ssh wrapper file %s: %s', tmp_ssh_wraper, exc)

[mtorromeo]

I can't say I like calling a function with an obviously wrong parameter. I would have kept at least a check if tmp_ssh_wrapper which does not require any syscall and would avoid a function call with consequent exception catch but I am happy to oblige.

Note that I wrote it this way because that's exactly how tmp_identity_file is removed a couple of lines below. Should I change that too?

[terminalmage]

I can't say I like calling a function with an obviously wrong parameter.

That's what exception handling is for.

https://en.wikipedia.org/wiki/Python_syntax_and_semantics#Exceptions

[mtorromeo]

PR updated

[rallytime]

@mtorromeo There is one lint error here. Can you fix it? https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/15309/violations/file/salt/modules/git.py/

Then we can get this in.

[mtorromeo]

Oops, I copy-pasted the suggested change without checking the syntax first, sorry.
It should be fine now.

[poliva83]

@mtorromeo @rallytime @terminalmage This change is introduced with 2017.7.3 and has caused regression in the git module for windows hosts. This is because line https://github.com/saltstack/salt/pull/43769/files#diff-59a19c9338fb59c2d90efccd4a0d618fR298 calls __salt__['file.remove'] passing in None. This causes an uncaught TypeError exception to occur here https://github.com/saltstack/salt/blob/v2017.7.3/salt/modules/win_file.py#L1059. You can produce the same error pretty easy on a windows host (see below).

PS C:\Users\olivap5> C:\salt\bin\python.exe
Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> os.path.expanduser(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
NameError: name 'os' is not defined
>>> import os
>>> os.path.expanduser(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\salt\bin\lib\ntpath.py", line 289, in expanduser
    if path[:1] != '~':
TypeError: 'NoneType' object has no attribute '__getitem__'

[mtorromeo]

I had a check on tmp_ssh_wrapper in place but I was asked to remove it in the code review #43769 (review)

I still prefer to use an if tmp_ssh_wrapper: but the alternative is to also catch the TypeError exception