[PATCH] credential: new attribute password_expiry_utc

[PATCH] credential: new attribute password_expiry_utc

[M Hickford via GitGitGadget]

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

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.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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
    
    Signed-off-by: M Hickford mirth.hickford@gmail.com

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

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..15ace648bdd 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -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
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..5cb8a186b45 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -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")) {
diff --git a/credential.c b/credential.c
index f6389a50684..0a3a9cbf0a2 100644
--- a/credential.c
+++ b/credential.c
@@ -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,
diff --git a/credential.h b/credential.h
index f430e77fea4..e10f7c2b313 100644
--- a/credential.h
+++ b/credential.h
@@ -126,6 +126,7 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	time_t password_expiry_utc;
 };
 
 #define CREDENTIAL_INIT { \

base-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68
-- 
gitgitgadget

Re: [PATCH] credential: new attribute password_expiry_utc

[Junio C Hamano]

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> 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.

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


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

A overly long line.  Please follow Documentation/CodingGuidelines
and Documentation/SubmittingPatches

> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index f3c89831d4a..5cb8a186b45 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -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);
> +			}

Style (multiple issues, check CodingGuidelines):

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

 * Using integral value or pointer value as a truth value does not
   require an explicit comparison with 0;

 * A single-statement block does not need {} around it;

 * Overly long line should be folded, with properly indented.

> diff --git a/credential.c b/credential.c
> index f6389a50684..0a3a9cbf0a2 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -7,6 +7,7 @@
>  #include "prompt.h"
>  #include "sigchain.h"
>  #include "urlmatch.h"
> +#include <time.h>

Don't include system headers directly; often git-compat-util.h
already has it, and if not, we need to find the right place to have
it in git-compat-util.h file, as there are platforms that are
finicky in inclusion order of the header files and definition of
feature macros.

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

Not a huge deal, but if the rule is "an credential with expiry
timestamp that is too old behaves as if it no longer exists or is
valid", then a large integer, not zero, may serve as a better
sentinel value for "this entry never expires".  Instead of having to
do

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

you can just do

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

and that would be simpler to understand for human readers, too.

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

Do not use // comment.  /* Our single-liner comment reads like this */

> +			c->password_expiry_utc = atoi(value);

Don't use atoi(); make sure value is not followed by a non-number,
e.g.

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

would give you 43<43q>, but you want to reject and silently ignore
such an expiry timestamp.

> +		// if expiry date has passed, ignore password and expiry fields

Ditto, but if you used a large value as sentinel for "never expires"
and wrote it like this

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

then it is clear enough that you do not even need such a comment.
The expression itself makes it clear what is going on (i.e. the
current time comes later than the expiry_utc value on the number
line hence it appears on the right to it, clearly showing that it
has passed the threshold).

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

Re: [PATCH] credential: new attribute password_expiry_utc

[Eric Sunshine]

On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>

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

> diff --git a/credential.c b/credential.c
> @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
> +               // 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"));

Using `_(...)` marks a string for localization, but doing so is
undesirable for debugging messages which are meant for the developer,
not the end user (and it creates extra work for translators). No
existing[1] trace_printf() calls in the codebase use `_(...)`.

[1]: Unfortunately, a couple examples exist in
Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be
removed.

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

Style in this project is `char *str`, not `char* str`. Also, drop
spaces around function arguments:

    char *str = malloc(length + 1);

> +               snprintf( str, length + 1, "%ld", c->password_expiry_utc );

Same.

> +               credential_write_item(fp, "password_expiry_utc", str, 0);
> +               free(str);
> +       }

xstrfmt() from strbuf.h can help simplify this entire block:

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

Re: [PATCH] credential: new attribute password_expiry_utc

[M Hickford]

On Sun, 29 Jan 2023 at 20:17, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > 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.
>
> 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.
>

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

> 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").
>
>
> > +`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.
> > +
>
> A overly long line.  Please follow Documentation/CodingGuidelines
> and Documentation/SubmittingPatches
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index f3c89831d4a..5cb8a186b45 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -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);
> > +                     }
>
> Style (multiple issues, check CodingGuidelines):
>
>                 if (e->item.password_expiry_utc)
>                         fprintf(out, "... overly long format template ...",
>                                 e->item.password_expiry_utc);
>
>  * Using integral value or pointer value as a truth value does not
>    require an explicit comparison with 0;
>
>  * A single-statement block does not need {} around it;
>
>  * Overly long line should be folded, with properly indented.
>
> > diff --git a/credential.c b/credential.c
> > index f6389a50684..0a3a9cbf0a2 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -7,6 +7,7 @@
> >  #include "prompt.h"
> >  #include "sigchain.h"
> >  #include "urlmatch.h"
> > +#include <time.h>
>
> Don't include system headers directly; often git-compat-util.h
> already has it, and if not, we need to find the right place to have
> it in git-compat-util.h file, as there are platforms that are
> finicky in inclusion order of the header files and definition of
> feature macros.
>
> > @@ -21,6 +22,7 @@ void credential_clear(struct credential *c)
> >       free(c->path);
> >       free(c->username);
> >       free(c->password);
> > +     c->password_expiry_utc = 0;
>
> Not a huge deal, but if the rule is "an credential with expiry
> timestamp that is too old behaves as if it no longer exists or is
> valid", then a large integer, not zero, may serve as a better
> sentinel value for "this entry never expires".  Instead of having to
> do
>
>         if (expiry && expiry < time()) {
>                 ... expired ...
>         }
>
> you can just do
>
>         if (expiry < time()) {
>                 ... expired ...
>         }
>
> and that would be simpler to understand for human readers, too.
>
> > @@ -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
>
> Do not use // comment.  /* Our single-liner comment reads like this */
>
> > +                     c->password_expiry_utc = atoi(value);
>
> Don't use atoi(); make sure value is not followed by a non-number,
> e.g.
>
>         const char *value = "43q";
>         printf("%d<%s>\n", atoi(value), value);
>
> would give you 43<43q>, but you want to reject and silently ignore
> such an expiry timestamp.
>
> > +             // if expiry date has passed, ignore password and expiry fields
>
> Ditto, but if you used a large value as sentinel for "never expires"
> and wrote it like this
>
>                 if (c->password_expiry_utc < time(NULL)) {
>
> then it is clear enough that you do not even need such a comment.
> The expression itself makes it clear what is going on (i.e. the
> current time comes later than the expiry_utc value on the number
> line hence it appears on the right to it, clearly showing that it
> has passed the threshold).
>
> > +             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;
> > +             }
> > +

[PATCH v2] credential: new attribute password_expiry_utc

[M Hickford via GitGitGadget]

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

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

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
     @@ 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(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..667c4d80e26 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -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
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..338058be7f9 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -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")) {
diff --git a/credential.c b/credential.c
index f6389a50684..354fa1652a9 100644
--- a/credential.c
+++ b/credential.c
@@ -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",
diff --git a/credential.h b/credential.h
index f430e77fea4..935b28a70f1 100644
--- a/credential.h
+++ b/credential.h
@@ -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

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Jeff King]

On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:

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

This "should not" seems weird to me. The logic you have here throws out
entries that have expired when they pass through Git. But wouldn't
helpers which store things want to know about and act on the expiration,
too?

For example, if Git learns about a credential that expires in 60
seconds and passes it to credential-cache which is configured
--timeout=300, wouldn't it want to set its internal ttl on the
credential to 60, rather than 300?

I think your plan here is that Git would then reject the credential if a
request is made at time now+65. But the cache is holding onto it much
longer than necessary.

Likewise, wouldn't anything that stores credentials at least want to be
able to store and regurgitate the expiration? For instance, even
credential-store would want to do this. I'm OK if it doesn't, and we can
consider it a quality-of-implementation issue and see if anybody cares
enough to implement it. But I'd think most "real" helpers would want to
do so.

So it seems like helpers really do need to support this "expiration"
notion. And it's actually Git itself which doesn't need to care about
it, assuming the helpers are doing something sensible (though it is OK
if Git _also_ throws away expired credentials to support helpers which
don't).

> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> index f3c89831d4a..338058be7f9 100644
> --- a/builtin/credential-cache--daemon.c
> +++ b/builtin/credential-cache--daemon.c
> @@ -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);
>  		}

Is there a particular reason to use TIME_MAX as the sentinel value here,
and not just "0"? It's not that big a deal either way, but it's more
usual in our code base to use "0" if there's no reason not to (and it
seems like nothing should be expiring in 1970 these days).

> @@ -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)) {

This is comparing a timestamp_t to a time_t, which may mix
signed/unsigned. I can't offhand think of anything that would go too
wrong there before 2038, so it's probably OK, but I wanted to call it
out.

> @@ -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;

Do we need this logic? It seems weird that a helper would output an
expiration but not a password in the first place. I guess ignoring the
expiration is probably a reasonable outcome, but I wonder if a helper
would ever want to just add an expiration to the data coming from
another helper.

I.e., could we just read the value directly into c->password_expiry_utc
as we do with other fields?

-Peff

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
>> index f3c89831d4a..338058be7f9 100644
>> --- a/builtin/credential-cache--daemon.c
>> +++ b/builtin/credential-cache--daemon.c
>> @@ -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);
>>  		}
>
> Is there a particular reason to use TIME_MAX as the sentinel value here,
> and not just "0"? It's not that big a deal either way, but it's more
> usual in our code base to use "0" if there's no reason not to (and it
> seems like nothing should be expiring in 1970 these days).

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.

Re: [PATCH] credential: new attribute password_expiry_utc

[Junio C Hamano]

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

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

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.

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Matthew John Cheetham]

On 2023-02-01 04:10, Jeff King wrote:

> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:
> 
>> +`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.
> 
> This "should not" seems weird to me. The logic you have here throws out
> entries that have expired when they pass through Git. But wouldn't
> helpers which store things want to know about and act on the expiration,
> too?
> 
> For example, if Git learns about a credential that expires in 60
> seconds and passes it to credential-cache which is configured
> --timeout=300, wouldn't it want to set its internal ttl on the
> credential to 60, rather than 300?
> 
> I think your plan here is that Git would then reject the credential if a
> request is made at time now+65. But the cache is holding onto it much
> longer than necessary.
> 
> Likewise, wouldn't anything that stores credentials at least want to be
> able to store and regurgitate the expiration? For instance, even
> credential-store would want to do this. I'm OK if it doesn't, and we can
> consider it a quality-of-implementation issue and see if anybody cares
> enough to implement it. But I'd think most "real" helpers would want to
> do so.
> 
> So it seems like helpers really do need to support this "expiration"
> notion. And it's actually Git itself which doesn't need to care about
> it, assuming the helpers are doing something sensible (though it is OK
> if Git _also_ throws away expired credentials to support helpers which
> don't).

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.


>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
>> index f3c89831d4a..338058be7f9 100644
>> --- a/builtin/credential-cache--daemon.c
>> +++ b/builtin/credential-cache--daemon.c
>> @@ -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);
>>  		}
> 
> Is there a particular reason to use TIME_MAX as the sentinel value here,
> and not just "0"? It's not that big a deal either way, but it's more
> usual in our code base to use "0" if there's no reason not to (and it
> seems like nothing should be expiring in 1970 these days).
> 
>> @@ -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)) {
> 
> This is comparing a timestamp_t to a time_t, which may mix
> signed/unsigned. I can't offhand think of anything that would go too
> wrong there before 2038, so it's probably OK, but I wanted to call it
> out.
> 
>> @@ -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;
> 
> Do we need this logic? It seems weird that a helper would output an
> expiration but not a password in the first place. I guess ignoring the
> expiration is probably a reasonable outcome, but I wonder if a helper
> would ever want to just add an expiration to the data coming from
> another helper.
> 
> I.e., could we just read the value directly into c->password_expiry_utc
> as we do with other fields?
> 
> -Peff

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Jeff King]

On Wed, Feb 01, 2023 at 09:12:01AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> >> index f3c89831d4a..338058be7f9 100644
> >> --- a/builtin/credential-cache--daemon.c
> >> +++ b/builtin/credential-cache--daemon.c
> >> @@ -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);
> >>  		}
> >
> > Is there a particular reason to use TIME_MAX as the sentinel value here,
> > and not just "0"? It's not that big a deal either way, but it's more
> > usual in our code base to use "0" if there's no reason not to (and it
> > seems like nothing should be expiring in 1970 these days).
> 
> 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.

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

-Peff

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Jeff King]

On Wed, Feb 01, 2023 at 12:02:16PM -0800, Matthew John Cheetham wrote:

> > So it seems like helpers really do need to support this "expiration"
> > notion. And it's actually Git itself which doesn't need to care about
> > it, assuming the helpers are doing something sensible (though it is OK
> > if Git _also_ throws away expired credentials to support helpers which
> > don't).
> 
> 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.

FWIW, that is my general philosophy, too.

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

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.

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

Yeah, agreed.

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

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

[PATCH v3] credential: new attribute password_expiry_utc

[M Hickford via GitGitGadget]

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

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

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
     @@ 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(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..29d184ab824 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -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
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 4522471c337..95636b18439 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -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
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..338058be7f9 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -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")) {
diff --git a/credential.c b/credential.c
index f6389a50684..d3e1bf7a679 100644
--- a/credential.c
+++ b/credential.c
@@ -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;
 }
 
diff --git a/credential.h b/credential.h
index f430e77fea4..935b28a70f1 100644
--- a/credential.h
+++ b/credential.h
@@ -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. */
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 3485c0534e6..96391015af5 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -35,6 +35,16 @@ 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"
 '
 
@@ -109,6 +119,43 @@ 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
@@ -149,6 +196,24 @@ 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
@@ -159,6 +224,17 @@ 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
@@ -181,6 +257,24 @@ 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

base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
-- 
gitgitgadget

Re: [PATCH v2] credential: new attribute password_expiry_utc

[M Hickford]

Thanks Jeff for the review

On Wed, 1 Feb 2023 at 12:10, Jeff King <peff@peff.net> wrote:
>
> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:
>
> > +`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.
>
> This "should not" seems weird to me. The logic you have here throws out
> entries that have expired when they pass through Git. But wouldn't
> helpers which store things want to know about and act on the expiration,
> too?

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

>
> For example, if Git learns about a credential that expires in 60
> seconds and passes it to credential-cache which is configured
> --timeout=300, wouldn't it want to set its internal ttl on the
> credential to 60, rather than 300?
>
> I think your plan here is that Git would then reject the credential if a
> request is made at time now+65. But the cache is holding onto it much
> longer than necessary.

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

>
> Likewise, wouldn't anything that stores credentials at least want to be
> able to store and regurgitate the expiration? For instance, even
> credential-store would want to do this. I'm OK if it doesn't, and we can
> consider it a quality-of-implementation issue and see if anybody cares
> enough to implement it. But I'd think most "real" helpers would want to
> do so.

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.

>
> So it seems like helpers really do need to support this "expiration"
> notion. And it's actually Git itself which doesn't need to care about
> it, assuming the helpers are doing something sensible (though it is OK
> if Git _also_ throws away expired credentials to support helpers which
> don't).
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index f3c89831d4a..338058be7f9 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -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);
> >               }
>
> Is there a particular reason to use TIME_MAX as the sentinel value here,
> and not just "0"? It's not that big a deal either way, but it's more
> usual in our code base to use "0" if there's no reason not to (and it
> seems like nothing should be expiring in 1970 these days).

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

>
> > @@ -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)) {
>
> This is comparing a timestamp_t to a time_t, which may mix
> signed/unsigned. I can't offhand think of anything that would go too
> wrong there before 2038, so it's probably OK, but I wanted to call it
> out.
>
> > @@ -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;
>
> Do we need this logic? It seems weird that a helper would output an
> expiration but not a password in the first place. I guess ignoring the
> expiration is probably a reasonable outcome, but I wonder if a helper
> would ever want to just add an expiration to the data coming from
> another helper.
> I.e., could we just read the value directly into c->password_expiry_utc
> as we do with other fields?

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

>
> -Peff

Re: [PATCH v2] credential: new attribute password_expiry_utc

[M Hickford]

On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham
<mjcheetham@outlook.com> wrote:
>
> On 2023-02-01 04:10, Jeff King wrote:
>
> > On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:
> >
> >> +`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.
> >
> > This "should not" seems weird to me. The logic you have here throws out
> > entries that have expired when they pass through Git. But wouldn't
> > helpers which store things want to know about and act on the expiration,
> > too?
> >
> > For example, if Git learns about a credential that expires in 60
> > seconds and passes it to credential-cache which is configured
> > --timeout=300, wouldn't it want to set its internal ttl on the
> > credential to 60, rather than 300?
> >
> > I think your plan here is that Git would then reject the credential if a
> > request is made at time now+65. But the cache is holding onto it much
> > longer than necessary.
> >
> > Likewise, wouldn't anything that stores credentials at least want to be
> > able to store and regurgitate the expiration? For instance, even
> > credential-store would want to do this. I'm OK if it doesn't, and we can
> > consider it a quality-of-implementation issue and see if anybody cares
> > enough to implement it. But I'd think most "real" helpers would want to
> > do so.
> >
> > So it seems like helpers really do need to support this "expiration"
> > notion. And it's actually Git itself which doesn't need to care about
> > it, assuming the helpers are doing something sensible (though it is OK
> > if Git _also_ throws away expired credentials to support helpers which
> > don't).
>
> 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.

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

>
> 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.
>
>
> >> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> >> index f3c89831d4a..338058be7f9 100644
> >> --- a/builtin/credential-cache--daemon.c
> >> +++ b/builtin/credential-cache--daemon.c
> >> @@ -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);
> >>              }
> >
> > Is there a particular reason to use TIME_MAX as the sentinel value here,
> > and not just "0"? It's not that big a deal either way, but it's more
> > usual in our code base to use "0" if there's no reason not to (and it
> > seems like nothing should be expiring in 1970 these days).
> >
> >> @@ -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)) {
> >
> > This is comparing a timestamp_t to a time_t, which may mix
> > signed/unsigned. I can't offhand think of anything that would go too
> > wrong there before 2038, so it's probably OK, but I wanted to call it
> > out.
> >
> >> @@ -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;
> >
> > Do we need this logic? It seems weird that a helper would output an
> > expiration but not a password in the first place. I guess ignoring the
> > expiration is probably a reasonable outcome, but I wonder if a helper
> > would ever want to just add an expiration to the data coming from
> > another helper.
> >
> > I.e., could we just read the value directly into c->password_expiry_utc
> > as we do with other fields?
> >
> > -Peff

Re: [PATCH] credential: new attribute password_expiry_utc

[M Hickford]

Thanks Eric for the review

On Mon, 30 Jan 2023 at 00:59, Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sat, Jan 28, 2023 at 9:08 AM M Hickford via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: M Hickford <mirth.hickford@gmail.com>
>
> Just a few comments in addition to those already provided by Junio...
>
> > diff --git a/credential.c b/credential.c
> > @@ -234,11 +236,23 @@ int credential_read(struct credential *c, FILE *fp)
> > +               // 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"));
>
> Using `_(...)` marks a string for localization, but doing so is
> undesirable for debugging messages which are meant for the developer,
> not the end user (and it creates extra work for translators). No
> existing[1] trace_printf() calls in the codebase use `_(...)`.

Done in patch v3.

>
> [1]: Unfortunately, a couple examples exist in
> Documentation/MyFirstObjectWalk.txt using `_(...)` but they should be
> removed.
>
> > @@ -269,6 +283,13 @@ void credential_write(const struct credential *c, FILE *fp)
> > +       if (c->password_expiry_utc != 0) {
> > +               int length = snprintf( NULL, 0, "%ld", c->password_expiry_utc);
> > +               char* str = malloc( length + 1 );
>
> Style in this project is `char *str`, not `char* str`. Also, drop
> spaces around function arguments:
>
>     char *str = malloc(length + 1);
>
> > +               snprintf( str, length + 1, "%ld", c->password_expiry_utc );
>
> Same.
>
> > +               credential_write_item(fp, "password_expiry_utc", str, 0);
> > +               free(str);
> > +       }
>
> xstrfmt() from strbuf.h can help simplify this entire block:
>
>     char *s = xstrfmt("%ld", c->password_expiry_utc);
>     credential_write_item(fp, "password_expiry_utc", str, 0);
>     free(s);

Neat. Done in patch v3.

Re: [PATCH v2] credential: new attribute password_expiry_utc

[Matthew John Cheetham]

On 2023-02-04 22:45, M Hickford wrote:

> On Wed, 1 Feb 2023 at 20:02, Matthew John Cheetham
> <mjcheetham@outlook.com> wrote:
>>
>> On 2023-02-01 04:10, Jeff King wrote:
>>
>>> On Wed, Feb 01, 2023 at 09:39:51AM +0000, M Hickford via GitGitGadget wrote:
>>>
>>>> +`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.
>>>
>>> This "should not" seems weird to me. The logic you have here throws out
>>> entries that have expired when they pass through Git. But wouldn't
>>> helpers which store things want to know about and act on the expiration,
>>> too?
>>>
>>> For example, if Git learns about a credential that expires in 60
>>> seconds and passes it to credential-cache which is configured
>>> --timeout=300, wouldn't it want to set its internal ttl on the
>>> credential to 60, rather than 300?
>>>
>>> I think your plan here is that Git would then reject the credential if a
>>> request is made at time now+65. But the cache is holding onto it much
>>> longer than necessary.
>>>
>>> Likewise, wouldn't anything that stores credentials at least want to be
>>> able to store and regurgitate the expiration? For instance, even
>>> credential-store would want to do this. I'm OK if it doesn't, and we can
>>> consider it a quality-of-implementation issue and see if anybody cares
>>> enough to implement it. But I'd think most "real" helpers would want to
>>> do so.
>>>
>>> So it seems like helpers really do need to support this "expiration"
>>> notion. And it's actually Git itself which doesn't need to care about
>>> it, assuming the helpers are doing something sensible (though it is OK
>>> if Git _also_ throws away expired credentials to support helpers which
>>> don't).
>>
>> 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.
> 
> 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
> ```


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.

> [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
> 
>>
>> 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.
>>
>>
>>>> diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
>>>> index f3c89831d4a..338058be7f9 100644
>>>> --- a/builtin/credential-cache--daemon.c
>>>> +++ b/builtin/credential-cache--daemon.c
>>>> @@ -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);
>>>>              }
>>>
>>> Is there a particular reason to use TIME_MAX as the sentinel value here,
>>> and not just "0"? It's not that big a deal either way, but it's more
>>> usual in our code base to use "0" if there's no reason not to (and it
>>> seems like nothing should be expiring in 1970 these days).
>>>
>>>> @@ -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)) {
>>>
>>> This is comparing a timestamp_t to a time_t, which may mix
>>> signed/unsigned. I can't offhand think of anything that would go too
>>> wrong there before 2038, so it's probably OK, but I wanted to call it
>>> out.
>>>
>>>> @@ -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;
>>>
>>> Do we need this logic? It seems weird that a helper would output an
>>> expiration but not a password in the first place. I guess ignoring the
>>> expiration is probably a reasonable outcome, but I wonder if a helper
>>> would ever want to just add an expiration to the data coming from
>>> another helper.
>>>
>>> I.e., could we just read the value directly into c->password_expiry_utc
>>> as we do with other fields?
>>>
>>> -Peff

Re: [PATCH v3] credential: new attribute password_expiry_utc

[Junio C Hamano]

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> 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.
> ...
>  t/t0300-credentials.sh             | 94 ++++++++++++++++++++++++++++++

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

Re: [PATCH v3] credential: new attribute password_expiry_utc

[Martin Ågren]

On Sat, 4 Feb 2023 at 23:03, M Hickford via GitGitGadget
<gitgitgadget@gmail.com> wrote:

> --- a/Documentation/gitcredentials.txt
> +++ b/Documentation/gitcredentials.txt
> @@ -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.

s/a unexpired/an unexpired/ (or "a non-expired", perhaps)

Martin

Re: [PATCH v3] credential: new attribute password_expiry_utc

[M Hickford]

On Tue, 14 Feb 2023 at 01:59, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > 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.
> > ...
> >  t/t0300-credentials.sh             | 94 ++++++++++++++++++++++++++++++
>
> 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

Re: [PATCH v3] credential: new attribute password_expiry_utc

[Calvin Wan]

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

I see you null out c->password in the expiry if block so that the
following c->password check in the following if statement fails.
While I think it's neat little trick, I wonder if others on list
think it's better to be more explicit with how the logic should
work (eg. adding the c->passowrd_expiry_utc check as an inner
block inside of the c->username && c->password block).

Re: [PATCH v3] credential: new attribute password_expiry_utc

[Lessley Dennington]

On 2/14/23 3:36 PM, M Hickford wrote:
> Curious, let me take a look. I see that the tests failed on freebsd too.
> 
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

---

diff --git a/credential.c b/credential.c
index d3e1bf7a67..b9a9a1d7b1 100644
--- a/credential.c
+++ b/credential.c
@@ -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;

Re: [PATCH v3] credential: new attribute password_expiry_utc

[Junio C Hamano]

Lessley Dennington <lessleydennington@gmail.com> writes:

> diff --git a/credential.c b/credential.c
> index d3e1bf7a67..b9a9a1d7b1 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -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;

Ah, that is quite understandable. Successful library function calls
would not _clera_ errno, so if there were a failure before the
control reaches this codepath, errno may have been set, and then
parse_timestamp() call, which would be a strto$some_integral_type() call,
may succeed and will leave errno as-is.  Your fix is absolutely correct
as long as we want to use "errno" after the call returns.

When strtoumax() etc. wants to report overflow or underflow, the
returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be
ERANGE, so it would probably want to check that errno is that value,
and/or what c->password_expiry_utc has these overflow/underflow
values.

> ... I have not confirmed on freebsd,
> though, as a heads up.

[PATCH v4] credential: new attribute password_expiry_utc

[M Hickford via GitGitGadget]

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

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

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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
     -
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      
       ## 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(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index ac2818b9f66..29d184ab824 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -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
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 4522471c337..100f045bb1a 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -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
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index f3c89831d4a..338058be7f9 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -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")) {
diff --git a/credential.c b/credential.c
index f6389a50684..f32011343f9 100644
--- a/credential.c
+++ b/credential.c
@@ -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;
 }
 
diff --git a/credential.h b/credential.h
index f430e77fea4..935b28a70f1 100644
--- a/credential.h
+++ b/credential.h
@@ -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. */
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 3485c0534e6..c66d91e82d8 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -35,6 +35,16 @@ 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"
 '
 
@@ -109,6 +119,43 @@ 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 ignores 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
@@ -149,6 +196,24 @@ 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
@@ -159,6 +224,17 @@ test_expect_success 'do not bother storing password-less credential' '
 	EOF
 '
 
+test_expect_success 'credential_approve does not store expired password' '
+	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
@@ -181,6 +257,24 @@ test_expect_success 'credential_reject calls all helpers' '
 	EOF
 '
 
+test_expect_success 'credential_reject erases credential regardless of expiry' '
+	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

base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
-- 
gitgitgadget

Re: [PATCH v3] credential: new attribute password_expiry_utc

[M Hickford]

On Thu, 16 Feb 2023 at 19:16, Calvin Wan <calvinwan@google.com> wrote:
>
> >  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)
>
> I see you null out c->password in the expiry if block so that the
> following c->password check in the following if statement fails.
> While I think it's neat little trick, I wonder if others on list
> think it's better to be more explicit with how the logic should
> work (eg. adding the c->passowrd_expiry_utc check as an inner
> block inside of the c->username && c->password block).

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.

Re: [PATCH v3] credential: new attribute password_expiry_utc

[M Hickford]

On Fri, 17 Feb 2023 at 21:59, Junio C Hamano <gitster@pobox.com> wrote:
>
> Lessley Dennington <lessleydennington@gmail.com> writes:
>
> > diff --git a/credential.c b/credential.c
> > index d3e1bf7a67..b9a9a1d7b1 100644
> > --- a/credential.c
> > +++ b/credential.c
> > @@ -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;
>
> Ah, that is quite understandable. Successful library function calls
> would not _clera_ errno, so if there were a failure before the
> control reaches this codepath, errno may have been set, and then
> parse_timestamp() call, which would be a strto$some_integral_type() call,
> may succeed and will leave errno as-is.  Your fix is absolutely correct
> as long as we want to use "errno" after the call returns.
>
> When strtoumax() etc. wants to report overflow or underflow, the
> returned value must be UINTMAX_MAX/UINTMAX_MIN and errno would be
> ERANGE, so it would probably want to check that errno is that value,
> and/or what c->password_expiry_utc has these overflow/underflow
> values.
>
> > ... I have not confirmed on freebsd,
> > though, as a heads up.
>

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.

Re: [PATCH v4] credential: new attribute password_expiry_utc

[Calvin Wan]

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

Thanks for clarifying this block!

Overall, this patch is additive and shouldn't cause any
regressions for current users of credential/credential-helper
so I'm all for adding an expiry attribute to alleviate the
use case pains you described above.

Reviewed-by: Calvin Wan <calvinwan@google.com>