Re: [PATCH v2] credential: new attribute password_expiry_utc

From: Jeff King <peff@peff.net>
Date: Feb 1, 2023, 12:10 UTC
Message-ID: <Y9pWxHfgPtgCKO+B@coredump.intra.peff.net>
In-Reply-To: <pull.1443.v2.git.git.1675244392025.gitgitgadget@gmail.com>

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

Show 8 quoted lines

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

Show 12 quoted lines

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

Show 6 quoted lines

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

Show 26 quoted lines

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