Server side programs

Are git-receive-pack and git-upload-pack the only two binaries that get called directly over a SSH-tunnel?

The reason I'm asking is that I'm adding support for userrelative paths (git pull ssh://host:~user/somedir) and removing the possibilities to use a compromised but limited account for finding out what other useraccounts are available.

Quoted 1 line · Andreas Ericsson

Are git-receive-pack and git-upload-pack the only two binaries that get called directly over a SSH-tunnel?

There are git-ssh-fetch and git-ssh-upload which call each other (and the older name pairs git-ssh-push and git-sh-pull do so as well). However, you do not have to use these commit walkers over ssh; fetch-pack/upload-pack pair work quite well.

I think your question is "what is the absolute minimum set of binaries you need to allow", so I think the two listed are enough. If you want to let your users coming over SSH *create* a new repository on your machine, you would need a bit more, though (namely, shell access to run mkdir and git-init-db).

Quoted 2 lines

The reason I'm asking is that I'm adding support for userrelative paths (git pull ssh://host:~user/somedir) and removing the possibilities to use a compromised but limited account for finding out what other useraccounts are available.

Sorry, it is not clear to me what you are adding. I do this regularly:

	$ git push kernel.org:git/
	$ git fetch kernel.org:git/

to push into and fetch from $HOME/git/ repository on the other end.

Also I can do this already (assuming the other end hangs all users under the same directory, presumably /home/$user):

	$ git fetch kernel.org:../torvalds/git/

Are you in addition trying to let me do this:

	$ git fetch kernel.org:~torvalds/git/

which would work even when ~torvalds == /home/torvalds and ~junio == /home2/junio, without having to tell people where the user home directories are?

At one point, Linus posted an outline of "restricted login shell for use with git over ssh". I think you could start from there, perhaps extend it so that it checks the binaries *and* pathnames the user can specify (e.g. only under your own $HOME is allowed, and no /../ in them, or something silly like that).

Anyways, the attached patch does this. I've tested all the various syntaxes and they work as expected. rsync, http and local files take the same syntax as before. I haven't added support for user-relative paths to the git-daemon (can't see the point, really) although that can be done easily enough.

Makefile +0 -0

index 903c57c..87188ea 100644

connect.c +3 -0

index b171c5d..0d78b3e 100644
@@ -436,33 +436,44 @@ static int git_tcp_connect(int fd[2], co
+	/* leading colon marks relative path for ssh.
+	 * Check for host == url and default to PROTO_SSH to allow
+	 *   $ git fetch kernel.org:git

receive-pack.c +0 -7

index 8f157bc..9a040ff 100644
@@ -265,18 +267,9 @@ int main(int argc, char **argv)
 	if (!dir)
 		usage(receive_pack_usage);
 
-	/* chdir to the directory. If that fails, try appending ".git" */
-	if (chdir(dir) < 0) {
-		if (chdir(mkpath("%s.git", dir)) < 0)
-			die("unable to cd to %s", dir);
-	}
-
-	/* If we have a ".git" directory, chdir to it */

Dear diary, on Sun, Oct 23, 2005 at 11:41:52AM CEST, I got a letter where Andreas Ericsson <[email protected]> told me that...

Quoted 1 line

Anyways, the attached patch does this. I've tested all the various syntaxes and they work as expected. rsync, http and local files take the same syntax as before. I haven't added support for user-relative paths to the git-daemon (can't see the point, really) although that can be done easily enough.

It would be useful to add a [PATCH] tag to subject when you submit a patch, so that we notice it better. ;-)

You don't update the documentation even though there seem to be some syntactic changes. You should at least update

Documentation/pull-fetch-param.txt

Also before Junio asks you, in the followup patches, you might want to sign off the patch if you want it integrated.

Quoted 8 lines

Makefile +0 -0

index 903c57c..87188ea 100644
@@ -359,6 +362,9 @@ git-cherry-pick: git-revert

%.o: %.S

	$(CC) -o $*.o -c $(ALL_CFLAGS) $<

Quoted 6 lines

+$(SERVERSIDE_PROGRAMS) : git-%$X : %.o srvside-ssh.o $(LIB_FILE)

Makefile +2 -0

+	$(CC) $(ALL_CFLAGS) -o $@ $(filter %o,$^) $(LIBS)
+

git-%$X: %.o $(LIB_FILE)

	$(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)

Why are you adding own compilation command, and why is it inconsistent with the git-%$X's one?

Quoted 9 lines

connect.c +3 -0

index b171c5d..0d78b3e 100644
@@ -436,33 +436,44 @@ static int git_tcp_connect(int fd[2], co
+	/* leading colon marks relative path for ssh.
+	 * Check for host == url and default to PROTO_SSH to allow
+	 *   $ git fetch kernel.org:git

connect.c static int git_tcp_connect(int fd[2], co · line 448

Quoted 10 lines

connect.c +8 -0

+	 */
+	if(ptr && (!path || ptr < path)) {
+		if(host == url)
+			protocol = PROTO_SSH;
+
+		if(protocol == PROTO_SSH) {
+			*ptr = '\0';
+			path = ptr + 1;
 		}
 	}

If I understand this right,

	ssh://foo.bar:baz/quux

will make foo.bar the host and baz/quux the path. Please, do NOT do this! It is supposed to be a URL, dammit! And you know, URLs have defined _syntax_, and that's important at least every time the URL gets out of GIT's context. Or stop it calling URL altogether, to prevent any confusion. But in URLs, the space between : and / is a port definition. See also RFC3986 (aka STD066) and RFC2718.

Thanks.

Quoted 16 lines

receive-pack.c +0 -7

index 8f157bc..9a040ff 100644
@@ -265,18 +267,9 @@ int main(int argc, char **argv)
 	if (!dir)
 		usage(receive_pack_usage);
 
-	/* chdir to the directory. If that fails, try appending ".git" */
-	if (chdir(dir) < 0) {
-		if (chdir(mkpath("%s.git", dir)) < 0)
-			die("unable to cd to %s", dir);
-	}
-
-	/* If we have a ".git" directory, chdir to it */

receive-pack.c int main(int argc, char **argv) · line 275

Quoted 10 lines

receive-pack.c +2 -4

-	chdir(".git");
-	putenv("GIT_DIR=.");
+	/* Find the right directory */
+	srvside_chdir(dir, 0);
 
-	if (access("objects", X_OK) < 0 || access("refs/heads", X_OK) < 0)
-		die("%s doesn't appear to be a git directory", dir);
 	write_head_info();
 
 	/* EOF */

No srvside_chdir() declaration?

srvside-ssh.c line 5

Quoted 11 lines

srvside-ssh.c new file +5 -0

new file mode 100644
index 0000000..0ed5d30
@@ -0,0 +1,63 @@
+#include "cache.h"
+#include <unistd.h>
+#include <pwd.h>
+
+extern const char *__progname;

How portable is this? It appears that no standard really defines this, and Google faintly hints at least some Cygwin-related problems...

Quoted 8 lines

upload-pack.c +0 -0

index accdba6..356c9b1 100644
@@ -5,6 +5,7 @@
 #include "object.h"
 

upload-pack.c line 11

Quoted 5 lines

upload-pack.c +1 -0

 static const char upload_pack_usage[] = "git-upload-pack [--strict] [--timeout=nn] <dir>";
+extern void srvside_chdir(const char *path, int strict);
 
 #define MAX_HAS 256
 #define MAX_NEEDS 256

What about a .h file?

Quoted 8 lines

Makefile +0 -0

index 903c57c..87188ea 100644
@@ -359,6 +362,9 @@ git-cherry-pick: git-revert
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 

Makefile git-cherry-pick: git-revert · line 370

Quoted 6 lines

Makefile +3 -0

+$(SERVERSIDE_PROGRAMS) : git-%$X : %.o srvside-ssh.o $(LIB_FILE)
+	$(CC) $(ALL_CFLAGS) -o $@ $(filter %o,$^) $(LIBS)
+
 git-%$X: %.o $(LIB_FILE)
 	$(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)
 

Why are you adding own compilation command, and why is it inconsistent with the git-%$X's one?

Quoted 8 lines · Petr Baudis

Quoted 8 lines

Makefile +0 -0

index 903c57c..87188ea 100644
@@ -359,6 +362,9 @@ git-cherry-pick: git-revert
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 

Quoted 9 lines

Makefile git-cherry-pick: git-revert · line 370

Quoted 6 lines

Makefile +3 -0

+$(SERVERSIDE_PROGRAMS) : git-%$X : %.o srvside-ssh.o $(LIB_FILE)
+	$(CC) $(ALL_CFLAGS) -o $@ $(filter %o,$^) $(LIBS)
+
 git-%$X: %.o $(LIB_FILE)
 	$(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)
 

Why are you adding own compilation command, and why is it inconsistent with the git-%$X's one?

Although I'd prefer the simplicity of putting srvside-ssh.o in LIB_OBJS, this is arguably defensible; it avoids relinking of everything else merely because srvside-ssh.c is changed.

I'm talking only about the compilation command, not about the dependency line.

Quoted 8 lines · Petr Baudis

Quoted 8 lines

Makefile +0 -0

index 903c57c..87188ea 100644
@@ -359,6 +362,9 @@ git-cherry-pick: git-revert
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 

Quoted 9 lines

Makefile git-cherry-pick: git-revert · line 370

Quoted 6 lines

Makefile +3 -0

+$(SERVERSIDE_PROGRAMS) : git-%$X : %.o srvside-ssh.o $(LIB_FILE)
+	$(CC) $(ALL_CFLAGS) -o $@ $(filter %o,$^) $(LIBS)
+
 git-%$X: %.o $(LIB_FILE)
 	$(CC) $(ALL_CFLAGS) -o $@ $(filter %.o,$^) $(LIBS)
 

Why are you adding own compilation command, and why is it inconsistent with the git-%$X's one?

Although I'd prefer the simplicity of putting srvside-ssh.o in LIB_OBJS, this is arguably defensible; it avoids relinking of everything else merely because srvside-ssh.c is changed.

Quoted 8 lines · Junio C Hamano

Andreas Ericsson <[email protected]> writes:
...
At one point, Linus posted an outline of "restricted login shell for use with git over ssh". I think you could start from there, perhaps extend it so that it checks the binaries *and* pathnames the user can specify (e.g. only under your own $HOME is allowed, and no /../ in them, or something silly like that).

I found this in the archives:
http://article.gmane.org/gmane.comp.version-control.git/5784/match=restricted+login

Is that what you're referring to?

Let me know if you want things done differently.

Quoted 11 lines · Andreas Ericsson

Quoted 8 lines · Junio C Hamano

Andreas Ericsson <[email protected]> writes:
...
At one point, Linus posted an outline of "restricted login shell for use with git over ssh". I think you could start from there, perhaps extend it so that it checks the binaries *and* pathnames the user can specify (e.g. only under your own $HOME is allowed, and no /../ in them, or something silly like that).

Quoted 4 lines

I found this in the archives:
http://article.gmane.org/gmane.comp.version-control.git/5784/match=restricted+login

Is that what you're referring to?

No, it is this one:

    http://marc.theaimsgroup.com/?l=git&m=112681457828137&w=2

But it is orthogonal to what you are doing in this patch.

Quoted 1 line

Let me know if you want things done differently.

I think srvside_chdir() should not do the userdir expansion under --strict (otherwise you would need a matching change in daemon.c as well, but I would rather not).

The --strict flag in upload-pack is to make sure git-daemon can see what is being accessed and make its policy decision even before it calls upload-pack. In a pathological case, somebody can create a directory "/~foo/bar/.git", where the "/~foo" directory is different from "/home/foo", and have git-daemon check that the former is OK and call your upload-pack. Your upload-pack uses srvside_chdir() and exposes /home/foo/bar/.git;

this circumvents git-daemon's policy decision, doesn't it?

I also agree with everything Pasky already said.

* In a URL, a colon after hostname means "port number follows". So it was a good intention to make these consistent:

git fetch ssh://kernel.org:git
git fetch kernel.org:git

it should not be done. IOW, if I wanted to use the former form (which I do not think I'd use myself), I should say either one of:

git fetch ssh://kernel.org:~/git
git fetch ssh://kernel.org:~junio/git

Oh, I just noticed you do not handle the former, because you did not have to, but now you need to.

* Use of "extern const char *__progname" is questionable. I could be easily talked into:

- have "extern const char *git_program_name" in cache.h or somewhere;

- convert programs (gradually) to set that at the beginning of main();

- update die() and error() to use that variable when reporting (both callers and implementation) -- this is optional.

See the attached patch, which adds git_progname as a global variable to daemon.c with a minimum of fuzz. The one-liner below will add it to the rest of the programs. GNU sed >= 4.0.9 required.

grep -l "int main" *.c | xargs -- sed -i '/^#include/i#include "main.h"'

cache.h +0 -0

index d776016..db5d667 100644
@@ -45,6 +45,13 @@
 #endif
 #endif
 

main.h new file +12 -0

new file mode 100644
index 0000000..472f134
@@ -0,0 +1,22 @@
+/* unistd.h must be available and the glibc version includes features.h
+ * from it which #defines __GLIBC__ and friends */
+#include <unistd.h>
+#ifndef __GLIBC__
+const char *git_progname;
+static int git_main(int, char **);
+
+int main(int argc, char **argv)
+{
+	char *p;
+	git_progname = p = *argv;
+

Could you please also trim the mails you are replying to a bit?

Dear diary, on Tue, Oct 25, 2005 at 11:11:54AM CEST, I got a letter where Andreas Ericsson <[email protected]> told me that...

Quoted 1 line

grep -l "int main" *.c | xargs -- sed -i '/^#include/i#include "main.h"'

Urgh. Now this is ugly. What about making it a bit more intrusive while quite more saner?

Also, when you already solve this for non-__GLIBC__ systems, I doubt that there is any win in keeping the __GLIBC__-specific hack, except that most developers won't see any bugs in the generic solution since they are using glibc.

Someone said that converting main()s to git_main()s would help the libification effort,

but I suspect that you actually want to set the progname to whatever you call when you call its git_main().

We can make it go into the init section, but that won't be too portable either.

So I'd say just add setup_progname("foo") at the start of your main().

Quoted 6 lines · Petr Baudis

Quoted 1 line

grep -l "int main" *.c | xargs -- sed -i '/^#include/i#include "main.h"'

Urgh. Now this is ugly. What about making it a bit more intrusive while quite more saner?

I'm not sure what you're referring to. The one-liner is a one-liner. It's sort of supposed to be ugly.

Quoted 6 lines · Andreas Ericsson

Quoted 2 lines · Petr Baudis

Urgh. Now this is ugly. What about making it a bit more intrusive while quite more saner?

I'm not sure what you're referring to. The one-liner is a one-liner. It's sort of supposed to be ugly.

Oh. I saw what you meant now. Lots of main.h included. :)

No, I didn't mean the oneliner at all, actually - just the notion that you stealthily hijack main(). We'll see what Junio thinks about it. ;)

Quoted 1 line

So I'd say just add setup_progname("foo") at the start of your main().

Someone said that converting main()s to git_main()s would help the libification effort,

* In a URL, a colon after hostname means "port number follows". So it was a good intention to make these consistent:

git fetch ssh://kernel.org:git
git fetch kernel.org:git

it should not be done. IOW, if I wanted to use the former form (which I do not think I'd use myself), I should say either one of:

git fetch ssh://kernel.org:~/git
git fetch ssh://kernel.org:~junio/git

Oh, I just noticed you do not handle the former, because you did not have to, but now you need to.

No, it is this one:

    http://marc.theaimsgroup.com/?l=git&m=112681457828137&w=2

But it is orthogonal to what you are doing in this patch.

It's incomplete and almost certainly buggy and generally broken, but here's somethign that you _could_ install as "git-shell", and then put that as somebodys shell in /etc/passwd, and it's a start. A very rough start.

Somebody else gets to test it out ;)

shell.c new file +6 -0

+		if (c != '\'') {
+			*dst++ = c;
+			continue;
+		}
+		switch (*++src) {
+		case '\0':

Quoted 15 lines · Linus Torvalds

It's incomplete and almost certainly buggy and generally broken, but here's somethign that you _could_ install as "git-shell", and then put that as somebodys shell in /etc/passwd, and it's a start. A very rough start.

Somebody else gets to test it out ;)

shell.c new file +11 -0

+		if (c != '\'') {
+			*dst++ = c;
+			continue;
+		}
+		switch (*++src) {
+		case '\0':
+			*dst = 0;
+			return arg;
+		case '\\':
+			if (*++src == '\'' &&
+			    *++src == '\'') {

shell.c line 30

Quoted 5 lines

shell.c new file +5 -0

+				*dst = '\'';
+				continue;
+			}
+		/* Fallthrough */
+		default:

I think this misses HPA's addition to minimally suppport csh braindamage (bang bang).

If this is meant to dequote shell-quoted paths, it really should be modal.

If this is meant to dequote shell-quoted paths, it really should be modal.

Quoted 1 line · H. Peter Anvin

If this is meant to dequote shell-quoted paths, it really should be modal.

It _only_ accepts quoted strings, so it "is" modal. It has one mode: string. And it's a bitch about enforcing it, too (it just dies if it wasn't one).

Quoted 1 line

But it is orthogonal to what you are doing in this patch.

Well, not necessarily.

It's quite arguable that sanity testing might be per-user and could be done by the shell. I'm not at all sure that srvside_chdir() should do any extra testing: if you have real ssh access, the user has the right to do anything he damn well pleases.

This adds a very git specific restricted shell, that can be added to /etc/shells and set to the pw_shell in the /etc/passwd file, to give users ability to push into repositories over ssh without giving them full interactive shell acount.

Quoted 5 lines · Junio C Hamano

This adds a very git specific restricted shell, that can be added to /etc/shells and set to the pw_shell in the /etc/passwd file, to give users ability to push into repositories over ssh without giving them full interactive shell acount.

Did you actually test that it works as somebody's login-shell and can be used for pushing?

Did you actually test that it works as somebody's login-shell and can be used for pushing?

I think it should add "pull" functionality too

Are git-receive-pack and git-upload-pack the only two binaries that get called directly over a SSH-tunnel?