From: Duy Nguyen <pclouds@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>
Subject: Re: [PATCH v2] upload-pack: allow shallow fetching from a read-only repository
Date: Wed, 5 Mar 2014 19:43:04 +0700 [thread overview]
Message-ID: <20140305124304.GA27214@lanh> (raw)
In-Reply-To: <xmqqr46hltrl.fsf@gitster.dls.corp.google.com>
On Wed, Mar 5, 2014 at 1:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I notice that the original code, with or without this change, allows
> upload-pack spawned by daemon to attempt to write into GIT_DIR.
> As upload-pack is supposed to be a read-only operation, this is
> quite bad.
>
> Perhaps we should give server operators an option to run their
> daemon -> upload-pack chain to always write to a throw-away
> directory of their choice, without ever attempting to write to
> GIT_DIR it serves?
That would be setting TMPDIR before running git-daemon, I think.
Except places that we ignore TMPDIR like this one.
> How well is the access to the temporary shallow file controlled in
> your code (sorry, but I do not recall carefully reading your patch
> that added the mechanism with security issues in mind, so now I am
> asking)? When it is redirected to TMPDIR (let's forget GIT_DIR for
> now---if an attacker can write into there, the repository is already
> lost), can an attacker race with us to cause us to overwrite we do
> not expect to?
I'm sorry to say that attackers were simply not a concern when I wrote
the patch. Not even that upload-pack is a read-only operation (so
obvious now that I think about this). I think racing is possible, yes.
> Even if it turns out that this patch is secure enough as-is, we
> definitely need to make sure that server operators, who want to keep
> their upload-pack truly a read-only operation, know that it is
> necessary to (1) keep the system user they run git-daemon under
> incapable of writing into GIT_DIR, and (2) make sure TMPDIR points
> at somewhere only git-daemon user and nobody else can write into,
> somewhere in the documentation.
If only there is a way to pass this info without a temporary
file. Multiplexing it to pack-objects' stdin should work. It may be
ugly, but it's probably the safest way.
Wait it does not look that ugly. We can feed "--shallow <SHA1>" lines
before sending want/have/edge objects. Something like this seems to
work (just ran a few shallow-related tests, not the whole test suite)
-- 8< --
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..130097c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2467,6 +2467,14 @@ static void get_object_list(int ac, const char **av)
write_bitmap_index = 0;
continue;
}
+ if (starts_with(line, "--shallow ")) {
+ unsigned char sha1[20];
+ if (get_sha1_hex(line + 10, sha1))
+ die("not an SHA-1 '%s'", line + 10);
+ register_shallow(sha1);
+ /* XXX: set shallow.c:is_shallow = 1 ? */
+ continue;
+ }
die("not a rev '%s'", line);
}
if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a5c50e4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -70,6 +70,14 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
return sz;
}
+static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
+{
+ FILE *fp = cb_data;
+ if (graft->nr_parent == -1)
+ fprintf(fp, "--shallow %s\n", sha1_to_hex(graft->sha1));
+ return 0;
+}
+
static void create_pack_file(void)
{
struct child_process pack_objects;
@@ -81,12 +89,10 @@ static void create_pack_file(void)
const char *argv[12];
int i, arg = 0;
FILE *pipe_fd;
- char *shallow_file = NULL;
if (shallow_nr) {
- shallow_file = setup_temporary_shallow(NULL);
argv[arg++] = "--shallow-file";
- argv[arg++] = shallow_file;
+ argv[arg++] = "";
}
argv[arg++] = "pack-objects";
argv[arg++] = "--revs";
@@ -114,6 +120,9 @@ static void create_pack_file(void)
pipe_fd = xfdopen(pack_objects.in, "w");
+ if (shallow_nr)
+ for_each_commit_graft(write_one_shallow, pipe_fd);
+
for (i = 0; i < want_obj.nr; i++)
fprintf(pipe_fd, "%s\n",
sha1_to_hex(want_obj.objects[i].item->sha1));
@@ -242,12 +251,6 @@ static void create_pack_file(void)
error("git upload-pack: git-pack-objects died with error.");
goto fail;
}
- if (shallow_file) {
- if (*shallow_file)
- unlink(shallow_file);
- free(shallow_file);
- }
-
/* flush the data */
if (0 <= buffered) {
data[0] = buffered;
-- 8< --
--
Duy
next prev parent reply other threads:[~2014-03-05 12:42 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-02-27 7:13 [PATCH] upload-pack: allow shallow fetching from a read-only repository Nguyễn Thái Ngọc Duy
2014-02-27 9:04 ` Jeff King
2014-02-27 9:10 ` [PATCH] shallow: verify shallow file after taking lock Jeff King
2014-02-27 9:22 ` Jeff King
2014-02-27 10:18 ` Duy Nguyen
2014-02-27 10:56 ` [PATCH] shallow: use stat_validity to check for up-to-date file Jeff King
2014-02-27 10:11 ` [PATCH] upload-pack: allow shallow fetching from a read-only repository Duy Nguyen
2014-02-27 11:25 ` [PATCH] shallow: automatically clean up shallow tempfiles Jeff King
2014-03-04 12:30 ` [PATCH v2] upload-pack: allow shallow fetching from a read-only repository Nguyễn Thái Ngọc Duy
2014-03-04 18:10 ` Junio C Hamano
2014-03-05 12:43 ` Duy Nguyen [this message]
2014-03-05 19:50 ` Junio C Hamano
2014-03-06 8:49 ` [PATCH v3] upload-pack: send shallow info over stdin to pack-objects Nguyễn Thái Ngọc Duy
2014-03-06 18:37 ` Junio C Hamano
2014-03-06 23:13 ` Duy Nguyen
2014-03-07 18:27 ` Junio C Hamano
2014-03-08 0:08 ` Duy Nguyen
2014-03-10 15:23 ` Junio C Hamano
2014-03-07 1:24 ` Duy Nguyen
2014-03-07 18:33 ` Junio C Hamano
2014-03-11 12:59 ` [PATCH v4] " Nguyễn Thái Ngọc Duy
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=20140305124304.GA27214@lanh \
--to=pclouds@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.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).