On 6/4/24 3:04 PM, Junio C Hamano wrote:
Aaron Plattner <aplattner@xxxxxxxxxx> 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.
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.
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. :)
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
Signed-off-by: Aaron Plattner <aplattner@xxxxxxxxxx>
---
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,
