Re: [PATCH] contrib: add a pair of credential helpers for Mac OS X's keychain

From: John Szakmeister <john@szakmeister.net>
Date: Oct 1, 2011, 06:57 UTC
Message-ID: <CAEBDL5XhLAccfMoSyBjDA4ZsCgc4FnxEVYyqJsnGfzDW3Otudw@mail.gmail.com>
In-Reply-To: <CAG+J_DyhcA7RmHwgGJBw4r9JRij0_ONp3ZMD6oMTJ_f4dvYW8w@mail.gmail.com>

On Fri, Sep 30, 2011 at 3:33 PM, Jay Soffian <jaysoffian@gmail.com> wrote:

Show 6 quoted lines

> On Thu, Sep 29, 2011 at 6:03 AM, John Szakmeister <john@szakmeister.net> wrote:
>>
>> I've been working on a version of the keychain credential cache as
>> well.  I did create a gui, although it's a bit painful.
>
> I still don't understand why a CLI app should have a GUI credential prompt.

For one, I saw it more as being useful to things other than the git command line. And the Mac already presents a GUI for unlocking the keychain if necessary. It's not like there isn't any precedent for it.

[snip]

Show 11 quoted lines

>> I think that makes sense.  I think one thing we have to be careful
>> about partial matches.  I wouldn't want the credential cache to send
>> off the wrong password to a service.  This may be me being cautious,
>> but if I don't have all the necessary bits, I'd rather we fail that to
>> guess which entry is right.
>
> The credential helper I wrote doesn't work that way. To do so would
> mean using a rather more complicated form of the OS X Security API. It
> asks for an entry using whatever fields it has, and OS X returns the
> first match that satisfies. It's up to the user to yea/nay that match
> if the credential helper isn't on the entry's ACL.

True, the user would at least have to acknowledge it.

Show 16 quoted lines

>>>> +     /* "GitHub for Mac" compatibility */
>>>> +     if (!strcmp(hostname, "github.com"))
>>>> +             hostname = "github.com/mac";
>>>
>>> Nice touch. :)
>>
>> I honestly don't understand why this needs to be done.
>
> Because GitHub for Mac stores its entries using "github.com/mac" as
> the hostname.
>
>> I don't use GitHub for Mac... does that mean this is busted for me?
>
> No. It just means that the credential helper and GitHub for Mac store
> their entry in a compatible fashion. (So that each can locate the
> entry stored by the other.)

Ah, interesting. But it does mean that it won't pick up the password I've cached via my browser, right?

Show 15 quoted lines

>> [snip]
>>> My series will also produce "cert:/path/to/certificate" when unlocking a
>>> certificate. The other candidates for conversion are smtp-auth (for
>>> send-email) and imap (for imap-send).  I guess for certs, you'd want to
>>> use the "generic" keychain type.
>>
>> There is a method for adding a certificate to the keychain:
>>   <http://developer.apple.com/library/mac/#documentation/Security/Reference/certifkeytrustservices/Reference/reference.html#//apple_ref/doc/uid/TP30000157>
>>
>> I'm not sure what that does exactly, but I do have a cert, and it
>> shows up as "certificate" in the keychain.
>
> That's for storing a certificate itself. In this case, I think we're
> just talking about storing the passphrase which protects the
> certificate's private key.

I could've sworn the docs mentioned storing the private key, but I don't see it. SecIdentityCreateWithCertificate() can get you the private key from a cert, once added. It's not clear how to go from cert to password in a manner that's compatible with other Mac apps.

[snip]

Show 7 quoted lines

>> By the time you get Keychain involved, the decision has been made.
>> Most applications offer that ability... and you're right, this should
>> probably offer the same capability.  That also means stashing that
>> data somewhere. :-(  OTOH, it does make for a better user experience.
>
> What, no? If you don't want git to store usernames/passwords stored in
> the OS X Keychain, don't use the git-osx-keychain credential helper.

I want *some* cached, and others not. I don't want to be forced to remember to take git-osx-keychain out of my credentials list, or something silly like that. That doesn't help on the user-friendliness front. Having a check box and storing the fact that we shouldn't cache the password in the repo's config seems like a reasonable approach. But I agree with Jeff in another part of this thread: I think the prompting of the username and password should be a separate mechanism. It could prompt about whether or not to store the password, and git could take care of whether or not to call the credential store.

-John