Skip to content
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

Fix unreachable ssh-id-wrapper template when root_dir is relative #48545

Merged
merged 1 commit into from Aug 21, 2018

Conversation

Projects
None yet
5 participants
@bbinet
Copy link
Contributor

commented Jul 12, 2018

Since the git commands will be executed from the user home directory, if
salt root_dir is set to a relative path (for example using root_dir: ./.tmp/ in salt config),
then the git/ssh-id-wrapper template may be unreachable and we may
encouter the following error that this patch is fixing:

[ERROR   ] Command '[u'git', u'ls-remote', u'git@github.com:bbinet/salt-formula-linux.git']' failed with return code: 128
[ERROR   ] stderr: error: cannot run .tmp/thin/py2/salt/templates/git/ssh-id-wrapper: No such file or directory
fatal: unable to fork
[ERROR   ] retcode: 128
[ERROR   ] Failed to check remote refs: Unable to authenticate using identity file:

error: cannot run .tmp/thin/py2/salt/templates/git/ssh-id-wrapper: No such file or directory
fatal: unable to fork

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Tests written?

Yes/No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@bbinet bbinet force-pushed the bbinet:fix-unreacheable-ssh-id-wrapper branch from 0f842b6 to 567a4aa Jul 12, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@bbinet Can you move this to the 2018.3 branch? The 2018.3.2 branch is closed for fixes at this time.

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

@rallytime Ok, will do

@bbinet bbinet changed the base branch from 2018.3.2 to 2018.3 Jul 12, 2018

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2018

@rallytime done

@rallytime rallytime requested a review from terminalmage Jul 12, 2018

@terminalmage
Copy link
Contributor

left a comment

Actually, I think we need to take a closer look here and make sure that root_dir can't be used to introduce a vulnerability.

@terminalmage terminalmage requested review from terminalmage and removed request for terminalmage Jul 13, 2018

@rallytime rallytime requested a review from thatch45 Jul 13, 2018

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@terminalmage But root_dir is a salt-master config param, so I guess the salt-master config file should be considered trusted (writable by admin only) ?

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@terminalmage @rallytime any feedback?

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

@thatch45 What do you think about this and @terminalmage's concerns?

@rallytime rallytime requested a review from cachedout Aug 13, 2018

@thatch45

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

I think that my main question here is that a relative root dir even works, that strikes me as a potential issue. I don't think that it will present a security issue.
All in all I am ok with this change, but I would rather pull support for a relative root dir.

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

@thatch45 relative root dir actually works perfectly (with this patch for version 2018.3): I'm using relative root dir in the following project: https://github.com/bbinet/salt-thin-setupify

I'm going to rebase this PR onto latest 2018.3 branch.

Fix unreachable ssh-id-wrapper template when root_dir is relative
Since the git commands will be executed from the user home directory, if
salt root_dir is set to a relative path (for example using
`root_dir: ./.tmp/` in salt config), then the git/ssh-id-wrapper
template may be unreachable and we may encouter the following error that
this patch is fixing:

```
[ERROR   ] Command '[u'git', u'ls-remote', u'git@github.com:bbinet/salt-formula-linux.git']' failed with return code: 128
[ERROR   ] stderr: error: cannot run .tmp/thin/py2/salt/templates/git/ssh-id-wrapper: No such file or directory
fatal: unable to fork
[ERROR   ] retcode: 128
[ERROR   ] Failed to check remote refs: Unable to authenticate using identity file:

error: cannot run .tmp/thin/py2/salt/templates/git/ssh-id-wrapper: No such file or directory
fatal: unable to fork
```

@bbinet bbinet force-pushed the bbinet:fix-unreacheable-ssh-id-wrapper branch from 3ce7f3c to 284dcf7 Aug 21, 2018

@bbinet

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

This is now rebased and ready to be merged.

@cachedout cachedout merged commit 60605f0 into saltstack:2018.3 Aug 21, 2018

5 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 running py3-ubuntu-1604...
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details

@bbinet bbinet deleted the bbinet:fix-unreacheable-ssh-id-wrapper branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.