From: Rahul Rameshbabu <rrameshbabu@nvidia•com>
To: Aaron Plattner <aplattner@nvidia•com>
Cc: Junio C Hamano <gitster@pobox•com>, git@vger•kernel.org
Subject: Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing
Date: Tue, 04 Jun 2024 15:28:53 -0700 [thread overview]
Message-ID: <87y17kbavu.fsf@nvidia.com> (raw)
In-Reply-To: <4521d5ab-c0e8-44d5-90aa-72555681219f@nvidia.com> (Aaron Plattner's message of "Tue, 4 Jun 2024 15:19:14 -0700")
On Tue, 04 Jun, 2024 15:19:14 -0700 Aaron Plattner <aplattner@nvidia•com> wrote:
> On 6/4/24 3:04 PM, Junio C Hamano wrote:
>> Aaron Plattner <aplattner@nvidia•com> writes:
>>
>>> When a struct credential expires, credential_fill() clears c->password
>>> so that clients don't try to use it later. However, a struct cred that
>>> uses an alternate authtype won't have a password, but might have a
>>> credential stored in c->credential.
>>>
>>> This is a problem, for example, when an OAuth2 bearer token is used. In
>>> the system I'm using, the OAuth2 configuration generates and caches a
>>> bearer token that is valid for an hour. After the token expires, git
>>> needs to call back into the credential helper to use a stored refresh
>>> token to get a new bearer token. But if c->credential is still non-NULL,
>>> git will instead try to use the expired token and fail with an error:
>>>
>>> fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'
>>>
>>> And on the server:
>>>
>>> [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: "exp" validation failure (1717522989): JWT expired 224 seconds ago
>>>
>>> Fix this by clearing both c->password and c->credential for an expired
>>> struct credential. While we're at it, use credential_clear_secrets()
>>> wherever both c->password and c->credential are being cleared, and use
>>> the full credential_clear() in credential_reject() after the credential
>>> has been erased from all of the helpers.
>> OK.
>>
>>>
>>> v2: Unify secret clearing into credential_clear_secrets(), use
>>> credential_clear() in credential_reject(), add a comment about why we
>>> can't use credential_clear() in credential_fill().
>> This does not belong to the commit log message proper. Those who
>> are reading "git log" would not know (and care about) an earlier
>> attempt. Writing the changes since the previous round(s) like the
>> above paragraph is very much appreciated as it helps reviewers who
>> saw them, but please do so after the three-dash "---" line below (I
>> can remove the paragraph while queueing, so no need to resend).
>
> Sorry, I wasn't sure what the convention was since this was my first patch to
> the list. Thanks for fixing it. You can feel free to properly wrap the last line
> of that comment that I missed too, if you if you like. :)
>
>> Will queue. Thanks.
>
> Thanks!
>
> Rahul (CC'd) and I had a series of patches to add something similar to the
> current authtype system but hadn't gotten around to sending them to the list
> before this more flexible mechanism was merged. It's nice that this worked out
> of the box with minimal adjustment.
>
> The credential helper he wrote is specific to the Microsoft "Entra ID" identity
> provider system, but hopefully it'll be generally useful once this stuff is in a
> git release. It really cleans up the authentication process over https for sites
> that support it.
Aaron made a commit to make it work with the authtype/credential
credential-helper infrastructure that landed in git-next.
https://github.com/Binary-Eater/git-credential-msal/commit/f71ca9c72ca1a2cf73373de76909f6007ac689cb
The support for authtype excites me since a number of large Git
providers like GitHub/GitLab/etc. have utilized Authorization Basic
incorrectly for supporting different authtypes with git previously.
Hoping they will move away from this practice in the future with this
enhancement.
>
> -- Aaron
>
>>> Signed-off-by: Aaron Plattner <aplattner@nvidia•com>
>>> ---
>>> credential.c | 19 +++++++++----------
>>> 1 file changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/credential.c b/credential.c
>>> index 758528b291..72c6f46b02 100644
>>> --- a/credential.c
>>> +++ b/credential.c
>>> @@ -20,12 +20,11 @@ void credential_init(struct credential *c)
>>> void credential_clear(struct credential *c)
>>> {
>>> + credential_clear_secrets(c);
>>> free(c->protocol);
>>> free(c->host);
>>> free(c->path);
>>> free(c->username);
>>> - free(c->password);
>>> - free(c->credential);
>>> free(c->oauth_refresh_token);
>>> free(c->authtype);
>>> string_list_clear(&c->helpers, 0);
>>> @@ -479,9 +478,14 @@ void credential_fill(struct credential *c, int all_capabilities)
>>> for (i = 0; i < c->helpers.nr; i++) {
>>> credential_do(c, c->helpers.items[i].string, "get");
>>> +
>>> if (c->password_expiry_utc < time(NULL)) {
>>> - /* Discard expired password */
>>> - FREE_AND_NULL(c->password);
>>> + /*
>>> + * Don't use credential_clear() here: callers such as
>>> + * cmd_credential() expect to still be able to call
>>> + * credential_write() on a struct credential whose secrets have expired.
>>> + */
>>> + credential_clear_secrets(c);
>>> /* Reset expiry to maintain consistency */
>>> c->password_expiry_utc = TIME_MAX;
>>> }
>>> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)
>>> for (i = 0; i < c->helpers.nr; i++)
>>> credential_do(c, c->helpers.items[i].string, "erase");
>>> - FREE_AND_NULL(c->username);
>>> - FREE_AND_NULL(c->password);
>>> - FREE_AND_NULL(c->credential);
>>> - FREE_AND_NULL(c->oauth_refresh_token);
>>> - c->password_expiry_utc = TIME_MAX;
>>> - c->approved = 0;
>>> + credential_clear(c);
>>> }
>>> static int check_url_component(const char *url, int quiet,
--
Thanks,
Rahul Rameshbabu
next prev parent reply other threads:[~2024-06-04 22:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-04 19:29 [PATCH v2] credential: clear expired c->credential, unify secret clearing Aaron Plattner
2024-06-04 20:51 ` Junio C Hamano
2024-06-04 21:21 ` brian m. carlson
2024-06-04 22:04 ` Junio C Hamano
2024-06-04 22:19 ` Aaron Plattner
2024-06-04 22:28 ` Rahul Rameshbabu [this message]
2024-06-05 5:12 ` Junio C Hamano
2024-06-05 8:57 ` Jeff King
2024-06-05 16:45 ` Aaron Plattner
2024-06-06 8:08 ` Jeff King
2024-06-05 17:06 ` Junio C Hamano
2024-06-06 8:10 ` Jeff King
2024-06-06 15:13 ` Junio C Hamano
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=87y17kbavu.fsf@nvidia.com \
--to=rrameshbabu@nvidia$(echo .)com \
--cc=aplattner@nvidia$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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