threads / patch / 61584

patchcredential: clear expired c->credential in addition to c->password

Subject: [PATCH] credential: clear expired c->credential in addition to c->password

## tl;dr

3 messages between Jun 4, 2024 and Jun 4, 2024. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Aaron Plattner· Jun 4, 2024, 18:02 UTC · lore

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. Clear that too.

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

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
---
 credential.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to credential.c +2 −1

diff --git a/credential.c b/credential.c
index 758528b291..38b51e11cb 100644
--- a/credential.c
+++ b/credential.c
@@ -480,8 +480,9 @@ 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 */
+			/* Discard expired credentials */
 			FREE_AND_NULL(c->password);
+			FREE_AND_NULL(c->credential);
 			/* Reset expiry to maintain consistency */
 			c->password_expiry_utc = TIME_MAX;
 		}

-- 
2.45.2.409.g7b0defb391

Junio C Hamano· Jun 4, 2024, 18:24 UTC · re: Aaron Plattner · lore

Re: [PATCH] credential: clear expired c->credential in addition to c->password

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. Clear that too.

Hmph, piling another thing on top of these selected "discard/reset" we already have should make us rethink a few things.

 - Is this the only place we discard/reset/clear?

 - Isn't there already a helper function that was DESIGNED to do
   this for us?

 - Are all these places we discard/reset/clear using that helper
   function?

For example, when we rejecting credential, shouldn't we be clearing the same members of the structure as we notice that the auth material is stale and has expired?

There is credential_clear() and credential_clear_secrets(). Would one of these want to be reused in this (and also reject) context?

Aaron Plattner· Jun 4, 2024, 18:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] credential: clear expired c->credential in addition to c->password

On 6/4/24 11:24 AM, Junio C Hamano wrote:

Show 24 quoted lines

> 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. Clear that too.
> 
> Hmph, piling another thing on top of these selected "discard/reset"
> we already have should make us rethink a few things.
> 
>   - Is this the only place we discard/reset/clear?
> 
>   - Isn't there already a helper function that was DESIGNED to do
>     this for us?
> 
>   - Are all these places we discard/reset/clear using that helper
>     function?
> 
> For example, when we rejecting credential, shouldn't we be clearing
> the same members of the structure as we notice that the auth material
> is stale and has expired?
> 
> There is credential_clear() and credential_clear_secrets().  Would
> one of these want to be reused in this (and also reject) context?

Good questions.

As far as I can tell, credential_clear() is for when we're done with a struct credential completely and want to reuse that memory for something. credential_clear_secrets() is used when we just want to reject the secret part of the struct cred but reuse the rest of the fields. I'll go through and see if I can determine which is which and send a patch to unify some of these.

-- Aaron