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@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.
>
> 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@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,
