credential/libsecret: support password_expiry_utc

From: M Hickford <[email protected]>

---
credential/libsecret: store password_expiry_utc

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

 .../libsecret/git-credential-libsecret.c      | 42 ++++++++++++++++---
 1 file changed, 37 insertions(+), 5 deletions(-)

index 2c5d76d789f..3f2b530db79 100644
@@ -39,6 +39,7 @@ struct credential {
 	char *path;
 	char *username;
 	char *password;
+	char *password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { 0 }

@@ -54,6 +55,20 @@ struct credential_operation {
 
 /* ----------------- Secret Service functions ----------------- */
 
+static const SecretSchema schema = {
+	"org.git.Password",
+	SECRET_SCHEMA_NONE,
+	{
+		{  "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 },
+	}
+};
+
 static char *make_label(struct credential *c)
 {
 	if (c->port)

@@ -78,6 +93,9 @@ 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;
 }

@@ -101,9 +119,11 @@ 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,
+					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
+					   // for backwards compatibility
+					   SECRET_SCHEMA_DONT_MATCH_NAME,
 					   NULL,
 					   &error);
 	g_hash_table_unref(attributes);

@@ -128,6 +148,12 @@ static int keyring_get(struct credential *c)
 			c->username = g_strdup(s);
 		}
 
+		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);

@@ -162,7 +188,7 @@ 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_password_storev_sync(&schema,
 				    attributes,
 				    NULL,
 				    label,

@@ -198,7 +224,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 +264,7 @@ 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);
 
 	credential_init(c);
 }

@@ -285,6 +312,9 @@ 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);

@@ -312,9 +342,11 @@ 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 */
 	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);
 }
 
 static void usage(const char *name)

base-commit: 73876f4861cd3d187a4682290ab75c9dccadbc56

gitgitgadget

From: M Hickford <[email protected]>

---
credential/libsecret: store password_expiry_utc

Patch v2 adds tests.

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

Range-diff vs v1:

1: 46ba4863f33 ! 1: 1e27677b6f5 credential/libsecret: support password_expiry_utc

     @@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_
       }
       
       static void usage(const char *name)
     +
     + ## t/lib-credential.sh ##
     +@@ t/lib-credential.sh: 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 http path.tld user
     + 	reject $1 https timeout.tld user
     + 	reject $1 https sso.tld
     +@@ t/lib-credential.sh: 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
     ++	'
     ++}
     ++
     + write_script askpass <<\EOF
     + echo >&2 askpass: $*
     + what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
     +
     + ## 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
     ++
     + 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
     + 
     ++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

 .../libsecret/git-credential-libsecret.c      | 42 ++++++++++++++++---
 t/lib-credential.sh                           | 30 +++++++++++++
 t/t0301-credential-cache.sh                   |  2 +
 t/t0303-credential-external.sh                |  2 +
 4 files changed, 71 insertions(+), 5 deletions(-)

index 2c5d76d789f..3f2b530db79 100644
@@ -39,6 +39,7 @@ struct credential {
 	char *path;
 	char *username;
 	char *password;
+	char *password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { 0 }

@@ -54,6 +55,20 @@ struct credential_operation {
 
 /* ----------------- Secret Service functions ----------------- */
 
+static const SecretSchema schema = {
+	"org.git.Password",
+	SECRET_SCHEMA_NONE,
+	{
+		{  "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 },
+	}
+};
+
 static char *make_label(struct credential *c)
 {
 	if (c->port)

@@ -78,6 +93,9 @@ 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;
 }

@@ -101,9 +119,11 @@ 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,
+					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
+					   // for backwards compatibility
+					   SECRET_SCHEMA_DONT_MATCH_NAME,
 					   NULL,
 					   &error);
 	g_hash_table_unref(attributes);

@@ -128,6 +148,12 @@ static int keyring_get(struct credential *c)
 			c->username = g_strdup(s);
 		}
 
+		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);

@@ -162,7 +188,7 @@ 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_password_storev_sync(&schema,
 				    attributes,
 				    NULL,
 				    label,

@@ -198,7 +224,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 +264,7 @@ 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);
 
 	credential_init(c);
 }

@@ -285,6 +312,9 @@ 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);

@@ -312,9 +342,11 @@ 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 */
 	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);
 }
 
 static void usage(const char *name)

index 5ea8bc9f1dc..9ebf7eeae48 100644
@@ -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 http path.tld user
 	reject $1 https timeout.tld user
 	reject $1 https sso.tld

@@ -298,6 +299,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
+	'
+}
+
 write_script askpass <<\EOF
 echo >&2 askpass: $*
 what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)

index 698b7159f03..f5ba727e53b 100755
@@ -30,6 +30,8 @@ test_atexit 'git credential-cache exit'
 # test that the daemon works with no special setup
 helper_test cache
 
+helper_test_password_expiry_utc cache
+
 test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
 	test_when_finished "
 		git credential-cache exit &&

index f028fd14182..f1478680bff 100755
@@ -52,6 +52,8 @@ else
 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
 fi
 
+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

base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e

gitgitgadget

Exactly.

I'll amend in v3.

Good spot. I must have misread the docs. I shall fix in v3 and test backwards compatibility carefully.

[1] https://gnome.pages.gitlab.gnome.org/libsecret/migrating-libgnome-keyring.html#working-with-schemas

Thanks Junio for the review.

The date has to be in the future otherwise the credential will be discarded by credential approve before it reaches the helper. That logic is already tested in t/t0300-credentials.sh. There isn't any expiry logic in the helper itself to test.

From: M Hickford <[email protected]>

d208bfd (credential: new attribute password_expiry_utc, 2023-02-18) introduced this attribute.

---
credential/libsecret: store password_expiry_utc

Patch v3 fixes backwards compatibility.

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

Range-diff vs v2:

1: 1e27677b6f5 ! 1: b46594c8897 credential/libsecret: support password_expiry_utc
@@ Metadata
## Commit message ##
credential/libsecret: support password_expiry_utc

+ d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
+ introduced this attribute.
+

## contrib/credential/libsecret/git-credential-libsecret.c ##
@@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera

+static const SecretSchema schema = {
+ "org.git.Password",
-+ SECRET_SCHEMA_NONE,
++ /* Ignore schema name for backwards compatibility with previous versions */
++ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
-- SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
-+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
-+ // for backwards compatibility
-+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
- &error);
- g_hash_table_unref(attributes);
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
c->username = g_strdup(s);
}

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

index 2c5d76d789f..182f0805c2b 100644
@@ -39,6 +39,7 @@ struct credential {
 	char *path;
 	char *username;
 	char *password;
+	char *password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { 0 }

@@ -54,6 +55,21 @@ struct credential_operation {
 
 /* ----------------- Secret Service functions ----------------- */
 
+static const SecretSchema schema = {
+	"org.git.Password",
+	/* Ignore schema name for backwards compatibility with previous versions */
+	SECRET_SCHEMA_DONT_MATCH_NAME,
+	{
+		{  "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 },
+	}
+};
+
 static char *make_label(struct credential *c)
 {
 	if (c->port)

@@ -78,6 +94,9 @@ 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;
 }

@@ -101,7 +120,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,

@@ -128,6 +147,12 @@ static int keyring_get(struct credential *c)
 			c->username = g_strdup(s);
 		}
 
+		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);

@@ -162,7 +187,7 @@ 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_password_storev_sync(&schema,
 				    attributes,
 				    NULL,
 				    label,

@@ -198,7 +223,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 +263,7 @@ 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);
 
 	credential_init(c);
 }

@@ -285,6 +311,9 @@ 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);

@@ -312,9 +341,11 @@ 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 */
 	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);
 }
 
 static void usage(const char *name)

index 5ea8bc9f1dc..9ebf7eeae48 100644
@@ -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 http path.tld user
 	reject $1 https timeout.tld user
 	reject $1 https sso.tld

@@ -298,6 +299,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
+	'
+}
+
 write_script askpass <<\EOF
 echo >&2 askpass: $*
 what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)

index 698b7159f03..f5ba727e53b 100755
@@ -30,6 +30,8 @@ test_atexit 'git credential-cache exit'
 # test that the daemon works with no special setup
 helper_test cache
 
+helper_test_password_expiry_utc cache
+
 test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
 	test_when_finished "
 		git credential-cache exit &&

index f028fd14182..f1478680bff 100755
@@ -52,6 +52,8 @@ else
 	helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
 fi
 
+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

base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e

gitgitgadget

Thanks for stopping me. I'll mark the topic as "on hold".

It does sound problematic, but if we think about what is used as keys and what is used as values, it does make a lot more sense to store the expiry as part of a value. After all, we are not even asking "give me the password that will expire in the most distant future" or anything like that. We consult the database with "who wants to access what server over which protocol at what port" as the key and expect we find the suitable authentication material to use. It would be best if we can treat the expiry date as an additional attribute of that authentication material.

Do the methods to store and retrieve a password from the keyring allow us to add such an extra attribute to the password? I have no idea how the Gnome keyring API works, but is there a way to mark each entry in the SecretSchemaAttributes as "this is used as a key" vs "this is used as a value---do not match"? Would thinking along such a line help?

Another possibility would be to store encoded concatenation of the real password and expiration timestamp and decode them into two upon retrieval. If we were the only user of the keystore, that may work, but if we are sharing the keystore with other applications, it would be a non-starter.

What do other application do, when using the keyring to store expirable passwords with services that do let you know the expiration time of the password? If they just ask the users again only after finding out that the password did not work, perhaps we should do the same, without being proactive and notice the expiry ourselves? That is, instead of failing the access to the server immediately upon seeing an auth failure, if the authentication material is know to have expiration time, can we let the application layer to ask the end-user to provide an refreshed password and try again? For such a scheme, we do not have to store ever-changing "password_expiry_utc" and contaminate the keyring with crufts whose expiry dates are the only difference. Instead we can just have a Boolean "does this site expire a valid password?" and use it to behave differently, if desired, from sites for which the passwords do not expire, perhaps?

Thanks Junio

Unfortunately not. The libsecret docs warn "Attributes are meant to be used for lookup of items; they’re not designed to be used as a generic key/value database".
https://gnome.pages.gitlab.gnome.org/libsecret/migrating-libgnome-keyring.html

Interestingly, Windows' wincred API doesn't have this issue, because it searches by a unique key, defined separately to attributes.

Yes, I think that would work nicely. A format such as that below would be backwards compatible (passwords already can't contain newlines) and self explanatory to any curious user browsing their secret store. I already have a draft that works much like this. I'll prepare a patch v4.

    7d7b554
    password_expiry_utc=1684179877
    oauth_refresh_token=be8a9aa3

Is the secret store ever shared with other applications such as a web browser? If so, sharing is already broken, because popular Git hosts such as GitHub and GitLab expect different passwords for web login and Git authentication (OAuth token or personal access token). A solution could be to introduce our own libsecret schema (as in the current patch) instead of continuing to use SECRET_SCHEMA_COMPAT_NETWORK potentially shared with other apps. I'm not sure whether that's worthwhile in this patch. I defer to you.

AFAIK major web browsers all implement their own password store rather than use libsecret, though apparently Chromium has a build flag to use libsecret on Linux.
https://source.chromium.org/search?q=libsecret%20-f:third_party&ss=chromium

In the case of HTTP error 401 Unauthorized, Git calls credential reject then exits. An improvement could be to first call credential fill again (this time without password prompt) and retry if a new credential is returned. A fresh credential if generated is likely to be valid. This would improve the experience when using a credential-generating helper together with a storage helper that drops password_expiry_utc.

Best remains to use a storage helper that stores password_expiry_utc. This avoids a doomed HTTP request. Storing oauth_refresh_token is also beneficial, because OAuth refresh is faster than cold OAuth.

It probably is a good argument. We do not have to worry about interoperating with browsers and their authentication with Git hosting sites. And the "newline can be used to add extra pieces of information" is a good trick ;-)

It may depend on what other Git GUI frontends may want to do. If there is no precedent and you are the pioneer, then using our own would be perfectly fine and we can let others also read from us if they want to; I presume that it would not prevent us from doing so even if did not use COMPAT_NETWORK (which gnome dev document even discourages of its use in new applications anyway).

Thanks.

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.

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.

---
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 <[email protected]>
      
       ## 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 <[email protected]>
      
     @@ 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(-)

index ef681f29d5b..31cf32ad969 100644
@@ -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)

index f1ab92ba35c..4ad498adbc4 100644
@@ -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
 

index c02a3b5969c..8300faadea9 100755
@@ -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' '

index f028fd14182..095574bfc6e 100755
@@ -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

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.

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.

---
credential/libsecret: store password_expiry_utc and oauth_refresh_token

Test that storing credential with expiry overwrites any old credential

Rename test users for clarity

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

Range-diff vs v4:

1: 07452a10372 ! 1: 38e02c5e24e credential/libsecret: store new attributes

     @@ t/lib-credential.sh: 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 user-expiry
     ++	reject $1 https example.com user-expiry-overwrite
       	reject $1 https example.com user4
       	reject $1 http path.tld user
       	reject $1 https timeout.tld user
     @@ t/lib-credential.sh: helper_test_timeout() {
      +		check approve $HELPER <<-\EOF
      +		protocol=https
      +		host=example.com
     -+		username=user3
     ++		username=user-expiry
      +		password=pass
      +		password_expiry_utc=9999999999
      +		EOF
     @@ t/lib-credential.sh: helper_test_timeout() {
      +		check fill $HELPER <<-\EOF
      +		protocol=https
      +		host=example.com
     -+		username=user3
     ++		username=user-expiry
      +		--
      +		protocol=https
      +		host=example.com
     -+		username=user3
     ++		username=user-expiry
      +		password=pass
      +		password_expiry_utc=9999999999
      +		--
      +		EOF
      +	'
     ++
     ++	test_expect_success "helper ($HELPER) overwrites when password_expiry_utc changes" '
     ++		check approve $HELPER <<-\EOF &&
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		password=pass1
     ++		password_expiry_utc=9999999998
     ++		EOF
     ++		check approve $HELPER <<-\EOF &&
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		password=pass2
     ++		password_expiry_utc=9999999999
     ++		EOF
     ++		check fill $HELPER <<-\EOF &&
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		--
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		password=pass2
     ++		password_expiry_utc=9999999999
     ++		EOF
     ++		check reject $HELPER <<-\EOF &&
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		password=pass2
     ++		EOF
     ++		check fill $HELPER <<-\EOF
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		--
     ++		protocol=https
     ++		host=example.com
     ++		username=user-expiry-overwrite
     ++		password=askpass-password
     ++		--
     ++		askpass: Password for '\''https://[email protected]'\'':
     ++		EOF
     ++	'
      +}
      +
       helper_test_oauth_refresh_token() {

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

index ef681f29d5b..31cf32ad969 100644
@@ -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)

index c02a3b5969c..8300faadea9 100755
@@ -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' '

index f028fd14182..095574bfc6e 100755
@@ -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: 9e49351c3060e1fa6e0d2de64505b7becf157f28

gitgitgadget