Re: [PATCH v2] credential: new attribute password_expiry_utc

[Matthew John Cheetham <mjcheetham@outlook.com>]

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