{"thread":{"id":"61599","subject":"[PATCH v3] credential: clear expired c->credential, unify secret clearing","startedAt":"2024-06-06T18:36:11Z","lastAt":"2024-06-08T11:32:33Z","messageCount":3,"participants":["Aaron Plattner","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"496556","messageId":"20240606183516.4077896-2-aplattner@nvidia.com","threadId":"61599","inReplyTo":null,"subject":"[PATCH v3] credential: clear expired c->credential, unify secret clearing","fromName":"Aaron Plattner","fromEmail":"aplattner@nvidia.com","sentAt":"2024-06-06T18:35:16Z","receivedAt":"2024-06-06T18:36: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.\n\nUpdate comments in credential.h to mention the new struct fields.\n\nSigned-off-by: Aaron Plattner <aplattner@nvidia.com>\n---\nv3: I reverted the behavior change to credential_reject() and just unified\neverything to use credential_clear_secrets() instead. We can rework\ncredential_reject() in a later change if we decide to. So the only behavior\nchange now should be the expiration case in credential_fill()\n\nI also updated some of the comments in credential.h to mention the new struct\nfields.\n\nThanks for your patience with this series, everyone!\n\n credential.c | 16 ++++++++++------\n credential.h | 34 ++++++++++++++++++----------------\n 2 files changed, 28 insertions(+), 22 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 758528b291..4b1a2b94fe 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,15 @@ 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\n+\t\t\t * 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,9 +533,8 @@ 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+\tcredential_clear_secrets(c);\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;\ndiff --git a/credential.h b/credential.h\nindex af8c287ff2..5f9e6ff2ef 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -5,8 +5,8 @@\n #include \"strvec.h\"\n \n /**\n- * The credentials API provides an abstracted way of gathering username and\n- * password credentials from the user.\n+ * The credentials API provides an abstracted way of gathering\n+ * authentication credentials from the user.\n  *\n  * Typical setup\n  * -------------\n@@ -116,11 +116,12 @@ struct credential_capability {\n };\n \n /**\n- * This struct represents a single username/password combination\n- * along with any associated context. All string fields should be\n- * heap-allocated (or NULL if they are not known or not applicable).\n- * The meaning of the individual context fields is the same as\n- * their counterparts in the helper protocol.\n+ * This struct represents a single login credential (typically a\n+ * username/password combination) along with any associated\n+ * context. All string fields should be heap-allocated (or NULL if\n+ * they are not known or not applicable). The meaning of the\n+ * individual context fields is the same as their counterparts in\n+ * the helper protocol.\n  *\n  * This struct should always be initialized with `CREDENTIAL_INIT` or\n  * `credential_init`.\n@@ -207,11 +208,12 @@ void credential_clear(struct credential *);\n \n /**\n  * Instruct the credential subsystem to fill the username and\n- * password fields of the passed credential struct by first\n- * consulting helpers, then asking the user. After this function\n- * returns, the username and password fields of the credential are\n- * guaranteed to be non-NULL. If an error occurs, the function will\n- * die().\n+ * password (or authtype and credential) fields of the passed\n+ * credential struct by first consulting helpers, then asking the\n+ * user. After this function returns, either the username and\n+ * password fields or the credential field of the credential are\n+ * guaranteed to be non-NULL. If an error occurs, the function\n+ * will die().\n  *\n  * If all_capabilities is set, this is an internal user that is prepared\n  * to deal with all known capabilities, and we should advertise that fact.\n@@ -232,10 +234,10 @@ void credential_approve(struct credential *);\n  * have been rejected. This will cause the credential subsystem to\n  * notify any helpers of the rejection (which allows them, for\n  * example, to purge the invalid credentials from storage). It\n- * will also free() the username and password fields of the\n- * credential and set them to NULL (readying the credential for\n- * another call to `credential_fill`). Any errors from helpers are\n- * ignored.\n+ * will also free() the username, password, and credential fields\n+ * of the credential and set them to NULL (readying the credential\n+ * for another call to `credential_fill`). Any errors from helpers\n+ * are ignored.\n  */\n void credential_reject(struct credential *);\n \n-- \n2.45.2.409.g7b0defb391\n\n"},{"id":"496559","messageId":"xmqqjzj1lxlm.fsf@gitster.g","threadId":"61599","inReplyTo":"20240606183516.4077896-2-aplattner@nvidia.com","subject":"Re: [PATCH v3] credential: clear expired c->credential, unify secret clearing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-06T18:44:53Z","receivedAt":"2024-06-06T18:44:55Z","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> v3: I reverted the behavior change to credential_reject() and just unified\n> everything to use credential_clear_secrets() instead. We can rework\n> credential_reject() in a later change if we decide to. So the only behavior\n> change now should be the expiration case in credential_fill()\n\nLooks good.\n\n>\n> I also updated some of the comments in credential.h to mention the new struct\n> fields.\n\nThanks for paying attention to such details.  I very much like these\nupdated comments.\n\nWill queue.\n"},{"id":"496711","messageId":"20240608113231.GD2966571@coredump.intra.peff.net","threadId":"61599","inReplyTo":"20240606183516.4077896-2-aplattner@nvidia.com","subject":"Re: [PATCH v3] credential: clear expired c->credential, unify secret clearing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T11:32:31Z","receivedAt":"2024-06-08T11:32:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 06, 2024 at 11:35:16AM -0700, Aaron Plattner wrote:\n\n> v3: I reverted the behavior change to credential_reject() and just unified\n> everything to use credential_clear_secrets() instead. We can rework\n> credential_reject() in a later change if we decide to. So the only behavior\n> change now should be the expiration case in credential_fill()\n> \n> I also updated some of the comments in credential.h to mention the new struct\n> fields.\n\nThanks, this one looks great to me.\n\n> Thanks for your patience with this series, everyone!\n\nLikewise!\n\n-Peff\n"}]}