Unset GIT_SSH_COMMAND before exec'ing git command

[mzagrabe]

Per: "man git", GIT_SSH_COMMAND will take precedence over GIT_SSH. Thus,
if GIT_SSH_COMMAND is set and leaks into the environment, then this
module's use of the GIT_SSH environment variable will not work.

Unset GIT_SSH_COMMAND for the environment in which the git command
execs.

[jcbollinger]

Are you sure that this PR fixes the right problem? I would be inclined to say that the problem is more general: not that this particular variable is leaking in, but that environment variables are leaking in from the agent's environment at all. The solution to that would be to take complete control of the environment of the git command (and to keep it as sparse as possible), not to exclude unwanted variables on a one-off basis every time a problem is discovered.

[mzagrabe]

I believe the problem is only manifesting itself when I run "puppet agent -t" via an interactive shell. I don't think the puppet daemon running is suffering environment contamination. The patch does fix the issue I was experiencing. I understand that a better solution might be to fully sanitize the environment. Let me know what you think.

…

-m

On Fri, Dec 6, 2019 at 8:12 AM John Bollinger ***@***.***> wrote: Are you sure that this PR fixes the right problem? I would be inclined to say that the problem is more general: that environment variables are leaking in from the agent's environment *at all*. The solution to that would be to take complete control of the environment of the git command (and to keep it as sparse as possible), not to exclude unwanted variables on a one-off basis every time a problem is discovered. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#435?email_source=notifications&email_token=ABBKKRVPG5X6A5X4RN7SQIDQXJMUFA5CNFSM4JV5SAP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGEGF2Y#issuecomment-562586347>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBKKRWFK7SQGAWL6L4EWCLQXJMUFANCNFSM4JV5SAPQ> .

[jcbollinger]

I believe the problem is only manifesting itself when I run "puppet agent
-t" via an interactive shell. I don't think the puppet daemon running is
suffering environment contamination.

I'm inclined to suspect that Puppet experiences exactly the same environment contamination whether run with --onetime (or with -t or --test, which imply --onetime) as it does when run without any of those options, as a daemon. If you haven't observed that then it is probably because the daemon is being started without the troublesome variable in its environment in the first place.

But that's beside the point, which is that the specific problem observed appears to be a symptom of a broader issue, and it would be better to solve that broader issue than to solve only this one special case. Otherwise, it is likely that some time in the future, substantially the same problem is going to need to be solved again.

The patch does fix the issue I was experiencing.

I don't doubt that it does, and that is an excellent way of establishing that you have characterized the problem correctly. Good work identifying the issue.

I understand that a better solution might be to fully sanitize the environment.

Then I really haven't anything more to say on the topic. I have no say in whether the PR is accepted -- I'm merely offering commentary. To be perfectly clear, however, my opinion is that the PR should be rejected in favor of a solution (not yet offered, to my knowledge) that resolves the issue by whitelisting environment variables or by exercising full control of the git environment.

On a separate topic, however, this is a Puppet-maintained module, so have you filed an issue against it in Puppet, Inc.'s bug tracker? Doing so will more effectively draw the attention of the right people.

[mzagrabe]

On Fri, Dec 6, 2019 at 10:42 AM John Bollinger ***@***.***> wrote: I believe the problem is only manifesting itself when I run "puppet agent -t" via an interactive shell. I don't think the puppet daemon running is suffering environment contamination. I'm inclined to suspect that Puppet experiences exactly the same environment contamination whether run with --onetime (or with -t or --test, which imply --onetime) as it does when run without any of those options, as a daemon. If you haven't observed that then it is probably because the daemon is being started without the troublesome variable in its environment in the first place.

I didn't observe it because I was doing all of my testing in an interactive environment. I never arranged puppet to run as a daemon and apply the catalog in such a way that would test the failure scenario experienced in the interactive environment.

But that's beside the point, which is that the specific problem observed appears to be a symptom of a broader issue, and it would be better to solve that broader issue than to solve only this one special case. Otherwise, it is likely that some time in the future, substantially the same problem is going to need to be solved again. The patch does fix the issue I was experiencing. I don't doubt that it does, and that is an excellent way of establishing that you have characterized the problem correctly. Good work identifying the issue. I understand that a better solution might be to fully sanitize the environment. Then I really haven't anything more to say on the topic. I have no say in whether the PR is accepted -- I'm merely offering commentary.

Thank you. I appreciate the commentary.

To be perfectly clear, however, my opinion is that the PR should be rejected in favor of a solution (not yet offered, to my knowledge) that resolves the issue by whitelisting environment variables or by exercising full control of the git environment.

My only concern with the whitelisting environment variables is "What is the definitive list of variables that need to be whitelisted in order for the git commands to work?" That seems like a larger and more disruptive change that is more challenging to test to see if all scenarios still work as expected. This PR is pretty minimal and fixes a bug. I guess if the more optimal solution is available, then advocating for rejecting this one is sensible, but if that patch never materializes, then the perfect is the enemy of the good.

On a separate topic, however, this is a Puppet-maintained module, so have you filed an issue against it in Puppet, Inc.'s bug tracker <https://tickets.puppetlabs.com/>?

Just did: https://tickets.puppetlabs.com/browse/MODULES-10245 Thanks!

…

[DavidS]

While @jcbollinger is right that other environment variables might also impact git's functioning, this is fixing an active problem, and there is nobody volunteering to fix the hypothetical bigger problem. I'll merge this for now.