[PATCH] credential: warn about git-credential-store [RFC]

[PATCH] credential: warn about git-credential-store [RFC]

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

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.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential: warn about git-credential-store [RFC]
    
    RFC for discussion. Some tests fail

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1856%2Fhickford%2Fstore-warn-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1856/hickford/store-warn-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1856

 credential.c                | 6 +++++-
 t/lib-credential.sh         | 2 ++
 t/t0302-credential-store.sh | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/credential.c b/credential.c
index 2594c0c4229..6e05bba7e2f 100644
--- a/credential.c
+++ b/credential.c
@@ -285,9 +285,13 @@ static int credential_getpass(struct repository *r, struct credential *c)
 	if (!c->username)
 		c->username = credential_ask_one("Username", c,
 						 PROMPT_ASKPASS|PROMPT_ECHO);
-	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).");
+
 		c->password = credential_ask_one("Password", c,
 						 PROMPT_ASKPASS);
+	}
 	trace2_region_leave("credential", "interactive", r);
 
 	return 0;
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 58b9c740605..47483f09006 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -67,6 +67,8 @@ reject() {
 helper_test() {
 	HELPER=$1
 
+	# help wanted: expect warning "git-credential-store saves passwords
+	# unencrypted" when helper equals "store"
 	test_expect_success "helper ($HELPER) has no existing data" '
 		check fill $HELPER <<-\EOF
 		protocol=https
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index c1cd60edd01..349b5f0b084 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -133,6 +133,7 @@ invalid_credential_test() {
 		password=askpass-password
 		--
 		askpass: Username for '\''https://example.com'\'':
+		warning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.
 		askpass: Password for '\''https://askpass-username@example.com'\'':
 		--
 		EOF
@@ -155,6 +156,7 @@ test_expect_success 'get: credentials with DOS line endings are invalid' '
 	password=askpass-password
 	--
 	askpass: Username for '\''https://example.com'\'':
+	warning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.
 	askpass: Password for '\''https://askpass-username@example.com'\'':
 	--
 	EOF
@@ -186,6 +188,7 @@ test_expect_success 'get: credentials with DOS line endings are invalid if path
 	password=askpass-password
 	--
 	askpass: Username for '\''https://example.com/repo.git'\'':
+	warning: git-credential-store saves passwords unencrypted on disk. For alternatives, see gitcredentials(7) or https://git-scm.com/doc/credential-helpers.
 	askpass: Password for '\''https://askpass-username@example.com/repo.git'\'':
 	--
 	EOF

base-commit: 4e746b1a31f9f0036032b6f94279cf16fb363203
-- 
gitgitgadget

Re: [PATCH] credential: warn about git-credential-store [RFC]

[Junio C Hamano]

> -	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).");

I have no strong opinion on the details of how the detection of use
of the "store" helper should be implemented, but I recall reading
somewhere that users can configure more than one helpers and they
are used in casdading fashion?  Insecure helpers may be configured
to come later on the list, so [0] might not be sufficient.  A few
other things are that git-credential-store could be installed in an
unusual place and credential.c:credential_do() may find it from its
absolute path.  Also the end-users can use third-party helpers,
whose names we do not control, but presumably they will not name
theirs exactly the same as the one we ship, so starts_with() may
want to get a bit tightened.  If somebody writes a custom helper
"git-credential-store-securely" and installs the binary in a
directory where "git" can find via the usual GIT_EXEC_PATH mechanism
as "git credential-store-securely", helpers.items[].string would say
"store-securely".

I agree with you that it is a rather unfortunate layering violation
that you need to know what helper would see the result from this
function, because you want to warn before the user gives the
password to us.

Warning immediately before the bits hits the disk platter (i.e., the
result of _fill() is passed to the helper) is not as secure because
there is no way to say "ah, was I using an insecure backend?  Then
please stop and do not store it there" later, so I do not think of a
strong reason to claim that it is a wrong place to give the warning.

Regarding the warning message, you may want to consider using the
advice mechanism for a thing like this, perhaps?  If somebody has a
legitimate reason why they need to use and cannot move away from the
backend, it does not help them at all to keep giving the same
warning() they are already aware of, without a way to say "Yes, I
know, I've seen it enough times, go shut up, please".

Thanks.

Re: [PATCH] credential: warn about git-credential-store [RFC]

[Jeff King]

On Fri, Jan 31, 2025 at 07:48:06PM +0000, M Hickford via GitGitGadget wrote:

> From: M Hickford <mirth.hickford@gmail.com>
> 
> 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

Re: [PATCH] credential: warn about git-credential-store [RFC]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 31, 2025 at 07:48:06PM +0000, M Hickford via GitGitGadget wrote:
>
>> From: M Hickford <mirth.hickford@gmail.com>
>> 
>> 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.

I buy this argument.  I think an earlier comment by brian was on a
similar wavelength.

Thanks.

Re: [PATCH] credential: warn about git-credential-store [RFC]

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1595 bytes --]

On 2025-01-31 at 19:48:06, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@gmail.com>
> 
> 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 don't think this is a good idea.  While it's typically recommended to
use a different credential helper, it can be difficult to do so in an
environment where you don't have a desktop, since all of the major
helpers use the system keychain, where a desktop is required.

If you have such an environment (such as a remote system) and can't use
SSH (because your corporate environment only allows HTTPS), then you
really don't have many, if any, alternatives[0].  All warning in this
case is going to do is just annoy the user, especially if they have many
such systems.

If we are going to do this, I'd recommend using the advice system, so
that users can just disable the warning.

[0] Okay, I lied.  I have a tool called Lawn (local spawn) which allows
you to run a command on your laptop or desktop from the remote machine,
such as a credential helper, but it's not in widespread use and I don't
think it's polished enough to recommend here.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]