threads / patch / 54638

patch, 2 partsconfig: allow specifying config entries via envvar pairs

Subject: [PATCH 0/2] config: allow specifying config entries via envvar pairs

## tl;dr

27 messages between Nov 13, 2020 and Nov 19, 2020. Diffs are folded; open one to read it.

replies: 26people: 6as markdown or json

Patrick Steinhardt· Nov 13, 2020, 12:16 UTC · lore
Hi,

this patch series adds a way to specify config entries via separate envvars `GIT_CONFIG_KEY_$n` and `GIT_CONFIG_VALUE_$n`. There's two main motivations:

    1. `GIT_CONFIG_PARAMETERS` is undocumented and requires parsing of
       the key-value pairs. This requires the user to properly escape
       all potentially harmful characters, which may be hard if the
       value is controlled by a third party.
    2. `git -c key=val` is not really suited to contain sensitive
       information, as command line arguments trivially show up in e.g.
       ps(1).
This new way of passing envvars tries to fix both of those shortcomings.
Patrick
Patrick Steinhardt (2):
  config: extract function to parse config pairs
  config: allow specifying config entries via envvar pairs
 Documentation/git-config.txt |  6 ++++
 config.c                     | 65 ++++++++++++++++++++++++++----------
 t/t1300-config.sh            | 23 +++++++++++++
 3 files changed, 76 insertions(+), 18 deletions(-)
-- 
2.29.2
Patrick Steinhardt· Nov 13, 2020, 12:16 UTC · re: Patrick Steinhardt · lore

[PATCH 1/2] config: extract function to parse config pairs

The function `git_config_parse_parameter` is responsible for parsing a `foo.bar=baz`-formatted configuration key, sanitizing the key and then processing it via the given callback function. Given that we're about to add a second user which is going to process keys in such which already has keys and values separated, this commit extracts a function `config_parse_pair` which only does the sanitization and processing part.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 config.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)
Show changes to config.c +17 −7
diff --git a/config.c b/config.c
index 2bdff4457b..3281b1374e 100644
--- a/config.c
+++ b/config.c
@@ -437,11 +437,26 @@ int git_config_key_is_valid(const char *key)
 	return !git_config_parse_key_1(key, NULL, NULL, 1);
 }
 
+static int config_parse_pair(const char *key, const char *value,
+			  config_fn_t fn, void *data)
+{
+	char *canonical_name;
+	int ret;
+
+	if (!strlen(key))
+		return error(_("empty config key"));
+	if (git_config_parse_key(key, &canonical_name, NULL))
+		return -1;
+
+	ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
+	free(canonical_name);
+	return ret;
+}
+
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
 	const char *value;
-	char *canonical_name;
 	struct strbuf **pair;
 	int ret;
 
@@ -462,12 +477,7 @@ int git_config_parse_parameter(const char *text,
 		return error(_("bogus config parameter: %s"), text);
 	}
 
-	if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) {
-		ret = -1;
-	} else {
-		ret = (fn(canonical_name, value, data) < 0) ? -1 : 0;
-		free(canonical_name);
-	}
+	ret = config_parse_pair(pair[0]->buf, value, fn, data);
 	strbuf_list_free(pair);
 	return ret;
 }
-- 
2.29.2
Patrick Steinhardt· Nov 13, 2020, 12:16 UTC · re: Patrick Steinhardt · lore

[PATCH 2/2] config: allow specifying config entries via envvar pairs

While not document, it is currently possible to specify config entries via the environment by passing `GIT_CONFIG_PARAMETERS`. This variable is expected to hold one or multiple "section.key=value" entries separated by space.

Next to being undocumented, this way of passing config entries has a major downside: the config keys need to be parsed. As such, it is left to the user to escape any potentially harmful characters in the value, which is quite hard to do if values are controlled by a third party.

This commit thus adds a new way of adding config entries via the environment which doesn't require splitting of keys and values. The user can specify an config entry's key via `GIT_CONFIG_KEY_$n` and a value via `GIT_CONFIG_VALUE_$n`, where `n` is any number starting with 1. It is possible to add multiple entries via consecutively numbered envvars `GIT_CONFIG_KEY_1`, `GIT_CONFIG_KEY_2`, etc, where each of the keys may have a matching value. When no matching value exists, it's assumed to be the empty value.

While the same can be achieved with `git -c <name>=<value>`, one may wish to not do so for potentially 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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/git-config.txt |  6 ++++++
 config.c                     | 41 ++++++++++++++++++++++++++----------
 t/t1300-config.sh            | 23 ++++++++++++++++++++
 3 files changed, 59 insertions(+), 11 deletions(-)
Show changes to 3 files +59 −11

Documentation/git-config.txt, config.c, t/t1300-config.sh

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7573160f21..83fbac3705 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -335,6 +335,12 @@ GIT_CONFIG_NOSYSTEM::
 	Whether to skip reading settings from the system-wide
 	$(prefix)/etc/gitconfig file. See linkgit:git[1] for details.
 
+GIT_CONFIG_KEY_1,GIT_CONFIG_VALUE_1::
+	Each pair of GIT_CONFIG_KEY_/GIT_CONFIG_VALUE_ is added to the process's
+	runtime configuration. It is possible to add multiple entries by adding
+	consecutively numbered pairs, starting at 1. If the value corresponding
+	to a key is not set, it is treated as if it was empty.
+
 See also <<FILES>>.
 
 
diff --git a/config.c b/config.c
index 3281b1374e..ab40479df2 100644
--- a/config.c
+++ b/config.c
@@ -485,37 +485,56 @@ int git_config_parse_parameter(const char *text,
 int git_config_from_parameters(config_fn_t fn, void *data)
 {
 	const char *env = getenv(CONFIG_DATA_ENVIRONMENT);
+	struct strbuf envvar = STRBUF_INIT;
 	int ret = 0;
-	char *envw;
+	char *envw = NULL;
 	const char **argv = NULL;
 	int nr = 0, alloc = 0;
 	int i;
 	struct config_source source;
 
-	if (!env)
-		return 0;
-
 	memset(&source, 0, sizeof(source));
 	source.prev = cf;
 	source.origin_type = CONFIG_ORIGIN_CMDLINE;
 	cf = &source;
 
-	/* sq_dequote will write over it */
-	envw = xstrdup(env);
+	if (env) {
+		/* sq_dequote will write over it */
+		envw = xstrdup(env);
 
-	if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
-		ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);
-		goto out;
+		if (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {
+			ret = error(_("bogus format in %s"), CONFIG_DATA_ENVIRONMENT);
+			goto out;
+		}
+
+		for (i = 0; i < nr; i++) {
+			if (git_config_parse_parameter(argv[i], fn, data) < 0) {
+				ret = -1;
+				goto out;
+			}
+		}
 	}
 
-	for (i = 0; i < nr; i++) {
-		if (git_config_parse_parameter(argv[i], fn, data) < 0) {
+	for (i = 1; i; i++) {
+		const char *key, *value;
+
+		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
+		if ((key = getenv(envvar.buf)) == NULL)
+			break;
+		strbuf_reset(&envvar);
+
+		strbuf_addf(&envvar, "GIT_CONFIG_VALUE_%d", i);
+		value = getenv(envvar.buf);
+		strbuf_reset(&envvar);
+
+		if (config_parse_pair(key, value, fn, data) < 0) {
 			ret = -1;
 			goto out;
 		}
 	}
 
 out:
+	strbuf_release(&envvar);
 	free(argv);
 	free(envw);
 	cf = source.prev;
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 825d9a184f..2ae9533aa8 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1316,6 +1316,29 @@ test_expect_success 'detect bogus GIT_CONFIG_PARAMETERS' '
 		git config --get-regexp "env.*"
 '
 
+test_expect_success 'git config handles environment config pairs' '
+	GIT_CONFIG_KEY_1="pair.one" GIT_CONFIG_VALUE_1="foo" \
+		GIT_CONFIG_KEY_2="pair.two" GIT_CONFIG_VALUE_2="bar" \
+		GIT_CONFIG_KEY_4="pair.four" GIT_CONFIG_VALUE_4="not-parsed" \
+		git config --get-regexp "pair.*" >actual &&
+	cat >expect <<-EOF &&
+	pair.one foo
+	pair.two bar
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'git config copes with missing config pair value' '
+	GIT_CONFIG_KEY_1="pair.one" git config --get-regexp "pair.*" >actual &&
+	echo pair.one >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git config fails with invalid config pair key' '
+	test_must_fail env GIT_CONFIG_KEY_1= git config --list &&
+	test_must_fail env GIT_CONFIG_KEY_1=missing-section git config --list
+'
+
 test_expect_success 'git config --edit works' '
 	git config -f tmp test.value no &&
 	echo test.value=yes >expect &&
-- 
2.29.2
Ævar Arnfjörð Bjarmason· Nov 13, 2020, 13:04 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Fri, Nov 13 2020, Patrick Steinhardt wrote:
> While not document, it is currently possible to specify config entries
"While not documented..."
> +		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> +		if ((key = getenv(envvar.buf)) == NULL)
> +			break;

The convention in git.git is to avoid explicit NULL checks. So maybe something like this, which also avoids the assignment inside an "if"

    key = getenv(envvar.buf);
    if (!key)
        break;
Show 26 quoted lines
> +test_expect_success 'git config handles environment config pairs' '
> +	GIT_CONFIG_KEY_1="pair.one" GIT_CONFIG_VALUE_1="foo" \
> +		GIT_CONFIG_KEY_2="pair.two" GIT_CONFIG_VALUE_2="bar" \
> +		GIT_CONFIG_KEY_4="pair.four" GIT_CONFIG_VALUE_4="not-parsed" \
> +		git config --get-regexp "pair.*" >actual &&
> +	cat >expect <<-EOF &&
> +	pair.one foo
> +	pair.two bar
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git config copes with missing config pair value' '
> +	GIT_CONFIG_KEY_1="pair.one" git config --get-regexp "pair.*" >actual &&
> +	echo pair.one >expect &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git config fails with invalid config pair key' '
> +	test_must_fail env GIT_CONFIG_KEY_1= git config --list &&
> +	test_must_fail env GIT_CONFIG_KEY_1=missing-section git config --list
> +'
> +
>  test_expect_success 'git config --edit works' '
>  	git config -f tmp test.value no &&
>  	echo test.value=yes >expect &&

I think we should have a bit more complete tests of what happens if you clobber existing config keys, and testing that this is set last after system/global/local config.

Junio C Hamano· Nov 16, 2020, 19:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 16 quoted lines
> On Fri, Nov 13 2020, Patrick Steinhardt wrote:
>
>> While not document, it is currently possible to specify config entries
>
> "While not documented..."
>
>> +		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
>> +		if ((key = getenv(envvar.buf)) == NULL)
>> +			break;
>
> The convention in git.git is to avoid explicit NULL checks. So maybe
> something like this, which also avoids the assignment inside an "if"
>
>     key = getenv(envvar.buf);
>     if (!key)
>         break;
All good suggestions, but...

"While not documented" yes, for sure, but we do not document it for a good reason---it is a pure implementation detail between Git process that runs another one as its internal implementation detail.

I especially do not think we want to read from unbounded number of GIT_CONFIG_KEY_<N> variables like this patch does. How would a script cleanse its environment to protect itself from stray such environment variable pair? Count up from 1 to forever? Run "env" and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No. What if some environment variables have newline in its values?)

Jeff King· Nov 17, 2020, 02:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> >> While not document, it is currently possible to specify config entries
> >> [in GIT_CONFIG_PARAMETERS]
> [...]
>
> "While not documented" yes, for sure, but we do not document it for
> a good reason---it is a pure implementation detail between Git
> process that runs another one as its internal implementation detail.

I have actually been quite tempted to document and promise that it will continue to work. Because it really is useful in some instances. The thing that has held me back is that the documentation would reveal how unforgiving the parser is. ;)

It insists that key/value pairs are shell-quoted as a whole. I think if we made it accept a some reasonable inputs:

  - do not require quoting for values that do not need it
  - allow any amount of shell-style single-quoting (whole parameters,
    just values, etc).
  - do not bother allowing other quoting, like double-quotes with
    backslashes. However, document backslash and double-quote as
    meta-characters that must not appear outside of single-quotes, to
    allow for future expansion.

then I'd feel comfortable making it a public-facing feature. And for most cases it would be pretty pleasant to use (and for the unpleasant ones, I'm not sure that a little quoting is any worse than the paired environment variables found here).

Show 6 quoted lines
> I especially do not think we want to read from unbounded number of
> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> script cleanse its environment to protect itself from stray such
> environment variable pair?  Count up from 1 to forever?  Run "env"
> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> some environment variables have newline in its values?)
Yeah, scripts can currently assume that:
  unset $(git rev-parse --local-env-vars)

will drop any config from the environment. In some cases, having rev-parse enumerate the GIT_CONFIG_KEY_* variables that are set would be sufficient. But that implies that rev-parse is seeing the same environment we're planning to clear. As it is now, a script is free to use rev-parse to generate that list, hold on to it, and then use it later.

-Peff
Patrick Steinhardt· Nov 17, 2020, 06:37 UTC · re: Jeff King · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Mon, Nov 16, 2020 at 09:34:54PM -0500, Jeff King wrote:
Show 32 quoted lines
> On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
> 
> > >> While not document, it is currently possible to specify config entries
> > >> [in GIT_CONFIG_PARAMETERS]
> > [...]
> >
> > "While not documented" yes, for sure, but we do not document it for
> > a good reason---it is a pure implementation detail between Git
> > process that runs another one as its internal implementation detail.
> 
> I have actually been quite tempted to document and promise that it will
> continue to work. Because it really is useful in some instances. The
> thing that has held me back is that the documentation would reveal how
> unforgiving the parser is. ;)
> 
> It insists that key/value pairs are shell-quoted as a whole. I think if
> we made it accept a some reasonable inputs:
> 
>   - do not require quoting for values that do not need it
> 
>   - allow any amount of shell-style single-quoting (whole parameters,
>     just values, etc).
> 
>   - do not bother allowing other quoting, like double-quotes with
>     backslashes. However, document backslash and double-quote as
>     meta-characters that must not appear outside of single-quotes, to
>     allow for future expansion.
> 
> then I'd feel comfortable making it a public-facing feature. And for
> most cases it would be pretty pleasant to use (and for the unpleasant
> ones, I'm not sure that a little quoting is any worse than the paired
> environment variables found here).

I tend to disagree there. As long as you control keys and values yourself it's not too hard, that's true. But as soon as you start processing untrusted keys or values, then it's getting a lot harder.

E.g. suppose you create a fetch mirror for a user, where the source is protected by a password. We don't want to write the password into the gitconfig of the mirror repository. Passing it via `-C` will show up in ps(1). Using GIT_CONFIG_PARAMETERS requires you to quote the value, which contains arbitrary data, and if you fail to do that correctly you now have an avenue for arbitrary config injection.

That scenario is roughly why I came up with the _KEY/_VALUE schema. It requires no quoting, is trivial to set up (at least for its target audience, which is mostly scripts or programs) and wouldn't show up in ps(1).

Show 17 quoted lines
> > I especially do not think we want to read from unbounded number of
> > GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> > script cleanse its environment to protect itself from stray such
> > environment variable pair?  Count up from 1 to forever?  Run "env"
> > and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> > some environment variables have newline in its values?)
> 
> Yeah, scripts can currently assume that:
> 
>   unset $(git rev-parse --local-env-vars)
> 
> will drop any config from the environment. In some cases, having
> rev-parse enumerate the GIT_CONFIG_KEY_* variables that are set would be
> sufficient. But that implies that rev-parse is seeing the same
> environment we're planning to clear. As it is now, a script is free to
> use rev-parse to generate that list, hold on to it, and then use it
> later.

Good point. Adjusting it would be trivial, though: unset all consecutive GIT_CONFIG_KEY_$n keys and potentially also GIT_CONFIG_VALUE_$n until we hit a gap. The parser would stop on the first gap anyway.

Patrick
> -Peff
Jeff King· Nov 17, 2020, 07:01 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Tue, Nov 17, 2020 at 07:37:34AM +0100, Patrick Steinhardt wrote:
Show 20 quoted lines
> > then I'd feel comfortable making it a public-facing feature. And for
> > most cases it would be pretty pleasant to use (and for the unpleasant
> > ones, I'm not sure that a little quoting is any worse than the paired
> > environment variables found here).
> 
> I tend to disagree there. As long as you control keys and values
> yourself it's not too hard, that's true. But as soon as you start
> processing untrusted keys or values, then it's getting a lot harder.
> 
> E.g. suppose you create a fetch mirror for a user, where the source is
> protected by a password. We don't want to write the password into the
> gitconfig of the mirror repository. Passing it via `-C` will show up in
> ps(1). Using GIT_CONFIG_PARAMETERS requires you to quote the value,
> which contains arbitrary data, and if you fail to do that correctly you
> now have an avenue for arbitrary config injection.
> 
> That scenario is roughly why I came up with the _KEY/_VALUE schema. It
> requires no quoting, is trivial to set up (at least for its target
> audience, which is mostly scripts or programs) and wouldn't show up in
> ps(1).

True. Shell-quoting is pretty easy, but it's still a thing that the caller has to do (and get right). Within Git we have nice routines for that, but if you're calling from another program, you probably don't.

Show 14 quoted lines
> > Yeah, scripts can currently assume that:
> > 
> >   unset $(git rev-parse --local-env-vars)
> > 
> > will drop any config from the environment. In some cases, having
> > rev-parse enumerate the GIT_CONFIG_KEY_* variables that are set would be
> > sufficient. But that implies that rev-parse is seeing the same
> > environment we're planning to clear. As it is now, a script is free to
> > use rev-parse to generate that list, hold on to it, and then use it
> > later.
> 
> Good point. Adjusting it would be trivial, though: unset all consecutive
> GIT_CONFIG_KEY_$n keys and potentially also GIT_CONFIG_VALUE_$n until we
> hit a gap. The parser would stop on the first gap anyway.
That doesn't help this case, which currently works:
  # remember the list so we don't have to invoke rev-parse in
  # each iteration of the loop
  vars=$(git rev-parse --local-env-vars)
  for repo in $repos; do
	# start with a clean slate
	unset $vars
	export GIT_DIR=$repo
	git do-some-thing
	# oops, these won't get cleared in the next go-round because
	# they weren't set when we called rev-parse
	export GIT_CONFIG_KEY_1=foo.bar
	export GIT_CONFIG_VALUE_1=whatever
	git do-another-thing
  done

Now I have no idea if anybody is doing something along those lines, and it's a bit convoluted (I'd probably use a subshell). And obviously nobody is doing this _exact_ thing yet, because the key/value variables don't exist yet. So maybe it would all work out (the caller of this script could have set git vars, but in that case we'd pick them up in the rev-parse call).

So I dunno. It's probably unlikely to bite somebody, but it is a departure from the current design.

-Peff
Ævar Arnfjörð Bjarmason· Nov 17, 2020, 14:22 UTC · re: Jeff King · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Tue, Nov 17 2020, Jeff King wrote:
Show 32 quoted lines
> On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
>
>> >> While not document, it is currently possible to specify config entries
>> >> [in GIT_CONFIG_PARAMETERS]
>> [...]
>>
>> "While not documented" yes, for sure, but we do not document it for
>> a good reason---it is a pure implementation detail between Git
>> process that runs another one as its internal implementation detail.
>
> I have actually been quite tempted to document and promise that it will
> continue to work. Because it really is useful in some instances. The
> thing that has held me back is that the documentation would reveal how
> unforgiving the parser is. ;)
>
> It insists that key/value pairs are shell-quoted as a whole. I think if
> we made it accept a some reasonable inputs:
>
>   - do not require quoting for values that do not need it
>
>   - allow any amount of shell-style single-quoting (whole parameters,
>     just values, etc).
>
>   - do not bother allowing other quoting, like double-quotes with
>     backslashes. However, document backslash and double-quote as
>     meta-characters that must not appear outside of single-quotes, to
>     allow for future expansion.
>
> then I'd feel comfortable making it a public-facing feature. And for
> most cases it would be pretty pleasant to use (and for the unpleasant
> ones, I'm not sure that a little quoting is any worse than the paired
> environment variables found here).

I wonder if something like the git config -z format wouldn't be easier, with the twist that we'd obviously not support \0. So we'd need an optional length prefix. : = unspecified.

    :user.name
    Jeff K
    :alias.ci
    commit
    :10:bin.ary
    <10 byte string, might have a \n>
    :other.key
    Other Value

Maybe that's overly fragile, or maybe another format would be better. I was trying to come up with one where the common case wouldn't require knowing about shell quoting/unquoting, and where you could still do:

    GIT_CONFIG_PARAMETERS=":my.new\nvalue\n$GIT_CONFIG_PARAMETERS"

Or equivalent, and still just keep $GIT_CONFIG_PARAMETERS as-is to pass it along.

Your "do not require quoting" accomplishes that, and it's arguably a lot
Jeff King· Nov 17, 2020, 23:57 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Tue, Nov 17, 2020 at 03:22:05PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 19 quoted lines
> > then I'd feel comfortable making it a public-facing feature. And for
> > most cases it would be pretty pleasant to use (and for the unpleasant
> > ones, I'm not sure that a little quoting is any worse than the paired
> > environment variables found here).
> 
> I wonder if something like the git config -z format wouldn't be easier,
> with the twist that we'd obviously not support \0. So we'd need an
> optional length prefix. : = unspecified.
> 
>     :user.name
>     Jeff K
>     :alias.ci
>     commit
>     :10:bin.ary
>     <10 byte string, might have a \n>
>     :other.key
>     Other Value
> 
> Maybe that's overly fragile, or maybe another format would be better.

Yeah, length-delimited strings are an alternative that some people think is less error-prone than quoting. And we do use pkt-lines. They're also a pain for humans to write (it's nicer if they're optional, but when you _do_ have to start using them, now you are stuck counting things up).

Show 10 quoted lines
> I was trying to come up with one where the common case wouldn't
> require knowing about shell quoting/unquoting, and where you could
> still do:
> 
>     GIT_CONFIG_PARAMETERS=":my.new\nvalue\n$GIT_CONFIG_PARAMETERS"
> 
> Or equivalent, and still just keep $GIT_CONFIG_PARAMETERS as-is to pass
> it along.
> 
> Your "do not require quoting" accomplishes that, and it's arguably a lot

Looks like your mail got cut off. But yeah, the goal of making the quoting optional was to make it easier for humans to use for simple cases. It doesn't help at all with other programs inserting values, which can just as easily err on the side of caution.

BTW, there is another problem with GIT_CONFIG_PARAMETERS (and "git -c" in general). The dotted config-key format:

  section.subsection.key

is unambiguous by itself, even though "subsection" can contain arbitrary bytes, including dots. Because neither "section" nor "key" can contain dots, we can parse from either end, and take the whole middle as a subsection (and this is how we do it in the code).

But an assignment string like:
  section.subsection.key=value

_is_ ambiguous. We have to parse left-to-right up to the first equals (since "value" can contain arbitrary characters, including an equals). But "subsection" can have one, too, so we want to parse right-to-left there. E.g., in:

  one.two=three.four=five
this could be either of:
  - section is "one", key is "two", value is "three.four=five"
  - section is "one", subsection is "two=three", key is "four", value is
    "five"

We currently always parse it as the former (which I think is least-bad of the two, since values are more likely than subsections to contain arbitrary text with an equals).

-Peff
Ævar Arnfjörð Bjarmason· Nov 18, 2020, 13:44 UTC · re: Jeff King · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Wed, Nov 18 2020, Jeff King wrote:
Show 39 quoted lines
> On Tue, Nov 17, 2020 at 03:22:05PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > then I'd feel comfortable making it a public-facing feature. And for
>> > most cases it would be pretty pleasant to use (and for the unpleasant
>> > ones, I'm not sure that a little quoting is any worse than the paired
>> > environment variables found here).
>> 
>> I wonder if something like the git config -z format wouldn't be easier,
>> with the twist that we'd obviously not support \0. So we'd need an
>> optional length prefix. : = unspecified.
>> 
>>     :user.name
>>     Jeff K
>>     :alias.ci
>>     commit
>>     :10:bin.ary
>>     <10 byte string, might have a \n>
>>     :other.key
>>     Other Value
>> 
>> Maybe that's overly fragile, or maybe another format would be better.
>
> Yeah, length-delimited strings are an alternative that some people think
> is less error-prone than quoting. And we do use pkt-lines. They're also
> a pain for humans to write (it's nicer if they're optional, but when you
> _do_ have to start using them, now you are stuck counting things up).
>
>> I was trying to come up with one where the common case wouldn't
>> require knowing about shell quoting/unquoting, and where you could
>> still do:
>> 
>>     GIT_CONFIG_PARAMETERS=":my.new\nvalue\n$GIT_CONFIG_PARAMETERS"
>> 
>> Or equivalent, and still just keep $GIT_CONFIG_PARAMETERS as-is to pass
>> it along.
>> 
>> Your "do not require quoting" accomplishes that, and it's arguably a lot
>n
> Looks like your mail got cut off.
Nothing important, probably :)
Show 36 quoted lines
> But yeah, the goal of making the quoting optional was to make it
> easier for humans to use for simple cases. It doesn't help at all with
> other programs inserting values, which can just as easily err on the
> side of caution.
>
> BTW, there is another problem with GIT_CONFIG_PARAMETERS (and "git -c"
> in general). The dotted config-key format:
>
>   section.subsection.key
>
> is unambiguous by itself, even though "subsection" can contain arbitrary
> bytes, including dots. Because neither "section" nor "key" can contain
> dots, we can parse from either end, and take the whole middle as a
> subsection (and this is how we do it in the code).
>
> But an assignment string like:
>
>   section.subsection.key=value
>
> _is_ ambiguous. We have to parse left-to-right up to the first equals
> (since "value" can contain arbitrary characters, including an equals).
> But "subsection" can have one, too, so we want to parse right-to-left
> there. E.g., in:
>
>   one.two=three.four=five
>
> this could be either of:
>
>   - section is "one", key is "two", value is "three.four=five"
>
>   - section is "one", subsection is "two=three", key is "four", value is
>     "five"
>
> We currently always parse it as the former (which I think is least-bad
> of the two, since values are more likely than subsections to contain
> arbitrary text with an equals).

Yeah, it's a pain to parse if it's on one line. FWIW that's the main reason for why the format I suggested moved it to \n-delimited, because keys can't contain an \n, so you can unambiguously have them be \n-delimited (as git config -z does).

You do need to worry about a \n in the value, but for the common case where you don't have a \n there we wouldn't need to provide the length.

Or just provide tooling as you suggested in <20201118015907.GD650959@coredump.intra.peff.net>, which I like better than any one format suggestion (including the one I suggsted). I.e. we can document that:

 - The variable exists
 - You read/write/add to it using a return value from this tool

Which allows for keeping the value itself opaque and open to a future change.

brian m. carlson· Nov 18, 2020, 00:50 UTC · re: Jeff King · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On 2020-11-17 at 02:34:54, Jeff King wrote:
Show 32 quoted lines
> On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
> 
> > >> While not document, it is currently possible to specify config entries
> > >> [in GIT_CONFIG_PARAMETERS]
> > [...]
> >
> > "While not documented" yes, for sure, but we do not document it for
> > a good reason---it is a pure implementation detail between Git
> > process that runs another one as its internal implementation detail.
> 
> I have actually been quite tempted to document and promise that it will
> continue to work. Because it really is useful in some instances. The
> thing that has held me back is that the documentation would reveal how
> unforgiving the parser is. ;)
> 
> It insists that key/value pairs are shell-quoted as a whole. I think if
> we made it accept a some reasonable inputs:
> 
>   - do not require quoting for values that do not need it
> 
>   - allow any amount of shell-style single-quoting (whole parameters,
>     just values, etc).
> 
>   - do not bother allowing other quoting, like double-quotes with
>     backslashes. However, document backslash and double-quote as
>     meta-characters that must not appear outside of single-quotes, to
>     allow for future expansion.
> 
> then I'd feel comfortable making it a public-facing feature. And for
> most cases it would be pretty pleasant to use (and for the unpleasant
> ones, I'm not sure that a little quoting is any worse than the paired
> environment variables found here).

What if we didn't document it but provided a command that produced a suitable value? Maybe something like this:

  GIT_CONFIG_PARAMETERS=$(git rev-parse --quote-parameters a.b.c ENV_VAR d.e.f OTHER_ENV_VAR)
Or whatever we decide.

I don't personally love shell quoting as an interchange mechanism; I'd prefer something like URI-encoding, which is a bit more standardized and easier to reason about from a security perspective. But if we decide to change it, it doesn't matter, since it's still undocumented and this would be the only acceptable way to pass config through the environment.

Alternatively, we could just do this:
  git with-config --key a.b.c --value ENV_VAR --key d.e.f --value OTHER_ENV_VAR --command git foo
That would also leave it undocumented, but make it easier to work with.
Show 17 quoted lines
> > I especially do not think we want to read from unbounded number of
> > GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> > script cleanse its environment to protect itself from stray such
> > environment variable pair?  Count up from 1 to forever?  Run "env"
> > and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> > some environment variables have newline in its values?)
> 
> Yeah, scripts can currently assume that:
> 
>   unset $(git rev-parse --local-env-vars)
> 
> will drop any config from the environment. In some cases, having
> rev-parse enumerate the GIT_CONFIG_KEY_* variables that are set would be
> sufficient. But that implies that rev-parse is seeing the same
> environment we're planning to clear. As it is now, a script is free to
> use rev-parse to generate that list, hold on to it, and then use it
> later.

I'm also uncomfortable with an arbitrary number of keys and values. It becomes very tricky to cleanse the environment, and even if the code stops at the first gap, if you then add more entries, then you have to cleanse again or risk a security problem. I feel like this is only going to bite us in the future.

-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US
Jeff King· Nov 18, 2020, 01:59 UTC · re: brian m. carlson · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Wed, Nov 18, 2020 at 12:50:14AM +0000, brian m. carlson wrote:
Show 11 quoted lines
> > then I'd feel comfortable making it a public-facing feature. And for
> > most cases it would be pretty pleasant to use (and for the unpleasant
> > ones, I'm not sure that a little quoting is any worse than the paired
> > environment variables found here).
> 
> What if we didn't document it but provided a command that produced a
> suitable value?  Maybe something like this:
> 
>   GIT_CONFIG_PARAMETERS=$(git rev-parse --quote-parameters a.b.c ENV_VAR d.e.f OTHER_ENV_VAR)
> 
> Or whatever we decide.
I think we mostly already have that tooling.
  $ GIT_CONFIG_PARAMETERS=$(
      git rev-parse --sq-quote \
        foo.bar=value \
        'section.key=with spaces' \
        "or.even=embedded 'quotes'" |
      sed 's/^ //'; # yuck
    )
  $ export GIT_CONFIG_PARAMETERS
  $ git config --list --show-scope | grep ^command
  command	foo.bar=value
  command	section.key=with spaces
  command	or.even=embedded 'quotes'

The "yuck" there is because --sq-quote insists on putting a space at the front. Our parser should probably ignore that, but right now it's quite picky.

Though I suppose:
  - do you mean ENV_VAR to literally look up an environment variable?
    That solves Patrick's original problem of not wanting to put
    sensitive values onto the command line. But it's much more annoying
    to use if you _don't_ already have the value in an environment
    variable. I'm not sure if that original problem matters here, as a
    program that does a lot of this would probably implement the quoting
    itself.
  - obviously it is doubling down on the shell-quoting as a public
    thing, and part of your point is that we would leave it opaque.
    I'm OK with that aspect (even if it ends up as an alias for
    --sq-quote for now). I'm not entirely sure people aren't using this
    in the wild already, though. Saying "it was undocumented" may give
    us a leg to stand on if we change the format, but it will still be
    annoying to people we break.
  - my example above still has the "a.b=c.d=e" ambiguity that I
    mentioned earlier. Fixing that requires changing the format in the
    environment variable.
Show 5 quoted lines
> I don't personally love shell quoting as an interchange mechanism; I'd
> prefer something like URI-encoding, which is a bit more standardized and
> easier to reason about from a security perspective.  But if we decide to
> change it, it doesn't matter, since it's still undocumented and this
> would be the only acceptable way to pass config through the environment.

Yes, I think concatenating uri_encode(key) + "=" + uri_encode(value) would be easier to reason about, and solves the ambiguity problem. If we are willing to break from the existing format, it's probably a reasonable direction.

Show 5 quoted lines
> Alternatively, we could just do this:
> 
>   git with-config --key a.b.c --value ENV_VAR --key d.e.f --value OTHER_ENV_VAR --command git foo
> 
> That would also leave it undocumented, but make it easier to work with.
I wondered at first how this is different from:
  git -c a.b.c=value foo

but I guess it is meant to do the same environment-lookup? We could probably add:

  git --env-config a.b.c=ENV_VAR foo

to have Git internally stick $ENV_VAR into $GIT_CONFIG_PARAMETERS. That avoids all of the rev-parse rigamarole, keeps the format of the environment variable opaque, and solves Patrick's problem of putting the value onto the command-line.

It doesn't solve the ambiguity with "=" in the subsection, but IMHO that is not all that important a problem.

-Peff
brian m. carlson· Nov 18, 2020, 02:25 UTC · re: Jeff King · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On 2020-11-18 at 01:59:07, Jeff King wrote:
Show 18 quoted lines
> I think we mostly already have that tooling.
> 
>   $ GIT_CONFIG_PARAMETERS=$(
>       git rev-parse --sq-quote \
>         foo.bar=value \
>         'section.key=with spaces' \
>         "or.even=embedded 'quotes'" |
>       sed 's/^ //'; # yuck
>     )
>   $ export GIT_CONFIG_PARAMETERS
>   $ git config --list --show-scope | grep ^command
>   command	foo.bar=value
>   command	section.key=with spaces
>   command	or.even=embedded 'quotes'
> 
> The "yuck" there is because --sq-quote insists on putting a space at the
> front. Our parser should probably ignore that, but right now it's quite
> picky.
I feel that this approach leaves a few things to be desired, yes.
Show 9 quoted lines
> Though I suppose:
> 
>   - do you mean ENV_VAR to literally look up an environment variable?
>     That solves Patrick's original problem of not wanting to put
>     sensitive values onto the command line. But it's much more annoying
>     to use if you _don't_ already have the value in an environment
>     variable. I'm not sure if that original problem matters here, as a
>     program that does a lot of this would probably implement the quoting
>     itself.

Yeah, I did mean that, or various options to either read a literal value or from the environment. Your proposal, while it obviously works, doesn't solve Patrick's problem.

Show 7 quoted lines
>   - obviously it is doubling down on the shell-quoting as a public
>     thing, and part of your point is that we would leave it opaque.
>     I'm OK with that aspect (even if it ends up as an alias for
>     --sq-quote for now). I'm not entirely sure people aren't using this
>     in the wild already, though. Saying "it was undocumented" may give
>     us a leg to stand on if we change the format, but it will still be
>     annoying to people we break.

I've been telling people for some time that Git doesn't have a way to do this, so I'm comfortable sticking with that line until we have a documented way to do it. I knew we had an internal environment variable (because I was curious and looked), but I knew from just looking at it that it was internal.

> Yes, I think concatenating uri_encode(key) + "=" + uri_encode(value)
> would be easier to reason about, and solves the ambiguity problem. If we
> are willing to break from the existing format, it's probably a
> reasonable direction.

We'll have to deal with embedded NULs, but other than that it seems robust and easy to reason about. I like the proposal below better, though.

Show 13 quoted lines
> I wondered at first how this is different from:
> 
>   git -c a.b.c=value foo
> 
> but I guess it is meant to do the same environment-lookup? We could
> probably add:
> 
>   git --env-config a.b.c=ENV_VAR foo
> 
> to have Git internally stick $ENV_VAR into $GIT_CONFIG_PARAMETERS. That
> avoids all of the rev-parse rigamarole, keeps the format of the
> environment variable opaque, and solves Patrick's problem of putting the
> value onto the command-line.

Sure, that could be an option. It's the simplest, and we already know how to handle config this way. People will be able to figure out how to use it pretty easily.

> It doesn't solve the ambiguity with "=" in the subsection, but IMHO that
> is not all that important a problem.

I'm fine with saying that we don't support environment variable names with equals signs and just going with that. It solves the ambiguity problem and I'm not sure that they could usefully work on Unix anyway.

Moreover, people usually use the unrestricted data in the second chunk for URLs, which shouldn't have unquoted equals signs. You could technically name other second fields that way, but why would you when you could just not?

So I'm not too worried about it either way.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US
Patrick Steinhardt· Nov 18, 2020, 07:04 UTC · re: brian m. carlson · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Wed, Nov 18, 2020 at 02:25:32AM +0000, brian m. carlson wrote:
Show 9 quoted lines
> On 2020-11-18 at 01:59:07, Jeff King wrote:
> > Yes, I think concatenating uri_encode(key) + "=" + uri_encode(value)
> > would be easier to reason about, and solves the ambiguity problem. If we
> > are willing to break from the existing format, it's probably a
> > reasonable direction.
> 
> We'll have to deal with embedded NULs, but other than that it seems
> robust and easy to reason about.  I like the proposal below better,
> though.

This would be easy enough to handle and fixes all the problems I currently have.

Show 17 quoted lines
> > I wondered at first how this is different from:
> > 
> >   git -c a.b.c=value foo
> > 
> > but I guess it is meant to do the same environment-lookup? We could
> > probably add:
> > 
> >   git --env-config a.b.c=ENV_VAR foo
> > 
> > to have Git internally stick $ENV_VAR into $GIT_CONFIG_PARAMETERS. That
> > avoids all of the rev-parse rigamarole, keeps the format of the
> > environment variable opaque, and solves Patrick's problem of putting the
> > value onto the command-line.
> 
> Sure, that could be an option.  It's the simplest, and we already know
> how to handle config this way.  People will be able to figure out how to
> use it pretty easily.

At first, this idea sounds quite interesting. But only until one realizes that it's got the exact same problem which I'm trying to solve: there's still a point in time where one can observe config values via the command line, even though that moment now is a lot shorter compared to running the "real" git command with those keys.

Show 13 quoted lines
> > It doesn't solve the ambiguity with "=" in the subsection, but IMHO that
> > is not all that important a problem.
> 
> I'm fine with saying that we don't support environment variable names
> with equals signs and just going with that.  It solves the ambiguity
> problem and I'm not sure that they could usefully work on Unix anyway.
> 
> Moreover, people usually use the unrestricted data in the second chunk
> for URLs, which shouldn't have unquoted equals signs.  You could
> technically name other second fields that way, but why would you when
> you could just not?
> 
> So I'm not too worried about it either way.

There's not only URLs though, but also paths in 'includeIf.*.path'. Sure, it's unlikely that paths have an equals sign embedded. But I think we should try to not paint ourselves into a corner.

This problem also would be nicely solved by URI-encoding both key and value and then passing it via GIT_CONFIG_PARAMETER.

Patrick
brian m. carlson· Nov 19, 2020, 02:11 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On 2020-11-18 at 07:04:30, Patrick Steinhardt wrote:
Show 10 quoted lines
> On Wed, Nov 18, 2020 at 02:25:32AM +0000, brian m. carlson wrote:
> > Sure, that could be an option.  It's the simplest, and we already know
> > how to handle config this way.  People will be able to figure out how to
> > use it pretty easily.
> 
> At first, this idea sounds quite interesting. But only until one
> realizes that it's got the exact same problem which I'm trying to solve:
> there's still a point in time where one can observe config values via
> the command line, even though that moment now is a lot shorter compared
> to running the "real" git command with those keys.
I don't think that's the case.  This command:
  git --env-config a.b.c=ENV_VAR
would be equivalent to this shell command:
  git -c "a.b.c=$ENV_VAR"

In other words, ENV_VAR is the _name_ of a environment variable to read for the config value. Subprocesses would inherit it using the undocumented GIT_CONFIG_PARAMETERS.

Or are you trying to hide the configuration key as well?
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US
Patrick Steinhardt· Nov 19, 2020, 06:37 UTC · re: brian m. carlson · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Thu, Nov 19, 2020 at 02:11:16AM +0000, brian m. carlson wrote:
Show 25 quoted lines
> On 2020-11-18 at 07:04:30, Patrick Steinhardt wrote:
> > On Wed, Nov 18, 2020 at 02:25:32AM +0000, brian m. carlson wrote:
> > > Sure, that could be an option.  It's the simplest, and we already know
> > > how to handle config this way.  People will be able to figure out how to
> > > use it pretty easily.
> > 
> > At first, this idea sounds quite interesting. But only until one
> > realizes that it's got the exact same problem which I'm trying to solve:
> > there's still a point in time where one can observe config values via
> > the command line, even though that moment now is a lot shorter compared
> > to running the "real" git command with those keys.
> 
> I don't think that's the case.  This command:
> 
>   git --env-config a.b.c=ENV_VAR
> 
> would be equivalent to this shell command:
> 
>   git -c "a.b.c=$ENV_VAR"
> 
> In other words, ENV_VAR is the _name_ of a environment variable to read
> for the config value.  Subprocesses would inherit it using the
> undocumented GIT_CONFIG_PARAMETERS.
> 
> Or are you trying to hide the configuration key as well?

No. I just didn't realize that it's supposed to be the name of an envvar.

Patrick
Junio C Hamano· Nov 18, 2020, 05:44 UTC · re: brian m. carlson · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 5 quoted lines
> I don't personally love shell quoting as an interchange mechanism; I'd
> prefer something like URI-encoding, which is a bit more standardized and
> easier to reason about from a security perspective.  But if we decide to
> change it, it doesn't matter, since it's still undocumented and this
> would be the only acceptable way to pass config through the environment.
Surely.  
I am kind-of surprised that nobody has brought up json or yaml ;-)
Patrick Steinhardt· Nov 17, 2020, 06:28 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
Show 24 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Fri, Nov 13 2020, Patrick Steinhardt wrote:
> >
> >> While not document, it is currently possible to specify config entries
> >
> > "While not documented..."
> >
> >> +		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
> >> +		if ((key = getenv(envvar.buf)) == NULL)
> >> +			break;
> >
> > The convention in git.git is to avoid explicit NULL checks. So maybe
> > something like this, which also avoids the assignment inside an "if"
> >
> >     key = getenv(envvar.buf);
> >     if (!key)
> >         break;
> 
> All good suggestions, but...
> 
> "While not documented" yes, for sure, but we do not document it for
> a good reason---it is a pure implementation detail between Git
> process that runs another one as its internal implementation detail.
Agreed
Show 6 quoted lines
> I especially do not think we want to read from unbounded number of
> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> script cleanse its environment to protect itself from stray such
> environment variable pair?  Count up from 1 to forever?  Run "env"
> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> some environment variables have newline in its values?)

You only have to unset `GIT_CONFIG_KEY_1` as the parser will stop iterating on the first missing key. More generally, if you set `n` keys, it's sufficient to unset key `n+1`.

Patrick
Junio C Hamano· Nov 17, 2020, 07:06 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

Patrick Steinhardt <ps@pks.im> writes:
Show 10 quoted lines
>> I especially do not think we want to read from unbounded number of
>> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
>> script cleanse its environment to protect itself from stray such
>> environment variable pair?  Count up from 1 to forever?  Run "env"
>> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
>> some environment variables have newline in its values?)
>
> You only have to unset `GIT_CONFIG_KEY_1` as the parser will stop
> iterating on the first missing key. More generally, if you set `n` keys,
> it's sufficient to unset key `n+1`.

Yes, but those who want to set N keys would likely to be content with setting 1..N and happily forget unsetting N+1, and that is where "how would one cleanse the environment to give a clean slate?" comes from.

Ævar Arnfjörð Bjarmason· Nov 18, 2020, 13:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Tue, Nov 17 2020, Junio C Hamano wrote:
Show 17 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
>
>>> I especially do not think we want to read from unbounded number of
>>> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
>>> script cleanse its environment to protect itself from stray such
>>> environment variable pair?  Count up from 1 to forever?  Run "env"
>>> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
>>> some environment variables have newline in its values?)
>>
>> You only have to unset `GIT_CONFIG_KEY_1` as the parser will stop
>> iterating on the first missing key. More generally, if you set `n` keys,
>> it's sufficient to unset key `n+1`.
>
> Yes, but those who want to set N keys would likely to be content
> with setting 1..N and happily forget unsetting N+1, and that is
> where "how would one cleanse the environment to give a clean slate?"
> comes from.

Not as an argument from whataboutism, but just to note a bug/existing prior art:

Nobody in this thread has mentioned GIT_PUSH_OPTION_* which works pretty much like Patrick's suggestion, and it looks like --local-env-vars misses those:

    $ GIT_PUSH_OPTION_0=foo GIT_PUSH_OPTION_COUNT=20 git rev-parse --local-env-vars | grep GIT_PUSH
    $

I haven't tested this, but I expect there's a bug where a push hook itself does a local push to another repo and that repo has a hook, that the push options are erroneously carried forward to the sub-process.

That might also be a feature, depending on your point of view.
Patrick Steinhardt· Nov 18, 2020, 13:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Wed, Nov 18, 2020 at 02:49:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 36 quoted lines
> 
> On Tue, Nov 17 2020, Junio C Hamano wrote:
> 
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> >>> I especially do not think we want to read from unbounded number of
> >>> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> >>> script cleanse its environment to protect itself from stray such
> >>> environment variable pair?  Count up from 1 to forever?  Run "env"
> >>> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> >>> some environment variables have newline in its values?)
> >>
> >> You only have to unset `GIT_CONFIG_KEY_1` as the parser will stop
> >> iterating on the first missing key. More generally, if you set `n` keys,
> >> it's sufficient to unset key `n+1`.
> >
> > Yes, but those who want to set N keys would likely to be content
> > with setting 1..N and happily forget unsetting N+1, and that is
> > where "how would one cleanse the environment to give a clean slate?"
> > comes from.
> 
> Not as an argument from whataboutism, but just to note a bug/existing
> prior art:
> 
> Nobody in this thread has mentioned GIT_PUSH_OPTION_* which works pretty
> much like Patrick's suggestion, and it looks like --local-env-vars
> misses those:
> 
>     $ GIT_PUSH_OPTION_0=foo GIT_PUSH_OPTION_COUNT=20 git rev-parse --local-env-vars | grep GIT_PUSH
>     $
> 
> I haven't tested this, but I expect there's a bug where a push hook
> itself does a local push to another repo and that repo has a hook, that
> the push options are erroneously carried forward to the sub-process.
> 
> That might also be a feature, depending on your point of view.
I didn't actually know about it, thanks for pointing it out!

If we're going to use the same `_COUNT` approach, then I think the issues which were discussed would mostly go away. No gaps needed, no requirement to unset `$n+1`. Any properly behaving user would know to set it as otherwise the written code/script cannot work. Also, git-rev-parse(1) wouldn't have to dynamically adjust based on whether a previously existing gap was filled with new keys or not.

I'd be happy to pursue that road, but I'll wait for some feedback first.
Patrick
Junio C Hamano· Nov 18, 2020, 16:01 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 10 quoted lines
> Nobody in this thread has mentioned GIT_PUSH_OPTION_* which works pretty
> much like Patrick's suggestion, and it looks like --local-env-vars
> misses those:
>
>     $ GIT_PUSH_OPTION_0=foo GIT_PUSH_OPTION_COUNT=20 git rev-parse --local-env-vars | grep GIT_PUSH
>     $
>
> I haven't tested this, but I expect there's a bug where a push hook
> itself does a local push to another repo and that repo has a hook, that
> the push options are erroneously carried forward to the sub-process.
True.

Nobody mentioned the environment variable in the discussion, and nobody discovered and was motivated enough to report and/or fix it, may be a good indication that these variables are not much used in real life and certainly not in combination with hooks that further push things out.

Ævar Arnfjörð Bjarmason· Nov 17, 2020, 14:03 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Mon, Nov 16 2020, Junio C Hamano wrote:
Show 24 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Nov 13 2020, Patrick Steinhardt wrote:
>>
>>> While not document, it is currently possible to specify config entries
>>
>> "While not documented..."
>>
>>> +		strbuf_addf(&envvar, "GIT_CONFIG_KEY_%d", i);
>>> +		if ((key = getenv(envvar.buf)) == NULL)
>>> +			break;
>>
>> The convention in git.git is to avoid explicit NULL checks. So maybe
>> something like this, which also avoids the assignment inside an "if"
>>
>>     key = getenv(envvar.buf);
>>     if (!key)
>>         break;
>
> All good suggestions, but...
>
> "While not documented" yes, for sure, but we do not document it for
> a good reason---it is a pure implementation detail between Git
> process that runs another one as its internal implementation detail.
*nod* I didn't mean it should be treated as some API, just "it happens
 to work". I do agree with Jeff downthread that it would be nice to have
 it explicitly supported.
Show 6 quoted lines
> I especially do not think we want to read from unbounded number of
> GIT_CONFIG_KEY_<N> variables like this patch does.  How would a
> script cleanse its environment to protect itself from stray such
> environment variable pair?  Count up from 1 to forever?  Run "env"
> and grep for "GIT_CONFIG_KEY_[0-9]*=" (the answer is No.  What if
> some environment variables have newline in its values?)

Purely on an implementation note, if we went that route we could provide something based on compat/unsetenv.c (or environ iteration in general) that would loop over the env, but I agree it would be better to make GIT_CONFIG_PARAMETERS nice.

Philip Oakley· Nov 13, 2020, 16:37 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On 13/11/2020 12:16, Patrick Steinhardt wrote:
Show 7 quoted lines
> This commit thus adds a new way of adding config entries via the
> environment which doesn't require splitting of keys and values. The user
> can specify an config entry's key via `GIT_CONFIG_KEY_$n` and a value
> via `GIT_CONFIG_VALUE_$n`, where `n` is any number starting with 1. It
> is possible to add multiple entries via consecutively numbered envvars
> `GIT_CONFIG_KEY_1`, `GIT_CONFIG_KEY_2`, etc, where each of the keys may
> have a matching value. 
> When no matching value exists, it's assumed to be
> the empty value.

Is this a good choice of default in the face of potential mistyping when entering commands, or cut&paste editing of scripts. It's easy to see cases of mismatched KEY_2 VALUE_1 entries.

Wouldn't it be better to warn about un-matched key/value pairs?
> +GIT_CONFIG_KEY_1,GIT_CONFIG_VALUE_1::
Shouldn't the man page entry indicate that it's '<n>'  ?
Philip
Patrick Steinhardt· Nov 17, 2020, 06:40 UTC · re: Philip Oakley · lore

Re: [PATCH 2/2] config: allow specifying config entries via envvar pairs

On Fri, Nov 13, 2020 at 04:37:33PM +0000, Philip Oakley wrote:
Show 16 quoted lines
> On 13/11/2020 12:16, Patrick Steinhardt wrote:
> > This commit thus adds a new way of adding config entries via the
> > environment which doesn't require splitting of keys and values. The user
> > can specify an config entry's key via `GIT_CONFIG_KEY_$n` and a value
> > via `GIT_CONFIG_VALUE_$n`, where `n` is any number starting with 1. It
> > is possible to add multiple entries via consecutively numbered envvars
> > `GIT_CONFIG_KEY_1`, `GIT_CONFIG_KEY_2`, etc, where each of the keys may
> > have a matching value. 
> 
> > When no matching value exists, it's assumed to be
> > the empty value.
> Is this a good choice of default in the face of potential mistyping when
> entering commands, or cut&paste editing of scripts. It's easy to see
> cases of mismatched KEY_2 VALUE_1 entries.
> 
> Wouldn't it be better to warn about un-matched key/value pairs?
Good point. I'll change this on the next iteration.
> > +GIT_CONFIG_KEY_1,GIT_CONFIG_VALUE_1::
> 
> Shouldn't the man page entry indicate that it's '<n>'  ?

I wasn't quite sure how to document it, but using `<n>` would indicate better that this can be multiple envvars.

Patrick
> Philip
Ævar Arnfjörð Bjarmason· Nov 13, 2020, 13:11 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 0/2] config: allow specifying config entries via envvar pairs

On Fri, Nov 13 2020, Patrick Steinhardt wrote:
Show 12 quoted lines
> this patch series adds a way to specify config entries via separate
> envvars `GIT_CONFIG_KEY_$n` and `GIT_CONFIG_VALUE_$n`. There's two main
> motivations:
>
>     1. `GIT_CONFIG_PARAMETERS` is undocumented and requires parsing of
>        the key-value pairs. This requires the user to properly escape
>        all potentially harmful characters, which may be hard if the
>        value is controlled by a third party.
>
>     2. `git -c key=val` is not really suited to contain sensitive
>        information, as command line arguments trivially show up in e.g.
>        ps(1).

FWIW we had an off-list discussion about this where the desire was to have the equivalent of a transitory password in a config file without the bad pattern of putting it in an on-disk config file. The advertised solution we have now is core.askpass, but a user might for some reason not want the hassle of an external program.

I noted that you can do that with some clever hacks that aren't explicitly documented:

1) Use the insteadOf config to on-the-fly rewrite a password-less https
   URL to have a user/password:
    git -c url.https://user:password@.insteadOf=https:// push
   But that has the downside of showing the password in "ps" as Patrick
   notes. That's OS dependant, but is the default on e.g. Linux, as
   opposed to envars. See "hidepid" in the "procfs" manpage.
2) Doing the same via an env var, but via GIT_CONFIG_PARAMETERS:
    GIT_CONFIG_PARAMETERS="'url.https://user:password@.insteadOf=https://'" git push
3) This doesn't work, but I wish it did. First put:
    [include]
    path = /dev/fd/321
   In your .git/config. Then:
    (echo "[url \"https://user:password\"]" && echo "insteadOf = https://") | { git remote get-url origin; } 321<&0
   The reason it doesn't work is because the "git remote" config
   machinery, unlike the general machinery, explicitly doesn't handle
   includes. I didn't poke at that for long, but I expect that's just an
   omission. It wants to not read remote.origin.url from ~/.gitconfig or
   whatever, but I don't see why we wouldn't follow includes in
   .git/config.

← back to recent threads