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

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

[Aaron Plattner]

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(-)

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

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

[Junio C Hamano]

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?

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

[Aaron Plattner]

On 6/4/24 11:24 AM, 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. 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