Re: [PATCH 3/3] clone: auto-enable git-credential-store when necessary

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, May 19 2019, Jeff King wrote:

> If the user clones with a URL containing a password and has no
> credential helper configured, we're stuck. We don't want to write the
> password into .git/config because that risks accidentally disclosing it.
> But if we don't record it somewhere, subsequent fetches will fail unless
> the user is there to input the password.
>
> The best advice we can give the user is to set up a credential helper.
> But we can actually go a step further and enable the "store" helper for
> them. This still records the password in plaintext, but:
>
>   1. It's not inside the repo directory, which makes it slightly less
>      likely to be disclosed.
>
>   2. The permissions on the storage file are tighter than what would be
>      on .git/config.
>
> So this is generally a security win over the old behavior of writing it
> into .git/config. And it's a usability win over the more recent behavior
> of just forgetting the password entirely.
>
> The biggest downside is that it's a bit magical from the user's
> perspective, because now the password is off in some other file (usually
> ~/.git-credentials, but sometimes in $XDG_CONFIG_HOME). Which
> complicates things if they want to purge the repo and password, for
> example, because now they can't just delete the repository directory.
>
> The file location is documented, though, and we point people to the
> documentation. So perhaps it will be enough (and better still, may lead
> to them configuring a more secure helper).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> If we do decide this is too magical, I think the best alternate path is
> to advise them on credential helpers, and how to seed the password into
> the helper (basically configure the helper and then "git fetch"
> should work).
>
> One other thing I wondered: if they do have a helper configured this
> patch will omit the advice entirely, but we'll still print the warning.
> Is that useful (to remind them that passwords in URLs are a bad thing),
> or is it just annoying?
>
>  builtin/clone.c            | 19 ++++++++++++++++---
>  credential.c               | 13 +++++++++++++
>  credential.h               |  6 ++++++
>  t/t5550-http-fetch-dumb.sh |  2 +-
>  4 files changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 15fffc3268..94d2659154 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -31,6 +31,7 @@
>  #include "packfile.h"
>  #include "list-objects-filter-options.h"
>  #include "object-store.h"
> +#include "credential.h"
>
>  /*
>   * Overall FIXMEs:
> @@ -894,8 +895,14 @@ static int dir_exists(const char *path)
>  static const char sanitized_url_advice[] = N_(
>  "The URL you provided to Git contains a password. It will be\n"
>  "used to clone the repository, but to avoid accidental disclosure\n"
> -"the password will not be recorded. Further fetches from the remote\n"
> -"may require you to provide the password interactively.\n"
> +"the password will not be recorded in the repository config.\n"
> +"Since you have no credential helper configured, the \"store\" helper\n"
> +"has been enabled for this repository, and will provide the password\n"
> +"for further fetches.\n"
> +"\n"
> +"Note that the password is still stored in plaintext in the filesystem;\n"
> +"consider configuring a more secure helper. See \"git help gitcredentials\"\n"
> +"and \"git help git-credential-store\" for details.\n"
>  );
>
>  int cmd_clone(int argc, const char **argv, const char *prefix)
> @@ -1090,7 +1097,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	sanitized_repo = transport_strip_url(repo, 0);
>  	if (strcmp(repo, sanitized_repo)) {
>  		warning(_("omitting password while storing URL in on-disk config"));
> -		advise(_(sanitized_url_advice));
> +		if (!url_has_credential_helper(sanitized_repo)) {
> +			strbuf_addf(&key, "credential.%s.helper",
> +				    sanitized_repo);
> +			git_config_set(key.buf, "store");
> +			strbuf_reset(&key);
> +			advise(_(sanitized_url_advice));
> +		}
>  	}
>  	strbuf_addf(&key, "remote.%s.url", option_origin);
>  	git_config_set(key.buf, sanitized_repo);
> diff --git a/credential.c b/credential.c
> index 62be651b03..0a70edcee5 100644
> --- a/credential.c
> +++ b/credential.c
> @@ -372,3 +372,16 @@ void credential_from_url(struct credential *c, const char *url)
>  			*p-- = '\0';
>  	}
>  }
> +
> +int url_has_credential_helper(const char *url)
> +{
> +	struct credential c = CREDENTIAL_INIT;
> +	int ret;
> +
> +	credential_from_url(&c, url);
> +	credential_apply_config(&c);
> +	ret = c.helpers.nr > 0;
> +
> +	credential_clear(&c);
> +	return ret;
> +}
> diff --git a/credential.h b/credential.h
> index 6b0cd16be2..e6d6d6fa40 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -32,4 +32,10 @@ void credential_from_url(struct credential *, const char *url);
>  int credential_match(const struct credential *have,
>  		     const struct credential *want);
>
> +/*
> + * Return true if feeding "url" to the credential system would trigger one
> + * or more helpers.
> + */
> +int url_has_credential_helper(const char *url);
> +
>  #endif /* CREDENTIAL_H */
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index d8c85f3126..2723e91ae0 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -70,7 +70,7 @@ test_expect_success 'username is retained in URL, password is not' '
>  	! grep pass url
>  '
>
> -test_expect_failure 'fetch of password-URL clone uses stored auth' '
> +test_expect_success 'fetch of password-URL clone uses stored auth' '
>  	set_askpass wrong &&
>  	git -C clone-auth-none fetch &&
>  	expect_askpass none

I've only looked at this very briefly, there's a regression here where
you're assuming that having a configured credential helper means it
works.

I.e. I have a ~/.gitconfig where I point to some-gnome-thing-or-other
what doesn't exist on my VPS in my ~/.gitconfig, cloning just warns
about it being missing, but will store the password in the repo.

With this you detect that I have the helper, don't store it, but then my
helper doesn't work, whereas this worked before.

That's an X-Y problem of config includes being rather limited (now I'm
just putting up with the error), but I expect this'll apply to others.