diff mbox series

[4/4] osxkeychain: store new attributes

Message ID f18435b2189bb08bcdba3b28523db1d4484f66cf.1708212896.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit d5b35bba86e6fdf0484ea71bf5b8ef1167f14015
Headers show
Series osxkeychain: bring in line with other credential helpers | expand

Commit Message

Bo Anderson Feb. 17, 2024, 11:34 p.m. UTC 
From: Bo Anderson <mail@boanderson.me>

d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token,
2023-04-21) introduced new credential attributes but support was missing
from git-credential-osxkeychain.

Support these attributes by appending the data to the password in the
keychain, separated by line breaks. Line breaks cannot appear in a git
credential password so it is an appropriate separator.

Fixes the remaining test failures with osxkeychain:

    18 - helper (osxkeychain) gets password_expiry_utc
    19 - helper (osxkeychain) overwrites when password_expiry_utc
    changes
    21 - helper (osxkeychain) gets oauth_refresh_token

Signed-off-by: Bo Anderson <mail@boanderson.me>
---
 .../osxkeychain/git-credential-osxkeychain.c  | 68 +++++++++++++++++--
 1 file changed, 62 insertions(+), 6 deletions(-)

Comments

Eric Sunshine Feb. 18, 2024, 6:31 a.m. UTC | #1 
On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)
> and a5c76569e7 (credential: new attribute oauth_refresh_token,
> 2023-04-21) introduced new credential attributes but support was missing
> from git-credential-osxkeychain.
> [...]
> Signed-off-by: Bo Anderson <mail@boanderson.me>
> ---
> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
> @@ -6,10 +6,12 @@
>  static CFStringRef host;
> +static CFNumberRef port;
>  static CFStringRef path;
> -static CFNumberRef port;
> @@ -17,6 +19,10 @@ static void clear_credential(void)
> +       if (port) {
> +               CFRelease(port);
> +               port = NULL;
> +       }
> @@ -29,12 +35,18 @@ static void clear_credential(void)
> -       if (port) {
> -               CFRelease(port);
> -               port = NULL;
> +       if (password_expiry_utc) {
> +               CFRelease(password_expiry_utc);
> +               password_expiry_utc = NULL;
> +       }

The relocation of `port` is unrelated to the stated purpose of this
patch. We would normally avoid this sort of "noise" change since it
obscures the "real" changes made by the patch, and would instead place
it in its own patch. That said, it's such a minor issue, I doubt that
it's worth a reroll.
diff mbox series

Patch

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 9e742796336..6a40917b1ef 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -6,10 +6,12 @@ 
 #define ENCODING kCFStringEncodingUTF8
 static CFStringRef protocol; /* Stores constant strings - not memory managed */
 static CFStringRef host;
+static CFNumberRef port;
 static CFStringRef path;
 static CFStringRef username;
 static CFDataRef password;
-static CFNumberRef port;
+static CFDataRef password_expiry_utc;
+static CFDataRef oauth_refresh_token;
 
 static void clear_credential(void)
 {
@@ -17,6 +19,10 @@  static void clear_credential(void)
 		CFRelease(host);
 		host = NULL;
 	}
+	if (port) {
+		CFRelease(port);
+		port = NULL;
+	}
 	if (path) {
 		CFRelease(path);
 		path = NULL;
@@ -29,12 +35,18 @@  static void clear_credential(void)
 		CFRelease(password);
 		password = NULL;
 	}
-	if (port) {
-		CFRelease(port);
-		port = NULL;
+	if (password_expiry_utc) {
+		CFRelease(password_expiry_utc);
+		password_expiry_utc = NULL;
+	}
+	if (oauth_refresh_token) {
+		CFRelease(oauth_refresh_token);
+		oauth_refresh_token = NULL;
 	}
 }
 
+#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1
+
 __attribute__((format (printf, 1, 2), __noreturn__))
 static void die(const char *err, ...)
 {
@@ -197,9 +209,27 @@  static OSStatus delete_ref(const void *itemRef)
 		CFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue);
 		result = SecItemCopyMatching(query, (CFTypeRef *)&data);
 		if (!result) {
-			if (CFEqual(data, password))
+			CFDataRef kc_password;
+			const UInt8 *raw_data;
+			const UInt8 *line;
+
+			/* Don't match appended metadata */
+			raw_data = CFDataGetBytePtr(data);
+			line = memchr(raw_data, '\n', CFDataGetLength(data));
+			if (line)
+				kc_password = CFDataCreateWithBytesNoCopy(
+						kCFAllocatorDefault,
+						raw_data,
+						line - raw_data,
+						kCFAllocatorNull);
+			else
+				kc_password = data;
+
+			if (CFEqual(kc_password, password))
 				result = SecItemDelete(delete_query);
 
+			if (line)
+				CFRelease(kc_password);
 			CFRelease(data);
 		}
 
@@ -250,6 +280,7 @@  static OSStatus delete_internet_password(void)
 
 static OSStatus add_internet_password(void)
 {
+	CFMutableDataRef data;
 	CFDictionaryRef attrs;
 	OSStatus result;
 
@@ -257,7 +288,23 @@  static OSStatus add_internet_password(void)
 	if (!protocol || !host || !username || !password)
 		return -1;
 
-	attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,
+	data = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);
+	if (password_expiry_utc) {
+		CFDataAppendBytes(data,
+		    (const UInt8 *)STRING_WITH_LENGTH("\npassword_expiry_utc="));
+		CFDataAppendBytes(data,
+				  CFDataGetBytePtr(password_expiry_utc),
+				  CFDataGetLength(password_expiry_utc));
+	}
+	if (oauth_refresh_token) {
+		CFDataAppendBytes(data,
+		    (const UInt8 *)STRING_WITH_LENGTH("\noauth_refresh_token="));
+		CFDataAppendBytes(data,
+				  CFDataGetBytePtr(oauth_refresh_token),
+				  CFDataGetLength(oauth_refresh_token));
+	}
+
+	attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, data,
 				      NULL);
 
 	result = SecItemAdd(attrs, NULL);
@@ -268,6 +315,7 @@  static OSStatus add_internet_password(void)
 		CFRelease(query);
 	}
 
+	CFRelease(data);
 	CFRelease(attrs);
 
 	return result;
@@ -339,6 +387,14 @@  static void read_credential(void)
 			password = CFDataCreate(kCFAllocatorDefault,
 						(UInt8 *)v,
 						strlen(v));
+		else if (!strcmp(buf, "password_expiry_utc"))
+			password_expiry_utc = CFDataCreate(kCFAllocatorDefault,
+							   (UInt8 *)v,
+							   strlen(v));
+		else if (!strcmp(buf, "oauth_refresh_token"))
+			oauth_refresh_token = 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