[PATCH v4] credential/libsecret: store new attributes

From: M Hickford via GitGitGadget <gitgitgadget@gmail.com>
Date: May 17, 2023, 06:55 UTC
Message-ID: <pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com>
In-Reply-To: <pull.1469.v3.git.git.1683270298313.gitgitgadget@gmail.com>

From: M Hickford <mirth.hickford@gmail.com>

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

libsecret assumes attribute values are non-confidential and unchanging, so 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.

Alternatives considered: store password_expiry_utc in a libsecret attribute. This has the problem that libsecret creates new items rather than overwrites when attribute values change.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential/libsecret: store password_expiry_utc and oauth_refresh_token
    
    Patch v4 stores password_expiry_utc inside secret instead of in an
    attribute.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v4
Pull-Request: https://github.com/git/git/pull/1469

Range-diff vs v3:

 1:  b46594c8897 ! 1:  07452a10372 credential/libsecret: support password_expiry_utc
     @@ Metadata
      Author: M Hickford <mirth.hickford@gmail.com>
      
       ## Commit message ##
     -    credential/libsecret: support password_expiry_utc
     +    credential/libsecret: store new attributes
      
          d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
     -    introduced this attribute.
     +    and a5c76569e7 (credential: new attribute oauth_refresh_token)
     +    introduced new credential attributes.
     +
     +    libsecret assumes attribute values are non-confidential and
     +    unchanging, so 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.
     +
     +    Alternatives considered: store password_expiry_utc in a libsecret
     +    attribute. This has the problem that libsecret creates new items
     +    rather than overwrites when attribute values change.
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
     @@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential {
       	char *username;
       	char *password;
      +	char *password_expiry_utc;
     ++	char *oauth_refresh_token;
       };
       
       #define CREDENTIAL_INIT { 0 }
     @@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
       
      +static const SecretSchema schema = {
      +	"org.git.Password",
     -+	/* Ignore schema name for backwards compatibility with previous versions */
     ++	/* Ignore schema name during search for backwards compatibility */
      +	SECRET_SCHEMA_DONT_MATCH_NAME,
      +	{
     ++		/*
     ++		 * libsecret assumes attribute values are non-confidential and
     ++		 * unchanging, so we can't include oauth_refresh_token or
     ++		 * password_expiry_utc.
     ++		 */
      +		{  "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
      +		{  "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
      +		{  "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
      +		{  "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
      +		{  "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
     -+		{  "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
      +		{  NULL, 0 },
      +	}
      +};
     @@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
       static char *make_label(struct credential *c)
       {
       	if (c->port)
     -@@ contrib/credential/libsecret/git-credential-libsecret.c: static GHashTable *make_attr_list(struct credential *c)
     - 		g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
     - 	if (c->path)
     - 		g_hash_table_insert(al, "object", g_strdup(c->path));
     -+	if (c->password_expiry_utc)
     -+		g_hash_table_insert(al, "password_expiry_utc",
     -+			g_strdup(c->password_expiry_utc));
     - 
     - 	return al;
     - }
      @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
       
       	attributes = make_attr_list(c);
     @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
       					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
       					   NULL,
      @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
     - 			c->username = g_strdup(s);
     - 		}
     + 		SecretItem *item;
     + 		SecretValue *secret;
     + 		const char *s;
     ++		gchar **parts;
     + 
     + 		item = items->data;
     + 		secret = secret_item_get_secret(item);
     +@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
       
     -+		s = g_hash_table_lookup(attributes, "password_expiry_utc");
     -+		if (s) {
     -+			g_free(c->password_expiry_utc);
     -+			c->password_expiry_utc = g_strdup(s);
     -+		}
     -+
       		s = secret_value_get_text(secret);
       		if (s) {
     - 			g_free(c->password);
     +-			g_free(c->password);
     +-			c->password = g_strdup(s);
     ++			/*
     ++			 * Passwords and other attributes encoded in following format:
     ++			 *   hunter2
     ++			 *   password_expiry_utc=1684189401
     ++			 *   oauth_refresh_token=xyzzy
     ++			 */
     ++			parts = g_strsplit(s, "\n", 0);
     ++			if (g_strv_length(parts) >= 1) {
     ++				g_free(c->password);
     ++				c->password = g_strdup(parts[0]);
     ++			}
     ++			for (int i = 1; i < g_strv_length(parts); i++) {
     ++				if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
     ++					g_free(c->password_expiry_utc);
     ++					c->password_expiry_utc = g_strdup(&parts[i][20]);
     ++				} else if (g_str_has_prefix(parts[i], "oauth_refresh_token=")) {
     ++					g_free(c->oauth_refresh_token);
     ++					c->oauth_refresh_token = g_strdup(&parts[i][20]);
     ++				}
     ++			}
     ++			g_strfreev(parts);
     + 		}
     + 
     + 		g_hash_table_unref(attributes);
     +@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_store(struct credential *c)
     + 	char *label = NULL;
     + 	GHashTable *attributes = NULL;
     + 	GError *error = NULL;
     ++	GString *secret = NULL;
     + 
     + 	/*
     + 	 * Sanity check that what we are storing is actually sensible.
      @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_store(struct credential *c)
       
       	label = make_label(c);
       	attributes = make_attr_list(c);
      -	secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
     ++	secret = g_string_new(c->password);
     ++	if (c->password_expiry_utc) {
     ++		g_string_append_printf(secret, "\npassword_expiry_utc=%s",
     ++			c->password_expiry_utc);
     ++	}
     ++	if (c->oauth_refresh_token) {
     ++		g_string_append_printf(secret, "\noauth_refresh_token=%s",
     ++			c->oauth_refresh_token);
     ++	}
      +	secret_password_storev_sync(&schema,
       				    attributes,
       				    NULL,
       				    label,
     +-				    c->password,
     ++				    secret->str,
     + 				    NULL,
     + 				    &error);
     ++	g_string_free(secret, TRUE);
     + 	g_free(label);
     + 	g_hash_table_unref(attributes);
     + 
      @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_erase(struct credential *c)
       		return EXIT_FAILURE;
       
     @@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_
       	g_free(c->username);
       	g_free(c->password);
      +	g_free(c->password_expiry_utc);
     ++	g_free(c->oauth_refresh_token);
       
       	credential_init(c);
       }
     @@ contrib/credential/libsecret/git-credential-libsecret.c: static int credential_r
       		} else if (!strcmp(key, "password")) {
       			g_free(c->password);
       			c->password = g_strdup(value);
     -@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_write_item(FILE *fp, const char *key, const char *value)
     - 
     - static void credential_write(const struct credential *c)
     - {
     --	/* only write username/password, if set */
     -+	/* only write username/password/expiry, if set */
     + 			while (*value)
     + 				*value++ = '\0';
     ++		} else if (!strcmp(key, "oauth_refresh_token")) {
     ++			g_free(c->oauth_refresh_token);
     ++			c->oauth_refresh_token = g_strdup(value);
     ++			while (*value)
     ++				*value++ = '\0';
     + 		}
     + 		/*
     + 		 * Ignore other lines; we don't know what they mean, but
     +@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_write(const struct credential *c)
     + 	/* only write username/password, if set */
       	credential_write_item(stdout, "username", c->username);
       	credential_write_item(stdout, "password", c->password);
      +	credential_write_item(stdout, "password_expiry_utc",
      +		c->password_expiry_utc);
     ++	credential_write_item(stdout, "oauth_refresh_token",
     ++		c->oauth_refresh_token);
       }
       
       static void usage(const char *name)
     @@ t/lib-credential.sh: helper_test_clean() {
       	reject $1 https example.com user1
       	reject $1 https example.com user2
      +	reject $1 https example.com user3
     + 	reject $1 https example.com user4
       	reject $1 http path.tld user
       	reject $1 https timeout.tld user
     - 	reject $1 https sso.tld
      @@ t/lib-credential.sh: helper_test_timeout() {
       	'
       }
     @@ t/lib-credential.sh: helper_test_timeout() {
      +	'
      +}
      +
     - write_script askpass <<\EOF
     - echo >&2 askpass: $*
     - what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
     + helper_test_oauth_refresh_token() {
     + 	HELPER=$1
     + 
      
       ## t/t0301-credential-cache.sh ##
      @@ t/t0301-credential-cache.sh: test_atexit 'git credential-cache exit'
     + 
       # test that the daemon works with no special setup
       helper_test cache
     - 
      +helper_test_password_expiry_utc cache
     -+
     + helper_test_oauth_refresh_token cache
     + 
       test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
     - 	test_when_finished "
     - 		git credential-cache exit &&
      
       ## t/t0303-credential-external.sh ##
     -@@ t/t0303-credential-external.sh: else
     - 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
     - fi
     +@@ t/t0303-credential-external.sh: test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
     + helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
       
     + helper_test "$GIT_TEST_CREDENTIAL_HELPER"
      +helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
     -+
     - # clean afterwards so that we are good citizens
     - # and don't leave cruft in the helper's storage, which
     - # might be long-term system storage
     ++helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
     + 
     + if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
     + 	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"

 .../libsecret/git-credential-libsecret.c      | 78 +++++++++++++++++--
 t/lib-credential.sh                           | 30 +++++++
 t/t0301-credential-cache.sh                   |  1 +
 t/t0303-credential-external.sh                |  2 +
 4 files changed, 105 insertions(+), 6 deletions(-)

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index ef681f29d5b..31cf32ad969 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,8 @@ struct credential {
 	char *path;
 	char *username;
 	char *password;
+	char *password_expiry_utc;
+	char *oauth_refresh_token;
 };
 
 #define CREDENTIAL_INIT { 0 }
@@ -54,6 +56,25 @@ struct credential_operation {
 
 /* ----------------- Secret Service functions ----------------- */
 
+static const SecretSchema schema = {
+	"org.git.Password",
+	/* Ignore schema name during search for backwards compatibility */
+	SECRET_SCHEMA_DONT_MATCH_NAME,
+	{
+		/*
+		 * libsecret assumes attribute values are non-confidential and
+		 * unchanging, so we can't include oauth_refresh_token or
+		 * password_expiry_utc.
+		 */
+		{  "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+		{  "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+		{  "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+		{  "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+		{  "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+		{  NULL, 0 },
+	}
+};
+
 static char *make_label(struct credential *c)
 {
 	if (c->port)
@@ -101,7 +122,7 @@ static int keyring_get(struct credential *c)
 
 	attributes = make_attr_list(c);
 	items = secret_service_search_sync(service,
-					   SECRET_SCHEMA_COMPAT_NETWORK,
+					   &schema,
 					   attributes,
 					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
 					   NULL,
@@ -117,6 +138,7 @@ static int keyring_get(struct credential *c)
 		SecretItem *item;
 		SecretValue *secret;
 		const char *s;
+		gchar **parts;
 
 		item = items->data;
 		secret = secret_item_get_secret(item);
@@ -130,8 +152,27 @@ static int keyring_get(struct credential *c)
 
 		s = secret_value_get_text(secret);
 		if (s) {
-			g_free(c->password);
-			c->password = g_strdup(s);
+			/*
+			 * Passwords and other attributes encoded in following format:
+			 *   hunter2
+			 *   password_expiry_utc=1684189401
+			 *   oauth_refresh_token=xyzzy
+			 */
+			parts = g_strsplit(s, "\n", 0);
+			if (g_strv_length(parts) >= 1) {
+				g_free(c->password);
+				c->password = g_strdup(parts[0]);
+			}
+			for (int i = 1; i < g_strv_length(parts); i++) {
+				if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
+					g_free(c->password_expiry_utc);
+					c->password_expiry_utc = g_strdup(&parts[i][20]);
+				} else if (g_str_has_prefix(parts[i], "oauth_refresh_token=")) {
+					g_free(c->oauth_refresh_token);
+					c->oauth_refresh_token = g_strdup(&parts[i][20]);
+				}
+			}
+			g_strfreev(parts);
 		}
 
 		g_hash_table_unref(attributes);
@@ -148,6 +189,7 @@ static int keyring_store(struct credential *c)
 	char *label = NULL;
 	GHashTable *attributes = NULL;
 	GError *error = NULL;
+	GString *secret = NULL;
 
 	/*
 	 * Sanity check that what we are storing is actually sensible.
@@ -162,13 +204,23 @@ static int keyring_store(struct credential *c)
 
 	label = make_label(c);
 	attributes = make_attr_list(c);
-	secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+	secret = g_string_new(c->password);
+	if (c->password_expiry_utc) {
+		g_string_append_printf(secret, "\npassword_expiry_utc=%s",
+			c->password_expiry_utc);
+	}
+	if (c->oauth_refresh_token) {
+		g_string_append_printf(secret, "\noauth_refresh_token=%s",
+			c->oauth_refresh_token);
+	}
+	secret_password_storev_sync(&schema,
 				    attributes,
 				    NULL,
 				    label,
-				    c->password,
+				    secret->str,
 				    NULL,
 				    &error);
+	g_string_free(secret, TRUE);
 	g_free(label);
 	g_hash_table_unref(attributes);
 
@@ -198,7 +250,7 @@ static int keyring_erase(struct credential *c)
 		return EXIT_FAILURE;
 
 	attributes = make_attr_list(c);
-	secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+	secret_password_clearv_sync(&schema,
 				    attributes,
 				    NULL,
 				    &error);
@@ -238,6 +290,8 @@ static void credential_clear(struct credential *c)
 	g_free(c->path);
 	g_free(c->username);
 	g_free(c->password);
+	g_free(c->password_expiry_utc);
+	g_free(c->oauth_refresh_token);
 
 	credential_init(c);
 }
@@ -284,11 +338,19 @@ static int credential_read(struct credential *c)
 		} else if (!strcmp(key, "username")) {
 			g_free(c->username);
 			c->username = g_strdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			g_free(c->password_expiry_utc);
+			c->password_expiry_utc = g_strdup(value);
 		} else if (!strcmp(key, "password")) {
 			g_free(c->password);
 			c->password = g_strdup(value);
 			while (*value)
 				*value++ = '\0';
+		} else if (!strcmp(key, "oauth_refresh_token")) {
+			g_free(c->oauth_refresh_token);
+			c->oauth_refresh_token = g_strdup(value);
+			while (*value)
+				*value++ = '\0';
 		}
 		/*
 		 * Ignore other lines; we don't know what they mean, but
@@ -314,6 +376,10 @@ static void credential_write(const struct credential *c)
 	/* only write username/password, if set */
 	credential_write_item(stdout, "username", c->username);
 	credential_write_item(stdout, "password", c->password);
+	credential_write_item(stdout, "password_expiry_utc",
+		c->password_expiry_utc);
+	credential_write_item(stdout, "oauth_refresh_token",
+		c->oauth_refresh_token);
 }
 
 static void usage(const char *name)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index f1ab92ba35c..4ad498adbc4 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -43,6 +43,7 @@ helper_test_clean() {
 	reject $1 https example.com store-user
 	reject $1 https example.com user1
 	reject $1 https example.com user2
+	reject $1 https example.com user3
 	reject $1 https example.com user4
 	reject $1 http path.tld user
 	reject $1 https timeout.tld user
@@ -328,6 +329,35 @@ helper_test_timeout() {
 	'
 }
 
+helper_test_password_expiry_utc() {
+	HELPER=$1
+
+	test_expect_success "helper ($HELPER) stores password_expiry_utc" '
+		check approve $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user3
+		password=pass
+		password_expiry_utc=9999999999
+		EOF
+	'
+
+	test_expect_success "helper ($HELPER) gets password_expiry_utc" '
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=example.com
+		username=user3
+		--
+		protocol=https
+		host=example.com
+		username=user3
+		password=pass
+		password_expiry_utc=9999999999
+		--
+		EOF
+	'
+}
+
 helper_test_oauth_refresh_token() {
 	HELPER=$1
 
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index c02a3b5969c..8300faadea9 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -29,6 +29,7 @@ test_atexit 'git credential-cache exit'
 
 # test that the daemon works with no special setup
 helper_test cache
+helper_test_password_expiry_utc cache
 helper_test_oauth_refresh_token cache
 
 test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..095574bfc6e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -45,6 +45,8 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
 helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
 
 helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
 
 if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
 	say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"

base-commit: 0df2c180904f6b709766f9c24669a9d01543f915

-- 
gitgitgadget