credential: new attribute password_expiry_utc

From: M Hickford <[email protected]>

If password has expired, credential fill no longer returns early, so later helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. The expiry logic is entirely in the git credential layer; compatible helpers simply store and return the expiry attribute verbatim.

Store new attribute in cache.

---
credential: new attribute password_expiry_utc

Some passwords, such as a personal access token or OAuth access token, may have an expiry date (as long as years for PATs or as short as hours for an OAuth access token). Add a new credential attribute password_expiry_utc.

If password has expired, credential fill no longer returns early, so later helpers have opportunity to generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute. The expiry logic is entirely in the git credential layer. Credential-generating helpers need only output the expiry attribute. Storage helpers should store the expiry if they can.

Store expiry attribute in cache.

This is particularly useful when a storage helper and a credential-generating helper are configured together, eg.

[credential]
helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth

Without this patch, credential fill may return an expired credential from storage, causing authentication to fail. With this patch: a fresh credential is generated if and only if the credential is expired.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16/files

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v1
Pull-Request: https://github.com/git/git/pull/1443

 Documentation/git-credential.txt   |  4 ++++
 builtin/credential-cache--daemon.c |  3 +++
 credential.c                       | 21 +++++++++++++++++++++
 credential.h                       |  1 +
 4 files changed, 29 insertions(+)

index ac2818b9f66..15ace648bdd 100644
@@ -144,6 +144,10 @@ Git understands the following attributes:
 
 	The credential's password, if we are asking it to be stored.
 
+`password_expiry_utc`::
+
+	If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, `git credential fill` ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970.
+
 `url`::
 
 	When this special attribute is read by `git credential`, the

index f3c89831d4a..5cb8a186b45 100644
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			if (e->item.password_expiry_utc != 0) {
+				fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc);
+			}
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {

index f6389a50684..0a3a9cbf0a2 100644
@@ -7,6 +7,7 @@
 #include "prompt.h"
 #include "sigchain.h"
 #include "urlmatch.h"
+#include <time.h>
 
 void credential_init(struct credential *c)
 {

@@ -21,6 +22,7 @@ void credential_clear(struct credential *c)
 	free(c->path);
 	free(c->username);
 	free(c->password);
+	c->password_expiry_utc = 0;
 	string_list_clear(&c->helpers, 0);
 
 	credential_init(c);

@@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			// TODO: ignore if can't parse integer
+			c->password_expiry_utc = atoi(value);
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {
 			c->quit = !!git_config_bool("quit", value);
 		}
+
+		// if expiry date has passed, ignore password and expiry fields
+		if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
+			trace_printf(_("Password has expired.\n"));
+			FREE_AND_NULL(c->username);
+			FREE_AND_NULL(c->password);
+			c->password_expiry_utc = 0;
+		}
+
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do

@@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	if (c->password_expiry_utc != 0) {
+		int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
+		char* str = malloc( length + 1 );
+		snprintf( str, length + 1, "%ld", c->password_expiry_utc );
+		credential_write_item(fp, "password_expiry_utc", str, 0);
+		free(str);
+	}
 }
 
 static int run_credential_helper(struct credential *c,

index f430e77fea4..e10f7c2b313 100644
@@ -126,6 +126,7 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	time_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \

base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68

gitgitgadget

It is unclear what you are describing in the above. The current behaviour without the patch? The behaviour of the code if this patch gets applied? Write it in such a way that it is clear why the patch is a good idea, not just "this would not hurt because it is backwards compatible".

The usual way to do so is to sell your change in this order:

- Give background information to help readers understand what you are going to write in the following explanation.

- Describe the current behaviour without any change to the code;

- Present a situation where the current code results in an undesirable outcome. What exactly happens, what visible effect it has to the user, how the code could do better to help the user?

- Propose an updated behaviour that would behave better in the above sample situation presented.

Curiously, what you wrote below the "---" line, that will not be part of the log message, looks to be organized better than the above. The first paragraph (except for the "Add ...") prepares the readers, It is still unclear if the second paragraph "when expired" describes what happens with the current code (i.e. highlighting why a change is needed) or what you want to happen with the patch, but the paragraph should first explain the problem in the current behaviour to motivate readers to learn why the updated code would lead to a better world. And follow that with the behaviour of the updated code and its effect (e.g. "without first trying a credential that is stale and see it fail before asking to reauthenticate, such a known-to-be-stale credential gets discarded automatically").

		if (e->item.password_expiry_utc)
			fprintf(out, "... overly long format template ...",
				e->item.password_expiry_utc);

	if (expiry && expiry < time()) {
		... expired ...
	}

	if (expiry < time()) {
		... expired ...
	}

	const char *value = "43q";
	printf("%d<%s>\n", atoi(value), value);

		if (c->password_expiry_utc < time(NULL)) {

Just a few comments in addition to those already provided by Junio...

    char *str = malloc(length + 1);

    char *s = xstrfmt("%ld", c->password_expiry_utc);
    credential_write_item(fp, "password_expiry_utc", str, 0);
    free(s);

Thanks for the guidance. Writing a better commit message clarified my own thoughts.

From: M Hickford <[email protected]>

Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token.

Currently the credential protocol has no expiry attribute. When multiple helpers are configured, credential fill tries each helper in turn until it has a username and password, returning early.

When a storage helper and a credential-generating helper are configured together, the credential is necessarily stored without expiry, so credential fill may later return an expired credential from storage.

```
[credential]
	helper = storage  # eg. cache or osxkeychain
	helper = generate  # eg. oauth
```

An improvement is to introduce a password expiry attribute to the credential protocol. If the expiry date has passed, credential fill ignores the password attribute, so subsequent helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute.

Note that the expiry logic is entirely within the credential layer. Compatible helpers store and retrieve the new attribute like any other. This keeps the helper contract simple.

This patch adds support for the new attribute to cache.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

Future ideas: make it possible for a storage helper to provide OAuth refresh token to subsequent helpers.
https://github.com/gitgitgadget/git/pull/1394

---
credential: new attribute password_expiry_utc

Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token.

Currently the credential protocol has no expiry attribute. When multiple helpers are configured, credential fill tries each helper in turn until it has a username and password, returning early.

When a storage helper and a credential-generating helper are configured together, the credential is necessarily stored without expiry, so credential fill may later return an expired credential from storage.

[credential]
helper = storage # eg. cache or osxkeychain
helper = generate # eg. oauth

An improvement is to introduce a password expiry attribute to the credential protocol. If the password has expired, credential fill no longer returns early, so subsequent helpers can generate a fresh credential. This is backwards compatible -- no change in behaviour with helpers that discard the expiry attribute.

Note that the expiry logic is entirely within the credential layer. Compatible helpers store and retrieve the new attribute like any other. This keeps the helper contract simple.

This patch adds support for the new attribute to cache.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

Future ideas: make it possible for a storage helper to provide OAuth refresh token to subsequent helpers.
https://github.com/gitgitgadget/git/pull/1394

Questions for reviewers:

* Does the behaviour implemented match the documentation? (I'm not famiiliar with C)
* Any edge cases?
* How to test in t0300-credentials.sh ?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v2
Pull-Request: https://github.com/git/git/pull/1443

Range-diff vs v1:

1: 184b0aa6514 ! 1: b9ee729ee4d credential: new attribute password_expiry_utc
@@ Metadata
## Commit message ##
credential: new attribute password_expiry_utc

- If password has expired, credential fill no longer returns early,
- so later helpers can generate a fresh credential. This is backwards
- compatible -- no change in behaviour with helpers that discard the
- expiry attribute. The expiry logic is entirely in the git credential
- layer; compatible helpers simply store and return the expiry
- attribute verbatim.
+ Some passwords have an expiry date known at generation. This may be
+ years away for a personal access token or hours for an OAuth access
+ token.

- Store new attribute in cache.
+ Currently the credential protocol has no expiry attribute. When multiple
+ helpers are configured, credential fill tries each helper in turn
+ until it has a username and password, returning early.
+
+ When a storage helper and a credential-generating helper are configured
+ together, the credential is necessarily stored without expiry, so
+ credential fill may later return an expired credential from storage.
+
+ ```
+ [credential]
+ helper = storage # eg. cache or osxkeychain
+ helper = generate # eg. oauth
+ ```
+
+ An improvement is to introduce a password expiry attribute to the
+ credential protocol. If the expiry date has passed, credential fill
+ ignores the password attribute, so subsequent helpers can generate a
+ fresh credential. This is backwards compatible -- no change in
+ behaviour with helpers that discard the expiry attribute.
+
+ Note that the expiry logic is entirely within the credential layer.
+ Compatible helpers store and retrieve the new attribute like any other.
+ This keeps the helper contract simple.
+
+ This patch adds support for the new attribute to cache.
+
+ Example usage in a credential-generating helper
+ https://github.com/hickford/git-credential-oauth/pull/16
+
+ Future ideas: make it possible for a storage helper to provide OAuth
+ refresh token to subsequent helpers.
+ https://github.com/gitgitgadget/git/pull/1394

@@ Documentation/git-credential.txt: Git understands the following attributes:

+password_expiry_utc::
+
-+ If password is a personal access token or OAuth access token, it may have an expiry date. When getting credentials from a helper, git credential fill ignores the password attribute if the expiry date has passed. Storage helpers should store this attribute if possible. Helpers should not implement expiry logic themselves. Represented as Unix time UTC, seconds since 1970.
++ If password is a personal access token or OAuth access token, it may have an
++ expiry date. When getting credentials from a helper, git credential fill
++ ignores the password attribute if the expiry date has passed. Storage
++ helpers should store this attribute if possible. Helpers should not
++ implement expiry logic themselves. Represented as Unix time UTC, seconds
++ since 1970.
+
url::

@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE
if (e) {
fprintf(out, "username=%s\n", e->item.username); fprintf(out, "password=%s\n", e->item.password);
-+ if (e->item.password_expiry_utc != 0) {
-+ fprintf(out, "password_expiry_utc=%ld\n", e->item.password_expiry_utc);
-+ }
++ if (e->item.password_expiry_utc != TIME_MAX)
++ fprintf(out, "password_expiry_utc=%"PRItime"\n",
++ e->item.password_expiry_utc);
}
}
else if (!strcmp(action.buf, "exit")) {
@@ credential.c
#include "prompt.h"
#include "sigchain.h"
#include "urlmatch.h"
-+#include <time.h>
++#include "git-compat-util.h"

void credential_init(struct credential *c)
{
-@@ credential.c: void credential_clear(struct credential *c)
- free(c->path);
- free(c->username);
- free(c->password);
-+ c->password_expiry_utc = 0;
- string_list_clear(&c->helpers, 0);
+@@ credential.c: static void credential_getpass(struct credential *c)
+ if (!c->username)
+ c->username = credential_ask_one("Username", c,
+ PROMPT_ASKPASS|PROMPT_ECHO);
+- if (!c->password)
++ if (!c->password || c->password_expiry_utc < time(NULL)) {
++ c->password_expiry_utc = TIME_MAX;
+ c->password = credential_ask_one("Password", c,
+ PROMPT_ASKPASS);
++ }
+ }
+
+ int credential_read(struct credential *c, FILE *fp)
+ {
+ struct strbuf line = STRBUF_INIT;

- credential_init(c);
++ int password_updated = 0;
++ timestamp_t this_password_expiry = TIME_MAX;
++
+ while (strbuf_getline(&line, fp) != EOF) {
+ char *key = line.buf;
+ char *value = strchr(key, '=');
+@@ credential.c: int credential_read(struct credential *c, FILE *fp)
+ } else if (!strcmp(key, "password")) {
+ free(c->password);
+ c->password = xstrdup(value);
++ password_updated = 1;
+ } else if (!strcmp(key, "protocol")) {
+ free(c->protocol);
+ c->protocol = xstrdup(value);
@@ credential.c: int credential_read(struct credential *c, FILE *fp)
} else if (!strcmp(key, "path")) {
free(c->path);
c->path = xstrdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
-+ // TODO: ignore if can't parse integer
-+ c->password_expiry_utc = atoi(value);
++ this_password_expiry = parse_timestamp(value, NULL, 10);
++ if (this_password_expiry == 0 || errno) {
++ this_password_expiry = TIME_MAX;
++ }
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
- c->quit = !!git_config_bool("quit", value);
- }
-+
-+ // if expiry date has passed, ignore password and expiry fields
-+ if (c->password_expiry_utc != 0 && time(NULL) > c->password_expiry_utc) {
-+ trace_printf(_("Password has expired.\n"));
-+ FREE_AND_NULL(c->username);
-+ FREE_AND_NULL(c->password);
-+ c->password_expiry_utc = 0;
-+ }
+@@ credential.c: int credential_read(struct credential *c, FILE *fp)
+ */
+ }
+
++ if (password_updated)
++ c->password_expiry_utc = this_password_expiry;
+
- /*
- * Ignore other lines; we don't know what they mean, but
- * this future-proofs us when later versions of git do
+ strbuf_release(&line);
+ return 0;
+ }
@@ credential.c: void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0); credential_write_item(fp, "password", c->password, 0);
-+ if (c->password_expiry_utc != 0) {
-+ int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
-+ char* str = malloc( length + 1 );
-+ snprintf( str, length + 1, "%ld", c->password_expiry_utc );
-+ credential_write_item(fp, "password_expiry_utc", str, 0);
-+ free(str);
++ if (c->password_expiry_utc != TIME_MAX) {
++ char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
++ credential_write_item(fp, "password_expiry_utc", s, 0);
++ free(s);
+ }
}

static int run_credential_helper(struct credential *c,
+@@ credential.c: void credential_fill(struct credential *c) +
+ for (i = 0; i < c->helpers.nr; i++) {
+ credential_do(c, c->helpers.items[i].string, "get");
+- if (c->username && c->password)
++ if (c->username && c->password && time(NULL) < c->password_expiry_utc)
+ return;
+ if (c->quit)
+ die("credential helper '%s' told us to quit",

## credential.h ##
@@ credential.h: struct credential {
char *protocol;
char *host;
char *path;
-+ time_t password_expiry_utc;
++ timestamp_t password_expiry_utc;
};

#define CREDENTIAL_INIT { \
+ .helpers = STRING_LIST_INIT_DUP, \
++ .password_expiry_utc = TIME_MAX, \
+ }
+
+ /* Initialize a credential structure, setting all fields to empty. */

 Documentation/git-credential.txt   |  9 +++++++++
 builtin/credential-cache--daemon.c |  3 +++
 credential.c                       | 24 ++++++++++++++++++++++--
 credential.h                       |  2 ++
 4 files changed, 36 insertions(+), 2 deletions(-)

index ac2818b9f66..667c4d80e26 100644
@@ -144,6 +144,15 @@ Git understands the following attributes:
 
 	The credential's password, if we are asking it to be stored.
 
+`password_expiry_utc`::
+
+	If password is a personal access token or OAuth access token, it may have an
+	expiry date. When getting credentials from a helper, `git credential fill`
+	ignores the password attribute if the expiry date has passed. Storage
+	helpers should store this attribute if possible. Helpers should not
+	implement expiry logic themselves. Represented as Unix time UTC, seconds
+	since 1970.
+
 `url`::
 
 	When this special attribute is read by `git credential`, the

index f3c89831d4a..338058be7f9 100644
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			if (e->item.password_expiry_utc != TIME_MAX)
+				fprintf(out, "password_expiry_utc=%"PRItime"\n",
+					e->item.password_expiry_utc);
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {

index f6389a50684..354fa1652a9 100644
@@ -7,6 +7,7 @@
 #include "prompt.h"
 #include "sigchain.h"
 #include "urlmatch.h"
+#include "git-compat-util.h"
 
 void credential_init(struct credential *c)
 {

@@ -195,15 +196,20 @@ static void credential_getpass(struct credential *c)
 	if (!c->username)
 		c->username = credential_ask_one("Username", c,
 						 PROMPT_ASKPASS|PROMPT_ECHO);
-	if (!c->password)
+	if (!c->password || c->password_expiry_utc < time(NULL)) {
+		c->password_expiry_utc = TIME_MAX;
 		c->password = credential_ask_one("Password", c,
 						 PROMPT_ASKPASS);
+	}
 }
 
 int credential_read(struct credential *c, FILE *fp)
 {
 	struct strbuf line = STRBUF_INIT;
 
+	int password_updated = 0;
+	timestamp_t this_password_expiry = TIME_MAX;
+
 	while (strbuf_getline(&line, fp) != EOF) {
 		char *key = line.buf;
 		char *value = strchr(key, '=');

@@ -225,6 +231,7 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "password")) {
 			free(c->password);
 			c->password = xstrdup(value);
+			password_updated = 1;
 		} else if (!strcmp(key, "protocol")) {
 			free(c->protocol);
 			c->protocol = xstrdup(value);

@@ -234,6 +241,11 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			this_password_expiry = parse_timestamp(value, NULL, 10);
+			if (this_password_expiry == 0 || errno) {
+				this_password_expiry = TIME_MAX;
+			}
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {

@@ -246,6 +258,9 @@ int credential_read(struct credential *c, FILE *fp)
 		 */
 	}
 
+	if (password_updated)
+		c->password_expiry_utc = this_password_expiry;
+
 	strbuf_release(&line);
 	return 0;
 }

@@ -269,6 +284,11 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	if (c->password_expiry_utc != TIME_MAX) {
+		char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
+		credential_write_item(fp, "password_expiry_utc", s, 0);
+		free(s);
+	}
 }
 
 static int run_credential_helper(struct credential *c,

@@ -342,7 +362,7 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
-		if (c->username && c->password)
+		if (c->username && c->password && time(NULL) < c->password_expiry_utc)
 			return;
 		if (c->quit)
 			die("credential helper '%s' told us to quit",

index f430e77fea4..935b28a70f1 100644
@@ -126,10 +126,12 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	timestamp_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.password_expiry_utc = TIME_MAX, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */

base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68

gitgitgadget

This is my fault ;-). Here, there is no difference between 0 and TIME_MAX, but elsewhere the code needed

	if (expiry != 0 && expiry < time(NULL))

to see if the entry has expired. If the sentinel for an entry that will never expire were TIME_MAX, you do not need the first half of the expression.

I am OK either way.

It is great to hear that somebody agreeing with me on this point.

We try to write in a way that would make it clear to readers of "git log", but I often notice that trying to explain the changes to the code clearly does force me to realize better ways to write the changes.

I have often wondered about how, and if, Git should handle expiring credentials where the expiration is known. In my opinion I think Git should be doing *less* decision making with credentials and authentication in general, and leave that up to credential helpers.

The original design of credential helpers from what I can see (and Peff can correct me here of course!) is that they were really only thought about as storage-style helpers. Helpers are consulted for a known credential, and told about bad (erase) or good (store) credentials, all without any context about the request or remote responses.

If no credential helper can respond then Git itself prompts for a user/pass; so Git, or rather the user, is the 'generator'.

Of course that's not to say that credential generating helpers don't exist or are wrong - Git Credential Manager being of course one example rather close to home for me! However the current model, even with generating helpers, is still that Git will try and make the request given the details included in the helper response.

It doesn't make sense that a generating helper that knows about expiration would instead choose to respond with an expired credential rather than just try and generate a new credential.

Now in the case of a simple storage helper without such logic, after returning an expired credential should Git not be calling 'erase' back to the same helper to inform it that it has a stale credential and should be deleted? This would also require some affinity between calls to get/erase/store.

Ah. That at least is a compelling reason to use TIME_MAX. I'm OK with it.

-Peff

FWIW, that is my general philosophy, too.

They were always intended to be generators, too. In the early days we discussed having more fancy graphical prompts via helpers, though most people simply use the askpass interface for this.

My personal config for the last, say, 12 years, has been to pull the password out of a read-only store, and ignore "store" and "erase" requests entirely. Which I think counts as a generator. :)

But yeah, if there is more context that we can be giving the helpers to let them make a better decision, I'm all for it. I think your www-authenticate patches are a good step in that direction. I'm not sure what other context would be useful.

Yeah, agreed.

That's a good point. I think in practice it would mostly happen that Git would throw away the expired credential, generate a new one (either from another helper or via prompting), and then if that works, overwrite the old one with a 'store' request.

If the new credential doesn't work (or the user aborts), the expired credential is left in the helper. But in theory that doesn't matter. It will still be expired when it's served up again later. And it's not a security problem to hold onto it longer, since it's no longer valid. Still, it feels a bit clunky compared to having the helper realize it's holding garbage.

-Peff

From: M Hickford <[email protected]>

Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token.

When multiple credential helpers are configured, credential fill tries each helper in turn until it has a username and password, returning early. If Git authentication succeeds, credential approve stores the successful credential in all helpers. If authentication fails, credential reject erases matching credentials in all helpers. Helpers implement corresponding operations: get, store, erase.

The credential protocol has no expiry attribute, so helpers cannot store expiry information. (Even if a helper returned an improvised expiry attribute, git credential discards unrecognised attributes between operations and between helpers.)

As a workaround, whenever monolithic helper Git Credential Manager (GCM) retrieves an OAuth credential from its storage, it makes a HTTP request to check whether the OAuth token has expired [1]. This complicates and slows the authentication happy path.

Worse is the case that a storage helper and a credential-generating helper are configured together:

[credential]
helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth or manager

credential approve stores the generated credential in both helpers without expiry information. Later credential fill may return an expired credential from storage. There is no workaround, no matter how clever the second helper.

Introduce a password expiry attribute. In credential fill, ignore expired passwords and continue to query subsequent helpers.

In the example above, credential fill ignores the expired credential and a fresh credential is generated. If authentication succeeds, credential approve replaces the expired credential in storage. If authentication fails, the expired credential is erased by credential reject. It is unnecessary but harmless for storage helpers to self prune expired credentials.

Add support for the new attribute to credential-cache. Eventually, I hope to see support in other storage helpers.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

[1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217

---
credential: new attribute password_expiry_utc

details in commit message

Changes in patch v3:

* Tests
* Simplified credential_read, moved expiry logic to credential_fill

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v3
Pull-Request: https://github.com/git/git/pull/1443

Range-diff vs v2:

1: b9ee729ee4d ! 1: 1846815a5c1 credential: new attribute password_expiry_utc
@@ Commit message
years away for a personal access token or hours for an OAuth access token.

- Currently the credential protocol has no expiry attribute. When multiple
- helpers are configured, credential fill tries each helper in turn
- until it has a username and password, returning early.
+ When multiple credential helpers are configured, credential fill tries
+ each helper in turn until it has a username and password, returning
+ early. If Git authentication succeeds, credential approve
+ stores the successful credential in all helpers. If authentication
+ fails, credential reject erases matching credentials in all helpers.
+ Helpers implement corresponding operations: get, store, erase.

- When a storage helper and a credential-generating helper are configured
- together, the credential is necessarily stored without expiry, so
- credential fill may later return an expired credential from storage.
+ The credential protocol has no expiry attribute, so helpers cannot
+ store expiry information. (Even if a helper returned an improvised
+ expiry attribute, git credential discards unrecognised attributes
+ between operations and between helpers.)

- ```
- [credential]
- helper = storage # eg. cache or osxkeychain
- helper = generate # eg. oauth
- ```
+ As a workaround, whenever monolithic helper Git Credential Manager (GCM)
+ retrieves an OAuth credential from its storage, it makes a HTTP request
+ to check whether the OAuth token has expired [1]. This complicates and
+ slows the authentication happy path.

- An improvement is to introduce a password expiry attribute to the
- credential protocol. If the expiry date has passed, credential fill
- ignores the password attribute, so subsequent helpers can generate a
- fresh credential. This is backwards compatible -- no change in
- behaviour with helpers that discard the expiry attribute.
+ Worse is the case that a storage helper and a credential-generating
+ helper are configured together:

- Note that the expiry logic is entirely within the credential layer.
- Compatible helpers store and retrieve the new attribute like any other.
- This keeps the helper contract simple.
+ [credential]
+ helper = storage # eg. cache or osxkeychain
+ helper = generate # eg. oauth or manager

- This patch adds support for the new attribute to cache.
+ credential approve stores the generated credential in both helpers
+ without expiry information. Later credential fill may return an
+ expired credential from storage. There is no workaround, no matter how
+ clever the second helper.
+
+ Introduce a password expiry attribute. In credential fill, ignore
+ expired passwords and continue to query subsequent helpers.
+
+ In the example above, credential fill ignores the expired credential
+ and a fresh credential is generated. If authentication succeeds,
+ credential approve replaces the expired credential in storage.
+ If authentication fails, the expired credential is erased by
+ credential reject. It is unnecessary but harmless for storage
+ helpers to self prune expired credentials.
+
+ Add support for the new attribute to credential-cache.
+ Eventually, I hope to see support in other storage helpers.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

- Future ideas: make it possible for a storage helper to provide OAuth
- refresh token to subsequent helpers.
- https://github.com/gitgitgadget/git/pull/1394
+ [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217

@@ Documentation/git-credential.txt: Git understands the following attributes:

+password_expiry_utc::
+
-+ If password is a personal access token or OAuth access token, it may have an
-+ expiry date. When getting credentials from a helper, git credential fill
-+ ignores the password attribute if the expiry date has passed. Storage
-+ helpers should store this attribute if possible. Helpers should not
-+ implement expiry logic themselves. Represented as Unix time UTC, seconds
-+ since 1970.
++ Generated passwords such as an OAuth access token may have an expiry date.
++ When reading credentials from helpers, git credential fill ignores expired
++ passwords. Represented as Unix time UTC, seconds since 1970.
+
url::

When this special attribute is read by git credential, the

+ ## Documentation/gitcredentials.txt ##
+@@ Documentation/gitcredentials.txt: helper::
+ If there are multiple instances of the credential.helper configuration
+ variable, each helper will be tried in turn, and may provide a username,
+ password, or nothing. Once Git has acquired both a username and a +-password, no more helpers will be tried.
++unexpired password, no more helpers will be tried.
+ +
+ If credential.helper is configured to the empty string, this resets
+ the helper list to empty (so you may override a helper set by a
+
## builtin/credential-cache--daemon.c ##
@@ builtin/credential-cache--daemon.c: static void serve_one_client(FILE *in, FILE *out)
if (e) {
@@ credential.c

void credential_init(struct credential *c)
{
-@@ credential.c: static void credential_getpass(struct credential *c)
- if (!c->username)
- c->username = credential_ask_one("Username", c,
- PROMPT_ASKPASS|PROMPT_ECHO);
-- if (!c->password)
-+ if (!c->password || c->password_expiry_utc < time(NULL)) {
-+ c->password_expiry_utc = TIME_MAX;
- c->password = credential_ask_one("Password", c,
- PROMPT_ASKPASS);
-+ }
- }
-
- int credential_read(struct credential *c, FILE *fp)
- {
- struct strbuf line = STRBUF_INIT;
-
-+ int password_updated = 0;
-+ timestamp_t this_password_expiry = TIME_MAX;
-+
- while (strbuf_getline(&line, fp) != EOF) {
- char *key = line.buf;
- char *value = strchr(key, '=');
-@@ credential.c: int credential_read(struct credential *c, FILE *fp)
- } else if (!strcmp(key, "password")) {
- free(c->password);
- c->password = xstrdup(value);
-+ password_updated = 1;
- } else if (!strcmp(key, "protocol")) {
- free(c->protocol);
- c->protocol = xstrdup(value);
@@ credential.c: int credential_read(struct credential *c, FILE *fp)
} else if (!strcmp(key, "path")) {
free(c->path);
c->path = xstrdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
-+ this_password_expiry = parse_timestamp(value, NULL, 10);
-+ if (this_password_expiry == 0 || errno) {
-+ this_password_expiry = TIME_MAX;
-+ }
++ c->password_expiry_utc = parse_timestamp(value, NULL, 10);
++ if (c->password_expiry_utc == 0 || errno)
++ c->password_expiry_utc = TIME_MAX;
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
} else if (!strcmp(key, "quit")) {
-@@ credential.c: int credential_read(struct credential *c, FILE *fp)
- */
- }
-
-+ if (password_updated)
-+ c->password_expiry_utc = this_password_expiry;
-+
- strbuf_release(&line);
- return 0;
- }
@@ credential.c: void credential_write(const struct credential *c, FILE *fp)
credential_write_item(fp, "path", c->path, 0); credential_write_item(fp, "username", c->username, 0);
@@ credential.c: void credential_fill(struct credential *c)

for (i = 0; i < c->helpers.nr; i++) {
credential_do(c, c->helpers.items[i].string, "get");
-- if (c->username && c->password)
-+ if (c->username && c->password && time(NULL) < c->password_expiry_utc)
++ if (c->password_expiry_utc < time(NULL)) {
++ FREE_AND_NULL(c->password);
++ c->password_expiry_utc = TIME_MAX;
++ }
+ if (c->username && c->password)
return;
if (c->quit)
- die("credential helper '%s' told us to quit",
+@@ credential.c: void credential_approve(struct credential *c) +
+ if (c->approved)
+ return;
+- if (!c->username || !c->password)
++ if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
+ return;
+
+ credential_apply_config(c);
+@@ credential.c: void credential_reject(struct credential *c) +
+ FREE_AND_NULL(c->username);
+ FREE_AND_NULL(c->password);
++ c->password_expiry_utc = TIME_MAX;
+ c->approved = 0;
+ }
+

## credential.h ##
@@ credential.h: struct credential {
@@ credential.h: struct credential {
}

/* Initialize a credential structure, setting all fields to empty. */
+
+ ## t/t0300-credentials.sh ##
+@@ t/t0300-credentials.sh: test_expect_success 'setup helper scripts' '
+ test -z "$pass" || echo password=$pass
+ EOF
+
++ write_script git-credential-verbatim-with-expiry <<-\EOF &&
++ user=$1; shift
++ pass=$1; shift
++ pexpiry=$1; shift
++ . ./dump
++ test -z "$user" || echo username=$user
++ test -z "$pass" || echo password=$pass
++ test -z "$pexpiry" || echo password_expiry_utc=$pexpiry
++ EOF
++
+ PATH="$PWD:$PATH"
+ '
+
+@@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through partial response' '
+ EOF
+ '
+
++test_expect_success 'credential_fill populates password_expiry_utc' '
++ check fill "verbatim-with-expiry one two 9999999999" <<-\EOF
++ protocol=http
++ host=example.com
++ --
++ protocol=http
++ host=example.com
++ username=one
++ password=two
++ password_expiry_utc=9999999999
++ --
++ verbatim-with-expiry: get
++ verbatim-with-expiry: protocol=http
++ verbatim-with-expiry: host=example.com
++ EOF
++'
++
++test_expect_success 'credential_fill continues through expired password' '
++ check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF
++ protocol=http
++ host=example.com
++ --
++ protocol=http
++ host=example.com
++ username=three
++ password=four
++ --
++ verbatim-with-expiry: get
++ verbatim-with-expiry: protocol=http
++ verbatim-with-expiry: host=example.com
++ verbatim: get
++ verbatim: protocol=http
++ verbatim: host=example.com
++ verbatim: username=one
++ EOF
++'
++
+ test_expect_success 'credential_fill passes along metadata' '
+ check fill "verbatim one two" <<-\EOF
+ protocol=ftp
+@@ t/t0300-credentials.sh: test_expect_success 'credential_approve calls all helpers' '
+ EOF
+ '
+
++test_expect_success 'credential_approve stores password expiry' '
++ check approve useless <<-\EOF
++ protocol=http
++ host=example.com
++ username=foo
++ password=bar
++ password_expiry_utc=9999999999
++ --
++ --
++ useless: store
++ useless: protocol=http
++ useless: host=example.com
++ useless: username=foo
++ useless: password=bar
++ useless: password_expiry_utc=9999999999
++ EOF
++'
++
+ test_expect_success 'do not bother storing password-less credential' '
+ check approve useless <<-\EOF
+ protocol=http
+@@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less credential' '
+ EOF
+ '
+
++test_expect_success 'credential_approve does not store expired credential' '
++ check approve useless <<-\EOF
++ protocol=http
++ host=example.com
++ username=foo
++ password=bar
++ password_expiry_utc=5
++ --
++ --
++ EOF
++'
+
+ test_expect_success 'credential_reject calls all helpers' '
+ check reject useless "verbatim one two" <<-\EOF
+@@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers' '
+ EOF
+ '
+
++test_expect_success 'credential_reject erases expired credential' '
++ check reject useless <<-\EOF
++ protocol=http
++ host=example.com
++ username=foo
++ password=bar
++ password_expiry_utc=5
++ --
++ --
++ useless: erase
++ useless: protocol=http
++ useless: host=example.com
++ useless: username=foo
++ useless: password=bar
++ useless: password_expiry_utc=5
++ EOF
++'
++
+ test_expect_success 'usernames can be preserved' '
+ check fill "verbatim \"\" three" <<-\EOF
+ protocol=http

 Documentation/git-credential.txt   |  6 ++
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c |  3 +
 credential.c                       | 17 +++++-
 credential.h                       |  2 +
 t/t0300-credentials.sh             | 94 ++++++++++++++++++++++++++++++
 6 files changed, 122 insertions(+), 2 deletions(-)

index ac2818b9f66..29d184ab824 100644
@@ -144,6 +144,12 @@ Git understands the following attributes:
 
 	The credential's password, if we are asking it to be stored.
 
+`password_expiry_utc`::
+
+	Generated passwords such as an OAuth access token may have an expiry date.
+	When reading credentials from helpers, `git credential fill` ignores expired
+	passwords. Represented as Unix time UTC, seconds since 1970.
+
 `url`::
 
 	When this special attribute is read by `git credential`, the

index 4522471c337..95636b18439 100644
@@ -167,7 +167,7 @@ helper::
 If there are multiple instances of the `credential.helper` configuration
 variable, each helper will be tried in turn, and may provide a username,
 password, or nothing. Once Git has acquired both a username and a
-password, no more helpers will be tried.
+unexpired password, no more helpers will be tried.
 +
 If `credential.helper` is configured to the empty string, this resets
 the helper list to empty (so you may override a helper set by a

index f3c89831d4a..338058be7f9 100644
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			if (e->item.password_expiry_utc != TIME_MAX)
+				fprintf(out, "password_expiry_utc=%"PRItime"\n",
+					e->item.password_expiry_utc);
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {

index f6389a50684..d3e1bf7a679 100644
@@ -7,6 +7,7 @@
 #include "prompt.h"
 #include "sigchain.h"
 #include "urlmatch.h"
+#include "git-compat-util.h"
 
 void credential_init(struct credential *c)
 {

@@ -234,6 +235,10 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			c->password_expiry_utc = parse_timestamp(value, NULL, 10);
+			if (c->password_expiry_utc == 0 || errno)
+				c->password_expiry_utc = TIME_MAX;
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {

@@ -269,6 +274,11 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	if (c->password_expiry_utc != TIME_MAX) {
+		char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
+		credential_write_item(fp, "password_expiry_utc", s, 0);
+		free(s);
+	}
 }
 
 static int run_credential_helper(struct credential *c,

@@ -342,6 +352,10 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
+		if (c->password_expiry_utc < time(NULL)) {
+			FREE_AND_NULL(c->password);
+			c->password_expiry_utc = TIME_MAX;
+		}
 		if (c->username && c->password)
 			return;
 		if (c->quit)

@@ -360,7 +374,7 @@ void credential_approve(struct credential *c)
 
 	if (c->approved)
 		return;
-	if (!c->username || !c->password)
+	if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
 		return;
 
 	credential_apply_config(c);

@@ -381,6 +395,7 @@ void credential_reject(struct credential *c)
 
 	FREE_AND_NULL(c->username);
 	FREE_AND_NULL(c->password);
+	c->password_expiry_utc = TIME_MAX;
 	c->approved = 0;
 }
 

index f430e77fea4..935b28a70f1 100644
@@ -126,10 +126,12 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	timestamp_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.password_expiry_utc = TIME_MAX, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */

base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473

gitgitgadget

Thanks Jeff for the review

I wanted to keep the helper contract as simple as possible "here is an attribute to store and retrieve like any other". It doesn't matter if a helper stores a password beyond expiry, because Git will erase or replace it when it fails authentication or another succeeds. Relax the language in the patch v3 commit message to describe "self-pruning of expired passwords is unnecessary but harmless".

Even after the password expires, there's value to storing other attributes such as username and (perhaps in future) oauth-refresh-token. Hence I named the attribute 'password_expiry_utc' explicitly rather than 'credential_expiry_utc'.

Absolutely. Eventually I'd like to see support in git-credential-osxkeychain, git-credential-wincred, git-credential-libsecret etc. Mentioned this in patch v3 description.

Junio made a persuasive argument for readability
https://lore.kernel.org/git/CAPig+cQPLMrUKp0aqLCknSYCs5TAso-VSBYsQbGZ8g8wgY2Liw@mail.gmail.com/T/#mf66955cf3f53c073c68ad5ade7213617907bab63

Done in patch v3. The logic that remains (moved to credential_fill) is a simple sanity check.

GCM would benefit from being able to store expiry too. Whenever GCM retrieves an OAuth credential from storage, it queries the server to check whether the access token has expired [1]. This would become unnecessary. I've added more about this in patch v3 commit message.

Further, it solves a problem if GCM is configured after another storage helper:

```
[credential]
    helper = storage  # eg. osx-keychain or exotic
    helper = manager
```

Currently this may return an expired credential from storage.

Background for others: GCM is typically configured as the *only* helper, with its own internal storage configuration [2]. These reimplement or wrap popular Git storage helpers [3][4][5].

```
[credential]
    helper = manager
    credentialStore = keychain
```

[1] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/GitLab/GitLabHostProvider.cs
[2] https://github.com/GitCredentialManager/git-credential-manager/blob/main/docs/credstores.md
[3] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/MacOS/MacOSKeychain.cs
[4] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/Interop/Linux/SecretServiceCollection.cs
[5] https://github.com/GitCredentialManager/git-credential-manager/blob/main/src/shared/Core/CredentialCacheStore.cs

Thanks Eric for the review

Done in patch v3.

Neat. Done in patch v3.

One of the things that concerns me about the credential helper system today is the lack of 'affinity' across calls, and I think that we may disagree on this here.

A desire I have for the future of Git auth is that helpers can negotiate to start a more long-lived or complicated converstation, with retry and detailed feedback on the auth responses. I'm increasingly feeling that the get/store/erase model is not sufficient for this.

Imagine the scenario where the auth mechanism has a nonce that is updated on each successful (or failed) response. Here we'd want the helper that offered credentials to be told about the successful response for book-keeping, and not have that message 'stolen' by a simpler storage helper.

Another scenario would be multiple user accounts; a helper has credentials that would potentially be valid for the request (same host/forge or IdP for example), and we're wanting to avoid unnecessary user prompts as the user has not specified explicitly the account to 'bind' to that clone or specific remote. Why could we not return credentials to Git, also indicating that we have an ability to retry on a 401/403?

Increasingly, modern auth schemes have credentials that are so short lived (maybe bound to the exact specific request.. a one time use) that it really doesn't make sense to store any credentials at all. With such one-time-use credentials, and a simple storage helper ahead of a generating helper every other request would fail and the user would need to retry the command. Imagine the case that the OS/hardware security device holds the 'real' key or credential that is used to derive a one-time-use credential.

Strong auth mechamisms often tie credential generation strongly to storage.

https://github.com/git/git/actions/runs/4169057114/jobs/7217377625

Other platforms seem to be OK, but Windows test seems to be unhappy with it when this topic gets merged to 'seen'.

Curious, let me take a look. I see that the tests failed on freebsd too.

I don't have a Windows machine to debug. If anyone reading has a hypothesis, please share.

I shall try changing the default value for a password without expiry from TIME_MAX to 0, see if that works any better [2].

[1] https://github.com/git/git/pull/1443/checks?check_run_id=11112315291
[2] https://github.com/git/git/pull/1443/checks?check_run_id=11343677685

I was curious about this as well and took a look on my Windows machine. It appears that errno will be set to 'No such file or directory' unless you explicitly set it before checking it. This is odd, given that the helper script I wrote worked just fine without this requirement. However, I took a look through the rest of the codebase and noticed that errno = 0 seems to always be declared before this type of conditional check. That did indeed fix the issue - tests all pass on Windows with this patch. I have not confirmed on freebsd, though, as a heads up.

Thanks,
Lessley

---

index d3e1bf7a67..b9a9a1d7b1 100644
@@ -236,6 +236,7 @@ int credential_read(struct credential *c, FILE *fp)
                        free(c->path);
                        c->path = xstrdup(value);
                } else if (!strcmp(key, "password_expiry_utc")) {
+                       errno = 0;
                        c->password_expiry_utc = parse_timestamp(value, NULL, 10);
                        if (c->password_expiry_utc == 0 || errno)
                                c->password_expiry_utc = TIME_MAX;

From: M Hickford <[email protected]>

Some passwords have an expiry date known at generation. This may be years away for a personal access token or hours for an OAuth access token.

When multiple credential helpers are configured, credential fill tries each helper in turn until it has a username and password, returning early. If Git authentication succeeds, credential approve stores the successful credential in all helpers. If authentication fails, credential reject erases matching credentials in all helpers. Helpers implement corresponding operations: get, store, erase.

The credential protocol has no expiry attribute, so helpers cannot store expiry information. Even if a helper returned an improvised expiry attribute, git credential discards unrecognised attributes between operations and between helpers.

This is a particular issue when a storage helper and a credential-generating helper are configured together:

[credential]
helper = storage # eg. cache or osxkeychain helper = generate # eg. oauth

credential approve stores the generated credential in both helpers without expiry information. Later credential fill may return an expired credential from storage. There is no workaround, no matter how clever the second helper. The user sees authentication fail (a retry will succeed).

Introduce a password expiry attribute. In credential fill, ignore expired passwords and continue to query subsequent helpers.

In the example above, credential fill ignores the expired password and a fresh credential is generated. If authentication succeeds, credential approve replaces the expired password in storage. If authentication fails, the expired credential is erased by credential reject. It is unnecessary but harmless for storage helpers to self prune expired credentials.

Add support for the new attribute to credential-cache. Eventually, I hope to see support in other popular storage helpers.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

---
credential: new attribute password_expiry_utc

details in commit message

Changes in patch v4:

* Set errno = 0 to fix tests on Windows and FreeBSD (thanks Lessley Dennington)
* Clarify rationale as discussed at review club

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1443%2Fhickford%2Fpassword-expiry-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1443/hickford/password-expiry-v4
Pull-Request: https://github.com/git/git/pull/1443

Range-diff vs v3:

1: 1846815a5c1 ! 1: ae8bddbb30a credential: new attribute password_expiry_utc
@@ Commit message
Helpers implement corresponding operations: get, store, erase.

The credential protocol has no expiry attribute, so helpers cannot
- store expiry information. (Even if a helper returned an improvised
+ store expiry information. Even if a helper returned an improvised
expiry attribute, git credential discards unrecognised attributes
- between operations and between helpers.)
+ between operations and between helpers.

- As a workaround, whenever monolithic helper Git Credential Manager (GCM)
- retrieves an OAuth credential from its storage, it makes a HTTP request
- to check whether the OAuth token has expired [1]. This complicates and
- slows the authentication happy path.
-
- Worse is the case that a storage helper and a credential-generating
- helper are configured together:
+ This is a particular issue when a storage helper and a
+ credential-generating helper are configured together:

[credential]
helper = storage # eg. cache or osxkeychain
- helper = generate # eg. oauth or manager
+ helper = generate # eg. oauth

credential approve stores the generated credential in both helpers without expiry information. Later credential fill may return an expired credential from storage. There is no workaround, no matter how
- clever the second helper.
+ clever the second helper. The user sees authentication fail (a retry
+ will succeed).

Introduce a password expiry attribute. In credential fill, ignore expired passwords and continue to query subsequent helpers.

- In the example above, credential fill ignores the expired credential
+ In the example above, credential fill ignores the expired password
and a fresh credential is generated. If authentication succeeds,
- credential approve replaces the expired credential in storage.
+ credential approve replaces the expired password in storage.
If authentication fails, the expired credential is erased by credential reject. It is unnecessary but harmless for storage helpers to self prune expired credentials.

Add support for the new attribute to credential-cache.
- Eventually, I hope to see support in other storage helpers.
+ Eventually, I hope to see support in other popular storage helpers.

Example usage in a credential-generating helper
https://github.com/hickford/git-credential-oauth/pull/16

- [1] https://github.com/GitCredentialManager/git-credential-manager/blob/66b94e489ad8cc1982836355493e369770b30211/src/shared/GitLab/GitLabHostProvider.cs#L217
-

## Documentation/git-credential.txt ##
@@ Documentation/gitcredentials.txt: helper::
variable, each helper will be tried in turn, and may provide a username, password, or nothing. Once Git has acquired both a username and a
-password, no more helpers will be tried.
-+unexpired password, no more helpers will be tried.
++non-expired password, no more helpers will be tried.
+
If credential.helper is configured to the empty string, this resets the helper list to empty (so you may override a helper set by a
@@ credential.c: int credential_read(struct credential *c, FILE *fp)
free(c->path);
c->path = xstrdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
++ errno = 0;
+ c->password_expiry_utc = parse_timestamp(value, NULL, 10);
-+ if (c->password_expiry_utc == 0 || errno)
++ if (c->password_expiry_utc == 0 || errno == ERANGE)
+ c->password_expiry_utc = TIME_MAX;
} else if (!strcmp(key, "url")) {
credential_from_url(c, value);
@@ credential.c: void credential_fill(struct credential *c)
for (i = 0; i < c->helpers.nr; i++) {
credential_do(c, c->helpers.items[i].string, "get");
+ if (c->password_expiry_utc < time(NULL)) {
++ /* Discard expired password */
+ FREE_AND_NULL(c->password);
++ /* Reset expiry to maintain consistency */
+ c->password_expiry_utc = TIME_MAX;
+ }
if (c->username && c->password)
@@ t/t0300-credentials.sh: test_expect_success 'credential_fill continues through p
+ EOF
+'
+
-+test_expect_success 'credential_fill continues through expired password' ' ++test_expect_success 'credential_fill ignores expired password' '
+ check fill "verbatim-with-expiry one two 5" "verbatim three four" <<-\EOF
+ protocol=http
+ host=example.com
@@ t/t0300-credentials.sh: test_expect_success 'do not bother storing password-less
EOF
'

-+test_expect_success 'credential_approve does not store expired credential' ' ++test_expect_success 'credential_approve does not store expired password' '
+ check approve useless <<-\EOF
+ protocol=http
+ host=example.com
@@ t/t0300-credentials.sh: test_expect_success 'credential_reject calls all helpers
EOF
'

-+test_expect_success 'credential_reject erases expired credential' ' ++test_expect_success 'credential_reject erases credential regardless of expiry' '
+ check reject useless <<-\EOF
+ protocol=http
+ host=example.com

 Documentation/git-credential.txt   |  6 ++
 Documentation/gitcredentials.txt   |  2 +-
 builtin/credential-cache--daemon.c |  3 +
 credential.c                       | 20 ++++++-
 credential.h                       |  2 +
 t/t0300-credentials.sh             | 94 ++++++++++++++++++++++++++++++
 6 files changed, 125 insertions(+), 2 deletions(-)

index ac2818b9f66..29d184ab824 100644
@@ -144,6 +144,12 @@ Git understands the following attributes:
 
 	The credential's password, if we are asking it to be stored.
 
+`password_expiry_utc`::
+
+	Generated passwords such as an OAuth access token may have an expiry date.
+	When reading credentials from helpers, `git credential fill` ignores expired
+	passwords. Represented as Unix time UTC, seconds since 1970.
+
 `url`::
 
 	When this special attribute is read by `git credential`, the

index 4522471c337..100f045bb1a 100644
@@ -167,7 +167,7 @@ helper::
 If there are multiple instances of the `credential.helper` configuration
 variable, each helper will be tried in turn, and may provide a username,
 password, or nothing. Once Git has acquired both a username and a
-password, no more helpers will be tried.
+non-expired password, no more helpers will be tried.
 +
 If `credential.helper` is configured to the empty string, this resets
 the helper list to empty (so you may override a helper set by a

index f3c89831d4a..338058be7f9 100644
@@ -127,6 +127,9 @@ static void serve_one_client(FILE *in, FILE *out)
 		if (e) {
 			fprintf(out, "username=%s\n", e->item.username);
 			fprintf(out, "password=%s\n", e->item.password);
+			if (e->item.password_expiry_utc != TIME_MAX)
+				fprintf(out, "password_expiry_utc=%"PRItime"\n",
+					e->item.password_expiry_utc);
 		}
 	}
 	else if (!strcmp(action.buf, "exit")) {

index f6389a50684..f32011343f9 100644
@@ -7,6 +7,7 @@
 #include "prompt.h"
 #include "sigchain.h"
 #include "urlmatch.h"
+#include "git-compat-util.h"
 
 void credential_init(struct credential *c)
 {

@@ -234,6 +235,11 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "password_expiry_utc")) {
+			errno = 0;
+			c->password_expiry_utc = parse_timestamp(value, NULL, 10);
+			if (c->password_expiry_utc == 0 || errno == ERANGE)
+				c->password_expiry_utc = TIME_MAX;
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {

@@ -269,6 +275,11 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	if (c->password_expiry_utc != TIME_MAX) {
+		char *s = xstrfmt("%"PRItime, c->password_expiry_utc);
+		credential_write_item(fp, "password_expiry_utc", s, 0);
+		free(s);
+	}
 }
 
 static int run_credential_helper(struct credential *c,

@@ -342,6 +353,12 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
+		if (c->password_expiry_utc < time(NULL)) {
+			/* Discard expired password */
+			FREE_AND_NULL(c->password);
+			/* Reset expiry to maintain consistency */
+			c->password_expiry_utc = TIME_MAX;
+		}
 		if (c->username && c->password)
 			return;
 		if (c->quit)

@@ -360,7 +377,7 @@ void credential_approve(struct credential *c)
 
 	if (c->approved)
 		return;
-	if (!c->username || !c->password)
+	if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
 		return;
 
 	credential_apply_config(c);

@@ -381,6 +398,7 @@ void credential_reject(struct credential *c)
 
 	FREE_AND_NULL(c->username);
 	FREE_AND_NULL(c->password);
+	c->password_expiry_utc = TIME_MAX;
 	c->approved = 0;
 }
 

index f430e77fea4..935b28a70f1 100644
@@ -126,10 +126,12 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	timestamp_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.password_expiry_utc = TIME_MAX, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */

base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473

gitgitgadget

It's important to reset the expiry date as well as discard the expired password so that fill accepts a later password without expiry (see test cases). I'll add a comment in patch v4.

That's a subtle one! Thanks Lessley very much for your help debugging. I shall send a patch v4 with this and some minor changes discussed in review club.