On Thu, May 19, 2016 at 04:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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 dunno. Certainly I am not running such a site. But something like kernel.org might fit into that boat. For a long time I think people had actual shell accounts and a common git-daemon served the repositories. I think that these days it might be more locked-down, though. > 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" I was hoping to avoid setting up configuration restriction via the configuration files, if only because it implies some ordering in the parsing. So for example, you'd need to do a separate pass to load the restrictions system, and then actually parse the config. I guess that's not too bad with the caching system that's in place, though. > 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. Yeah, I was also hoping to avoid something too baroque. :) I don't know if there's much value in restricting things like gc.auto. If they can make arbitrary edits to the config file, they can run arbitrary code. I think this is _just_ about protecting a git-daemon serving the untrusted repositories (or a user fetching from an untrusted other-user on the system). > 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. One thing I didn't elaborate on is that the "don't respect this key from the repo config" could be made more featureful. For example, your core.allowDangerousHooks could just as easily be an environment variable: $GIT_ALLOW_DANGEROUS_CONFIG. [1] And then you could set that on your servers, and only set uploadpack.packObjectsHook in the repositories you wanted, achieving your goal. This does still leave the pack-objects hook unlike the other hooks (in that it leaves the command in the config rather than in a script), but I actually like that flexibility. Being able to use "git -c" to set the hook for a one-shot invocation is kind of nice (though you do have to do tricks with "--upload-pack=" to get it to cross the remote boundary). -Peff [1] We also talked long ago (in the v1.6.x days, regarding a post-upload-pack hook) of auto-enabling "dangerous" hooks when getuid() matched the owner of the hook. We could do the same thing for the config file (though TBH, it is confusing enough of a rule that I think I prefer something like the explicit environment variable). -- 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