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

From: Jeff King <peff@peff.net>
Date: Oct 3, 2011, 10:59 UTC
Message-ID: <20111003105908.GF16078@sigill.intra.peff.net>
In-Reply-To: <CAG+J_Dww1yOeq1LHQYMiObPKqrWbk4t8Hn=G9WpYWXFBbHiuhQ@mail.gmail.com>

On Fri, Sep 30, 2011 at 06:42:22PM -0400, Jay Soffian wrote:

Show 5 quoted lines

> Usually it won't. In the default case, the keychain is unlocked and no
> permission is needed to add an entry, nor to retrieve that entry by
> the application which added it. The prompt will only occur if the
> credential helper is not on the entry's ACL, or if the keychain is
> locked.

Yeah. I was thinking the ACL prompt would come up more often, but I guess most people would hit "allow always", since it would get annoying pretty quickly otherwise (I didn't, because I was testing).

Side note: do you know how to edit those ACLs? I couldn't find it in the keychain manager. It would be helpful for testing to be able to tweak it (as a workaround, I just modified the binary, which apparently the keychain code cares about).

Show 7 quoted lines

> > Another example: if you're running gpg-agent, and you run "git tag -s",
> > you'll be prompted for your key passphrase in an out-of-band dialog.
> >
> > Maybe it doesn't make sense for the actual username/password, though.
> 
> Personally, it made sense to me do it at the CLI (obviously). But the
> source code in in contrib now. :-)

Let's leave it that way, then. It's open source, after all. If somebody really wants a dialog, they can add it. They can even make it optional (you can do "git config credential.helper 'osxkeychain --dialog'" if the thing actually takes options).

Show 7 quoted lines

> >> 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. :-(
> >
> > Yes, but I think that's a bug that should be fixed in git. :)
> 
> Yes it should. :-)

I think the only thing holding this back is portability. But it's awful to make reasonable platforms suffer because it can't be done portably. We should at least do the right thing when we can, and fall back to using getpass() otherwise. I'll see what I can do.

> I think that actually makes more sense. There's already an existing
> mechanism to customized (2) via GIT_ASKPASS, right? So it overlaps for
> the credential helper to do that doesn't it?

Sort of. The askpass interface was really invented for asking for ssh passphrases. So:

  1. It can only ask for one thing at a time. So you get two dialogs,
     not one, and there's no place for anything else, like a "remember
     this password" checkbox.

  2. Like the getpass() hack in git, it won't show you what you've typed
     so far for the username (there are several implementations, so it's
     possible one could have an extra feature for this, but the ones
     I've seen just always show some opaque image for each character
     typed).

  3. There's no way to pre-fill the username field, and let the user
     override. So you end up just taking whatever username we already
     have, ask for the password, and then try the authentication. The
     user's only recourse is to restart the command with a different URL
     (with the alternative username in the URL).

> Okay, the more I think about this, the more I think the existing
> design is both too much (asking the credential helper to do anything
> other than store/retrieve passwords) and too little (not breaking out
> the fields distinctly).

I remember in some initial research that there may have been some system which really wanted to do the prompting itself. But now I can't find any reference to it. I think it may have been the freedesktop Secrets API, but now that I read their documentation again, I think I may simply have been mistaking the prompt for unlocking the secure storage itself, not the actual username and password.

Show 9 quoted lines

> > That's not an unreasonable attitude. I mostly let the browser store
> > passwords, but sometimes override it for specific sites. But in this
> > case, I think it would be more per-repo. And you can turn off the helper
> > for a particular repo (actually, I'm not sure you can, but you probably
> > should be able to).
> 
> If the credential helper becomes no more than a store/retrieve, it's
> git that would do the prompting "Store credentials via
> git-osxkeychain?" after logging in successfully with the credentials.

Maybe. I had assumed we would hand it off to the helper, and it would make the decision (possibly after consulting with the user). But I suppose git could do it itself.

-Peff