| Message ID | 470396d36f938f0070b8c849a85b1a30949056e3.1610453228.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ce81b1da230cf04e231ce337c2946c0671ffb303 |
| Headers | show |
| Series | config: allow specifying config entries via envvar pairs | expand |
On Tue, Jan 12 2021, Patrick Steinhardt wrote: A minor doc bug that wasn't spotted before landing. Here we say "--config-env foo=bar" will work: > diff --git a/Documentation/git.txt b/Documentation/git.txt > index a6d4ad0818..d36e6fd482 100644 > --- a/Documentation/git.txt > +++ b/Documentation/git.txt > @@ -13,7 +13,7 @@ SYNOPSIS > [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path] > [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare] > [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>] > - [--super-prefix=<path>] > + [--super-prefix=<path>] [--config-env <name>=<envvar>] > <command> [<args>] > > DESCRIPTION > @@ -80,6 +80,28 @@ config file). Including the equals but with an empty value (like `git -c > foo.bar= ...`) sets `foo.bar` to the empty string which `git config > --type=bool` will convert to `false`. But not here, we ask for "--config-env=" (note the "="): > +--config-env=<name>=<envvar>:: > + Like `-c <name>=<value>`, give configuration variable > + '<name>' a value, where <envvar> is the name of an > + environment variable from which to retrieve the value. Unlike > + `-c` there is no shortcut for directly setting the value to an > + empty string, instead the environment variable itself must be > + set to the empty string. It is an error if the `<envvar>` does not exist > + in the environment. `<envvar>` may not contain an equals sign > + to avoid ambiguity with `<name>`s which contain one. > ++ > [...] > + } else if (skip_prefix(cmd, "--config-env=", &cmd)) { > + git_config_push_env(cmd); But as this... > +test_expect_success 'git --config-env=key=envvar support' ' > + cat >expect <<-\EOF && > + value > + value > + false > + EOF > + { > + ENVVAR=value git --config-env=core.name=ENVVAR config core.name && > + ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase && > + ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag > + } >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'git --config-env fails with invalid parameters' ' > + test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error && > + test_i18ngrep "invalid config format: foo.flag" error && > + test_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error && > + test_i18ngrep "missing environment variable name for configuration ${SQ}foo.flag${SQ}" error && > + sane_unset NONEXISTENT && > + test_must_fail git --config-env=foo.flag=NONEXISTENT config --bool foo.flag 2>error && > + test_i18ngrep "missing environment variable ${SQ}NONEXISTENT${SQ} for configuration ${SQ}foo.flag${SQ}" error > +' > + > +test_expect_success 'git -c and --config-env work together' ' > + cat >expect <<-\EOF && > + bar.cmd cmd-value > + bar.env env-value > + EOF > + ENVVAR=env-value git \ > + -c bar.cmd=cmd-value \ > + --config-env=bar.env=ENVVAR \ > + config --get-regexp "^bar.*" >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'git -c and --config-env override each other' ' > + cat >expect <<-\EOF && > + env > + cmd > + EOF > + { > + ENVVAR=env git -c bar.bar=cmd --config-env=bar.bar=ENVVAR config bar.bar && > + ENVVAR=env git --config-env=bar.bar=ENVVAR -c bar.bar=cmd config bar.bar > + } >actual && > + test_cmp expect actual > +' > + > test_expect_success 'git config --edit works' ' > git config -f tmp test.value no && > echo test.value=yes >expect && ...and the tests show we just support the --opt=foo=bar form, not --opt foo=bar. Bonus points to anyone sorting out some of the existing inconsistencies when fixing this, i.e. --exec-path supports either the "=" form, or not, but various other skip_prefix() in the same function don't, seemingly (but I have not tested) for no good reason. It seems to me that having a skip_prefix_opt() or something would be a good fix for this, i.e. a "maybe trim the last '='" version of skip_prefix. Then we could just consistently use that. Or maybe there's some reason we don't want to be as lax as --exec-path with any other option...
On Fri, Apr 16, 2021 at 05:40:36PM +0200, Ævar Arnfjörð Bjarmason wrote: > Bonus points to anyone sorting out some of the existing inconsistencies > when fixing this, i.e. --exec-path supports either the "=" form, or not, > but various other skip_prefix() in the same function don't, seemingly > (but I have not tested) for no good reason. I suspect just because it's more (per-option) work to support both types, and nobody really cared enough to do so. > It seems to me that having a skip_prefix_opt() or something would be a > good fix for this, i.e. a "maybe trim the last '='" version of > skip_prefix. Then we could just consistently use that. There's a similar situation in the revision parser (which does not use our regular parse-options). There we have a parse_long_opt() helper which does the right thing. We could use that more widely. I also wouldn't be surprised if we could leverage one of the sub-functions of parse-options, but it might turn into a rabbit hole. Converting the whole thing to the usual parse_options() might get awkward, since many of the options operate at time-of-parse, not after we've seen everything (I suspect many of them don't care either way, but you're always risking subtle regressions there). > Or maybe there's some reason we don't want to be as lax as --exec-path > with any other option... I can't think of one. -Peff
On Sat, Apr 17, 2021 at 04:38:31AM -0400, Jeff King wrote: > On Fri, Apr 16, 2021 at 05:40:36PM +0200, Ævar Arnfjörð Bjarmason wrote: > > > Bonus points to anyone sorting out some of the existing inconsistencies > > when fixing this, i.e. --exec-path supports either the "=" form, or not, > > but various other skip_prefix() in the same function don't, seemingly > > (but I have not tested) for no good reason. > > I suspect just because it's more (per-option) work to support both > types, and nobody really cared enough to do so. > > > It seems to me that having a skip_prefix_opt() or something would be a > > good fix for this, i.e. a "maybe trim the last '='" version of > > skip_prefix. Then we could just consistently use that. > > There's a similar situation in the revision parser (which does not use > our regular parse-options). There we have a parse_long_opt() helper > which does the right thing. We could use that more widely. > > I also wouldn't be surprised if we could leverage one of the > sub-functions of parse-options, but it might turn into a rabbit hole. > Converting the whole thing to the usual parse_options() might get > awkward, since many of the options operate at time-of-parse, not after > we've seen everything (I suspect many of them don't care either way, but > you're always risking subtle regressions there). > > > Or maybe there's some reason we don't want to be as lax as --exec-path > > with any other option... > > I can't think of one. > > -Peff `--exec-path` does two different things based on whether you pass a "=" or not: either you tell git where its binaries are, or you ask it where it thinks they're. It's still true for some (most?) of the other options though. Patrick
On Sat, Apr 17 2021, Jeff King wrote: > On Fri, Apr 16, 2021 at 05:40:36PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Bonus points to anyone sorting out some of the existing inconsistencies >> when fixing this, i.e. --exec-path supports either the "=" form, or not, >> but various other skip_prefix() in the same function don't, seemingly >> (but I have not tested) for no good reason. > > I suspect just because it's more (per-option) work to support both > types, and nobody really cared enough to do so. > >> It seems to me that having a skip_prefix_opt() or something would be a >> good fix for this, i.e. a "maybe trim the last '='" version of >> skip_prefix. Then we could just consistently use that. > > There's a similar situation in the revision parser (which does not use > our regular parse-options). There we have a parse_long_opt() helper > which does the right thing. We could use that more widely. > > I also wouldn't be surprised if we could leverage one of the > sub-functions of parse-options, but it might turn into a rabbit hole. > Converting the whole thing to the usual parse_options() might get > awkward, since many of the options operate at time-of-parse, not after > we've seen everything (I suspect many of them don't care either way, but > you're always risking subtle regressions there). So we could use parse_options() and guarantee the existing behavior if they were all OPT_CALLBACK?
On Mon, Apr 19 2021, Patrick Steinhardt wrote: > On Sat, Apr 17, 2021 at 04:38:31AM -0400, Jeff King wrote: >> On Fri, Apr 16, 2021 at 05:40:36PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> > Bonus points to anyone sorting out some of the existing inconsistencies >> > when fixing this, i.e. --exec-path supports either the "=" form, or not, >> > but various other skip_prefix() in the same function don't, seemingly >> > (but I have not tested) for no good reason. >> >> I suspect just because it's more (per-option) work to support both >> types, and nobody really cared enough to do so. >> >> > It seems to me that having a skip_prefix_opt() or something would be a >> > good fix for this, i.e. a "maybe trim the last '='" version of >> > skip_prefix. Then we could just consistently use that. >> >> There's a similar situation in the revision parser (which does not use >> our regular parse-options). There we have a parse_long_opt() helper >> which does the right thing. We could use that more widely. >> >> I also wouldn't be surprised if we could leverage one of the >> sub-functions of parse-options, but it might turn into a rabbit hole. >> Converting the whole thing to the usual parse_options() might get >> awkward, since many of the options operate at time-of-parse, not after >> we've seen everything (I suspect many of them don't care either way, but >> you're always risking subtle regressions there). >> >> > Or maybe there's some reason we don't want to be as lax as --exec-path >> > with any other option... >> >> I can't think of one. >> >> -Peff > > `--exec-path` does two different things based on whether you pass a "=" > or not: either you tell git where its binaries are, or you ask it where > it thinks they're. It's still true for some (most?) of the other options > though. I don't know how I got those conflated, but FWIW I meant (or at least think I did) the likes of --namespace[=], --git-dir[=] and --work-tree[=] where the "=" is really optional, but otherwise the option behaves the same.
On Tue, Apr 20, 2021 at 12:59:16PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> It seems to me that having a skip_prefix_opt() or something would be a > >> good fix for this, i.e. a "maybe trim the last '='" version of > >> skip_prefix. Then we could just consistently use that. > > > > There's a similar situation in the revision parser (which does not use > > our regular parse-options). There we have a parse_long_opt() helper > > which does the right thing. We could use that more widely. > > > > I also wouldn't be surprised if we could leverage one of the > > sub-functions of parse-options, but it might turn into a rabbit hole. > > Converting the whole thing to the usual parse_options() might get > > awkward, since many of the options operate at time-of-parse, not after > > we've seen everything (I suspect many of them don't care either way, but > > you're always risking subtle regressions there). > > So we could use parse_options() and guarantee the existing behavior if > they were all OPT_CALLBACK? I _think_ so, but the result might be quite hard to read (the logic would be scattered all over a bunch of tiny callbacks). But it might not be too bad. Especially if you figure out which ones actually need the time-of-parse logic and use more vanilla OPT_* for the others (that's the rabbit hole I alluded to). I think things like the "--exec-path" behavior that Patrick mentioned would still work (I think it's just a stock OPTARG). -Peff
On Fri, Apr 23 2021, Jeff King wrote: > On Tue, Apr 20, 2021 at 12:59:16PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> >> It seems to me that having a skip_prefix_opt() or something would be a >> >> good fix for this, i.e. a "maybe trim the last '='" version of >> >> skip_prefix. Then we could just consistently use that. >> > >> > There's a similar situation in the revision parser (which does not use >> > our regular parse-options). There we have a parse_long_opt() helper >> > which does the right thing. We could use that more widely. >> > >> > I also wouldn't be surprised if we could leverage one of the >> > sub-functions of parse-options, but it might turn into a rabbit hole. >> > Converting the whole thing to the usual parse_options() might get >> > awkward, since many of the options operate at time-of-parse, not after >> > we've seen everything (I suspect many of them don't care either way, but >> > you're always risking subtle regressions there). >> >> So we could use parse_options() and guarantee the existing behavior if >> they were all OPT_CALLBACK? > > I _think_ so, but the result might be quite hard to read (the logic > would be scattered all over a bunch of tiny callbacks). But it might not > be too bad. Especially if you figure out which ones actually need the > time-of-parse logic and use more vanilla OPT_* for the others (that's > the rabbit hole I alluded to). > > I think things like the "--exec-path" behavior that Patrick mentioned > would still work (I think it's just a stock OPTARG). [Mostly for my own future reference]: There's also the parse_options_step() API to process options one at a time, which AFAICT could be used in this case. But having glanced at it again (but not come up with a patch) I think it could be handled with OPT_CALLBACK + not caring about the order, mostly. The "envchanged" could be passed as a custom flag probably...
diff --git a/Documentation/git.txt b/Documentation/git.txt index a6d4ad0818..d36e6fd482 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -13,7 +13,7 @@ SYNOPSIS [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path] [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare] [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>] - [--super-prefix=<path>] + [--super-prefix=<path>] [--config-env <name>=<envvar>] <command> [<args>] DESCRIPTION @@ -80,6 +80,28 @@ config file). Including the equals but with an empty value (like `git -c foo.bar= ...`) sets `foo.bar` to the empty string which `git config --type=bool` will convert to `false`. +--config-env=<name>=<envvar>:: + Like `-c <name>=<value>`, give configuration variable + '<name>' a value, where <envvar> is the name of an + environment variable from which to retrieve the value. Unlike + `-c` there is no shortcut for directly setting the value to an + empty string, instead the environment variable itself must be + set to the empty string. It is an error if the `<envvar>` does not exist + in the environment. `<envvar>` may not contain an equals sign + to avoid ambiguity with `<name>`s which contain one. ++ +This is useful for cases where you want to pass transitory +configuration options to git, but are doing so on OS's where +other processes might be able to read your cmdline +(e.g. `/proc/self/cmdline`), but not your environ +(e.g. `/proc/self/environ`). That behavior is the default on +Linux, but may not be on your system. ++ +Note that this might add security for variables such as +`http.extraHeader` where the sensitive information is part of +the value, but not e.g. `url.<base>.insteadOf` where the +sensitive information can be part of the key. + --exec-path[=<path>]:: Path to wherever your core Git programs are installed. This can also be controlled by setting the GIT_EXEC_PATH diff --git a/config.c b/config.c index 1137bd73af..fd8c0c4dfc 100644 --- a/config.c +++ b/config.c @@ -345,6 +345,31 @@ void git_config_push_parameter(const char *text) strbuf_release(&env); } +void git_config_push_env(const char *spec) +{ + struct strbuf buf = STRBUF_INIT; + const char *env_name; + const char *env_value; + + env_name = strrchr(spec, '='); + if (!env_name) + die(_("invalid config format: %s"), spec); + env_name++; + if (!*env_name) + die(_("missing environment variable name for configuration '%.*s'"), + (int)(env_name - spec - 1), spec); + + env_value = getenv(env_name); + if (!env_value) + die(_("missing environment variable '%s' for configuration '%.*s'"), + env_name, (int)(env_name - spec - 1), spec); + + strbuf_add(&buf, spec, env_name - spec); + strbuf_addstr(&buf, env_value); + git_config_push_parameter(buf.buf); + strbuf_release(&buf); +} + static inline int iskeychar(int c) { return isalnum(c) || c == '-'; diff --git a/config.h b/config.h index c1449bb790..19a9adbaa9 100644 --- a/config.h +++ b/config.h @@ -138,6 +138,7 @@ int git_config_from_mem(config_fn_t fn, int git_config_from_blob_oid(config_fn_t fn, const char *name, const struct object_id *oid, void *data); void git_config_push_parameter(const char *text); +void git_config_push_env(const char *spec); int git_config_from_parameters(config_fn_t fn, void *data); void read_early_config(config_fn_t cb, void *data); void read_very_early_config(config_fn_t cb, void *data); diff --git a/git.c b/git.c index 5a8ff12f87..b5f63d346b 100644 --- a/git.c +++ b/git.c @@ -29,7 +29,7 @@ const char git_usage_string[] = " [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n" " [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n" " [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n" - " [--super-prefix=<path>]\n" + " [--super-prefix=<path>] [--config-env=<name>=<envvar>]\n" " <command> [<args>]"); const char git_more_info_string[] = @@ -255,6 +255,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) git_config_push_parameter((*argv)[1]); (*argv)++; (*argc)--; + } else if (skip_prefix(cmd, "--config-env=", &cmd)) { + git_config_push_env(cmd); } else if (!strcmp(cmd, "--literal-pathspecs")) { setenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, "1", 1); if (envchanged) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 97a04c6cc2..853f2509c5 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -1316,6 +1316,54 @@ test_expect_success 'detect bogus GIT_CONFIG_PARAMETERS' ' git config --get-regexp "env.*" ' +test_expect_success 'git --config-env=key=envvar support' ' + cat >expect <<-\EOF && + value + value + false + EOF + { + ENVVAR=value git --config-env=core.name=ENVVAR config core.name && + ENVVAR=value git --config-env=foo.CamelCase=ENVVAR config foo.camelcase && + ENVVAR= git --config-env=foo.flag=ENVVAR config --bool foo.flag + } >actual && + test_cmp expect actual +' + +test_expect_success 'git --config-env fails with invalid parameters' ' + test_must_fail git --config-env=foo.flag config --bool foo.flag 2>error && + test_i18ngrep "invalid config format: foo.flag" error && + test_must_fail git --config-env=foo.flag= config --bool foo.flag 2>error && + test_i18ngrep "missing environment variable name for configuration ${SQ}foo.flag${SQ}" error && + sane_unset NONEXISTENT && + test_must_fail git --config-env=foo.flag=NONEXISTENT config --bool foo.flag 2>error && + test_i18ngrep "missing environment variable ${SQ}NONEXISTENT${SQ} for configuration ${SQ}foo.flag${SQ}" error +' + +test_expect_success 'git -c and --config-env work together' ' + cat >expect <<-\EOF && + bar.cmd cmd-value + bar.env env-value + EOF + ENVVAR=env-value git \ + -c bar.cmd=cmd-value \ + --config-env=bar.env=ENVVAR \ + config --get-regexp "^bar.*" >actual && + test_cmp expect actual +' + +test_expect_success 'git -c and --config-env override each other' ' + cat >expect <<-\EOF && + env + cmd + EOF + { + ENVVAR=env git -c bar.bar=cmd --config-env=bar.bar=ENVVAR config bar.bar && + ENVVAR=env git --config-env=bar.bar=ENVVAR -c bar.bar=cmd config bar.bar + } >actual && + test_cmp expect actual +' + test_expect_success 'git config --edit works' ' git config -f tmp test.value no && echo test.value=yes >expect &&
While it's already possible to pass runtime configuration via `git -c <key>=<value>`, it may be undesirable to use when the value contains 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. To enable this usecase without leaking credentials, this commit introduces a new switch `--config-env=<key>=<envvar>`. Instead of directly passing a value for the given key, it instead allows the user to specify the name of an environment variable. The value of that variable will then be used as value of the key. Co-authored-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git.txt | 24 +++++++++++++++++++++- config.c | 25 ++++++++++++++++++++++ config.h | 1 + git.c | 4 +++- t/t1300-config.sh | 48 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 2 deletions(-)