Skip to contentSkip to conversation

credential/osxkeychain: store new attributes

PATCH In review M Hickford via GitGitGadget 4 participants 6 messages Feb 13, 2024 – Feb 27, 2024

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 credential/osxkeychain: store new attributes

From: M Hickford <[email protected]>

d208bfd (credential: new attribute password_expiry_utc, 2023-02-18) and a5c76569e7 (credential: new attribute oauth_refresh_token) introduced new credential attributes.

Similar to 7144dee3 (credential/libsecret: erase matching creds only, 2023-07-26), we encode the new attributes in the secret, separated by newline:

    hunter2
    password_expiry_utc=1684189401
    oauth_refresh_token=xyzzy

This is extensible and backwards compatible. The credential protocol already assumes that attribute values do not contain newlines.

---
[RFC] contrib/credential/osxkeychain: store new attributes

Is any keen MacOS user interested in building and testing this RFC patch? I personally don't have a MacOS machine, so haven't tried building it. Fixes are surely necessary. Once it builds, you can test the feature with:

GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh

The feature would help git-credential-oauth users on MacOS
https://github.com/hickford/git-credential-oauth/issues/42

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1663%2Fhickford%2Fosxkeychain-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1663/hickford/osxkeychain-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1663

 .../osxkeychain/git-credential-osxkeychain.c  | 56 ++++++++++++++++++-
 1 file changed, 54 insertions(+), 2 deletions(-)
contrib/credential/osxkeychain/git-credential-osxkeychain.c +54 -2
index 5f2e5f16c88..25ffa84f4ba 100644
@@ -8,6 +8,8 @@ static char *host;
 static char *path;
 static char *username;
 static char *password;
+static char *password_expiry_utc;
+static char *oauth_refresh_token;
 static UInt16 port;
 
 __attribute__((format (printf, 1, 2)))
@@ -22,6 +24,17 @@ static void die(const char *err, ...)
 	exit(1);
 }
 
+
+static void *xmalloc(size_t size)
+{
+	void *ret = malloc(size);
+	if (!ret && !size)
+		ret = malloc(1);
+	if (!ret)
+		 die("Out of memory");
+	return ret;
+}
+
 static void *xstrdup(const char *s1)
 {
 	void *ret = strdup(s1);
@@ -69,11 +82,27 @@ static void find_internet_password(void)
 	void *buf;
 	UInt32 len;
 	SecKeychainItemRef item;
+	char *line;
+	char *remaining_lines;
+	char *part;
+	char *remaining_parts;
 
 	if (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))
 		return;
 
-	write_item("password", buf, len);
+	line = strtok_r(buf, "\n", &remaining_lines);
+	write_item("password", line, strlen(line));
+	while(line != NULL) {
+		part = strtok_r(line, "=", &remaining_parts);
+		if (!strcmp(part, "oauth_refresh_token")) {
+			write_item("oauth_refresh_token", remaining_parts, strlen(remaining_parts));
+		}
+		if (!strcmp(part, "password_expiry_utc")) {
+			write_item("password_expiry_utc", remaining_parts, strlen(remaining_parts));
+		}
+		line = strtok_r(NULL, "\n", &remaining_lines);
+	}
+
 	if (!username)
 		find_username_in_item(item);
 
@@ -100,13 +129,32 @@ static void delete_internet_password(void)
 
 static void add_internet_password(void)
 {
+	int len;
+
 	/* Only store complete credentials */
 	if (!protocol || !host || !username || !password)
 		return;
 
+	char *secret;
+	if (password_expiry_utc && oauth_refresh_token) {
+		len = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen("\npassword_expiry_utc=\noauth_refresh_token=");
+		secret = xmalloc(len);
+		snprintf(secret, len, len, "%s\npassword_expiry_utc=%s\noauth_refresh_token=%s", password, oauth_refresh_token);
+	} else if (oauth_refresh_token) {
+		len = strlen(password) + strlen(oauth_refresh_token) + strlen("\noauth_refresh_token=");
+		secret = xmalloc(len);
+		snprintf(secret, len, len, "%s\noauth_refresh_token=%s", password, oauth_refresh_token);
+	} else if (password_expiry_utc) {
+		len = strlen(password) + strlen(password_expiry_utc) + strlen("\npassword_expiry_utc=");
+		secret = xmalloc(len);
+		snprintf(secret, len, len, "%s\npassword_expiry_utc=%s", password, password_expiry_utc);
+	} else {
+		secret = xstrdup(password);
+	}
+
 	if (SecKeychainAddInternetPassword(
 	      KEYCHAIN_ARGS,
-	      KEYCHAIN_ITEM(password),
+	      KEYCHAIN_ITEM(secret),
 	      NULL))
 		return;
 }
@@ -161,6 +209,10 @@ static void read_credential(void)
 			username = xstrdup(v);
 		else if (!strcmp(buf, "password"))
 			password = xstrdup(v);
+		else if (!strcmp(buf, "password_expiry_utc"))
+			password_expiry_utc = xstrdup(v);
+		else if (!strcmp(buf, "oauth_refresh_token"))
+			oauth_refresh_token = xstrdup(v);
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do

base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 14 lines · M Hickford via GitGitGadget

From: M Hickford <[email protected]>

d208bfd (credential: new attribute password_expiry_utc, 2023-02-18) and a5c76569e7 (credential: new attribute oauth_refresh_token) introduced new credential attributes.

Similar to 7144dee3 (credential/libsecret: erase matching creds only, 2023-07-26), we encode the new attributes in the secret, separated by newline:

    hunter2
    password_expiry_utc=1684189401
    oauth_refresh_token=xyzzy
Quoted 5 lines

This is extensible and backwards compatible. The credential protocol already assumes that attribute values do not contain newlines.

---

OK, this adds both oauth_refresh_token and password_expiry_utc, unlike the recent one for wincred, which already stored the expiry but the support for oauth_refresh_token was added with f061959e (credential/wincred: store oauth_refresh_token, 2024-01-28).

Quoted 7 lines

[RFC] contrib/credential/osxkeychain: store new attributes

Is any keen MacOS user interested in building and testing this RFC patch? I personally don't have a MacOS machine, so haven't tried building it. Fixes are surely necessary. Once it builds, you can test the feature with:

Quoted 5 lines

GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh

The feature would help git-credential-oauth users on MacOS
https://github.com/hickford/git-credential-oauth/issues/42

I do not use macOS to use this on, so let's see how others can help.

Thanks. Will queue.

#2 lore raw
Quoted 38 lines · Junio C Hamano
Quoted 14 lines · M Hickford via GitGitGadget

From: M Hickford <[email protected]>

d208bfd (credential: new attribute password_expiry_utc, 2023-02-18) and a5c76569e7 (credential: new attribute oauth_refresh_token) introduced new credential attributes.

Similar to 7144dee3 (credential/libsecret: erase matching creds only, 2023-07-26), we encode the new attributes in the secret, separated by newline:

    hunter2
    password_expiry_utc=1684189401
    oauth_refresh_token=xyzzy
Quoted 5 lines

This is extensible and backwards compatible. The credential protocol already assumes that attribute values do not contain newlines.

---

OK, this adds both oauth_refresh_token and password_expiry_utc, unlike the recent one for wincred, which already stored the expiry but the support for oauth_refresh_token was added with f061959e (credential/wincred: store oauth_refresh_token, 2024-01-28).

Quoted 10 lines

[RFC] contrib/credential/osxkeychain: store new attributes

Is any keen MacOS user interested in building and testing this RFC patch? I personally don't have a MacOS machine, so haven't tried building it. Fixes are surely necessary. Once it builds, you can test the feature with:

GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh

Quoted 6 lines
Quoted 2 lines

The feature would help git-credential-oauth users on MacOS
https://github.com/hickford/git-credential-oauth/issues/42

I do not use macOS to use this on, so let's see how others can help.

Thanks. Will queue.

A first-time contributor contacted me to say they are working on a more comprehensive patch to credential-osxkeychain, so let's wait for that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116

#4 lore raw
Quoted 10 lines · M Hickford
Quoted 6 lines · M Hickford
Quoted 2 lines

Thanks. Will queue.

A first-time contributor contacted me to say they are working on a more comprehensive patch to credential-osxkeychain, so let's wait for that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116

Please disregard my patch and look at Bo Anderson's instead
https://lore.kernel.org/git/[email protected]/

Will drop mh/credential-oauth-refresh-token-with-osxkeychain topic. The other one seemed to have got some reviews and I think the current status of the series is that it is Bo's turn to respond with a new iteration of the series.

Thanks.

#5 lore raw
In reply to M Hickford via GitGitGadget
Quoted 6 lines · M Hickford via GitGitGadget

Is any keen MacOS user interested in building and testing this RFC patch? I personally don't have a MacOS machine, so haven't tried building it. Fixes are surely necessary. Once it builds, you can test the feature with:

GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh

You might also need:

GIT_TEST_CREDENTIAL_HELPER_SETUP="export HOME=$HOME"

according to 34961d30da (contrib: add credential helper for OS X Keychain, 2011-12-10). IIRC I also ran into problems trying to test over ssh, as those sessions did not have access to the keychain.

(Sorry, I haven't touched a mac since adding the helper back then, but maybe those hints will help somebody else).

Review

Quoted 4 lines

static void add_internet_password(void)
{

(quoted diff) +2 -0
+	int len;
+

This should probably be a size_t to avoid integer overflow for malicious inputs. I suspect it's hard to get a super-long string into the system. We do use the dynamic getline(), but stuff like host, user, etc, almost certainly comes from the user or from a URL that was passed over a command-line. Maybe oauth_refresh_token() could be long, though?

Anyway, probably better safe than sorry (though see below).

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void delete_internet_password(void) · line 138

Quoted 5 lines
contrib/credential/osxkeychain/git-credential-osxkeychain.c +1 -0
 	/* Only store complete credentials */
 	if (!protocol || !host || !username || !password)
 		return;
 
+	char *secret;

This is a decl-after-statement, which our style forbids (though I am happy to defer on style issues to anybody who volunteers to maintain a slice of contrib/, and I don't think we need to worry about pre-c99 compilers here).

contrib/credential/osxkeychain/git-credential-osxkeychain.c static void delete_internet_password(void) · line 142

Quoted 4 lines
contrib/credential/osxkeychain/git-credential-osxkeychain.c +4 -0
+	if (password_expiry_utc && oauth_refresh_token) {
+		len = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen("\npassword_expiry_utc=\noauth_refresh_token=");
+		secret = xmalloc(len);
+		snprintf(secret, len, len, "%s\npassword_expiry_utc=%s\noauth_refresh_token=%s", password, oauth_refresh_token);

Do you need to add one more byte to "len" for the NUL terminator?

I think there is also a mismatch in your snprintf call, which has three %s placeholders and only two var-args.

Since we added xmalloc() as a helper, I wonder if we could go just a little further with (totally untested):

__attribute__((format (printf, 1, 2)))
char *xstrfmt(const char *fmt, ...)

  {
          va_list ap, cp;
	  char *ret;
	  int len;

	  va_start(ap, fmt);

	  va_copy(cp, ap);
	  len = vsnprintf(NULL, 0, fmt, cp);
	  va_end(cp);

	  /*
	   * sadly we must use int for the length, since that's what the
	   * standard specifies. But good implementations will return a
	   * negative value if the resulting length would overflow.
	   */
	   if (len < 0)
	            die("xstrfmt string too long");

	   ret = xmalloc(len + 1);
	   vsnprintf(ret, len, fmt, ap);
	   va_end(ap);

	   return ret;

}

Then you can just write:

secret = xstrfmt("%s\npassword_expiry_utc=%s\noauth_refresh_token=%s",

                   password, password_expiry_utc, oauth_refresh_token);

Even across the three instances, I doubt it is saving any lines, but it is much easier to verify that we sized the buffer correctly and did not introduce an overflow.

-Peff