From: Aaron Plattner <aplattner@nvidia.com>
Date: Wed, 05 Jun 2024 16:45:32 GMT
Subject: Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing
Message-ID: <dcbbd00f-1730-41fd-90d3-c7b070c4f17d@nvidia.com>
In-Reply-To: <20240605085733.GE2345232@coredump.intra.peff.net>

On 6/5/24 1:57 AM, Jeff King wrote:
> 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 comment above credential_reject() mentions that it is "readying the 
credential for another call to `credential_fill`" which does imply that 
you can use it again right away without having to fill in the protocol / 
host / path fields. So you're probably right that this should remain the 
way it was.

> The rest of the patch made sense to me, though. As would using
> credential_clear_secrets() here to replace the equivalent lines.

That's certainly fine with me. Using credential_clear_secrets() to just 
replace those two lines would definitely keep the original behavior of 
this code.

I'll send a v3 patch to do that.

-- Aaron

> 
> -Peff