On Thu, May 19, 2016 at 04:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

This is the "could we just set a bool" option I discussed in the commit
message. The problem is that it doesn't let the admin say "I don't trust
these repositories, but I _do_ want to run just this one hook when
serving them, and not any other hooks".

Indeed. I wonder if there's really any overlap in practice between
systems administrators on a central Git server that are going to want
this relatively obscure feature *but* have potentially malicious users
/ repos, or enough to warrant this unusual edge case in how this
specific hook is configured, as opposed to reducing the special case
in how the hook is run with something like core.runDangerousHooks.

I dunno. Certainly I am not running such a site. But something like
kernel.org might fit into that boat. For a long time I think people had
actual shell accounts and a common git-daemon served the repositories. I
think that these days it might be more locked-down, though.

I'm definitely not saying that these patches should be blocked by
this, but it occurs to me that both your uploadpack.packObjectsHook
implementation and my proposed core.runDangerousHooks which normalizes
it a bit in some ways, but leaves it as a special case in others, are
both stumbling their way toward hacks that we might also solve with
some generally configurable restrictions system that takes advantage
of your earlier patches, e.g.:

    $ cat /etc/gitconfig
    # Not "repository" so hooksPath can't be set per-repo
    [core]
        configRestriction                 = "core.hooksPath: system, global"

I was hoping to avoid setting up configuration restriction via the
configuration files, if only because it implies some ordering in the
parsing. So for example, you'd need to do a separate pass to load the
restrictions system, and then actually parse the config.

I guess that's not too bad with the caching system that's in place,
though.

Of course those are some rather large hoops to jump through just to
accomplish this particular thing, but it would be more generally
composable and you could e.g. say users can't disable gc.auto or
whatever on their repos if they're hosted on your server. Which of
course assumes that you control the git binary and they can't run
their own.

Yeah, I was also hoping to avoid something too baroque. :) I don't know
if there's much value in restricting things like gc.auto. If they can
make arbitrary edits to the config file, they can run arbitrary code. I
think this is _just_ about protecting a git-daemon serving the untrusted
repositories (or a user fetching from an untrusted other-user on the
system).

Yeah, the reason I'm prodding you about this is because I want to test
this out at some point, and a *really* nice thing about the Git
configuration facility is that you can test all these sorts of things
on a per-repo basis now due to how all the git-config variables work
now.

With uploadpack.packObjectsHook you *can* do that by defining a global
pass-through hook, but it makes it more of a hassle to test changes
that straddle the divide between testing & production.

One thing I didn't elaborate on is that the "don't respect this key from
the repo config" could be made more featureful. For example, your
core.allowDangerousHooks could just as easily be an environment
variable: $GIT_ALLOW_DANGEROUS_CONFIG. [1] 

And then you could set that on your servers, and only set
uploadpack.packObjectsHook in the repositories you wanted, achieving
your goal.

This does still leave the pack-objects hook unlike the other hooks (in
that it leaves the command in the config rather than in a script), but I
actually like that flexibility. Being able to use "git -c" to set the
hook for a one-shot invocation is kind of nice (though you do have to do
tricks with "--upload-pack=" to get it to cross the remote boundary).

-Peff

[1] We also talked long ago (in the v1.6.x days, regarding a
post-upload-pack hook) of auto-enabling "dangerous" hooks when getuid()
matched the owner of the hook. We could do the same thing for the config
file (though TBH, it is confusing enough of a rule that I think I prefer
something like the explicit environment variable).