Thanks Jeff for the review
On Wed, 1 Feb 2023 at 12:10, Jeff King <peff@xxxxxxxx> 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@xxxxxxxxxxxxxx/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
