diff mbox series

[v6,2/8] config: add new way to pass config via `--config-env`

Message ID 9b8461010e641369316d00e2fc58c16e0e191f42.1610001187.git.ps@pks.im (mailing list archive)
State New, archived
Headers show
Series config: allow specifying config entries via env | expand

Commit Message

Patrick Steinhardt Jan. 7, 2021, 6:36 a.m. UTC
While it's already possible to pass runtime configuration via `git -c
<key>=<value>`, it may be undesirable to use when the value contains
sensitive information. E.g. if one wants to set `http.extraHeader` to
contain an authentication token, doing so via `-c` would trivially leak
those credentials via e.g. ps(1), which typically also shows command
arguments.

To enable this usecase without leaking credentials, this commit
introduces a new switch `--config-env=<key>=<envvar>`. Instead of
directly passing a value for the given key, it instead allows the user
to specify the name of an environment variable. The value of that
variable will then be used as value of the key.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git.txt | 24 ++++++++++++++++++++++-
 config.c              | 21 ++++++++++++++++++++
 config.h              |  1 +
 git.c                 |  4 +++-
 t/t1300-config.sh     | 45 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 93 insertions(+), 2 deletions(-)

Comments

Simon Ruderich Jan. 10, 2021, 8:29 p.m. UTC | #1
On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:
> [snip]
>
> +void git_config_push_env(const char *spec)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *env_name;
> +	const char *env_value;
> +
> +	env_name = strrchr(spec, '=');
> +	if (!env_name)
> +		die("invalid config format: %s", spec);
> +	env_name++;
> +
> +	env_value = getenv(env_name);
> +	if (!env_value)
> +		die("config variable missing for '%s'", env_name);

I think "environment variable" should be mentioned in the error
message to make it clear what kind of "variable" is missing.

Btw. shouldn't these strings get translated (or does die() do
that automatically)?

Regards
Simon
Junio C Hamano Jan. 11, 2021, 12:29 a.m. UTC | #2
Simon Ruderich <simon@ruderich.org> writes:

> On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:
>> [snip]
>>
>> +void git_config_push_env(const char *spec)
>> +{
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char *env_name;
>> +	const char *env_value;
>> +
>> +	env_name = strrchr(spec, '=');
>> +	if (!env_name)
>> +		die("invalid config format: %s", spec);
>> +	env_name++;
>> +
>> +	env_value = getenv(env_name);
>> +	if (!env_value)
>> +		die("config variable missing for '%s'", env_name);
>
> I think "environment variable" should be mentioned in the error
> message to make it clear what kind of "variable" is missing.

Good observation.  This parses foo=bar and complains about bar
missing in the environment; It is not a "config variable" that is
missing.

It is "'bar', which is supposed to be there whose value is going to
be used as the value of configuration variable 'foo', is missing."

I wonder if we should also talk about 'foo' at the same time as a
hint for what went wrong?  E.g.

	die(_("missing environment variable '%s' for configuration '%.*s'"),
            env_name, (int)((env_name-1) - spec), spec);

I don't offhand know if that is too much info that may not be all
that useful, though.

> Btw. shouldn't these strings get translated (or does die() do
> that automatically)?

The format string given to die/error/warn should be marked with _().

Thanks.
Patrick Steinhardt Jan. 11, 2021, 8:24 a.m. UTC | #3
On Sun, Jan 10, 2021 at 04:29:01PM -0800, Junio C Hamano wrote:
> Simon Ruderich <simon@ruderich.org> writes:
> 
> > On Thu, Jan 07, 2021 at 07:36:52AM +0100, Patrick Steinhardt wrote:
> >> [snip]
> >>
> >> +void git_config_push_env(const char *spec)
> >> +{
> >> +	struct strbuf buf = STRBUF_INIT;
> >> +	const char *env_name;
> >> +	const char *env_value;
> >> +
> >> +	env_name = strrchr(spec, '=');
> >> +	if (!env_name)
> >> +		die("invalid config format: %s", spec);
> >> +	env_name++;
> >> +
> >> +	env_value = getenv(env_name);
> >> +	if (!env_value)
> >> +		die("config variable missing for '%s'", env_name);
> >
> > I think "environment variable" should be mentioned in the error
> > message to make it clear what kind of "variable" is missing.
> 
> Good observation.  This parses foo=bar and complains about bar
> missing in the environment; It is not a "config variable" that is
> missing.
> 
> It is "'bar', which is supposed to be there whose value is going to
> be used as the value of configuration variable 'foo', is missing."

Indeed.

> I wonder if we should also talk about 'foo' at the same time as a
> hint for what went wrong?  E.g.
> 
> 	die(_("missing environment variable '%s' for configuration '%.*s'"),
>             env_name, (int)((env_name-1) - spec), spec);
> 
> I don't offhand know if that is too much info that may not be all
> that useful, though.

No, I think that this error message is quite useful as it points out
both what's missing and why we expect it to exist in the first place.

> > Btw. shouldn't these strings get translated (or does die() do
> > that automatically)?
> 
> The format string given to die/error/warn should be marked with _().
> 
> Thanks.

Right, will fix.

Patrick
diff mbox series

Patch

diff --git a/Documentation/git.txt b/Documentation/git.txt
index a6d4ad0818..d36e6fd482 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,7 +13,7 @@  SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--super-prefix=<path>]
+    [--super-prefix=<path>] [--config-env <name>=<envvar>]
     <command> [<args>]
 
 DESCRIPTION
@@ -80,6 +80,28 @@  config file). Including the equals but with an empty value (like `git -c
 foo.bar= ...`) sets `foo.bar` to the empty string which `git config
 --type=bool` will convert to `false`.
 
+--config-env=<name>=<envvar>::
+	Like `-c <name>=<value>`, give configuration variable
+	'<name>' a value, where <envvar> is the name of an
+	environment variable from which to retrieve the value. Unlike
+	`-c` there is no shortcut for directly setting the value to an
+	empty string, instead the environment variable itself must be
+	set to the empty string.  It is an error if the `<envvar>` does not exist
+	in the environment. `<envvar>` may not contain an equals sign
+	to avoid ambiguity with `<name>`s which contain one.
++
+This is useful for cases where you want to pass transitory
+configuration options to git, but are doing so on OS's where
+other processes might be able to read your cmdline
+(e.g. `/proc/self/cmdline`), but not your environ
+(e.g. `/proc/self/environ`). That behavior is the default on
+Linux, but may not be on your system.
++
+Note that this might add security for variables such as
+`http.extraHeader` where the sensitive information is part of
+the value, but not e.g. `url.<base>.insteadOf` where the
+sensitive information can be part of the key.
+
 --exec-path[=<path>]::
 	Path to wherever your core Git programs are installed.
 	This can also be controlled by setting the GIT_EXEC_PATH
diff --git a/config.c b/config.c
index 1137bd73af..cde3511110 100644
--- a/config.c
+++ b/config.c
@@ -345,6 +345,27 @@  void git_config_push_parameter(const char *text)
 	strbuf_release(&env);
 }
 
+void git_config_push_env(const char *spec)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *env_name;
+	const char *env_value;
+
+	env_name = strrchr(spec, '=');
+	if (!env_name)
+		die("invalid config format: %s", spec);
+	env_name++;
+
+	env_value = getenv(env_name);
+	if (!env_value)
+		die("config variable missing for '%s'", env_name);
+
+	strbuf_add(&buf, spec, env_name - spec);
+	strbuf_addstr(&buf, env_value);
+	git_config_push_parameter(buf.buf);
+	strbuf_release(&buf);
+}
+
 static inline int iskeychar(int c)
 {
 	return isalnum(c) || c == '-';
diff --git a/config.h b/config.h
index c1449bb790..19a9adbaa9 100644
--- a/config.h
+++ b/config.h
@@ -138,6 +138,7 @@  int git_config_from_mem(config_fn_t fn,
 int git_config_from_blob_oid(config_fn_t fn, const char *name,
 			     const struct object_id *oid, void *data);
 void git_config_push_parameter(const char *text);
+void git_config_push_env(const char *spec);
 int git_config_from_parameters(config_fn_t fn, void *data);
 void read_early_config(config_fn_t cb, void *data);
 void read_very_early_config(config_fn_t cb, void *data);
diff --git a/git.c b/git.c
index 5a8ff12f87..b5f63d346b 100644
--- a/git.c
+++ b/git.c
@@ -29,7 +29,7 @@  const char git_usage_string[] =
 	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
 	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
 	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	   "           [--super-prefix=<path>]\n"
+	   "           [--super-prefix=<path>] [--config-env=<name>=<envvar>]\n"
 	   "           <command> [<args>]");
 
 const char git_more_info_string[] =
@@ -255,6 +255,8 @@  static int handle_options(const char ***argv, int *argc, int *envchanged)
 			git_config_push_parameter((*argv)[1]);
 			(*argv)++;
 			(*argc)--;
+		} else if (skip_prefix(cmd, "--config-env=", &cmd)) {
+			git_config_push_env(cmd);
 		} else if (!strcmp(cmd, "--literal-pathspecs")) {
 			setenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, "1", 1);
 			if (envchanged)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97a04c6cc2..46a94814d5 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1316,6 +1316,51 @@  test_expect_success 'detect bogus GIT_CONFIG_PARAMETERS' '
 		git config --get-regexp "env.*"
 '
 
+test_expect_success 'git --config-env=key=envvar support' '
+	cat >expect <<-\EOF &&
+	value
+	value
+	false
+	EOF
+	{
+		env ENVVAR=value git --config-env=core.name=ENVVAR config core.name &&
+		env ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase &&
+		env ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag
+	} >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git --config-env fails with invalid parameters' '
+	test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error &&
+	test_i18ngrep "invalid config format" error &&
+	test_must_fail git --config-env=foo.flag=NONEXISTENT config --bool foo.flag 2>error &&
+	test_i18ngrep "config variable missing" error
+'
+
+test_expect_success 'git -c and --config-env work together' '
+	cat >expect <<-\EOF &&
+	bar.cmd cmd-value
+	bar.env env-value
+	EOF
+	env ENVVAR=env-value git \
+		-c bar.cmd=cmd-value \
+		--config-env=bar.env=ENVVAR \
+		config --get-regexp "^bar.*" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git -c and --config-env override each other' '
+	cat >expect <<-\EOF &&
+	env
+	cmd
+	EOF
+	{
+		env ENVVAR=env git -c bar.bar=cmd --config-env=bar.bar=ENVVAR config bar.bar &&
+		env ENVVAR=env git --config-env=bar.bar=ENVVAR -c bar.bar=cmd config bar.bar
+	} >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&