Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects

From: Jeff King <peff@peff.net>
Date: May 26, 2016, 05:37 UTC
Message-ID: <20160526053758.GA21580@sigill.intra.peff.net>
In-Reply-To: <CACBZZX7nuy3RYPQ5e_FFqvCTStZka49ZSNNe9q2agffa7fp-EA@mail.gmail.com>

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

Show 11 quoted lines

> > 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.

Show 12 quoted lines

> 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.

Show 6 quoted lines

> 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).

Show 9 quoted lines

> 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).