Skip to content

/ pygit2

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

authenticate using ssh agent #424

Merged
merged 2 commits into from Oct 10, 2014
Merged

authenticate using ssh agent #424

merged 2 commits into from Oct 10, 2014

Conversation

@kyriakosoikonomakos
Copy link
Contributor

kyriakosoikonomakos commented Sep 10, 2014

Introduce the ability for pygit2 to use the SSH agent for authentication.

Example use:
import pygit2
x = pygit2.credentials.SSHKeyFromAgent('git')
r = pygit2.clone_repository('ssh://git@github.com/some-org/some-repo', '/tmp/something', credentials=x)
@carlosmn
Copy link
Member

carlosmn commented Sep 10, 2014

Neat, but the naming doesn't match the ssh keypair constructor we already have. The class for the full ssh keypair is Keypair, and this is about getting that from the agent, so this should either be KeypairFromAgent or Keypair should change.

Have you considered having a static method in Keypair to act as a constructor, so we can use it like Keypair.from_agent('git') since this is about working with a Keypair as well?
@kyriakosoikonomakos
Copy link
Contributor Author

kyriakosoikonomakos commented Sep 11, 2014

@carlosmn does that look better?
@carlosmn
Copy link
Member

carlosmn commented Sep 11, 2014

Yeah. Though you took both of my suggestions when I meant them as alternatives. Having simply KeypairFromAgent without Keypair.from_agent() is fine.

What I was thinking a an alternative to that was having Keypair.from_agent() (as @staticmethod) return a new Keypair with None keys so the if check happens on the key fields rather than on the type.

But having the KeypairFromAgent is maybe clearer in its usage, so having just that without from_agent() would work better.
@vtemian vtemian mentioned this pull request Sep 15, 2014
Closed
0 of 3 tasks complete
@kyriakosoikonomakos
Copy link
Contributor Author

kyriakosoikonomakos commented Sep 15, 2014

@carlosmn Done.
pygit2/credentials.py Outdated
class KeypairFromAgent(Keypair):
def __init__(self, username):
self._username = username
super(KeypairFromAgent, self).__init__(username, None, None, None)

This comment has been minimized.

Sign in to view
@carlosmn

carlosmn Sep 15, 2014

Member

One of these lies is redundant. If we're going to rely on the superclass' constructor, then there's no need for us to store the username explicitly.
@kyriakosoikonomakos @ashb
authenticate using ssh agent
Loading status checks…
3e87ada
@ashb ashb force-pushed the kyriakosoikonomakos:ssh-agent branch from 4b9fe1c to 3e87ada Sep 15, 2014
@ashb
Copy link
Contributor

ashb commented Sep 15, 2014

@carlosmn Thanks

I've removed that redundant line and rebased everything down to one commit (my personal preference since the overall change is small. If we want it back the commit pre-force push was 4b9fe1c)
@jbiel
Copy link

jbiel commented Sep 24, 2014

Thanks for this patch!
@ashb
Copy link
Contributor

ashb commented Sep 25, 2014

@carlosmn Anything outstanding to get this merged or do is it just a matter of you having the cycles to take a look (I know that feeling)?
pygit2/remote.py Outdated
err = C.git_cred_ssh_key_new(ccred, to_bytes(name), to_bytes(pubkey),
to_bytes(privkey), to_bytes(passphrase))

if isinstance(creds, KeypairFromAgent):

This comment has been minimized.

Sign in to view
@carlosmn

carlosmn Sep 25, 2014

Member

I'd rather see this choose based on the values returned from .credential_tuple rather than based on inheritance. It's not too big a deal, but the idea behind these is that each instance tells us about what it's doing, rather than rely on the type.

This comment has been minimized.

Sign in to view
@ashb

ashb Sep 26, 2014

Contributor

We thought about this and weren't sure which way to go. The only thing useful we could detect is tuple[1:] are all None. We went with this way as this is what the Ruby libgit2 bindings do.

Happy to change this to trigger off pubkey & privkey being None instead if you'd prefer.

This comment has been minimized.

Sign in to view
@carlosmn

carlosmn Oct 6, 2014

Member

Yeah, that's how I'd go. This is designed so you can use your own classes which get the data from wherever you need (disc, the user, a database, whatever), and there's no need for them to hang off of these classes, all they need to do is have this attribute with the data we need.

This comment has been minimized.

Sign in to view
@ashb

ashb Oct 6, 2014

Contributor

So something like this diff then?

@@ -455,9 +456,13 @@ def get_credentials(fn, url, username, allowed):

     elif cred_type == C.GIT_CREDTYPE_SSH_KEY:
        name, pubkey, privkey, passphrase = creds.credential_tuple
-        err = C.git_cred_ssh_key_new(ccred, to_bytes(name), to_bytes(pubkey),
-                                     to_bytes(privkey), to_bytes(passphrase))
-
+        if pubkey is None and privkey is None:
+            err = C.git_cred_ssh_key_from_agent(ccred, to_bytes(name))
+        else:
+            err = C.git_cred_ssh_key_new(ccred, to_bytes(name),
+                                         to_bytes(pubkey), to_bytes(privkey),
+                                         to_bytes(passphrase))
     else:
         raise TypeError("unsupported credential type")

This comment has been minimized.

Sign in to view
@carlosmn

carlosmn Oct 7, 2014

Member

Yeah, that should work just fine.

This comment has been minimized.

Sign in to view
@ashb

ashb Oct 7, 2014

Contributor

I've made this change now.
@ashb
Use ssh_agent when pub+priv key are None, not based on the class
Loading status checks…
9aa39aa
@carlosmn
Copy link
Member

carlosmn commented Oct 7, 2014

Great, 👍
@jdavid
Copy link
Member

jdavid commented Oct 8, 2014

If it is good for @carlosmn it is good for me, could you merge @carlosmn ?
carlosmn added a commit that referenced this pull request Oct 10, 2014
@carlosmn
Merge pull request #424 from kyriakosoikonomakos/ssh-agent
Loading status checks…
fa20589 
authenticate using ssh agent
@carlosmn carlosmn merged commit fa20589 into libgit2:master Oct 10, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@KINFOO KINFOO mentioned this pull request Dec 2, 2014
Closed
@inmantaci inmantaci mentioned this pull request Sep 12, 2016
Closed
@The-Loeki The-Loeki mentioned this pull request Mar 14, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@kyriakosoikonomakos @carlosmn @ashb @jbiel @jdavid
You can’t perform that action at this time.