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 <redacted>
---
credential.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
From: brian m. carlson <hidden> Date: 2024-06-04 21:21:10
On 2024-06-04 at 19:29:28, Aaron Plattner wrote:
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.
I think this is fine. I'm assuming that the credential (and other
appurtenant information, such as the state[] values) are still passed to
the erase call, and if so, I don't see a problem.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
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.
-Peff
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
From: Jeff King <hidden> Date: 2024-06-06 08:08:21
On Wed, Jun 05, 2024 at 09:45:32AM -0700, Aaron Plattner wrote:
quoted
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.
Heh, OK. I was the one who wrote that comment originally, which I guess
is why it was in the back of my mind. ;)
As I said, clearing "username" is a little questionable there. But it
also gives the user a chance to update the field, so maybe it's not so
bad. There might be other fields in the same boat, but I think you'd
really have to think about each one. I'm content to leave the code as it
is for now, and if somebody comes up with a case where reject+fill
doesn't behave as they expect, we can think about it further.
quoted
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.