Re: [PATCH v2] credential: new attribute password_expiry_utc

[Jeff King <peff@peff.net>]

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