Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

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