| Message ID | 20240604180224.1484537-1-aplattner@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential: clear expired c->credential in addition to c->password | expand |
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?
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
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; }
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(-)