{"thread":{"id":"60909","subject":"[PATCH] credential/osxkeychain: store new attributes","startedAt":"2024-02-13T21:43:41Z","lastAt":"2024-02-27T20:13:50Z","messageCount":6,"participants":["M Hickford via GitGitGadget","Junio C Hamano","M Hickford","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488600","messageId":"pull.1663.git.1707860618119.gitgitgadget@gmail.com","threadId":"60909","inReplyTo":null,"subject":"[PATCH] credential/osxkeychain: store new attributes","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-02-13T21:43:38Z","receivedAt":"2024-02-13T21:43:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nd208bfd (credential: new attribute password_expiry_utc, 2023-02-18)\nand a5c76569e7 (credential: new attribute oauth_refresh_token)\nintroduced new credential attributes.\n\nSimilar to 7144dee3 (credential/libsecret: erase matching creds only,\n2023-07-26), we encode the new attributes in the secret, separated by\nnewline:\n\n hunter2\n password_expiry_utc=1684189401\n oauth_refresh_token=xyzzy\n\nThis is extensible and backwards compatible. The credential protocol\nalready assumes that attribute values do not contain newlines.\n\nSigned-off-by: M Hickford \n---\n [RFC] contrib/credential/osxkeychain: store new attributes\n \n Is any keen MacOS user interested in building and testing this RFC\n patch? I personally don't have a MacOS machine, so haven't tried\n building it. Fixes are surely necessary. Once it builds, you can test\n the feature with:\n \n GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh\n \n \n The feature would help git-credential-oauth users on MacOS\n https://github.com/hickford/git-credential-oauth/issues/42\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1663%2Fhickford%2Fosxkeychain-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1663/hickford/osxkeychain-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1663\n\n .../osxkeychain/git-credential-osxkeychain.c | 56 ++++++++++++++++++-\n 1 file changed, 54 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex 5f2e5f16c88..25ffa84f4ba 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -8,6 +8,8 @@ static char *host;\n static char *path;\n static char *username;\n static char *password;\n+static char *password_expiry_utc;\n+static char *oauth_refresh_token;\n static UInt16 port;\n \n __attribute__((format (printf, 1, 2)))\n@@ -22,6 +24,17 @@ static void die(const char *err, ...)\n \texit(1);\n }\n \n+\n+static void *xmalloc(size_t size)\n+{\n+\tvoid *ret = malloc(size);\n+\tif (!ret && !size)\n+\t\tret = malloc(1);\n+\tif (!ret)\n+\t\t die(\"Out of memory\");\n+\treturn ret;\n+}\n+\n static void *xstrdup(const char *s1)\n {\n \tvoid *ret = strdup(s1);\n@@ -69,11 +82,27 @@ static void find_internet_password(void)\n \tvoid *buf;\n \tUInt32 len;\n \tSecKeychainItemRef item;\n+\tchar *line;\n+\tchar *remaining_lines;\n+\tchar *part;\n+\tchar *remaining_parts;\n \n \tif (SecKeychainFindInternetPassword(KEYCHAIN_ARGS, &len, &buf, &item))\n \t\treturn;\n \n-\twrite_item(\"password\", buf, len);\n+\tline = strtok_r(buf, \"\\n\", &remaining_lines);\n+\twrite_item(\"password\", line, strlen(line));\n+\twhile(line != NULL) {\n+\t\tpart = strtok_r(line, \"=\", &remaining_parts);\n+\t\tif (!strcmp(part, \"oauth_refresh_token\")) {\n+\t\t\twrite_item(\"oauth_refresh_token\", remaining_parts, strlen(remaining_parts));\n+\t\t}\n+\t\tif (!strcmp(part, \"password_expiry_utc\")) {\n+\t\t\twrite_item(\"password_expiry_utc\", remaining_parts, strlen(remaining_parts));\n+\t\t}\n+\t\tline = strtok_r(NULL, \"\\n\", &remaining_lines);\n+\t}\n+\n \tif (!username)\n \t\tfind_username_in_item(item);\n \n@@ -100,13 +129,32 @@ static void delete_internet_password(void)\n \n static void add_internet_password(void)\n {\n+\tint len;\n+\n \t/* Only store complete credentials */\n \tif (!protocol || !host || !username || !password)\n \t\treturn;\n \n+\tchar *secret;\n+\tif (password_expiry_utc && oauth_refresh_token) {\n+\t\tlen = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen(\"\\npassword_expiry_utc=\\noauth_refresh_token=\");\n+\t\tsecret = xmalloc(len);\n+\t\tsnprintf(secret, len, len, \"%s\\npassword_expiry_utc=%s\\noauth_refresh_token=%s\", password, oauth_refresh_token);\n+\t} else if (oauth_refresh_token) {\n+\t\tlen = strlen(password) + strlen(oauth_refresh_token) + strlen(\"\\noauth_refresh_token=\");\n+\t\tsecret = xmalloc(len);\n+\t\tsnprintf(secret, len, len, \"%s\\noauth_refresh_token=%s\", password, oauth_refresh_token);\n+\t} else if (password_expiry_utc) {\n+\t\tlen = strlen(password) + strlen(password_expiry_utc) + strlen(\"\\npassword_expiry_utc=\");\n+\t\tsecret = xmalloc(len);\n+\t\tsnprintf(secret, len, len, \"%s\\npassword_expiry_utc=%s\", password, password_expiry_utc);\n+\t} else {\n+\t\tsecret = xstrdup(password);\n+\t}\n+\n \tif (SecKeychainAddInternetPassword(\n \t KEYCHAIN_ARGS,\n-\t KEYCHAIN_ITEM(password),\n+\t KEYCHAIN_ITEM(secret),\n \t NULL))\n \t\treturn;\n }\n@@ -161,6 +209,10 @@ static void read_credential(void)\n \t\t\tusername = xstrdup(v);\n \t\telse if (!strcmp(buf, \"password\"))\n \t\t\tpassword = xstrdup(v);\n+\t\telse if (!strcmp(buf, \"password_expiry_utc\"))\n+\t\t\tpassword_expiry_utc = xstrdup(v);\n+\t\telse if (!strcmp(buf, \"oauth_refresh_token\"))\n+\t\t\toauth_refresh_token = xstrdup(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\nbase-commit: c875e0b8e036c12cfbf6531962108a063c7a821c\n-- \ngitgitgadget\n"},{"id":"488662","messageId":"xmqqzfw2vr7c.fsf@gitster.g","threadId":"60909","inReplyTo":"pull.1663.git.1707860618119.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential/osxkeychain: store new attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-14T18:25:11Z","receivedAt":"2024-02-14T18:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> From: M Hickford \n>\n> d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)\n> and a5c76569e7 (credential: new attribute oauth_refresh_token)\n> introduced new credential attributes.\n>\n> Similar to 7144dee3 (credential/libsecret: erase matching creds only,\n> 2023-07-26), we encode the new attributes in the secret, separated by\n> newline:\n>\n> hunter2\n> password_expiry_utc=1684189401\n> oauth_refresh_token=xyzzy\n>\n> This is extensible and backwards compatible. The credential protocol\n> already assumes that attribute values do not contain newlines.\n>\n> Signed-off-by: M Hickford \n> ---\n\nOK, this adds both oauth_refresh_token and password_expiry_utc,\nunlike the recent one for wincred, which already stored the expiry\nbut the support for oauth_refresh_token was added with f061959e\n(credential/wincred: store oauth_refresh_token, 2024-01-28).\n\n> [RFC] contrib/credential/osxkeychain: store new attributes\n> \n> Is any keen MacOS user interested in building and testing this RFC\n> patch? I personally don't have a MacOS machine, so haven't tried\n> building it. Fixes are surely necessary. Once it builds, you can test\n> the feature with:\n> \n> GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh\n> \n> \n> The feature would help git-credential-oauth users on MacOS\n> https://github.com/hickford/git-credential-oauth/issues/42\n\nI do not use macOS to use this on, so let's see how others can help.\n\nThanks. Will queue.\n\n"},{"id":"488670","messageId":"CAGJzqsmSzMqEG1OU9dH6CORV6=L7qUAFNJSmi41Lqrajf9mSew@mail.gmail.com","threadId":"60909","inReplyTo":"xmqqzfw2vr7c.fsf@gitster.g","subject":"Re: [PATCH] credential/osxkeychain: store new attributes","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2024-02-14T22:35:01Z","receivedAt":"2024-02-14T22:35:40Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 14 Feb 2024 at 18:25, Junio C Hamano wrote:\n>\n> \"M Hickford via GitGitGadget\" writes:\n>\n> > From: M Hickford \n> >\n> > d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)\n> > and a5c76569e7 (credential: new attribute oauth_refresh_token)\n> > introduced new credential attributes.\n> >\n> > Similar to 7144dee3 (credential/libsecret: erase matching creds only,\n> > 2023-07-26), we encode the new attributes in the secret, separated by\n> > newline:\n> >\n> > hunter2\n> > password_expiry_utc=1684189401\n> > oauth_refresh_token=xyzzy\n> >\n> > This is extensible and backwards compatible. The credential protocol\n> > already assumes that attribute values do not contain newlines.\n> >\n> > Signed-off-by: M Hickford \n> > ---\n>\n> OK, this adds both oauth_refresh_token and password_expiry_utc,\n> unlike the recent one for wincred, which already stored the expiry\n> but the support for oauth_refresh_token was added with f061959e\n> (credential/wincred: store oauth_refresh_token, 2024-01-28).\n>\n> > [RFC] contrib/credential/osxkeychain: store new attributes\n> >\n> > Is any keen MacOS user interested in building and testing this RFC\n> > patch? I personally don't have a MacOS machine, so haven't tried\n> > building it. Fixes are surely necessary. Once it builds, you can test\n> > the feature with:\n> >\n> > GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh\n> >\n> >\n> > The feature would help git-credential-oauth users on MacOS\n> > https://github.com/hickford/git-credential-oauth/issues/42\n>\n> I do not use macOS to use this on, so let's see how others can help.\n>\n> Thanks. Will queue.\n\nA first-time contributor contacted me to say they are working on a\nmore comprehensive patch to credential-osxkeychain, so let's wait for\nthat instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116\n"},{"id":"488680","messageId":"20240215045938.GB2821179@coredump.intra.peff.net","threadId":"60909","inReplyTo":"pull.1663.git.1707860618119.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential/osxkeychain: store new attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-15T04:59:38Z","receivedAt":"2024-02-15T04:59:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 13, 2024 at 09:43:38PM +0000, M Hickford via GitGitGadget wrote:\n\n> Is any keen MacOS user interested in building and testing this RFC\n> patch? I personally don't have a MacOS machine, so haven't tried\n> building it. Fixes are surely necessary. Once it builds, you can test\n> the feature with:\n> \n> GIT_TEST_CREDENTIAL_HELPER=osxkeychain ./t0303-credential-external.sh\n\nYou might also need:\n\n GIT_TEST_CREDENTIAL_HELPER_SETUP=\"export HOME=$HOME\"\n\naccording to 34961d30da (contrib: add credential helper for OS X\nKeychain, 2011-12-10). IIRC I also ran into problems trying to test over\nssh, as those sessions did not have access to the keychain.\n\n(Sorry, I haven't touched a mac since adding the helper back then, but\nmaybe those hints will help somebody else).\n\n> static void add_internet_password(void)\n> {\n> +\tint len;\n> +\n\nThis should probably be a size_t to avoid integer overflow for malicious\ninputs. I suspect it's hard to get a super-long string into the system.\nWe do use the dynamic getline(), but stuff like host, user, etc, almost\ncertainly comes from the user or from a URL that was passed over a\ncommand-line. Maybe oauth_refresh_token() could be long, though?\n\nAnyway, probably better safe than sorry (though see below).\n\n> \t/* Only store complete credentials */\n> \tif (!protocol || !host || !username || !password)\n> \t\treturn;\n> \n> +\tchar *secret;\n\nThis is a decl-after-statement, which our style forbids (though I am\nhappy to defer on style issues to anybody who volunteers to maintain\na slice of contrib/, and I don't think we need to worry about pre-c99\ncompilers here).\n\n> +\tif (password_expiry_utc && oauth_refresh_token) {\n> +\t\tlen = strlen(password) + strlen(password_expiry_utc) + strlen(oauth_refresh_token) + strlen(\"\\npassword_expiry_utc=\\noauth_refresh_token=\");\n> +\t\tsecret = xmalloc(len);\n> +\t\tsnprintf(secret, len, len, \"%s\\npassword_expiry_utc=%s\\noauth_refresh_token=%s\", password, oauth_refresh_token);\n\nDo you need to add one more byte to \"len\" for the NUL terminator?\n\nI think there is also a mismatch in your snprintf call, which has three\n%s placeholders and only two var-args.\n\nSince we added xmalloc() as a helper, I wonder if we could go just a\nlittle further with (totally untested):\n\n __attribute__((format (printf, 1, 2)))\n char *xstrfmt(const char *fmt, ...)\n {\n va_list ap, cp;\n\t char *ret;\n\t int len;\n\n\t va_start(ap, fmt);\n\n\t va_copy(cp, ap);\n\t len = vsnprintf(NULL, 0, fmt, cp);\n\t va_end(cp);\n\n\t /*\n\t * sadly we must use int for the length, since that's what the\n\t * standard specifies. But good implementations will return a\n\t * negative value if the resulting length would overflow.\n\t */\n\t if (len < 0)\n\t die(\"xstrfmt string too long\");\n\n\t ret = xmalloc(len + 1);\n\t vsnprintf(ret, len, fmt, ap);\n\t va_end(ap);\n\n\t return ret;\n }\n\nThen you can just write:\n\n secret = xstrfmt(\"%s\\npassword_expiry_utc=%s\\noauth_refresh_token=%s\",\n password, password_expiry_utc, oauth_refresh_token);\n\nEven across the three instances, I doubt it is saving any lines, but it\nis much easier to verify that we sized the buffer correctly and did not\nintroduce an overflow.\n\n-Peff\n"},{"id":"489570","messageId":"CAGJzqsknN_RmYeT0xcn4cTLcJhsxSOUC6ppRVepxMDf3day5Fw@mail.gmail.com","threadId":"60909","inReplyTo":"CAGJzqsmSzMqEG1OU9dH6CORV6=L7qUAFNJSmi41Lqrajf9mSew@mail.gmail.com","subject":"Re: [PATCH] credential/osxkeychain: store new attributes","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2024-02-27T20:00:00Z","receivedAt":"2024-02-27T20:01:06Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 14 Feb 2024 at 22:35, M Hickford wrote:\n>\n> > Thanks. Will queue.\n>\n> A first-time contributor contacted me to say they are working on a\n> more comprehensive patch to credential-osxkeychain, so let's wait for\n> that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116\n\nPlease disregard my patch and look at Bo Anderson's instead\nhttps://lore.kernel.org/git/pull.1667.git.1708212896.gitgitgadget@gmail.com/\n"},{"id":"489572","messageId":"xmqqsf1dr7gk.fsf@gitster.g","threadId":"60909","inReplyTo":"CAGJzqsknN_RmYeT0xcn4cTLcJhsxSOUC6ppRVepxMDf3day5Fw@mail.gmail.com","subject":"Re: [PATCH] credential/osxkeychain: store new attributes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T20:13:47Z","receivedAt":"2024-02-27T20:13:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"M Hickford writes:\n\n> On Wed, 14 Feb 2024 at 22:35, M Hickford wrote:\n>>\n>> > Thanks. Will queue.\n>>\n>> A first-time contributor contacted me to say they are working on a\n>> more comprehensive patch to credential-osxkeychain, so let's wait for\n>> that instead. https://github.com/gitgitgadget/git/pull/1663#issuecomment-1942763116\n>\n> Please disregard my patch and look at Bo Anderson's instead\n> https://lore.kernel.org/git/pull.1667.git.1708212896.gitgitgadget@gmail.com/\n\nWill drop mh/credential-oauth-refresh-token-with-osxkeychain topic.\nThe other one seemed to have got some reviews and I think the\ncurrent status of the series is that it is Bo's turn to respond with\na new iteration of the series.\n\nThanks.\n"}]}