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

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

[Jay Soffian]

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.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
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.

 contrib/credential-osxkeychain/Makefile            |   14 +
 .../git-credential-osxkeychain.c                   |  300 ++++++++++++++++++++
 .../git-credential-osxkeychain.py                  |  148 ++++++++++
 3 files changed, 462 insertions(+), 0 deletions(-)
 create mode 100644 contrib/credential-osxkeychain/Makefile
 create mode 100644 contrib/credential-osxkeychain/git-credential-osxkeychain.c
 create mode 100755 contrib/credential-osxkeychain/git-credential-osxkeychain.py

diff --git a/contrib/credential-osxkeychain/Makefile b/contrib/credential-osxkeychain/Makefile
new file mode 100644
index 0000000000..a0a7074cc6
--- /dev/null
+++ b/contrib/credential-osxkeychain/Makefile
@@ -0,0 +1,14 @@
+all:: git-credential-osxkeychain
+
+CC = gcc
+RM = rm -f
+CFLAGS = -O2 -Wall
+
+git-credential-osxkeychain: git-credential-osxkeychain.o
+	$(CC) -o $@ $< -Wl,-framework -Wl,Security
+
+git-credential-osxkeychain.o: git-credential-osxkeychain.c
+	$(CC) -c $(CFLAGS) $<
+
+clean:
+	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
diff --git a/contrib/credential-osxkeychain/git-credential-osxkeychain.c b/contrib/credential-osxkeychain/git-credential-osxkeychain.c
new file mode 100644
index 0000000000..2f611f7348
--- /dev/null
+++ b/contrib/credential-osxkeychain/git-credential-osxkeychain.c
@@ -0,0 +1,300 @@
+/* Copyright 2011 Jay Soffian. All rights reserved.
+ * FreeBSD License.
+ *
+ * A git credential helper that interfaces with the Mac OS X keychain
+ * via the Security framework.
+ */
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <termios.h>
+#include <Security/Security.h>
+
+static void die(const char *err, ...)
+{
+	char msg[4096];
+	va_list params;
+	va_start(params, err);
+	vsnprintf(msg, sizeof(msg), err, params);
+	fprintf(stderr, "%s\n", msg);
+	va_end(params);
+	exit(1);
+}
+
+void *xmalloc(size_t size)
+{
+	void *ret = malloc(size);
+	if (!ret)
+		die("Out of memory");
+	return ret;
+}
+
+void *xstrdup(const char *s1)
+{
+	void *ret = strdup(s1);
+	if (!ret)
+		die("Out of memory");
+	return ret;
+}
+
+void emit_user_pass(char *username, char *password)
+{
+	if (username)
+		printf("username=%s\n", username);
+	if (password)
+		printf("password=%s\n", password);
+}
+
+typedef enum { USERNAME, PASSWORD } prompt_type;
+
+void prompt(FILE *file, const char *what, const char *desc)
+{
+	if (desc)
+		fprintf(file, "%s for '%s': ", what, desc);
+	else
+		fprintf(file, "%s: ", what);
+}
+
+char *prompt_tty(prompt_type what, char *description)
+{
+	struct termios old;
+	struct termios new;
+	char buf[128];
+	int buf_len;
+	int fd = open("/dev/tty", O_RDWR|O_NOCTTY);
+	FILE *tty = fdopen(fd, "w+");
+	if (what == USERNAME) {
+		prompt(tty, "Username", description);
+	}
+	else {
+		prompt(tty, "Password", description);
+		tcgetattr(fd, &old);
+		memcpy(&new, &old, sizeof(struct termios));
+		new.c_lflag &= ~ECHO;
+		tcsetattr(fd, TCSADRAIN, &new);
+	}
+	if (!fgets(buf, sizeof(buf), tty)) {
+		fprintf(tty, "\n");
+		fclose(tty);
+		return NULL;
+	}
+	if (what == PASSWORD) {
+		tcsetattr(fd, TCSADRAIN, &old);
+		fprintf(tty, "\n");
+	}
+	fclose(tty);
+	buf_len = strlen(buf);
+	if (buf[buf_len-1] == '\n')
+		buf[buf_len-1] = '\0';
+	return xstrdup(buf);
+}
+
+char *username_from_keychain_item(SecKeychainItemRef item)
+{
+	OSStatus status;
+	SecKeychainAttributeList list;
+	SecKeychainAttribute attr;
+	list.count = 1;
+	list.attr = &attr;
+	attr.tag = kSecAccountItemAttr;
+	char *username;
+
+	status = SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL);
+	if (status != noErr)
+		return NULL;
+	username = xmalloc(attr.length + 1);
+	strncpy(username, attr.data, attr.length);
+	username[attr.length] = '\0';
+	SecKeychainItemFreeContent(&list, NULL);
+	return username;
+}
+
+int find_internet_password(SecProtocolType protocol,
+			   char *hostname,
+			   char *username)
+{
+	void *password_buf;
+	UInt32 password_len;
+	OSStatus status;
+	char *password;
+	int free_username = 0;
+	SecKeychainItemRef item;
+
+	status = SecKeychainFindInternetPassword(
+			NULL,
+			strlen(hostname), hostname,
+			0, NULL,
+			username ? strlen(username) : 0, username,
+			0, NULL,
+			0,
+			protocol,
+			kSecAuthenticationTypeDefault,
+			&password_len, &password_buf,
+			&item);
+	if (status != noErr)
+		return -1;
+
+	password = xmalloc(password_len + 1);
+	strncpy(password, password_buf, password_len);
+	password[password_len] = '\0';
+	SecKeychainItemFreeContent(NULL, password_buf);
+	if (!username) {
+		username = username_from_keychain_item(item);
+		free_username = 1;
+	}
+	emit_user_pass(username, password);
+	if (free_username)
+		free(username);
+	free(password);
+	return 0;
+}
+
+void delete_internet_password(SecProtocolType protocol,
+			      char *hostname,
+			      char *username)
+{
+	OSStatus status;
+	SecKeychainItemRef item;
+
+	status = SecKeychainFindInternetPassword(
+			NULL,
+			strlen(hostname), hostname,
+			0, NULL,
+			username ? strlen(username) : 0, username,
+			0, NULL,
+			0,
+			protocol,
+			kSecAuthenticationTypeDefault,
+			0, NULL,
+			&item);
+	if (status != noErr)
+		return;
+	SecKeychainItemDelete(item);
+}
+
+void add_internet_password(SecProtocolType protocol,
+			   char *hostname,
+			   char *username,
+			   char *password,
+			   char *comment)
+{
+	const char *label_format = "%s (%s)";
+	char *label;
+	OSStatus status;
+	SecKeychainItemRef item;
+	SecKeychainAttributeList list;
+	SecKeychainAttribute attr;
+	list.count = 1;
+	list.attr = &attr;
+	status = SecKeychainAddInternetPassword(
+			NULL,
+			strlen(hostname), hostname,
+			0, NULL,
+			strlen(username), username,
+			0, NULL,
+			0,
+			protocol,
+			kSecAuthenticationTypeDefault,
+			strlen(password), password,
+			&item);
+	if (status != noErr)
+		return;
+
+	/* set the comment */
+	attr.tag = kSecCommentItemAttr;
+	attr.data = comment;
+	attr.length = strlen(comment);
+	SecKeychainItemModifyContent(item, &list, 0, NULL);
+
+	/* override the label */
+	label = xmalloc(strlen(hostname) + strlen(username) +
+			strlen(label_format));
+	sprintf(label, label_format, hostname, username);
+	attr.tag = kSecLabelItemAttr;
+	attr.data = label;
+	attr.length = strlen(label);
+	SecKeychainItemModifyContent(item, &list, 0, NULL);
+}
+
+int main(int argc, const char **argv)
+{
+	const char *usage =
+		"Usage: git credential-osxkeychain --unique=TOKEN [options]\n"
+		"Options:\n"
+		"    --description=DESCRIPTION\n"
+		"    --username=USERNAME\n"
+		"    --reject";
+	char *description = NULL, *username = NULL, *unique = NULL;
+	char *hostname, *password;
+	int i, free_username = 0, reject = 0;
+	SecProtocolType protocol = 0;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strncmp(arg, "--description=", 14)) {
+			description = (char *) arg + 14;
+		}
+		else if (!strncmp(arg, "--username=", 11)) {
+			username = (char *) arg + 11;
+		}
+		else if (!strncmp(arg, "--unique=", 9)) {
+			unique = (char *) arg + 9;
+		}
+		else if (!strcmp(arg, "--reject")) {
+			reject = 1;
+		}
+		else if (!strcmp(arg, "--help")) {
+			die(usage);
+		}
+		else
+			die("Unrecognized argument `%s'; try --help", arg);
+	}
+
+	if (!unique)
+		die("Must specify --unique=TOKEN; try --help");
+
+	hostname = strchr(unique, ':');
+	if (!hostname)
+		die("Invalid token `%s'", unique);
+	*hostname++ = '\0';
+
+	/* "GitHub for Mac" compatibility */
+	if (!strcmp(hostname, "github.com"))
+		hostname = "github.com/mac";
+
+	if (!strcmp(unique, "https")) {
+		protocol = kSecProtocolTypeHTTPS;
+	} else if (!strcmp(unique, "http")) {
+		protocol = kSecProtocolTypeHTTP;
+	}
+	else
+		die("Unrecognized protocol `%s'", unique);
+
+	/* if this is a rejection delete the existing creds */
+	if (reject) {
+		delete_internet_password(protocol, hostname, username);
+		return 0;
+	}
+
+	/* otherwise look for a matching keychain item */
+	if (!find_internet_password(protocol, hostname, username))
+		return 0;
+
+	/* no keychain item found, prompt the user and store the result */
+	if (!username) {
+		if (!(username = prompt_tty(USERNAME, description)))
+			return 0;
+		free_username = 1;
+	}
+	if (!(password = prompt_tty(PASSWORD, description)))
+		return 0;
+
+	add_internet_password(protocol, hostname, username, password,
+			      description ? description : "default");
+	emit_user_pass(username, password);
+	if (free_username)
+		free(username);
+	free(password);
+	return 0;
+}
diff --git a/contrib/credential-osxkeychain/git-credential-osxkeychain.py b/contrib/credential-osxkeychain/git-credential-osxkeychain.py
new file mode 100755
index 0000000000..ae5ec00d68
--- /dev/null
+++ b/contrib/credential-osxkeychain/git-credential-osxkeychain.py
@@ -0,0 +1,148 @@
+#!/usr/bin/python
+# Copyright 2011 Jay Soffian. All rights reserved.
+# FreeBSD License.
+"""
+A git credential helper that interfaces with the Mac OS X keychain via
+/usr/bin/security.
+"""
+
+import os
+import re
+import sys
+import termios
+from getpass import _raw_input
+from optparse import OptionParser
+from subprocess import Popen, PIPE
+
+USERNAME = 'USERNAME'
+PASSWORD = 'PASSWORD'
+PROMPTS = dict(USERNAME='Username', PASSWORD='Password')
+
+def prompt_tty(what, desc):
+    """Prompt on TTY for username or password with optional description"""
+    prompt = '%s%s: ' % (PROMPTS[what], " for '%s'" % desc if desc else '')
+    # Borrowed mostly from getpass.py
+    fd = os.open('/dev/tty', os.O_RDWR|os.O_NOCTTY)
+    tty = os.fdopen(fd, 'w+', 1)
+    if what == USERNAME:
+        return _raw_input(prompt, tty, tty)
+    old = termios.tcgetattr(fd) # a copy to save
+    new = old[:]
+    new[3] &= ~termios.ECHO  # 3 == 'lflags'
+    try:
+        termios.tcsetattr(fd, termios.TCSADRAIN, new)
+        return _raw_input(prompt, tty, tty)
+    finally:
+        termios.tcsetattr(fd, termios.TCSADRAIN, old)
+        tty.write('\n')
+
+def emit_user_pass(username, password):
+    if username:
+        print 'username=' + username
+    if password:
+        print 'password=' + password
+
+def make_security_args(command, protocol, hostname, username):
+    args = ['/usr/bin/security', command]
+    # tlfd is 'dflt' backwards - obvious /usr/bin/security bug
+    # but allows us to ignore matching saved web forms.
+    args.extend(['-t', 'tlfd'])
+    args.extend(['-r', protocol])
+    if hostname:
+        args.extend(['-s', hostname])
+    if username:
+        args.extend(['-a', username])
+    return args
+
+def find_internet_password(protocol, hostname, username):
+    args = make_security_args('find-internet-password',
+                              protocol, hostname, username)
+    args.append('-g') # asks for password on stderr
+    p = Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE)
+    # grok stdout for username
+    out, err = p.communicate()
+    if p.returncode != 0:
+        return
+    for line in out.splitlines(): # pylint:disable-msg=E1103
+        m = re.search(r'^\s+"acct"<blob>=[^"]*"(.*)"$', line)
+        if m:
+            username = m.group(1)
+            break
+    # grok stderr for password
+    m = re.search(r'^password:[^"]*"(.*)"$', err)
+    if not m:
+        return
+    emit_user_pass(username, m.group(1))
+    return True
+
+def delete_internet_password(protocol, hostname, username):
+    args = make_security_args('delete-internet-password',
+                              protocol, hostname, username)
+    p = Popen(args, stdin=PIPE, stdout=PIPE, stderr=PIPE)
+    p.communicate()
+
+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)
+
+def main():
+    p = OptionParser()
+    p.add_option('--description')
+    p.add_option('--reject', action='store_true')
+    p.add_option('--unique', dest='token', help='REQUIRED OPTION')
+    p.add_option('--username')
+    opts, _ = p.parse_args()
+
+    if not opts.token:
+        p.error('--unique option required')
+    if not ':' in opts.token:
+        print >> sys.stderr, "Invalid token: '%s'" % opts.token
+        return 1
+    protocol, hostname = opts.token.split(':', 1)
+    if protocol not in ('http', 'https'):
+        print >> sys.stderr, "Unsupported protocol: '%s'" % protocol
+        return 1
+    if protocol == 'https':
+        protocol = 'htps'
+
+    # "GitHub for Mac" compatibility
+    if hostname == 'github.com':
+        hostname = 'github.com/mac'
+
+    # if this is a rejection delete the existing creds
+    if opts.reject:
+        delete_internet_password(protocol, hostname, opts.username)
+        return 0
+
+    # otherwise look for creds
+    if find_internet_password(protocol, hostname, opts.username):
+        return 0
+
+    # creds not found, so prompt the user then store the creds
+    username = opts.username
+    if username is None:
+        username = prompt_tty(USERNAME, opts.description)
+    password = prompt_tty(PASSWORD, opts.description)
+    add_internet_password(protocol, hostname, username, password)
+    emit_user_pass(username, password)
+    return 0
+
+if __name__ == '__main__':
+    sys.exit(main())
-- 
1.7.7.rc1.1.g011e1

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

[Jeff King]

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.

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. :)

> +	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.

> +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...".

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

[Chris Mear]

On 29 Sep 2011, at 08:56, Jeff King <peff@peff.net> wrote:

>> 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.

It's normal for applications on OS X to present their own UI when asking for credentials in order to save them to the keychain. For example, web passwords stored by Safari are captured via the normal web page form elements, but stored in the keychain.

That said, most terminal applications aren't aware of the Mac OS X keychain. So I suppose it may surprise users to come across the password prompt in the terminal UI, even though that is the same pattern that graphical programs follow.

But it's not without precedent: I'm pretty certain Subversion collects credentials directly in the terminal for storage in the keychain.

Chris

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

[John Szakmeister]

On Thu, Sep 29, 2011 at 3:56 AM, Jeff King <peff@peff.net> wrote:
[snip]
> 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'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 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'm not sure there is.  For instance, ssh will look up passphrases for
ssh keys in the keychain, but if you ssh into the box, and then try to
ssh to somewhere else, it'll prompt you for the passphrase for the
key.  In my own experiments, the lookup will come back with a failure,
despite the fact that the keychain is already unlocked. :-(

Looking at the Subversion source, they recognize the issue too:

> * XXX (2005-12-07): If no GUI is available (e.g. over a SSH session),
> * you won't be prompted for credentials with which to unlock your
> * keychain.  Apple recognizes lack of TTY prompting as a known
> * problem.

...obviously, Apple hasn't fixed this issue. :-(

> 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.

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.

[snip]
> 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.

I think this is a good idea too.  Keychain definitely wants this
information stored in separate fields, and I suspect the secrets api
does too (I haven't found any formal docs about the preferred way of
storing attributes related to urls though).

>> +     /* "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.  I don't use
GitHub for Mac... does that mean this is busted for me?

[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.

> 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.

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.

[snip]
> 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".

I'm not sure how easy it is to do that.  I think it's meant to be a
security measure that it points out the executable name... but it does
make it less friendly for users. :-(

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

Yep, I agree.  And it's worse when using the security command line
tool... when you grant security access to the key, then any app could
technically gain access to the item via the security tool.  That's one
of the reasons I didn't pursue that route early on.

-John

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

[John Szakmeister]

On Thu, Sep 29, 2011 at 6:03 AM, John Szakmeister <john@szakmeister.net> wrote:
[snip]
> Yep, I agree.  And it's worse when using the security command line
> tool... when you grant security access to the key, then any app could
> technically gain access to the item via the security tool.  That's one
> of the reasons I didn't pursue that route early on.

Thinking about this a little more, git-credential-anything has the
same problem.  I can run it, and it'll dump out my password for
anybody.  I'd rather it didn't do that.  I think it would be more
satisfying to have the mechanisms built into git itself, without a
separate application.  I'm not sure how practical that is though.

-John

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

[Jay Soffian]

On Thu, Sep 29, 2011 at 3:56 AM, Jeff King <peff@peff.net> 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.

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

[Jay Soffian]

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.

>>  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.

Sorry, missed this part in my previous reply. I don't understand - how
do you ever send a username to the credential helper if you don't get
it from the config? But in any case, if you have a username (via
config or some other way), yes, I think it should be given to the
credential helper.

> 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.

>>> +     /* "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.)

> [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 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.
>
> 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.

j.

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

[Jeff King]

On Fri, Sep 30, 2011 at 03:16:58PM -0400, Jay Soffian wrote:

> 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.

Because security prompts sometimes are out of band. In particular,
you're already interacting with the user outside of the terminal;
opening the keychain will get you an "allow git to open the keychain"
dialog.

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.

> 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

I meant running sshd on OS X, and then ssh-ing in from some other box.
Your credential helper doesn't seem to work at all (I guess because it
has no access to the keychains). I don't think it's a big deal, though.
It's the minority case, and if somebody wants to figure out how to make
it work later, they can.

> 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. :)

As far as being hack-ish, the original design was that you could chain
these things if you wanted. But in practice, I don't know how useful
that is. In fact, after all of this discussion, I'm wondering how useful
it is that the helpers are allowed to prompt themselves at all.

The KDE one does it. But would people be happier if git simply did:

  1. ask the helper for a credential. if we get it, done

  2. prompt the user

  3. if the credential is valid, ask the helper to store

That's way less flexible. But it also makes the helpers really simple.

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

Because the helper might have an alternate way of asking for the
password (e.g., the KDE helper has its own dialog). Maybe it will never
be useful. I just wanted to future-proof helpers by giving them sane
behavior for this case, on the off chance that future versions of git do
actually do this.

> > 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.

Don't get me wrong; I think you did the only sane thing. It was more
"Hmm, I was hoping that the right way to use the various security APIs
wouldn't need to...". And clearly my hope was wrong. :)

But this is exactly the sort of feedback I was hoping for; the interface
to the helpers could be better, and we are catching it now.

> > 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.

Not unreasonable. Again, my real concern is not perfect helpers, but
having helpers that exercise the helper interface enough so that we know
whether that interface is sufficient.

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

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

> 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.

Not for me (on 10.7). Doing:

  cp git-credential-osxkeychain /tmp/iTunes
  /tmp/iTunes --unique=https:foo.tld

gives me the dialog "iTunes wants to access the 'login' keychain" with a
password prompt.

Anyway, that's neither here nor there. It would be nice if we could set
the text, but if we can't, then we'll have to live with it.

-Peff

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

[Jeff King]

On Fri, Sep 30, 2011 at 03:33:37PM -0400, Jay Soffian wrote:

> Sorry, missed this part in my previous reply. I don't understand - how
> do you ever send a username to the credential helper if you don't get
> it from the config? But in any case, if you have a username (via
> config or some other way), yes, I think it should be given to the
> credential helper.

For example:

  git fetch https://user@host/repo.git
  git fetch https://user2@host/repo.git

-Peff

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

[Jay Soffian]

On Fri, Sep 30, 2011 at 6:11 PM, Jeff King <peff@peff.net> wrote:
> Because security prompts sometimes are out of band. In particular,
> you're already interacting with the user outside of the terminal;
> opening the keychain will get you an "allow git to open the keychain"
> dialog.

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.

> 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. :-)

> I meant running sshd on OS X, and then ssh-ing in from some other box.
> Your credential helper doesn't seem to work at all (I guess because it
> has no access to the keychains). I don't think it's a big deal, though.
> It's the minority case, and if somebody wants to figure out how to make
> it work later, they can.

As far as I know there's no way to make this work. When you login over
ssh, it's a different security context, and there is no access to the
password field of keychain entries.

>> 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. :-)

> As far as being hack-ish, the original design was that you could chain
> these things if you wanted. But in practice, I don't know how useful
> that is. In fact, after all of this discussion, I'm wondering how useful
> it is that the helpers are allowed to prompt themselves at all.
>
> The KDE one does it. But would people be happier if git simply did:
>
>  1. ask the helper for a credential. if we get it, done
>
>  2. prompt the user
>
>  3. if the credential is valid, ask the helper to store
>
> That's way less flexible. But it also makes the helpers really simple.

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?

> Because the helper might have an alternate way of asking for the
> password (e.g., the KDE helper has its own dialog). Maybe it will never
> be useful. I just wanted to future-proof helpers by giving them sane
> behavior for this case, on the off chance that future versions of git do
> actually do this.
>>
> Don't get me wrong; I think you did the only sane thing. It was more
> "Hmm, I was hoping that the right way to use the various security APIs
> wouldn't need to...". And clearly my hope was wrong. :)
>
> But this is exactly the sort of feedback I was hoping for; the interface
> to the helpers could be better, and we are catching it now.

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

> 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.

>> It is for security reasons. 99% of users will probably just click

> Anyway, that's neither here nor there. It would be nice if we could set
> the text, but if we can't, then we'll have to live with it.

The C version of the helper will use the name of the helper binary.

j.

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

[John Szakmeister]

On Fri, Sep 30, 2011 at 3:33 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
> 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]
>> 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.

>>>> +     /* "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?

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

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

[Jeff King]

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

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

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

> >> 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.

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

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

[Jay Soffian]

On Mon, Oct 3, 2011 at 6:59 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Sep 30, 2011 at 06:42:22PM -0400, Jay Soffian wrote:
>
>> 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).

In the normal case, the keychain entry would be added via the
credential helper, so they'd never even see the prompt since the
binary which adds an entry is automatically on that entry's ACL.

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

Double-click on the entry in Keychain Access, then click the "Access
Control" tab.

j.

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

[Jay Soffian]

On Sat, Oct 1, 2011 at 2:57 AM, John Szakmeister <john@szakmeister.net> wrote:
>>> 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?

Correct. I can add code to also make it look for the password entry as
stored by Safari/Chrome. It's actually stored as a slightly different
entry type ("Web form password" vs "Internet password"), so it's not
just the hostname difference.

j.

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

[Jeff King]

On Mon, Oct 03, 2011 at 09:13:12AM -0400, Jay Soffian wrote:

> > 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).
> 
> In the normal case, the keychain entry would be added via the
> credential helper, so they'd never even see the prompt since the
> binary which adds an entry is automatically on that entry's ACL.

Ah, that makes sense. That wasn't what happened for me; the first time I
ran it, before I had ever given it a password, it asked if it could
access the login keychain. But that was because I _already_ had an entry
there from the GitHub for Mac client.

So I assume it was typical that most users would see it at least that
first time. But it's probably not.

> > 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).
> 
> Double-click on the entry in Keychain Access, then click the "Access
> Control" tab.

Thanks. For some reason I was thinking the ACL was based on the
keychain, but of course having it per-entry makes much more sense. So I
was just looking in the wrong place.

-Peff

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

[Junio C Hamano]

Now, I would like to start moving the core part of the credential helper
series to 'master'. Could people summarize what the current status of
various moving parts are?

Here is an example of write-up, based on my understanding (I said it
should be pretty-much ready, but please correct me if I am mistaken).

 - The core part have seen some updates while it was cooking in 'next',
   with help from inputs by credential plug-in authors. The API and the
   sample implementation should be stable enough that it can be given to
   people who follow 'master' perhaps with 'experimental, minor details
   still subject to change' label attached.

I'd like to see similar write-ups for plug-ins from people who were
involved in the topic, at least on the following:

 - Mac OS X keychain?

 - Gnome?

 - KDE?

 - Windows?

 - Others?

Thanks.

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

[Jeff King]

On Tue, Oct 04, 2011 at 10:13:33AM -0700, Junio C Hamano wrote:

> Now, I would like to start moving the core part of the credential helper
> series to 'master'. Could people summarize what the current status of
> various moving parts are?
> 
> Here is an example of write-up, based on my understanding (I said it
> should be pretty-much ready, but please correct me if I am mistaken).
> 
>  - The core part have seen some updates while it was cooking in 'next',
>    with help from inputs by credential plug-in authors. The API and the
>    sample implementation should be stable enough that it can be given to
>    people who follow 'master' perhaps with 'experimental, minor details
>    still subject to change' label attached.

No, sadly I don't think we're there yet. The two big open questions are:

  1. Should we be giving more context details to the helpers, and/or
     should we be breaking down the information into pieces?

     I think the answer is probably yes. Certainly OS X would benefit
     from the broken-down pieces. My feeling is that we could hand
     helpers both broken-down pieces as well as an inclusive URL. So
     something like:

       git credential-foo \
         --type=network --protocol=https --host=example.com \
         --username=user1 --path=repo.git \
         --url=https://user1@example.com/repo.git

     and then the helper can pick what it likes from there.

     One thing I haven't figured out is how the user would tell git "no,
     the repo path is not relevant for determining the auth domain".
     That feature can come later, but I want to make sure that helpers
     know they might or might not get the "--path" option. I guess that
     is just a matter of documentation; I'm just a little nervous
     committing to it without having figured out the details.

  2. There has been some talk that the helper interface should perhaps
     be vastly simplified from "get the credentials and give them to
     git" to merely being a store/retrieve system, where the invocations
     would be something like (pretend git is the shell):

       # git asks if we have a stored credential
       git credential-foo --get --url=...,etc | read password

       # we had a successful authentication; ask the helper to store it;
       echo password | git credential-foo --put --url=...,etc

     That makes the helpers much simpler, and makes interacting with the
     user more uniform across helpers.

     It disallows helpers doing specialized interactions or dialogs. I'm
     not sure how much we want that. I thought some systems might find
     that more natural, but that's debatable. Jay implemented a git-like
     pure-terminal prompt in his helper. Lukas used the kde
     password-dialog for the KDE helper. I'm not sure what people really want.

I can produce patches for both if we want to see what they would look
like. But I kind of think it's not a matter of "what will the code look
like" (both of them are pretty straightforward to implement), but "what
do we want the interface to look like". And that is really as much up to
me as to people who are going to write and use the helpers.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> No, sadly I don't think we're there yet.

It is much better to proceed with caution than unleashing something whose
semantics will later have to change so drastically that we end up hurting
end users, so nobody has to say "sadly". By spelling out what more needs
to be there for us to get there, we are making progress.

> The two big open questions are:
>
>   1. Should we be giving more context details to the helpers, and/or
>      should we be breaking down the information into pieces?
>
>      I think the answer is probably yes. Certainly OS X would benefit
>      from the broken-down pieces. My feeling is that we could hand
>      helpers both broken-down pieces as well as an inclusive URL. So
>      something like:
>
>        git credential-foo \
>          --type=network --protocol=https --host=example.com \
>          --username=user1 --path=repo.git \
>          --url=https://user1@example.com/repo.git
>
>      and then the helper can pick what it likes from there.

Hmm, don't we first want to enumerate contexts where we might want to get
the access information from the user? E.g.

 * "transport" aka "git fetch/push"; I think you meant this by --type=network,
   but there probably are other kinds of accesses over "network".
 * "imap-send".
 * "send-email".
 * "tag -s" and perhaps upcoming "push --signed" or "commit --gpg-sign"?

Anything else?

I am not sure where the unlocking passphrase for ssl cert fits in this
picture, though.

>      One thing I haven't figured out is how the user would tell git "no,
>      the repo path is not relevant for determining the auth domain".
>      That feature can come later, but I want to make sure that helpers
>      know they might or might not get the "--path" option. I guess that
>      is just a matter of documentation; I'm just a little nervous
>      committing to it without having figured out the details.

Well, shouldn't the caller of credential-foo be passing --authdomain as a
separate piece anyway?  In the above example, if example.com operates as a
single auth domain no matter what repository, --authdomain may just say
"". If it uses per repository auth domain, --authdomain may say "repo.git"
(I am assuming <authdomain> is not globally unique but unique within the
context of <type, host> or perhaps <type, proto, host>). 

>   2. There has been some talk that the helper interface should perhaps
>      be vastly simplified from "get the credentials and give them to
>      git" to merely being a store/retrieve system, where the invocations
>      would be something like (pretend git is the shell):
>
>        # git asks if we have a stored credential
>        git credential-foo --get --url=...,etc | read password
>
>        # we had a successful authentication; ask the helper to store it;
>        echo password | git credential-foo --put --url=...,etc
>
>      That makes the helpers much simpler, and makes interacting with the
>      user more uniform across helpers.
>
>      It disallows helpers doing specialized interactions or dialogs.

Very true.

I have this suspicion that running dialog on various desktop environments
may be orthogonal to what credential store to be used. Once we know the
repertoire of the broken-down pieces, we may want to add "dialog-foo"
family of helpers whose sole purpose is to interact with the user to get
missing information.  E.g. we may notice that we need to ask both the
username and password, and invoke

    $ dialog-foo --msg=$msg --ask=username --ask-secret=password

where $msg tells the user why we are asking these pieces (e.g. trying to
push into https://example.com/repo.git), and then read from the helper
what the user gave it.

If the URL had user1@ in it, then the $msg would say that we are trying to
push into https://example.com/repo.git as user1, and we invoke the helper
with only --ask-secret=password (without --ask=username).

The same "helper" executable _could_ implement both interfaces, but I
think they are logically separate.

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

[Jay Soffian]

On Tue, Oct 4, 2011 at 3:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmm, don't we first want to enumerate contexts where we might want to get
> the access information from the user? E.g.
>
>  * "transport" aka "git fetch/push"; I think you meant this by --type=network,
>   but there probably are other kinds of accesses over "network".
>  * "imap-send".
>  * "send-email".
>  * "tag -s" and perhaps upcoming "push --signed" or "commit --gpg-sign"?
>
> Anything else?

Perhaps it would be illustrative to look at the OS X keychain API call
for adding a password to the store:

OSStatus SecKeychainAddInternetPassword (
   SecKeychainRef keychain,
   UInt32 serverNameLength,
   const char *serverName,
   UInt32 securityDomainLength,
   const char *securityDomain,
   UInt32 accountNameLength,
   const char *accountName,
   UInt32 pathLength,
   const char *path,
   UInt16 port,
   SecProtocolType protocol,
   SecAuthenticationType authenticationType,
   UInt32 passwordLength,
   const void *passwordData,
   SecKeychainItemRef *itemRef
);

SecProtocolType is an enum of 4-char values such as 'ftp ', 'http',
etc. Similarly for SecAuthenticationType which uses values such as
'form' (web form), 'http' (basic auth), etc.

http://developer.apple.com/library/mac/#documentation/Security/Reference/keychainservices/Reference/reference.html#//apple_ref/doc/c_ref/SecKeychainAddInternetPassword
http://developer.apple.com/library/mac/#documentation/Security/Reference/keychainservices/Reference/reference.html#//apple_ref/doc/c_ref/SecProtocolType
http://developer.apple.com/library/mac/#documentation/Security/Reference/keychainservices/Reference/reference.html#//apple_ref/doc/c_ref/SecAuthenticationType

j.