Bug 803217 - scm get does not respect SSH_AUTH_SOCK environment variable
scm get does not respect SSH_AUTH_SOCK environment variable
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: mock (Show other bugs)
rawhide
All All
unspecified Severity medium
: ---
: ---
Assigned To: Clark Williams
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-14 04:55 EDT by Christian Albrecht
Modified: 2012-06-05 14:27 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-05-26 03:37:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
possible fix to scm environment issue (2.50 KB, patch)
2012-03-17 11:06 EDT, Clark Williams
no flags Details | Diff
Preserve SSH_AUTH_SOCK for SCM (1.63 KB, text/plain)
2012-03-20 08:46 EDT, Marko Myllynen
no flags Details
updated possible fix to scm environment issue (3.28 KB, patch)
2012-03-23 15:59 EDT, Clark Williams
no flags Details | Diff
Final (hopefully) scm environment fix (3.72 KB, patch)
2012-03-26 11:03 EDT, Clark Williams
no flags Details | Diff
Configure environment from command line (2.21 KB, patch)
2012-05-29 07:13 EDT, Max Romanov
no flags Details | Diff

  None (edit)
Description Christian Albrecht 2012-03-14 04:55:56 EDT
Description of problem:

I connect from my workstation via ssh to a build host and checkout the sources from a non-local git repository. So far so good, I have no problem when I do "git clone git@repository.example.com:my_package.git my_package" by hand. When I use mock in combination with the scm option I receive an error.


[calbrech@rhel6-dev-mock ~]$ mock -v --scm-enable --scm-option package=my_package
DEBUG: Initializing SCM integration...
DEBUG: SCM checkout command: git clone  git@repository.example.com:my_package.git my_package
DEBUG: SCM checkout post command: git checkout master
DEBUG: SCM checkout directory: /tmp/tmp3Y65mA.mock-scm.my_package
DEBUG: Executing command: ['git', 'clone', 'git@repository.example.com:my_package.git', 'my_package']
DEBUG: Initialized empty Git repository in /tmp/tmp3Y65mA.mock-scm.my_package/my_package/.git/
DEBUG: Permission denied, please try again.
DEBUG: Permission denied, please try again.
DEBUG: Permission denied (publickey,gssapi-with-mic,password).
DEBUG: fatal: The remote end hung up unexpectedly
DEBUG: Child returncode was: 128
ERROR: Command failed. See logs for output.
 # ['git', 'clone', 'git@repository.example.com:my_package.git', 'my_package']



Version-Release number of selected component (if applicable):

I have installed mock-1.1.21-1.el6.noarch from EPEL


Possible solution:


diff --git a/py/mockbuild/scm.py b/py/mockbuild/scm.py
index 10ce46f..2610aa8 100644
--- a/py/mockbuild/scm.py
+++ b/py/mockbuild/scm.py
@@ -83,7 +83,7 @@ class scmWorker(object):
         # non-interactively
         old_home = os.environ['HOME']
         os.environ['HOME'] = pwd.getpwuid(os.getuid()).pw_dir
-        mockbuild.util.do(shlex.split(self.get), shell=False, cwd=self.wrk_dir)
+        mockbuild.util.do(shlex.split(self.get), env=os.environ, shell=False, cwd=self.wrk_dir)
         if self.postget:
             mockbuild.util.do(shlex.split(self.postget), shell=False, cwd=self.src_dir)
         os.environ['HOME'] = old_home


This patch works for me. To access a git repository via ssh public key authentification you need pass the SSH_AUTH_SOCK environment variable to the git host.
Comment 1 Marko Myllynen 2012-03-16 07:04:13 EDT
> To access a git repository via ssh public key authentification you need pass
> the SSH_AUTH_SOCK environment variable to the git host.

This is something that was discussed in the past but IIRC we never quite conclude what would be the best approach. Currently there is the following in scm.py:

os.environ['SSH_AUTH_SOCK'] = pwd.getpwuid(os.getuid()).pw_dir + "/.ssh/auth_sock"

So if you have $HOME/.ssh/auth_sock then it works already. However, this of course requires manual setup and your patch would get rid of that, being much more user friendly.

Clark, do you remember anything related to this, does the above patch look safe security-wise? Iff so, I think we should update it to remove the .ssh/auth_sock line and apply.

Thanks.
Comment 2 Christian Albrecht 2012-03-16 07:57:01 EDT
If you pass an empty env dict to mockbuild.util.do() the whole environment is cleared in util.py.

I've tried to remove the .ssh/auth_sock line in scm.py, but it seems that the environment is cleared somewhere else before the mock.build.do() is called. So it makes sense to have the ~/.ssh/auth_sock environment variable set in scm.py.
Comment 3 Clark Williams 2012-03-16 13:42:24 EDT
(In reply to comment #1)
> 
> Clark, do you remember anything related to this, does the above patch look safe
> security-wise? Iff so, I think we should update it to remove the .ssh/auth_sock
> line and apply.

Marko, I don't think we ever came to an agreement on how to handle it. But (as Christian states in #c2), there are still places lurking where we nuke the environment.

I think our best bet is to track down places where we're clearing the environment and replace them with self.env, then make sure that self.env has what we want for SSH_AUTH_SOCK.
Comment 4 Clark Williams 2012-03-17 11:06:16 EDT
Created attachment 570811 [details]
possible fix to scm environment issue

Had a thought while I was sitting drinking coffee :)

What do you guys think of this where we copy the environ in the SCM constructor, change it to suit and use it always when we're doing SCM operations?

This is completely untested, just wanted to see what you guys thought.
Comment 5 Christian Albrecht 2012-03-19 05:38:13 EDT
This is a good approach.

Because os.environ always return a dict, I would use os.environ.copy() instead of copy.copy(). Both methods would return a shallow copy of the dict. And we keep the imports as short as possible.

I test your patch this afternoon.
Comment 6 Marko Myllynen 2012-03-19 06:04:35 EDT
Yes, this looks like an improvement but for SSH_AUTH_SOCK I wonder would it be better to add SSH_AUTH_SOCK to KEEP_ENV_VARS in /etc/security/console.apps/mock?
Comment 7 Christian Albrecht 2012-03-19 08:42:53 EDT
Clark, your patch worked well for me (tested with both copy.copy() and os.environ.copy()).

Marko, I have tried your suggestion from #c6 too. It doesn't work for me. But there is a chance that I made some mistake :)
Comment 8 Marko Myllynen 2012-03-19 09:18:44 EDT
(In reply to comment #7)
> Marko, I have tried your suggestion from #c6 too. It doesn't work for me. But
> there is a chance that I made some mistake :)

Try using the following in /etc/security/console.apps/mock and uncommenting the SSH_AUTH_SOCK line in scm.py:

KEEP_ENV_VARS=COLUMNS,SSH_AUTH_SOCK
Comment 9 Christian Albrecht 2012-03-19 10:36:32 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > Marko, I have tried your suggestion from #c6 too. It doesn't work for me. But
> > there is a chance that I made some mistake :)
> 
> Try using the following in /etc/security/console.apps/mock and uncommenting the
> SSH_AUTH_SOCK line in scm.py:
> 
> KEEP_ENV_VARS=COLUMNS,SSH_AUTH_SOCK

Thats what I have done in my first try. 

I had to pass the environment dict to mockbuild.util.do() again to get it working. With this approach the ~/.ssh/auth_sock hack isn't needed anymore.
Comment 10 Clark Williams 2012-03-19 10:50:56 EDT
(In reply to comment #5)
> This is a good approach.
> 
> Because os.environ always return a dict, I would use os.environ.copy() instead
> of copy.copy(). Both methods would return a shallow copy of the dict. And we
> keep the imports as short as possible.

Doh! I completely forgot that dictionaries have a copy() method. Good catch. 

I've added this to my work branch and I'll see about pushing it to testing this week.
Comment 11 Clark Williams 2012-03-19 10:54:47 EDT
(In reply to comment #8)
> (In reply to comment #7)
> > Marko, I have tried your suggestion from #c6 too. It doesn't work for me. But
> > there is a chance that I made some mistake :)
> 
> Try using the following in /etc/security/console.apps/mock and uncommenting the
> SSH_AUTH_SOCK line in scm.py:
> 
> KEEP_ENV_VARS=COLUMNS,SSH_AUTH_SOCK

Should we add this to etc/consolehelper/mock?
Comment 12 Marko Myllynen 2012-03-20 08:46:50 EDT
Created attachment 571387 [details]
Preserve SSH_AUTH_SOCK for SCM

> Should we add this to etc/consolehelper/mock?

Yes, please see the attached patch. It also drops SSH_AUTH_SOCK right after SCM has grabbed it (strictly speaking probably not necessary at least currently but better safe than sorry).
Comment 13 Clark Williams 2012-03-21 11:21:25 EDT
Hmmm.

Since we're unconditionally setting HOME in our local environment copy, shouldn't we be moving SSH_AUTH_SOCK as well? I'm not sure we should drop setting SSH_AUTH_SOCK in scm.py...
Comment 14 Clark Williams 2012-03-21 12:28:34 EDT
(In reply to comment #13)
> Hmmm.
> 
> Since we're unconditionally setting HOME in our local environment copy,
> shouldn't we be moving SSH_AUTH_SOCK as well? I'm not sure we should drop
                  ^^^^^ 
That's "setting" not moving. 

> setting SSH_AUTH_SOCK in scm.py...
Comment 15 Marko Myllynen 2012-03-22 03:29:02 EDT
> I'm not sure we should drop setting SSH_AUTH_SOCK in scm.py...

The idea was to preserve SSH_AUTH_SOCK in the environment passed to mock and thus copy it in scm.py alongside with other variables. So if it's there we'll get it automatically. I think it could be dropped but if you have some scenario in mind where the hard-coded value would be useful then SSH_AUTH_SOCK could be set to it conditionally.
Comment 16 Clark Williams 2012-03-22 13:27:46 EDT
ahh, I misunderstood the intent. My thought was to use the value of SSH_AUTH_SOCK from the containing environment if it's there and to hard-code it if it's not, but upon reflection maybe hard-coding a value to be in the home directory of the account running mock is not the best idea. 

Here's what I have currently:


diff --git a/py/mockbuild/scm.py b/py/mockbuild/scm.py
index e93e39e..22d89d2 100644
--- a/py/mockbuild/scm.py
+++ b/py/mockbuild/scm.py
@@ -76,7 +76,8 @@ class scmWorker(object):
         # non-interactively
         self.environ['HOME'] = pwd.getpwuid(os.getuid()).pw_dir
         self.environ['CVS_RSH'] = "ssh"
-        self.environ['SSH_AUTH_SOCK'] = pwd.getpwuid(os.getuid()).pw_dir + "/.ssh/auth_sock"
+        if not self.environ.has_key('SSH_AUTH_SOCK'):
+            self.environ['SSH_AUTH_SOCK'] = pwd.getpwuid(os.getuid()).pw_dir + "/.ssh/auth_sock"
 
     decorate(traceLog())
     def get_sources(self):

But I'm not sure that it makes sense to add it if it's not in the original environment. 

Your thoughts?
Comment 17 Clark Williams 2012-03-23 15:59:53 EDT
Created attachment 572370 [details]
updated possible fix to scm environment issue

Here's what I have currently for this BZ. Please look this over and let me know if further changes are needed.
Comment 18 Marko Myllynen 2012-03-26 04:26:15 EDT
Looks good though I'd still drop SSH_AUTH_SOCK in mock.py just in case.

Thanks.
Comment 19 Clark Williams 2012-03-26 10:52:17 EDT
Oh, you mean in os.environ, after we make the copy? Yeah that makes sense.
Comment 20 Clark Williams 2012-03-26 11:03:29 EDT
Created attachment 572777 [details]
Final (hopefully) scm environment fix

Ok, here's what I've got in my work branch. I'll try and get a new mock spun up today or tomorrow for testing.
Comment 21 Fedora Update System 2012-03-30 14:31:26 EDT
mock-1.1.22-1.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/mock-1.1.22-1.el6
Comment 22 Fedora Update System 2012-03-30 14:32:01 EDT
mock-1.1.22-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/mock-1.1.22-1.fc15
Comment 23 Fedora Update System 2012-03-30 14:32:26 EDT
mock-1.0.29-1.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/mock-1.0.29-1.el5
Comment 24 Fedora Update System 2012-03-30 14:32:53 EDT
mock-1.1.22-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mock-1.1.22-1.fc17
Comment 25 Fedora Update System 2012-03-30 14:33:39 EDT
mock-1.1.22-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/mock-1.1.22-1.fc16
Comment 26 Fedora Update System 2012-05-04 10:31:10 EDT
mock-1.1.22-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/mock-1.1.22-2.fc17
Comment 27 Fedora Update System 2012-05-04 18:17:53 EDT
Package mock-1.1.22-2.fc17:
* should fix your issue,
* was pushed to the Fedora 17 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing mock-1.1.22-2.fc17'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-7324/mock-1.1.22-2.fc17
then log in and leave karma (feedback).
Comment 28 Fedora Update System 2012-05-26 03:37:01 EDT
mock-1.1.22-2.1.fc17 has been pushed to the Fedora 17 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Max Romanov 2012-05-29 05:16:51 EDT
mock-1.0.29-1.el5 on RedHat 5.8 still have this issue because usermode-1.88-3.el5.2 ignores KEEP_ENV_VARS option.
Comment 30 Max Romanov 2012-05-29 07:13:52 EDT
Created attachment 587386 [details]
Configure environment from command line

This patch adds an ability to configure environment variables using command-line switch --environment. The main use case is to pass SSH_AUTH_SOCK value when usermode package is too old to understand KEEP_ENV_VARS option: --environment SSH_AUTH_SOCK=$SSH_AUTH_SOCK
Comment 31 Marko Myllynen 2012-06-05 09:22:26 EDT
Hi Max, thanks for your patch suggestion - it'd be best if you could file a new BZ against mock/EPEL5 to have it properly reviewed.
Comment 32 Max Romanov 2012-06-05 14:27:40 EDT
Marko, thanks for advise. Patch is attached to new bug 828987.

Note You need to log in before you can comment on or make changes to this bug.