credential/osxkeychain: store new attributes

Commit Message

M Hickford Feb. 13, 2024, 9:43 p.m. UTC

From: M Hickford <[e-mail]>

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

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

    hunter2
    password_expiry_utc=1684189401
    oauth_refresh_token=xyzzy

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

Signed-off-by: M Hickford <[e-mail]>
---
    [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(-)


base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c

Comments

Junio C Hamano Feb. 14, 2024, 6:25 p.m. UTC | #1

"M Hickford via GitGitGadget" <[e-mail]> writes:

> From: M Hickford <[e-mail]>
>
> d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
> and a5c76569e7 (credential: new attribute oauth_refresh_token)
> introduced new credential attributes.
>
> Similar to 7144dee3 (credential/libsecret: erase matching creds only,
> 2023-07-26), we encode the new attributes in the secret, separated by
> newline:
>
>     hunter2
>     password_expiry_utc=1684189401
>     oauth_refresh_token=xyzzy
>
> This is extensible and backwards compatible. The credential protocol
> already assumes that attribute values do not contain newlines.
>
> Signed-off-by: M Hickford <[e-mail]>
> ---

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).

>     [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

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

Thanks.  Will queue.

M Hickford Feb. 14, 2024, 10:35 p.m. UTC | #2

On Wed, 14 Feb 2024 at 18:25, Junio C Hamano <[e-mail]> wrote:
>
> "M Hickford via GitGitGadget" <[e-mail]> writes:
>
> > From: M Hickford <[e-mail]>
> >
> > d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
> > and a5c76569e7 (credential: new attribute oauth_refresh_token)
> > introduced new credential attributes.
> >
> > Similar to 7144dee3 (credential/libsecret: erase matching creds only,
> > 2023-07-26), we encode the new attributes in the secret, separated by
> > newline:
> >
> >     hunter2
> >     password_expiry_utc=1684189401
> >     oauth_refresh_token=xyzzy
> >
> > This is extensible and backwards compatible. The credential protocol
> > already assumes that attribute values do not contain newlines.
> >
> > Signed-off-by: M Hickford <[e-mail]>
> > ---
>
> 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).
>
> >     [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
>
> I do not use macOS to use this on, so let's see how others can help.
>
> Thanks.  Will queue.

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

Jeff King Feb. 15, 2024, 4:59 a.m. UTC | #3

On Tue, Feb 13, 2024 at 09:43:38PM +0000, M Hickford via GitGitGadget wrote:

>     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).

>  static void add_internet_password(void)
>  {
> +	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).

>  	/* 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).

> +	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

M Hickford Feb. 27, 2024, 8 p.m. UTC | #4

On Wed, 14 Feb 2024 at 22:35, M Hickford <[e-mail]> wrote:
>
> > 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/[e-mail]/

Junio C Hamano Feb. 27, 2024, 8:13 p.m. UTC | #5

M Hickford <[e-mail]> writes:

> On Wed, 14 Feb 2024 at 22:35, M Hickford <[e-mail]> wrote:
>>
>> > 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/[e-mail]/

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.

Patch

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index 5f2e5f16c88..25ffa84f4ba 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -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