threads / patch / 28387

patchcontrib: add a credential helper for Mac OS X's keychain

Subject: [PATCH] contrib: add a credential helper for Mac OS X's keychain

## tl;dr

7 messages between Sep 14, 2011 and Sep 15, 2011. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Jay Soffian· Sep 14, 2011, 17:58 UTC · lore

This credential helper adds, searches, and removes entries from the Mac OS X keychain via OS X's Security Framework.

Tested with 10.6.8.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
And here's a C version. I cargo-culted the Makefile from
contrib/svn-fe/Makefile. Sadly, linking against git bloats the
binary quite a bit which is dissappointing since this helper
won't be installed as a hard-link. Hmm, maybe it can be if I
dlopen the security framework instead of linking against it.
 contrib/credential-osxkeychain/Makefile            |   35 ++++
 .../git-credential-osxkeychain.c                   |  205 ++++++++++++++++++++
 2 files changed, 240 insertions(+), 0 deletions(-)
 create mode 100644 contrib/credential-osxkeychain/Makefile
 create mode 100644 contrib/credential-osxkeychain/git-credential-osxkeychain.c
Show changes to 2 files +240 −0

contrib/credential-osxkeychain/Makefile, contrib/credential-osxkeychain/git-credential-osxkeychain.c

diff --git a/contrib/credential-osxkeychain/Makefile b/contrib/credential-osxkeychain/Makefile
new file mode 100644
index 0000000000..dc6bbbc3f9
--- /dev/null
+++ b/contrib/credential-osxkeychain/Makefile
@@ -0,0 +1,35 @@
+all:: git-credential-osxkeychain
+
+CC = gcc
+RM = rm -f
+CFLAGS = -O2 -Wall -I../.. -DSHA1_HEADER='<openssl/sha.h>'
+GIT_LIBS = ../../libgit.a ../../xdiff/lib.a
+LIBS = $(GIT_LIBS) -lz -liconv -lcrypto -lssl
+
+QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir
+QUIET_SUBDIR1 =
+
+ifneq ($(findstring $(MAKEFLAGS),w),w)
+PRINT_DIR = --no-print-directory
+else # "make -w"
+NO_SUBDIR = :
+endif
+
+ifneq ($(findstring $(MAKEFLAGS),s),s)
+ifndef V
+	QUIET_CC      = @echo '   ' CC $@;
+	QUIET_LINK    = @echo '   ' LINK $@;
+	QUIET_SUBDIR0 = +@subdir=
+	QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo '   ' SUBDIR $$subdir; \
+	                $(MAKE) $(PRINT_DIR) -C $$subdir
+endif
+endif
+
+git-credential-osxkeychain: git-credential-osxkeychain.o $(GIT_LIBS)
+	$(QUIET_LINK)$(CC) -o $@ $< $(LIBS) -Wl,-framework -Wl,Security
+
+git-credential-osxkeychain.o: git-credential-osxkeychain.c
+	$(QUIET_CC)$(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..64bcc636cb
--- /dev/null
+++ b/contrib/credential-osxkeychain/git-credential-osxkeychain.c
@@ -0,0 +1,205 @@
+/* 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 <stdlib.h>
+#include <Security/Security.h>
+#include "cache.h"
+#include "credential.h"
+#include "parse-options.h"
+
+void emit_user_pass(char *username, char *password)
+{
+	if (username)
+		printf("username=%s\n", username);
+	if (password)
+		printf("password=%s\n", password);
+}
+
+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;
+	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)
+{
+	struct strbuf label = STRBUF_INIT;
+	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 */
+	strbuf_addf(&label, "%s (%s)", hostname, username);
+	attr.tag = kSecLabelItemAttr;
+	attr.data = label.buf;
+	attr.length = label.len;
+	SecKeychainItemModifyContent(item, &list, 0, NULL);
+}
+
+int main(int argc, const char **argv)
+{
+	const char * const usage[] = {
+		"git credential-osxkeychain [options]",
+		NULL
+	};
+	struct credential c = { NULL };
+	int reject = 0;
+	SecProtocolType protocol;
+	char *hostname;
+	struct option options[] = {
+		OPT_BOOLEAN(0, "reject", &reject,
+			    "reject a stored credential"),
+		OPT_STRING(0, "username", &c.username, "name",
+			   "an existing username"),
+		OPT_STRING(0, "description", &c.description, "desc",
+			   "human-readable description of the credential"),
+		OPT_STRING(0, "unique", &c.unique, "token",
+			   "a unique context for the credential [REQUIRED]"),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, NULL, options, usage, 0);
+	if (argc)
+		usage_with_options(usage, options);
+
+	if (!c.unique)
+		die(_("--unique option required"));
+	hostname = strchr(c.unique, ':');
+	if (!hostname)
+		die(_("Invalid token: '%s'"), c.unique);
+	*hostname++ = '\0';
+
+	/* "GitHub for Mac" compatibility */
+	if (!strcmp(hostname, "github.com"))
+		hostname = "github.com/mac";
+
+	if (!strcmp(c.unique, "https")) {
+		protocol = kSecProtocolTypeHTTPS;
+	} else if (!strcmp(c.unique, "http")) {
+		protocol = kSecProtocolTypeHTTP;
+	}
+	else
+		die(_("Unsupported protocol: '%s'"), c.unique);
+
+	/* if this is a rejection delete the existing creds */
+	if (reject) {
+		delete_internet_password(protocol, hostname, c.username);
+		return 0;
+	}
+
+	/* otherwise look for a matching keychain item */
+	if (!find_internet_password(protocol, hostname, c.username))
+		return 0;
+
+	/* no keychain item found, prompt the user and store the result */
+	credential_getpass(&c);
+	add_internet_password(protocol, hostname, c.username, c.password,
+			      c.description ? c.description : "default");
+	emit_user_pass(c.username, c.password);
+	return 0;
+}
-- 
1.7.7.rc1.1.g011e1
Jay Soffian· Sep 14, 2011, 18:19 UTC · re: Jay Soffian · lore

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

On Wed, Sep 14, 2011 at 1:58 PM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 17 quoted lines
> diff --git a/contrib/credential-osxkeychain/git-credential-osxkeychain.c b/contrib/credential-osxkeychain/git-credential-osxkeychain.c
> new file mode 100644
> index 0000000000..64bcc636cb
> --- /dev/null
> +++ b/contrib/credential-osxkeychain/git-credential-osxkeychain.c
>
> [...]
>
> +int find_internet_password(SecProtocolType protocol,
> +                          char *hostname,
> +                          char *username)
> +{
> +       void *password_buf;
> +       UInt32 password_len;
> +       OSStatus status;
> +       char *password;
> +       int free_username;
int free_username = 0;
j.
Jay Soffian· Sep 14, 2011, 22:55 UTC · re: Jay Soffian · lore

[PATCH] credential-osxkeychain: load Security framework dynamically

Use dlopen() / dysym() instead of dynmically linking to the Security framework. A followup commit will refactor things such that git-credential-osxkeychain can be hardlinked to git.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
> Hmm, maybe it can be if I dlopen the security framework instead of linking
> against it.

Something like this. I'm going to pause here for feedback. Is the (not yet existant) followup commit referenced above allowing git-credential-osxkeychain to be a hard link to git a worthwhile endeavor? Or would a better approach be to make git-credential-osxkeychain.c not use any git code?

 contrib/credential-osxkeychain/Makefile            |   12 +++-
 .../credential-osxkeychain/generate_security.py    |   73 ++++++++++++++++++++
 2 files changed, 82 insertions(+), 3 deletions(-)
 create mode 100755 contrib/credential-osxkeychain/generate_security.py
Show changes to 2 files +82 −3

contrib/credential-osxkeychain/Makefile, contrib/credential-osxkeychain/generate_security.py

diff --git a/contrib/credential-osxkeychain/Makefile b/contrib/credential-osxkeychain/Makefile
index dc6bbbc3f9..001d695cb8 100644
--- a/contrib/credential-osxkeychain/Makefile
+++ b/contrib/credential-osxkeychain/Makefile
@@ -25,11 +25,17 @@ ifndef V
 endif
 endif
 
-git-credential-osxkeychain: git-credential-osxkeychain.o $(GIT_LIBS)
-	$(QUIET_LINK)$(CC) -o $@ $< $(LIBS) -Wl,-framework -Wl,Security
+git-credential-osxkeychain: git-credential-osxkeychain.o security.o $(GIT_LIBS)
+	$(QUIET_LINK)$(CC) -o $@ $< security.o $(LIBS)
 
 git-credential-osxkeychain.o: git-credential-osxkeychain.c
 	$(QUIET_CC)$(CC) -c $(CFLAGS) $<
 
+security.o: security.c
+	$(QUIET_CC)$(CC) -c $(CFLAGS) $<
+
+security.c: generate_security.py
+	python generate_security.py
+
 clean:
-	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
+	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o security.?
diff --git a/contrib/credential-osxkeychain/generate_security.py b/contrib/credential-osxkeychain/generate_security.py
new file mode 100755
index 0000000000..db94672e95
--- /dev/null
+++ b/contrib/credential-osxkeychain/generate_security.py
@@ -0,0 +1,73 @@
+#!/usr/bin/python
+
+import re
+
+func_decls = """
+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);
+OSStatus SecKeychainFindInternetPassword(CFTypeRef keychainOrArray, 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, void **passwordData, SecKeychainItemRef *itemRef);
+OSStatus SecKeychainItemCopyContent(SecKeychainItemRef itemRef, SecItemClass *itemClass, SecKeychainAttributeList *attrList, UInt32 *length, void **outData);
+OSStatus SecKeychainItemDelete(SecKeychainItemRef itemRef);
+OSStatus SecKeychainItemFreeContent(SecKeychainAttributeList *attrList, void *data);
+OSStatus SecKeychainItemModifyContent(SecKeychainItemRef itemRef, const SecKeychainAttributeList *attrList, UInt32 length, const void *data);
+"""
+
+header = r"""
+#include <dlfcn.h>
+#include <Security/Security.h>
+#include "cache.h"
+
+const char *security_framework =
+	"/System/Library/Frameworks/Security.framework/Security";
+
+void *load_security()
+{
+	static void *security;
+	if (!security) {
+		if (!(security = dlopen(security_framework, RTLD_LAZY)))
+			die(_("dlopen(\"%s\") failed: %s"),
+			    security_framework, dlerror());
+	}
+	return security;
+}
+"""
+
+func_tmpl = """
+%(func_decl)s
+{
+	%(func_rv)s (*func)(%(arg_types)s) =
+		dlsym(load_security(), "%(func_name)s");
+	if (!func)
+		die(_("dlsym(%(func_name)s) failed: %%s"), dlerror());
+	return func(%(arg_names)s);
+}
+"""
+
+def generate_func(decl):
+	func_rv, func_name, func_args = re.search(
+		r'^(.*?)\s+([^(]+)\((.*)\);$', decl).groups()
+	func_args = [s.strip() for s in func_args.split(',')]
+	arg_types = []
+	arg_names = []
+	for arg in func_args:
+		arg_type, arg_name = re.search(r'^(.*?)([a-zA-Z]+)$', arg).groups()
+		arg_types.append(arg_type.strip())
+		arg_names.append(arg_name.strip())
+	return func_tmpl % dict(
+		func_decl=decl.rstrip(';'),
+		func_name=func_name,
+		func_rv=func_rv,
+		arg_types=', '.join(arg_types),
+		arg_names=', '.join(arg_names),
+	)
+
+def main():
+	f = open('security.c', 'w')
+	f.write(header)
+	for decl in func_decls.splitlines():
+		decl = decl.strip()
+		if decl:
+			f.write(generate_func(decl))
+	f.close()
+
+if __name__ == '__main__':
+	main()
-- 
1.7.7.rc1.1.g011e1
Jeff King· Sep 14, 2011, 23:08 UTC · re: Jay Soffian · lore

Re: [PATCH] credential-osxkeychain: load Security framework dynamically

On Wed, Sep 14, 2011 at 06:55:26PM -0400, Jay Soffian wrote:
> Something like this. I'm going to pause here for feedback. Is the (not yet
> existant) followup commit referenced above allowing git-credential-osxkeychain
> to be a hard link to git a worthwhile endeavor? Or would a better approach be
> to make git-credential-osxkeychain.c not use any git code?

To be honest, I was surprised to see you linking against git. I had always envisioned OS-specific helpers as living outside of the git.git repo. That's why I provided git-credential-getpass; it should be the only part of git that helpers really want to reuse.

What are you getting from git that is useful? From my skim of your patch, it looks like xmalloc/die, parse_options, and credential_getpass.

The first can be replaced with a few trivial lines of code. The second is overkill, I think. The helper code will always hand you the "--option=value" form, and I always intended it to stay that way (whether that is well documented, I'm not sure). But a simple loop with strncmps would be fine.

The hardest part is credential_getpass. You can call "git credential-getpass", but you'll have to read the output yourself (though it's quite simple to parse; see read_credential_response).

I'm not a fan of cutting and pasting code, and this will add a number of lines to your C program. But I feel like keeping the build completely separate from core git is probably a good boundary (especially because this will not be getting built or tested all the time, as most of the core code is).

-Peff
Jay Soffian· Sep 14, 2011, 23:56 UTC · re: Jeff King · lore

Re: [PATCH] credential-osxkeychain: load Security framework dynamically

On Wed, Sep 14, 2011 at 7:08 PM, Jeff King <peff@peff.net> wrote:
Show 6 quoted lines
> On Wed, Sep 14, 2011 at 06:55:26PM -0400, Jay Soffian wrote:
>
> To be honest, I was surprised to see you linking against git. I had
> always envisioned OS-specific helpers as living outside of the git.git
> repo.  That's why I provided git-credential-getpass; it should be the
> only part of git that helpers really want to reuse.

Okay, I think I was led astray by the fact that credential-{cache,store}.c (at least the latter of which is meant as nothing more than an example helper right?) links with git.

> What are you getting from git that is useful? From my skim of your
> patch, it looks like xmalloc/die, parse_options, and credential_getpass.
Correct.
Show 15 quoted lines
> The first can be replaced with a few trivial lines of code. The second
> is overkill, I think. The helper code will always hand you the
> "--option=value" form, and I always intended it to stay that way
> (whether that is well documented, I'm not sure). But a simple loop with
> strncmps would be fine.
>
> The hardest part is credential_getpass. You can call "git
> credential-getpass", but you'll have to read the output yourself (though
> it's quite simple to parse; see read_credential_response).
>
> I'm not a fan of cutting and pasting code, and this will add a number of
> lines to your C program. But I feel like keeping the build completely
> separate from core git is probably a good boundary (especially because
> this will not be getting built or tested all the time, as most of the
> core code is).
Okay.
j.
Jeff King· Sep 15, 2011, 00:16 UTC · re: Jay Soffian · lore

Re: [PATCH] credential-osxkeychain: load Security framework dynamically

On Wed, Sep 14, 2011 at 07:56:42PM -0400, Jay Soffian wrote:
> Okay, I think I was led astray by the fact that
> credential-{cache,store}.c (at least the latter of which is meant as
> nothing more than an example helper right?) links with git.

No, credential-store is meant to be used. It's just that it has a security tradeoff that makes it the wrong choice for most cases. So it's meant to be used sparingly. :)

As for those helpers being linked against git, I guess it doesn't make them the best example code. But I wanted them to be always available as a lowest common denominator (because even if you have a fancy local keychain, it is likely that you'll end up at some point using git across an ssh connection, and I wanted to provide _something_ there).

Not having any external dependencies, those helpers don't pollute our code base too much. Building and testing them with the rest of git keeps the code fresh and unbroken. Maybe it would be better if they provided a clearer separation as an example. I'm open to that if people think it's worth splitting them out. I suspect I could write credential-store as something like 10 lines of perl.

-Peff
Junio C Hamano· Sep 14, 2011, 23:18 UTC · re: Jay Soffian · lore

Re: [PATCH] credential-osxkeychain: load Security framework dynamically

Jay Soffian <jaysoffian@gmail.com> writes:
> Something like this. I'm going to pause here for feedback. Is the (not yet
> existant) followup commit referenced above allowing git-credential-osxkeychain
> to be a hard link to git a worthwhile endeavor? Or would a better approach be
> to make git-credential-osxkeychain.c not use any git code?
Most definitely the latter, I would think.

The whole point of making the Git credential code talk with a defined interface with external programs is so that these keychain helpers can be written independently from the rest of Git.

If the reason why your keychain helper benefits from linking with the rest of Git is because some pieces of information you need in order to respond to the requests from credential interface is hard to get if your helper is built as an independent program, that is a sign that we are not exposing enough information to scripts, iow, the failure in the design of the credential interface. If that is the case (and I doubt it is), we would need to fix the interface (either the credential interface, or perhaps "git config") so that such an independent program does not have to peek inside the internals of Git.

If the reason is because you want to reuse some generic C API we have that are not necessarily tied to Git (e.g. strbuf, string-list, etc.), on the other hand, please resist the temptation to do so. It would not help your program to serve as an example of independent external keychain helpers, i.e. a demonstration of how simple to write them.

← back to recent threads