Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 4, 2024, 22:04 UTC
- Message-ID
- <xmqqo78gtldz.fsf@gitster.g>
- In-Reply-To
- <20240604192929.3252626-1-aplattner@nvidia.com>
Aaron Plattner <aplattner@nvidia.com> writes:
Show 23 quoted lines
> 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).
Will queue. Thanks.
Show 54 quoted lines
> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
> ---
> 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,