authenticate using ssh agent

[kyriakosoikonomakos]

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]

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]

@carlosmn does that look better?

[carlosmn]

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.

[kyriakosoikonomakos]

@carlosmn Done.

[carlosmn]

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.

[ashb]

@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]

Thanks for this patch!

[ashb]

@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)?

[carlosmn]

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.

[ashb]

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.

[carlosmn]

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.

[ashb]

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")

[carlosmn]

Yeah, that should work just fine.

[ashb]

I've made this change now.

[carlosmn]

Great, 👍

[jdavid]

If it is good for @carlosmn it is good for me, could you merge @carlosmn ?