Re: [PATCH v2] hooks: propose project configured hooks

[Derrick Stolee <stolee@gmail.com>]

On 3/25/2021 9:43 PM, Albert Cui via GitGitGadget wrote:
> From: Albert Cui <albertqcui@gmail.com>
> 
> Hooks today are configured at the repository level, making it difficult to
> share hooks across repositories. Configuration-based hook management, by
> moving hooks configuration to the config, makes this much easier. However,
> there is still no good way for project maintainers to encourage or enforce
> adoption of specific hook commands on specific hook events in a repository.
> As such, there are many tools that provide this functionality on top of Git.
> 
> This patch documents the requirements we propose for this feature as well as
> a design sketch for implementation.

Sorry for being so late in reviewing this.

My first reaction is that this feature is suggesting multiple security
vulnerabilities as core functionality. It also seems to be tied to
niche projects (in number of projects, not necessarily the size of those
projects).

I was recommended in conversation to think of this as a way to take
existing ad-hoc behavior and standardize it with a "Git-blessed"
solution. I'm not sure this proposal makes a strong enough case for
why having a "configure-hooks.sh" script in the base of the repo is
not enough. It simultaneously does not use existing precedents like
.gitattributes or .gitignore as direction in using the worktree at
HEAD as a mechanism for communicating details. I find using a separate
ref for hooks to be a non-starter and the design should be rebuilt from
scratch.

I also expect that a significant portion of users will see a message
like "this repository needs hooks" and will just say "yes" to get rid
of the prompt. There needs to be sufficient opportunity for users to
inspect the hook configuration and avoid frustrated or distracted users
from doing the wrong thing.

Server-side checks should always exist, so users who don't follow the
project's guidelines using the recommended hooks will be blocked. The
important thing is that there is an easy way for willing participants
to install the correct hooks. This doesn't mean we should make it
almost automatic.

Also, please proactively pursue a security review of the feature,
including non-technical risks such as social engineering, forks, or
other possible attacks. This idea seems so risky that I would be
against accepting it unless a security expert has done a thorough
review.

> +We propose adding native Git functionality to allow project maintainers to
> +specify hooks that a user ought to install and utilize in their development
> +workflows.

I think providing a way for repository owners to _recommend_ how cloners
should interact with the repository is a good idea. I think starting with
hooks is perhaps a significant jump to the most complicated version of
that idea.

As you think of this design, it might be good to think about how some
recommended Git config (within an allow-list) might fit into this system
as well. I would have started there, with things like "Use partial clone"
or "use sparse-checkout". Those are really things that need to happen at
clone time, they can't really happen after-the-fact, which helps justifying
a modification to 'git clone'. The hook configuration doesn't _need_ to
happen during 'git clone'. More on this timing later.

The .gitattributes file is the closest analogue I could find in current
functionality, but it operates on a path-based scope, not repository scope.

> +Server-side vs Local Checks
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
> +In the ideal world, developers and project maintainers use both local and server
> +side checks in their workflow. However, for many smaller projects, this may not
> +be possible: CI may be too expensive to run or configure. The number of local
> +solutions to this use case speaks to this need (see <<prior-art, Prior Art>>).
> +Bringing this natively to Git can give all these developers a well-supported,
> +secure implementation opposed to the fragmentation we see today.

I'm not sure this is a good selling point for small projects. If they
are small, then the CI to verify commits is cheap(er).

Local hooks should never be used as a replacement for server-side checks.
A user could always use a repository without the local hooks and push
commits that have not been vetted locally. The extreme example is to have
a commit hook that compiles the code and runs all the tests. Would you
then remove all CI builds?

Making it easier to adopt local hooks can avoid some pain points when
users are blocked by the server-side checks.

> +Server-side vs Local Checks
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
> +User Goals / Critical User Journeys
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...

I appreciate the motivation in this document. However, the motivation
doesn't really justify why this should be baked into Git itself, since
a "configure-repo" script in the base of the repo would suffice to
achieve that functionality.

The reason to put this in Git is to standardize this process so it
is not different in each repository. It might be good to spend time
justifying that angle.

> +Security Considerations and Design Principles
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +We must balance the desire to make hooks setup easy for developers --- allowing
> +them to get hooks set up with low friction, and hence increasing the probability
> +of them adopting these hooks --- with protecting users from the security risks
> +of arbitrary code execution on their hosts.
> +
> +To inform the design, we propose these design principles:
> +
> +* User consent: Users must explicitly agree to hooks usage; no hooks should
> +execute without consent, and users should re-consent if hooks update. Users can
> +opt-out of hooks.
> +
> +* Trust comes from the central repository:
> +  ** Most users don't have the time or expertise to properly audit every hook
> +  and what it does. There must be trust between the user and the remote that the
> +  code came from, and the Git project should ensure trust to the degree it can
> +  e.g. enforce HTTPS for its integrity guarantees.
> +
> +  ** Since developers will likely build their local clone in their development
> +  process, at some point, arbitrary code from the repository will be executed.
> +  In this sense, hooks _with user consent_ do not introduce a new attack surface.

It is critical that users are presented with this consent at the correct
times. For instance, I believe configuring local hooks should only be
done _after_ "git clone" completes. That allows a user to inspect the
worktree to their content instead of in the middle of an interactive shell
session or something. (The "git clone" command could output a message to
stderr saying "This repository recommends configuring local hooks. Run
'git <tbd>' to inspect the hooks and configure them.")

We've had enough code-execution bugs with "git clone" that I want to
completely avoid that possibility here.

> +* Give users visibility: Git must allow users to make informed decisions. This
> +means surfacing essential information to the user in a visible manner e.g. what
> +remotes the hooks are coming from, whether the hooks have changed in the latest
> +checkout.

As a user moves HEAD, we should similarly avoid updating the hooks
automatically, but instead present a message to the user to update their
hooks using an intentional command.

> +    ** This could be a path to a script/binary within the repository

Binaries will be tricky if you want users of multiple platforms to
interact with your repository. And scripts can be slower than binaries.

How could someone build hooks from source using your workflow? Perhaps
users are expected to locally compile the code before configuring the
hooks?

> +    ** This could be a path to a script/binary contained within submodules of
> +    the repository

This gives me significant chills. Proceed with caution here.

I understand the reason to want this feature: you could have a suite
of repositories using a common hook set that lives in each as a
submodule. I just want to point out that this adds yet another
dimension for attack.

> +    ** This could be a user installed command or script/binary that exists
> +    outside of the repository and is present in `$PATH`

Like `rm -rf ~/*`? I'm trying to think of dangerous things to do without
elevation. It could help here to clarify the intended user pattern here:
"This repository requires that you install tool X." This seems unlikely
to be necessarily true at clone time, so the users will have a broken
state if they don't run some extra steps. How will that be communicated?

Requirements like these make me think that these repositories would be
better off with a script that configures the hooks after checking if
these things actually exist on the PATH (and installs them if not). I
would lower the priority of this one for now.

> +* This configuration should only apply if it was received over HTTPS

Avoiding http:// and git:// makes sense. Why not SSH?

> +* A setup command for users to set up hooks
> +
> +    ** Hook setup could happen at clone time assuming the user has consented
> +    e.g. if `--setup-hooks` is passed to `git clone`

This is not enough consent.

> +* Users must explicitly approve hooks at least once
> +
> +    ** Running the setup command should count as approval, including if the user
> +    consented during the clone
> +
> +    ** When a hook command changes, a user should re-approve execution (note:
> +    implementation should not interfere with requirement listed in “Fast
> +    Follows")

Users should explicitly approve hooks any time they would change.
They should also be able to explore the source of the change using
whatever editors and tools they want, so the worktree should change
to its new state without new hooks, _then_ the user could consider
updating hooks based on that new state.

> +Fast Follows
> +^^^^^^^^^^^^
> +
> +* When prompted to execute a hook, users can specify always or never, even if
> +the hook updates

I don't understand what this means. "when prompted to execute a hook" are you
saying that the user will get a message saying "Git will now run the pre-commit
hook, are you ok with that?"

"even if the hook updates": I've made my stance clear that the user should be
in complete control of when the hooks update.

> +Out of Scope
> +^^^^^^^^^^^^
> +
> +* Ensuring the user has installed software that isn't distributed with the repo

If you are going to allow hooks to run something on the PATH, then Git
should probably check that such an executable exists before setting the
config and causing problems.

> +Implementation Exploration: Check "magic" branch for configs at fetch time
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Example User Experience
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +===== Case 1: Consent through clone
> +
> +....
> +$ git clone --setup-hooks
> +...
> +
> +The following hooks were installed from remote `origin` ($ORIGIN_URL):
> +
> +pre-commit: git-secrets --pre_commit_hook
> +pre-push:  $GIT_ROOT/pre_push.sh
> +....

Nope. I think this workflow is a non-starter.

> +===== Case 2: Prompting after clone
> +....
> +$ git clone
> +...
> +
> +Remote `origin` ($ORIGIN_URL) suggest installing the following hooks:
> +
> +pre-commit: git-secrets --pre_commit_hook
> +pre-push:  $GIT_ROOT/pre_push.sh

Yes, this works for me.

> +# instead of prompting, we could give users commands to run instead
> +# see case 3
> +
> +Do you wish to install them?
> +1. Yes (this time)
> +2. Yes (always from origin)
> +3. No (not this time)
> +4. No (never)

I'd rather see the installation as a separate step. That gives more
weight to the users' consent. Even if you do have a prompt here that
says Yes/No, *do not* include "always from origin".

> +===== Case 3: Re-prompting when hooks change
> +....
> +$ git pull
> +
> +The following hooks were updated from remote `origin` ($ORIGIN_URL):
> +
> +pre-push:  $GIT_ROOT/pre_push.sh
> +
> +If you wish to install them, run `git hook setup origin`.

Good. Stop here. Perhaps also describe this as something that happens
with "git checkout" because it matters when HEAD updates, even if the
commit was fetched earlier.

> +===== Case 4: Nudging when hooks weren't installed
> +....
> +$ git commit
> +advice: The repository owner has recommended a 'pre-commit' hook that was not run.
> +To view it, run `git show origin/refs/recommended-config:some-pre-commit`. To install it, run `git hook setup origin pre-commit`
> +
> +Turn off this advice by setting config variable advice.missingHook to false."
> +....

These nudges seem like a good pattern, especially with the advice config.

> +Implementation Sketch
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +* Perform fetch as normal
> +
> +* After fetch is complete, Git checks for a "magic" config branch (e.g.
> ++origin/refs/recommended-config+) which contains information about config lines
> +an end-user may want (including hooks).

I think this is the wrong direction to go. You are recommending a few things:

1. Some branch names are more special than others.
2. Hooks live in a separate history than the rest of the repository.
3. Users cannot inspect the hooks in their worktree before installation.

Instead, think about things like .gitignore and .gitattributes, as they can
change as the repository changes. Make a special _filename_ or directory:
for example ".githooks/".

> +* As part of the fetch subcommand, Git prompts users to install the configs
> +contained there.

Prompt users that they are available and can be configured using another
command.

I summarized my thoughts at the top.

Thanks,
-Stolee