{"thread":{"id":"61585","subject":"[PATCH v2] credential: clear expired c->credential, unify secret clearing","startedAt":"2024-06-04T19:30:11Z","lastAt":"2024-06-06T15:13:51Z","messageCount":13,"participants":["Aaron Plattner","Junio C Hamano","brian m. carlson","Rahul Rameshbabu","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"496307","messageId":"20240604192929.3252626-1-aplattner@nvidia.com","threadId":"61585","inReplyTo":null,"subject":"[PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-04T19:29:28Z","receivedAt":"2024-06-04T19:30:11Z","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.\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://<oauth2-enabled-server>/repository'\n\nAnd on the server:\n\n [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n\nFix this by clearing both c->password and c->credential for an expired\nstruct credential. While we're at it, use credential_clear_secrets()\nwherever both c->password and c->credential are being cleared, and use\nthe full credential_clear() in credential_reject() after the credential\nhas been erased from all of the helpers.\n\nv2: Unify secret clearing into credential_clear_secrets(), use\ncredential_clear() in credential_reject(), add a comment about why we\ncan't use credential_clear() in credential_fill().\n\nSigned-off-by: Aaron Plattner <aplattner@nvidia.com>\n---\n credential.c | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 758528b291..72c6f46b02 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -20,12 +20,11 @@ void credential_init(struct credential *c)\n \n void credential_clear(struct credential *c)\n {\n+\tcredential_clear_secrets(c);\n \tfree(c->protocol);\n \tfree(c->host);\n \tfree(c->path);\n \tfree(c->username);\n-\tfree(c->password);\n-\tfree(c->credential);\n \tfree(c->oauth_refresh_token);\n \tfree(c->authtype);\n \tstring_list_clear(&c->helpers, 0);\n@@ -479,9 +478,14 @@ void credential_fill(struct credential *c, int all_capabilities)\n \n \tfor (i = 0; i < c->helpers.nr; i++) {\n \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n+\n \t\tif (c->password_expiry_utc < time(NULL)) {\n-\t\t\t/* Discard expired password */\n-\t\t\tFREE_AND_NULL(c->password);\n+\t\t\t/*\n+\t\t\t * Don't use credential_clear() here: callers such as\n+\t\t\t * cmd_credential() expect to still be able to call\n+\t\t\t * credential_write() on a struct credential whose secrets have expired.\n+\t\t\t */\n+\t\t\tcredential_clear_secrets(c);\n \t\t\t/* Reset expiry to maintain consistency */\n \t\t\tc->password_expiry_utc = TIME_MAX;\n \t\t}\n@@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n \tfor (i = 0; i < c->helpers.nr; i++)\n \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n \n-\tFREE_AND_NULL(c->username);\n-\tFREE_AND_NULL(c->password);\n-\tFREE_AND_NULL(c->credential);\n-\tFREE_AND_NULL(c->oauth_refresh_token);\n-\tc->password_expiry_utc = TIME_MAX;\n-\tc->approved = 0;\n+\tcredential_clear(c);\n }\n \n static int check_url_component(const char *url, int quiet,\n-- \n2.45.2.410.gb47f57dd90.dirty\n\n"},{"id":"496311","messageId":"xmqqtti8tos2.fsf@gitster.g","threadId":"61585","inReplyTo":"20240604192929.3252626-1-aplattner@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-04T20:51:25Z","receivedAt":"2024-06-04T20:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian, on top of your topic that was merged last month c5c9acf7\n(Merge branch 'bc/credential-scheme-enhancement', 2024-05-08), do\nthese changes make sense to you as a fix/clean-up?\n\nThanks.\n\nAaron Plattner <aplattner@nvidia.com> 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.\n>\n> This is a problem, for example, when an OAuth2 bearer token is used. In\n> the system I'm using, the OAuth2 configuration generates and caches a\n> bearer token that is valid for an hour. After the token expires, git\n> needs to call back into the credential helper to use a stored refresh\n> token to get a new bearer token. But if c->credential is still non-NULL,\n> git will instead try to use the expired token and fail with an error:\n>\n>  fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'\n>\n> And on the server:\n>\n>  [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n>\n> Fix this by clearing both c->password and c->credential for an expired\n> struct credential. While we're at it, use credential_clear_secrets()\n> wherever both c->password and c->credential are being cleared, and use\n> the full credential_clear() in credential_reject() after the credential\n> has been erased from all of the helpers.\n>\n> v2: Unify secret clearing into credential_clear_secrets(), use\n> credential_clear() in credential_reject(), add a comment about why we\n> can't use credential_clear() in credential_fill().\n>\n> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>\n> ---\n>  credential.c | 19 +++++++++----------\n>  1 file changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/credential.c b/credential.c\n> index 758528b291..72c6f46b02 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -20,12 +20,11 @@ void credential_init(struct credential *c)\n>  \n>  void credential_clear(struct credential *c)\n>  {\n> +\tcredential_clear_secrets(c);\n>  \tfree(c->protocol);\n>  \tfree(c->host);\n>  \tfree(c->path);\n>  \tfree(c->username);\n> -\tfree(c->password);\n> -\tfree(c->credential);\n>  \tfree(c->oauth_refresh_token);\n>  \tfree(c->authtype);\n>  \tstring_list_clear(&c->helpers, 0);\n> @@ -479,9 +478,14 @@ void credential_fill(struct credential *c, int all_capabilities)\n>  \n>  \tfor (i = 0; i < c->helpers.nr; i++) {\n>  \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n> +\n>  \t\tif (c->password_expiry_utc < time(NULL)) {\n> -\t\t\t/* Discard expired password */\n> -\t\t\tFREE_AND_NULL(c->password);\n> +\t\t\t/*\n> +\t\t\t * Don't use credential_clear() here: callers such as\n> +\t\t\t * cmd_credential() expect to still be able to call\n> +\t\t\t * credential_write() on a struct credential whose secrets have expired.\n> +\t\t\t */\n> +\t\t\tcredential_clear_secrets(c);\n>  \t\t\t/* Reset expiry to maintain consistency */\n>  \t\t\tc->password_expiry_utc = TIME_MAX;\n>  \t\t}\n> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>  \tfor (i = 0; i < c->helpers.nr; i++)\n>  \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>  \n> -\tFREE_AND_NULL(c->username);\n> -\tFREE_AND_NULL(c->password);\n> -\tFREE_AND_NULL(c->credential);\n> -\tFREE_AND_NULL(c->oauth_refresh_token);\n> -\tc->password_expiry_utc = TIME_MAX;\n> -\tc->approved = 0;\n> +\tcredential_clear(c);\n>  }\n>  \n>  static int check_url_component(const char *url, int quiet,\n"},{"id":"496312","messageId":"Zl-FQ3SwNKM_4x6Q@tapette.crustytoothpaste.net","threadId":"61585","inReplyTo":"20240604192929.3252626-1-aplattner@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-06-04T21:21:07Z","receivedAt":"2024-06-04T21:21:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-06-04 at 19:29:28, Aaron Plattner wrote:\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.\n> \n> This is a problem, for example, when an OAuth2 bearer token is used. In\n> the system I'm using, the OAuth2 configuration generates and caches a\n> bearer token that is valid for an hour. After the token expires, git\n> needs to call back into the credential helper to use a stored refresh\n> token to get a new bearer token. But if c->credential is still non-NULL,\n> git will instead try to use the expired token and fail with an error:\n> \n>  fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'\n> \n> And on the server:\n> \n>  [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n> \n> Fix this by clearing both c->password and c->credential for an expired\n> struct credential. While we're at it, use credential_clear_secrets()\n> wherever both c->password and c->credential are being cleared, and use\n> the full credential_clear() in credential_reject() after the credential\n> has been erased from all of the helpers.\n\nI think this is fine.  I'm assuming that the credential (and other\nappurtenant information, such as the state[] values) are still passed to\nthe erase call, and if so, I don't see a problem.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"496317","messageId":"xmqqo78gtldz.fsf@gitster.g","threadId":"61585","inReplyTo":"20240604192929.3252626-1-aplattner@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-04T22:04:40Z","receivedAt":"2024-06-04T22:04:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aaron Plattner <aplattner@nvidia.com> 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.\n>\n> This is a problem, for example, when an OAuth2 bearer token is used. In\n> the system I'm using, the OAuth2 configuration generates and caches a\n> bearer token that is valid for an hour. After the token expires, git\n> needs to call back into the credential helper to use a stored refresh\n> token to get a new bearer token. But if c->credential is still non-NULL,\n> git will instead try to use the expired token and fail with an error:\n>\n>  fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'\n>\n> And on the server:\n>\n>  [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n>\n> Fix this by clearing both c->password and c->credential for an expired\n> struct credential. While we're at it, use credential_clear_secrets()\n> wherever both c->password and c->credential are being cleared, and use\n> the full credential_clear() in credential_reject() after the credential\n> has been erased from all of the helpers.\n\nOK.\n\n>\n> v2: Unify secret clearing into credential_clear_secrets(), use\n> credential_clear() in credential_reject(), add a comment about why we\n> can't use credential_clear() in credential_fill().\n\nThis does not belong to the commit log message proper.  Those who\nare reading \"git log\" would not know (and care about) an earlier\nattempt.  Writing the changes since the previous round(s) like the\nabove paragraph is very much appreciated as it helps reviewers who\nsaw them, but please do so after the three-dash \"---\" line below (I\ncan remove the paragraph while queueing, so no need to resend).\n\nWill queue.  Thanks.\n\n\n> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>\n> ---\n>  credential.c | 19 +++++++++----------\n>  1 file changed, 9 insertions(+), 10 deletions(-)\n>\n> diff --git a/credential.c b/credential.c\n> index 758528b291..72c6f46b02 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -20,12 +20,11 @@ void credential_init(struct credential *c)\n>  \n>  void credential_clear(struct credential *c)\n>  {\n> +\tcredential_clear_secrets(c);\n>  \tfree(c->protocol);\n>  \tfree(c->host);\n>  \tfree(c->path);\n>  \tfree(c->username);\n> -\tfree(c->password);\n> -\tfree(c->credential);\n>  \tfree(c->oauth_refresh_token);\n>  \tfree(c->authtype);\n>  \tstring_list_clear(&c->helpers, 0);\n> @@ -479,9 +478,14 @@ void credential_fill(struct credential *c, int all_capabilities)\n>  \n>  \tfor (i = 0; i < c->helpers.nr; i++) {\n>  \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n> +\n>  \t\tif (c->password_expiry_utc < time(NULL)) {\n> -\t\t\t/* Discard expired password */\n> -\t\t\tFREE_AND_NULL(c->password);\n> +\t\t\t/*\n> +\t\t\t * Don't use credential_clear() here: callers such as\n> +\t\t\t * cmd_credential() expect to still be able to call\n> +\t\t\t * credential_write() on a struct credential whose secrets have expired.\n> +\t\t\t */\n> +\t\t\tcredential_clear_secrets(c);\n>  \t\t\t/* Reset expiry to maintain consistency */\n>  \t\t\tc->password_expiry_utc = TIME_MAX;\n>  \t\t}\n> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>  \tfor (i = 0; i < c->helpers.nr; i++)\n>  \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>  \n> -\tFREE_AND_NULL(c->username);\n> -\tFREE_AND_NULL(c->password);\n> -\tFREE_AND_NULL(c->credential);\n> -\tFREE_AND_NULL(c->oauth_refresh_token);\n> -\tc->password_expiry_utc = TIME_MAX;\n> -\tc->approved = 0;\n> +\tcredential_clear(c);\n>  }\n>  \n>  static int check_url_component(const char *url, int quiet,\n"},{"id":"496318","messageId":"4521d5ab-c0e8-44d5-90aa-72555681219f@nvidia.com","threadId":"61585","inReplyTo":"xmqqo78gtldz.fsf@gitster.g","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-04T22:19:14Z","receivedAt":"2024-06-04T22:19:28Z","isPatch":true,"sender":{"key":"aplattner@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/343551?v=4"},"body":"On 6/4/24 3:04 PM, Junio C Hamano wrote:\n> Aaron Plattner <aplattner@nvidia.com> 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.\n>>\n>> This is a problem, for example, when an OAuth2 bearer token is used. In\n>> the system I'm using, the OAuth2 configuration generates and caches a\n>> bearer token that is valid for an hour. After the token expires, git\n>> needs to call back into the credential helper to use a stored refresh\n>> token to get a new bearer token. But if c->credential is still non-NULL,\n>> git will instead try to use the expired token and fail with an error:\n>>\n>>   fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'\n>>\n>> And on the server:\n>>\n>>   [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n>>\n>> Fix this by clearing both c->password and c->credential for an expired\n>> struct credential. While we're at it, use credential_clear_secrets()\n>> wherever both c->password and c->credential are being cleared, and use\n>> the full credential_clear() in credential_reject() after the credential\n>> has been erased from all of the helpers.\n> \n> OK.\n> \n>>\n>> v2: Unify secret clearing into credential_clear_secrets(), use\n>> credential_clear() in credential_reject(), add a comment about why we\n>> can't use credential_clear() in credential_fill().\n> \n> This does not belong to the commit log message proper.  Those who\n> are reading \"git log\" would not know (and care about) an earlier\n> attempt.  Writing the changes since the previous round(s) like the\n> above paragraph is very much appreciated as it helps reviewers who\n> saw them, but please do so after the three-dash \"---\" line below (I\n> can remove the paragraph while queueing, so no need to resend).\n\nSorry, I wasn't sure what the convention was since this was my first \npatch to the list. Thanks for fixing it. You can feel free to properly \nwrap the last line of that comment that I missed too, if you if you like. :)\n\n> \n> Will queue.  Thanks.\n\nThanks!\n\nRahul (CC'd) and I had a series of patches to add something similar to \nthe current authtype system but hadn't gotten around to sending them to \nthe list before this more flexible mechanism was merged. It's nice that \nthis worked out of the box with minimal adjustment.\n\nThe credential helper he wrote is specific to the Microsoft \"Entra ID\" \nidentity provider system, but hopefully it'll be generally useful once \nthis stuff is in a git release. It really cleans up the authentication \nprocess over https for sites that support it.\n\n-- Aaron\n\n>> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>\n>> ---\n>>   credential.c | 19 +++++++++----------\n>>   1 file changed, 9 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/credential.c b/credential.c\n>> index 758528b291..72c6f46b02 100644\n>> --- a/credential.c\n>> +++ b/credential.c\n>> @@ -20,12 +20,11 @@ void credential_init(struct credential *c)\n>>   \n>>   void credential_clear(struct credential *c)\n>>   {\n>> +\tcredential_clear_secrets(c);\n>>   \tfree(c->protocol);\n>>   \tfree(c->host);\n>>   \tfree(c->path);\n>>   \tfree(c->username);\n>> -\tfree(c->password);\n>> -\tfree(c->credential);\n>>   \tfree(c->oauth_refresh_token);\n>>   \tfree(c->authtype);\n>>   \tstring_list_clear(&c->helpers, 0);\n>> @@ -479,9 +478,14 @@ void credential_fill(struct credential *c, int all_capabilities)\n>>   \n>>   \tfor (i = 0; i < c->helpers.nr; i++) {\n>>   \t\tcredential_do(c, c->helpers.items[i].string, \"get\");\n>> +\n>>   \t\tif (c->password_expiry_utc < time(NULL)) {\n>> -\t\t\t/* Discard expired password */\n>> -\t\t\tFREE_AND_NULL(c->password);\n>> +\t\t\t/*\n>> +\t\t\t * Don't use credential_clear() here: callers such as\n>> +\t\t\t * cmd_credential() expect to still be able to call\n>> +\t\t\t * credential_write() on a struct credential whose secrets have expired.\n>> +\t\t\t */\n>> +\t\t\tcredential_clear_secrets(c);\n>>   \t\t\t/* Reset expiry to maintain consistency */\n>>   \t\t\tc->password_expiry_utc = TIME_MAX;\n>>   \t\t}\n>> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>>   \tfor (i = 0; i < c->helpers.nr; i++)\n>>   \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>>   \n>> -\tFREE_AND_NULL(c->username);\n>> -\tFREE_AND_NULL(c->password);\n>> -\tFREE_AND_NULL(c->credential);\n>> -\tFREE_AND_NULL(c->oauth_refresh_token);\n>> -\tc->password_expiry_utc = TIME_MAX;\n>> -\tc->approved = 0;\n>> +\tcredential_clear(c);\n>>   }\n>>   \n>>   static int check_url_component(const char *url, int quiet,\n"},{"id":"496319","messageId":"87y17kbavu.fsf@nvidia.com","threadId":"61585","inReplyTo":"4521d5ab-c0e8-44d5-90aa-72555681219f@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Rahul Rameshbabu","fromEmail":"rrameshbabu@nvidia.com","sentAt":"2024-06-04T22:28:53Z","receivedAt":"2024-06-04T22:28:59Z","isPatch":true,"sender":{"key":"rrameshbabu@nvidia.com","avatar":null},"body":"On Tue, 04 Jun, 2024 15:19:14 -0700 Aaron Plattner <aplattner@nvidia.com> wrote:\n> On 6/4/24 3:04 PM, Junio C Hamano wrote:\n>> Aaron Plattner <aplattner@nvidia.com> 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.\n>>>\n>>> This is a problem, for example, when an OAuth2 bearer token is used. In\n>>> the system I'm using, the OAuth2 configuration generates and caches a\n>>> bearer token that is valid for an hour. After the token expires, git\n>>> needs to call back into the credential helper to use a stored refresh\n>>> token to get a new bearer token. But if c->credential is still non-NULL,\n>>> git will instead try to use the expired token and fail with an error:\n>>>\n>>>   fatal: Authentication failed for 'https://<oauth2-enabled-server>/repository'\n>>>\n>>> And on the server:\n>>>\n>>>   [auth_openidc:error] [client <ip>:34012] oidc_proto_validate_exp: \"exp\" validation failure (1717522989): JWT expired 224 seconds ago\n>>>\n>>> Fix this by clearing both c->password and c->credential for an expired\n>>> struct credential. While we're at it, use credential_clear_secrets()\n>>> wherever both c->password and c->credential are being cleared, and use\n>>> the full credential_clear() in credential_reject() after the credential\n>>> has been erased from all of the helpers.\n>> OK.\n>> \n>>>\n>>> v2: Unify secret clearing into credential_clear_secrets(), use\n>>> credential_clear() in credential_reject(), add a comment about why we\n>>> can't use credential_clear() in credential_fill().\n>> This does not belong to the commit log message proper.  Those who\n>> are reading \"git log\" would not know (and care about) an earlier\n>> attempt.  Writing the changes since the previous round(s) like the\n>> above paragraph is very much appreciated as it helps reviewers who\n>> saw them, but please do so after the three-dash \"---\" line below (I\n>> can remove the paragraph while queueing, so no need to resend).\n>\n> Sorry, I wasn't sure what the convention was since this was my first patch to\n> the list. Thanks for fixing it. You can feel free to properly wrap the last line\n> of that comment that I missed too, if you if you like. :)\n>\n>> Will queue.  Thanks.\n>\n> Thanks!\n>\n> Rahul (CC'd) and I had a series of patches to add something similar to the\n> current authtype system but hadn't gotten around to sending them to the list\n> before this more flexible mechanism was merged. It's nice that this worked out\n> of the box with minimal adjustment.\n>\n> The credential helper he wrote is specific to the Microsoft \"Entra ID\" identity\n> provider system, but hopefully it'll be generally useful once this stuff is in a\n> git release. It really cleans up the authentication process over https for sites\n> that support it.\n\nAaron made a commit to make it work with the authtype/credential\ncredential-helper infrastructure that landed in git-next.\n\n  https://github.com/Binary-Eater/git-credential-msal/commit/f71ca9c72ca1a2cf73373de76909f6007ac689cb\n\nThe support for authtype excites me since a number of large Git\nproviders like GitHub/GitLab/etc. have utilized Authorization Basic\nincorrectly for supporting different authtypes with git previously.\nHoping they will move away from this practice in the future with this\nenhancement.\n\n>\n> -- Aaron\n>\n>>> Signed-off-by: Aaron Plattner <aplattner@nvidia.com>\n>>> ---\n>>>   credential.c | 19 +++++++++----------\n>>>   1 file changed, 9 insertions(+), 10 deletions(-)\n>>>\n>>> diff --git a/credential.c b/credential.c\n>>> index 758528b291..72c6f46b02 100644\n>>> --- a/credential.c\n>>> +++ b/credential.c\n>>> @@ -20,12 +20,11 @@ void credential_init(struct credential *c)\n>>>     void credential_clear(struct credential *c)\n>>>   {\n>>> +\tcredential_clear_secrets(c);\n>>>   \tfree(c->protocol);\n>>>   \tfree(c->host);\n>>>   \tfree(c->path);\n>>>   \tfree(c->username);\n>>> -\tfree(c->password);\n>>> -\tfree(c->credential);\n>>>   \tfree(c->oauth_refresh_token);\n>>>   \tfree(c->authtype);\n>>>   \tstring_list_clear(&c->helpers, 0);\n>>> @@ -479,9 +478,14 @@ 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>>> +\n>>>   \t\tif (c->password_expiry_utc < time(NULL)) {\n>>> -\t\t\t/* Discard expired password */\n>>> -\t\t\tFREE_AND_NULL(c->password);\n>>> +\t\t\t/*\n>>> +\t\t\t * Don't use credential_clear() here: callers such as\n>>> +\t\t\t * cmd_credential() expect to still be able to call\n>>> +\t\t\t * credential_write() on a struct credential whose secrets have expired.\n>>> +\t\t\t */\n>>> +\t\t\tcredential_clear_secrets(c);\n>>>   \t\t\t/* Reset expiry to maintain consistency */\n>>>   \t\t\tc->password_expiry_utc = TIME_MAX;\n>>>   \t\t}\n>>> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>>>   \tfor (i = 0; i < c->helpers.nr; i++)\n>>>   \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>>>   -\tFREE_AND_NULL(c->username);\n>>> -\tFREE_AND_NULL(c->password);\n>>> -\tFREE_AND_NULL(c->credential);\n>>> -\tFREE_AND_NULL(c->oauth_refresh_token);\n>>> -\tc->password_expiry_utc = TIME_MAX;\n>>> -\tc->approved = 0;\n>>> +\tcredential_clear(c);\n>>>   }\n>>>     static int check_url_component(const char *url, int quiet,\n\n-- \nThanks,\n\nRahul Rameshbabu\n"},{"id":"496320","messageId":"xmqqfrtsynu7.fsf@gitster.g","threadId":"61585","inReplyTo":"87y17kbavu.fsf@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T05:12:48Z","receivedAt":"2024-06-05T05:12:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rahul Rameshbabu <rrameshbabu@nvidia.com> writes:\n\n>>> Aaron Plattner <aplattner@nvidia.com> writes:\n>> ...\n>> Rahul (CC'd) and I had a series of patches to add something similar to the\n>> current authtype system but hadn't gotten around to sending them to the list\n>> before this more flexible mechanism was merged. It's nice that this worked out\n>> of the box with minimal adjustment.\n>>\n>> The credential helper he wrote is specific to the Microsoft \"Entra ID\" identity\n>> provider system, but hopefully it'll be generally useful once this stuff is in a\n>> git release. It really cleans up the authentication process over https for sites\n>> that support it.\n>\n> Aaron made a commit to make it work with the authtype/credential\n> credential-helper infrastructure that landed in git-next.\n>\n>   https://github.com/Binary-Eater/git-credential-msal/commit/f71ca9c72ca1a2cf73373de76909f6007ac689cb\n>\n> The support for authtype excites me since a number of large Git\n> providers like GitHub/GitLab/etc. have utilized Authorization Basic\n> incorrectly for supporting different authtypes with git previously.\n> Hoping they will move away from this practice in the future with this\n> enhancement.\n\nThanks for a huge praise that I do not personally deserve ;-)  Kudos\ngo to brian and folks who helped reviewing his topic.\n\n"},{"id":"496350","messageId":"20240605085733.GE2345232@coredump.intra.peff.net","threadId":"61585","inReplyTo":"20240604192929.3252626-1-aplattner@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-05T08:57:33Z","receivedAt":"2024-06-05T08:57:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 04, 2024 at 12:29:28PM -0700, Aaron Plattner wrote:\n\n> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>  \tfor (i = 0; i < c->helpers.nr; i++)\n>  \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>  \n> -\tFREE_AND_NULL(c->username);\n> -\tFREE_AND_NULL(c->password);\n> -\tFREE_AND_NULL(c->credential);\n> -\tFREE_AND_NULL(c->oauth_refresh_token);\n> -\tc->password_expiry_utc = TIME_MAX;\n> -\tc->approved = 0;\n> +\tcredential_clear(c);\n>  }\n\nI'm skeptical of this hunk. The caller will usually have filled in parts\nof a credential struct like scheme and host, and then we picked up the\nrest from helpers or by prompting the user. Rejecting the credential\nshould certainly clear the bogus password field and other secrets. But\nshould it clear the host field?\n\nI think it may be somewhat academic for now because we'll generally exit\nthe program immediately after rejecting the credential. But occasionally\nthe topic comes up of retrying auth within a command. So you might have\na loop like this (or knowing our http code, probably some more baroque\nequivalent spread across multiple functions):\n\n  credential_from_url(&cred, url);\n  for (int attempt = 0; attempt < 5; attempt++) {\n\tcredential_fill(&cred);\n\tswitch (do_something(url, &cred)) {\n\tcase OK: /* it worked */\n\t\treturn 0;\n\tcase AUTH_ERROR:\n\t\t/* try again */\n\t\tcredential_reject(&cred);\n\t}\n  }\n  return -1; /* too many failures */\n\nAnd in that case you really want to retain the \"query\" parts of the\ncredential after the reject. In this toy example you could just move the\nurl-to-cred parsing into the loop, but in the real world it's often more\ncomplicated.\n\nArguably even the original code is a bit questionable for this, because\nwe don't know if the username came from a helper or from the user, or if\nit was part of the original URL (e.g., \"https://user@example.com/\"\nshould prompt only for the password). But it feels like this hunk is\nmaking it worse.\n\nThe rest of the patch made sense to me, though. As would using\ncredential_clear_secrets() here to replace the equivalent lines.\n\n-Peff\n"},{"id":"496378","messageId":"dcbbd00f-1730-41fd-90d3-c7b070c4f17d@nvidia.com","threadId":"61585","inReplyTo":"20240605085733.GE2345232@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-05T16:45:32Z","receivedAt":"2024-06-05T16:45:37Z","isPatch":true,"sender":{"key":"aplattner@nvidia.com","avatar":"https://avatars.githubusercontent.com/u/343551?v=4"},"body":"On 6/5/24 1:57 AM, Jeff King wrote:\n> On Tue, Jun 04, 2024 at 12:29:28PM -0700, Aaron Plattner wrote:\n> \n>> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>>   \tfor (i = 0; i < c->helpers.nr; i++)\n>>   \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>>   \n>> -\tFREE_AND_NULL(c->username);\n>> -\tFREE_AND_NULL(c->password);\n>> -\tFREE_AND_NULL(c->credential);\n>> -\tFREE_AND_NULL(c->oauth_refresh_token);\n>> -\tc->password_expiry_utc = TIME_MAX;\n>> -\tc->approved = 0;\n>> +\tcredential_clear(c);\n>>   }\n> \n> I'm skeptical of this hunk. The caller will usually have filled in parts\n> of a credential struct like scheme and host, and then we picked up the\n> rest from helpers or by prompting the user. Rejecting the credential\n> should certainly clear the bogus password field and other secrets. But\n> should it clear the host field?\n> \n> I think it may be somewhat academic for now because we'll generally exit\n> the program immediately after rejecting the credential. But occasionally\n> the topic comes up of retrying auth within a command. So you might have\n> a loop like this (or knowing our http code, probably some more baroque\n> equivalent spread across multiple functions):\n> \n>    credential_from_url(&cred, url);\n>    for (int attempt = 0; attempt < 5; attempt++) {\n> \tcredential_fill(&cred);\n> \tswitch (do_something(url, &cred)) {\n> \tcase OK: /* it worked */\n> \t\treturn 0;\n> \tcase AUTH_ERROR:\n> \t\t/* try again */\n> \t\tcredential_reject(&cred);\n> \t}\n>    }\n>    return -1; /* too many failures */\n> \n> And in that case you really want to retain the \"query\" parts of the\n> credential after the reject. In this toy example you could just move the\n> url-to-cred parsing into the loop, but in the real world it's often more\n> complicated.\n> \n> Arguably even the original code is a bit questionable for this, because\n> we don't know if the username came from a helper or from the user, or if\n> it was part of the original URL (e.g., \"https://user@example.com/\"\n> should prompt only for the password). But it feels like this hunk is\n> making it worse.\n\nThe comment above credential_reject() mentions that it is \"readying the \ncredential for another call to `credential_fill`\" which does imply that \nyou can use it again right away without having to fill in the protocol / \nhost / path fields. So you're probably right that this should remain the \nway it was.\n\n> The rest of the patch made sense to me, though. As would using\n> credential_clear_secrets() here to replace the equivalent lines.\n\nThat's certainly fine with me. Using credential_clear_secrets() to just \nreplace those two lines would definitely keep the original behavior of \nthis code.\n\nI'll send a v3 patch to do that.\n\n-- Aaron\n\n> \n> -Peff\n"},{"id":"496383","messageId":"xmqqtti7tj32.fsf@gitster.g","threadId":"61585","inReplyTo":"20240605085733.GE2345232@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T17:06:41Z","receivedAt":"2024-06-05T17:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jun 04, 2024 at 12:29:28PM -0700, Aaron Plattner wrote:\n>\n>> @@ -528,12 +532,7 @@ void credential_reject(struct credential *c)\n>>  \tfor (i = 0; i < c->helpers.nr; i++)\n>>  \t\tcredential_do(c, c->helpers.items[i].string, \"erase\");\n>>  \n>> -\tFREE_AND_NULL(c->username);\n>> -\tFREE_AND_NULL(c->password);\n>> -\tFREE_AND_NULL(c->credential);\n>> -\tFREE_AND_NULL(c->oauth_refresh_token);\n>> -\tc->password_expiry_utc = TIME_MAX;\n>> -\tc->approved = 0;\n>> +\tcredential_clear(c);\n>>  }\n>\n> I'm skeptical of this hunk. The caller will usually have filled in parts\n> of a credential struct like scheme and host, and then we picked up the\n> rest from helpers or by prompting the user. Rejecting the credential\n> should certainly clear the bogus password field and other secrets. But\n> should it clear the host field?\n>\n> I think it may be somewhat academic for now because we'll generally exit\n> the program immediately after rejecting the credential. But occasionally\n> the topic comes up of retrying auth within a command. So you might have\n> a loop like this (or knowing our http code, probably some more baroque\n> equivalent spread across multiple functions):\n>\n>   credential_from_url(&cred, url);\n>   for (int attempt = 0; attempt < 5; attempt++) {\n> \tcredential_fill(&cred);\n> \tswitch (do_something(url, &cred)) {\n> \tcase OK: /* it worked */\n> \t\treturn 0;\n> \tcase AUTH_ERROR:\n> \t\t/* try again */\n> \t\tcredential_reject(&cred);\n> \t}\n>   }\n>   return -1; /* too many failures */\n>\n> And in that case you really want to retain the \"query\" parts of the\n> credential after the reject. In this toy example you could just move the\n> url-to-cred parsing into the loop, but in the real world it's often more\n> complicated.\n>\n> Arguably even the original code is a bit questionable for this, because\n> we don't know if the username came from a helper or from the user, or if\n> it was part of the original URL (e.g., \"https://user@example.com/\"\n> should prompt only for the password). But it feels like this hunk is\n> making it worse.\n>\n> The rest of the patch made sense to me, though. As would using\n> credential_clear_secrets() here to replace the equivalent lines.\n\nSo we have clear() that is to \"clear everything\", clear_secret()\nthat is to \"clear auth material\", but we would want another \"clear\nevery members other than used as query keys\" level?\n\nThat way, anytime we add different kind of \"auth material\" (like\nbrian's series did), existing code paths that call clear_secret() do\nnot have to change, and if we add different kind of \"query keys\",\nthe reject code would not have to change?  Or is the reject code\npath the only thing that cares about what members are used as query\nkeys, in which case we do not need the third helper?\n\nThanks.  \n"},{"id":"496457","messageId":"20240606080819.GB658959@coredump.intra.peff.net","threadId":"61585","inReplyTo":"dcbbd00f-1730-41fd-90d3-c7b070c4f17d@nvidia.com","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:08:19Z","receivedAt":"2024-06-06T08:08:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2024 at 09:45:32AM -0700, Aaron Plattner wrote:\n\n> > And in that case you really want to retain the \"query\" parts of the\n> > credential after the reject. In this toy example you could just move the\n> > url-to-cred parsing into the loop, but in the real world it's often more\n> > complicated.\n> > \n> > Arguably even the original code is a bit questionable for this, because\n> > we don't know if the username came from a helper or from the user, or if\n> > it was part of the original URL (e.g., \"https://user@example.com/\"\n> > should prompt only for the password). But it feels like this hunk is\n> > making it worse.\n> \n> The comment above credential_reject() mentions that it is \"readying the\n> credential for another call to `credential_fill`\" which does imply that you\n> can use it again right away without having to fill in the protocol / host /\n> path fields. So you're probably right that this should remain the way it\n> was.\n\nHeh, OK. I was the one who wrote that comment originally, which I guess\nis why it was in the back of my mind. ;)\n\nAs I said, clearing \"username\" is a little questionable there. But it\nalso gives the user a chance to update the field, so maybe it's not so\nbad. There might be other fields in the same boat, but I think you'd\nreally have to think about each one. I'm content to leave the code as it\nis for now, and if somebody comes up with a case where reject+fill\ndoesn't behave as they expect, we can think about it further.\n\n> > The rest of the patch made sense to me, though. As would using\n> > credential_clear_secrets() here to replace the equivalent lines.\n> \n> That's certainly fine with me. Using credential_clear_secrets() to just\n> replace those two lines would definitely keep the original behavior of this\n> code.\n> \n> I'll send a v3 patch to do that.\n\nGreat, thanks!\n\n-Peff\n"},{"id":"496458","messageId":"20240606081054.GC658959@coredump.intra.peff.net","threadId":"61585","inReplyTo":"xmqqtti7tj32.fsf@gitster.g","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-06T08:10:54Z","receivedAt":"2024-06-06T08:10:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2024 at 10:06:41AM -0700, Junio C Hamano wrote:\n\n> So we have clear() that is to \"clear everything\", clear_secret()\n> that is to \"clear auth material\", but we would want another \"clear\n> every members other than used as query keys\" level?\n> \n> That way, anytime we add different kind of \"auth material\" (like\n> brian's series did), existing code paths that call clear_secret() do\n> not have to change, and if we add different kind of \"query keys\",\n> the reject code would not have to change?  Or is the reject code\n> path the only thing that cares about what members are used as query\n> keys, in which case we do not need the third helper?\n\nI can't think of another place besides the reject path where we'd want\nthat (though I'm certainly open to being corrected if somebody finds\nsuch a spot). But mostly I am not all that confident that the set of\nitems that reject() is clearing is the best one. So I'd just as soon\nleave it as a weird internal detail for now, rather than codifying it in\na function.\n\nI dunno. I guess it is the same lines of code in either spot, but\nsomehow sticking it in a clear_response() helper seems like an\nendorsement that the author knew what they were doing. ;)\n\n-Peff\n"},{"id":"496524","messageId":"xmqqed9ap0id.fsf@gitster.g","threadId":"61585","inReplyTo":"20240606081054.GC658959@coredump.intra.peff.net","subject":"Re: [PATCH v2] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T15:13:46Z","receivedAt":"2024-06-06T15:13:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> items that reject() is clearing is the best one. So I'd just as soon\n> leave it as a weird internal detail for now, rather than codifying it in\n> a function.\n>\n> I dunno. I guess it is the same lines of code in either spot, but\n> somehow sticking it in a clear_response() helper seems like an\n> endorsement that the author knew what they were doing. ;)\n\nTrue.  It probably belongs to too premature abstraction.  Thanks.\n\n"}]}