On 2020-04-20 at 23:53:10, Emily Shaffer wrote: > +=== Config schema > + > +Hooks can be introduced by editing the configuration manually. There are two new > +sections added, `hook` and `hookcmd`. > + > +==== `hook` > + > +Primarily contains subsections for each hook event. These subsections define > +hook command execution order; hook commands can be specified by passing the > +command directly if no additional configuration is needed, or by passing the > +name of a `hookcmd`. If Git does not find a `hookcmd` whose subsection matches > +the value of the given command string, Git will try to execute the string > +directly. Hook event subsections can also contain per-hook-event settings. Can we say explicitly that the commands are invoked by the shell? Or is the plan to try to parse them without passing to the shell? > +Also contains top-level hook execution settings, for example, > +`hook.warnHookDir`, `hook.runHookDir`, or `hook.disableAll`. > + > +---- > +[hook "pre-commit"] > + command = perl-linter > + command = /usr/bin/git-secrets --pre-commit > + > +[hook "pre-applypatch"] > + command = perl-linter > + error = ignore > + > +[hook] > + warnHookDir = true > + runHookDir = prompt > +---- > + > +==== `hookcmd` > + > +Defines a hook command and its attributes, which will be used when a hook event > +occurs. Unqualified attributes are assumed to apply to this hook during all hook > +events, but event-specific attributes can also be supplied. The example runs > +`/usr/bin/lint-it --language=perl <args passed by Git>`, but for repos which > +include this config, the hook command will be skipped for all events to which > +it's normally subscribed _except_ `pre-commit`. > + > +---- > +[hookcmd "perl-linter"] > + command = /usr/bin/lint-it --language=perl > + skip = true > + pre-commit-skip = false > +---- This seems fine to me. I like this design and it seems sane. > +== Implementation > + > +=== Library > + > +`hook.c` and `hook.h` are responsible for interacting with the config files. In > +the case when the code generating a hook event doesn't have special concerns > +about how to run the hooks, the hook library will provide a basic API to call > +all hooks in config order with an `argv_array` provided by the code which > +generates the hook event: > + > +*`int run_hooks(const char *hookname, struct argv_array *args)`* > + > +This call includes the hook command provided by `run-command.h:find_hook()`; > +eventually, this legacy hook will be gated by a config `hook.runHookDir`. The > +config is checked against a number of cases: > + > +- "no": the legacy hook will not be run > +- "interactive": Git will prompt the user before running the legacy hook > +- "warn": Git will print a warning to stderr before running the legacy hook > +- "yes" (default): Git will silently run the legacy hook > + > +If `hook.runHookDir` is provided more than once, Git will use the most > +restrictive setting provided, for security reasons. I don't think this is consistent with the way the rest of our options work. What if someone generally wants to disable legacy hooks but then works with a program in a repository that requires them? > +== Caveats > + > +=== Security and repo config > + > +Part of the motivation behind this refactor is to mitigate hooks as an attack > +vector;footnote:[https://lore.kernel.org/git/20171002234517.GV19555@xxxxxxxxxxxxxxxxxxxxxxxxx/] > +however, as the design stands, users can still provide hooks in the repo-level > +config, which is included when a repo is zipped and sent elsewhere. The > +security of the repo-level config is still under discussion; this design > +generally assumes the repo-level config is secure, which is not true yet. The > +goal is to avoid an overcomplicated design to work around a problem which has > +ceased to exist. I want to be clear that I'm very much opposed to trying to "secure" the config as a whole. I believe that it's going to ultimately lead to a variety of new and interesting attack vectors and will lead to Git becoming a CVE factory. Vim has this problem with modelines, for example. I think we should maintain the status quo that the only safe things you can do with an untrusted repository are clone and fetch because it sets a clear security boundary. Having said that, I'm otherwise pretty happy with this design and I'm looking forward to seeing it implemented. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Attachment:
signature.asc
Description: PGP signature
