threads / patch / 45655

patch, 3 partsquarantine-push loose ends

Subject: [PATCH 0/3] quarantine-push loose ends

## tl;dr

5 messages between Apr 10, 2017 and Apr 10, 2017. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Jeff King· Apr 10, 2017, 22:10 UTC · lore
On Mon, Apr 10, 2017 at 05:14:24PM -0400, Jeff King wrote:
Show 8 quoted lines
> > Also, I think this whole thing could really do with some documentation
> > in githooks(5). E.g. what hooks does it apply for? The test is just
> > for pre-receive but the patch changes run_update_hook(), does it also
> > take effect for update? Ditto the caveat about update-ref.
> 
> My thinking was that the cases where the effects were user-visible were
> sufficiently insane that nobody would need to know or care when the
> feature was in use.

I guess it can't hurt to write about it, though. Here's that, plus a cleanup on the stray tmp_objdir_env() call you noticed. The final patch provides some safety for the ref-update case. My assumption all along has been that nobody would want to update random refs from inside the pre-receive hook. This doubles down on that by making it forbidden. I don't think that's a big loss, because doing so now is extremely dangerous. If that assumption is wrong, the correct path forward is to make the quarantining configurable.

  [1/3]: receive-pack: drop tmp_objdir_env from run_update_hook
  [2/3]: receive-pack: document user-visible quarantine effects
  [3/3]: refs: reject ref updates while GIT_QUARANTINE_PATH is set
 Documentation/git-receive-pack.txt | 29 +++++++++++++++++++++++++++++
 Documentation/githooks.txt         |  3 +++
 builtin/receive-pack.c             |  1 -
 refs.c                             |  6 ++++++
 t/t5547-push-quarantine.sh         | 11 +++++++++++
 5 files changed, 49 insertions(+), 1 deletion(-)
Jeff King· Apr 10, 2017, 22:13 UTC · re: Jeff King · lore

[PATCH 1/3] receive-pack: drop tmp_objdir_env from run_update_hook

Since 722ff7f87 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03), we have to feed the pre-receive hook the tmp_objdir environment, so that git programs run from the hook know where to find the objects.

That commit modified run_update_hook() to do the same, but there it is a noop. By the time we get to the update hooks, we have already migrated the objects from quarantine, and so tmp_objdir_env() will always return NULL. We can drop this useless call.

Note that the ordering here and the lack of support for the update hook is intentional. The update hook calls are interspersed with actual ref updates, and we must migrate the objects before any refs are updated (since otherwise those refs would appear broken to outside processes). So the only other options are:

  - remain in quarantine for the _first_ ref, but not the
    others. This is sufficiently confusing that it can be
    rejected outright.
  - run all the individual update hooks first, then migrate,
    then update all the refs. But this changes the repository
    state that the update hooks see (i.e., whether or not
    refs from the same push are updated yet or not).

So the functionality is fine and remains unchanged with this patch; we're just cleaning up a useless and confusing line of code.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 1 -
 1 file changed, 1 deletion(-)
Show changes to builtin/receive-pack.c +0 −1
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index aca9c33d8..b4469b3cc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -772,7 +772,6 @@ static int run_update_hook(struct command *cmd)
 	proc.stdout_to_stderr = 1;
 	proc.err = use_sideband ? -1 : 0;
 	proc.argv = argv;
-	proc.env = tmp_objdir_env(tmp_objdir);
 
 	code = start_command(&proc);
 	if (code)
-- 
2.12.2.952.g759391acc
Jeff King· Apr 10, 2017, 22:13 UTC · re: Jeff King · lore

[PATCH 2/3] receive-pack: document user-visible quarantine effects

Commit 722ff7f87 (receive-pack: quarantine objects until pre-receive accepts, 2016-10-03) changed the underlying details of how we take in objects. This is mostly transparent to the user, but there are a few things they might notice. Let's document them.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-receive-pack.txt | 28 ++++++++++++++++++++++++++++
 Documentation/githooks.txt         |  3 +++
 2 files changed, 31 insertions(+)
Show changes to 2 files +31 −0

Documentation/git-receive-pack.txt, Documentation/githooks.txt

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 0ccd5fbc7..7267ecfbe 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -114,6 +114,8 @@ will be performed, and the update, post-receive and post-update
 hooks will not be invoked either.  This can be useful to quickly
 bail out if the update is not to be supported.
 
+See the notes on the quarantine environment below.
+
 update Hook
 -----------
 Before each ref is updated, if $GIT_DIR/hooks/update file exists
@@ -214,6 +216,32 @@ if the repository is packed and is served via a dumb transport.
 	exec git update-server-info
 
 
+Quarantine Environment
+----------------------
+
+When `receive-pack` takes in objects, they are placed into a temporary
+"quarantine" directory within the `$GIT_DIR/objects` directory and
+migrated into the main object store only after the `pre-receive` hook
+has completed. If the push fails before then, the temporary directory is
+removed entirely.
+
+This has a few user-visible effects and caveats:
+
+  1. Pushes which fail due to problems with the incoming pack, missing
+     objects, or due to the `pre-receive` hook will not leave any
+     on-disk data. This is usually helpful to prevent repeated failed
+     pushes from filling up your disk, but can make debugging more
+     challenging.
+
+  2. Any objects created by the `pre-receive` hook will be created in
+     the quarantine directory (and migrated only if it succeeds).
+
+  3. The `pre-receive` hook MUST NOT update any refs to point to
+     quarantined objects. Other programs accessing the repository will
+     not be able to see the objects (and if the pre-receive hook fails,
+     those refs would become corrupted).
+
+
 SEE ALSO
 --------
 linkgit:git-send-pack[1], linkgit:gitnamespaces[7]
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9565dc3fd..32343ae29 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -256,6 +256,9 @@ environment variables will not be set. If the client selects
 to use push options, but doesn't transmit any, the count variable
 will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 
+See the section on "Quarantine Environment" in
+linkgit:git-receive-pack[1] for some caveats.
+
 [[update]]
 update
 ~~~~~~
-- 
2.12.2.952.g759391acc
Jeff King· Apr 10, 2017, 22:14 UTC · re: Jeff King · lore

[PATCH 3/3] refs: reject ref updates while GIT_QUARANTINE_PATH is set

As documented in git-receive-pack(1), updating a ref from within the pre-receive hook is dangerous and can corrupt your repo. This patch forbids ref updates entirely during the hook to make it harder for adventurous hook writers to shoot themselves in the foot.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-receive-pack.txt |  3 ++-
 refs.c                             |  6 ++++++
 t/t5547-push-quarantine.sh         | 11 +++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)
Show changes to 3 files +19 −1

Documentation/git-receive-pack.txt, refs.c, t/t5547-push-quarantine.sh

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 7267ecfbe..86a4b32f0 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -239,7 +239,8 @@ This has a few user-visible effects and caveats:
   3. The `pre-receive` hook MUST NOT update any refs to point to
      quarantined objects. Other programs accessing the repository will
      not be able to see the objects (and if the pre-receive hook fails,
-     those refs would become corrupted).
+     those refs would become corrupted). For safety, any ref updates
+     from within `pre-receive` are automatically rejected.
 
 
 SEE ALSO
diff --git a/refs.c b/refs.c
index 0272e332c..62b405d0b 100644
--- a/refs.c
+++ b/refs.c
@@ -1516,6 +1516,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 {
 	struct ref_store *refs = get_ref_store(NULL);
 
+	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+		strbuf_addstr(err,
+			      _("ref updates forbidden inside quarantine environment"));
+		return -1;
+	}
+
 	return refs->be->transaction_commit(refs, transaction, err);
 }
 
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
index af9fcd833..113c87007 100755
--- a/t/t5547-push-quarantine.sh
+++ b/t/t5547-push-quarantine.sh
@@ -58,4 +58,15 @@ test_expect_success 'push to repo path with path separator (colon)' '
 	git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
 '
 
+test_expect_success 'updating a ref from quarantine is forbidden' '
+	git init --bare update.git &&
+	write_script update.git/hooks/pre-receive <<-\EOF &&
+	read old new refname
+	git update-ref refs/heads/unrelated $new
+	exit 1
+	EOF
+	test_must_fail git push update.git HEAD &&
+	git -C update.git fsck
+'
+
 test_done
-- 
2.12.2.952.g759391acc
Ævar Arnfjörð Bjarmason· Apr 10, 2017, 22:51 UTC · re: Jeff King · lore

Re: [PATCH 0/3] quarantine-push loose ends

On Tue, Apr 11, 2017 at 12:10 AM, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> On Mon, Apr 10, 2017 at 05:14:24PM -0400, Jeff King wrote:
>
>> > Also, I think this whole thing could really do with some documentation
>> > in githooks(5). E.g. what hooks does it apply for? The test is just
>> > for pre-receive but the patch changes run_update_hook(), does it also
>> > take effect for update? Ditto the caveat about update-ref.
>>
>> My thinking was that the cases where the effects were user-visible were
>> sufficiently insane that nobody would need to know or care when the
>> feature was in use.
>
> I guess it can't hurt to write about it, though. Here's that, plus a
> cleanup on the stray tmp_objdir_env() call you noticed. The final patch
> provides some safety for the ref-update case. My assumption all along
> has been that nobody would want to update random refs from inside the
> pre-receive hook. This doubles down on that by making it forbidden. I
> don't think that's a big loss, because doing so now is extremely
> dangerous. If that assumption is wrong, the correct path forward is to
> make the quarantining configurable.

Thanks a lot. Cleared up all the questions I have, and now we have permanent docs & more testing for them.

Show 11 quoted lines
>   [1/3]: receive-pack: drop tmp_objdir_env from run_update_hook
>   [2/3]: receive-pack: document user-visible quarantine effects
>   [3/3]: refs: reject ref updates while GIT_QUARANTINE_PATH is set
>
>  Documentation/git-receive-pack.txt | 29 +++++++++++++++++++++++++++++
>  Documentation/githooks.txt         |  3 +++
>  builtin/receive-pack.c             |  1 -
>  refs.c                             |  6 ++++++
>  t/t5547-push-quarantine.sh         | 11 +++++++++++
>  5 files changed, 49 insertions(+), 1 deletion(-)
>

← back to recent threads