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

[Junio C Hamano <gitster@pobox.com>]

Brian, on top of your topic that was merged last month c5c9acf7
(Merge branch 'bc/credential-scheme-enhancement', 2024-05-08), do
these changes make sense to you as a fix/clean-up?

Thanks.

Aaron Plattner <aplattner@nvidia.com> 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.
>
> 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().
>
> 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,