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

[Albert Cui <albertqcui@gmail.com>]

On Mon, Mar 29, 2021 at 4:20 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> > +Security Considerations and Design Principles
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> [snip]
> > +  ** 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 might be worth saying that we want to make configuration of
> project-configured hooks to be approximately as easy/automatic as
> building (that is, the user still has to explicitly run a build, and
> isn't prompted at the end of their clone whether they want to build it
> right away).

+1, I like phrasing it this way.
> > +
> > +* 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.
>    ^~~~~~~~
> Better say "fetch", if we are proposing this magic branch thing.
>
> > +* This configuration should only apply if it was received over HTTPS
>
> Meaning, non-HTTPS fetches should just not update this special branch?

Yes, though I erroneously forgot to include SSH as well. I think the
main issue is
person-in-the-middle type attacks.

> > +* A setup command for users to set up hooks
> AIUI, this is proposed to be part of `git hook`, right?
>
> I don't think it needs to be part of this doc but it'd be nice to also
> support installing just a subset, like:
>
>   git hook setup pre-commit
>   git hook setup --interactive
>
Correct, I think `git hook` is a natural evolution. This is a nice to
have that we can document.

>
> > +Fast Follows
> > +^^^^^^^^^^^^
> > +
> > +* When prompted to execute a hook, users can specify always or never, even if
> > +the hook updates
>
> I think we want to base this on the remote URL, right? I know we talked
> a little offline about how to mitigate vs. malicious maintainer (for
> example this whole mess with The Great Suspender) and I'm not sure what
> solution there might be.
>
> I wonder if it's worth it to notify users that their always-okayed hooks
> were updated during fetch?
>
It definitely aligns with the security principles to notify, even if
they have OK'd updates.

> > +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
>
> Hm, I thought we wanted to consider storing the hook body in the magic
> branch as well? To avoid changing hook implementation during bisect, for
> example?
>

Good question. If we consider this as an extension of config-based
hooks, then I think
it's logical to still support hooks in the repo itself. In
documentation, we might suggest
that people who want to use this feature store the hook in the magic
branch for that
reason.