Re: [PATCH] hooks: propose repository owner configured hooks

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 8563 bytes --]

On 2021-03-18 at 22:03:35, Albert Cui via GitGitGadget wrote:
> +User Goals / Critical User Journeys
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +* As a repository owner / project maintainer,
> +
> +    ** I want to enforce code style so I require developers to use a
> +    standardized linter.

This sounds like you're proposing pre-commit hooks for this purpose.
Let me quote you this passage from the FAQ that I wrote that explains my
position perfectly:

  [S]ome advanced users find `pre-commit` hooks to be an impediment to
  workflows that use temporary commits to stage work in progress or that create
  fixup commits, so it's better to push these kinds of checks to the server
  anyway.

It also outlines that pre-commit hooks are not an effective control and
for effective implementation, this must live on the server.

I can never imagine myself wanting to use a pre-commit hook for these
reasons.  It would interrupt my typical workflow which uses many, many
fixup and squash commits and near constant rebases.

Similarly, a pre-push hook prevents me from pushing up in-progress code
to a remote so my colleagues and other interested parties can follow my
progress, contribute design feedback, and, if I need to be out of the
office for a while, pick up my work and continue it.

> +    ** I want to prevent leaks / share code widely so I check that developers
> +    are uploading code to the right place.

This is also not an effective control.  Any user can bypass this
trivially, so there's little point in shipping a hook that does this.

> +    ** I want this to just work for all the developers in my repository, without
> +    needing to support them through configuration.

Unfortunately, in most cases, what hooks are to be used and whether they
should be used is a developer choice, and developers should be able to
use or not use hooks much the same way as they choose an editor.  Thus, a
proposal that a project maintainer should specify a set of hooks to be
used on a developer machine is much like that maintainer proposing that
a developer use a specific editor.

While there are situations where that's done, in general, it's not a
best practice to do so, so I'm generally negative on the idea of
automatically deploying hooks to developer machines.  I think doing so
is a fundamental mistake in the developer process in most circumstances.
Certainly there are exceptions, but I think they are so few that the
burden of running a script is very minor in comparison.

> +Design Principles
> +~~~~~~~~~~~~~~~~~
> +
> +* *Make it easy:* Developers just want to get their work done. Introducing
> +friction is bad, especially when it prevents development from even starting e.g.
> +workspace set up.

I think this assumes that developers need these hooks to be productive,
and I'm not in agreement with that assumption.  For the reasons I've
outlined above, I think that's a misunderstanding about best practices
and how codebases should be structured.

> +* *Treat hooks as software, not configuration:* We take seriously the
> +responsibility that comes with causing arbitrary code to run on a user's
> +machine. Such code needs to come from a reputable source, be automatically
> +updated, and run with user consent.

I don't think we can securely automatically update hooks from a remote
source.  That sounds like a terrible idea from a security perspective.

If users really want this, they can add a post-checkout hook to do so.

> +Feature Requirements

> +* Users must explicitly approve hook execution at least once (yes/no)
> +
> +    ** This could happen during setup or at execution time
> +
> +    ** When a hook command changes, a user should re-approve execution (note:
> +    implementation should not interfere with requirement listed in “Fast
> +    Follows")
> +
> +    * Users do not need to run setup scripts to install hooks --- the setup flow
> +    happens automatically at clone time

This sounds like you're proposing automatically installing hooks, which
would of course be a security vulnerability.  If that's not what you're
proposing, then perhaps you'll want to rephrase this point.

> +* Automation is able to continue to use clone and other commands
> +non-interactively
> +
> +* Works across Windows/Linux/macOS

Git supports other platforms as well.

> +Implementation Exploration: Check "magic" branch for configs at fetch time
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Example workflow
> +^^^^^^^^^^^^^^^^
> +
> +* 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).

As I explain further below, shipping config is a security problem.

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

I don't want to be prompted to install hooks, because I never want to
use hooks from a repository like this.

Moreover, how could a user possibly make an educated decision?  They
can't actually see the hooks at this point because no checkout has
occurred, so the user must make a leap of faith based on the purported
reputation of the repository owner.

In similar situations, such as piping curl to bash, the recommendation
is always to download the script manually, inspect it for any problems
or vulnerabilities, and only then execute it.  This proposal doesn't
allow that.

> +    ** User responses to that prompt could be "sticky" - e.g. a user could reply
> +    "no (this time)", "no (never)", "yes (this time)", or "yes (always)".
> +    Always/never indicate that the user trusts the remote this config is coming
> +    from, and should not apply to configs fetched from other remotes.

"Yes (always)" is almost certainly a security problem.

> +Future Work
> +~~~~~~~~~~~
> +
> +* Extending this to allow repository owners to specify specific configurations
> +in general e.g. this repository should use partial-clone with these parameters.

I don't think any sort of automatic configuration is going to be secure.
Git has had numerous vulnerabilities with the .gitmodules file and it is
highly restricted in its contents.  So I'm pretty much completely
opposed to this since I think it's likely to result in Git becoming a
CVE magnet.

Even if the entire rest of the project disagreed with me and adopted
this, the config would need to be restricted to an allowlist, not a
denylist, because we have good and sufficient evidence that the latter
is not secure.  Even things as simple as changing diff colors can cause
a security problem (e.g., hiding malicious code in a code review), so
I'm having trouble imagining how we could come up with more than a tiny
set of options that could be legitimately shipped securely, at which
point the extra maintenance and development burden becomes excessive.

> +* A way to perform general set up of a code base e.g. installing dependencies,
> +any first-build related tasks

There are already a variety of proposals for this, such as
https://github.com/github/scripts-to-rule-them-all.  I don't believe Git
needs to be in the business of doing this, and all of my concerns about
security are appropriate here as well.

In general, I have grave concerns about this proposal.  It makes
assumptions that users should or will want to use the repository owner's
hooks, which I disagree with.  I think prompting users to install hooks
will encourage them to be much more likely to do so, especially for less
skilled users, which is likely to lead to social engineering attacks.
And overall, this proposal sounds like it does not give enough
consideration to the giant security risks it introduces.  I don't think
this proposal as it stands should be accepted, and I am having a hard
time thinking of any similar proposal I could support.

I appreciate that this proposal does make things slightly easier for a
subset of users and environments, but I think picking a standard set of
scripts for those users and environments and letting users execute them
if and when they want is only slightly more difficult and much more
secure.  I've used that approach at work and it works great, so I feel
confident it can work in most other environments as well.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]