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, 10:12 UTC
Message-ID: <CACBZZX5SVJ2CSB0AS3Lj1A8_S+ejGOPUDn6Sc3whotkyFwxEiA@mail.gmail.com>
In-Reply-To: <20160518224537.GF22443@sigill.intra.peff.net>

On Thu, May 19, 2016 at 12:45 AM, Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

>   3. You may want to insert a caching layer around
>      pack-objects; it is the most CPU- and memory-intensive
>      part of serving a fetch, and its output is a pure
>      function[1] of its input, making it an ideal place to
>      consolidate identical requests.

Cool to see this on the list after we talked briefly about this at Git Merge. Being able to cache this so simply is a great optimization.

As I recall you guys at GitHub ended up writing your own utility to cache output depending on stdin/argv because none existed already.

If anyone on-list knows about a generic command-line utility like that (because apparently Peff couldn't think of any, and neither can I) that would be useful to know.

Show 7 quoted lines

> This hook is unlike the normal hook scripts found in the
> "hooks/" directory of a repository. Because we promise that
> upload-pack is safe to run in an untrusted repository, we
> cannot execute arbitrary code or commands found in the
> repository (neither in hooks/, nor in the config). So
> instead, this hook is triggered from a config variable that
> is explicitly ignored in the per-repo config.

So do I understand correctly that you're trying to guard against the case where you e.g.:

    rsync untrusted.example.com:/tmp/poison.git /tmp/
    git clone /tmp/poison.git /tmp/safe.git

Not hosing your system if the poison.git/config has a uploadpack.packObjectsHook that's "sudo rm -rf /".

And similarly having this run the hook on the remote:

    # foo.example.com has a /etc/gitconfig with
uploadpack.packObjectsHook "sudo rm -rf /";
    echo -n | ssh foo.example.com "git upload-pack /tmp/poison.git

But not this:

    # bar.example.com has a /tmp/poison.git/config with
uploadpack.packObjectsHook "sudo rm -rf /";
    echo -n | ssh foo.example.com "git upload-pack /tmp/poison.git

We've already accepted that "push" hooks like the pre-receive or update hook can do something malicious like this, so on one hand maybe we should say if you scp raw *.git repositories with hooks this sort of thing might happen, or if you ssh to a remote box and run their per-repo hooks it's really their problem to make sure their users don't run malicious hooks on your behalf.

But I agree with you (if I've understand what this actually does) that saying that it's always safe to "git clone" a repository is more valuable and worth jumping through some hoops for.

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