From 27db485c34392df4fe6fbaf57a43f15bd7bf4a36 Mon Sep 17 00:00:00 2001 From: Aaron Plattner Date: Thu, 6 Jun 2024 11:35:16 -0700 Subject: [PATCH] credential: clear expired c->credential, unify secret clearing 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:///repository' And on the server: [auth_openidc:error] [client :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. Update comments in credential.h to mention the new struct fields. Signed-off-by: Aaron Plattner Signed-off-by: Junio C Hamano --- credential.c | 16 ++++++++++------ credential.h | 34 ++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/credential.c b/credential.c index 758528b291..4b1a2b94fe 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,15 @@ 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,9 +533,8 @@ void credential_reject(struct credential *c) for (i = 0; i < c->helpers.nr; i++) credential_do(c, c->helpers.items[i].string, "erase"); + credential_clear_secrets(c); 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; diff --git a/credential.h b/credential.h index af8c287ff2..5f9e6ff2ef 100644 --- a/credential.h +++ b/credential.h @@ -5,8 +5,8 @@ #include "strvec.h" /** - * The credentials API provides an abstracted way of gathering username and - * password credentials from the user. + * The credentials API provides an abstracted way of gathering + * authentication credentials from the user. * * Typical setup * ------------- @@ -116,11 +116,12 @@ struct credential_capability { }; /** - * This struct represents a single username/password combination - * along with any associated context. All string fields should be - * heap-allocated (or NULL if they are not known or not applicable). - * The meaning of the individual context fields is the same as - * their counterparts in the helper protocol. + * This struct represents a single login credential (typically a + * username/password combination) along with any associated + * context. All string fields should be heap-allocated (or NULL if + * they are not known or not applicable). The meaning of the + * individual context fields is the same as their counterparts in + * the helper protocol. * * This struct should always be initialized with `CREDENTIAL_INIT` or * `credential_init`. @@ -207,11 +208,12 @@ void credential_clear(struct credential *); /** * Instruct the credential subsystem to fill the username and - * password fields of the passed credential struct by first - * consulting helpers, then asking the user. After this function - * returns, the username and password fields of the credential are - * guaranteed to be non-NULL. If an error occurs, the function will - * die(). + * password (or authtype and credential) fields of the passed + * credential struct by first consulting helpers, then asking the + * user. After this function returns, either the username and + * password fields or the credential field of the credential are + * guaranteed to be non-NULL. If an error occurs, the function + * will die(). * * If all_capabilities is set, this is an internal user that is prepared * to deal with all known capabilities, and we should advertise that fact. @@ -232,10 +234,10 @@ void credential_approve(struct credential *); * have been rejected. This will cause the credential subsystem to * notify any helpers of the rejection (which allows them, for * example, to purge the invalid credentials from storage). It - * will also free() the username and password fields of the - * credential and set them to NULL (readying the credential for - * another call to `credential_fill`). Any errors from helpers are - * ignored. + * will also free() the username, password, and credential fields + * of the credential and set them to NULL (readying the credential + * for another call to `credential_fill`). Any errors from helpers + * are ignored. */ void credential_reject(struct credential *);