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

Allow setting per-repo SSH key #234

Closed
teeberg opened this issue Jan 18, 2015 · 15 comments
Closed

Allow setting per-repo SSH key #234

teeberg opened this issue Jan 18, 2015 · 15 comments

Comments

@teeberg
Copy link
Contributor

teeberg commented Jan 18, 2015

I've locally patched this by temporarily setting the GIT_SSH environment variable to a script that would include a deployment SSH key in the SSH command. I think this would be awesome to have built-in. I find this much more convenient and flexible than having to parse and update the ~/.ssh/config.

Would there be interest in a pull request for this feature?

@Byron Byron added this to the v1.0.0 milestone Jan 19, 2015
@Byron
Copy link
Member

Byron commented Jan 19, 2015

Of course, I would be looking forward to receiving one.
I will assign this ticket to the 'far in the future' release until I receive a PR or get around to implementing this myself.
Version 0.3.6 could be released this week, and your PR could be part of it if it makes it here in time.
Thank you

@teeberg
Copy link
Contributor Author

teeberg commented Jan 20, 2015

Glad to hear that. I'm looking into it! :)

Right now, I'm trying to figure out what would be the best syntax to support it. Should it be a keyword argument to fetch/pull/clone/push?

remote.pull(sshkey=GIT_SSH_KEY)

or

Repo.clone_from(git_url, repo_dir, sshkey=GIT_SSH_KEY)

Or a property of Remote that gets passed in upon instantiation? If you need it for either of fetch/pull/clone/push, then you will need it for all other commands too, so the latter seems more appropriate. But remote objects may not have been initialized by the user, so what do we do if it wasn't?

In my own class, I've made it a context manager that temporarily injects GIT_SSH into the environment, something like:

with repo.git.environment(sshkey=GIT_SSH_KEY):
    remote.pull()

but this may get repetitive really fast...

Being much more familiar with the codebase than I am, what's your favorite/opinion/recommendation? I'm also open to other proposals and implementation suggestions!

@Byron
Copy link
Member

Byron commented Jan 20, 2015

From what I see, you have pinpointed the options precisely already !

If your envisioned implementation indeed sets environment variables that are to be inherited by called git processes, it might be most comfortable to set it up where it happens, in the Git type.

In that case, the API could look like this:

r = git.Repo('repo_dir')
previous_env = r.git.set_environment(dict(GIT_SSH_KEY="I really don't know what to pass here ;)"))
# Now all upcoming git calls will have the GIT_SSH_KEY environment variable set, along with all the other default ones.
# Returning the previous version of the dict (which may be empty) helps to change environments on the fly, for example with a custom context manager.

As you can see, my suggestion is very generic, but should be just what you need to get GIT_SSH_KEY to work.

Additionally, it would be great if you could add a small tutorial to the respective sphinx rst file, along with a test for the example code in test_docs.py .

I hope that makes sense, please let me know if there are any other questions or concerns, I will be happy to help.

@teeberg
Copy link
Contributor Author

teeberg commented Jan 21, 2015

How it works is: The GIT_SSH environment variable points to an executable (e.g. a python script) that will be used instead of ssh. This script then simply calls ssh, passing the SSH key and all passed options. That is, given environment variables like

GIT_SSH_KEY='/home/user/gitrepo/deployment_key'
GIT_SSH='/usr/local/lib/python2.7/dist-packages/git/scripts/ssh_with_key.py'

the mentioned $GIT_SSH script (ssh_with_key.py) would pretty much be:

ssh_options = ['-i', os.environ['GIT_SSH_KEY']]
return subprocess.call(['ssh'] + ssh_options + sys.argv[1:])

git uses the complete value of GIT_SSH as the program name, so you can't just dynamically set it to ssh -i $GIT_SSH_KEY. Hence the need for an extra script to help out.

What I like about a context manager is that it wouldn't affect other remotes if you are operating on more than one. But maybe we could just have both options, where the context manager uses the API that you're suggesting. :-)

@Byron
Copy link
Member

Byron commented Jan 21, 2015

Thanks for the heads-up !
In that case, I definitely put my votes on the general environment API I previously proposed, along with the said configuration manager. The latter would just swap environments accordingly.

I am looking forward to your PR ! No pressure, but I intend to make a new release today or tomorrow, and unless you don't have some code already, I might just implement this one for you if you don't mind. It seems like a thing of 45 minutes to me.

@Byron Byron modified the milestones: v0.3.6 - Features, v1.0.0 Jan 21, 2015
@teeberg
Copy link
Contributor Author

teeberg commented Jan 21, 2015

I'm happy to let you implement it :-) You surely have a better feeling of where everything should go! Here's what I thought how set_environment could work:

def set_environment(self, **kwargs):
    old_env = {}
    for key, value in kwargs.iteritems():
        # set value if it is None
        if value is not None:
            if key in os.environ:
                old_env[key] = os.environ[key]
            else:
                old_env[key] = None
            os.environ[key] = value
        # remove key from environment if its value is None
        elif key in os.environ:
            old_env[key] = os.environ[key]
            del os.environ[key]
    return old_env

With that function, the context manager would be as easy as doing:

@contextlib.contextmanager
def patch_environment(self, **kwargs):
    old_env = self.set_environment(**kwargs)
    yield
    self.set_environment(old_env)

For testing it, you could generate a new SSH key and set it as the deployment key for this repository. Or that's the path I would have taken. :-)

@Byron
Copy link
Member

Byron commented Jan 21, 2015

Hmm, why don't we work on this so you can put this code up into a branch which I can use as a basis. That way you can get your name into the project and get some more reward for your time :) !

The main issue I take here is that the os.environ is off the charts for the "we don't change globals" reason. The additional environment variables could be stored in a member variable though, which gets merged into the process environment about here, overwriting existing values if necessary.

If you can manage to post your branch to be used say, until tomorrow, I will definitely merge it in. That's the timeframe I expect myself to be done with all other tickets.

Good luck !

@teeberg
Copy link
Contributor Author

teeberg commented Jan 21, 2015

Sounds good too, I pretty much have all the pieces together. :-) Then you can concentrate on other tickets!

@Byron
Copy link
Member

Byron commented Jan 22, 2015

You can watch the archived live-stream on youtube

Byron added a commit that referenced this issue Jan 22, 2015
However, I kept information on how to achieve the same thing with
`custom_environment()` in the test.

Related to #234
@teeberg
Copy link
Contributor Author

teeberg commented Feb 9, 2015

Turns out, starting with git 2.3, this is pretty much built-in. Using the new GIT_SSH_COMMAND, you can specify the ssh -i ... command now directly, rather than relying on an external script. :-) And even better, with the environment context manager, it's fully supported too! :-)

@Byron
Copy link
Member

Byron commented Feb 9, 2015

I am glad to hear that ! In a way it seems it was good that setuptools refused to make that extra script executable - it would have been unnecessary special-case baggage after all.
Happy End ... (? ;) )

@teeberg
Copy link
Contributor Author

teeberg commented Feb 9, 2015

Yeah it was quite special-case, no hard feelings. :-)

@Byron
Copy link
Member

Byron commented Mar 2, 2015

@teeberg Maybe you can chime in on this question on stackoverflow. He keeps posting in the comments of my answer though - maybe it's worth a new question so you can earn some points as well :).

@Byron
Copy link
Member

Byron commented Mar 2, 2015

@teeberg Here is the new question - maybe you know what's happening there.

@teeberg
Copy link
Contributor Author

teeberg commented Mar 2, 2015

I'll look into it :-) Thanks for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants