Re: [PATCH] doc: propose hooks managed by the config
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Apr 25, 2020, 20:57 UTC
- Message-ID
- <20200425205727.GB6421@camp.crustytoothpaste.net>
- In-Reply-To
- <20200420235310.94493-1-emilyshaffer@google.com>
On 2020-04-20 at 23:53:10, Emily Shaffer wrote:
Show 13 quoted lines
> +=== 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?
Show 32 quoted lines
> +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.
Show 23 quoted lines
> +== 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?
Show 12 quoted lines
> +== 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@aiede.mtv.corp.google.com/] > +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