[PATCH 1/4] osxkeychain: replace deprecated SecKeychain API

From

Bo Anderson via GitGitGadget <gitgitgadget@gmail.com>

Date

Feb 17, 2024, 23:34 UTC

Message-ID

<f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com>

In-Reply-To

<pull.1667.git.1708212896.gitgitgadget@gmail.com>

From: Bo Anderson <mail@boanderson.me>

    8 - helper (osxkeychain) overwrites on store
    9 - helper (osxkeychain) can forget host

The new API is compatible with credentials saved with the older API.

Signed-off-by: Bo Anderson <mail@boanderson.me>
---
 contrib/credential/osxkeychain/Makefile       |   3 +-
 .../osxkeychain/git-credential-osxkeychain.c  | 265 +++++++++++++-----
 2 files changed, 199 insertions(+), 69 deletions(-)

diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 4b3a08a2bac..238f5f8c36f 100644
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall
 -include ../../../config.mak
 
 git-credential-osxkeychain: git-credential-osxkeychain.o
-	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security
+	$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \
+		-framework Security -framework CoreFoundation
 
 git-credential-osxkeychain.o: git-credential-osxkeychain.c
 	$(CC) -c $(CFLAGS) $<
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 5f2e5f16c88..dc294ae944a 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -3,14 +3,39 @@
 #include <stdlib.h>
 #include <Security/Security.h>
 
-static SecProtocolType protocol;
-static char *host;
-static char *path;
-static char *username;
-static char *password;
-static UInt16 port;
-
-__attribute__((format (printf, 1, 2)))
+#define ENCODING kCFStringEncodingUTF8
+static CFStringRef protocol; /* Stores constant strings - not memory managed */
+static CFStringRef host;
+static CFStringRef path;
+static CFStringRef username;
+static CFDataRef password;
+static CFNumberRef port;
+
+static void clear_credential(void)
+{
+	if (host) {
+		CFRelease(host);
+		host = NULL;
+	}
+	if (path) {
+		CFRelease(path);
+		path = NULL;
+	}
+	if (username) {
+		CFRelease(username);
+		username = NULL;
+	}
+	if (password) {
+		CFRelease(password);
+		password = NULL;
+	}
+	if (port) {
+		CFRelease(port);
+		port = NULL;
+	}
+}
+
+__attribute__((format (printf, 1, 2), __noreturn__))
 static void die(const char *err, ...)
 {
 	char msg[4096];
@@ -19,70 +44,135 @@ static void die(const char *err, ...)
 	vsnprintf(msg, sizeof(msg), err, params);
 	fprintf(stderr, "%s\n", msg);
 	va_end(params);
+	clear_credential();
 	exit(1);
 }
 
-static void *xstrdup(const char *s1)
+static void *xmalloc(size_t len)
 {
-	void *ret = strdup(s1);
+	void *ret = malloc(len);
 	if (!ret)
 		die("Out of memory");
 	return ret;
 }
 
-#define KEYCHAIN_ITEM(x) (x ? strlen(x) : 0), x
-#define KEYCHAIN_ARGS \
-	NULL, /* default keychain */ \
-	KEYCHAIN_ITEM(host), \
-	0, NULL, /* account domain */ \
-	KEYCHAIN_ITEM(username), \
-	KEYCHAIN_ITEM(path), \
-	port, \
-	protocol, \
-	kSecAuthenticationTypeDefault
-
-static void write_item(const char *what, const char *buf, int len)
+static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
+{
+	va_list args;
+	const void *key;
+	CFMutableDictionaryRef result;
+
+	result = CFDictionaryCreateMutable(allocator,
+					   0,
+					   &kCFTypeDictionaryKeyCallBacks,
+					   &kCFTypeDictionaryValueCallBacks);
+
+
+	va_start(args, allocator);
+	while ((key = va_arg(args, const void *)) != NULL) {
+		const void *value;
+		value = va_arg(args, const void *);
+		if (value)
+			CFDictionarySetValue(result, key, value);
+	}
+	va_end(args);
+
+	return result;
+}
+
+#define CREATE_SEC_ATTRIBUTES(...) \
+	create_dictionary(kCFAllocatorDefault, \
+			  kSecClass, kSecClassInternetPassword, \
+			  kSecAttrServer, host, \
+			  kSecAttrAccount, username, \
+			  kSecAttrPath, path, \
+			  kSecAttrPort, port, \
+			  kSecAttrProtocol, protocol, \
+			  kSecAttrAuthenticationType, \
+			  kSecAttrAuthenticationTypeDefault, \
+			  __VA_ARGS__);
+
+static void write_item(const char *what, const char *buf, size_t len)
 {
 	printf("%s=", what);
 	fwrite(buf, 1, len, stdout);
 	putchar('\n');
 }
 
-static void find_username_in_item(SecKeychainItemRef item)
+static void find_username_in_item(CFDictionaryRef item)
 {
-	SecKeychainAttributeList list;
-	SecKeychainAttribute attr;
+	CFStringRef account_ref;
+	char *username_buf;
+	CFIndex buffer_len;
 
-	list.count = 1;
-	list.attr = &attr;
-	attr.tag = kSecAccountItemAttr;
+	account_ref = CFDictionaryGetValue(item, kSecAttrAccount);
+	if (!account_ref)
+	{
+		write_item("username", "", 0);
+		return;
+	}
 
-	if (SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL))
+	username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
+	if (username_buf)
+	{
+		write_item("username", username_buf, strlen(username_buf));
 		return;
+	}
 
-	write_item("username", attr.data, attr.length);
-	SecKeychainItemFreeContent(&list, NULL);
+	/* If we can't get a CString pointer then
+	 * we need to allocate our own buffer */
+	buffer_len = CFStringGetMaximumSizeForEncoding(
+			CFStringGetLength(account_ref), ENCODING) + 1;
+	username_buf = xmalloc(buffer_len);
+	if (CFStringGetCString(account_ref,
+				username_buf,
+				buffer_len,
+				ENCODING)) {
+		write_item("username", username_buf, buffer_len - 1);
+	}
+	free(username_buf);
 }
 
-static void find_internet_password(void)
+static OSStatus find_internet_password(void)
 {
-	void *buf;
-	UInt32 len;
-	SecKeychainItemRef item;
+	CFDictionaryRef attrs;
+	CFDictionaryRef item;
+	CFDataRef data;
+	OSStatus result;
 
-	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
-		return;
+	attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,
+				      kSecReturnAttributes, kCFBooleanTrue,
+				      kSecReturnData, kCFBooleanTrue,
+				      NULL);
+	result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);
+	if (result) {
+		goto out;
+	}
 
-	write_item("password", buf, len);
+	data = CFDictionaryGetValue(item, kSecValueData);
+
+	write_item("password",
+		   (const char *)CFDataGetBytePtr(data),
+		   CFDataGetLength(data));
 	if (!username)
 		find_username_in_item(item);
 
-	SecKeychainItemFreeContent(NULL, buf);
+	CFRelease(item);
+
+out:
+	CFRelease(attrs);
+
+	/* We consider not found to not be an error */
+	if (result == errSecItemNotFound)
+		result = errSecSuccess;
+
+	return result;
 }
 
-static void delete_internet_password(void)
+static OSStatus delete_internet_password(void)
 {
-	SecKeychainItemRef item;
+	CFDictionaryRef attrs;
+	OSStatus result;
 
 	/*
 	 * Require at least a protocol and host for removal, which is what git
@@ -90,25 +180,42 @@ static void delete_internet_password(void)
 	 * Keychain manager.
 	 */
 	if (!protocol || !host)
-		return;
+		return -1;
 
-	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, 0, NULL, &item))
-		return;
+	attrs = CREATE_SEC_ATTRIBUTES(NULL);
+	result = SecItemDelete(attrs);
+	CFRelease(attrs);
+
+	/* We consider not found to not be an error */
+	if (result == errSecItemNotFound)
+		result = errSecSuccess;
 
-	SecKeychainItemDelete(item);
+	return result;
 }
 
-static void add_internet_password(void)
+static OSStatus add_internet_password(void)
 {
+	CFDictionaryRef attrs;
+	OSStatus result;
+
 	/* Only store complete credentials */
 	if (!protocol || !host || !username || !password)
-		return;
+		return -1;
 
-	if (SecKeychainAddInternetPassword(
-	      KEYCHAIN_ARGS,
-	      KEYCHAIN_ITEM(password),
-	      NULL))
-		return;
+	attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+				      NULL);
+
+	result = SecItemAdd(attrs, NULL);
+	if (result == errSecDuplicateItem) {
+		CFDictionaryRef query;
+		query = CREATE_SEC_ATTRIBUTES(NULL);
+		result = SecItemUpdate(query, attrs);
+		CFRelease(query);
+	}
+
+	CFRelease(attrs);
+
+	return result;
 }
 
 static void read_credential(void)
@@ -131,36 +238,52 @@ static void read_credential(void)
 
 		if (!strcmp(buf, "protocol")) {
 			if (!strcmp(v, "imap"))
-				protocol = kSecProtocolTypeIMAP;
+				protocol = kSecAttrProtocolIMAP;
 			else if (!strcmp(v, "imaps"))
-				protocol = kSecProtocolTypeIMAPS;
+				protocol = kSecAttrProtocolIMAPS;
 			else if (!strcmp(v, "ftp"))
-				protocol = kSecProtocolTypeFTP;
+				protocol = kSecAttrProtocolFTP;
 			else if (!strcmp(v, "ftps"))
-				protocol = kSecProtocolTypeFTPS;
+				protocol = kSecAttrProtocolFTPS;
 			else if (!strcmp(v, "https"))
-				protocol = kSecProtocolTypeHTTPS;
+				protocol = kSecAttrProtocolHTTPS;
 			else if (!strcmp(v, "http"))
-				protocol = kSecProtocolTypeHTTP;
+				protocol = kSecAttrProtocolHTTP;
 			else if (!strcmp(v, "smtp"))
-				protocol = kSecProtocolTypeSMTP;
-			else /* we don't yet handle other protocols */
+				protocol = kSecAttrProtocolSMTP;
+			else {
+				/* we don't yet handle other protocols */
+				clear_credential();
 				exit(0);
+			}
 		}
 		else if (!strcmp(buf, "host")) {
 			char *colon = strchr(v, ':');
 			if (colon) {
+				UInt16 port_i;
 				*colon++ = '\0';
-				port = atoi(colon);
+				port_i = atoi(colon);
+				port = CFNumberCreate(kCFAllocatorDefault,
+						      kCFNumberShortType,
+						      &port_i);
 			}
-			host = xstrdup(v);
+			host = CFStringCreateWithCString(kCFAllocatorDefault,
+							 v,
+							 ENCODING);
 		}
 		else if (!strcmp(buf, "path"))
-			path = xstrdup(v);
+			path = CFStringCreateWithCString(kCFAllocatorDefault,
+							 v,
+							 ENCODING);
 		else if (!strcmp(buf, "username"))
-			username = xstrdup(v);
+			username = CFStringCreateWithCString(
+					kCFAllocatorDefault,
+					v,
+					ENCODING);
 		else if (!strcmp(buf, "password"))
-			password = xstrdup(v);
+			password = CFDataCreate(kCFAllocatorDefault,
+						(UInt8 *)v,
+						strlen(v));
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do
@@ -173,6 +296,7 @@ static void read_credential(void)
 
 int main(int argc, const char **argv)
 {
+	OSStatus result = 0;
 	const char *usage =
 		"usage: git credential-osxkeychain <get|store|erase>";
 
@@ -182,12 +306,17 @@ int main(int argc, const char **argv)
 	read_credential();
 
 	if (!strcmp(argv[1], "get"))
-		find_internet_password();
+		result = find_internet_password();
 	else if (!strcmp(argv[1], "store"))
-		add_internet_password();
+		result = add_internet_password();
 	else if (!strcmp(argv[1], "erase"))
-		delete_internet_password();
+		result = delete_internet_password();
 	/* otherwise, ignore unknown action */
 
+	if (result)
+		die("failed to %s: %d", argv[1], (int)result);
+
+	clear_credential();
+
 	return 0;
 }

-- 
gitgitgadget