[1/1] Git config allows value setting from stdin

Commit Message

Zeger-Jan van de Weg Sept. 17, 2019, 1:31 p.m. UTC

When setting values in the git config, the value is part of the
arguments for execution. This potentially leaks the value through
logging, or other programs like `ps`.

Prior to this change, there was no option to do this. This change adds
the `--stdin` to be combined with `--add`. When passed, the value cannot
be passed and is read through stdin.

Signed-off-by: Zeger-Jan van de Weg <[e-mail]>
---
 Documentation/git-config.txt |  5 ++++-
 builtin/config.c             | 23 +++++++++++++++++++++--
 t/t1300-config.sh            | 11 +++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Sept. 17, 2019, 4:59 p.m. UTC | #1

Zeger-Jan van de Weg <[e-mail]> writes:

> Subject: Re: [PATCH 1/1] Git config allows value setting from stdin

Probably something like this, with the "<area>:" prefix.

	config: learn the --stdin option to take args from the standard input

> When setting values in the git config, the value is part of the
> arguments for execution. This potentially leaks the value through
> logging, or other programs like `ps`.

OK.

> Prior to this change, there was no option to do this. This change adds
> the `--stdin` to be combined with `--add`. When passed, the value cannot
> be passed and is read through stdin.

That's overly verbose.

	Add the `--stdin` option that can be used with `--add` to
	instead take the variables and values from the standard
	input to hide them from prying eyes.

or something?  When you say "Add", we know there isn't any right now
(that is why you are adding, after all).

Also, shouldn't the variable also be considered sensitive?  IOW

	$ git config --stdin --add <<\EOF
	remote.hidden.url=https://user:[e-mail]/repo
	EOF

instead of

	$ git config --stdin --add remote.hidden.url <<\EOF
	https://user:[e-mail]/repo
	EOF

?  

Incidentally, allowing it to take variable=value pair would also
allow you to set many of them in batch, which is another benefit.

> -'git config' [<file-option>] [--type=<type>] --add name value
> +'git config' [<file-option>] [--type=<type>] --add [--stdin] name [value]

This does not convey "you pass name and value without --stdin, or
you pass only name with --stdin" and instead allow a nonsense like
"git config --add name".  Splitting it into two would be a way to
tell this unambiguously to the readers, e.g. 

	git config [--type=<type>] --add name value
	git config [--type=<type>] --add --stdin name

although I suspect we would also want to allow treating the varilabe
names as sensitive.

Thanks.

Patch

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index ff9310f958..e6e7f6b60b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -10,7 +10,7 @@  SYNOPSIS
 --------
 [verse]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] name [value [value_regex]]
-'git config' [<file-option>] [--type=<type>] --add name value
+'git config' [<file-option>] [--type=<type>] --add [--stdin] name [value]
 'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get-all name [value_regex]
@@ -212,6 +212,9 @@  Valid `<type>`'s include:
 	output without getting confused e.g. by values that
 	contain line breaks.
 
+--stdin::
+	Instead passing in the value as argument, read the value from stdin.
+
 --name-only::
 	Output only the names of config variables for `--list` or
 	`--get-regexp`.
diff --git a/builtin/config.c b/builtin/config.c
index 98d65bc0ad..ea6fb0168c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -30,6 +30,7 @@  static struct git_config_source given_config_source;
 static int actions, type;
 static char *default_value;
 static int end_null;
+static int stdin_input;
 static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
@@ -152,6 +153,7 @@  static struct option builtin_config_options[] = {
 	OPT_CALLBACK_VALUE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
+	OPT_BOOL(0, "stdin", &stdin_input, N_("allow input from stdin when setting a value")),
 	OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
 	OPT_BOOL(0, "includes", &respect_includes_opt, N_("respect include directives on lookup")),
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
@@ -719,6 +721,9 @@  int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	if (actions != ACTION_ADD && stdin_input)
+		die(_("--stdin has to be combined with --add"));
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
@@ -784,8 +789,22 @@  int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
-		check_argc(argc, 2, 2);
-		value = normalize_value(argv[0], argv[1]);
+
+		if (stdin_input) {
+			struct strbuf input = STRBUF_INIT;
+			check_argc(argc, 1, 1);
+			if (strbuf_read_once(&input, 0, 1000) < 0)
+				die_errno("could not read from stdin");
+
+			strbuf_trim_trailing_newline(&input);
+			value = normalize_value(argv[0], input.buf);
+			strbuf_release(&input);
+		}
+		else {
+			check_argc(argc, 2, 2);
+			value = normalize_value(argv[0], argv[1]);
+		}
+
 		UNLEAK(value);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], value,
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 428177c390..d78abf85ee 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -380,6 +380,17 @@  test_expect_success '--add' '
 	test_cmp expect output
 '
 
+test_expect_success '--add with --stdin' '
+	echo true | git config --add --stdin foo.mepmep &&
+	git config --get foo.mepmep >output &&
+	echo true >expect &&
+	test_cmp expect output
+'
+
+test_expect_success '--stdin without --add' '
+	test_expect_code 128 git config --stdin foo.mepmep
+'
+
 cat > .git/config << EOF
 [novalue]
 	variable