* [PATCH 0/1] git-config --add allows values from stdin @ 2019-09-17 13:31 Zeger-Jan van de Weg 2019-09-17 13:31 ` [PATCH 1/1] Git config allows value setting " Zeger-Jan van de Weg 2019-09-22 3:11 ` [PATCH 0/1] git-config --add allows values " Taylor Blau 0 siblings, 2 replies; 6+ messages in thread From: Zeger-Jan van de Weg @ 2019-09-17 13:31 UTC (permalink / raw) To: git; +Cc: Zeger-Jan van de Weg When adding or updating configuration values using git-config, the values could all be observed by different processes as these are passed as arguments. In some environments all commands executed are also all logged. When the value contains secrets, this is a side effect that would be great to avoid. At GitLab we use Rugged/libgit2 to circumvent this property[1]. The following patch allows a value to be set through stdin when the user passes a `--stdin` flag. [1]: https://gitlab.com/gitlab-org/gitaly/blob/8ab5bd595984678838f3f09a96798b149e68a939/ruby/lib/gitlab/git/http_auth.rb#L14-15 Zeger-Jan van de Weg (1): Git config allows value setting from stdin Documentation/git-config.txt | 5 ++++- builtin/config.c | 23 +++++++++++++++++++++-- t/t1300-config.sh | 11 +++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] Git config allows value setting from stdin 2019-09-17 13:31 [PATCH 0/1] git-config --add allows values from stdin Zeger-Jan van de Weg @ 2019-09-17 13:31 ` Zeger-Jan van de Weg 2019-09-17 16:59 ` Junio C Hamano 2019-09-22 3:11 ` [PATCH 0/1] git-config --add allows values " Taylor Blau 1 sibling, 1 reply; 6+ messages in thread From: Zeger-Jan van de Weg @ 2019-09-17 13:31 UTC (permalink / raw) To: git; +Cc: Zeger-Jan van de Weg 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 <git@zjvandeweg.nl> --- Documentation/git-config.txt | 5 ++++- builtin/config.c | 23 +++++++++++++++++++++-- t/t1300-config.sh | 11 +++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) 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 -- 2.23.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] Git config allows value setting from stdin 2019-09-17 13:31 ` [PATCH 1/1] Git config allows value setting " Zeger-Jan van de Weg @ 2019-09-17 16:59 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2019-09-17 16:59 UTC (permalink / raw) To: Zeger-Jan van de Weg; +Cc: git Zeger-Jan van de Weg <git@zjvandeweg.nl> 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:pass@over.there/repo EOF instead of $ git config --stdin --add remote.hidden.url <<\EOF https://user:pass@over.there/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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] git-config --add allows values from stdin 2019-09-17 13:31 [PATCH 0/1] git-config --add allows values from stdin Zeger-Jan van de Weg 2019-09-17 13:31 ` [PATCH 1/1] Git config allows value setting " Zeger-Jan van de Weg @ 2019-09-22 3:11 ` Taylor Blau 2019-09-23 9:46 ` Phillip Wood 2019-09-23 11:45 ` SZEDER Gábor 1 sibling, 2 replies; 6+ messages in thread From: Taylor Blau @ 2019-09-22 3:11 UTC (permalink / raw) To: Zeger-Jan van de Weg; +Cc: git Hi ZJ, On Tue, Sep 17, 2019 at 03:31:34PM +0200, Zeger-Jan van de Weg wrote: > When adding or updating configuration values using git-config, the > values could all be observed by different processes as these are passed > as arguments. In some environments all commands executed are also all > logged. When the value contains secrets, this is a side effect that > would be great to avoid. At GitLab we use Rugged/libgit2 to circumvent > this property[1]. > > The following patch allows a value to be set through stdin when the user > passes a `--stdin` flag. Interesting. I had thought some time ago about making an interactive line-oriented 'mode' for using 'git-config(1)', which would allow callers to add/delete/fetch multiple variables using only a single process. This would satisfy a more general use-case than yours: particularly my idea was grown out of wanting to specify or read many configuration entries at once when using a tool built around Git, such as Git LFS. I had not considered tying '--stdin' to the '--add' (implicit or not) mode of 'git-config(1)'. It is an interesting idea to be sure. On the one hand, it lends itself to other modes, such as '--get' combined with '--stdin', or '--unset' in the same fashion. One could imagine that each of these would take either a key/value-pair (in the case of '--add') or a set of key(s) (in the remaining cases). The most desirable aspect is that this would allow for a clear path to this series being picked up. On the other hand, tying '--stdin' to a particular mode of using 'git conifg' seems overly restrictive to me. If I am building a tool that wants to fetch some values in the configuration, and then add/unset others based on the results using only a single process, I don't think that a mode-based '--stdin' flag gets the job done. One happy medium that comes to mind is a new '--interactive' mode, which implies '--stdin' and would allow the above use-case, e.g.: $ git config --interactive <<\EOF get core.myval set core.foo bar unset core.baz EOF (An off-topic note is that it would be interesting to allow more fanciful options than 'get', e.g., 'get' with a '--type' specifier, or some such). I'm not sure if anyone actually wants to use 'git-config(1)' in this way, but I figured that I would at least share some things that I was thinking about when initially considering this proposal. > [1]: https://gitlab.com/gitlab-org/gitaly/blob/8ab5bd595984678838f3f09a96798b149e68a939/ruby/lib/gitlab/git/http_auth.rb#L14-15 > > Zeger-Jan van de Weg (1): > Git config allows value setting from stdin > > Documentation/git-config.txt | 5 ++++- > builtin/config.c | 23 +++++++++++++++++++++-- > t/t1300-config.sh | 11 +++++++++++ > 3 files changed, 36 insertions(+), 3 deletions(-) > > -- > 2.23.0 > Thanks, Taylor ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] git-config --add allows values from stdin 2019-09-22 3:11 ` [PATCH 0/1] git-config --add allows values " Taylor Blau @ 2019-09-23 9:46 ` Phillip Wood 2019-09-23 11:45 ` SZEDER Gábor 1 sibling, 0 replies; 6+ messages in thread From: Phillip Wood @ 2019-09-23 9:46 UTC (permalink / raw) To: Taylor Blau, Zeger-Jan van de Weg; +Cc: git Hi Taylor and ZJ On 22/09/2019 04:11, Taylor Blau wrote: > Hi ZJ, > > On Tue, Sep 17, 2019 at 03:31:34PM +0200, Zeger-Jan van de Weg wrote: >> When adding or updating configuration values using git-config, the >> values could all be observed by different processes as these are passed >> as arguments. In some environments all commands executed are also all >> logged. When the value contains secrets, this is a side effect that >> would be great to avoid. How much extra security does this actually add? - do the processes that can observe the command line arguments also have read access to the git config file? At GitLab we use Rugged/libgit2 to circumvent >> this property[1]. >> >> The following patch allows a value to be set through stdin when the user >> passes a `--stdin` flag. > > Interesting. I had thought some time ago about making an interactive > line-oriented 'mode' for using 'git-config(1)', which would allow > callers to add/delete/fetch multiple variables using only a single > process. > > This would satisfy a more general use-case than yours: particularly my > idea was grown out of wanting to specify or read many configuration > entries at once when using a tool built around Git, such as Git LFS. > > I had not considered tying '--stdin' to the '--add' (implicit or not) > mode of 'git-config(1)'. It is an interesting idea to be sure. > > On the one hand, it lends itself to other modes, such as '--get' > combined with '--stdin', or '--unset' in the same fashion. One could > imagine that each of these would take either a key/value-pair (in the > case of '--add') or a set of key(s) (in the remaining cases). The most > desirable aspect is that this would allow for a clear path to this > series being picked up. It would be great to be able to --get multiple values and I can see people wanting to be able to --unset them as well. > On the other hand, tying '--stdin' to a particular mode of using 'git > conifg' seems overly restrictive to me. If I am building a tool that > wants to fetch some values in the configuration, and then add/unset > others based on the results using only a single process, I don't think > that a mode-based '--stdin' flag gets the job done. That's true but I don't know how common it is compared to a script wanting to read a bunch of config variables at startup (i.e. does it warrant the extra complexity) Best Wishes Phillip > One happy medium that comes to mind is a new '--interactive' mode, which > implies '--stdin' and would allow the above use-case, e.g.: > > $ git config --interactive <<\EOF > get core.myval > set core.foo bar > unset core.baz > EOF > > (An off-topic note is that it would be interesting to allow more > fanciful options than 'get', e.g., 'get' with a '--type' specifier, or > some such). > > I'm not sure if anyone actually wants to use 'git-config(1)' in this > way, but I figured that I would at least share some things that I was > thinking about when initially considering this proposal. > >> [1]: https://gitlab.com/gitlab-org/gitaly/blob/8ab5bd595984678838f3f09a96798b149e68a939/ruby/lib/gitlab/git/http_auth.rb#L14-15 >> >> Zeger-Jan van de Weg (1): >> Git config allows value setting from stdin >> >> Documentation/git-config.txt | 5 ++++- >> builtin/config.c | 23 +++++++++++++++++++++-- >> t/t1300-config.sh | 11 +++++++++++ >> 3 files changed, 36 insertions(+), 3 deletions(-) >> >> -- >> 2.23.0 >> > > Thanks, > Taylor > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/1] git-config --add allows values from stdin 2019-09-22 3:11 ` [PATCH 0/1] git-config --add allows values " Taylor Blau 2019-09-23 9:46 ` Phillip Wood @ 2019-09-23 11:45 ` SZEDER Gábor 1 sibling, 0 replies; 6+ messages in thread From: SZEDER Gábor @ 2019-09-23 11:45 UTC (permalink / raw) To: Taylor Blau; +Cc: Zeger-Jan van de Weg, git On Sat, Sep 21, 2019 at 11:11:28PM -0400, Taylor Blau wrote: > Hi ZJ, > > On Tue, Sep 17, 2019 at 03:31:34PM +0200, Zeger-Jan van de Weg wrote: > > When adding or updating configuration values using git-config, the > > values could all be observed by different processes as these are passed > > as arguments. In some environments all commands executed are also all > > logged. When the value contains secrets, this is a side effect that > > would be great to avoid. At GitLab we use Rugged/libgit2 to circumvent > > this property[1]. > > > > The following patch allows a value to be set through stdin when the user > > passes a `--stdin` flag. > > Interesting. I had thought some time ago about making an interactive > line-oriented 'mode' for using 'git-config(1)', which would allow > callers to add/delete/fetch multiple variables using only a single > process. > > This would satisfy a more general use-case than yours: particularly my > idea was grown out of wanting to specify or read many configuration > entries at once when using a tool built around Git, such as Git LFS. Getting multiple configuration variables from a single 'git config' invocation is already supported fairly well, in the sense that it's not at all difficult to parse the output of 'git config --list -z' into a map/dict of the language of your choice. Though, of course, it doesn't offer the normalization and unit conversion of '--type'. > I had not considered tying '--stdin' to the '--add' (implicit or not) > mode of 'git-config(1)'. It is an interesting idea to be sure. > > On the one hand, it lends itself to other modes, such as '--get' > combined with '--stdin', or '--unset' in the same fashion. One could > imagine that each of these would take either a key/value-pair (in the > case of '--add') or a set of key(s) (in the remaining cases). The most > desirable aspect is that this would allow for a clear path to this > series being picked up. > > On the other hand, tying '--stdin' to a particular mode of using 'git > conifg' seems overly restrictive to me. If I am building a tool that > wants to fetch some values in the configuration, and then add/unset > others based on the results using only a single process, I don't think > that a mode-based '--stdin' flag gets the job done. > > One happy medium that comes to mind is a new '--interactive' mode, which > implies '--stdin' and would allow the above use-case, e.g.: > > $ git config --interactive <<\EOF > get core.myval > set core.foo bar > unset core.baz > EOF I think the option should still be called '--stdin' instead of '--interactive'. Consider the rather similar 'git update-ref --stdin', which allows creating, updating, and deleting refs in one go as well. > (An off-topic note is that it would be interesting to allow more > fanciful options than 'get', e.g., 'get' with a '--type' specifier, or > some such). > > I'm not sure if anyone actually wants to use 'git-config(1)' in this > way, but I figured that I would at least share some things that I was > thinking about when initially considering this proposal. Once upon a time I had a script-generated repo with 10+k remotes, and 3 config variables per remote. The resulting configuration file was over 1MB, and creating it by forking a 'git config' to write those configuration variables one at a time took over 18mins (IIRC). I ended up special casing the writing of the initial huge configuration file with simple print() calls, which only took about a second or two. So I would welcome a more general 'git config --stdin' that could write configuration variables in bulk. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-23 11:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-17 13:31 [PATCH 0/1] git-config --add allows values from stdin Zeger-Jan van de Weg 2019-09-17 13:31 ` [PATCH 1/1] Git config allows value setting " Zeger-Jan van de Weg 2019-09-17 16:59 ` Junio C Hamano 2019-09-22 3:11 ` [PATCH 0/1] git-config --add allows values " Taylor Blau 2019-09-23 9:46 ` Phillip Wood 2019-09-23 11:45 ` SZEDER Gábor
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).