{"thread":{"id":"59907","subject":"[PATCH] credential/libsecret: erase matching creds only","startedAt":"2023-06-24T07:21:41Z","lastAt":"2023-07-26T19:47:01Z","messageCount":5,"participants":["M Hickford via GitGitGadget","Junio C Hamano","M Hickford"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"478764","messageId":"pull.1527.git.git.1687591293705.gitgitgadget@gmail.com","threadId":"59907","inReplyTo":null,"subject":"[PATCH] credential/libsecret: erase matching creds only","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-06-24T07:21:33Z","receivedAt":"2023-06-24T07:21:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nFix test \"helper ... does not erase a password distinct from input\"\nintroduced in aeb21ce22e (credential: avoid erasing distinct password,\n2023-06-13)\n\nSigned-off-by: M Hickford \n---\n credential/libsecret: erase more carefully\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1527%2Fhickford%2Ffix-libsecret-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1527/hickford/fix-libsecret-v1\nPull-Request: https://github.com/git/git/pull/1527\n\n .../libsecret/git-credential-libsecret.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex ef681f29d5b..9110714601e 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -52,6 +52,8 @@ struct credential_operation {\n \n #define CREDENTIAL_OP_END { NULL, NULL }\n \n+static void credential_clear(struct credential *c);\n+\n /* ----------------- Secret Service functions ----------------- */\n \n static char *make_label(struct credential *c)\n@@ -185,6 +187,7 @@ static int keyring_erase(struct credential *c)\n {\n \tGHashTable *attributes = NULL;\n \tGError *error = NULL;\n+\tstruct credential existing = CREDENTIAL_INIT;\n \n \t/*\n \t * Sanity check that we actually have something to match\n@@ -197,6 +200,20 @@ static int keyring_erase(struct credential *c)\n \tif (!c->protocol && !c->host && !c->path && !c->username)\n \t\treturn EXIT_FAILURE;\n \n+\tif (c->password) {\n+\t\texisting.host = g_strdup(c->host);\n+\t\texisting.path = g_strdup(c->path);\n+\t\texisting.port = c->port;\n+\t\texisting.protocol = g_strdup(c->protocol);\n+\t\texisting.username = g_strdup(c->username);\n+\t\tkeyring_get(&existing);\n+\t\tif (existing.password && strcmp(c->password, existing.password)) {\n+\t\t\tcredential_clear(&existing);\n+\t\t\treturn EXIT_SUCCESS;\n+\t\t}\n+\t\tcredential_clear(&existing);\n+\t}\n+\n \tattributes = make_attr_list(c);\n \tsecret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,\n \t\t\t\t attributes,\n\nbase-commit: 6ff334181cfb6485d3ba50843038209a2a253907\n-- \ngitgitgadget\n"},{"id":"479899","messageId":"pull.1527.v2.git.git.1690387585634.gitgitgadget@gmail.com","threadId":"59907","inReplyTo":"pull.1527.git.git.1687591293705.gitgitgadget@gmail.com","subject":"[PATCH v2] credential/libsecret: erase matching creds only","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-07-26T16:06:25Z","receivedAt":"2023-07-26T16:06:33Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nThe credential erase request typically includes protocol, host, username\nand password.\n\ncredential-libsecret erases a stored credential if it matches protocol,\nhost and username, regardless of password.\n\nThis is confusing in the case the stored password differs from that\nin the request. This case can occur when multiple credential helpers are\nconfigured.\n\nOnly erase credential if stored password matches request (or request\nomits password).\n\nThis fixes test \"helper ... does not erase a password distinct from\ninput\" introduced in aeb21ce22e (credential: avoid erasing distinct\npassword, 2023-06-13)\n\nSigned-off-by: M Hickford \n---\n credential/libsecret: erase more carefully\n \n Patch v2 has a more detailed commit message following\n https://lore.kernel.org/git/xmqqpmax5c4v.fsf@gitster.g/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1527%2Fhickford%2Ffix-libsecret-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1527/hickford/fix-libsecret-v2\nPull-Request: https://github.com/git/git/pull/1527\n\nRange-diff vs v1:\n\n 1: dcc429c1afc ! 1: b5d5c3159ad credential/libsecret: erase matching creds only\n @@ Metadata\n ## Commit message ##\n credential/libsecret: erase matching creds only\n \n - Fix test \"helper ... does not erase a password distinct from input\"\n - introduced in aeb21ce22e (credential: avoid erasing distinct password,\n - 2023-06-13)\n + The credential erase request typically includes protocol, host, username\n + and password.\n +\n + credential-libsecret erases a stored credential if it matches protocol,\n + host and username, regardless of password.\n +\n + This is confusing in the case the stored password differs from that\n + in the request. This case can occur when multiple credential helpers are\n + configured.\n +\n + Only erase credential if stored password matches request (or request\n + omits password).\n +\n + This fixes test \"helper ... does not erase a password distinct from\n + input\" introduced in aeb21ce22e (credential: avoid erasing distinct\n + password, 2023-06-13)\n \n Signed-off-by: M Hickford \n \n\n\n .../libsecret/git-credential-libsecret.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex ef681f29d5b..9110714601e 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -52,6 +52,8 @@ struct credential_operation {\n \n #define CREDENTIAL_OP_END { NULL, NULL }\n \n+static void credential_clear(struct credential *c);\n+\n /* ----------------- Secret Service functions ----------------- */\n \n static char *make_label(struct credential *c)\n@@ -185,6 +187,7 @@ static int keyring_erase(struct credential *c)\n {\n \tGHashTable *attributes = NULL;\n \tGError *error = NULL;\n+\tstruct credential existing = CREDENTIAL_INIT;\n \n \t/*\n \t * Sanity check that we actually have something to match\n@@ -197,6 +200,20 @@ static int keyring_erase(struct credential *c)\n \tif (!c->protocol && !c->host && !c->path && !c->username)\n \t\treturn EXIT_FAILURE;\n \n+\tif (c->password) {\n+\t\texisting.host = g_strdup(c->host);\n+\t\texisting.path = g_strdup(c->path);\n+\t\texisting.port = c->port;\n+\t\texisting.protocol = g_strdup(c->protocol);\n+\t\texisting.username = g_strdup(c->username);\n+\t\tkeyring_get(&existing);\n+\t\tif (existing.password && strcmp(c->password, existing.password)) {\n+\t\t\tcredential_clear(&existing);\n+\t\t\treturn EXIT_SUCCESS;\n+\t\t}\n+\t\tcredential_clear(&existing);\n+\t}\n+\n \tattributes = make_attr_list(c);\n \tsecret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,\n \t\t\t\t attributes,\n\nbase-commit: a80be152923a46f04a06bade7bcc72870e46ca09\n-- \ngitgitgadget\n"},{"id":"479905","messageId":"xmqqzg3id27e.fsf@gitster.g","threadId":"59907","inReplyTo":"pull.1527.v2.git.git.1690387585634.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] credential/libsecret: erase matching creds only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-26T17:15:01Z","receivedAt":"2023-07-26T17:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"M Hickford via GitGitGadget\" writes:\n\n> From: M Hickford \n>\n> The credential erase request typically includes protocol, host, username\n> and password.\n>\n> credential-libsecret erases a stored credential if it matches protocol,\n> host and username, regardless of password.\n>\n> This is confusing in the case the stored password differs from that\n> in the request. This case can occur when multiple credential helpers are\n> configured.\n>\n> Only erase credential if stored password matches request (or request\n> omits password).\n\nThis is much better.\n\n> This fixes test \"helper ... does not erase a password distinct from\n> input\" introduced in aeb21ce22e (credential: avoid erasing distinct\n> password, 2023-06-13)\n\nThis was still confusing for a patch that does not touch anything in\nt/, but after re-reading aeb21ce22e and the above a few times, I\nthink I get it. Adding the following\n\n\t, when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to\n\t\"libsecret\".\n\nat the end may help, but perhaps it is too obvious for folks who are\nready to actually review this change---presumably they are familiar\nwith how t0303 is to be used and read that without being told from\nthe context?\n\nThanks.\n\n"},{"id":"479915","messageId":"CAGJzqskUL23Grv84uuFpm0kJFvNQE1PvR=67WLDt6bAA2OxmXw@mail.gmail.com","threadId":"59907","inReplyTo":"xmqqzg3id27e.fsf@gitster.g","subject":"Re: [PATCH v2] credential/libsecret: erase matching creds only","fromName":"M Hickford","fromEmail":"mirth.hickford@gmail.com","sentAt":"2023-07-26T19:44:56Z","receivedAt":"2023-07-26T19:45:41Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"On Wed, 26 Jul 2023 at 18:15, Junio C Hamano wrote:\n> This was still confusing for a patch that does not touch anything in\n> t/, but after re-reading aeb21ce22e and the above a few times, I\n> think I get it. Adding the following\n>\n> , when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to\n> \"libsecret\".\n>\n> at the end may help, but perhaps it is too obvious for folks who are\n> ready to actually review this change---presumably they are familiar\n> with how t0303 is to be used and read that without being told from\n> the context?\n\nGood idea, the clearer the better. I'll add this in patch v3.\n"},{"id":"479916","messageId":"pull.1527.v3.git.git.1690400814492.gitgitgadget@gmail.com","threadId":"59907","inReplyTo":"pull.1527.v2.git.git.1690387585634.gitgitgadget@gmail.com","subject":"[PATCH v3] credential/libsecret: erase matching creds only","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-07-26T19:46:54Z","receivedAt":"2023-07-26T19:47:01Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford \n\nThe credential erase request typically includes protocol, host, username\nand password.\n\ncredential-libsecret erases a stored credential if it matches protocol,\nhost and username, regardless of password.\n\nThis is confusing in the case the stored password differs from that\nin the request. This case can occur when multiple credential helpers are\nconfigured.\n\nOnly erase credential if stored password matches request (or request\nomits password).\n\nThis fixes test \"helper (libsecret) does not erase a password distinct\nfrom input\" when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to\n\"libsecret\". This test was added in aeb21ce22e (credential: avoid\nerasing distinct password, 2023-06-13).\n\nSigned-off-by: M Hickford \n---\n credential/libsecret: erase more carefully\n \n Patch v3 adds instructions how to run the fixed test\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1527%2Fhickford%2Ffix-libsecret-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1527/hickford/fix-libsecret-v3\nPull-Request: https://github.com/git/git/pull/1527\n\nRange-diff vs v2:\n\n 1: b5d5c3159ad ! 1: 6b3db416c61 credential/libsecret: erase matching creds only\n @@ Commit message\n Only erase credential if stored password matches request (or request\n omits password).\n \n - This fixes test \"helper ... does not erase a password distinct from\n - input\" introduced in aeb21ce22e (credential: avoid erasing distinct\n - password, 2023-06-13)\n + This fixes test \"helper (libsecret) does not erase a password distinct\n + from input\" when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to\n + \"libsecret\". This test was added in aeb21ce22e (credential: avoid\n + erasing distinct password, 2023-06-13).\n \n Signed-off-by: M Hickford \n \n\n\n .../libsecret/git-credential-libsecret.c | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c\nindex ef681f29d5b..9110714601e 100644\n--- a/contrib/credential/libsecret/git-credential-libsecret.c\n+++ b/contrib/credential/libsecret/git-credential-libsecret.c\n@@ -52,6 +52,8 @@ struct credential_operation {\n \n #define CREDENTIAL_OP_END { NULL, NULL }\n \n+static void credential_clear(struct credential *c);\n+\n /* ----------------- Secret Service functions ----------------- */\n \n static char *make_label(struct credential *c)\n@@ -185,6 +187,7 @@ static int keyring_erase(struct credential *c)\n {\n \tGHashTable *attributes = NULL;\n \tGError *error = NULL;\n+\tstruct credential existing = CREDENTIAL_INIT;\n \n \t/*\n \t * Sanity check that we actually have something to match\n@@ -197,6 +200,20 @@ static int keyring_erase(struct credential *c)\n \tif (!c->protocol && !c->host && !c->path && !c->username)\n \t\treturn EXIT_FAILURE;\n \n+\tif (c->password) {\n+\t\texisting.host = g_strdup(c->host);\n+\t\texisting.path = g_strdup(c->path);\n+\t\texisting.port = c->port;\n+\t\texisting.protocol = g_strdup(c->protocol);\n+\t\texisting.username = g_strdup(c->username);\n+\t\tkeyring_get(&existing);\n+\t\tif (existing.password && strcmp(c->password, existing.password)) {\n+\t\t\tcredential_clear(&existing);\n+\t\t\treturn EXIT_SUCCESS;\n+\t\t}\n+\t\tcredential_clear(&existing);\n+\t}\n+\n \tattributes = make_attr_list(c);\n \tsecret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,\n \t\t\t\t attributes,\n\nbase-commit: a80be152923a46f04a06bade7bcc72870e46ca09\n-- \ngitgitgadget\n"}]}