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

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

[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.

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.

Update comments in credential.h to mention the new struct fields.

Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
---
v3: I reverted the behavior change to credential_reject() and just unified
everything to use credential_clear_secrets() instead. We can rework
credential_reject() in a later change if we decide to. So the only behavior
change now should be the expiration case in credential_fill()

I also updated some of the comments in credential.h to mention the new struct
fields.

Thanks for your patience with this series, everyone!

 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 *);
 
-- 
2.45.2.409.g7b0defb391

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

[Junio C Hamano]

Aaron Plattner <aplattner@nvidia.com> writes:

> v3: I reverted the behavior change to credential_reject() and just unified
> everything to use credential_clear_secrets() instead. We can rework
> credential_reject() in a later change if we decide to. So the only behavior
> change now should be the expiration case in credential_fill()

Looks good.

>
> I also updated some of the comments in credential.h to mention the new struct
> fields.

Thanks for paying attention to such details.  I very much like these
updated comments.

Will queue.

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

[Jeff King]

On Thu, Jun 06, 2024 at 11:35:16AM -0700, Aaron Plattner wrote:

> v3: I reverted the behavior change to credential_reject() and just unified
> everything to use credential_clear_secrets() instead. We can rework
> credential_reject() in a later change if we decide to. So the only behavior
> change now should be the expiration case in credential_fill()
> 
> I also updated some of the comments in credential.h to mention the new struct
> fields.

Thanks, this one looks great to me.

> Thanks for your patience with this series, everyone!

Likewise!

-Peff