{"thread":{"id":"61584","subject":"[PATCH] credential: clear expired c->credential in addition to c->password","startedAt":"2024-06-04T18:03:50Z","lastAt":"2024-06-04T18:51:45Z","messageCount":3,"participants":["Aaron Plattner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496301","messageId":"20240604180224.1484537-1-aplattner@nvidia.com","threadId":"61584","inReplyTo":null,"subject":"[PATCH] credential: clear expired c->credential in addition to c->password","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-04T18:02:20Z","receivedAt":"2024-06-04T18:03:50Z","isPatch":true,"sender":{"key":"aplattner@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/343551?v=4"},"body":"When a struct credential expires, credential_fill() clears c->password\nso that clients don't try to use it later. However, a struct cred that\nuses an alternate authtype won't have a password, but might have a\ncredential stored in c->credential. Clear that too.\n\nThis is a problem, for example, when an OAuth2 bearer token is used. In\nthe system I'm using, the OAuth2 configuration generates and caches a\nbearer token that is valid for an hour. After the token expires, git\nneeds to call back into the credential helper to use a stored refresh\ntoken to get a new bearer token. But if c->credential is still non-NULL,\ngit will instead try to use the expired token and fail with an error:\n\n fatal: Authentication failed for 'https:///repository'\n\nAnd on the server:\n\n [auth_openidc:error] [client :34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n\nSigned-off-by: Aaron Plattner \n---\n credential.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 758528b291..38b51e11cb 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -480,8 +480,9 @@ void credential_fill(struct credential *c, int all_capabilities)\n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n \t\tif (c->password_expiry_utc < time(NULL)) {\n-\t\t\t/* Discard expired password */\n+\t\t\t/* Discard expired credentials */\n \t\t\tFREE_AND_NULL(c->password);\n+\t\t\tFREE_AND_NULL(c->credential);\n \t\t\t/* Reset expiry to maintain consistency */\n \t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t}\n-- \n2.45.2.409.g7b0defb391\n\n"},{"id":"496303","messageId":"xmqqed9cva5s.fsf@gitster.g","threadId":"61584","inReplyTo":"20240604180224.1484537-1-aplattner@nvidia.com","subject":"Re: [PATCH] credential: clear expired c->credential in addition to c->password","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-04T18:24:15Z","receivedAt":"2024-06-04T18:24:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Plattner writes:\n\n> When a struct credential expires, credential_fill() clears c->password\n> so that clients don't try to use it later. However, a struct cred that\n> uses an alternate authtype won't have a password, but might have a\n> credential stored in c->credential. Clear that too.\n\nHmph, piling another thing on top of these selected \"discard/reset\"\nwe already have should make us rethink a few things.\n\n - Is this the only place we discard/reset/clear?\n\n - Isn't there already a helper function that was DESIGNED to do\n this for us?\n\n - Are all these places we discard/reset/clear using that helper\n function?\n\nFor example, when we rejecting credential, shouldn't we be clearing\nthe same members of the structure as we notice that the auth material\nis stale and has expired?\n\nThere is credential_clear() and credential_clear_secrets(). Would\none of these want to be reused in this (and also reject) context?\n"},{"id":"496306","messageId":"4639c4de-9915-4e3c-9c5f-9c55cfd46637@nvidia.com","threadId":"61584","inReplyTo":"xmqqed9cva5s.fsf@gitster.g","subject":"Re: [PATCH] credential: clear expired c->credential in addition to c->password","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-04T18:51:40Z","receivedAt":"2024-06-04T18:51:45Z","isPatch":true,"sender":{"key":"aplattner@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/343551?v=4"},"body":"On 6/4/24 11:24 AM, Junio C Hamano wrote:\n> Aaron Plattner writes:\n> \n>> When a struct credential expires, credential_fill() clears c->password\n>> so that clients don't try to use it later. However, a struct cred that\n>> uses an alternate authtype won't have a password, but might have a\n>> credential stored in c->credential. Clear that too.\n> \n> Hmph, piling another thing on top of these selected \"discard/reset\"\n> we already have should make us rethink a few things.\n> \n> - Is this the only place we discard/reset/clear?\n> \n> - Isn't there already a helper function that was DESIGNED to do\n> this for us?\n> \n> - Are all these places we discard/reset/clear using that helper\n> function?\n> \n> For example, when we rejecting credential, shouldn't we be clearing\n> the same members of the structure as we notice that the auth material\n> is stale and has expired?\n> \n> There is credential_clear() and credential_clear_secrets(). Would\n> one of these want to be reused in this (and also reject) context?\n\nGood questions.\n\nAs far as I can tell, credential_clear() is for when we're done with a \nstruct credential completely and want to reuse that memory for \nsomething. credential_clear_secrets() is used when we just want to \nreject the secret part of the struct cred but reuse the rest of the \nfields. I'll go through and see if I can determine which is which and \nsend a patch to unify some of these.\n\n-- Aaron\n"}]}