On Thu, May 19, 2016 at 2:08 PM, Jeff King [off-list ref] wrote:

On Thu, May 19, 2016 at 12:12:43PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

quoted

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.