{"thread":{"id":"60943","subject":"[PATCH 0/4] osxkeychain: bring in line with other credential helpers","startedAt":"2024-02-17T23:34:59Z","lastAt":"2024-04-02T14:54:49Z","messageCount":19,"participants":["Bo Anderson via GitGitGadget","Eric Sunshine","Bo Anderson","M Hickford","Jeff King","Junio C Hamano","Robert Coup"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"488854","messageId":"pull.1667.git.1708212896.gitgitgadget@gmail.com","threadId":"60943","inReplyTo":null,"subject":"[PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-17T23:34:52Z","receivedAt":"2024-02-17T23:34:59Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"git-credential-osxkeychain has largely fallen behind other external\ncredential helpers in the features it supports, and hasn't received any\nfunctional changes since 2013. As it stood, osxkeychain failed seven tests\nin the external credential helper test suite:\n\nnot ok 8 - helper (osxkeychain) overwrites on store\nnot ok 9 - helper (osxkeychain) can forget host\nnot ok 11 - helper (osxkeychain) does not erase a password distinct from input\nnot ok 15 - helper (osxkeychain) erases all matching credentials\nnot ok 18 - helper (osxkeychain) gets password_expiry_utc\nnot ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes\nnot ok 21 - helper (osxkeychain) gets oauth_refresh_token\n\n\nosxkeychain also made use of macOS APIs that had been deprecated since 2014.\nReplacement API was able to be used without regressing the minimum supported\nmacOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer\nin osxkeychain, 2023-05-01).\n\nAfter this set of patches, osxkeychain passes all tests in the external\ncredential helper test suite.\n\nBo Anderson (4):\n  osxkeychain: replace deprecated SecKeychain API\n  osxkeychain: erase all matching credentials\n  osxkeychain: erase matching passwords only\n  osxkeychain: store new attributes\n\n contrib/credential/osxkeychain/Makefile       |   3 +-\n .../osxkeychain/git-credential-osxkeychain.c  | 376 ++++++++++++++----\n 2 files changed, 310 insertions(+), 69 deletions(-)\n\n\nbase-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1667%2FBo98%2Fosxkeychain-update-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1667/Bo98/osxkeychain-update-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1667\n-- \ngitgitgadget\n"},{"id":"488855","messageId":"f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"[PATCH 1/4] osxkeychain: replace deprecated SecKeychain API","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-17T23:34:53Z","receivedAt":"2024-02-17T23:35:00Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"From: Bo Anderson <mail@boanderson.me>\n\nThe SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.\nThe replacement SecItem API however is available as far back as macOS\n10.6.\n\nWhile supporting older macOS was perhaps prevously a concern,\ngit-credential-osxkeychain already requires a minimum of macOS 10.7\nsince 5747c8072b (contrib/credential: avoid fixed-size buffer in\nosxkeychain, 2023-05-01) so using the newer API should not regress the\nrange of macOS versions supported.\n\nAdapting to use the newer SecItem API also happens to fix two test\nfailures in osxkeychain:\n\n    8 - helper (osxkeychain) overwrites on store\n    9 - helper (osxkeychain) can forget host\n\nThe new API is compatible with credentials saved with the older API.\n\nSigned-off-by: Bo Anderson <mail@boanderson.me>\n---\n contrib/credential/osxkeychain/Makefile       |   3 +-\n .../osxkeychain/git-credential-osxkeychain.c  | 265 +++++++++++++-----\n 2 files changed, 199 insertions(+), 69 deletions(-)\n\ndiff --git a/contrib/credential/osxkeychain/Makefile b/contrib/credential/osxkeychain/Makefile\nindex 4b3a08a2bac..238f5f8c36f 100644\n--- a/contrib/credential/osxkeychain/Makefile\n+++ b/contrib/credential/osxkeychain/Makefile\n@@ -8,7 +8,8 @@ CFLAGS = -g -O2 -Wall\n -include ../../../config.mak\n \n git-credential-osxkeychain: git-credential-osxkeychain.o\n-\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) -Wl,-framework -Wl,Security\n+\t$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS) \\\n+\t\t-framework Security -framework CoreFoundation\n \n git-credential-osxkeychain.o: git-credential-osxkeychain.c\n \t$(CC) -c $(CFLAGS) $<\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 5f2e5f16c88..dc294ae944a 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -3,14 +3,39 @@\n #include <stdlib.h>\n #include <Security/Security.h>\n \n-static SecProtocolType protocol;\n-static char *host;\n-static char *path;\n-static char *username;\n-static char *password;\n-static UInt16 port;\n-\n-__attribute__((format (printf, 1, 2)))\n+#define ENCODING kCFStringEncodingUTF8\n+static CFStringRef protocol; /* Stores constant strings - not memory managed */\n+static CFStringRef host;\n+static CFStringRef path;\n+static CFStringRef username;\n+static CFDataRef password;\n+static CFNumberRef port;\n+\n+static void clear_credential(void)\n+{\n+\tif (host) {\n+\t\tCFRelease(host);\n+\t\thost = NULL;\n+\t}\n+\tif (path) {\n+\t\tCFRelease(path);\n+\t\tpath = NULL;\n+\t}\n+\tif (username) {\n+\t\tCFRelease(username);\n+\t\tusername = NULL;\n+\t}\n+\tif (password) {\n+\t\tCFRelease(password);\n+\t\tpassword = NULL;\n+\t}\n+\tif (port) {\n+\t\tCFRelease(port);\n+\t\tport = NULL;\n+\t}\n+}\n+\n+__attribute__((format (printf, 1, 2), __noreturn__))\n static void die(const char *err, ...)\n {\n \tchar msg[4096];\n@@ -19,70 +44,135 @@ static void die(const char *err, ...)\n \tvsnprintf(msg, sizeof(msg), err, params);\n \tfprintf(stderr, \"%s\\n\", msg);\n \tva_end(params);\n+\tclear_credential();\n \texit(1);\n }\n \n-static void *xstrdup(const char *s1)\n+static void *xmalloc(size_t len)\n {\n-\tvoid *ret = strdup(s1);\n+\tvoid *ret = malloc(len);\n \tif (!ret)\n \t\tdie(\"Out of memory\");\n \treturn ret;\n }\n \n-#define KEYCHAIN_ITEM(x) (x ? strlen(x) : 0), x\n-#define KEYCHAIN_ARGS \\\n-\tNULL, /* default keychain */ \\\n-\tKEYCHAIN_ITEM(host), \\\n-\t0, NULL, /* account domain */ \\\n-\tKEYCHAIN_ITEM(username), \\\n-\tKEYCHAIN_ITEM(path), \\\n-\tport, \\\n-\tprotocol, \\\n-\tkSecAuthenticationTypeDefault\n-\n-static void write_item(const char *what, const char *buf, int len)\n+static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)\n+{\n+\tva_list args;\n+\tconst void *key;\n+\tCFMutableDictionaryRef result;\n+\n+\tresult = CFDictionaryCreateMutable(allocator,\n+\t\t\t\t\t   0,\n+\t\t\t\t\t   &kCFTypeDictionaryKeyCallBacks,\n+\t\t\t\t\t   &kCFTypeDictionaryValueCallBacks);\n+\n+\n+\tva_start(args, allocator);\n+\twhile ((key = va_arg(args, const void *)) != NULL) {\n+\t\tconst void *value;\n+\t\tvalue = va_arg(args, const void *);\n+\t\tif (value)\n+\t\t\tCFDictionarySetValue(result, key, value);\n+\t}\n+\tva_end(args);\n+\n+\treturn result;\n+}\n+\n+#define CREATE_SEC_ATTRIBUTES(...) \\\n+\tcreate_dictionary(kCFAllocatorDefault, \\\n+\t\t\t  kSecClass, kSecClassInternetPassword, \\\n+\t\t\t  kSecAttrServer, host, \\\n+\t\t\t  kSecAttrAccount, username, \\\n+\t\t\t  kSecAttrPath, path, \\\n+\t\t\t  kSecAttrPort, port, \\\n+\t\t\t  kSecAttrProtocol, protocol, \\\n+\t\t\t  kSecAttrAuthenticationType, \\\n+\t\t\t  kSecAttrAuthenticationTypeDefault, \\\n+\t\t\t  __VA_ARGS__);\n+\n+static void write_item(const char *what, const char *buf, size_t len)\n {\n \tprintf(\"%s=\", what);\n \tfwrite(buf, 1, len, stdout);\n \tputchar('\\n');\n }\n \n-static void find_username_in_item(SecKeychainItemRef item)\n+static void find_username_in_item(CFDictionaryRef item)\n {\n-\tSecKeychainAttributeList list;\n-\tSecKeychainAttribute attr;\n+\tCFStringRef account_ref;\n+\tchar *username_buf;\n+\tCFIndex buffer_len;\n \n-\tlist.count = 1;\n-\tlist.attr = &attr;\n-\tattr.tag = kSecAccountItemAttr;\n+\taccount_ref = CFDictionaryGetValue(item, kSecAttrAccount);\n+\tif (!account_ref)\n+\t{\n+\t\twrite_item(\"username\", \"\", 0);\n+\t\treturn;\n+\t}\n \n-\tif (SecKeychainItemCopyContent(item, NULL, &list, NULL, NULL))\n+\tusername_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);\n+\tif (username_buf)\n+\t{\n+\t\twrite_item(\"username\", username_buf, strlen(username_buf));\n \t\treturn;\n+\t}\n \n-\twrite_item(\"username\", attr.data, attr.length);\n-\tSecKeychainItemFreeContent(&list, NULL);\n+\t/* If we can't get a CString pointer then\n+\t * we need to allocate our own buffer */\n+\tbuffer_len = CFStringGetMaximumSizeForEncoding(\n+\t\t\tCFStringGetLength(account_ref), ENCODING) + 1;\n+\tusername_buf = xmalloc(buffer_len);\n+\tif (CFStringGetCString(account_ref,\n+\t\t\t\tusername_buf,\n+\t\t\t\tbuffer_len,\n+\t\t\t\tENCODING)) {\n+\t\twrite_item(\"username\", username_buf, buffer_len - 1);\n+\t}\n+\tfree(username_buf);\n }\n \n-static void find_internet_password(void)\n+static OSStatus find_internet_password(void)\n {\n-\tvoid *buf;\n-\tUInt32 len;\n-\tSecKeychainItemRef item;\n+\tCFDictionaryRef attrs;\n+\tCFDictionaryRef item;\n+\tCFDataRef data;\n+\tOSStatus result;\n \n-\tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n-\t\treturn;\n+\tattrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,\n+\t\t\t\t      kSecReturnAttributes, kCFBooleanTrue,\n+\t\t\t\t      kSecReturnData, kCFBooleanTrue,\n+\t\t\t\t      NULL);\n+\tresult = SecItemCopyMatching(attrs, (CFTypeRef *)&item);\n+\tif (result) {\n+\t\tgoto out;\n+\t}\n \n-\twrite_item(\"password\", buf, len);\n+\tdata = CFDictionaryGetValue(item, kSecValueData);\n+\n+\twrite_item(\"password\",\n+\t\t   (const char *)CFDataGetBytePtr(data),\n+\t\t   CFDataGetLength(data));\n \tif (!username)\n \t\tfind_username_in_item(item);\n \n-\tSecKeychainItemFreeContent(NULL, buf);\n+\tCFRelease(item);\n+\n+out:\n+\tCFRelease(attrs);\n+\n+\t/* We consider not found to not be an error */\n+\tif (result == errSecItemNotFound)\n+\t\tresult = errSecSuccess;\n+\n+\treturn result;\n }\n \n-static void delete_internet_password(void)\n+static OSStatus delete_internet_password(void)\n {\n-\tSecKeychainItemRef item;\n+\tCFDictionaryRef attrs;\n+\tOSStatus result;\n \n \t/*\n \t * Require at least a protocol and host for removal, which is what git\n@@ -90,25 +180,42 @@ static void delete_internet_password(void)\n \t * Keychain manager.\n \t */\n \tif (!protocol || !host)\n-\t\treturn;\n+\t\treturn -1;\n \n-\tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, 0, NULL, &item))\n-\t\treturn;\n+\tattrs = CREATE_SEC_ATTRIBUTES(NULL);\n+\tresult = SecItemDelete(attrs);\n+\tCFRelease(attrs);\n+\n+\t/* We consider not found to not be an error */\n+\tif (result == errSecItemNotFound)\n+\t\tresult = errSecSuccess;\n \n-\tSecKeychainItemDelete(item);\n+\treturn result;\n }\n \n-static void add_internet_password(void)\n+static OSStatus add_internet_password(void)\n {\n+\tCFDictionaryRef attrs;\n+\tOSStatus result;\n+\n \t/* Only store complete credentials */\n \tif (!protocol || !host || !username || !password)\n-\t\treturn;\n+\t\treturn -1;\n \n-\tif (SecKeychainAddInternetPassword(\n-\t      KEYCHAIN_ARGS,\n-\t      KEYCHAIN_ITEM(password),\n-\t      NULL))\n-\t\treturn;\n+\tattrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,\n+\t\t\t\t      NULL);\n+\n+\tresult = SecItemAdd(attrs, NULL);\n+\tif (result == errSecDuplicateItem) {\n+\t\tCFDictionaryRef query;\n+\t\tquery = CREATE_SEC_ATTRIBUTES(NULL);\n+\t\tresult = SecItemUpdate(query, attrs);\n+\t\tCFRelease(query);\n+\t}\n+\n+\tCFRelease(attrs);\n+\n+\treturn result;\n }\n \n static void read_credential(void)\n@@ -131,36 +238,52 @@ static void read_credential(void)\n \n \t\tif (!strcmp(buf, \"protocol\")) {\n \t\t\tif (!strcmp(v, \"imap\"))\n-\t\t\t\tprotocol = kSecProtocolTypeIMAP;\n+\t\t\t\tprotocol = kSecAttrProtocolIMAP;\n \t\t\telse if (!strcmp(v, \"imaps\"))\n-\t\t\t\tprotocol = kSecProtocolTypeIMAPS;\n+\t\t\t\tprotocol = kSecAttrProtocolIMAPS;\n \t\t\telse if (!strcmp(v, \"ftp\"))\n-\t\t\t\tprotocol = kSecProtocolTypeFTP;\n+\t\t\t\tprotocol = kSecAttrProtocolFTP;\n \t\t\telse if (!strcmp(v, \"ftps\"))\n-\t\t\t\tprotocol = kSecProtocolTypeFTPS;\n+\t\t\t\tprotocol = kSecAttrProtocolFTPS;\n \t\t\telse if (!strcmp(v, \"https\"))\n-\t\t\t\tprotocol = kSecProtocolTypeHTTPS;\n+\t\t\t\tprotocol = kSecAttrProtocolHTTPS;\n \t\t\telse if (!strcmp(v, \"http\"))\n-\t\t\t\tprotocol = kSecProtocolTypeHTTP;\n+\t\t\t\tprotocol = kSecAttrProtocolHTTP;\n \t\t\telse if (!strcmp(v, \"smtp\"))\n-\t\t\t\tprotocol = kSecProtocolTypeSMTP;\n-\t\t\telse /* we don't yet handle other protocols */\n+\t\t\t\tprotocol = kSecAttrProtocolSMTP;\n+\t\t\telse {\n+\t\t\t\t/* we don't yet handle other protocols */\n+\t\t\t\tclear_credential();\n \t\t\t\texit(0);\n+\t\t\t}\n \t\t}\n \t\telse if (!strcmp(buf, \"host\")) {\n \t\t\tchar *colon = strchr(v, ':');\n \t\t\tif (colon) {\n+\t\t\t\tUInt16 port_i;\n \t\t\t\t*colon++ = '\\0';\n-\t\t\t\tport = atoi(colon);\n+\t\t\t\tport_i = atoi(colon);\n+\t\t\t\tport = CFNumberCreate(kCFAllocatorDefault,\n+\t\t\t\t\t\t      kCFNumberShortType,\n+\t\t\t\t\t\t      &port_i);\n \t\t\t}\n-\t\t\thost = xstrdup(v);\n+\t\t\thost = CFStringCreateWithCString(kCFAllocatorDefault,\n+\t\t\t\t\t\t\t v,\n+\t\t\t\t\t\t\t ENCODING);\n \t\t}\n \t\telse if (!strcmp(buf, \"path\"))\n-\t\t\tpath = xstrdup(v);\n+\t\t\tpath = CFStringCreateWithCString(kCFAllocatorDefault,\n+\t\t\t\t\t\t\t v,\n+\t\t\t\t\t\t\t ENCODING);\n \t\telse if (!strcmp(buf, \"username\"))\n-\t\t\tusername = xstrdup(v);\n+\t\t\tusername = CFStringCreateWithCString(\n+\t\t\t\t\tkCFAllocatorDefault,\n+\t\t\t\t\tv,\n+\t\t\t\t\tENCODING);\n \t\telse if (!strcmp(buf, \"password\"))\n-\t\t\tpassword = xstrdup(v);\n+\t\t\tpassword = CFDataCreate(kCFAllocatorDefault,\n+\t\t\t\t\t\t(UInt8 *)v,\n+\t\t\t\t\t\tstrlen(v));\n \t\t/*\n \t\t * Ignore other lines; we don't know what they mean, but\n \t\t * this future-proofs us when later versions of git do\n@@ -173,6 +296,7 @@ static void read_credential(void)\n \n int main(int argc, const char **argv)\n {\n+\tOSStatus result = 0;\n \tconst char *usage =\n \t\t\"usage: git credential-osxkeychain <get|store|erase>\";\n \n@@ -182,12 +306,17 @@ int main(int argc, const char **argv)\n \tread_credential();\n \n \tif (!strcmp(argv[1], \"get\"))\n-\t\tfind_internet_password();\n+\t\tresult = find_internet_password();\n \telse if (!strcmp(argv[1], \"store\"))\n-\t\tadd_internet_password();\n+\t\tresult = add_internet_password();\n \telse if (!strcmp(argv[1], \"erase\"))\n-\t\tdelete_internet_password();\n+\t\tresult = delete_internet_password();\n \t/* otherwise, ignore unknown action */\n \n+\tif (result)\n+\t\tdie(\"failed to %s: %d\", argv[1], (int)result);\n+\n+\tclear_credential();\n+\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"488856","messageId":"08284fa8e845e28da3c9a85d06475d5fbeb5cfcb.1708212896.git.gitgitgadget@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"[PATCH 2/4] osxkeychain: erase all matching credentials","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-17T23:34:54Z","receivedAt":"2024-02-17T23:35:02Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"From: Bo Anderson <mail@boanderson.me>\n\nOther credential managers erased all matching credentials, as indicated\nby a test case that osxkeychain failed:\n\n    15 - helper (osxkeychain) erases all matching credentials\n\nSigned-off-by: Bo Anderson <mail@boanderson.me>\n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex dc294ae944a..e9cee3aed45 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -182,7 +182,8 @@ static OSStatus delete_internet_password(void)\n \tif (!protocol || !host)\n \t\treturn -1;\n \n-\tattrs = CREATE_SEC_ATTRIBUTES(NULL);\n+\tattrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll,\n+\t\t\t\t      NULL);\n \tresult = SecItemDelete(attrs);\n \tCFRelease(attrs);\n \n-- \ngitgitgadget\n\n"},{"id":"488857","messageId":"f7ac228aae69941032d904c3c6222216786c1d0e.1708212896.git.gitgitgadget@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"[PATCH 3/4] osxkeychain: erase matching passwords only","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-17T23:34:55Z","receivedAt":"2024-02-17T23:35:02Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"From: Bo Anderson <mail@boanderson.me>\n\nOther credential helpers support deleting credentials that match a\nspecified password. See 7144dee3ec (credential/libsecret: erase matching\ncreds only, 2023-07-26) and cb626f8e5c (credential/wincred: erase\nmatching creds only, 2023-07-26).\n\nSupport this in osxkeychain too by extracting, decrypting and comparing\nthe stored password before deleting.\n\nFixes the following test failure with osxkeychain:\n\n    11 - helper (osxkeychain) does not erase a password distinct from\n    input\n\nSigned-off-by: Bo Anderson <mail@boanderson.me>\n---\n .../osxkeychain/git-credential-osxkeychain.c  | 56 ++++++++++++++++++-\n 1 file changed, 55 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex e9cee3aed45..9e742796336 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -169,9 +169,55 @@ static OSStatus find_internet_password(void)\n \treturn result;\n }\n \n+static OSStatus delete_ref(const void *itemRef)\n+{\n+\tCFArrayRef item_ref_list;\n+\tCFDictionaryRef delete_query;\n+\tOSStatus result;\n+\n+\titem_ref_list = CFArrayCreate(kCFAllocatorDefault,\n+\t\t\t\t      &itemRef,\n+\t\t\t\t      1,\n+\t\t\t\t      &kCFTypeArrayCallBacks);\n+\tdelete_query = create_dictionary(kCFAllocatorDefault,\n+\t\t\t\t\t kSecClass, kSecClassInternetPassword,\n+\t\t\t\t\t kSecMatchItemList, item_ref_list,\n+\t\t\t\t\t NULL);\n+\n+\tif (password) {\n+\t\t/* We only want to delete items with a matching password */\n+\t\tCFIndex capacity;\n+\t\tCFMutableDictionaryRef query;\n+\t\tCFDataRef data;\n+\n+\t\tcapacity = CFDictionaryGetCount(delete_query) + 1;\n+\t\tquery = CFDictionaryCreateMutableCopy(kCFAllocatorDefault,\n+\t\t\t\t\t\t      capacity,\n+\t\t\t\t\t\t      delete_query);\n+\t\tCFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue);\n+\t\tresult = SecItemCopyMatching(query, (CFTypeRef *)&data);\n+\t\tif (!result) {\n+\t\t\tif (CFEqual(data, password))\n+\t\t\t\tresult = SecItemDelete(delete_query);\n+\n+\t\t\tCFRelease(data);\n+\t\t}\n+\n+\t\tCFRelease(query);\n+\t} else {\n+\t\tresult = SecItemDelete(delete_query);\n+\t}\n+\n+\tCFRelease(delete_query);\n+\tCFRelease(item_ref_list);\n+\n+\treturn result;\n+}\n+\n static OSStatus delete_internet_password(void)\n {\n \tCFDictionaryRef attrs;\n+\tCFArrayRef refs;\n \tOSStatus result;\n \n \t/*\n@@ -183,10 +229,18 @@ static OSStatus delete_internet_password(void)\n \t\treturn -1;\n \n \tattrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitAll,\n+\t\t\t\t      kSecReturnRef, kCFBooleanTrue,\n \t\t\t\t      NULL);\n-\tresult = SecItemDelete(attrs);\n+\tresult = SecItemCopyMatching(attrs, (CFTypeRef *)&refs);\n \tCFRelease(attrs);\n \n+\tif (!result) {\n+\t\tfor (CFIndex i = 0; !result && i < CFArrayGetCount(refs); i++)\n+\t\t\tresult = delete_ref(CFArrayGetValueAtIndex(refs, i));\n+\n+\t\tCFRelease(refs);\n+\t}\n+\n \t/* We consider not found to not be an error */\n \tif (result == errSecItemNotFound)\n \t\tresult = errSecSuccess;\n-- \ngitgitgadget\n\n"},{"id":"488858","messageId":"f18435b2189bb08bcdba3b28523db1d4484f66cf.1708212896.git.gitgitgadget@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"[PATCH 4/4] osxkeychain: store new attributes","fromName":"Bo Anderson via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-17T23:34:56Z","receivedAt":"2024-02-17T23:35:03Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"From: Bo Anderson <mail@boanderson.me>\n\nd208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)\nand a5c76569e7 (credential: new attribute oauth_refresh_token,\n2023-04-21) introduced new credential attributes but support was missing\nfrom git-credential-osxkeychain.\n\nSupport these attributes by appending the data to the password in the\nkeychain, separated by line breaks. Line breaks cannot appear in a git\ncredential password so it is an appropriate separator.\n\nFixes the remaining test failures with osxkeychain:\n\n    18 - helper (osxkeychain) gets password_expiry_utc\n    19 - helper (osxkeychain) overwrites when password_expiry_utc\n    changes\n    21 - helper (osxkeychain) gets oauth_refresh_token\n\nSigned-off-by: Bo Anderson <mail@boanderson.me>\n---\n .../osxkeychain/git-credential-osxkeychain.c  | 68 +++++++++++++++++--\n 1 file changed, 62 insertions(+), 6 deletions(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 9e742796336..6a40917b1ef 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -6,10 +6,12 @@\n #define ENCODING kCFStringEncodingUTF8\n static CFStringRef protocol; /* Stores constant strings - not memory managed */\n static CFStringRef host;\n+static CFNumberRef port;\n static CFStringRef path;\n static CFStringRef username;\n static CFDataRef password;\n-static CFNumberRef port;\n+static CFDataRef password_expiry_utc;\n+static CFDataRef oauth_refresh_token;\n \n static void clear_credential(void)\n {\n@@ -17,6 +19,10 @@ static void clear_credential(void)\n \t\tCFRelease(host);\n \t\thost = NULL;\n \t}\n+\tif (port) {\n+\t\tCFRelease(port);\n+\t\tport = NULL;\n+\t}\n \tif (path) {\n \t\tCFRelease(path);\n \t\tpath = NULL;\n@@ -29,12 +35,18 @@ static void clear_credential(void)\n \t\tCFRelease(password);\n \t\tpassword = NULL;\n \t}\n-\tif (port) {\n-\t\tCFRelease(port);\n-\t\tport = NULL;\n+\tif (password_expiry_utc) {\n+\t\tCFRelease(password_expiry_utc);\n+\t\tpassword_expiry_utc = NULL;\n+\t}\n+\tif (oauth_refresh_token) {\n+\t\tCFRelease(oauth_refresh_token);\n+\t\toauth_refresh_token = NULL;\n \t}\n }\n \n+#define STRING_WITH_LENGTH(s) s, sizeof(s) - 1\n+\n __attribute__((format (printf, 1, 2), __noreturn__))\n static void die(const char *err, ...)\n {\n@@ -197,9 +209,27 @@ static OSStatus delete_ref(const void *itemRef)\n \t\tCFDictionarySetValue(query, kSecReturnData, kCFBooleanTrue);\n \t\tresult = SecItemCopyMatching(query, (CFTypeRef *)&data);\n \t\tif (!result) {\n-\t\t\tif (CFEqual(data, password))\n+\t\t\tCFDataRef kc_password;\n+\t\t\tconst UInt8 *raw_data;\n+\t\t\tconst UInt8 *line;\n+\n+\t\t\t/* Don't match appended metadata */\n+\t\t\traw_data = CFDataGetBytePtr(data);\n+\t\t\tline = memchr(raw_data, '\\n', CFDataGetLength(data));\n+\t\t\tif (line)\n+\t\t\t\tkc_password = CFDataCreateWithBytesNoCopy(\n+\t\t\t\t\t\tkCFAllocatorDefault,\n+\t\t\t\t\t\traw_data,\n+\t\t\t\t\t\tline - raw_data,\n+\t\t\t\t\t\tkCFAllocatorNull);\n+\t\t\telse\n+\t\t\t\tkc_password = data;\n+\n+\t\t\tif (CFEqual(kc_password, password))\n \t\t\t\tresult = SecItemDelete(delete_query);\n \n+\t\t\tif (line)\n+\t\t\t\tCFRelease(kc_password);\n \t\t\tCFRelease(data);\n \t\t}\n \n@@ -250,6 +280,7 @@ static OSStatus delete_internet_password(void)\n \n static OSStatus add_internet_password(void)\n {\n+\tCFMutableDataRef data;\n \tCFDictionaryRef attrs;\n \tOSStatus result;\n \n@@ -257,7 +288,23 @@ static OSStatus add_internet_password(void)\n \tif (!protocol || !host || !username || !password)\n \t\treturn -1;\n \n-\tattrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,\n+\tdata = CFDataCreateMutableCopy(kCFAllocatorDefault, 0, password);\n+\tif (password_expiry_utc) {\n+\t\tCFDataAppendBytes(data,\n+\t\t    (const UInt8 *)STRING_WITH_LENGTH(\"\\npassword_expiry_utc=\"));\n+\t\tCFDataAppendBytes(data,\n+\t\t\t\t  CFDataGetBytePtr(password_expiry_utc),\n+\t\t\t\t  CFDataGetLength(password_expiry_utc));\n+\t}\n+\tif (oauth_refresh_token) {\n+\t\tCFDataAppendBytes(data,\n+\t\t    (const UInt8 *)STRING_WITH_LENGTH(\"\\noauth_refresh_token=\"));\n+\t\tCFDataAppendBytes(data,\n+\t\t\t\t  CFDataGetBytePtr(oauth_refresh_token),\n+\t\t\t\t  CFDataGetLength(oauth_refresh_token));\n+\t}\n+\n+\tattrs = CREATE_SEC_ATTRIBUTES(kSecValueData, data,\n \t\t\t\t      NULL);\n \n \tresult = SecItemAdd(attrs, NULL);\n@@ -268,6 +315,7 @@ static OSStatus add_internet_password(void)\n \t\tCFRelease(query);\n \t}\n \n+\tCFRelease(data);\n \tCFRelease(attrs);\n \n \treturn result;\n@@ -339,6 +387,14 @@ static void read_credential(void)\n \t\t\tpassword = CFDataCreate(kCFAllocatorDefault,\n \t\t\t\t\t\t(UInt8 *)v,\n \t\t\t\t\t\tstrlen(v));\n+\t\telse if (!strcmp(buf, \"password_expiry_utc\"))\n+\t\t\tpassword_expiry_utc = CFDataCreate(kCFAllocatorDefault,\n+\t\t\t\t\t\t\t   (UInt8 *)v,\n+\t\t\t\t\t\t\t   strlen(v));\n+\t\telse if (!strcmp(buf, \"oauth_refresh_token\"))\n+\t\t\toauth_refresh_token = CFDataCreate(kCFAllocatorDefault,\n+\t\t\t\t\t\t\t   (UInt8 *)v,\n+\t\t\t\t\t\t\t   strlen(v));\n \t\t/*\n \t\t * Ignore other lines; we don't know what they mean, but\n \t\t * this future-proofs us when later versions of git do\n-- \ngitgitgadget\n"},{"id":"488861","messageId":"CAPig+cR_XYjArdYpU-qm+Wont=yEEXe5hANRyz+YRdhv=UZf=Q@mail.gmail.com","threadId":"60943","inReplyTo":"f7031316a043b36fac10ecf784d2294894967e7b.1708212896.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-18T06:08:48Z","receivedAt":"2024-02-18T06:09:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> The SecKeychain API was deprecated in macOS 10.10, nearly 10 years ago.\n> The replacement SecItem API however is available as far back as macOS\n> 10.6.\n>\n> While supporting older macOS was perhaps prevously a concern,\n> git-credential-osxkeychain already requires a minimum of macOS 10.7\n> since 5747c8072b (contrib/credential: avoid fixed-size buffer in\n> osxkeychain, 2023-05-01) so using the newer API should not regress the\n> range of macOS versions supported.\n>\n> Adapting to use the newer SecItem API also happens to fix two test\n> failures in osxkeychain:\n>\n>     8 - helper (osxkeychain) overwrites on store\n>     9 - helper (osxkeychain) can forget host\n>\n> The new API is compatible with credentials saved with the older API.\n>\n> Signed-off-by: Bo Anderson <mail@boanderson.me>\n\nI haven't studied the SecItem API, so I can't comment on the meat of\nthe patch, but I can make a few generic observations...\n\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -3,14 +3,39 @@\n> -__attribute__((format (printf, 1, 2)))\n> +#define ENCODING kCFStringEncodingUTF8\n> +static CFStringRef protocol; /* Stores constant strings - not memory managed */\n> +static CFStringRef host;\n> [...]\n> +\n> +static void clear_credential(void)\n> +{\n> +       if (host) {\n> +               CFRelease(host);\n> +               host = NULL;\n> +       }\n> +       [...]\n> +}\n> +\n> +__attribute__((format (printf, 1, 2), __noreturn__))\n\nThe addition of `__noreturn__` to the `__attribute__` seems unrelated\nto the stated purpose of this patch. As such, it typically would be\nplaced in its own patch. If it really is too minor for a separate\npatch, mentioning it in the commit message as a \"While at it...\" would\nbe helpful.\n\n> @@ -19,70 +44,135 @@ static void die(const char *err, ...)\n> +static CFDictionaryRef create_dictionary(CFAllocatorRef allocator, ...)\n> +{\n> +       va_list args;\n> +       const void *key;\n> +       CFMutableDictionaryRef result;\n> +\n> +       result = CFDictionaryCreateMutable(allocator,\n> +                                          0,\n> +                                          &kCFTypeDictionaryKeyCallBacks,\n> +                                          &kCFTypeDictionaryValueCallBacks);\n> +\n> +\n\nStyle: one blank line is preferred over two\n\n> +       va_start(args, allocator);\n> +       while ((key = va_arg(args, const void *)) != NULL) {\n> +               const void *value;\n> +               value = va_arg(args, const void *);\n> +               if (value)\n> +                       CFDictionarySetValue(result, key, value);\n> +       }\n> +       va_end(args);\n\nA couple related comments...\n\nIf va_arg() ever returns NULL for `value`, the next iteration of the\nloop will call va_arg() again, but calling va_arg() again after it has\nalready returned NULL is likely undefined behavior. At minimum, I\nwould have expected this to be written as:\n\n    while (...) {\n        ...\n        if (!value)\n            break;\n        CFDictionarySetValue(...);\n    }\n\nHowever, isn't it a programmer error if va_arg() returns NULL for\n`value`? If so, I'd think we'd want to scream loudly about that rather\nthan silently ignoring it. So, perhaps something like this:\n\n    while (...) {\n        ...\n        if (!value) {\n            fprintf(stderr, \"BUG: ...\");\n            abort();\n        }\n        CFDictionarySetValue(...);\n   }\n\nOr, perhaps just call the existing die() function in this file with a\nsuitable \"BUG ...\" message.\n\n> +static void find_username_in_item(CFDictionaryRef item)\n>  {\n> +       CFStringRef account_ref;\n> +       char *username_buf;\n> +       CFIndex buffer_len;\n>\n> +       account_ref = CFDictionaryGetValue(item, kSecAttrAccount);\n> +       if (!account_ref)\n> +       {\n> +               write_item(\"username\", \"\", 0);\n> +               return;\n> +       }\n\nStyle: opening brace sticks to the `if` line:\n\n    if !(account_ref) {\n        ...\n    }\n\nSame comment applies to the `if` below.\n\n> +       username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);\n> +       if (username_buf)\n> +       {\n> +               write_item(\"username\", username_buf, strlen(username_buf));\n>                 return;\n> +       }\n\nAccording to the documentation for CFStringGetCStringPtr(), the\nreturned C-string is not newly-allocated, so the caller does not have\nto free it. Therefore, can `username_buf` be declared `const char *`\nrather than `char *` to make it clear to readers that nothing is being\nleaked here? Same comment about the `(char *)` cast.\n\n> +       /* If we can't get a CString pointer then\n> +        * we need to allocate our own buffer */\n\nStyle:\n\n    /*\n     * Multi-line comments\n     * are formatted like this.\n     */\n\n> +       buffer_len = CFStringGetMaximumSizeForEncoding(\n> +                       CFStringGetLength(account_ref), ENCODING) + 1;\n> +       username_buf = xmalloc(buffer_len);\n> +       if (CFStringGetCString(account_ref,\n> +                               username_buf,\n> +                               buffer_len,\n> +                               ENCODING)) {\n> +               write_item(\"username\", username_buf, buffer_len - 1);\n> +       }\n> +       free(username_buf);\n\nOkay, this explains why `username_buf` is declared `char *` rather\nthan `const char *`. Typically, when we have a situation in which a\nvalue may or may not need freeing, we use a `to_free` variable like\nthis:\n\n    const char *username_buf;\n    char *to_free = NULL;\n    ...\n    username_buf = (const char *)CFStringGetCStringPtr(...);\n    if (username_buf) {\n        ...\n        return;\n    }\n    ...\n    username_buf = to_free = xmalloc(buffer_len);\n    if (CFStringGetCString(...))\n        ...\n    free(to_free);\n\nBut that may be overkill for this simple case, and what you have here\nmay be \"good enough\" for anyone already familiar with the API and who\nknows that the `return` after CFStringGetCStringPtr() isn't leaking.\n\n> +static OSStatus find_internet_password(void)\n>  {\n> +       CFDictionaryRef attrs;\n> +       [...]\n>\n> +       attrs = CREATE_SEC_ATTRIBUTES(kSecMatchLimit, kSecMatchLimitOne,\n> +                                     kSecReturnAttributes, kCFBooleanTrue,\n> +                                     kSecReturnData, kCFBooleanTrue,\n> +                                     NULL);\n> +       result = SecItemCopyMatching(attrs, (CFTypeRef *)&item);\n> +       if (result) {\n> +               goto out;\n> +       }\n\nWe omit braces when the body is a single statement:\n\n    if (result)\n        goto out;\n\n(Same comment applies to other code in this patch.)\n\n> +       data = CFDictionaryGetValue(item, kSecValueData);\n> +       [...]\n> +\n> +out:\n> +       CFRelease(attrs);\n\nGood, `attrs` is released in all cases.\n\n> +static OSStatus add_internet_password(void)\n>  {\n> +       [...]\n> +       attrs = CREATE_SEC_ATTRIBUTES(kSecValueData, password,\n> +                                     NULL);\n> +       result = SecItemAdd(attrs, NULL);\n> +       if (result == errSecDuplicateItem) {\n> +               CFDictionaryRef query;\n> +               query = CREATE_SEC_ATTRIBUTES(NULL);\n> +               result = SecItemUpdate(query, attrs);\n> +               CFRelease(query);\n> +       }\n> +       CFRelease(attrs);\n> +       return result;\n>  }\n\nGood, `attrs` and `query` are released in all cases.\n"},{"id":"488862","messageId":"CAPig+cRz3LoBKjfjywYfWuAy7s1sygTKnTbm_9Gg1SM3Y-srUA@mail.gmail.com","threadId":"60943","inReplyTo":"f18435b2189bb08bcdba3b28523db1d4484f66cf.1708212896.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] osxkeychain: store new attributes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-18T06:31:34Z","receivedAt":"2024-02-18T06:31:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> d208bfdfef (credential: new attribute password_expiry_utc, 2023-02-18)\n> and a5c76569e7 (credential: new attribute oauth_refresh_token,\n> 2023-04-21) introduced new credential attributes but support was missing\n> from git-credential-osxkeychain.\n> [...]\n> Signed-off-by: Bo Anderson <mail@boanderson.me>\n> ---\n> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n> @@ -6,10 +6,12 @@\n>  static CFStringRef host;\n> +static CFNumberRef port;\n>  static CFStringRef path;\n> -static CFNumberRef port;\n> @@ -17,6 +19,10 @@ static void clear_credential(void)\n> +       if (port) {\n> +               CFRelease(port);\n> +               port = NULL;\n> +       }\n> @@ -29,12 +35,18 @@ static void clear_credential(void)\n> -       if (port) {\n> -               CFRelease(port);\n> -               port = NULL;\n> +       if (password_expiry_utc) {\n> +               CFRelease(password_expiry_utc);\n> +               password_expiry_utc = NULL;\n> +       }\n\nThe relocation of `port` is unrelated to the stated purpose of this\npatch. We would normally avoid this sort of \"noise\" change since it\nobscures the \"real\" changes made by the patch, and would instead place\nit in its own patch. That said, it's such a minor issue, I doubt that\nit's worth a reroll.\n"},{"id":"488863","messageId":"CAPig+cQ_SCBNc7-x=eTz+kMg0_bg4uc2SubbJpq=i7Ok_Y719Q@mail.gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-18T06:38:06Z","receivedAt":"2024-02-18T06:38:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 17, 2024 at 6:35 PM Bo Anderson via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> git-credential-osxkeychain has largely fallen behind other external\n> credential helpers in the features it supports, and hasn't received any\n> functional changes since 2013. [...]\n>\n> osxkeychain also made use of macOS APIs that had been deprecated since 2014.\n> Replacement API was able to be used without regressing the minimum supported\n> macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer\n> in osxkeychain, 2023-05-01).\n\nAlthough I'm not familiar with the SecItem API nor with the Git\nkeychain API, I gave this series a readthrough and left a few minor\ncomments. Aside from a few very minor style nits, perhaps the only\nsubstantive comment was that patch [1/4] could do a slightly better\njob of protecting against future programmer error, but even that is\nminor in that it doesn't impact the functionality actually implemented\nby the patch, thus may not be worth a reroll.\n\nOverall, despite not being familiar with the APIs in question,\neverything I read in the patches made sense and was cleanly\nimplemented. Nicely done.\n"},{"id":"488875","messageId":"AFC4D25B-D6ED-4706-A804-CA0183B84604@boanderson.me","threadId":"60943","inReplyTo":"CAPig+cR_XYjArdYpU-qm+Wont=yEEXe5hANRyz+YRdhv=UZf=Q@mail.gmail.com","subject":"Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API","fromName":"Bo Anderson","fromEmail":"mail@boanderson.me","sentAt":"2024-02-18T14:48:41Z","receivedAt":"2024-02-18T14:48:58Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"> On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> \n> I haven't studied the SecItem API, so I can't comment on the meat of\n> the patch, but I can make a few generic observations...\n\nThanks for taking a look!\n\n>> diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n>> @@ -3,14 +3,39 @@\n>> -__attribute__((format (printf, 1, 2)))\n>> +#define ENCODING kCFStringEncodingUTF8\n>> +static CFStringRef protocol; /* Stores constant strings - not memory managed */\n>> +static CFStringRef host;\n>> [...]\n>> +\n>> +static void clear_credential(void)\n>> +{\n>> +       if (host) {\n>> +               CFRelease(host);\n>> +               host = NULL;\n>> +       }\n>> +       [...]\n>> +}\n>> +\n>> +__attribute__((format (printf, 1, 2), __noreturn__))\n> \n> The addition of `__noreturn__` to the `__attribute__` seems unrelated\n> to the stated purpose of this patch. As such, it typically would be\n> placed in its own patch. If it really is too minor for a separate\n> patch, mentioning it in the commit message as a \"While at it...\" would\n> be helpful.\n\nAcknowledged. It is indeed a bit of a nothing change that doesn’t really do much on its own, but when paired with the port variable reorder could potentially make a “minor code cleanup” commit.\n\n>> +       va_start(args, allocator);\n>> +       while ((key = va_arg(args, const void *)) != NULL) {\n>> +               const void *value;\n>> +               value = va_arg(args, const void *);\n>> +               if (value)\n>> +                       CFDictionarySetValue(result, key, value);\n>> +       }\n>> +       va_end(args);\n> \n> A couple related comments...\n> \n> If va_arg() ever returns NULL for `value`, the next iteration of the\n> loop will call va_arg() again, but calling va_arg() again after it has\n> already returned NULL is likely undefined behavior. At minimum, I\n> would have expected this to be written as:\n> \n> while (...) {\n>     ...\n>     if (!value)\n>         break;\n>     CFDictionarySetValue(...);\n> }\n> \n> However, isn't it a programmer error if va_arg() returns NULL for\n> `value`? If so, I'd think we'd want to scream loudly about that rather\n> than silently ignoring it. So, perhaps something like this:\n> \n> while (...) {\n>     ...\n>     if (!value) {\n>         fprintf(stderr, \"BUG: ...\");\n>         abort();\n>     }\n>     CFDictionarySetValue(...);\n> }\n> \n> Or, perhaps just call the existing die() function in this file with a\n> suitable \"BUG ...\" message.\n> \n\nIn this case it’s by design to accept and check for NULL values as it greatly simplifies the code. Inputs to the credential helpers have various optional fields, such as port and path. It is programmer error to pass NULL to the SecItem API (runtime crash) so in order to simplify having to check each individual field in all of the callers (and probably ditch varargs since you can’t really do dynamic varargs), I check the value here instead. That means you can do something like:\n\n create_dictionary(kCFAllocatorDefault,\n     kSecAttrServer, host,\n     kSecAttrPath, path, \\\n     kSecAttrPort, port,\n     NULL)\n\nAnd it will only include the key-value pairs that have non-NULL values.\n\nIt would indeed be programmer error to not pass key-value pairs, though it is equally programmer error to not have a terminating NULL.\n\n>> +       username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);\n>> +       if (username_buf)\n>> +       {\n>> +               write_item(\"username\", username_buf, strlen(username_buf));\n>>             return;\n>> +       }\n> \n> According to the documentation for CFStringGetCStringPtr(), the\n> returned C-string is not newly-allocated, so the caller does not have\n> to free it. Therefore, can `username_buf` be declared `const char *`\n> rather than `char *` to make it clear to readers that nothing is being\n> leaked here? Same comment about the `(char *)` cast.\n> \n>> +       /* If we can't get a CString pointer then\n>> +        * we need to allocate our own buffer */\n> \n> Style:\n> \n> /*\n>  * Multi-line comments\n>  * are formatted like this.\n>  */\n> \n>> +       buffer_len = CFStringGetMaximumSizeForEncoding(\n>> +                       CFStringGetLength(account_ref), ENCODING) + 1;\n>> +       username_buf = xmalloc(buffer_len);\n>> +       if (CFStringGetCString(account_ref,\n>> +                               username_buf,\n>> +                               buffer_len,\n>> +                               ENCODING)) {\n>> +               write_item(\"username\", username_buf, buffer_len - 1);\n>> +       }\n>> +       free(username_buf);\n> \n> Okay, this explains why `username_buf` is declared `char *` rather\n> than `const char *`. Typically, when we have a situation in which a\n> value may or may not need freeing, we use a `to_free` variable like\n> this:\n> \n> const char *username_buf;\n> char *to_free = NULL;\n> ...\n> username_buf = (const char *)CFStringGetCStringPtr(...);\n> if (username_buf) {\n>     ...\n>     return;\n> }\n> ...\n> username_buf = to_free = xmalloc(buffer_len);\n> if (CFStringGetCString(...))\n>     ...\n> free(to_free);\n> \n> But that may be overkill for this simple case, and what you have here\n> may be \"good enough\" for anyone already familiar with the API and who\n> knows that the `return` after CFStringGetCStringPtr() isn't leaking.\n\nWould it make sense to just have a comment paired with the CFStringGetCStringPtr return explaining why it doesn’t need to be freed there? I’m OK with the to_free variable however if that’s clearer. Idea in my mind was pairing it based on `xmalloc` but I can see why pairing based on variable is clearer.\n\n"},{"id":"488879","messageId":"CAPig+cTQ246qEMWe7h9E7nTZrEMSSoxiNGC1gViycXrAkC35Vw@mail.gmail.com","threadId":"60943","inReplyTo":"AFC4D25B-D6ED-4706-A804-CA0183B84604@boanderson.me","subject":"Re: [PATCH 1/4] osxkeychain: replace deprecated SecKeychain API","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-18T18:39:45Z","receivedAt":"2024-02-18T18:39:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 18, 2024 at 9:48 AM Bo Anderson <mail@boanderson.me> wrote:\n> > On 18 Feb 2024, at 06:08, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> >> +    va_start(args, allocator);\n> >> +    while ((key = va_arg(args, const void *)) != NULL) {\n> >> +        const void *value;\n> >> +        value = va_arg(args, const void *);\n> >> +        if (value)\n> >> +            CFDictionarySetValue(result, key, value);\n> >> +    }\n> >> +    va_end(args);\n> >\n> > However, isn't it a programmer error if va_arg() returns NULL for\n> > `value`? If so, I'd think we'd want to scream loudly about that rather\n> > than silently ignoring it. So, perhaps something like this: [...]\n>\n> In this case it’s by design to accept and check for NULL values as\n> it greatly simplifies the code. Inputs to the credential helpers\n> have various optional fields, such as port and path. It is\n> programmer error to pass NULL to the SecItem API (runtime crash) so\n> in order to simplify having to check each individual field in all of\n> the callers (and probably ditch varargs since you can’t really do\n> dynamic varargs), I check the value here instead. That means you can\n> do something like:\n>\n> create_dictionary(kCFAllocatorDefault,\n>   kSecAttrServer, host,\n>   kSecAttrPath, path, \\\n>   kSecAttrPort, port,\n>   NULL)\n>\n> And it will only include the key-value pairs that have non-NULL\n> values.\n>\n> It would indeed be programmer error to  not pass key-value pairs,\n> though it is equally programmer  error to  not have a terminating\n> NULL.\n\nOkay. I had thought that this check was merely protecting against\nprogrammer error, but the described use-case to avoid passing NULL to\nSecItem API makes perfect sense. It might be helpful to future readers\nto explain this either as a function-level comment (explaining how to\ncall the function) or as an in-code comment.\n\n> >> +    username_buf = (char *)CFStringGetCStringPtr(account_ref, ENCODING);\n> >> +    if (username_buf)\n> >> +    {\n> >> +        write_item(\"username\", username_buf, strlen(username_buf));\n> >>       return;\n> >> +    }\n> >\n> > According to the documentation for CFStringGetCStringPtr(), the\n> > returned C-string is not newly-allocated, so the caller does not have\n> > to free it. Therefore, can `username_buf` be declared `const char *`\n> > rather than `char *` to make it clear to readers that nothing is being\n> > leaked here? Same comment about the `(char *)` cast.\n> >\n> >> +    buffer_len = CFStringGetMaximumSizeForEncoding(\n> >> +            CFStringGetLength(account_ref), ENCODING) + 1;\n> >> +    username_buf = xmalloc(buffer_len);\n> >> +    if (CFStringGetCString(account_ref,\n> >> +                username_buf,\n> >> +                buffer_len,\n> >> +                ENCODING)) {\n> >> +        write_item(\"username\", username_buf, buffer_len - 1);\n> >> +    }\n> >> +    free(username_buf);\n> >\n> > Okay, this explains why `username_buf` is declared `char *` rather\n> > than `const char *`. Typically, when we have a situation in which a\n> > value may or may not need freeing, we use a `to_free` variable like\n> > this: [...]\n> >\n> > But that may be overkill for this simple case, and what you have here\n> > may be \"good enough\" for anyone already familiar with the API and who\n> > knows that the `return` after CFStringGetCStringPtr() isn't leaking.\n>\n> Would it make sense to just have a comment paired with the\n> CFStringGetCStringPtr return explaining why it doesn’t need to be\n> freed there? I’m OK with the to_free variable however if that’s\n> clearer. Idea in my mind was pairing it based on `xmalloc` but I can\n> see why pairing based on variable is clearer.\n\nMost likely, anyone working on this code is already familiar with the\nCoreFoundation API, thus would understand implicitly that this isn't\nleaking. But, yes, a simple comment should be plenty sufficient for\neveryone else if you are re-rolling anyhow.\n"},{"id":"488890","messageId":"20240218204044.11365-1-mirth.hickford@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2024-02-18T20:40:44Z","receivedAt":"2024-02-18T20:40:48Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"> git-credential-osxkeychain has largely fallen behind other external\n> credential helpers in the features it supports, and hasn't received any\n> functional changes since 2013. As it stood, osxkeychain failed seven tests\n> in the external credential helper test suite:\n> \n> not ok 8 - helper (osxkeychain) overwrites on store\n> not ok 9 - helper (osxkeychain) can forget host\n> not ok 11 - helper (osxkeychain) does not erase a password distinct from input\n> not ok 15 - helper (osxkeychain) erases all matching credentials\n> not ok 18 - helper (osxkeychain) gets password_expiry_utc\n> not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes\n> not ok 21 - helper (osxkeychain) gets oauth_refresh_token\n>\n> After this set of patches, osxkeychain passes all tests in the external\ncredential helper test suite.\n\nGreat work!\n\nCould these tests run as part of macOS CI?\n"},{"id":"488897","messageId":"CFC1A507-A9EF-4330-8C98-34C2B73BC036@boanderson.me","threadId":"60943","inReplyTo":"20240218204044.11365-1-mirth.hickford@gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Bo Anderson","fromEmail":"mail@boanderson.me","sentAt":"2024-02-18T23:23:58Z","receivedAt":"2024-02-18T23:24:14Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"> On 18 Feb 2024, at 20:40, M Hickford <mirth.hickford@gmail.com> wrote:\n> \n>> git-credential-osxkeychain has largely fallen behind other external\n>> credential helpers in the features it supports, and hasn't received any\n>> functional changes since 2013. As it stood, osxkeychain failed seven tests\n>> in the external credential helper test suite:\n>> \n>> not ok 8 - helper (osxkeychain) overwrites on store\n>> not ok 9 - helper (osxkeychain) can forget host\n>> not ok 11 - helper (osxkeychain) does not erase a password distinct from input\n>> not ok 15 - helper (osxkeychain) erases all matching credentials\n>> not ok 18 - helper (osxkeychain) gets password_expiry_utc\n>> not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes\n>> not ok 21 - helper (osxkeychain) gets oauth_refresh_token\n>> \n>> After this set of patches, osxkeychain passes all tests in the external\n> credential helper test suite.\n> \n> Great work!\n> \n> Could these tests run as part of macOS CI?\n\nDo we do so for any of the other external credential helpers?\n\nIt definitely makes sense in principle. Though the concern perhaps will be that any new features added to the credential helpers and thus its test suite would need adding to each credential helper simultaneously to avoid failing CI. Ideally we would do exactly that, though that requires knowledge on each of the keystore APIs used in each of the credential helpers."},{"id":"489843","messageId":"CAGJzqs=wQA=t4CMVu-kap1ga4DX+KnaVMGy71ewmZ7QkFHF8sg@mail.gmail.com","threadId":"60943","inReplyTo":"CFC1A507-A9EF-4330-8C98-34C2B73BC036@boanderson.me","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2024-03-04T08:00:00Z","receivedAt":"2024-03-04T08:00:35Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Sun, 18 Feb 2024 at 23:24, Bo Anderson <mail@boanderson.me> wrote:\n>\n> > On 18 Feb 2024, at 20:40, M Hickford <mirth.hickford@gmail.com> wrote:\n> >\n> >\n> > Could these tests run as part of macOS CI?\n>\n> Do we do so for any of the other external credential helpers?\n>\n\nWe don't.\n\n> It definitely makes sense in principle. Though the concern perhaps will be that any new features added to the credential helpers and thus its test suite would need adding to each credential helper simultaneously to avoid failing CI. Ideally we would do exactly that, though that requires knowledge on each of the keystore APIs used in each of the credential helpers.\n\nGood point.\n"},{"id":"490165","messageId":"20240307094708.GA2650063@coredump.intra.peff.net","threadId":"60943","inReplyTo":"CAGJzqs=wQA=t4CMVu-kap1ga4DX+KnaVMGy71ewmZ7QkFHF8sg@mail.gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-07T09:47:08Z","receivedAt":"2024-03-07T09:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2024 at 08:00:00AM +0000, M Hickford wrote:\n\n> > It definitely makes sense in principle. Though the concern perhaps\n> > will be that any new features added to the credential helpers and\n> > thus its test suite would need adding to each credential helper\n> > simultaneously to avoid failing CI. Ideally we would do exactly\n> > that, though that requires knowledge on each of the keystore APIs\n> > used in each of the credential helpers.\n> \n> Good point.\n\nI think we suffer from that somewhat already. You cannot run t0303\nsuccessfully against credential-store anymore, as of 0ce02e2fec\n(credential/libsecret: store new attributes, 2023-06-16).\n\nThere is some prior art in the GIT_TEST_CREDENTIAL_HELPER_TIMEOUT\nvariable, as time is not a concept to every helper (like store, for\nexample). Other new tests like the password-expiry and oauth features\ncould be gated on similar variables. That would help non-CI users\ntesting helpers manually, and then CI jobs could set the appropriate\nswitches for each helper that they cover.\n\nAll that said, I'd be surprised if testing osxkeychain in the CI\nenvironment worked. Back when I worked on it in 2011, I found that I had\nto actually run the tests in a local terminal; even a remote ssh login\ncould not access the keychain. It's possible that things have changed\nsince then, though, or perhaps I was imply ignorant of how to configure\nthings correctly.\n\n-Peff\n"},{"id":"491995","messageId":"20240401214057.2018-1-mirth.hickford@gmail.com","threadId":"60943","inReplyTo":"pull.1667.git.1708212896.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2024-04-01T21:40:57Z","receivedAt":"2024-04-01T21:41:20Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"> From: \"Bo Anderson via GitGitGadget\" <gitgitgadget@gmail.com>\n> To: git@vger.kernel.org\n> Cc: Bo Anderson <mail@boanderson.me>\n> Subject: [PATCH 0/4] osxkeychain: bring in line with other credential helpers\n> Date: Sat, 17 Feb 2024 23:34:52 +0000\t[thread overview]\n> Message-ID: <pull.1667.git.1708212896.gitgitgadget@gmail.com> (raw)\n> \n> git-credential-osxkeychain has largely fallen behind other external\n> credential helpers in the features it supports, and hasn't received any\n> functional changes since 2013. As it stood, osxkeychain failed seven tests\n> in the external credential helper test suite:\n> \n> not ok 8 - helper (osxkeychain) overwrites on store\n> not ok 9 - helper (osxkeychain) can forget host\n> not ok 11 - helper (osxkeychain) does not erase a password distinct from input\n> not ok 15 - helper (osxkeychain) erases all matching credentials\n> not ok 18 - helper (osxkeychain) gets password_expiry_utc\n> not ok 19 - helper (osxkeychain) overwrites when password_expiry_utc changes\n> not ok 21 - helper (osxkeychain) gets oauth_refresh_token\n> \n> \n> osxkeychain also made use of macOS APIs that had been deprecated since 2014.\n> Replacement API was able to be used without regressing the minimum supported\n> macOS established in 5747c8072b (contrib/credential: avoid fixed-size buffer\n> in osxkeychain, 2023-05-01).\n> \n> After this set of patches, osxkeychain passes all tests in the external\n> credential helper test suite.\n> \n> Bo Anderson (4):\n>   osxkeychain: replace deprecated SecKeychain API\n>   osxkeychain: erase all matching credentials\n>   osxkeychain: erase matching passwords only\n>   osxkeychain: store new attributes\n> \n>  contrib/credential/osxkeychain/Makefile       |   3 +-\n>  .../osxkeychain/git-credential-osxkeychain.c  | 376 ++++++++++++++----\n>  2 files changed, 310 insertions(+), 69 deletions(-)\n> \n> \n> base-commit: 3e0d3cd5c7def4808247caf168e17f2bbf47892b\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1667%2FBo98%2Fosxkeychain-update-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1667/Bo98/osxkeychain-update-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1667\n> -- \n> gitgitgadget\n\nHi. Is this patch ready to cook in seen?\n"},{"id":"492000","messageId":"xmqqv8507mpv.fsf@gitster.g","threadId":"60943","inReplyTo":"20240401214057.2018-1-mirth.hickford@gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-01T22:16:28Z","receivedAt":"2024-04-01T22:16:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"M Hickford <mirth.hickford@gmail.com> writes:\n\n>> From: \"Bo Anderson via GitGitGadget\" <gitgitgadget@gmail.com>\n>> To: git@vger.kernel.org\n>> Cc: Bo Anderson <mail@boanderson.me>\n>> Subject: [PATCH 0/4] osxkeychain: bring in line with other credential helpers\n>> Date: Sat, 17 Feb 2024 23:34:52 +0000\t[thread overview]\n>> Message-ID: <pull.1667.git.1708212896.gitgitgadget@gmail.com> (raw)\n>>  ...\n>> git-credential-osxkeychain has largely fallen behind other external\n> Hi. Is this patch ready to cook in seen?\n\nA better nudge would be for somebody who uses macOS to resend the\npatches with their \"Tested-by:\" trailers added ;-).\n\nThanks.\n"},{"id":"492054","messageId":"CAFLLRpJZg3UhBRfihtjUsXcGSod4FhDCs8fD1k-=5SLnAdHeQw@mail.gmail.com","threadId":"60943","inReplyTo":"20240307094708.GA2650063@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2024-04-02T13:21:19Z","receivedAt":"2024-04-02T13:21:37Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi all,\n\n> All that said, I'd be surprised if testing osxkeychain in the CI\n> environment worked. Back when I worked on it in 2011, I found that I had\n> to actually run the tests in a local terminal; even a remote ssh login\n> could not access the keychain. It's possible that things have changed\n> since then, though, or perhaps I was imply ignorant of how to configure\n> things correctly.\n\nI have gotten keychain working in Github Actions before: there's some\nhelpers for it, but you can also basically do it manually via the\nsteps from [1]. Basically anyone who needs to do Apple code-signing in\nCI has to make it work.\n\n@Bo, how are you actually testing this manually? Following these steps:\n\n$ make\n$ (cd contrib/credential/osxkeychain && make)\n$ ln -s contrib/credential/osxkeychain/git-credential-osxkeychain .\n$ cd t\n$ make GIT_TEST_CREDENTIAL_HELPER=osxkeychain t0303-credential-external.sh\n\nI get 'A keychain cannot be found to store \"store-user\".' in a popup\ndialog when #2 runs; then similar for other tests in 0303. For #14 I\nget a slight alternative with \"A keychain cannot be found\". There's a\n\"Reset To Defaults\" button, but that wipes everything. AFAIK I have a\nrelatively normal setup, with a login keychain as default. macOS\n14.3.1; arm64.\n\n$ security list-keychains\n    \"/Users/rc/Library/Keychains/login.keychain-db\"\n    \"/Library/Keychains/System.keychain\"\n$ security default-keychain\n    \"/Users/rc/Library/Keychains/login.keychain-db\"\n$ security unlock-keychain\npassword to unlock default: ...\n\nI don't see any settings or code for setting which keychain the\ncredential helper uses, so I guess it's the default one?\n\nCheers,\n\nRob :)\n\n[1] https://docs.github.com/en/actions/deployment/deploying-xcode-applications/installing-an-apple-certificate-on-macos-runners-for-xcode-development\n"},{"id":"492055","messageId":"98F1A6E9-4553-48BE-830C-8FDA9F3B5744@boanderson.me","threadId":"60943","inReplyTo":"CAFLLRpJZg3UhBRfihtjUsXcGSod4FhDCs8fD1k-=5SLnAdHeQw@mail.gmail.com","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Bo Anderson","fromEmail":"mail@boanderson.me","sentAt":"2024-04-02T13:53:03Z","receivedAt":"2024-04-02T13:53:28Z","isPatch":true,"sender":{"key":"mail@boanderson.me","avatar":"https://avatars.githubusercontent.com/u/1190754?v=4"},"body":"The test script does not interact well with the env filtering. This was the case before this change too.\n\nTo interact with your default keychain, you will need:\n\nGIT_TEST_CREDENTIAL_HELPER_SETUP=\"export HOME=$HOME”\n\nThis is because the default macOS user keychain is local to your home directory - that’s why it’s giving errors about not finding any.\n\nBo\n\n> On 2 Apr 2024, at 14:21, Robert Coup <robert.coup@koordinates.com> wrote:\n> \n> Hi all,\n> \n>> All that said, I'd be surprised if testing osxkeychain in the CI\n>> environment worked. Back when I worked on it in 2011, I found that I had\n>> to actually run the tests in a local terminal; even a remote ssh login\n>> could not access the keychain. It's possible that things have changed\n>> since then, though, or perhaps I was imply ignorant of how to configure\n>> things correctly.\n> \n> I have gotten keychain working in Github Actions before: there's some\n> helpers for it, but you can also basically do it manually via the\n> steps from [1]. Basically anyone who needs to do Apple code-signing in\n> CI has to make it work.\n> \n> @Bo, how are you actually testing this manually? Following these steps:\n> \n> $ make\n> $ (cd contrib/credential/osxkeychain && make)\n> $ ln -s contrib/credential/osxkeychain/git-credential-osxkeychain .\n> $ cd t\n> $ make GIT_TEST_CREDENTIAL_HELPER=osxkeychain t0303-credential-external.sh\n> \n> I get 'A keychain cannot be found to store \"store-user\".' in a popup\n> dialog when #2 runs; then similar for other tests in 0303. For #14 I\n> get a slight alternative with \"A keychain cannot be found\". There's a\n> \"Reset To Defaults\" button, but that wipes everything. AFAIK I have a\n> relatively normal setup, with a login keychain as default. macOS\n> 14.3.1; arm64.\n> \n> $ security list-keychains\n>    \"/Users/rc/Library/Keychains/login.keychain-db\"\n>    \"/Library/Keychains/System.keychain\"\n> $ security default-keychain\n>    \"/Users/rc/Library/Keychains/login.keychain-db\"\n> $ security unlock-keychain\n> password to unlock default: ...\n> \n> I don't see any settings or code for setting which keychain the\n> credential helper uses, so I guess it's the default one?\n> \n> Cheers,\n> \n> Rob :)\n> \n> [1] https://docs.github.com/en/actions/deployment/deploying-xcode-applications/installing-an-apple-certificate-on-macos-runners-for-xcode-development\n\n"},{"id":"492059","messageId":"CAFLLRp+Dd5M5Y+uoSpPk8-xpFHc_kBJJQfiHr4rt34ey0HAXbg@mail.gmail.com","threadId":"60943","inReplyTo":"98F1A6E9-4553-48BE-830C-8FDA9F3B5744@boanderson.me","subject":"Re: [PATCH 0/4] osxkeychain: bring in line with other credential helpers","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2024-04-02T14:54:31Z","receivedAt":"2024-04-02T14:54:49Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi Bo,\n\nOn Tue, 2 Apr 2024 at 14:53, Bo Anderson <mail@boanderson.me> wrote:\n>\n> The test script does not interact well with the env filtering. This was the case before this change too.\n\nI guess without writing a helper-specific test or having some\nper-helper-setup thing it's a bit tricky.\n\n> To interact with your default keychain, you will need:\n>\n> GIT_TEST_CREDENTIAL_HELPER_SETUP=\"export HOME=$HOME”\n>\n> This is because the default macOS user keychain is local to your home directory - that’s why it’s giving errors about not finding any.\n\nAnd with that, the tests all pass :-) Comparing with master where 7/21 failed.\n\nTested-by: Robert Coup <robert.coup@koordinates.com>\n\nCould we document that setup step somewhere? I guess the simplest is\nprobably just to put it in the header of\nt/t0303-credential-external.sh; maybe along the lines of the patch\nbelow.\n\nRob :)\n\n\ndiff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh\nindex 095574bfc6..e4e693b233 100755\n--- a/t/t0303-credential-external.sh\n+++ b/t/t0303-credential-external.sh\n@@ -27,6 +27,13 @@ timeout, you can test that feature with:\n If your helper requires additional setup before the tests are started,\n you can set GIT_TEST_CREDENTIAL_HELPER_SETUP to a sequence of shell\n commands.\n+\n+- osxkeychain:\n+\n+  Because the default macOS user keychain is local to your home\n+  directory, you will need:\n+\n+    GIT_TEST_CREDENTIAL_HELPER_SETUP=\"export HOME=$HOME”\n '\n\n . ./test-lib.sh\n"}]}