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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 5, 2024, 17:06 UTC
Message-ID: <xmqqtti7tj32.fsf@gitster.g>
In-Reply-To: <20240605085733.GE2345232@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 53 quoted lines

> On Tue, Jun 04, 2024 at 12:29:28PM -0700, Aaron Plattner wrote:
>
>> @@ -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);
>>  }
>
> I'm skeptical of this hunk. The caller will usually have filled in parts
> of a credential struct like scheme and host, and then we picked up the
> rest from helpers or by prompting the user. Rejecting the credential
> should certainly clear the bogus password field and other secrets. But
> should it clear the host field?
>
> I think it may be somewhat academic for now because we'll generally exit
> the program immediately after rejecting the credential. But occasionally
> the topic comes up of retrying auth within a command. So you might have
> a loop like this (or knowing our http code, probably some more baroque
> equivalent spread across multiple functions):
>
>   credential_from_url(&cred, url);
>   for (int attempt = 0; attempt < 5; attempt++) {
> 	credential_fill(&cred);
> 	switch (do_something(url, &cred)) {
> 	case OK: /* it worked */
> 		return 0;
> 	case AUTH_ERROR:
> 		/* try again */
> 		credential_reject(&cred);
> 	}
>   }
>   return -1; /* too many failures */
>
> And in that case you really want to retain the "query" parts of the
> credential after the reject. In this toy example you could just move the
> url-to-cred parsing into the loop, but in the real world it's often more
> complicated.
>
> Arguably even the original code is a bit questionable for this, because
> we don't know if the username came from a helper or from the user, or if
> it was part of the original URL (e.g., "https://user@example.com/"
> should prompt only for the password). But it feels like this hunk is
> making it worse.
>
> The rest of the patch made sense to me, though. As would using
> credential_clear_secrets() here to replace the equivalent lines.

So we have clear() that is to "clear everything", clear_secret() that is to "clear auth material", but we would want another "clear every members other than used as query keys" level?

That way, anytime we add different kind of "auth material" (like brian's series did), existing code paths that call clear_secret() do not have to change, and if we add different kind of "query keys", the reject code would not have to change? Or is the reject code path the only thing that cares about what members are used as query keys, in which case we do not need the third helper?

Thanks.