[PATCH] osxkeychain: avoid incorrectly skipping store operation

[PATCH] osxkeychain: avoid incorrectly skipping store operation

[Koji Nakamaru via GitGitGadget]

From: Koji Nakamaru <koji.nakamaru@gree.net>

git-credential-osxkeychain skips storing a credential if its "get"
action sets "state[]=osxkeychain:seen=1". This behavior was introduced
in e1ab45b2 (osxkeychain: state to skip unnecessary store operations,
2024-05-15), which appeared in v2.46.

However, this state[] persists even if a credential returned by
"git-credential-osxkeychain get" is invalid and a subsequent helper's
"get" operation returns a valid credential. Another subsequent helper
(such as [1]) may expect git-credential-osxkeychain to store the valid
credential, but the "store" operation is incorrectly skipped because it
only checks "state[]=osxkeychain:seen=1".

To solve this issue, "state[]=osxkeychain:seen" needs to contain enough
information to identify whether the current "store" input matches the
output from the previous "get" operation (and not a credential from
another helper).

Set "state[]=osxkeychain:seen" to a value encoding the credential output
by "get", and compare it with a value encoding the credential input by
"store".

[1]: https://github.com/hickford/git-credential-oauth

Reported-by: Petter Sælen <petter@saelen.eu>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
---
    osxkeychain: avoid incorrectly skipping store operation

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1999%2FKojiNakamaru%2Ffix%2Fosxkeychain-state-seen-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1999/KojiNakamaru/fix/osxkeychain-state-seen-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1999

 .../osxkeychain/git-credential-osxkeychain.c  | 158 +++++++++++++++++-
 1 file changed, 151 insertions(+), 7 deletions(-)

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 611c9798b3..c973e844a5 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -12,7 +12,7 @@ static CFStringRef username;
 static CFDataRef password;
 static CFDataRef password_expiry_utc;
 static CFDataRef oauth_refresh_token;
-static int state_seen;
+static char *state_seen;
 
 static void clear_credential(void)
 {
@@ -61,6 +61,12 @@ static void die(const char *err, ...)
 	exit(1);
 }
 
+/*
+ * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
+ * introduce significant dependencies. Therefore, we define simplified
+ * versions here to keep this code self-contained.
+ */
+
 static void *xmalloc(size_t len)
 {
 	void *ret = malloc(len);
@@ -69,6 +75,30 @@ static void *xmalloc(size_t len)
 	return ret;
 }
 
+static void *xcalloc(size_t count, size_t size)
+{
+	void *ret = calloc(count, size);
+	if (!ret)
+		die("Out of memory");
+	return ret;
+}
+
+static void *xrealloc(void *ptr, size_t size)
+{
+	void *ret = realloc(ptr, size);
+	if (!ret)
+		die("Out of memory");
+	return ret;
+}
+
+static char *xstrdup(const char *str)
+{
+	char *ret = strdup(str);
+	if (!ret)
+		die("Out of memory");
+	return ret;
+}
+
 static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
 {
 	va_list args;
@@ -112,6 +142,98 @@ static void write_item(const char *what, const char *buf, size_t len)
 	putchar('\n');
 }
 
+struct sb {
+	char *buf;
+	int size;
+};
+
+static void sb_init(struct sb *sb)
+{
+	sb->size = 1024;
+	sb->buf = xcalloc(sb->size, 1);
+}
+
+static void sb_release(struct sb *sb)
+{
+	if (sb->buf) {
+		free(sb->buf);
+		sb->buf = NULL;
+		sb->size = 0;
+	}
+}
+
+static void sb_add(struct sb *sb, const char *s, int n)
+{
+	int len = strlen(sb->buf);
+	int size = sb->size;
+	if (size < len + n + 1) {
+		sb->size = len + n + 1;
+		sb->buf = xrealloc(sb->buf, sb->size);
+	}
+	strncat(sb->buf, s, n);
+	sb->buf[len + n] = '\0';
+}
+
+static void write_item_sb(struct sb *sb, const char *what, const char *buf, int n)
+{
+	char s[32];
+
+	sprintf(s, "__%s=", what);
+	sb_add(sb, s, strlen(s));
+	sb_add(sb, buf, n);
+}
+
+static void write_item_sb_cfstring(struct sb *sb, const char *what, CFStringRef ref)
+{
+	char *buf;
+	int len;
+
+	if (!ref)
+		return;
+	len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
+	buf = xmalloc(len);
+	if (CFStringGetCString(ref, buf, len, ENCODING))
+		write_item_sb(sb, what, buf, strlen(buf));
+	free(buf);
+}
+
+static void write_item_sb_cfnumber(struct sb *sb, const char *what, CFNumberRef ref)
+{
+	short n;
+	char buf[32];
+
+	if (!ref)
+		return;
+	if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
+		return;
+	sprintf(buf, "%d", n);
+	write_item_sb(sb, what, buf, strlen(buf));
+}
+
+static void write_item_sb_cfdata(struct sb *sb, const char *what, CFDataRef ref)
+{
+	char *buf;
+	int len;
+
+	if (!ref)
+		return;
+	buf = (char *)CFDataGetBytePtr(ref);
+	if (!buf || strlen(buf) == 0)
+		return;
+	len = CFDataGetLength(ref);
+	write_item_sb(sb, what, buf, len);
+}
+
+static void encode_state_seen(struct sb *sb)
+{
+	sb_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
+	write_item_sb_cfstring(sb, "host", host);
+	write_item_sb_cfnumber(sb, "port", port);
+	write_item_sb_cfstring(sb, "path", path);
+	write_item_sb_cfstring(sb, "username", username);
+	write_item_sb_cfdata(sb, "password", password);
+}
+
 static void find_username_in_item(CFDictionaryRef item)
 {
 	CFStringRef account_ref;
@@ -124,6 +246,7 @@ static void find_username_in_item(CFDictionaryRef item)
 		write_item("username", "", 0);
 		return;
 	}
+	username = CFStringCreateCopy(kCFAllocatorDefault, account_ref);
 
 	username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
 	if (username_buf)
@@ -163,6 +286,7 @@ static OSStatus find_internet_password(void)
 	}
 
 	data = CFDictionaryGetValue(item, kSecValueData);
+	password = CFDataCreateCopy(kCFAllocatorDefault, data);
 
 	write_item("password",
 		   (const char *)CFDataGetBytePtr(data),
@@ -173,7 +297,14 @@ static OSStatus find_internet_password(void)
 	CFRelease(item);
 
 	write_item("capability[]", "state", strlen("state"));
-	write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
+	{
+		struct sb sb;
+
+		sb_init(&sb);
+		encode_state_seen(&sb);
+		write_item("state[]", sb.buf, strlen(sb.buf));
+		sb_release(&sb);
+	}
 
 out:
 	CFRelease(attrs);
@@ -288,13 +419,22 @@ static OSStatus add_internet_password(void)
 	CFDictionaryRef attrs;
 	OSStatus result;
 
-	if (state_seen)
-		return errSecSuccess;
-
 	/* Only store complete credentials */
 	if (!protocol || !host || !username || !password)
 		return -1;
 
+	if (state_seen) {
+		struct sb sb;
+
+		sb_init(&sb);
+		encode_state_seen(&sb);
+		if (!strcmp(state_seen, sb.buf)) {
+			sb_release(&sb);
+			return errSecSuccess;
+		}
+		sb_release(&sb);
+	}
+
 	data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
 	if (password_expiry_utc) {
 		CFDataAppendBytes(data,
@@ -403,8 +543,9 @@ static void read_credential(void)
 							   (UInt8 *)v,
 							   strlen(v));
 		else if (!strcmp(buf, "state[]")) {
-			if (!strcmp(v, "osxkeychain:seen=1"))
-				state_seen = 1;
+			int len = strlen("osxkeychain:seen=");
+			if (!strncmp(v, "osxkeychain:seen=", len))
+				state_seen = xstrdup(v);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
@@ -443,5 +584,8 @@ int main(int argc, const char **argv)
 
 	clear_credential();
 
+	if (state_seen)
+		free(state_seen);
+
 	return 0;
 }

base-commit: 4badef0c3503dc29059d678abba7fac0f042bc84
-- 
gitgitgadget

Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation

[Junio C Hamano]

"Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +/*
> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
> + * introduce significant dependencies. Therefore, we define simplified
> + * versions here to keep this code self-contained.
> + */

Sorry, but I do not quite understand this comment.  The program is
shipped as a part of Git, and using these functions and linking with
libgit.a may pull strbuf.o and some other *.o files out of libgit.a
to link with git-credential-osxkeychain.o to produce the executable,
but how can that be "significant dependencies"?  For anybody who is
building git-credential-osxkeychain, the necessary sources come for
free.

It is not like we are forcing git-credential-osxkeychain to link
with a shared object libgit.so and making git-credential-osxkeychain
depend on it, or anything like that, which may require consumers of
binary distribution of git-credential-osxkeychain to also install
another package that has libgit.so in it (which is likely to be the
"git" package).  Even if it were the case (which is not), what good
would it be to have git-credential-osxkeychain on your system
without having git on the same system?

Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> +/*
>> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
>> + * introduce significant dependencies. Therefore, we define simplified
>> + * versions here to keep this code self-contained.
>> + */
>
> Sorry, but I do not quite understand this comment.  The program is
> shipped as a part of Git, and using these functions and linking with
> libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> to link with git-credential-osxkeychain.o to produce the executable,
> but how can that be "significant dependencies"?  For anybody who is
> building git-credential-osxkeychain, the necessary sources come for
> free.
>
> It is not like we are forcing git-credential-osxkeychain to link
> with a shared object libgit.so and making git-credential-osxkeychain
> depend on it, or anything like that, which may require consumers of
> binary distribution of git-credential-osxkeychain to also install
> another package that has libgit.so in it (which is likely to be the
> "git" package).  Even if it were the case (which is not), what good
> would it be to have git-credential-osxkeychain on your system
> without having git on the same system?

The rest of the patch, excluding the poor-man's reimplementation of
helper functions, looked like they match what the proposed log
message described.

It seems that credential material like username and password are
included in plaintext as part of the state[], but is this a safe
thing to do?  The keychain will give out the credential material in
a way the requestor with sufficient priviledges can read, and this
state[] is stored in the same place, so I am guessing that this is
not adding any extra security concerns, but I just wanted to make
sure you've considered any security implications.

Thanks.

Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation

[Koji Nakamaru]

On Fri, Nov 14, 2025 at 5:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> +/*
> >> + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
> >> + * introduce significant dependencies. Therefore, we define simplified
> >> + * versions here to keep this code self-contained.
> >> + */
> >
> > Sorry, but I do not quite understand this comment.  The program is
> > shipped as a part of Git, and using these functions and linking with
> > libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> > to link with git-credential-osxkeychain.o to produce the executable,
> > but how can that be "significant dependencies"?  For anybody who is
> > building git-credential-osxkeychain, the necessary sources come for
> > free.
> >
> > It is not like we are forcing git-credential-osxkeychain to link
> > with a shared object libgit.so and making git-credential-osxkeychain
> > depend on it, or anything like that, which may require consumers of
> > binary distribution of git-credential-osxkeychain to also install
> > another package that has libgit.so in it (which is likely to be the
> > "git" package).  Even if it were the case (which is not), what good
> > would it be to have git-credential-osxkeychain on your system
> > without having git on the same system?

I see your point. I was following the current implementation's approach
(it has its own xmalloc() and die()) and thought the comment would be
appropriate if we continued that approach. I will refactor the code to
use libgit instead.

> The rest of the patch, excluding the poor-man's reimplementation of
> helper functions, looked like they match what the proposed log
> message described.
>
> It seems that credential material like username and password are
> included in plaintext as part of the state[], but is this a safe
> thing to do?  The keychain will give out the credential material in
> a way the requestor with sufficient priviledges can read, and this
> state[] is stored in the same place, so I am guessing that this is
> not adding any extra security concerns, but I just wanted to make
> sure you've considered any security implications.

Yes, that was considered. The credential helper protocol already
passes credentials in plaintext between helpers via the "store"
operation. Since the data in state[] is handled in the same
manner, it doesn't introduce an additional security risk beyond
what the existing protocol already entails.

--
Koji Nakamaru

[PATCH v2] osxkeychain: avoid incorrectly skipping store operation

[Koji Nakamaru via GitGitGadget]

From: Koji Nakamaru <koji.nakamaru@gree.net>

git-credential-osxkeychain skips storing a credential if its "get"
action sets "state[]=osxkeychain:seen=1". This behavior was introduced
in e1ab45b2 (osxkeychain: state to skip unnecessary store operations,
2024-05-15), which appeared in v2.46.

However, this state[] persists even if a credential returned by
"git-credential-osxkeychain get" is invalid and a subsequent helper's
"get" operation returns a valid credential. Another subsequent helper
(such as [1]) may expect git-credential-osxkeychain to store the valid
credential, but the "store" operation is incorrectly skipped because it
only checks "state[]=osxkeychain:seen=1".

To solve this issue, "state[]=osxkeychain:seen" needs to contain enough
information to identify whether the current "store" input matches the
output from the previous "get" operation (and not a credential from
another helper).

Set "state[]=osxkeychain:seen" to a value encoding the credential output
by "get", and compare it with a value encoding the credential input by
"store".

[1]: https://github.com/hickford/git-credential-oauth

Reported-by: Petter Sælen <petter@saelen.eu>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
---
    osxkeychain: avoid incorrectly skipping store operation
    
    Changes since v1:
    
     * Use functions provided by wrapper.h and strbuf.h instead of
       self-defined ones.
     * Adjust Makefile and meson.build to use these functions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1999%2FKojiNakamaru%2Ffix%2Fosxkeychain-state-seen-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1999/KojiNakamaru/fix/osxkeychain-state-seen-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1999

Range-diff vs v1:

 1:  b14045b4de ! 1:  dd36d290ff osxkeychain: avoid incorrectly skipping store operation
     @@ Commit message
      
          Reported-by: Petter Sælen <petter@saelen.eu>
          Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
          Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net>
      
     + ## contrib/credential/osxkeychain/Makefile ##
     +@@
     + # The default target of this Makefile is...
     + all:: git-credential-osxkeychain
     + 
     ++include ../../../config.mak.uname
     + -include ../../../config.mak.autogen
     + -include ../../../config.mak
     + 
     ++ifdef ZLIB_NG
     ++	BASIC_CFLAGS += -DHAVE_ZLIB_NG
     ++        ifdef ZLIB_NG_PATH
     ++		BASIC_CFLAGS += -I$(ZLIB_NG_PATH)/include
     ++		EXTLIBS += $(call libpath_template,$(ZLIB_NG_PATH)/$(lib))
     ++        endif
     ++	EXTLIBS += -lz-ng
     ++else
     ++        ifdef ZLIB_PATH
     ++		BASIC_CFLAGS += -I$(ZLIB_PATH)/include
     ++		EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
     ++        endif
     ++	EXTLIBS += -lz
     ++endif
     ++ifndef NO_ICONV
     ++        ifdef NEEDS_LIBICONV
     ++                ifdef ICONVDIR
     ++			BASIC_CFLAGS += -I$(ICONVDIR)/include
     ++			ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
     ++                else
     ++			ICONV_LINK =
     ++                endif
     ++                ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
     ++			ICONV_LINK += -lintl
     ++                endif
     ++		EXTLIBS += $(ICONV_LINK) -liconv
     ++        endif
     ++endif
     ++ifndef LIBC_CONTAINS_LIBINTL
     ++	EXTLIBS += -lintl
     ++endif
     ++
     + prefix ?= /usr/local
     + gitexecdir ?= $(prefix)/libexec/git-core
     + 
     + CC ?= gcc
     +-CFLAGS ?= -g -O2 -Wall
     ++CFLAGS ?= -g -O2 -Wall -I../../.. $(BASIC_CFLAGS)
     ++LDFLAGS ?= $(BASIC_LDFLAGS) $(EXTLIBS)
     + INSTALL ?= install
     + RM ?= rm -f
     + 
     + %.o: %.c
     + 	$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
     + 
     +-git-credential-osxkeychain: git-credential-osxkeychain.o
     ++git-credential-osxkeychain: git-credential-osxkeychain.o ../../../libgit.a
     + 	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
     + 		-framework Security -framework CoreFoundation
     + 
     +@@ contrib/credential/osxkeychain/Makefile: install: git-credential-osxkeychain
     + 	$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
     + 	$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
     + 
     ++../../../libgit.a:
     ++	cd ../../..; make libgit.a
     ++
     + clean:
     + 	$(RM) git-credential-osxkeychain git-credential-osxkeychain.o
     + 
     +
       ## contrib/credential/osxkeychain/git-credential-osxkeychain.c ##
     +@@
     + #include <string.h>
     + #include <stdlib.h>
     + #include <Security/Security.h>
     ++#include "git-compat-util.h"
     ++#include "strbuf.h"
     ++#include "wrapper.h"
     + 
     + #define ENCODING kCFStringEncodingUTF8
     + static CFStringRef protocol; /* Stores constant strings - not memory managed */
      @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static CFStringRef username;
       static CFDataRef password;
       static CFDataRef password_expiry_utc;
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static CFStringRef
       
       static void clear_credential(void)
       {
     -@@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void die(const char *err, ...)
     - 	exit(1);
     - }
     +@@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void clear_credential(void)
       
     -+/*
     -+ * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
     -+ * introduce significant dependencies. Therefore, we define simplified
     -+ * versions here to keep this code self-contained.
     -+ */
     -+
     - static void *xmalloc(size_t len)
     - {
     - 	void *ret = malloc(len);
     -@@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void *xmalloc(size_t len)
     - 	return ret;
     - }
     + #define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
       
     -+static void *xcalloc(size_t count, size_t size)
     -+{
     -+	void *ret = calloc(count, size);
     -+	if (!ret)
     -+		die("Out of memory");
     -+	return ret;
     -+}
     -+
     -+static void *xrealloc(void *ptr, size_t size)
     -+{
     -+	void *ret = realloc(ptr, size);
     -+	if (!ret)
     -+		die("Out of memory");
     -+	return ret;
     -+}
     -+
     -+static char *xstrdup(const char *str)
     -+{
     -+	char *ret = strdup(str);
     -+	if (!ret)
     -+		die("Out of memory");
     -+	return ret;
     -+}
     -+
     +-__attribute__((format (printf, 1, 2), __noreturn__))
     +-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);
     +-	clear_credential();
     +-	exit(1);
     +-}
     +-
     +-static void *xmalloc(size_t len)
     +-{
     +-	void *ret = malloc(len);
     +-	if (!ret)
     +-		die("Out of memory");
     +-	return ret;
     +-}
     +-
       static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
       {
       	va_list args;
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void write_i
       	putchar('\n');
       }
       
     -+struct sb {
     -+	char *buf;
     -+	int size;
     -+};
     -+
     -+static void sb_init(struct sb *sb)
     -+{
     -+	sb->size = 1024;
     -+	sb->buf = xcalloc(sb->size, 1);
     -+}
     -+
     -+static void sb_release(struct sb *sb)
     -+{
     -+	if (sb->buf) {
     -+		free(sb->buf);
     -+		sb->buf = NULL;
     -+		sb->size = 0;
     -+	}
     -+}
     -+
     -+static void sb_add(struct sb *sb, const char *s, int n)
     -+{
     -+	int len = strlen(sb->buf);
     -+	int size = sb->size;
     -+	if (size < len + n + 1) {
     -+		sb->size = len + n + 1;
     -+		sb->buf = xrealloc(sb->buf, sb->size);
     -+	}
     -+	strncat(sb->buf, s, n);
     -+	sb->buf[len + n] = '\0';
     -+}
     -+
     -+static void write_item_sb(struct sb *sb, const char *what, const char *buf, int n)
     ++static void write_item_strbuf(struct strbuf *sb, const char *what, const char *buf, int n)
      +{
      +	char s[32];
      +
     -+	sprintf(s, "__%s=", what);
     -+	sb_add(sb, s, strlen(s));
     -+	sb_add(sb, buf, n);
     ++	xsnprintf(s, sizeof(s), "__%s=", what);
     ++	strbuf_add(sb, s, strlen(s));
     ++	strbuf_add(sb, buf, n);
      +}
      +
     -+static void write_item_sb_cfstring(struct sb *sb, const char *what, CFStringRef ref)
     ++static void write_item_strbuf_cfstring(struct strbuf *sb, const char *what, CFStringRef ref)
      +{
      +	char *buf;
      +	int len;
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void write_i
      +	len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
      +	buf = xmalloc(len);
      +	if (CFStringGetCString(ref, buf, len, ENCODING))
     -+		write_item_sb(sb, what, buf, strlen(buf));
     ++		write_item_strbuf(sb, what, buf, strlen(buf));
      +	free(buf);
      +}
      +
     -+static void write_item_sb_cfnumber(struct sb *sb, const char *what, CFNumberRef ref)
     ++static void write_item_strbuf_cfnumber(struct strbuf *sb, const char *what, CFNumberRef ref)
      +{
      +	short n;
      +	char buf[32];
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void write_i
      +		return;
      +	if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
      +		return;
     -+	sprintf(buf, "%d", n);
     -+	write_item_sb(sb, what, buf, strlen(buf));
     ++	xsnprintf(buf, sizeof(buf), "%d", n);
     ++	write_item_strbuf(sb, what, buf, strlen(buf));
      +}
      +
     -+static void write_item_sb_cfdata(struct sb *sb, const char *what, CFDataRef ref)
     ++static void write_item_strbuf_cfdata(struct strbuf *sb, const char *what, CFDataRef ref)
      +{
      +	char *buf;
      +	int len;
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static void write_i
      +	if (!buf || strlen(buf) == 0)
      +		return;
      +	len = CFDataGetLength(ref);
     -+	write_item_sb(sb, what, buf, len);
     ++	write_item_strbuf(sb, what, buf, len);
      +}
      +
     -+static void encode_state_seen(struct sb *sb)
     ++static void encode_state_seen(struct strbuf *sb)
      +{
     -+	sb_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
     -+	write_item_sb_cfstring(sb, "host", host);
     -+	write_item_sb_cfnumber(sb, "port", port);
     -+	write_item_sb_cfstring(sb, "path", path);
     -+	write_item_sb_cfstring(sb, "username", username);
     -+	write_item_sb_cfdata(sb, "password", password);
     ++	strbuf_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
     ++	write_item_strbuf_cfstring(sb, "host", host);
     ++	write_item_strbuf_cfnumber(sb, "port", port);
     ++	write_item_strbuf_cfstring(sb, "path", path);
     ++	write_item_strbuf_cfstring(sb, "username", username);
     ++	write_item_strbuf_cfdata(sb, "password", password);
      +}
      +
       static void find_username_in_item(CFDictionaryRef item)
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static OSStatus fin
       	write_item("capability[]", "state", strlen("state"));
      -	write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
      +	{
     -+		struct sb sb;
     ++		struct strbuf sb;
      +
     -+		sb_init(&sb);
     ++		strbuf_init(&sb, 1024);
      +		encode_state_seen(&sb);
      +		write_item("state[]", sb.buf, strlen(sb.buf));
     -+		sb_release(&sb);
     ++		strbuf_release(&sb);
      +	}
       
       out:
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static OSStatus add
       		return -1;
       
      +	if (state_seen) {
     -+		struct sb sb;
     ++		struct strbuf sb;
      +
     -+		sb_init(&sb);
     ++		strbuf_init(&sb, 1024);
      +		encode_state_seen(&sb);
      +		if (!strcmp(state_seen, sb.buf)) {
     -+			sb_release(&sb);
     ++			strbuf_release(&sb);
      +			return errSecSuccess;
      +		}
     -+		sb_release(&sb);
     ++		strbuf_release(&sb);
      +	}
      +
       	data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: int main(int argc,
      +
       	return 0;
       }
     +
     + ## contrib/credential/osxkeychain/meson.build ##
     +@@
     + executable('git-credential-osxkeychain',
     +   sources: 'git-credential-osxkeychain.c',
     +   dependencies: [
     ++    libgit,
     +     dependency('CoreFoundation'),
     +     dependency('Security'),
     +   ],


 contrib/credential/osxkeychain/Makefile       |  41 +++++-
 .../osxkeychain/git-credential-osxkeychain.c  | 120 ++++++++++++++----
 contrib/credential/osxkeychain/meson.build    |   1 +
 3 files changed, 132 insertions(+), 30 deletions(-)

diff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile
index 9680717abe..c68445b82d 100644
--- a/contrib/credential/osxkeychain/Makefile
+++ b/contrib/credential/osxkeychain/Makefile
@@ -1,21 +1,55 @@
 # The default target of this Makefile is...
 all:: git-credential-osxkeychain
 
+include ../../../config.mak.uname
 -include ../../../config.mak.autogen
 -include ../../../config.mak
 
+ifdef ZLIB_NG
+	BASIC_CFLAGS += -DHAVE_ZLIB_NG
+        ifdef ZLIB_NG_PATH
+		BASIC_CFLAGS += -I$(ZLIB_NG_PATH)/include
+		EXTLIBS += $(call libpath_template,$(ZLIB_NG_PATH)/$(lib))
+        endif
+	EXTLIBS += -lz-ng
+else
+        ifdef ZLIB_PATH
+		BASIC_CFLAGS += -I$(ZLIB_PATH)/include
+		EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
+        endif
+	EXTLIBS += -lz
+endif
+ifndef NO_ICONV
+        ifdef NEEDS_LIBICONV
+                ifdef ICONVDIR
+			BASIC_CFLAGS += -I$(ICONVDIR)/include
+			ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
+                else
+			ICONV_LINK =
+                endif
+                ifdef NEEDS_LIBINTL_BEFORE_LIBICONV
+			ICONV_LINK += -lintl
+                endif
+		EXTLIBS += $(ICONV_LINK) -liconv
+        endif
+endif
+ifndef LIBC_CONTAINS_LIBINTL
+	EXTLIBS += -lintl
+endif
+
 prefix ?= /usr/local
 gitexecdir ?= $(prefix)/libexec/git-core
 
 CC ?= gcc
-CFLAGS ?= -g -O2 -Wall
+CFLAGS ?= -g -O2 -Wall -I../../.. $(BASIC_CFLAGS)
+LDFLAGS ?= $(BASIC_LDFLAGS) $(EXTLIBS)
 INSTALL ?= install
 RM ?= rm -f
 
 %.o: %.c
 	$(CC) $(CFLAGS) $(CPPFLAGS) -o $@ -c $<
 
-git-credential-osxkeychain: git-credential-osxkeychain.o
+git-credential-osxkeychain: git-credential-osxkeychain.o ../../../libgit.a
 	$(CC) $(CFLAGS) -o $@ $^ $(LDFLAGS) \
 		-framework Security -framework CoreFoundation
 
@@ -23,6 +57,9 @@ install: git-credential-osxkeychain
 	$(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir)
 	$(INSTALL) -m 755 $< $(DESTDIR)$(gitexecdir)
 
+../../../libgit.a:
+	cd ../../..; make libgit.a
+
 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
index 611c9798b3..b180267034 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -2,6 +2,9 @@
 #include <string.h>
 #include <stdlib.h>
 #include <Security/Security.h>
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "wrapper.h"
 
 #define ENCODING kCFStringEncodingUTF8
 static CFStringRef protocol; /* Stores constant strings - not memory managed */
@@ -12,7 +15,7 @@ static CFStringRef username;
 static CFDataRef password;
 static CFDataRef password_expiry_utc;
 static CFDataRef oauth_refresh_token;
-static int state_seen;
+static char *state_seen;
 
 static void clear_credential(void)
 {
@@ -48,27 +51,6 @@ static void clear_credential(void)
 
 #define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
 
-__attribute__((format (printf, 1, 2), __noreturn__))
-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);
-	clear_credential();
-	exit(1);
-}
-
-static void *xmalloc(size_t len)
-{
-	void *ret = malloc(len);
-	if (!ret)
-		die("Out of memory");
-	return ret;
-}
-
 static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)
 {
 	va_list args;
@@ -112,6 +94,66 @@ static void write_item(const char *what, const char *buf, size_t len)
 	putchar('\n');
 }
 
+static void write_item_strbuf(struct strbuf *sb, const char *what, const char *buf, int n)
+{
+	char s[32];
+
+	xsnprintf(s, sizeof(s), "__%s=", what);
+	strbuf_add(sb, s, strlen(s));
+	strbuf_add(sb, buf, n);
+}
+
+static void write_item_strbuf_cfstring(struct strbuf *sb, const char *what, CFStringRef ref)
+{
+	char *buf;
+	int len;
+
+	if (!ref)
+		return;
+	len = CFStringGetMaximumSizeForEncoding(CFStringGetLength(ref), ENCODING) + 1;
+	buf = xmalloc(len);
+	if (CFStringGetCString(ref, buf, len, ENCODING))
+		write_item_strbuf(sb, what, buf, strlen(buf));
+	free(buf);
+}
+
+static void write_item_strbuf_cfnumber(struct strbuf *sb, const char *what, CFNumberRef ref)
+{
+	short n;
+	char buf[32];
+
+	if (!ref)
+		return;
+	if (!CFNumberGetValue(ref, kCFNumberShortType, &n))
+		return;
+	xsnprintf(buf, sizeof(buf), "%d", n);
+	write_item_strbuf(sb, what, buf, strlen(buf));
+}
+
+static void write_item_strbuf_cfdata(struct strbuf *sb, const char *what, CFDataRef ref)
+{
+	char *buf;
+	int len;
+
+	if (!ref)
+		return;
+	buf = (char *)CFDataGetBytePtr(ref);
+	if (!buf || strlen(buf) == 0)
+		return;
+	len = CFDataGetLength(ref);
+	write_item_strbuf(sb, what, buf, len);
+}
+
+static void encode_state_seen(struct strbuf *sb)
+{
+	strbuf_add(sb, "osxkeychain:seen=", strlen("osxkeychain:seen="));
+	write_item_strbuf_cfstring(sb, "host", host);
+	write_item_strbuf_cfnumber(sb, "port", port);
+	write_item_strbuf_cfstring(sb, "path", path);
+	write_item_strbuf_cfstring(sb, "username", username);
+	write_item_strbuf_cfdata(sb, "password", password);
+}
+
 static void find_username_in_item(CFDictionaryRef item)
 {
 	CFStringRef account_ref;
@@ -124,6 +166,7 @@ static void find_username_in_item(CFDictionaryRef item)
 		write_item("username", "", 0);
 		return;
 	}
+	username = CFStringCreateCopy(kCFAllocatorDefault, account_ref);
 
 	username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);
 	if (username_buf)
@@ -163,6 +206,7 @@ static OSStatus find_internet_password(void)
 	}
 
 	data = CFDictionaryGetValue(item, kSecValueData);
+	password = CFDataCreateCopy(kCFAllocatorDefault, data);
 
 	write_item("password",
 		   (const char *)CFDataGetBytePtr(data),
@@ -173,7 +217,14 @@ static OSStatus find_internet_password(void)
 	CFRelease(item);
 
 	write_item("capability[]", "state", strlen("state"));
-	write_item("state[]", "osxkeychain:seen=1", strlen("osxkeychain:seen=1"));
+	{
+		struct strbuf sb;
+
+		strbuf_init(&sb, 1024);
+		encode_state_seen(&sb);
+		write_item("state[]", sb.buf, strlen(sb.buf));
+		strbuf_release(&sb);
+	}
 
 out:
 	CFRelease(attrs);
@@ -288,13 +339,22 @@ static OSStatus add_internet_password(void)
 	CFDictionaryRef attrs;
 	OSStatus result;
 
-	if (state_seen)
-		return errSecSuccess;
-
 	/* Only store complete credentials */
 	if (!protocol || !host || !username || !password)
 		return -1;
 
+	if (state_seen) {
+		struct strbuf sb;
+
+		strbuf_init(&sb, 1024);
+		encode_state_seen(&sb);
+		if (!strcmp(state_seen, sb.buf)) {
+			strbuf_release(&sb);
+			return errSecSuccess;
+		}
+		strbuf_release(&sb);
+	}
+
 	data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
 	if (password_expiry_utc) {
 		CFDataAppendBytes(data,
@@ -403,8 +463,9 @@ static void read_credential(void)
 							   (UInt8 *)v,
 							   strlen(v));
 		else if (!strcmp(buf, "state[]")) {
-			if (!strcmp(v, "osxkeychain:seen=1"))
-				state_seen = 1;
+			int len = strlen("osxkeychain:seen=");
+			if (!strncmp(v, "osxkeychain:seen=", len))
+				state_seen = xstrdup(v);
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
@@ -443,5 +504,8 @@ int main(int argc, const char **argv)
 
 	clear_credential();
 
+	if (state_seen)
+		free(state_seen);
+
 	return 0;
 }
diff --git a/contrib/credential/osxkeychain/meson.build b/contrib/credential/osxkeychain/meson.build
index 3c7677f736..ec91d0c14b 100644
--- a/contrib/credential/osxkeychain/meson.build
+++ b/contrib/credential/osxkeychain/meson.build
@@ -1,6 +1,7 @@
 executable('git-credential-osxkeychain',
   sources: 'git-credential-osxkeychain.c',
   dependencies: [
+    libgit,
     dependency('CoreFoundation'),
     dependency('Security'),
   ],

base-commit: fd372d9b1a69a01a676398882bbe3840bf51fe72
-- 
gitgitgadget

Re: [PATCH] osxkeychain: avoid incorrectly skipping store operation

[Jeff King]

On Thu, Nov 13, 2025 at 12:28:15PM -0800, Junio C Hamano wrote:

> "Koji Nakamaru via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > +/*
> > + * NOTE: We could use functions in strbuf.h and/or wrapper.h, but those
> > + * introduce significant dependencies. Therefore, we define simplified
> > + * versions here to keep this code self-contained.
> > + */
> 
> Sorry, but I do not quite understand this comment.  The program is
> shipped as a part of Git, and using these functions and linking with
> libgit.a may pull strbuf.o and some other *.o files out of libgit.a
> to link with git-credential-osxkeychain.o to produce the executable,
> but how can that be "significant dependencies"?  For anybody who is
> building git-credential-osxkeychain, the necessary sources come for
> free.

Back when we added the contrib/credential helpers, I tried to avoid
linking with Git for two reasons:

  1. The idea was that these _could_ be independent projects, and we
     would not be on the hook for writing or maintaining everyone's pet
     platform helper. So even though they are in our tree, the hope was
     that they'd be simple enough to be totally independent programs
     (and would not even have to be written in C). And avoiding any
     dependencies kept us honest there.

     It may be that the cost of not being able to re-use our usual code
     is too high for the philosophical benefit, though.

  2. If stuff in contrib/ depends on code in libgit.a, then changes in
     the latter can break them. And I don't think we have a great flow
     for detecting such breakage. Maybe one of the CI jobs builds
     osxkeychain now? I'm not even sure.

     The xmalloc and strbuf interfaces are pretty stable, so it may be
     that the right rule is "you can depend on libgit.a, but only
     lightly".

Mostly just offering my two cents (and a little backstory). I'm not
terribly opposed to loosening the rule, but we may expect some breakage
via (2) from time to time.

-Peff