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

On 6/5/24 1:57 AM, Jeff King wrote:

On Tue, Jun 04, 2024 at 12:29:28PM -0700, Aaron Plattner wrote:

quoted

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