{"thread":{"id":"62880","subject":"[PATCH] credential: warn about git-credential-store [RFC]","startedAt":"2025-01-31T19:48:09Z","lastAt":"2025-02-02T23:41:08Z","messageCount":5,"participants":["M Hickford via GitGitGadget","Junio C Hamano","Jeff King","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"511589","messageId":"pull.1856.git.1738352886190.gitgitgadget@gmail.com","threadId":"62880","inReplyTo":null,"subject":"[PATCH] credential: warn about git-credential-store [RFC]","fromName":"M Hickford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-31T19:48:06Z","receivedAt":"2025-01-31T19:48:09Z","isPatch":true,"sender":{"key":"mirth.hickford@gmail.com","avatar":"https://avatars.githubusercontent.com/u/105314?v=4"},"body":"From: M Hickford <mirth.hickford@gmail.com>\n\ngit-credential-store saves secrets unencrypted on disk.\n\nWarn the user before they type their password, suggesting alternative\ncredential helpers.\n\nAn alternative could be to warn in \"credential-store store\". A\ndisadvantage is that the user wouldn't see the warning until after they\ntyped their password, which is less helpful. The warning would appear\nagain every time the user authenticated, which feels too frequently.\n\nSigned-off-by: M Hickford <mirth.hickford@gmail.com>\n---\n    credential: warn about git-credential-store [RFC]\n    \n    RFC for discussion. Some tests fail\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1856%2Fhickford%2Fstore-warn-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1856/hickford/store-warn-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1856\n\n credential.c                | 6 +++++-\n t/lib-credential.sh         | 2 ++\n t/t0302-credential-store.sh | 3 +++\n 3 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 2594c0c4229..6e05bba7e2f 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -285,9 +285,13 @@ static int credential_getpass(struct repository *r, struct credential *c)\n \tif (!c->username)\n \t\tc->username = credential_ask_one(\"Username\", c,\n \t\t\t\t\t\t PROMPT_ASKPASS|PROMPT_ECHO);\n-\tif (!c->password)\n+\tif (!c->password) {\n+\t\tif (c->helpers.nr >= 1 && starts_with(c->helpers.items[0].string, \"store\"))\n+\t\t\twarning(\"git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7).\");\n+\n \t\tc->password = credential_ask_one(\"Password\", c,\n \t\t\t\t\t\t PROMPT_ASKPASS);\n+\t}\n \ttrace2_region_leave(\"credential\", \"interactive\", r);\n \n \treturn 0;\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex 58b9c740605..47483f09006 100644\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -67,6 +67,8 @@ reject() {\n helper_test() {\n \tHELPER=$1\n \n+\t# help wanted: expect warning \"git-credential-store saves passwords\n+\t# unencrypted\" when helper equals \"store\"\n \ttest_expect_success \"helper ($HELPER) has no existing data\" '\n \t\tcheck fill $HELPER <<-\\EOF\n \t\tprotocol=https\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex c1cd60edd01..349b5f0b084 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -133,6 +133,7 @@ invalid_credential_test() {\n \t\tpassword=askpass-password\n \t\t--\n \t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\twarning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.\n \t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n \t\t--\n \t\tEOF\n@@ -155,6 +156,7 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '\n \tpassword=askpass-password\n \t--\n \taskpass: Username for '\\''https://example.com'\\'':\n+\twarning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.\n \taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n \t--\n \tEOF\n@@ -186,6 +188,7 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path\n \tpassword=askpass-password\n \t--\n \taskpass: Username for '\\''https://example.com/repo.git'\\'':\n+\twarning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.\n \taskpass: Password for '\\''https://askpass-username@example.com/repo.git'\\'':\n \t--\n \tEOF\n\nbase-commit: 4e746b1a31f9f0036032b6f94279cf16fb363203\n-- \ngitgitgadget\n"},{"id":"511590","messageId":"xmqqlduq7nqd.fsf@gitster.g","threadId":"62880","inReplyTo":"pull.1856.git.1738352886190.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential: warn about git-credential-store [RFC]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-31T20:05:46Z","receivedAt":"2025-01-31T20:05:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> -\tif (!c->password)\n> +\tif (!c->password) {\n> +\t\tif (c->helpers.nr >= 1 && starts_with(c->helpers.items[0].string, \"store\"))\n> +\t\t\twarning(\"git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7).\");\n\nI have no strong opinion on the details of how the detection of use\nof the \"store\" helper should be implemented, but I recall reading\nsomewhere that users can configure more than one helpers and they\nare used in casdading fashion?  Insecure helpers may be configured\nto come later on the list, so [0] might not be sufficient.  A few\nother things are that git-credential-store could be installed in an\nunusual place and credential.c:credential_do() may find it from its\nabsolute path.  Also the end-users can use third-party helpers,\nwhose names we do not control, but presumably they will not name\ntheirs exactly the same as the one we ship, so starts_with() may\nwant to get a bit tightened.  If somebody writes a custom helper\n\"git-credential-store-securely\" and installs the binary in a\ndirectory where \"git\" can find via the usual GIT_EXEC_PATH mechanism\nas \"git credential-store-securely\", helpers.items[].string would say\n\"store-securely\".\n\nI agree with you that it is a rather unfortunate layering violation\nthat you need to know what helper would see the result from this\nfunction, because you want to warn before the user gives the\npassword to us.\n\nWarning immediately before the bits hits the disk platter (i.e., the\nresult of _fill() is passed to the helper) is not as secure because\nthere is no way to say \"ah, was I using an insecure backend?  Then\nplease stop and do not store it there\" later, so I do not think of a\nstrong reason to claim that it is a wrong place to give the warning.\n\nRegarding the warning message, you may want to consider using the\nadvice mechanism for a thing like this, perhaps?  If somebody has a\nlegitimate reason why they need to use and cannot move away from the\nbackend, it does not help them at all to keep giving the same\nwarning() they are already aware of, without a way to say \"Yes, I\nknow, I've seen it enough times, go shut up, please\".\n\nThanks.\n"},{"id":"511618","messageId":"20250201025413.GB4088801@coredump.intra.peff.net","threadId":"62880","inReplyTo":"pull.1856.git.1738352886190.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential: warn about git-credential-store [RFC]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-01T02:54:13Z","receivedAt":"2025-02-01T02:54:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 31, 2025 at 07:48:06PM +0000, M Hickford via GitGitGadget wrote:\n\n> From: M Hickford <mirth.hickford@gmail.com>\n> \n> git-credential-store saves secrets unencrypted on disk.\n> \n> Warn the user before they type their password, suggesting alternative\n> credential helpers.\n> \n> An alternative could be to warn in \"credential-store store\". A\n> disadvantage is that the user wouldn't see the warning until after they\n> typed their password, which is less helpful. The warning would appear\n> again every time the user authenticated, which feels too frequently.\n\nI certainly don't disagree that \"store\" is relatively insecure,\nbut...who are we trying to help here? We do not turn on \"store\" by\ndefault, so anybody who is running it would had to have explicitly\nconfigured it as a helper. And there's a big warning already at the top\nof the manpage.\n\nIf we think it's so bad that we need to spam people with a warning, then\nperhaps we should just remove it entirely. Or if people aren't seeing\nthe warning, can we call it \"git-credential-plaintext\" or something that\nwill make it more obviously not secure?\n\n> -\tif (!c->password)\n> +\tif (!c->password) {\n> +\t\tif (c->helpers.nr >= 1 && starts_with(c->helpers.items[0].string, \"store\"))\n> +\t\t\twarning(\"git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7).\");\n> +\n\nAs Junio noted, this won't catch \"store\" as the second helper. It would\nalso not catch \"store --file=/path/to/store\" or using a shell invocation\nlike \"!git credential-store\".\n\nThis location also won't notice that \"store\" will be passed credentials\nprovided by other helpers (not just ones from the terminal).\n\nI think you'd have to put the warning in credential-store itself to hit\nit reliably. If you wanted to avoid warning excessively, it could\nprobably notice when the stored entry was already there. As you note, it\nwill already have written the password, but the warning could advise on\nhow to delete it (yes, it will be on disk for a moment until they delete\nit, but I think we are getting at diminishing returns of advice).\n\nAlternatively, if we force a user to acknowledge a config option, then\nthey can't miss it. And we can put the check wherever we like, without\nwriting anything. Something like:\n\ndiff --git a/builtin/credential-store.c b/builtin/credential-store.c\nindex e669e99dbf..6b6dca79b1 100644\n--- a/builtin/credential-store.c\n+++ b/builtin/credential-store.c\n@@ -119,6 +119,14 @@ static void store_credential_file(const char *fn, struct credential *c)\n static void store_credential(const struct string_list *fns, struct credential *c)\n {\n \tstruct string_list_item *fn;\n+\tint allow = 0;\n+\n+\tgit_config_get_bool(\"credential.allowinsecurehelpers\", &allow);\n+\tif (!allow) {\n+\t\twarning(\"yikes!\");\n+\t\t/* probably also advise() how to set the config */\n+\t\treturn;\n+\t}\n \n \t/*\n \t * Sanity check that what we are storing is actually sensible.\n\nThat's a breaking change for people using credential-store, but you\ncould perhaps ease them into it with a \"warn\" mode (which they could\nthen squelch the warning by setting the option early). And then\neventually it defaults to refusing to store.\n\nAgain, if we are going this far, I kind of wonder if we should just\nremove the helper.\n\n-Peff\n"},{"id":"511630","messageId":"Z53ybUCIHPG78Vj2@tapette.crustytoothpaste.net","threadId":"62880","inReplyTo":"pull.1856.git.1738352886190.gitgitgadget@gmail.com","subject":"Re: [PATCH] credential: warn about git-credential-store [RFC]","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2025-02-01T10:07:41Z","receivedAt":"2025-02-01T10:07:49Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2025-01-31 at 19:48:06, M Hickford via GitGitGadget wrote:\n> From: M Hickford <mirth.hickford@gmail.com>\n> \n> git-credential-store saves secrets unencrypted on disk.\n> \n> Warn the user before they type their password, suggesting alternative\n> credential helpers.\n> \n> An alternative could be to warn in \"credential-store store\". A\n> disadvantage is that the user wouldn't see the warning until after they\n> typed their password, which is less helpful. The warning would appear\n> again every time the user authenticated, which feels too frequently.\n\nI don't think this is a good idea.  While it's typically recommended to\nuse a different credential helper, it can be difficult to do so in an\nenvironment where you don't have a desktop, since all of the major\nhelpers use the system keychain, where a desktop is required.\n\nIf you have such an environment (such as a remote system) and can't use\nSSH (because your corporate environment only allows HTTPS), then you\nreally don't have many, if any, alternatives[0].  All warning in this\ncase is going to do is just annoy the user, especially if they have many\nsuch systems.\n\nIf we are going to do this, I'd recommend using the advice system, so\nthat users can just disable the warning.\n\n[0] Okay, I lied.  I have a tool called Lawn (local spawn) which allows\nyou to run a command on your laptop or desktop from the remote machine,\nsuch as a credential helper, but it's not in widespread use and I don't\nthink it's polished enough to recommend here.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"511670","messageId":"xmqqo6zj3ofi.fsf@gitster.g","threadId":"62880","inReplyTo":"20250201025413.GB4088801@coredump.intra.peff.net","subject":"Re: [PATCH] credential: warn about git-credential-store [RFC]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-02T23:41:05Z","receivedAt":"2025-02-02T23:41:08Z","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 Fri, Jan 31, 2025 at 07:48:06PM +0000, M Hickford via GitGitGadget wrote:\n>\n>> From: M Hickford <mirth.hickford@gmail.com>\n>> \n>> git-credential-store saves secrets unencrypted on disk.\n>> \n>> Warn the user before they type their password, suggesting alternative\n>> credential helpers.\n>> \n>> An alternative could be to warn in \"credential-store store\". A\n>> disadvantage is that the user wouldn't see the warning until after they\n>> typed their password, which is less helpful. The warning would appear\n>> again every time the user authenticated, which feels too frequently.\n>\n> I certainly don't disagree that \"store\" is relatively insecure,\n> but...who are we trying to help here? We do not turn on \"store\" by\n> default, so anybody who is running it would had to have explicitly\n> configured it as a helper. And there's a big warning already at the top\n> of the manpage.\n\nI buy this argument.  I think an earlier comment by brian was on a\nsimilar wavelength.\n\nThanks.\n\n"}]}