credential/osxkeychain: store new attributes
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=xyzzyThis 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
@@ -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
Quoted 14 lines · M Hickford via GitGitGadget
From: M Hickford <[email protected]>
d208bfd (credential: new attribute
password_expiry_utc, 2023-02-18) anda5c76569e7(credential: new attributeoauth_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.
Signed-off-by M Hickford <[email protected]>---
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.
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) anda5c76569e7(credential: new attributeoauth_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=xyzzyQuoted 5 lines
This is extensible and backwards compatible. The credential protocol already assumes that attribute values do not contain newlines.
Signed-off-by M Hickford <[email protected]>---
OK, this adds both
oauth_refresh_tokenandpassword_expiry_utc, unlike the recent one for wincred, which already stored the expiry but the support foroauth_refresh_tokenwas added withf061959e(credential/wincred: storeoauth_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/42I 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
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]/
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.
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