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
