On Thu, Sep 29, 2011 at 3:56 AM, Jeff King <peff@xxxxxxxx> wrote: > On Wed, Sep 14, 2011 at 10:51:53PM -0400, Jay Soffian wrote: > >> 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. This makes no sense to me at all. Ignore OS X for the moment. You use git on the command-line. Why would there be any expectation of it interacting with the user via anything other than the terminal. Anyway, I expect the username/password prompt on the command-line, in the same terminal window where I just ran the git command that needs credentials. > 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. I don't understand where you're running ssh from/to in this scenario, but OS X has a notion of security contexts (this is a bit of a tangent): http://developer.apple.com/library/mac/#technotes/tn2083/_index.html > 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. I found it ugly that git's native getpass doesn't echo the username back, and it seems hackish to me for the credential helper to turn back around and invoke it in any case. :-( >> + 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. Okay, that wasn't clear from whatever documentation I read on how credential helpers should behave. But why invoke the credential helper just to ask for a password? >> + 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. Keychain entries have distinct fields. I broke apart the token and stored it the way other applications mostly do on OS X. >> + /* "GitHub for Mac" compatibility */ >> + if (!strcmp(hostname, "github.com")) >> + hostname = "github.com/mac"; > > Nice touch. :) > >> + 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. Yep, I was punting on certificate for v1. > 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. Each keychain entry has an ACL of applications that are allowed to access it. When an application asks for an entry and the application isn't on that entry's ACL, OS X (not the application) presents the user the dialog you refer to. The application has no control over that dialog. Now, I could have the credential helper ask the user "store this password?" after prompting for it, but why even use a credential helper if you don't want it to store your credentials? >> +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". That's partly why I wrote the C version. > [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...". It is for security reasons. 99% of users will probably just click "Okay" no matter what. For the 1% that bother to pay attention to the dialog, it provides the full path to the binary. I'd be suspicious if /tmp/iTunes wanted a password. j. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html