Server side programs

Server side programs

[Andreas Ericsson]

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

The git tutorial explicitly mentions this for git-receive-pack. Several 
of the other docs (git-fetch-pack, git-peek-remote and git-clone-pack) 
mentions git-upload-pack. No other program from the git suite is 
mentioned as an immediate target for ssh connections.

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.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: Server side programs

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> 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).

> 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).

Re: User-relative paths (was: Server side programs)

[Andreas Ericsson]

[-- Attachment #1: Type: text/plain, Size: 3617 bytes --]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>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).
> 

That's good to know.

> 
>>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.
> 

Looking at the old code I see that that should work. I'll re-work the 
patch to take this into account. The help-text was a bit fuzzy however, 
and if you tried

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

it wouldn't work. Here's how I decided it was somehow broken, or at 
least non-intuitive:

	$ git clone ssh://git.op5.se:git foo
	defaulting to local storage area
	ssh: ssh: Name or service not known
	fatal: unexpected EOF


> 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?
> 

Yes. Our servers' home-directories have varying paths and it'd be nice 
to be able to pull other team-members code without having to remember 
how that particular server is configured.

> 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?

There's no reason why git-upload-pack and git-receive-pack couldn't be 
used safely with a perfectly ordinary shell provided we're careful about 
giving the same error message for when a directory is missing and when 
it isn't a git archive.

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.

This patch works without conflicts alongside Johannes Schindelin's 
recent contribution.

Let me know if you want things done differently.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[-- Attachment #2: git-userrelative-paths.diff --]
[-- Type: text/plain, Size: 7655 bytes --]

diff --git a/Makefile b/Makefile
index 903c57c..87188ea 100644
--- a/Makefile
+++ b/Makefile
@@ -115,15 +115,18 @@ PROGRAMS = \
 	git-local-fetch$X git-ls-files$X git-ls-tree$X git-merge-base$X \
 	git-merge-index$X git-mktag$X git-pack-objects$X git-patch-id$X \
 	git-peek-remote$X git-prune-packed$X git-read-tree$X \
-	git-receive-pack$X git-rev-list$X git-rev-parse$X \
+	git-rev-list$X git-rev-parse$X \
 	git-send-pack$X git-show-branch$X \
 	git-show-index$X git-ssh-fetch$X \
 	git-ssh-upload$X git-tar-tree$X git-unpack-file$X \
 	git-unpack-objects$X git-update-index$X git-update-server-info$X \
-	git-upload-pack$X git-verify-pack$X git-write-tree$X \
+	git-verify-pack$X git-write-tree$X \
 	git-update-ref$X git-symbolic-ref$X git-check-ref-format$X \
 	$(SIMPLE_PROGRAMS)
 
+# server side programs called (possibly) over an ssh-tunnel
+SERVERSIDE_PROGRAMS = git-receive-pack$X git-upload-pack$X
+
 # Backward compatibility -- to be removed after 1.0
 PROGRAMS += git-ssh-pull$X git-ssh-push$X
 
@@ -315,7 +318,7 @@ SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)
 export prefix TAR INSTALL DESTDIR SHELL_PATH template_dir
 ### Build rules
 
-all: $(PROGRAMS) $(SCRIPTS)
+all: $(PROGRAMS) $(SCRIPTS) $(SERVERSIDE_PROGRAMS)
 
 all:
 	$(MAKE) -C templates
@@ -359,6 +362,9 @@ git-cherry-pick: git-revert
 %.o: %.S
 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
 
+$(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)
 
@@ -383,6 +389,7 @@ init-db.o: init-db.c
 
 $(LIB_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H)
+$(patsubst git-%$X,%.o,$(SERVERSIDE_PROGRAMS)): $(LIB_H) srvside-ssh.o
 $(DIFF_OBJS): diffcore.h
 
 $(LIB_FILE): $(LIB_OBJS)
@@ -410,9 +417,10 @@ check:
 
 ### Installation rules
 
-install: $(PROGRAMS) $(SCRIPTS)
+install: $(PROGRAMS) $(SCRIPTS) $(SERVERSIDE_PROGRAMS)
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(bindir))
-	$(INSTALL) $(PROGRAMS) $(SCRIPTS) $(call shellquote,$(DESTDIR)$(bindir))
+	$(INSTALL) $(PROGRAMS) $(SERVERSIDE_PROGRAMS) $(SCRIPTS) \
+		$(call shellquote,$(DESTDIR)$(bindir))
 	sh ./cmd-rename.sh $(call shellquote,$(DESTDIR)$(bindir))
 	$(MAKE) -C templates install
 	$(INSTALL) -d -m755 $(call shellquote,$(DESTDIR)$(GIT_PYTHON_DIR))
diff --git a/connect.c b/connect.c
index b171c5d..0d78b3e 100644
--- a/connect.c
+++ b/connect.c
@@ -436,33 +436,44 @@ static int git_tcp_connect(int fd[2], co
 int git_connect(int fd[2], char *url, const char *prog)
 {
 	char command[1024];
-	char *host, *path;
-	char *colon;
+	char *host, *path, *ptr = NULL;
 	int pipefd[2][2];
 	pid_t pid;
 	enum protocol protocol;
 
+	protocol = PROTO_LOCAL;
 	host = NULL;
 	path = url;
-	colon = strchr(url, ':');
-	protocol = PROTO_LOCAL;
-	if (colon) {
-		*colon = 0;
-		host = url;
-		path = colon+1;
-		protocol = PROTO_SSH;
-		if (!memcmp(path, "//", 2)) {
-			char *slash = strchr(path + 2, '/');
-			if (slash) {
-				int nr = slash - path - 2;
-				memmove(path, path+2, nr);
-				path[nr] = 0;
-				protocol = get_protocol(url);
-				host = path;
-				path = slash;
-			}
+	host = strstr(url, "://");
+	if (host) {
+		*host = '\0';
+		host += 3;
+		protocol = get_protocol(url);
+	}
+	else host = url;
+
+	ptr = strchr(host, ':');
+	path = strchr(host, '/');
+
+	/* leading colon marks relative path for ssh.
+	 * Check for host == url and default to PROTO_SSH to allow
+	 *   $ git fetch kernel.org:git
+	 */
+	if(ptr && (!path || ptr < path)) {
+		if(host == url)
+			protocol = PROTO_SSH;
+
+		if(protocol == PROTO_SSH) {
+			*ptr = '\0';
+			path = ptr + 1;
 		}
 	}
+	else if(path != url) {
+		/* null-terminate host and copy path if it isn't relative */
+		ptr = strdup(path);
+		*path = '\0';
+		path = ptr;
+	}
 
 	if (protocol == PROTO_GIT)
 		return git_tcp_connect(fd, prog, host, path);
diff --git a/receive-pack.c b/receive-pack.c
index 8f157bc..9a040ff 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -8,6 +8,8 @@ static const char receive_pack_usage[] =
 
 static const char unpacker[] = "git-unpack-objects";
 
+extern void srvside_chdir(const char *path, int strict);
+
 static int show_ref(const char *path, const unsigned char *sha1)
 {
 	packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
@@ -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 */
-	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 */
diff --git a/srvside-ssh.c b/srvside-ssh.c
new file mode 100644
index 0000000..0ed5d30
--- /dev/null
+++ b/srvside-ssh.c
@@ -0,0 +1,63 @@
+#include "cache.h"
+#include <unistd.h>
+#include <pwd.h>
+
+extern const char *__progname;
+
+
+/*
+ * Provide support for user-relative paths, but carefully.
+ *
+ * If someone has compromised an account with access limited to a few
+ * commands (git-receive-pack, git-upload-pack) we don't want to let
+ * them find out about other users through git.  We prevent that by the
+ * simple expedient of maintaining the 'path' variable as is and always
+ * supplying the same (not entirely) helpful error message.
+ */
+#define DIR_OOPS \
+	die("%s: '%s': unable to chdir or not a git-archive", __progname, path)
+void srvside_chdir(char *path, int strict)
+{
+	char *dir = path;
+	struct passwd *pw;
+
+	if(chdir(path) < 0 && *path == '~') {
+		char *slash;
+		char *user = (char *)path + 1;
+
+		if((slash = strchr(dir, '/')))
+			*slash = '\0';
+
+		if(!(pw = getpwnam(user))) {
+			if(slash)
+				*slash = '/';
+			DIR_OOPS;
+		}
+		if(chdir(pw->pw_dir) < 0) {
+			DIR_OOPS;
+		}
+
+		/* We're in someones homedir so re-insert the slash (for the
+		 * error message) and set dir just beyond it. If there was no
+		 * spoon we supply the local one */
+		if(slash) {
+			*slash = '/';
+			dir = slash + 1;
+		}
+		else
+			dir = "./";
+	}
+
+	/* chdir to the directory. If that fails, try appending ".git" */
+	if (chdir(dir) < 0) {
+		if (strict || chdir(mkpath("%s.git", dir)) < 0)
+			DIR_OOPS;
+	}
+	if (!strict)
+		chdir(".git");
+
+	if (access("objects", X_OK) || access("refs", X_OK))
+		DIR_OOPS;
+
+	putenv("GIT_DIR=.");
+}
diff --git a/upload-pack.c b/upload-pack.c
index accdba6..356c9b1 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -5,6 +5,7 @@
 #include "object.h"
 
 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
@@ -202,7 +203,6 @@ static int upload_pack(void)
 
 int main(int argc, char **argv)
 {
-	const char *dir;
 	int i;
 	int strict = 0;
 
@@ -227,20 +227,10 @@ int main(int argc, char **argv)
 	
 	if (i != argc-1)
 		usage(upload_pack_usage);
-	dir = argv[i];
 
-	/* chdir to the directory. If that fails, try appending ".git" */
-	if (chdir(dir) < 0) {
-		if (strict || chdir(mkpath("%s.git", dir)) < 0)
-			die("git-upload-pack unable to chdir to %s", dir);
-	}
-	if (!strict)
-		chdir(".git");
-
-	if (access("objects", X_OK) || access("refs", X_OK))
-		die("git-upload-pack: %s doesn't seem to be a git archive", dir);
+	/* Find the right directory */
+	srvside_chdir(argv[i], strict);
 
-	putenv("GIT_DIR=.");
 	upload_pack();
 	return 0;
 }

Re: User-relative paths (was: Server side programs)

[Petr Baudis]

Dear diary, on Sun, Oct 23, 2005 at 11:41:52AM CEST, I got a letter
where Andreas Ericsson <ae@op5.se> told me 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.

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.

> diff --git a/Makefile b/Makefile
> index 903c57c..87188ea 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -359,6 +362,9 @@ git-cherry-pick: git-revert
>  %.o: %.S
>  	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
>  
> +$(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?

> diff --git a/connect.c b/connect.c
> index b171c5d..0d78b3e 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -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
> +	 */
> +	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.

> diff --git a/receive-pack.c b/receive-pack.c
> index 8f157bc..9a040ff 100644
> --- a/receive-pack.c
> +++ b/receive-pack.c
> @@ -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 */
> -	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?

> diff --git a/srvside-ssh.c b/srvside-ssh.c
> new file mode 100644
> index 0000000..0ed5d30
> --- /dev/null
> +++ b/srvside-ssh.c
> @@ -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...

> diff --git a/upload-pack.c b/upload-pack.c
> index accdba6..356c9b1 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -5,6 +5,7 @@
>  #include "object.h"
>  
>  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?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: User-relative paths

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

>> diff --git a/Makefile b/Makefile
>> index 903c57c..87188ea 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -359,6 +362,9 @@ git-cherry-pick: git-revert
>>  %.o: %.S
>>  	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
>>  
>> +$(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.

Re: User-relative paths

[Petr Baudis]

Dear diary, on Sun, Oct 23, 2005 at 09:50:25PM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> Petr Baudis <pasky@suse.cz> writes:
> 
> >> diff --git a/Makefile b/Makefile
> >> index 903c57c..87188ea 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -359,6 +362,9 @@ git-cherry-pick: git-revert
> >>  %.o: %.S
> >>  	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
> >>  
> >> +$(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.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: User-relative paths

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

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

Ah, I missed that typo.  Thanks.

Re: User-relative paths

[Daniel Barkalow]

On Sun, 23 Oct 2005, Junio C Hamano wrote:

> Petr Baudis <pasky@suse.cz> writes:
> 
> >> diff --git a/Makefile b/Makefile
> >> index 903c57c..87188ea 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -359,6 +362,9 @@ git-cherry-pick: git-revert
> >>  %.o: %.S
> >>  	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
> >>  
> >> +$(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.

The line:

$(SERVERSIDE_PROGRAMS): srvside-ssh.o

would suffice for that.

	-Daniel
*This .sig left intentionally blank*

Re: User-relative paths

[Andreas Ericsson]

Petr Baudis wrote:
> Dear diary, on Sun, Oct 23, 2005 at 11:41:52AM CEST, I got a letter
> where Andreas Ericsson <ae@op5.se> told me 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.
> 
> 
> It would be useful to add a [PATCH] tag to subject when you submit a
> patch, so that we notice it better. ;-)
> 

Will do in the future. I thought it was auto-imported for buildtest if 
it was.

> 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
> 

True. I'll need to re-work the patch a bit to take Junio's RFC on paths 
into account. I'll do this then.

> Also before Junio asks you, in the followup patches, you might want to
> sign off the patch if you want it integrated.
> 
> 
>>diff --git a/Makefile b/Makefile
>>index 903c57c..87188ea 100644
>>--- a/Makefile
>>+++ b/Makefile
>>@@ -359,6 +362,9 @@ git-cherry-pick: git-revert
>> %.o: %.S
>> 	$(CC) -o $*.o -c $(ALL_CFLAGS) $<
>> 
>>+$(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?
> 

Mainly because I'm really no good at Makefiles and just noticed that 
this seems to do what I want. My own projects rarely stretch over 15 
files and it's usually just one or two binaries, so I haven't gotten 
round to learning the finer points of make.

> 
>>diff --git a/connect.c b/connect.c
>>index b171c5d..0d78b3e 100644
>>--- a/connect.c
>>+++ b/connect.c
>>@@ -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
>>+	 */
>>+	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.
> 

Right you are. I was thinking scp like syntax rather than url.

> 
>>diff --git a/receive-pack.c b/receive-pack.c
>>index 8f157bc..9a040ff 100644
>>--- a/receive-pack.c
>>+++ b/receive-pack.c
>>@@ -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 */
>>-	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?
> 
> 
>>diff --git a/srvside-ssh.c b/srvside-ssh.c
>>new file mode 100644
>>index 0000000..0ed5d30
>>--- /dev/null
>>+++ b/srvside-ssh.c
>>@@ -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...
> 

Somewhat, but not very. It was more of a quick hack since the old code 
had hardcoded program names. If this gets supported in git-daemon as 
well it should say "git-daemon" in the error message, so I think either 
pass it as a parameter or invent some git_progname variable and use some 
small init-code for all programs.

> 
>>diff --git a/upload-pack.c b/upload-pack.c
>>index accdba6..356c9b1 100644
>>--- a/upload-pack.c
>>+++ b/upload-pack.c
>>@@ -5,6 +5,7 @@
>> #include "object.h"
>> 
>> 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?
> 

Prototype patch, sort of, and since it's only one function I thought 
it'd be better to keep it as unobtrusive as possible.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: User-relative paths

[Junio C Hamano]

Andreas Ericsson <ae@op5.se> writes:

> Junio C Hamano wrote:
>> Andreas Ericsson <ae@op5.se> 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?

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.

> 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.

Re: User-relative paths

[Linus Torvalds]

On Sun, 23 Oct 2005, Junio C Hamano wrote:
> 
> 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.

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.

So it's quite possible that you should do testing in the thing that 
receives the request, ie in a restricted shell (or, as we already do, in 
git-daemon).

I tried to find my original unquote example and stupid shell, but 
couldn't.  So I wrote something untested as usual.

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 ;)

		Linus

---
2906a25bbd1dedbd6bab9ed984a503340229b020
diff --git a/Makefile b/Makefile
index 7eacf61..34bbdb6 100644
--- a/Makefile
+++ b/Makefile
@@ -102,7 +102,7 @@ SCRIPT_PYTHON = \
 # The ones that do not have to link with lcrypto nor lz.
 SIMPLE_PROGRAMS = \
 	git-get-tar-commit-id$X git-mailinfo$X git-mailsplit$X \
-	git-stripspace$X git-var$X git-daemon$X
+	git-stripspace$X git-var$X git-daemon$X git-shell$X
 
 # ... and all the rest
 PROGRAMS = \
diff --git a/shell.c b/shell.c
new file mode 100644
index 0000000..676d398
--- /dev/null
+++ b/shell.c
@@ -0,0 +1,89 @@
+#include "cache.h"
+
+static char *dequote(char *arg)
+{
+	char *dst = arg;
+	char *src = arg;
+	char c;
+
+	if (*src != '\'')
+		return NULL;
+	for (;;) {
+		c = *++src;
+		if (!c)
+			return NULL;
+		if (c != '\'') {
+			*dst++ = c;
+			continue;
+		}
+		switch (*++src) {
+		case '\0':
+			*dst = 0;
+			return arg;
+		case '\\':
+			if (*++src == '\'' &&
+			    *++src == '\'') {
+				*dst = '\'';
+				continue;
+			}
+		/* Fallthrough */
+		default:
+			return NULL;
+		}
+	}
+}
+
+static int do_receive_pack(char *arg)
+{
+	char cwd[1000];
+	char *my_argv[4];
+
+	arg = dequote(arg);
+	if (!arg)	
+		die("bad argument");
+
+	my_argv[0] = "git-receive-pack";
+	my_argv[1] = arg;
+	my_argv[2] = NULL;
+	return execvp("git-receive-pack", my_argv);
+}
+
+static struct commands {
+	const char *name;
+	int (*exec)(char *arg);
+} cmd_list[] = {
+	{ "git-receive-pack", do_receive_pack },
+	{ NULL },
+};
+
+int main(int argc, char **argv)
+{
+	char *prog;
+	struct commands *cmd;
+
+	/* We want to see "-c cmd args", and nothing else */
+	if (argc != 3 || strcmp(argv[1], "-c"))
+		die("What do you think I am? A shell?");
+	prog = argv[2];
+	argv += 2;
+	argc -= 2;
+	for (cmd = cmd_list ; cmd->name ; cmd++) {
+		int len = strlen(cmd->name);
+		char *arg;
+		if (strncmp(cmd->name, prog, len))
+			continue;
+		arg = NULL;
+		switch (prog[len]) {
+		case '\0':
+			arg = NULL;
+			break;
+		case ' ':
+			arg = prog + len + 1;
+			break;
+		default:
+			continue;
+		}
+		exit(cmd->exec(arg));
+	}
+	die("unrecognized command '%s'", prog);
+}

Re: User-relative paths

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

>> 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.

The point of the patch, unless I am mistaken, is to add ~user/
expansion to the pathname grokking, so that a remote user does
not have to know exactly where on the server each user's home
directory is.

I agree 100% with you that srvside_chdir() is not the place to
do policy checking.  In order to avoid the aliasing problem
(which motivated HPA to add --strict option), the receiving end,
be it git-daemon driving upload-pack or git-shell driving
receive-pack or upload-pack, can do ~user/ expansion first, then
run their policy checking on the canonicalized path before
spawning the lower level programs using the already
canonicalized path.

To also support the case where upload-pack and receive-pack are
started directly from the ssh connection, these programs need to
apply ~user/ expansion to the incoming path themselves by
default.  In order to avoid double expansion, git-daemon and
git-shell should pass --no-user-expansion flag to the lower
level programs when it starts them if we do this.

A common library that takes the path supplied from the other end
and does ~user/ expansion would be useful for the above; we can
lift that logic from Andreas' srvside_chdir().

Re: User-relative paths

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> 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 ;)
>
> +		if (c != '\'') {
> +			*dst++ = c;
> +			continue;
> +		}
> +		switch (*++src) {
> +		case '\0':
> +			*dst = 0;
> +			return arg;
> +		case '\\':
> +			if (*++src == '\'' &&
> +			    *++src == '\'') {
> +				*dst = '\'';
> +				continue;
> +			}
> +		/* Fallthrough */
> +		default:

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

Re: User-relative paths

[H. Peter Anvin]

Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> 
>>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 ;)
>>
>>+		if (c != '\'') {
>>+			*dst++ = c;
>>+			continue;
>>+		}
>>+		switch (*++src) {
>>+		case '\0':
>>+			*dst = 0;
>>+			return arg;
>>+		case '\\':
>>+			if (*++src == '\'' &&
>>+			    *++src == '\'') {
>>+				*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.

	-hpa

Re: User-relative paths

[Linus Torvalds]

On Sun, 23 Oct 2005, H. Peter Anvin wrote:
> 
> 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).

		Linus

Re: User-relative paths

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Sun, 23 Oct 2005, H. Peter Anvin wrote:
> 
>>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).
> 

That wasn't what I meant.  '...' is a modal escape in the shell.  Thus, 
something like this which actually mimics the state machine, at least 
for the potential characters we care about.


#define EMIT(x) { ( ++len < n ) && *dst++ = (x) )

int unquote(char *dst, size_t n, const char *src)
{
	enum state st = { st_zero, st_quote, st_escape };
	int len = 0;
	char c;
	
	while ( (c = *src++) ) {
		switch ( st ) {
		case st_zero:
			if ( c == '\'' )
				st = st_quote;
			else if ( c == '\\' )
				st = st_escape;
			else
				EMIT(c);
			break;
			
		case st_quote:
			if ( c == '\'' )
				st = st_zero;
			else
				EMIT(c);
			break;
			
		case st_escape:
			EMIT(c);
			st = st_zero;
			break;
		}
	}
	
	if ( n )
		*dst = 0;
	
	return (st == st_zero) ? len : -1;
}

Re: User-relative paths

[Junio C Hamano]

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

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

You are right, but this is not meant to dequote arbitrary shell
quoted paths.  It specifically is for unwrapping what sq_quote()
produced, and refuses to proceed if you feed some shell-valid
but not sq_quote() produced string.  The reason is because it is
part of the git-shell "login shell" -- the input validation is
enforced there.

And the reason it lives next to quote.c::sq_quote() is that you
would hopefully notice sq_dequote() needs matching changes when
you ever update sq_quote.

[PATCH] Add git-shell.

[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.

[jc: I updated Linus' patch to match what the current sq_quote()
 does.]

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

   This one has the '!' dequoting I mentioned earlier, and
   actually has been tested twice.

   The system administrator should set things up so that the
   system default PATH environment variable lets users run the
   supported commands.

   We currently rely on the user to have a full shell access for
   repository administrative actions (e.g. git-init-db to create
   a repository, git-repack, and hooks management).  Probably
   some of them may need to become accessible from git-shell,
   but I do not know which ones offhand:

   - Creating a repository.  Probably wherever the user has
     write privilege, or maybe only under the home directory --
     the policy would be up to the system administrator, with
     usual filesystem quota applied.

   - I think setting up hooks should be forbidden -- the user
     can execute arbitrary commands if we allowed it.  Instead,
     the administrator can set things up for people, and would
     probably make creative use of hooks/update to implement
     access control (e.g. forbidding non-fast-forward pushes).

   - Similarly, packing can be left to hooks/post-update or cron
     job, either of which is under administrator control.

 Makefile |    2 +-
 quote.c  |   41 ++++++++++++++++++++++++++++++++++++++++-
 quote.h  |    6 ++++++
 shell.c  |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 2 deletions(-)
 create mode 100644 shell.c

applies-to: c043f0993afe5c057409ef748ce313c89c20c7dc
f176536e2cd5949fa134d8b24dc07dd1f0744b6f
diff --git a/Makefile b/Makefile
index 5bdf3cc..5b0306d 100644
--- a/Makefile
+++ b/Makefile
@@ -116,7 +116,7 @@ PROGRAMS = \
 	git-merge-index$X git-mktag$X git-pack-objects$X git-patch-id$X \
 	git-peek-remote$X git-prune-packed$X git-read-tree$X \
 	git-receive-pack$X git-rev-list$X git-rev-parse$X \
-	git-send-pack$X git-show-branch$X \
+	git-send-pack$X git-show-branch$X git-shell$X \
 	git-show-index$X git-ssh-fetch$X \
 	git-ssh-upload$X git-tar-tree$X git-unpack-file$X \
 	git-unpack-objects$X git-update-index$X git-update-server-info$X \
diff --git a/quote.c b/quote.c
index 009e694..e662a7d 100644
--- a/quote.c
+++ b/quote.c
@@ -15,6 +15,11 @@
 #undef EMIT
 #define EMIT(x) ( (++len < n) && (*bp++ = (x)) )
 
+static inline int need_bs_quote(char c)
+{
+	return (c == '\'' || c == '!');
+}
+
 size_t sq_quote_buf(char *dst, size_t n, const char *src)
 {
 	char c;
@@ -23,7 +28,7 @@ size_t sq_quote_buf(char *dst, size_t n,
 
 	EMIT('\'');
 	while ((c = *src++)) {
-		if (c == '\'' || c == '!') {
+		if (need_bs_quote(c)) {
 			EMIT('\'');
 			EMIT('\\');
 			EMIT(c);
@@ -52,6 +57,40 @@ char *sq_quote(const char *src)
 	return buf;
 }
 
+char *sq_dequote(char *arg)
+{
+	char *dst = arg;
+	char *src = arg;
+	char c;
+
+	if (*src != '\'')
+		return NULL;
+	for (;;) {
+		c = *++src;
+		if (!c)
+			return NULL;
+		if (c != '\'') {
+			*dst++ = c;
+			continue;
+		}
+		/* We stepped out of sq */
+		switch (*++src) {
+		case '\0':
+			*dst = 0;
+			return arg;
+		case '\\':
+			c = *++src;
+			if (need_bs_quote(c) && *++src == '\'') {
+				*dst++ = c;
+				continue;
+			}
+		/* Fallthrough */
+		default:
+			return NULL;
+		}
+	}
+}
+
 /*
  * C-style name quoting.
  *
diff --git a/quote.h b/quote.h
index 2fdde3b..2486e6e 100644
--- a/quote.h
+++ b/quote.h
@@ -31,6 +31,12 @@
 extern char *sq_quote(const char *src);
 extern size_t sq_quote_buf(char *dst, size_t n, const char *src);
 
+/* This unwraps what sq_quote() produces in place, but returns
+ * NULL if the input does not look like what sq_quote would have
+ * produced.
+ */
+extern char *sq_dequote(char *);
+
 extern int quote_c_style(const char *name, char *outbuf, FILE *outfp,
 			 int nodq);
 extern char *unquote_c_style(const char *quoted, const char **endp);
diff --git a/shell.c b/shell.c
new file mode 100644
index 0000000..2c4789e
--- /dev/null
+++ b/shell.c
@@ -0,0 +1,59 @@
+#include "cache.h"
+#include "quote.h"
+
+static int do_generic_cmd(const char *me, char *arg)
+{
+	const char *my_argv[4];
+
+	arg = sq_dequote(arg);
+	if (!arg)
+		die("bad argument");
+
+	my_argv[0] = me;
+	my_argv[1] = arg;
+	my_argv[2] = NULL;
+
+	return execvp(me, (char**) my_argv);
+}
+
+static struct commands {
+	const char *name;
+	int (*exec)(const char *me, char *arg);
+} cmd_list[] = {
+	{ "git-receive-pack", do_generic_cmd },
+	{ "git-upload-pack", do_generic_cmd },
+	{ NULL },
+};
+
+int main(int argc, char **argv)
+{
+	char *prog;
+	struct commands *cmd;
+
+	/* We want to see "-c cmd args", and nothing else */
+	if (argc != 3 || strcmp(argv[1], "-c"))
+		die("What do you think I am? A shell?");
+
+	prog = argv[2];
+	argv += 2;
+	argc -= 2;
+	for (cmd = cmd_list ; cmd->name ; cmd++) {
+		int len = strlen(cmd->name);
+		char *arg;
+		if (strncmp(cmd->name, prog, len))
+			continue;
+		arg = NULL;
+		switch (prog[len]) {
+		case '\0':
+			arg = NULL;
+			break;
+		case ' ':
+			arg = prog + len + 1;
+			break;
+		default:
+			continue;
+		}
+		exit(cmd->exec(cmd->name, arg));
+	}
+	die("unrecognized command '%s'", prog);
+}
---
0.99.8.GIT

Re: [PATCH] Add git-shell.

[Linus Torvalds]

On Sun, 23 Oct 2005, Junio C Hamano wrote:
>
> 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?

I think it should add "pull" functionality too, so that you can have 
restricted reading (hey, git may be open source, but not everything that 
is maintained in it necessarily will be..)

			Linus

Re: [PATCH] Add git-shell.

[Linus Torvalds]

On Sun, 23 Oct 2005, Linus Torvalds wrote:
> 
> 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

Gaah, I only read your description, didn't look closer at the patch. The 
description just said "push", but you added the pull side too, and 
apparently even tested it. Goodie.

Never mind me.

			Linus

Re: [PATCH] Add git-shell.

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Sun, 23 Oct 2005, Junio C Hamano wrote:
>>
>> 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?

I made a temporary user on my notebook, set it as his login
shell, and run peek-remote against it (which means the git-shell
spawned upload-pack on the other end).  No, I did not try pushing
when I sent the patch

But I just did, and it seems to work, so this should hit the
master branch soon ;-).

Re: User-relative paths

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

>  * 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.

The "should be this way" examples should have been something
like this:

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

Otherwise what I said does not make *any* sense.

          	 Junio "Oh, I am an idiot" Hamano

[PATCH] git_progname (was: Re: User-relative paths)

[Andreas Ericsson]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

Junio C Hamano wrote:
> Andreas Ericsson <ae@op5.se> writes:
> 
> 
>>Junio C Hamano wrote:
>>
>>>Andreas Ericsson <ae@op5.se> 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?
> 
> 
> 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.
> 
> 
>>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).
> 

True. I'll rework it.

> 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;


It shouldn't, because srvside_chdir() will only user-expand paths that 
start with a tilde.


> 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();
> 

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"'

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

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[-- Attachment #2: git_progname.diff --]
[-- Type: text/plain, Size: 1666 bytes --]

diff --git a/Makefile b/Makefile
index 5b0306d..f8e4511 100644
--- a/Makefile
+++ b/Makefile
@@ -147,7 +147,7 @@ LIB_FILE=libgit.a
 LIB_H = \
 	blob.h cache.h commit.h count-delta.h csum-file.h delta.h \
 	diff.h epoch.h object.h pack.h pkt-line.h quote.h refs.h \
-	run-command.h strbuf.h tag.h tree.h
+	run-command.h strbuf.h tag.h tree.h main.h
 
 DIFF_OBJS = \
 	diff.o diffcore-break.o diffcore-order.o diffcore-pathspec.o \
diff --git a/cache.h b/cache.h
index d776016..db5d667 100644
--- a/cache.h
+++ b/cache.h
@@ -45,6 +45,13 @@
 #endif
 #endif
 
+#if defined(__GLIBC__)
+extern const char *__progname;
+#define git_progname __progname
+#else
+extern const char *git_progname;
+#endif
+
 /*
  * Intensive research over the course of many years has shown that
  * port 9418 is totally unused by anything else. Or
diff --git a/daemon.c b/daemon.c
index 0c6182f..c197ee5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,3 +1,4 @@
+#include "main.h"
 #include "cache.h"
 #include "pkt-line.h"
 #include <signal.h>
diff --git a/main.h b/main.h
new file mode 100644
index 0000000..472f134
--- /dev/null
+++ b/main.h
@@ -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;
+
+	/* don't use any library functions. We won't have the headers */
+	while(*p)
+		if(*p++ == '/')
+			git_progname = p;
+
+	return git_main(argc, argv);
+}
+
+#define main(argc, argv) git_main(argc, argv)
+#endif /* __GLIBC__ */

Re: [PATCH] git_progname (was: Re: User-relative paths)

[Petr Baudis]

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 <ae@op5.se> told me that...
> 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"'

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

> diff --git a/cache.h b/cache.h
> index d776016..db5d667 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -45,6 +45,13 @@
>  #endif
>  #endif
>  
> +#if defined(__GLIBC__)
> +extern const char *__progname;
> +#define git_progname __progname
> +#else
> +extern const char *git_progname;
> +#endif
> +
>  /*
>   * Intensive research over the course of many years has shown that
>   * port 9418 is totally unused by anything else. Or

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.

> diff --git a/main.h b/main.h
> new file mode 100644
> index 0000000..472f134
> --- /dev/null
> +++ b/main.h
> @@ -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;
> +
> +	/* don't use any library functions. We won't have the headers */
> +	while(*p)
> +		if(*p++ == '/')
> +			git_progname = p;
> +
> +	return git_main(argc, argv);
> +}
> +
> +#define main(argc, argv) git_main(argc, argv)
> +#endif /* __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().

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] git_progname

[Andreas Ericsson]

Petr Baudis wrote:
> Could you please also trim the mails you are replying to a bit?
> 

Aye.

> Dear diary, on Tue, Oct 25, 2005 at 11:11:54AM CEST, I got a letter
> where Andreas Ericsson <ae@op5.se> told me that...
> 
>>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.

Including main.h is a fairly sane option for common initialization code.

> 
> 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.
> 

True. I'll rework it if someone thinks it's worth it.

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


I had some thoughts along those lines as well, especially for reading 
configuration files.

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

This I don't understand. Do you mean "set the progname to whatever you 
call" as in "set the progname to whatever the program author calls the 
program" or as in "set the progname to whatever it's called as from the 
command-line"?

> 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().
> 

Set the progname to whatever the author calls it then.

If so, I'd put
const char *git_progname = GIT_PROGNAME;
anywhere in the main() file and expanding the %.o: rule to include 
-DGIT_PROGNAME=\"git-$*\"

This would bark if there's a program that doesn't have it that's linked 
against something that uses it, which the setup_progname() approach 
wouldn't do.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git_progname

[Andreas Ericsson]

Andreas Ericsson wrote:
> Petr Baudis wrote:
> 
>>> 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.
> 

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

Silly me. Sorry about that.

I guess some manual editing could be done.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] git_progname

[Petr Baudis]

Dear diary, on Tue, Oct 25, 2005 at 02:53:14PM CEST, I got a letter
where Andreas Ericsson <ae@op5.se> told me that...
> Andreas Ericsson wrote:
> >Petr Baudis wrote:
> >>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. ;)

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] git_progname

[Junio C Hamano]

Petr Baudis <pasky@suse.cz> writes:

> 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. ;)

May I just say "yuck"?

I think what you suggested makes the most sense.

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

Re: [PATCH] git_progname (was: Re: User-relative paths)

[Matthias Urlichs]

Hi, Petr Baudis wrote:

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

Wasn't me, and frankly I doubt it. "old-style" programs just call the
compatibility interface instead, and "new-style" ones call the libraries'
initialization *after* they've parsed any environment vars or flags 
that are necessary.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
While the difficulties and dangers of problems tend to increase at a
geometric rate, the knowledge and manpower qualified to deal with these
problems tend to increase at an arithmetic rate.
					-- Yehezkel Dror

Re: Server side programs

[Linus Torvalds]

On Sun, 23 Oct 2005, Andreas Ericsson wrote:
>
> Are git-receive-pack and git-upload-pack the only two binaries that get called
> directly over a SSH-tunnel?

With the normal pack thing, yes.

They will exec other programs (mainly git-rev-list and 
git-[un]pack-objects), and a client can ask for other programs 
(git-send-pack takes an "--exec=" argument, for example), but those two 
should be sufficient if you have a server-side special "restricted shell" 
that you want to run instead of a real one.

One more issue: you can't create a new archive or delete an old one (or do 
administration like repacking, fsck etc) with those interfaces, so if you 
want these limited users to be able to do that, then you'd need to add a 
few administration commands too.

		Linus