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

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

[Patrick Steinhardt]

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

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Ævar Arnfjörð Bjarmason]

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;

> +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.

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

[Junio C Hamano]

Æ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.

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?)

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

[Jeff King]

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 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

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

[Patrick Steinhardt]

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

On Mon, Nov 16, 2020 at 09:34:54PM -0500, Jeff King wrote:
> 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).

> > 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

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

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

[Jeff King]

On Tue, Nov 17, 2020 at 07:37:34AM +0100, Patrick Steinhardt 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 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.

> > 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

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

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 17 2020, Jeff King wrote:

> 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

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

[Jeff King]

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

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

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

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 18 2020, Jeff King wrote:

> 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 :)

> 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.

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

[brian m. carlson]

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

On 2020-11-17 at 02:34:54, Jeff King wrote:
> 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.

> > 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

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

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

[Jeff King]

On Wed, Nov 18, 2020 at 12:50:14AM +0000, brian m. carlson 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).
> 
> 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.

> 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.

> 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

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

[brian m. carlson]

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

On 2020-11-18 at 01:59:07, Jeff King wrote:
> 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.

> 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.

>   - 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.

> 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

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

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

[Patrick Steinhardt]

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

On Wed, Nov 18, 2020 at 02:25:32AM +0000, brian m. carlson wrote:
> 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.

> > 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.

> > 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

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

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

[brian m. carlson]

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

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?
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

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

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

[Patrick Steinhardt]

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

On Thu, Nov 19, 2020 at 02:11:16AM +0000, brian m. carlson wrote:
> 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

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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

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

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

[Patrick Steinhardt]

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

On Mon, Nov 16, 2020 at 11:39:35AM -0800, Junio C Hamano wrote:
> Æ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

> 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

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

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

[Junio C Hamano]

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.

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Patrick Steinhardt]

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

On Wed, Nov 18, 2020 at 02:49:39PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> 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

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 16 2020, Junio C Hamano wrote:

> Æ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.

> 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.

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

[Philip Oakley]

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?


> +GIT_CONFIG_KEY_1,GIT_CONFIG_VALUE_1::

Shouldn't the man page entry indicate that it's '<n>'  ?

Philip

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

[Patrick Steinhardt]

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

On Fri, Nov 13, 2020 at 04:37:33PM +0000, Philip Oakley wrote:
> 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

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

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

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 13 2020, Patrick Steinhardt wrote:

> 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.