From: M Hickford <mirth.hickford@gmail.com>
To: Jeff King <peff@peff.net>
Cc: M Hickford via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>,
Cheetham <mjcheetham@outlook.com>,
Dennington <lessleydennington@gmail.com>,
M Hickford <mirth.hickford@gmail.com>
Subject: Re: [PATCH v2] credential: new attribute password_expiry_utc
Date: Sun, 5 Feb 2023 06:34:00 +0000 [thread overview]
Message-ID: <CAGJzqsmN4=SQv+6wPP8Tb+jmiyw=wrDKz49=s1+as+VkHgF1MQ@mail.gmail.com> (raw)
In-Reply-To: <Y9pWxHfgPtgCKO+B@coredump.intra.peff.net>
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
next prev parent reply other threads:[~2023-02-05 6:34 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-28 14:04 [PATCH] credential: new attribute password_expiry_utc M Hickford via GitGitGadget
2023-01-29 20:17 ` Junio C Hamano
2023-02-01 8:29 ` M Hickford
2023-02-01 18:50 ` Junio C Hamano
2023-01-30 0:59 ` Eric Sunshine
2023-02-05 6:49 ` M Hickford
2023-02-01 9:39 ` [PATCH v2] " M Hickford via GitGitGadget
2023-02-01 12:10 ` Jeff King
2023-02-01 17:12 ` Junio C Hamano
2023-02-02 0:12 ` Jeff King
2023-02-01 20:02 ` Matthew John Cheetham
2023-02-02 0:23 ` Jeff King
2023-02-05 6:45 ` M Hickford
2023-02-06 18:59 ` Matthew John Cheetham
2023-02-05 6:34 ` M Hickford [this message]
2023-02-04 21:16 ` [PATCH v3] " M Hickford via GitGitGadget
2023-02-14 1:59 ` Junio C Hamano
2023-02-14 22:36 ` M Hickford
2023-02-17 21:44 ` Lessley Dennington
2023-02-17 21:59 ` Junio C Hamano
2023-02-18 8:00 ` M Hickford
2023-02-14 8:03 ` Martin Ågren
2023-02-16 19:16 ` Calvin Wan
2023-02-18 8:00 ` M Hickford
2023-02-18 6:32 ` [PATCH v4] " M Hickford via GitGitGadget
2023-02-22 19:22 ` Calvin Wan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGJzqsmN4=SQv+6wPP8Tb+jmiyw=wrDKz49=s1+as+VkHgF1MQ@mail.gmail.com' \
--to=mirth.hickford@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=lessleydennington@gmail.com \
--cc=mjcheetham@outlook.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).