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

From: Jeff King <peff@peff.net>
Date: Sep 29, 2011, 07:56 UTC
Message-ID: <20110929075627.GB14022@sigill.intra.peff.net>
In-Reply-To: <1316055113-2353-1-git-send-email-jaysoffian@gmail.com>

On Wed, Sep 14, 2011 at 10:51:53PM -0400, Jay Soffian wrote:

Show 8 quoted lines

> This credential helper adds, searches, and removes entries from
> the Mac OS X keychain. The C version links against the Security
> framework and is probably the best choice for daily use.
> 
> A python version is also included primarily as a more readable
> example and uses the /usr/bin/security CLI to access the keychain.
> 
> Tested with 10.6.8.

So I finally got a nice working OS X setup (10.7) to play around with these. Overall, works as advertised. :) I have a few comments, though.

> Here's a C version that no longer links to git. I also kept the original
> Python version as an example. I decided not to call out to
> 'git credential-gitpass' as it was simple enough to manage /dev/tty
> and there's no portability issues since this is OS X specific.

This was my first one. I kind of expected there to be some kind of graphical password dialog. Especially because keychain will pop up a dialog and ask you "is it OK for git to access this password?". So I sort of assumed that people would assume that credentials happened outside of the regular terminal session (I see the same thing on Linux, for example, with gpg-agent, which will open a new window and grab focus).

But I have no idea what's "normal" on OS X.

I wondered if you were trying to be friendly to people who were connecting via ssh. But that doesn't seem to work at all. I couldn't get either version of your helper to actually do anything in an ssh session (even with the same user logged in on console). I guess there is some magic to hook it into the keychain manager.

Also, regarding opening /dev/tty yourself versus using getpass. There are a few magic things getpass will do that your helper won't:

  1. It respects core.askpass, GIT_ASKPASS, and SSH_ASKPASS if they are
     set.

  2. The "get the username from the config" feature is triggered at the
     time of prompting the user (so instead of asking for the username,
     we check the config and pretend the user told us).

     I did it this way originally so that helpers would have the first
     crack at setting a username, and we would fall back to the config.
     Thinking on it more, that may be backwards. If the user has told
     git "for github.com, I am user 'foo'", then that should probably
     take effect first, and --username=foo get passed to the helper.

     It doesn't make a big difference with long-term storage helpers,
     because you tell them your username once and they remember it. But
     for things like credential-cache, it lets you store the username
     for a long time, but only cache the password (which means not
     typing the username every time).

So I think maybe reason (2) should go away. But (1) is definitely worth considering.

> +       if (!unique)
> +               die("Must specify --unique=TOKEN; try --help");

My test harness checks that this case just asks for the password without bothering to do any lookup or storage. It probably doesn't really matter in practice; I think git should always be providing _some_ context.

> +	hostname = strchr(unique, ':');
> +	if (!hostname)
> +		die("Invalid token `%s'", unique);
> +	*hostname++ = '\0';

Hrm. I was really hoping people wouldn't need to pick apart the "unique" token, and it could remain an opaque blob. If helpers are going to do this sort of parsing, then I'd just as soon have git break it down for them, and do something like:

  git credential-osxkeychain \
    --protocol=https \
    --host=github.com \
    --path=peff/git.git
    --username=peff

to just hand over as much information as possible, and let the helper throw it all together if it wants to.

> +	/* "GitHub for Mac" compatibility */
> +	if (!strcmp(hostname, "github.com"))
> +		hostname = "github.com/mac";

Nice touch. :)

Show 7 quoted lines

> +	if (!strcmp(unique, "https")) {
> +		protocol = kSecProtocolTypeHTTPS;
> +	} else if (!strcmp(unique, "http")) {
> +		protocol = kSecProtocolTypeHTTP;
> +	}
> +	else
> +		die("Unrecognized protocol `%s'", unique);

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.

I wonder if some people would not want to cache cert passwords. Speaking of which, I remember keychain asking me "do you want to let git see this password?", but I don't ever remember it asking "do you want to save this password?". Is that usually automatic? Again, I was kind of expecting a dialog with a "remember this" checkbox.

Show 20 quoted lines

> +def add_internet_password(protocol, hostname, username, password):
> +    # We do this over a pipe so that we can provide the password more
> +    # securely than as an argument which would show up in ps output.
> +    # Unfortunately this is possibly less robust since the security man
> +    # page does not document how to quote arguments. Emprically it seems
> +    # that using the double-quote, escaping \ and " works properly.
> +    username = username.replace('\\', '\\\\').replace('"', '\\"')
> +    password = password.replace('\\', '\\\\').replace('"', '\\"')
> +    command = ' '.join([
> +        'add-internet-password', '-U',
> +        '-r', protocol,
> +        '-s', hostname,
> +        '-a "%s"' % username,
> +        '-w "%s"' % password,
> +        '-j default',
> +        '-l "%s (%s)"' % (hostname, username),
> +    ]) + '\n'
> +    args = ['/usr/bin/security', '-i']
> +    p = Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE)
> +    p.communicate(command)

I noticed that when using the python helper, the dialog asking something like: "security wants to know this password. Allow it?"

Which was kind of lame. I would hope we could convince it to say "git". But I didn't see any option in the "security" tool for specifying the context[1]. The C helper says "git-credential-osxkeychain". Which isn't the end of the world, but it would be prettier if it just said "git".

-Peff

[1] I can kind of see why they might not want you to set this for security reasons (because it makes impersonating other programs easy). On the other hand, saying "security" conveys absolutely nothing. And as far as I can tell, I could just call my program /tmp/iTunes, and it would say "iTunes wants to know this password...".