On Thu, May 19, 2016 at 12:45 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html