git-daemon --inetd

git-daemon --inetd

[H. Peter Anvin]

Some people have asked why we haven't enabled git-daemon on kernel.org. 
  The reason is that git-daemon --inetd is broken; there is a patch in 
the current repository which claims to fix it, but there isn't a release 
version yet.

On that note, though, it would be very nice if there was a way to run 
git-daemon in a chroot, or otherwise restrict it to a specific 
subhierarchy of the filesystem.

	-hpa

Re: git-daemon --inetd

[Linus Torvalds]

On Wed, 14 Sep 2005, H. Peter Anvin wrote:
> 
> On that note, though, it would be very nice if there was a way to run 
> git-daemon in a chroot, or otherwise restrict it to a specific 
> subhierarchy of the filesystem.

Hmm.. That should work fine. You can already just run it that way by just 
wrapping it in "chroot", but if you don't want that for some reason, how 
about a patch like this?

It compiles. That's just about all I can say about it.

		Linus

---
diff --git a/daemon.c b/daemon.c
--- a/daemon.c
+++ b/daemon.c
@@ -1,11 +1,19 @@
+/* setresuid/setresgid. GNU? Who are you kidding, glibc? */
+#define _GNU_SOURCE
+
 #include "cache.h"
 #include "pkt-line.h"
 #include <signal.h>
 #include <sys/wait.h>
 #include <sys/socket.h>
 #include <sys/time.h>
+#include <sys/types.h>
 #include <netdb.h>
 #include <netinet/in.h>
+#include <unistd.h>
+
+#include <pwd.h>
+#include <grp.h>
 
 static const char daemon_usage[] = "git-daemon [--inetd | --port=n]";
 
@@ -326,10 +334,50 @@ static int serve(int port)
 	}
 }
 
+/*
+ * This will drop our effective uid/gid to the real one,
+ * even if no "--user=" or "--group=" arguments were used.
+ *
+ * So it's not a no-op even without that.
+ */
+static void set_user_group(const char *user, const char *group)
+{
+	uid_t uid = getuid();
+	gid_t gid = getgid();
+	char *end;
+
+	if (user) {
+		struct passwd *pw = getpwnam(user);
+		if (pw) {
+			uid = pw->pw_uid;
+			gid = pw->pw_gid;
+		} else {
+			uid = strtol(user, &end, 10);
+			if (*end || end == user)
+				die("invalid user name '%s'", user);
+		}
+	}
+	if (group) {
+		struct group *grp = getgrnam(group);
+		if (grp) {
+			gid = grp->gr_gid;
+		} else {
+			gid = strtol(group, &end, 10);
+			if (*end || end == group)
+				die("invalid group name '%s'", group);
+		}
+	}
+	if (!uid || !gid)
+		die("I refuse to run as root");
+	if (setresgid(gid, gid, gid) || setresuid(uid, uid, uid))
+		die("I can't run as %d:%d", uid, gid);
+}
+
 int main(int argc, char **argv)
 {
 	int port = DEFAULT_GIT_PORT;
 	int inetd_mode = 0;
+	const char *user = NULL, *group = NULL;
 	int i;
 
 	for (i = 1; i < argc; i++) {
@@ -350,9 +398,32 @@ int main(int argc, char **argv)
 			continue;
 		}
 
+		if (!strncmp(arg, "--chroot=", 9)) {
+			if (chroot(arg+9) < 0)
+				die("unable to chroot to '%s': %s", arg+9, strerror(errno));
+			if (chdir("/") < 0)
+				die("unable to chdir to new root");
+
+			user = user ? user : "nobody";
+			group = group ? group : "nobody";
+			continue;
+		}
+
+		if (!strncmp(arg, "--user=", 7)) {
+			user = arg+7;
+			continue;
+		}
+
+		if (!strncmp(arg, "--group=", 8)) {
+			group = arg + 8;
+			continue;
+		}
+
 		usage(daemon_usage);
 	}
 
+	set_user_group(user, group);
+
 	if (inetd_mode) {
 		fclose(stderr); //FIXME: workaround
 		return execute();

Re: git-daemon --inetd

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Hmm.. That should work fine. You can already just run it that way by just 
> wrapping it in "chroot", but if you don't want that for some reason, how 
> about a patch like this?
> 
> It compiles. That's just about all I can say about it.
> 

Wrapping it in chroot() would mean having enough things in the chroot 
environment to support starting up programs, which is ugly.  I'll test 
the patch when I get a chance.

	-hpa

Re: git-daemon --inetd

[Linus Torvalds]

On Thu, 15 Sep 2005, H. Peter Anvin wrote:
> 
> Wrapping it in chroot() would mean having enough things in the chroot 
> environment to support starting up programs, which is ugly.  I'll test 
> the patch when I get a chance.

Fair enough. This still requires "git-upload-pack" and any libraries that
requires, of course. You might want to do a statically linked version or
something.

Hmm. A quick check seems to say that git-upload-pack only does libz and 
libc. And the SHA1 stuff, of course, but that you can always get static by 
using the git built-in mozilla versions.

			Linus

Re: git-daemon --inetd

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Thu, 15 Sep 2005, H. Peter Anvin wrote:
> 
>>Wrapping it in chroot() would mean having enough things in the chroot 
>>environment to support starting up programs, which is ugly.  I'll test 
>>the patch when I get a chance.
> 
> 
> Fair enough. This still requires "git-upload-pack" and any libraries that
> requires, of course. You might want to do a statically linked version or
> something.
> 
> Hmm. A quick check seems to say that git-upload-pack only does libz and 
> libc. And the SHA1 stuff, of course, but that you can always get static by 
> using the git built-in mozilla versions.
> 

Could git-upload-pack be integrated into the git-daemon binary, so that 
the exec isn't needed?

	-hpa

Re: git-daemon --inetd

[Linus Torvalds]

On Thu, 15 Sep 2005, H. Peter Anvin wrote:
> 
> Could git-upload-pack be integrated into the git-daemon binary, so that 
> the exec isn't needed?

Hmm.. Looking a bit closer, that's not really very helpful. 

git-upload-pack ends up fork/exec'ing git-rev-list too, to generate the 
list of commits. And then it ends up finishing off with 
"git-pack-objects". 

Yes, we could make it all be one huge thing, and try to have lots of 
functional interfaces. But that would be really quite ugly and big changes 
(they'd have to be asynchronous and give incremental results etc etc).

Maybe a post-1.0 effort. For now, I think you need to have a minimal
execution environment inside the chroot. Not a lot: git-upload-pack,
git-rev-list and git-pack-objects.

Now, it turns out that git-rev-list really is the most complex of them 
all, largely due to --merge-order. If you define NO_OPENSSL, you'll get a 
much simpler execution environment.

Btw, I think --merge-order was cool, but its weaker cousin --topo-order is
what is actually _used_. Maybe we should deprecate --merge-order? Right
now the only real user is git-archimport, and I think that one too really
only wants topo-order too. For example, right now I think git-archimport 
won't actually work if you build without openssl.

Jon? Martin?

		Linus

Re: git-daemon --inetd

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Could git-upload-pack be integrated into the git-daemon binary, so that 
> the exec isn't needed?

In the longer term, I'd like to extend git-daemon-export-ok to
mean:

    - the presense of empty file means any supported operation
      not just git-upload-pack is allowed for backward
      compatibility.

    - otherwise list of allowed git-* server side program.

and be able to extend daemon.c:execute() function to check
and execute server side programs other than git-upload-pack.  So
the answer from me is a mild no.

Re: git-daemon --inetd

[Martin Langhoff]

On 9/16/05, Linus Torvalds <torvalds@osdl.org> wrote:
> Btw, I think --merge-order was cool, but its weaker cousin --topo-order is
> what is actually _used_. Maybe we should deprecate --merge-order? Right
> now the only real user is git-archimport, and I think that one too really
> only wants topo-order too. For example, right now I think git-archimport
> won't actually work if you build without openssl.
> 
> Jon? Martin?

I've used merge-order because it made sense... in that it gave me the
same history and order that walking the arch history would. Most of
the time, anyway, and decided that I'd rather trust GIT over Arch on
merging strategies...

If topo-order is sane, then I'll just change that. Let me run a couple
of tests with it first ;)

WRT SSL... archimport doesn't care a bit about SSL, and if it does,
it's a bug. It calls tla to perform all the "fetch stuff from the Arch
repo operations", because I was too lazy to implement native support
for reading the (trivial) repo format over all the transports. And
with Arch, access to the remote repos is _fast_ (unlike cvs).

cheers,



martin

Re: git-daemon --inetd

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Wrapping it in chroot() would mean having enough things in the chroot 
> environment to support starting up programs, which is ugly.  I'll test 
> the patch when I get a chance.

I'd appreciate a forward of the patch with your acked by if you
find it acceptable for kernel.org use.

I'd apply Linus' patch myself and push it out in the proposed
updates branch, if you prefer.

Re: git-daemon --inetd

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Hmm.. That should work fine. You can already just run it that way by just 
> wrapping it in "chroot", but if you don't want that for some reason, how 
> about a patch like this?

Later exchanges between you and HPA appeared to me that we would
need a chroot environment which has "enough stuff" and that this
patch may not help him very much.  Am I mistaken?

If not, then...

> +		if (!strncmp(arg, "--chroot=", 9)) {
> +			if (chroot(arg+9) < 0)
> +				die("unable to chroot to '%s': %s", arg+9, strerror(errno));
> +			if (chdir("/") < 0)
> +				die("unable to chdir to new root");
> +
> +			user = user ? user : "nobody";
> +			group = group ? group : "nobody";
> +			continue;
> +		}
> +
> +		if (!strncmp(arg, "--user=", 7)) {
> +			user = arg+7;
> +			continue;
> +		}

I think resolving user and group to numeric before you do
chroot() might make the setting up of chrooted environment a
little simpler; no need for supporting getpwnam and getgrnam
there.  On the other hand it may not matter -- you can always
give numeric uid/gid to begin with.

Re: git-daemon --inetd

[H. Peter Anvin]

Junio C Hamano wrote:
> 
> Later exchanges between you and HPA appeared to me that we would
> need a chroot environment which has "enough stuff" and that this
> patch may not help him very much.  Am I mistaken?
> 

Well, building a chroot environment which supports execing is a bit of a 
pain, but it's fully doable.  mount --bind especially makes that quite 
feasible.  It's just more work.

> I think resolving user and group to numeric before you do
> chroot() might make the setting up of chrooted environment a
> little simpler; no need for supporting getpwnam and getgrnam
> there.  On the other hand it may not matter -- you can always
> give numeric uid/gid to begin with.

Yes, resolve the username first.

	-hpa

Re: git-daemon --inetd

[Linus Torvalds]

On Fri, 16 Sep 2005, Junio C Hamano wrote:
> 
> Later exchanges between you and HPA appeared to me that we would
> need a chroot environment which has "enough stuff" and that this
> patch may not help him very much.  Am I mistaken?

No, I think that's correct.

> I think resolving user and group to numeric before you do
> chroot() might make the setting up of chrooted environment a
> little simpler; no need for supporting getpwnam and getgrnam
> there.

Right you are. Much better.

>  On the other hand it may not matter -- you can always
> give numeric uid/gid to begin with.

Well, the symbolic names are much nicer and more readable. So it would be 
better to do the uid/gid translation early, and change the "chroot" thing 
to be done after all that.

It gets a bit messy.. Easy enough to just save a "const char *new_root",
but then you have to split up the "set_user_group()" to be two functions,
around the actual chroot(), since the chroot needs to be done while we're
still root.

I think we can drop the patch for now. It doesn't buy much.

		Linus

Re: git-daemon --inetd

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Well, the symbolic names are much nicer and more readable. So it would be 
> better to do the uid/gid translation early, and change the "chroot" thing 
> to be done after all that.
> 
> It gets a bit messy.. Easy enough to just save a "const char *new_root",
> but then you have to split up the "set_user_group()" to be two functions,
> around the actual chroot(), since the chroot needs to be done while we're
> still root.
> 

Actually, initgroups() and setgroups(), and setgid() for that matter, 
can be done before the chroot().  The only thing that needs to remain 
until the end is setuid().

At one time I played around in tftp-hpa with trying to get Linux to keep 
only CAP_SYS_CHROOT around, but I think I gave up on it.  The way Linux 
capabilities play with the rest of the permission system isn't very 
useful :(

	-hpa

Re: git-daemon --inetd

[Jon Seymour]

On 9/16/05, Martin Langhoff <martin.langhoff@gmail.com> wrote:
> On 9/16/05, Linus Torvalds <torvalds@osdl.org> wrote:
> > Btw, I think --merge-order was cool, but its weaker cousin --topo-order is
> > what is actually _used_. Maybe we should deprecate --merge-order? Right
> > now the only real user is git-archimport, and I think that one too really
> > only wants topo-order too. For example, right now I think git-archimport
> > won't actually work if you build without openssl.
> >
> > Jon? Martin?

Sorry for the long delay in replying - I need to fix my mail filter so
that I do see GIT mail that is actually directed to me!

Is the concern with --merge-order the complexity of the logic (and
hence size of object), the intrusiveness into rev-list.c or the fact
that it uses the OPEN_SSL?

I believe I can rework the merge-order algorithm to avoid the need for
bignum support for OPEN_SSL, though I am not going to get a chance to
do that until December at the earliest.

Much as I hate to see my baby being orphaned, how about we deprecate
--merge-order for now and arrange things so that users have to
explicitly enable it if they want it. If enough people complain about
that, then I can produce a bignum-free version in the December
timeframe?

Regards,

jon.

Re: git-daemon --inetd

[Linus Torvalds]

On Sat, 22 Oct 2005, Jon Seymour wrote:
> 
> Is the concern with --merge-order the complexity of the logic (and
> hence size of object), the intrusiveness into rev-list.c or the fact
> that it uses the OPEN_SSL?

Some of all. But mostly just the basic fact being that right now, nobody 
can really use --merge-order anyway, because if somebody compiles without 
OPEN_SSL, it just won't be there. So it's _practically_ useless.

		Linus