Re: [PATCH v2] credential: new attribute password_expiry_utc

[M Hickford <mirth.hickford@gmail.com>]

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