git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Duy Nguyen <pclouds@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 0/5] Multiple hook support
Date: Tue, 23 Apr 2019 19:34:38 -0700	[thread overview]
Message-ID: <20190424023438.GE98980@google.com> (raw)
In-Reply-To: <20190424004948.728326-1-sandals@crustytoothpaste.net>

Hi,

brian m. carlson wrote:

> I've talked with some people about this approach, and they've indicated
> they would prefer a configuration-based approach.

I would, too, mostly because that reduces the problem of securing
hooks to securing configuration.  See
https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
for more on this subject.

More precisely, a few problems with the current hooks system:

 1. There's not a standard way to have multiple hooks for a single event.
    That's what this series is about.

    (The recommended workaround has been to use a trampoline script as
    your hook, and to make that trampoline script implement whatever
    policy for the order of invocation and accumulation of results is
    appropriate, but that's a bit of a cop-out.)

 2. Because they are stored in the Git repository, they do not have a
    way to be automatically updated.

    (The recommended workaround is to use a trampoline script as your
    hook and put the actual hook somewhere standard like $PATH where
    it can be upgraded system-wide.  But that's a bit of a cop-out.)

 3. Because they are part of the Git repository, it is very easy to
    compromise a user's account by tricking them into running an
    attacker-authored hook.  Attacks include "hey admin, can you tell
    me why 'git commit' is failing in this repo?" and "here's a zip file
    containing a Git repository with our fancy software.  Feel free
    to look around, run 'git pull', etc".

    Similar attacks, probably even worse, apply to shell prompt scripts
    using commands from attacker-controlled .git/config.

    (The recommended workaround is to inspect .git/config and
    .git/hooks whenever you're looking at an untrusted repository, and
    to write your shell prompt script defensively.)

Solving (1) without (2) feels like a bit of a missed opportunity to
me.  Ideally, what I would like is

   i. A central registry of trustworthy Git hooks that can be upgraded
      using the system package manager to address (2).  Perhaps just
      git-hook-* commands on the $PATH.

  ii. Instead of putting hooks in .git/hooks, put a list of hooks to
      run for each event in .git/config.

 iii. For backward compatibility, perform a multi-stage migration.
      In the stage I am most interested in:

      When encountering a hook in .git/hooks, don't run it, but print
      a message about how to migrate it to the modern scheme.

      To make migration to the modern scheme painless, stick a
      standard trampoline script in .git/hooks in all converted and
      all newly "git init"ed repositories to allow old versions of Git
      to respect the configuration from (i) and (ii).

That doesn't handle core.pager et al, but those we can handle
separately (for example by, at least optionally, not respecting values
for them in per-repo config at all).

Thanks for tackling this.  What do you think?

Thanks,
Jonathan

  parent reply	other threads:[~2019-04-24  2:34 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-24  0:49 [PATCH 0/5] Multiple hook support brian m. carlson
2019-04-24  0:49 ` [PATCH 1/5] run-command: add preliminary support for multiple hooks brian m. carlson
2019-04-24  2:27   ` Junio C Hamano
2019-04-24 18:48     ` Johannes Sixt
2019-04-25  0:55       ` Junio C Hamano
2019-04-25  9:39         ` Ævar Arnfjörð Bjarmason
2019-04-25 10:04           ` Junio C Hamano
2019-04-25 19:40         ` Johannes Sixt
2019-04-26 20:58           ` brian m. carlson
2019-04-26 21:53             ` Johannes Sixt
2019-04-24 22:32     ` brian m. carlson
2019-04-24  0:49 ` [PATCH 2/5] builtin/receive-pack: add " brian m. carlson
2019-04-24  0:49 ` [PATCH 3/5] sequencer: " brian m. carlson
2019-04-24  9:51   ` Phillip Wood
2019-04-24 22:46     ` brian m. carlson
2019-04-25 14:59       ` Phillip Wood
2019-04-24  0:49 ` [PATCH 4/5] builtin/worktree: add support for multiple post-checkout hooks brian m. carlson
2019-04-24  0:49 ` [PATCH 5/5] transport: add support for multiple pre-push hooks brian m. carlson
2019-04-24  2:09 ` [PATCH 0/5] Multiple hook support Junio C Hamano
2019-04-24  2:22   ` brian m. carlson
2019-04-24  2:41     ` Junio C Hamano
2019-04-24  8:14     ` Ævar Arnfjörð Bjarmason
2019-04-24  2:34 ` Jonathan Nieder [this message]
2019-04-24  7:43   ` Ævar Arnfjörð Bjarmason
2019-04-24  8:22   ` Ævar Arnfjörð Bjarmason
2019-04-24 23:07   ` brian m. carlson
2019-04-24 23:26     ` Jonathan Nieder
2019-04-25 10:08     ` How to undo previously set configuration? (again) Ævar Arnfjörð Bjarmason
2019-04-25 10:43       ` Duy Nguyen
2019-04-25 11:58         ` Ævar Arnfjörð Bjarmason
2019-04-26 15:18           ` Ævar Arnfjörð Bjarmason
2019-04-25 14:36       ` Jonathan Nieder
2019-04-25 14:43         ` Barret Rhoden
2019-04-25 15:27           ` Ævar Arnfjörð Bjarmason
2019-04-25 15:25         ` Ævar Arnfjörð Bjarmason
2019-04-26  2:13         ` Junio C Hamano
2019-04-26  9:36         ` Duy Nguyen
2019-04-30 21:14           ` Jeff King
2019-05-01 11:41             ` Duy Nguyen
2019-05-01 12:18               ` Ævar Arnfjörð Bjarmason
2019-05-01 12:32                 ` Duy Nguyen
2019-05-01 21:09                   ` Jeff King
2019-05-01 21:15                 ` Jeff King
2019-04-24  8:10 ` [PATCH 0/5] Multiple hook support Ævar Arnfjörð Bjarmason
2019-04-24  9:55   ` Phillip Wood
2019-04-24 18:29   ` Bryan Turner
2019-04-24  9:49 ` Duy Nguyen
2019-04-24 22:49   ` brian m. carlson
2019-04-24 23:40   ` brian m. carlson
2019-04-25  0:08     ` Duy Nguyen
2019-04-30 21:39 ` Jeff King

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 \
    --in-reply-to=20190424023438.GE98980@google.com \
    --to=jrnieder@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).