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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 19, 2016 at 2:08 PM, Jeff King <peff@peff.net> wrote:
> 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.

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