On Wed, Dec 09, 2020 at 04:29:35PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Dec 09 2020, Patrick Steinhardt wrote:
> 
> > this is the fourth version of my patch series which aims to implement a
> > way to pass config entries via the environment while avoiding any
> > requirements to perform shell quoting on the user's side.
> >
> > Given that the What's Cooking report notes that my third version is
> > about to be dropped dropped because the `--config-env` way of doing
> > things is preferred, I've now adopted that approach. I've taken the
> > patch which Peff posted originally (with one change strchr->strrchr) and
> > added documentation and tests to it.
> >
> > This patch series still includes my old proposal as it would actually be
> > a better fit for our usecase at GitLab I have in mind, which is to put
> > all configuration which applies to all git commands into the commands
> > instead of using a config file for this. I have structured the series in
> > such a way though that those patches come last -- so if you continue to
> > think this approach shouldn't make it in, please feel free to drop
> > patches 3-6.
> 
> To add even more to your headaches (sorry!) I hadn't really fully looked
> at that --config-env proposal.
> 
> As noted in my per-patch reply in [1] it will still expose the key part
> of the key=value, and in at least one place (url.<base>.insteadOf) the
> key is where we'll pass the user/password on the command-line still with
> that.

True, that's one of the things I don't quite like about `--config-env`.

> I'd much prefer either your 6/6 over --config-env for that reason & that
> --config-env makes it impossible to pass a key with "=" in. For "-c" I
> don't think that's much of an issue, but e.g. with
> "url.<base>.insteadOf" needing to take arbitrary passwords + us
> implicitly/explicitly advertising this as a "here's how you can pass the
> password" feature not being able to have "=" is more painful.
> 
> I mildly prefer Jeff's suggestion of just getting GIT_CONFIG_PARAMETERS
> to the point where we could document it [2][3] to both of those, but
> that's mostly an asthetic concern of dealing with N values. It won't
> matter for the security aspect (but I think you (but haven't tested)
> that you still can't pass a "=", but your 6/6 does allow that).

Documenting the format would be interesting, but I'm still not quite
sure how it'd be used then. Using a separate `git shell-quote` binary
just to correctly convert the strings to what git would expect doesn't
seem ideal to me, also because it would mean a separate process for each
git invocation which wants to use GIT_CONFIG_PARAMETERS. On the other
hand, reimplementing the shellquoting functionality wherever you want to
use it doesn't sound ideal either.

[snip]

> I do that that whatever we go for this series would be much better if
> the commit messages / added docs explained why we're doing particular
> things, and to users why they'd use one method but not the other.

Makes sense. The commit messages do mention it, but docs don't. I plan
to take your explanation anyway as it's a lot better compared to what I
had, and it does explain why one would want to use `--config-env`.

> E.g. IIRC this whole series is because it's a hassle to invoke
> core.askpass in some stateful program where you'd like to just provide a
> transitory password. I think some brief cross-linking or explanation
> somewhere of these various ways to pass sensitive values around would be
> relly helpful.

It had been the original intention, yes. And it still is, but in fact
the usecase has broadened to also use it to get rid of our global git
config in Gitaly. Which is a little bit awkward to do with
`--config-env` or `-c`, as now a ps(1) would first show a dozen of
configuration values only to have the real command buried somewhere at
the back. It would have been easy to implement though with the
GIT_CONFIG_ envvars.

Granted, we could still do the same by just using GIT_CONFIG_PAREMETERS.
But I'm kind of hesitant to reimplement the shell-quoting procedures in
Gitaly, especially considering that we'd put untrusted data in there.

Patrick