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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 19, 2016, 14:54 UTC
Message-ID: <CACBZZX7nuy3RYPQ5e_FFqvCTStZka49ZSNNe9q2agffa7fp-EA@mail.gmail.com>
In-Reply-To: <20160519120848.GC3050@sigill.intra.peff.net>

On Thu, May 19, 2016 at 2:08 PM, Jeff King <peff@peff.net> wrote:

Show 19 quoted lines

> On Thu, May 19, 2016 at 12:12:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> But as you point out this makes the hook interface a bit unusual.
>> Wouldn't this give us the same security and normalize the hook
>> interface:
>>
>>  * Don't do the uploadpack.packObjectsHook variable, just have a
>> normal "pack-objects" hook that works like any other git hook
>>  * By default we don't run this hook unless core.runDangerousHooks (or
>> whatever we call it) is true.
>>  * The core.runDangerousHooks variable cannot be set on a per-repo
>> basis using your new config facility.
>>  * If there's a pack-objects hook and core.runDangerousHooks isn't
>> true we warn "not executing potentially unsafe hook $path_to_hook" and
>> carry on
>
> 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'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"
        hooksPath                            = "/etc/git/hooks"
        disableHook.pack-objects = false # "true" by default
    $ ls /etc/git/hooks/
    # pre-receive, update etc. would just wrap the repository hook,
    # but pack-objects is global
    pre-receive update pack-objects, [...]

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.

Show 13 quoted lines

>> This would allow use-cases that are a bit inconvenient with your patch
>> (again, if I'm understanding it correctly):
>>
>>  * I can set core.runDangerousHooks=true in /etc/gitconfig on my git
>> server because I also control all the repos, and I want to experiment
>> with trying this on a per-repo basis for users that are cloning from
>> me.
>>  * I can similarly play with this locally knowing I'm only cloning
>> repos I trust by setting core.runDangerousHooks=true in ~/.gitconfig
>
> Yes, those use cases are not well served by the git config alone. But
> you can do them (and much more) once your trusted hook is running (by
> checking $GIT_DIR, or looking in a database, or whatever you want).

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.

I.e. I might test this on one repo on our main git server, or on one active repo, i.e. I don't have to deal with the case where I make some silly syntax/logic error in the uploadpack.packObjectsHook dispatch code (which is only needed for the security consideration) and that impacts all repositories on the machine.