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>,
"Martin Ågren" <martin.agren@gmail.com>,
"Calvin Wan" <calvinwan@google.com>,
"M Hickford" <mirth.hickford@gmail.com>
Subject: Re: [PATCH v2] credential: new attribute oauth_refresh_token
Date: Tue, 25 Apr 2023 20:19:40 +0100 [thread overview]
Message-ID: <CAGJzqsmAESKYd5AYoZ17pg0F9Oe2sK_Odwpk4niQK9njtpCkTg@mail.gmail.com> (raw)
In-Reply-To: <20230425064758.GD4061254@coredump.intra.peff.net>
On Tue, 25 Apr 2023 at 07:48, Jeff King <peff@peff.net> wrote:
>
> On Fri, Apr 21, 2023 at 09:47:59AM +0000, M Hickford via GitGitGadget wrote:
>
> > Git authentication with OAuth access token is supported by every popular
> > Git host including GitHub, GitLab and BitBucket [1][2][3]. Credential
> > helpers Git Credential Manager (GCM) and git-credential-oauth generate
> > OAuth credentials [4][5]. Following RFC 6749, the application prints a
> > link for the user to authorize access in browser. A loopback redirect
> > communicates the response including access token to the application.
> >
> > For security, RFC 6749 recommends that OAuth response also includes
> > expiry date and refresh token [6]. After expiry, applications can use
> > the refresh token to generate a new access token without user
> > reauthorization in browser. GitLab and BitBucket set the expiry at two
> > hours [2][3]. (GitHub doesn't populate expiry or refresh token.)
> >
> > However the Git credential protocol has no attribute to store the OAuth
> > refresh token (unrecognised attributes are silently discarded). This
> > means that the user has to regularly reauthorize the helper in browser.
> > On a browserless system, this is particularly intrusive, requiring a
> > second device.
> >
> > Introduce a new attribute oauth_refresh_token. This is especially
> > useful when a storage helper and a read-only OAuth helper are configured
> > together. Recall that `credential fill` calls each helper until it has a
> > non-expired password.
> >
> > ```
> > [credential]
> > helper = storage # eg. cache or osxkeychain
> > helper = oauth
> > ```
>
> OK. I don't have much knowledge of OAuth, but taking the notion of "this
> is a useful thing for oauth clients to store" as a given, the
> implementation seems reasonable.
>
> It does feel a bit funny, in that nothing _except_ credential-cache is
> going to understand or store this thing. In general, I'd expect most
> helpers doing anything as complex as oauth to just implement their own
> storage layer. But I get that your goal here is trying to keep things as
> composable as possible. I just wonder how successful it will be if there
> is only one helper that you can actually use for storage.
>
> > Add support for the new attribute to credential-cache. Eventually, I
> > hope to see support in other popular storage helpers.
>
> At least your optimism is documented. :)
I have a draft patch for credential-libsecret
https://github.com/gitgitgadget/git/pull/1524 which I could add to
this patch series if you like. Helpers credential-wincred and
credential-osxkeychain would be easy to update following the same
approach.
>
> > Alternatives considered: ask helpers to store all unrecognised
> > attributes. This seems excessively complex for no obvious gain.
> > Helpers would also need extra information to distinguish between
> > confidential and non-confidential attributes.
>
> I've written the "store arbitrary attributes" patch before, and it
> actually is quite simple (if you neglect the "confidential" concept,
> which is largely what the current protocol and helpers do). But I don't
> know that it buys that much anyway, if helpers need to decide whether
> and how to store these items anyway. We get support in credential-cache
> for "free" because it happens to store the same in-memory struct that
> Git itself does, but most other helpers would need special patches. And
> because they're often interfacing with system storage, they can't
> necessarily just store arbitrary items.
>
> > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
> > index 62c09a271d6..9db5f00184d 100644
> > --- a/builtin/credential-cache--daemon.c
> > +++ b/builtin/credential-cache--daemon.c
> > @@ -133,6 +133,9 @@ static void serve_one_client(FILE *in, FILE *out)
> > if (e->item.password_expiry_utc != TIME_MAX)
> > fprintf(out, "password_expiry_utc=%"PRItime"\n",
> > e->item.password_expiry_utc);
> > + if (e->item.oauth_refresh_token)
> > + fprintf(out, "oauth_refresh_token=%s\n",
> > + e->item.oauth_refresh_token);
> > }
> > }
> > else if (!strcmp(action.buf, "exit")) {
>
> This hunk makes me wonder if the cache daemon should just be using
> credential_write(), which would then support this automatically (along
> with any other fields, like the expiry, or the new wwwauth stuff).
>
> It would mean that it spits back parts of the query (like host, etc) for
> a "get", rather than just username/password. That's not wrong, but is
> maybe a little weird.
>
> I can live with it as-is, certainly.
>
> > +helper_test_oauth_refresh_token() {
> > + HELPER=$1
> > +
> > + test_expect_success "helper ($HELPER) stores oauth_refresh_token" '
> > + check approve $HELPER <<-\EOF
> > + protocol=https
> > + host=example.com
> > + username=user4
> > + password=pass
> > + oauth_refresh_token=xyzzy
> > + EOF
> > + '
> > +
> > + test_expect_success "helper ($HELPER) gets oauth_refresh_token" '
> > + check fill $HELPER <<-\EOF
> > + protocol=https
> > + host=example.com
> > + username=user4
> > + --
> > + protocol=https
> > + host=example.com
> > + username=user4
> > + password=pass
> > + oauth_refresh_token=xyzzy
> > + --
> > + EOF
> > + '
> > +}
>
> This is in a separate function, so normal t0303 tests won't run it (and
> confuse users when they fail because they don't support this field).
> Good.
>
> Possibly t0303 should learn to optionally trigger this the way it does
> with helper_test_timeout. But I'm also content to leave it until
> somebody has a helper they want to test with it.
>
> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> > index c66d91e82d8..b49fc14a2bd 100755
> > --- a/t/t0300-credentials.sh
> > +++ b/t/t0300-credentials.sh
> > @@ -214,6 +214,24 @@ test_expect_success 'credential_approve stores password expiry' '
> > EOF
> > '
> >
> > +test_expect_success 'credential_approve stores oauth refresh token' '
> > + check approve useless <<-\EOF
> > + protocol=http
> > + host=example.com
> > + username=foo
> > + password=bar
> > + oauth_refresh_token=xyzzy
> > + --
> > + --
> > + useless: store
> > + useless: protocol=http
> > + useless: host=example.com
> > + useless: username=foo
> > + useless: password=bar
> > + useless: oauth_refresh_token=xyzzy
> > + EOF
> > +'
>
> Makes sense. This file is just checking how Git passes the data around
> between helpers.
>
> > diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> > index 698b7159f03..c02a3b5969c 100755
> > --- a/t/t0301-credential-cache.sh
> > +++ b/t/t0301-credential-cache.sh
> > @@ -29,6 +29,7 @@ test_atexit 'git credential-cache exit'
> >
> > # test that the daemon works with no special setup
> > helper_test cache
> > +helper_test_oauth_refresh_token cache
> >
> > test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
> > test_when_finished "
>
> And this one is just testing credential-cache, which we know supports
> the feature. Good.
>
> So the patch looks like it correctly implements the intent. I don't feel
> strongly about what the patch is trying to do, but it's not a problem
> area I know a lot about or use myself. So if it's doing something useful
> for you, that sounds reasonable to me. :)
Thanks Jeff for the review
>
> -Peff
next prev parent reply other threads:[~2023-04-25 19:20 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-14 6:46 [PATCH] credential: new attribute oauth_refresh_token M Hickford via GitGitGadget
2023-04-21 9:47 ` [PATCH v2] " M Hickford via GitGitGadget
2023-04-21 16:50 ` Junio C Hamano
2023-04-24 20:11 ` M Hickford
2023-04-25 6:47 ` Jeff King
2023-04-25 19:19 ` M Hickford [this message]
2023-05-02 10:25 ` Jeff King
2023-05-02 16:19 ` Felipe Contreras
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=CAGJzqsmAESKYd5AYoZ17pg0F9Oe2sK_Odwpk4niQK9njtpCkTg@mail.gmail.com \
--to=mirth.hickford@gmail.com \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=lessleydennington@gmail.com \
--cc=martin.agren@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 read-only IMAP folder(s) and NNTP newsgroup(s).