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

On Tue, 04 Jun, 2024 15:19:14 -0700 Aaron Plattner [off-list ref] wrote:

On 6/4/24 3:04 PM, Junio C Hamano wrote:

quoted

Aaron Plattner [off-list ref] writes:

quoted

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, and use
the full credential_clear() in credential_reject() after the credential
has been erased from all of the helpers.

OK.

quoted

v2: Unify secret clearing into credential_clear_secrets(), use
credential_clear() in credential_reject(), add a comment about why we
can't use credential_clear() in credential_fill().

This does not belong to the commit log message proper.  Those who
are reading "git log" would not know (and care about) an earlier
attempt.  Writing the changes since the previous round(s) like the
above paragraph is very much appreciated as it helps reviewers who
saw them, but please do so after the three-dash "---" line below (I
can remove the paragraph while queueing, so no need to resend).

Sorry, I wasn't sure what the convention was since this was my first patch to
the list. Thanks for fixing it. You can feel free to properly wrap the last line
of that comment that I missed too, if you if you like. :)

quoted

Will queue.  Thanks.

Thanks!

Rahul (CC'd) and I had a series of patches to add something similar to the
current authtype system but hadn't gotten around to sending them to the list
before this more flexible mechanism was merged. It's nice that this worked out
of the box with minimal adjustment.

The credential helper he wrote is specific to the Microsoft "Entra ID" identity
provider system, but hopefully it'll be generally useful once this stuff is in a
git release. It really cleans up the authentication process over https for sites
that support it.

Aaron made a commit to make it work with the authtype/credential
credential-helper infrastructure that landed in git-next.

  https://github.com/Binary-Eater/git-credential-msal/commit/f71ca9c72ca1a2cf73373de76909f6007ac689cb

The support for authtype excites me since a number of large Git
providers like GitHub/GitLab/etc. have utilized Authorization Basic
incorrectly for supporting different authtypes with git previously.
Hoping they will move away from this practice in the future with this
enhancement.

-- Aaron

quoted

quoted

Signed-off-by: Aaron Plattner <redacted>
---
  credential.c | 19 +++++++++----------
  1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/credential.c b/credential.c
index 758528b291..72c6f46b02 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,14 @@ 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,12 +532,7 @@ void credential_reject(struct credential *c)
  	for (i = 0; i < c->helpers.nr; i++)
  		credential_do(c, c->helpers.items[i].string, "erase");
  -	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;
+	credential_clear(c);
  }
    static int check_url_component(const char *url, int quiet,

-- 
Thanks,

Rahul Rameshbabu