| Message ID | 20200127100933.10765-2-git@zjvandeweg.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config: read instructions from stdin | expand |
On Mon, Jan 27, 2020 at 11:21 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote: > +static void update_config_stdin() > +{ > + struct strbuf input = STRBUF_INIT; > + const char *next; > + const char *max_char; > + > + if (strbuf_read(&input, 0, 1000) < 0) > + die_errno(_("could not read from stdin")); > + next = input.buf; > + > + max_char = input.buf + input.len; > + while(next < max_char) { Nit: space missing between "while" and "(". > + if (*next == term) > + die(_("empty command in input")); > + else if (isspace(*next)) > + die(_("whitespace before command")); > + else if (skip_prefix(next, "set ", &next)) > + next = parse_cmd_set(next, max_char); > + else if (skip_prefix(next, "unset ", &next)) > + next = parse_cmd_unset(next, max_char); > + else > + die(_("unknown command %s"), next); > + } > + > + strbuf_release(&input); > +} Thanks, Christian.
On Mon, Jan 27, 2020 at 5:17 AM Zeger-Jan van de Weg <git@zjvandeweg.nl> wrote: > 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`. > > Add the `--stdin` option that reads from stdin for instructions to set > and unset values to hide them from prying eyes. The instructions are based > on the `update-ref` DSL, and accept the set and unset commands. > > Signed-off-by: Zeger-Jan van de Weg <git@zjvandeweg.nl> > --- > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > @@ -259,6 +264,30 @@ Valid `<type>`'s include: > +STDIN > +----- > + > +With `--stdin`, config reads instructions from standard input and performs > +all modifications in sequence. > + > +Specify commands of the form: > + > + set SP <key> SP <newvalue> > + unset SP <key> If you follow the precedent of the git-update-ref documentation, these should be: set SP <key> SP <newvalue> LF unset SP <key> LF I'm not sure we really need to be calling the value "newvalue" (I guess you picked that up from git-update-ref.txt). "value" should be fine, thus: set SP <key> SP <value> LF unset SP <key> LF > +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without > +quoting: > + > + set SP <key> NULL <newvalue> > + unset SP <key> A few comments: First, this is talking about the NUL character, not a NULL pointer, so: s/NULL/NUL/ Second, this doesn't give any indication about how the lines should be terminated. It should instead be written as: set SP <key> NUL <value> NUL unset SP <key> NUL Third, importantly, unlike git-update-ref from which this DSL takes inspiration and in which "refs" might have oddball names for which NUL-termination makes sense, it's hard to imagine a case in which a configuration key would be so strange as to warrant NUL-termination. This observation suggests a simpler DSL in which only <value> is NUL-terminated: set SP <key> SP <value> NUL unset SP <key> LF (The proposed code changes in config.c would need adjustment, as well, to implement this revised DSL.) > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > @@ -380,6 +380,66 @@ test_expect_success '--add' ' > +test_expect_success '--stdin works on no input' ' > + echo -n "" | git config --stdin > +' `echo -n` is not portable. If you want no input at all, either grab it from /dev/null: git config --stdin </dev/null or use `printf` to suppress printing of the line-terminator: printf "" | git config --stdin > +test_expect_success '--stdin with --null flag' ' > + echo -ne "set bar.baz\0false" | git config --stdin --null && > + Git config --get bar.baz >output && > + echo false >expect && > + test_cmp expect output > +' Likewise, non-portable use of `echo -n` and `echo "...\0...". Use `printf` instead: printf "set bar.baz\0false" | git config --stdin --null && (But note that this isn't even testing NUL-termination of <value>.)
On Mon, Jan 27, 2020 at 11:09:33AM +0100, Zeger-Jan van de Weg wrote: > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index 899e92a1c9..9f7462284d 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -23,6 +23,7 @@ SYNOPSIS > 'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list > 'git config' [<file-option>] --get-color name [default] > 'git config' [<file-option>] --get-colorbool name [stdout-is-tty] > +'git config' [<file-option>] [-z|--null] --stdin Instead of a stdin mode just for setting, you have a general --stdin mode. That would later allow "get" commands, too. I'm not sure if that's a good idea, though. Here are two complications which came to mind immediately: - the permissions models for getting and setting might be different. Might we ever feed untrusted input to something like "git config --get --stdin", but not want it to be able to set any values? - what happens when get/set commands are intermixed? When are set commands flushed? E.g., if I send: set foo.bar baz get foo.bar then do I see "baz", or whatever was there before? I think those are solvable, but unless we have a compelling use case for a process that will both get and set values, it seems like it might be simpler to keep the major modes separate. > +STDIN > +----- > + > +With `--stdin`, config reads instructions from standard input and performs > +all modifications in sequence. > + > +Specify commands of the form: > + > + set SP <key> SP <newvalue> > + unset SP <key> This protocol doesn't leave much room for giving more details about each set operation. There are several things you can do on the command line now that you might want to specify on a per-key basis: - adding keys versus replacing - likewise, replace-all versus a single replace - likewise, unset versus unset-all - type interpretation / normalization - value regex matching Even if we don't implement all of that immediately, it would be nice to have a plan for how they'd be added to stdin mode eventually. > +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without > +quoting: This mentions quoting, but the earlier form doesn't discuss it at all. I can see from the quote that it does unquote_c_style(). It might be worth saying so explicitly; since we're dealing with config, people may assume that the syntax is like the one in config files (which is similar, but not quite the same). > +static const char *parse_cmd_set(const char *next, const char *max_char) > +{ > + char *key, *value; > + int ret; > + > + key = parse_key_or_value(&next, max_char); > + if (!key) > + die(_("set: missing key")); > + > + value = parse_key_or_value(&next, max_char); > + if (!value) > + die(_("set: missing value")); > + > + ret = git_config_set_in_file_gently(given_config_source.file, key, value); > + if (ret) > + die(_("cannot set key value pair: %d"), ret); > + > + free(key); > + free(value); > + return next; > +} I mentioned flushing earlier. It looks like this will flush each individual "set" command by rewriting the whole file. But another reason one might want to use --stdin is to set multiple keys in one go, either for efficiency or because you'd like readers to see the result atomically. I don't think we actually have a function to set multiple keys in one pass yet. And I'd be OK starting with a less-efficient implementation. But we probably should document that the current flushing behavior is not something people should count on, to leave us room to change it in the future. The code itself looked cleanly done, though I admit I didn't look too closely. If I've convinced you on any of the design considerations above, I think it would need rewritten a bit. -Peff
On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote: > Second, this doesn't give any indication about how the lines should be > terminated. It should instead be written as: > > set SP <key> NUL <value> NUL > unset SP <key> NUL > > Third, importantly, unlike git-update-ref from which this DSL takes > inspiration and in which "refs" might have oddball names for which > NUL-termination makes sense, it's hard to imagine a case in which a > configuration key would be so strange as to warrant NUL-termination. > This observation suggests a simpler DSL in which only <value> is > NUL-terminated: > > set SP <key> SP <value> NUL > unset SP <key> LF > > (The proposed code changes in config.c would need adjustment, as well, > to implement this revised DSL.) The section and key parts of a config key are pretty restricted, but the subsection portion can contain anything except newline and NUL. So in particular, it would be valid to have a space, which would make the input ambiguous. I agree it would probably be rare, but isn't the whole point of "-z" to be able to represent anything without worrying about quoting? -Peff
On Tue, Jan 28, 2020 at 4:24 AM Jeff King <peff@peff.net> wrote: > On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote: > > set SP <key> SP <value> NUL > > unset SP <key> LF > > The section and key parts of a config key are pretty restricted, but the > subsection portion can contain anything except newline and NUL. So in > particular, it would be valid to have a space, which would make the > input ambiguous. I was wondering if that might be the case (particularly, if space was allowed). Thanks for the education.
Jeff King <peff@peff.net> writes: > On Mon, Jan 27, 2020 at 11:59:03AM -0500, Eric Sunshine wrote: > >> Second, this doesn't give any indication about how the lines should be >> terminated. It should instead be written as: >> >> set SP <key> NUL <value> NUL >> unset SP <key> NUL >> >> Third, importantly, unlike git-update-ref from which this DSL takes >> inspiration and in which "refs" might have oddball names for which >> NUL-termination makes sense, it's hard to imagine a case in which a >> configuration key would be so strange as to warrant NUL-termination. >> This observation suggests a simpler DSL in which only <value> is >> NUL-terminated: >> >> set SP <key> SP <value> NUL >> unset SP <key> LF >> >> (The proposed code changes in config.c would need adjustment, as well, >> to implement this revised DSL.) > > The section and key parts of a config key are pretty restricted, but the > subsection portion can contain anything except newline and NUL. So in > particular, it would be valid to have a space, which would make the > input ambiguous. > > I agree it would probably be rare, but isn't the whole point of "-z" to > be able to represent anything without worrying about quoting? Yup. I was tempted to say, in addition to "without worrying about quoting", "without worrying about the syntax". But unfortunately that would not work. If we were to add a new "frotz" subcommand in a future version of Git that takes N args, frotz SP <arg1> NUL <arg2> NUL ... <argN> NUL may how you'd express it, and it would be wonderful if the current version of Git that does not know "frotz" subcommand can at least parse and ignore it. That cannot however be done, though, because there is no syntactic difference between the argument terminator (after each arg) and subcommand terminator (after each subcommand).
On Tue, Jan 28, 2020 at 11:28:36AM -0800, Junio C Hamano wrote: > > I agree it would probably be rare, but isn't the whole point of "-z" to > > be able to represent anything without worrying about quoting? > > Yup. I was tempted to say, in addition to "without worrying about > quoting", "without worrying about the syntax". > > But unfortunately that would not work. If we were to add a new > "frotz" subcommand in a future version of Git that takes N args, > > frotz SP <arg1> NUL <arg2> NUL ... <argN> NUL > > may how you'd express it, and it would be wonderful if the current > version of Git that does not know "frotz" subcommand can at least > parse and ignore it. That cannot however be done, though, because > there is no syntactic difference between the argument terminator > (after each arg) and subcommand terminator (after each subcommand). I do agree it would be nice if we could separate syntax from semantics, just because it lets us reuse and modify parsers more easily. We have the same problem on output (e.g., anything parsing "diff --name-status -z" has to know there are two name fields for an R or C entry), and it has tripped people up in the past. But: - I don't think we'd want a current version of Git to ignore an unknown frotz anyway. It would probably say "I don't understand frotz, so I don't know if it's safe to ignore it or not". - Given only the tool of NUL bytes, I'm not sure how to easily design a syntax that isn't awful. I.e., clearly something like: frotz SP 2 NUL <arg1> NUL <arg2> NUL works, but have we really made the world a better place? It would make more sense to me to use a standard serialization format like JSON, but that generally comes with its own inherited baggage (like non-UTF8 strings). -Peff
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 899e92a1c9..9f7462284d 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -23,6 +23,7 @@ SYNOPSIS 'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list 'git config' [<file-option>] --get-color name [default] 'git config' [<file-option>] --get-colorbool name [stdout-is-tty] +'git config' [<file-option>] [-z|--null] --stdin 'git config' [<file-option>] -e | --edit DESCRIPTION @@ -212,6 +213,10 @@ Valid `<type>`'s include: output without getting confused e.g. by values that contain line breaks. +--stdin:: + Instructions are read from the standard input, instead of the command + line interface. Refer to the STDIN section. + --name-only:: Output only the names of config variables for `--list` or `--get-regexp`. @@ -259,6 +264,30 @@ Valid `<type>`'s include: When using `--get`, and the requested variable is not found, behave as if <value> were the value assigned to the that variable. +STDIN +----- + +With `--stdin`, config reads instructions from standard input and performs +all modifications in sequence. + +Specify commands of the form: + + set SP <key> SP <newvalue> + unset SP <key> + +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without +quoting: + + set SP <key> NULL <newvalue> + unset SP <key> + +set:: + Set or update the value for <key> to <newvalue>. + +unset: + Remove the value from the configuration by <key>, when the <key> isn't + present in the configuration no error is returned. + CONFIGURATION ------------- `pager.config` is only respected when listing configuration, i.e., when diff --git a/builtin/config.c b/builtin/config.c index 98d65bc0ad..a449b00b65 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -50,6 +50,7 @@ static int show_origin; #define ACTION_GET_COLOR (1<<13) #define ACTION_GET_COLORBOOL (1<<14) #define ACTION_GET_URLMATCH (1<<15) +#define ACTION_STDIN (1<<16) /* * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than @@ -143,6 +144,7 @@ static struct option builtin_config_options[] = { OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT), OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR), OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL), + OPT_BIT(0, "stdin", &actions, N_("input changes from stdin"), ACTION_STDIN), OPT_GROUP(N_("Type")), OPT_CALLBACK('t', "type", &type, "", N_("value is given this type"), option_parse_type), OPT_CALLBACK_VALUE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL), @@ -594,6 +596,102 @@ static char *default_user_config(void) return strbuf_detach(&buf, NULL); } +static char *parse_key_or_value(const char **next, const char *max_char) { + struct strbuf token = STRBUF_INIT; + + if (term) { + if (**next == '"') { + const char **orig = next; + + if (unquote_c_style(&token, *next, next)) + die("badly quoted argument: %s", *orig); + if (*next && !isspace(**next)) + die("unexpected character after quoted argument: %s", *orig); + } else { + while (*next && !isspace(**next)) { + if (*next > max_char) + die("unexpected end of buffer"); + + strbuf_addch(&token, *(*next)++); + } + } + + (*next)++; + } else { + strbuf_addstr(&token, *next); + *next += token.len + 1; + } + + return strbuf_detach(&token, NULL); +} + +static const char *parse_cmd_set(const char *next, const char *max_char) +{ + char *key, *value; + int ret; + + key = parse_key_or_value(&next, max_char); + if (!key) + die(_("set: missing key")); + + value = parse_key_or_value(&next, max_char); + if (!value) + die(_("set: missing value")); + + ret = git_config_set_in_file_gently(given_config_source.file, key, value); + if (ret) + die(_("cannot set key value pair: %d"), ret); + + free(key); + free(value); + return next; +} + + +static const char *parse_cmd_unset(const char *next, const char *max_char) +{ + char *key; + int ret; + + key = parse_key_or_value(&next, max_char); + if (!key) + die(_("no key found to unset")); + + ret = git_config_set_in_file_gently(given_config_source.file, key, NULL); + if (ret) + die(_("cannot unset key: %d"), ret); + + free(key); + return next; +} + +static void update_config_stdin() +{ + struct strbuf input = STRBUF_INIT; + const char *next; + const char *max_char; + + if (strbuf_read(&input, 0, 1000) < 0) + die_errno(_("could not read from stdin")); + next = input.buf; + + max_char = input.buf + input.len; + while(next < max_char) { + if (*next == term) + die(_("empty command in input")); + else if (isspace(*next)) + die(_("whitespace before command")); + else if (skip_prefix(next, "set ", &next)) + next = parse_cmd_set(next, max_char); + else if (skip_prefix(next, "unset ", &next)) + next = parse_cmd_unset(next, max_char); + else + die(_("unknown command %s"), next); + } + + strbuf_release(&input); +} + int cmd_config(int argc, const char **argv, const char *prefix) { int nongit = !startup_info->have_repository; @@ -867,6 +965,12 @@ int cmd_config(int argc, const char **argv, const char *prefix) color_stdout_is_tty = git_config_bool("command line", argv[1]); return get_colorbool(argv[0], argc == 2); } + else if (actions == ACTION_STDIN) { + check_write(); + check_argc(argc, 0, 0); + + update_config_stdin(); + } return 0; } diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 983a0a1583..d8ad82922d 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -380,6 +380,66 @@ test_expect_success '--add' ' test_cmp expect output ' +test_expect_success '--stdin to set a value' ' + echo "set foo.mepmep true" | git config --stdin && + git config --get foo.mepmep >output && + echo true >expect && + test_cmp expect output +' + +test_expect_success '--stdin multiline support' ' + cat >stdin <<-EOF && + set bar.mepmep true + set foo.mepmep true + EOF + git config --stdin <stdin && + git config --get bar.mepmep >output && + git config --get foo.mepmep >>output && + echo -e "true\ntrue" > expect && + test_cmp expect output +' + +test_expect_success '--stdin hides input on errors' ' + cat >stdin <<-EOF && + set bar.mepmep + EOF + test_must_fail git config --stdin <stdin 2>output && + echo "fatal: unexpected end of buffer" >expect && + test_cmp expect output +' + +test_expect_success '--stdin fails on leading whitespace' ' + cat >stdin <<-EOF && + set bar.mepmep + EOF + test_must_fail git config --stdin <stdin +' + +test_expect_success '--stdin fails on unknown command' ' + cat >stdin <<-EOF && + foo bar.mepmep + EOF + test_must_fail git config --stdin <stdin +' + +test_expect_success '--stdin works on no input' ' + echo -n "" | git config --stdin +' + +test_expect_success '--stdin fails on unbalanced quotes' ' + cat >stdin <<-EOF && + set "foo.bar + EOF + test_must_fail git config --stdin <stdin +' + +test_expect_success '--stdin with --null flag' ' + echo -ne "set bar.baz\0false" | git config --stdin --null && + git config --get bar.baz >output && + echo false >expect && + test_cmp expect output +' + cat > .git/config << EOF [novalue] variable
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`. Add the `--stdin` option that reads from stdin for instructions to set and unset values to hide them from prying eyes. The instructions are based on the `update-ref` DSL, and accept the set and unset commands. Signed-off-by: Zeger-Jan van de Weg <git@zjvandeweg.nl> --- Documentation/git-config.txt | 29 ++++++++++ builtin/config.c | 104 +++++++++++++++++++++++++++++++++++ t/t1300-config.sh | 60 ++++++++++++++++++++ 3 files changed, 193 insertions(+)