On Fri, Jan 31, 2025 at 07:48:06PM +0000, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@xxxxxxxxx>
>
> git-credential-store saves secrets unencrypted on disk.
>
> Warn the user before they type their password, suggesting alternative
> credential helpers.
>
> An alternative could be to warn in "credential-store store". A
> disadvantage is that the user wouldn't see the warning until after they
> typed their password, which is less helpful. The warning would appear
> again every time the user authenticated, which feels too frequently.
I certainly don't disagree that "store" is relatively insecure,
but...who are we trying to help here? We do not turn on "store" by
default, so anybody who is running it would had to have explicitly
configured it as a helper. And there's a big warning already at the top
of the manpage.
If we think it's so bad that we need to spam people with a warning, then
perhaps we should just remove it entirely. Or if people aren't seeing
the warning, can we call it "git-credential-plaintext" or something that
will make it more obviously not secure?
> - if (!c->password)
> + if (!c->password) {
> + if (c->helpers.nr >= 1 && starts_with(c->helpers.items[0].string, "store"))
> + warning("git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7).");
> +
As Junio noted, this won't catch "store" as the second helper. It would
also not catch "store --file=/path/to/store" or using a shell invocation
like "!git credential-store".
This location also won't notice that "store" will be passed credentials
provided by other helpers (not just ones from the terminal).
I think you'd have to put the warning in credential-store itself to hit
it reliably. If you wanted to avoid warning excessively, it could
probably notice when the stored entry was already there. As you note, it
will already have written the password, but the warning could advise on
how to delete it (yes, it will be on disk for a moment until they delete
it, but I think we are getting at diminishing returns of advice).
Alternatively, if we force a user to acknowledge a config option, then
they can't miss it. And we can put the check wherever we like, without
writing anything. Something like:
diff --git a/builtin/credential-store.c b/builtin/credential-store.c
index e669e99dbf..6b6dca79b1 100644
--- a/builtin/credential-store.c
+++ b/builtin/credential-store.c
@@ -119,6 +119,14 @@ static void store_credential_file(const char *fn, struct credential *c)
static void store_credential(const struct string_list *fns, struct credential *c)
{
struct string_list_item *fn;
+ int allow = 0;
+
+ git_config_get_bool("credential.allowinsecurehelpers", &allow);
+ if (!allow) {
+ warning("yikes!");
+ /* probably also advise() how to set the config */
+ return;
+ }
/*
* Sanity check that what we are storing is actually sensible.
That's a breaking change for people using credential-store, but you
could perhaps ease them into it with a "warn" mode (which they could
then squelch the warning by setting the option early). And then
eventually it defaults to refusing to store.
Again, if we are going this far, I kind of wonder if we should just
remove the helper.
-Peff
