From: Junio C Hamano <firstname.lastname@example.org> To: "Albert Cui via GitGitGadget" <email@example.com> Cc: firstname.lastname@example.org, "Albert Cui" <email@example.com>, "brian m. carlson" <firstname.lastname@example.org>, "Ævar Arnfjörð Bjarmason" <email@example.com>, "Emily Shaffer" <firstname.lastname@example.org>, "Derrick Stolee" <email@example.com>, "Ed Maste" <firstname.lastname@example.org> Subject: Re: [PATCH v3] hooks: propose project configured hooks Date: Wed, 28 Apr 2021 11:48:57 +0900 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> (Albert Cui via GitGitGadget's message of "Sat, 24 Apr 2021 01:38:09 +0000") "Albert Cui via GitGitGadget" <email@example.com> writes: > diff --git a/Documentation/technical/project-configured-hooks.txt b/Documentation/technical/project-configured-hooks.txt > new file mode 100644 > index 000000000000..eb18eb6aa1b4 > --- /dev/null > +++ b/Documentation/technical/project-configured-hooks.txt > @@ -0,0 +1,528 @@ > +Project Configured Hooks > +------------------------ > + > +// The '+' in Gerrit URL frightens asciidoctor. > +:chrome-os: https://chromium.googlesource.com/chromiumos/docs/+/HEAD/contributing.md#Upload-changes > +:repohook-read: https://android.googlesource.com/platform/tools/repohooks/+/refs/heads/master/rh/config.py > +:repohook-config: https://android.googlesource.com/platform/tools/repohooks/+/refs/heads/master#config-files > + > +Background > +~~~~~~~~~~ > + > +Context > +^^^^^^^ > + > +Git has linkgit:githooks[hooks] functionality to allow users to > +execute commands or scripts when certain Git events occur. Some use cases > +include: > + > +* The `pre-commit` hook event: > +* The `commit-msg` hook event: the project may want to enforce a certain commit > +* The `pre-push` hook: the project may want to verify that pushes are going to > +the correct central repository to prevent leaks. > + > +A common thread between these use cases is to enforce certain behavior across > +many developers working in the same code base, encouraging best practices and > +healthy code quality. I suspect that this point has already been raised, but as all of you who thought about the hooks already know, it is not appropriate to characterize local hooks as a means to satisfy "the project may want to enforce" listed above. It ultimately is up to the project's pre-receive (or update) hook to reject history with house style violations, mistakenly added password files, commit log messages with swearwords, etc. Presenting local hooks as if they can be used as an enforcement mechanism is misleading to our readers. A more understandable description that is easier for readers to accept would be that local hooks can be used to help developers prevent their later pushes from getting rejected, by duplicating (possibly a subset of) the checks the server end uses to judge their contribution. You'll address that in the next section, so the above description that is misleading should be aligned with the next section that deals with "local checks vs receiving end checks", perhaps? As local checks must perform (possibly a subset of) checks done on the receiving end, it probably is a good idea to address how you would plan to manage the "duplication" of the checks. Anything you implement only on the client side "hooks" will not be enforced (they only stay "advisory" at best), unless the same or stricter checks are done on the receiving end. > +A large motivation for this change is providing projects a method to enable > +... > +local hook execution. We think there are still benefits to local checks: Yes, noticing problems early before it gets too late would save developer's frustration and work that needs to be redone. I do not think you'd need to say more than that two lines---the need for enforcing house style, accidental leakage of secrets, etc., is not "there are still benefits to do them locally". > +* Helps developers catch issues earlier: typically developers need to push to > +the remote to trigger server-side checks. Local hooks can be run anytime the > +developer wants. Yes, exactly. So, perhaps the introduction section should say how the checks in the entire change flow helps the project (i.e. house style, leak prevention, etc.), and the ultimate place of validation and rejection is at the receiving side when developers push. Then the "local vs server" section should say why is it good to have local checks at all (i.e. catch issues early), how local hooks should check (possibly a subset of) rules eventually enforced at the receiving end when the developer pushes, and how the duplication is managed (if any). > +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: server-side checks may be too expensive, either monetarily > +or in terms of time investment, to run. 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 personally do not find this convincing. At least, as a document that gives an official recommendation to the users that comes from this project, I would prefer not to see the "However, ... to run" part. I also do not see how one can claim that the number of "local solutions" speak to the need. It can happen when there are many folks who do not understand that a rule not enforced is a rule that people would deem not worth honoring. Depending on the local hooks for any enforcement is akin to depending on obscurity for your security needs. > +Security Threat Model > +~~~~~~~~~~~~~~~~~~~~~ > + > +==== Principles > + > +1. It is unreasonable to require users to audit hook code themselves. > + > + * Users do not have time > + * Users, in general, are unfamiliar with system exploits / security > + * Users may be unfamiliar with a hook’s implementation language > + * Hooks may be compiled binaries and unauditable > + > +2. It is reasonable for users to make a trust decision based on where a hook comes from. > + > + * Society functions on the basis of trust relationships formed between different > + agents --- this is the basis for being able to accomplish more than one person > + can personally accomplish. The complexity is giving users the power to make that > + trust decision. > + * Git users are already making trust decisions based on repository origin. The above only deals with the case where malicious project attacks their developers. But an earlier parts of the document gave (too much---if you ask me) stress on how wonderfully local hooks can enforce projects' wish on developers' behaviour, so there is another threat you would want to cover (if you want to keep that claim in the document, that is): a disobedient developer may bypass the hooks and attacks the project by pushing contents the project do not want to see. That's a threat going the other way, a "malicious" developer attacking the project. > +* [Minimum Feature] A repository owner can update a hook, at which point users who have > +installed it can decide whether to upgrade or uninstall it > + > + ** A good enhancement soon after would be to allow users to indicate at hook > + installation time that they want to accept all updates of this same hook > + from the same origin. That would be a useful usability feature when this eventually becomes widespread use. > +2. Adversary intercepts requests for a repository and inserts malicious hooks > +(person-in-the-middle-type attacks). Mitigation: > + > + ** Only receive hooks suggestions over HTTPS / SSH > + > + ** Include the hook origin (domain name) in the command to install a hook Signed scripts? You may use HTTPS/SSH origin to identify the project, but using something like GPG keys and allowing project developers to give acceptance based on the signer may give us more flexibility. A clone whose contents largely came from insecure means (e.g. http:// cdn perhaps?) could still offer hook scripts as long as they are signed by keys project participants trust, for example. I do not know if that would be an effective remedy for the "shiny new evil maintainer" problem, though. > +4. Adversary exploits an old security issue with a hook that has since been > +patched. Mitigation: > + > + ** Allow users to opt-into hooks getting auto-updated I am not sure what threat you are "mitigating" against here. If the suggested update to hooks come in-band as some Git object tied to the upstream repository (either as part of the main history, or a separate refspace), an adversary that is capable of downgrading a hook can feed a maliciously modified Makefile just as easily, no? > +*hook command*: A user script or executable which will be run on one or more > +hook events. > \ No newline at end of file Oops.
next prev parent reply other threads:[~2021-04-28 2:49 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-18 22:03 [PATCH] hooks: propose repository owner " Albert Cui via GitGitGadget 2021-03-18 22:29 ` Junio C Hamano 2021-03-18 23:45 ` Albert Cui 2021-03-19 1:28 ` brian m. carlson 2021-03-19 10:27 ` Ævar Arnfjörð Bjarmason 2021-04-06 0:35 ` Albert Cui 2021-04-07 22:47 ` Ævar Arnfjörð Bjarmason 2021-06-21 19:36 ` Jonathan Tan 2021-06-21 20:35 ` Ævar Arnfjörð Bjarmason 2021-03-26 1:43 ` [PATCH v2] hooks: propose project " Albert Cui via GitGitGadget 2021-03-29 23:20 ` Emily Shaffer 2021-04-01 20:02 ` Albert Cui 2021-03-30 15:24 ` Derrick Stolee 2021-04-05 22:45 ` Albert Cui 2021-04-05 23:09 ` Junio C Hamano 2021-04-05 23:40 ` Albert Cui 2021-04-06 0:13 ` Junio C Hamano 2021-04-06 0:27 ` Albert Cui 2021-04-06 23:15 ` brian m. carlson 2021-04-07 7:53 ` Ævar Arnfjörð Bjarmason 2021-04-07 13:09 ` Derrick Stolee 2021-04-07 18:40 ` Albert Cui 2021-04-07 20:02 ` Junio C Hamano 2021-04-07 22:23 ` Ævar Arnfjörð Bjarmason 2021-04-15 16:52 ` Ed Maste 2021-04-15 19:41 ` Junio C Hamano 2021-04-15 20:37 ` Ed Maste 2021-04-15 20:50 ` Junio C Hamano 2021-04-15 22:28 ` brian m. carlson 2021-04-02 9:59 ` Ævar Arnfjörð Bjarmason 2021-04-05 23:42 ` Albert Cui 2021-04-02 10:30 ` Ævar Arnfjörð Bjarmason 2021-04-03 0:58 ` Albert Cui 2021-04-24 1:38 ` [PATCH v3] " Albert Cui via GitGitGadget 2021-04-28 2:48 ` Junio C Hamano [this message] 2021-05-05 19:11 ` [PATCH v4] " Albert Cui via GitGitGadget 2021-06-03 3:31 ` Jonathan Tan 2021-06-03 20:16 ` Albert Cui 2021-06-03 22:10 ` Jonathan Tan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH v3] hooks: propose project configured hooks' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).