On Tue, 04 Jun, 2024 15:19:14 -0700 Aaron Plattner <aplattner@xxxxxxxxxx> wrote: > 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 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 > >>> 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, -- Thanks, Rahul Rameshbabu
