| Message ID | pull.1856.git.1738352886190.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | credential: warn about git-credential-store [RFC] | expand |
> - 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.
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
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.
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.
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