threads / patch / 18268

patchIntroduce a filter-path argument to git-daemon, for doing custom path transformations

Subject: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

## tl;dr

14 messages between Mar 11, 2009 and Mar 20, 2009. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Johan Sørensen· Mar 11, 2009, 15:17 UTC · lore

The argument is an executable script that will receive the path to the repos the client wishes to clone as an argument. It is then the responsibility of the script to return a zero-terminated string on its stdout with the real path of the target repository.

This buys us a lot of flexibility when it comes to managing different
repositories, possibly located in many different dirs, but with a uniform
url-structure to the outside world.
---
 Documentation/git-daemon.txt |    7 ++++
 daemon.c                     |   75 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 1 deletions(-)
Show changes to 2 files +81 −1

Documentation/git-daemon.txt, daemon.c

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 36f00ae..1eca344 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
 	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
+	     [--path-filter=executable]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
 	     [--allow-override=service] [--forbid-override=service]
@@ -71,6 +72,12 @@ OPTIONS
 	After interpolation, the path is validated against the directory
 	whitelist.
 
+--path-filter=executable::
+	To support a more flexible directory layout a path filter script 
+	can be used. The executable will receive the requested path from
+	the client as arg0. The executable must return a zero-terminated
+	string on stdout which is the real path 'git-daemon' should serve.
+
 --export-all::
 	Allow pulling from all directories that look like GIT repositories
 	(have the 'objects' and 'refs' subdirectories), even if they
diff --git a/daemon.c b/daemon.c
index d93cf96..b2571df 100644
--- a/daemon.c
+++ b/daemon.c
@@ -22,6 +22,7 @@ static const char daemon_usage[] =
 "           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
+"           [--path-filter=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
@@ -58,6 +59,10 @@ static char *canon_hostname;
 static char *ip_address;
 static char *tcp_port;
 
+/* if defined, the script will be executed with the requested path on stdin
+ * and _must_ return with a successful exitcode and the new path on stdout */
+static char *path_filter_script;
+
 static void logreport(int priority, const char *err, va_list params)
 {
 	if (log_syslog) {
@@ -287,9 +292,62 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static char *run_path_filter_script(char *requested_dir) {
+	pid_t pid;
+	char result[256]; /* arbitary */
+	char *real_path;
+	int pipe_out[2];
+	int exit_code = 1;
+
+	pipe(pipe_out);
+
+	loginfo("Executing path filter script: '%s %s'", path_filter_script, requested_dir);
+
+	switch ((pid = fork())) {
+		case -1:
+			logerror("path filter script fork() failed: %s", strerror(errno));
+			return NULL;
+		case 0:
+		close(pipe_out[0]);
+		dup2(pipe_out[1], 1);
+		close(pipe_out[1]);
+
+		execl(path_filter_script, path_filter_script, requested_dir, NULL);
+
+		/* execl failed if we got here */
+		logerror("path filter script execl() failed: %s", strerror(errno));
+		return NULL;
+	default:
+		close(pipe_out[1]);
+	
+		while(waitpid(pid, &exit_code, 0) < 0) {
+			switch(errno) {
+			case EINTR:
+			continue;
+			default:
+				logerror("path filter script waitpid() fail: %s", strerror(errno));
+				goto waitpid_error;
+			}
+		}
+		if (WIFEXITED(exit_code) && WEXITSTATUS(exit_code) == 0) {
+			read(pipe_out[0], result, sizeof(result) - 1);
+			loginfo("path filter script result: %s", result);
+		}
+		waitpid_error:
+		close(pipe_out[0]);
+	}
+
+	if (result) {
+		real_path = result;
+		return real_path;
+	}
+	return NULL;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
+	char *real_dir;
 	int enabled = service->enabled;
 
 	loginfo("Request %s for '%s'", service->name, dir);
@@ -299,8 +357,19 @@ static int run_service(char *dir, struct daemon_service *service)
 		errno = EACCES;
 		return -1;
 	}
+	loginfo("path_filter_script %s", path_filter_script);
+	if (!path_filter_script) {
+		real_dir = dir;
+	} else {
+		char *tdir;
+		if ((tdir = run_path_filter_script(dir))) {
+			real_dir = tdir;
+		} else {
+			real_dir = dir;
+		}
+	}
 
-	if (!(path = path_ok(dir)))
+	if (!(path = path_ok(real_dir)))
 		return -1;
 
 	/*
@@ -1018,6 +1087,10 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+		if (!prefixcmp(arg, "--path-filter=")) {
+			path_filter_script = arg + 14;
+			continue;
+		}
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
-- 
1.6.1
Johannes Sixt· Mar 11, 2009, 15:58 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Johan Sørensen schrieb:
Show 8 quoted lines
> The argument is an executable script that will receive the path to the repos
> the client wishes to clone as an argument. It is then the responsibility of the
> script to return a zero-terminated string on its stdout with the real path of
> the target repository.
> 
> This buys us a lot of flexibility when it comes to managing different
> repositories, possibly located in many different dirs, but with a uniform
> url-structure to the outside world.

It's the first time that I see a deamon with this feature - except perhaps Apache's ModRewrite. Are you sure you are not working around your problem at the wrong place?

Doesn't --interpolated-path already solve your problem? If not, then you at least you must describe in the documentation the use-cases when --path-filter should be preferred.

Your implementation does not pass the target hostname to the script, but it should; otherwise you lose flexibility (for virtual hosting).

Show 21 quoted lines
> +static char *run_path_filter_script(char *requested_dir) {
> +	pid_t pid;
> +	char result[256]; /* arbitary */
> +	char *real_path;
> +	int pipe_out[2];
> +	int exit_code = 1;
> +
> +	pipe(pipe_out);
> +
> +	loginfo("Executing path filter script: '%s %s'", path_filter_script, requested_dir);
> +
> +	switch ((pid = fork())) {
> +		case -1:
> +			logerror("path filter script fork() failed: %s", strerror(errno));
> +			return NULL;
> +		case 0:
> +		close(pipe_out[0]);
> +		dup2(pipe_out[1], 1);
> +		close(pipe_out[1]);
> +
> +		execl(path_filter_script, path_filter_script, requested_dir, NULL);

Use start_command()/finish_command() instead of rolling your own fork/exec combo.

-- Hannes
Johan Sørensen· Mar 12, 2009, 10:13 UTC · re: Johannes Sixt · lore
The parameter for filter-path is an executable that will receive the service
name, the client hostname and path to the repos the client requests as as
arguments. It is then the responsibility of the script to return a zero
terminated string on its stdout with the real path of the target repository.
---
 Documentation/git-daemon.txt |   13 ++++++++++
 daemon.c                     |   53 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 1 deletions(-)
Show changes to 2 files +65 −1

Documentation/git-daemon.txt, daemon.c

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 36f00ae..efd1687 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
 	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
+	     [--path-filter=executable]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
 	     [--allow-override=service] [--forbid-override=service]
@@ -71,6 +72,18 @@ OPTIONS
 	After interpolation, the path is validated against the directory
 	whitelist.
 
+--path-filter=executable::
+	To support a more flexible directory layout a path filter script 
+	can be used. The executable will receive the service name (upload-pack, 
+	upload-archive or receive-pack), the client hostname and the request git 
+	directory as arguments. The executable must return a zero-terminated string
+	on stdout which is the real path 'git-daemon' should serve. This is useful
+	when --interpolated-path doesn't buy you enough flexibility. You could for
+	instance keep support for old clone urls if you rename your repository, or
+	fetch a custom url-mapping from a third-party repo manager application, or
+	map deeply nested repository directories to a more sensible layout for the 
+	outside world.
+
 --export-all::
 	Allow pulling from all directories that look like GIT repositories
 	(have the 'objects' and 'refs' subdirectories), even if they
diff --git a/daemon.c b/daemon.c
index d93cf96..e6777c6 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pkt-line.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 
 #include <syslog.h>
 
@@ -22,6 +23,7 @@ static const char daemon_usage[] =
 "           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
+"           [--path-filter=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
@@ -58,6 +60,10 @@ static char *canon_hostname;
 static char *ip_address;
 static char *tcp_port;
 
+/* if defined, the script will be executed with the requested path on stdin
+ * and _must_ return with a successful exitcode and the new path on stdout */
+static char *path_filter_script;
+
 static void logreport(int priority, const char *err, va_list params)
 {
 	if (log_syslog) {
@@ -287,6 +293,37 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static char *run_path_filter_script(struct daemon_service *s, char *host, char *dir) {
+	char result[256]; /* arbitary */
+	char *real_path;
+	struct child_process filter_cmd;
+	const char *args[] = { path_filter_script, s->name, host, dir, NULL };
+
+	loginfo("Executing path filter script: '%s %s'", path_filter_script, dir);
+	memset(&filter_cmd, 0, sizeof(filter_cmd));
+	filter_cmd.argv = args;
+	filter_cmd.out = -1;
+	
+	if (start_command(&filter_cmd)) {
+		logerror("path filter: unable to fork path_filter_script");
+		return NULL;
+	}
+	
+	read(filter_cmd.out, result, sizeof(result) - 1);
+	
+	close(filter_cmd.out);
+	if (finish_command(&filter_cmd)) {
+		logerror("path filter died with strange error");
+		return NULL;
+	}
+
+	if (result) {
+		real_path = result;
+		return real_path;
+	}
+	return NULL;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -495,6 +532,7 @@ static void parse_extra_args(char *extra_args, int buflen)
 static int execute(struct sockaddr *addr)
 {
 	static char line[1000];
+	char *path;
 	int pktlen, len, i;
 
 	if (addr) {
@@ -553,11 +591,20 @@ static int execute(struct sockaddr *addr)
 		if (!prefixcmp(line, "git-") &&
 		    !strncmp(s->name, line + 4, namelen) &&
 		    line[namelen + 4] == ' ') {
+			path = line + namelen + 5;
+			if (path_filter_script) {
+				loginfo("path_filter_script %s for path %s", path_filter_script, path);
+				char *tdir;
+				if ((tdir = run_path_filter_script(s, hostname, path))) {
+					path = tdir;
+				}
+			}
+			
 			/*
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			return run_service(path, s);
 		}
 	}
 
@@ -1018,6 +1065,10 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+		if (!prefixcmp(arg, "--path-filter=")) {
+			path_filter_script = arg + 14;
+			continue;
+		}
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
-- 
1.6.1
Johannes Schindelin· Mar 12, 2009, 11:29 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Hi,
Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)
Still with me?  Good.  Nice to meet you.

Just for the record: responding to a patch is my strongest way of saying that I appreciate your work.

On Thu, 12 Mar 2009, Johan Sørensen wrote:
Show 6 quoted lines
> The parameter for filter-path is an executable that will receive the 
> service name, the client hostname and path to the repos the client 
> requests as as arguments. It is then the responsibility of the script to 
> return a zero terminated string on its stdout with the real path of the 
> target repository.
> ---
A sign-off is missing...

More importantly, you might want to point out the security concerns of running a script with the full permissions of git-daemon. (AFAICT from your patch you are not dropping any privileges at any point.)

Which brings me to another idea: we have quite a few places in Git where we use regular expressions. Would they not be enough for your use case?

Show 19 quoted lines
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 36f00ae..efd1687 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -71,6 +72,18 @@ OPTIONS
>  	After interpolation, the path is validated against the directory
>  	whitelist.
>  
> +--path-filter=executable::
> +	To support a more flexible directory layout a path filter script 
> +	can be used. The executable will receive the service name (upload-pack, 
> +	upload-archive or receive-pack), the client hostname and the request git 
> +	directory as arguments. The executable must return a zero-terminated string
> +	on stdout which is the real path 'git-daemon' should serve. This is useful
> +	when --interpolated-path doesn't buy you enough flexibility. You could for
> +	instance keep support for old clone urls if you rename your repository, or
> +	fetch a custom url-mapping from a third-party repo manager application, or
> +	map deeply nested repository directories to a more sensible layout for the 
> +	outside world.
Please keep the lines shorter than 81 characters.
Show 9 quoted lines
> diff --git a/daemon.c b/daemon.c
> index d93cf96..e6777c6 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -287,6 +293,37 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
>  	return 0;
>  }
>  
> +static char *run_path_filter_script(struct daemon_service *s, char *host, char *dir) {

Again, pretty long line. (I will refrain from saying that for every long line, but please cooperate by pretending I did ;-)

But there is more: what about concurrent accesses?
> +	char result[256]; /* arbitary */
Why not PATH_MAX?
Show 15 quoted lines
> +	char *real_path;
> +	struct child_process filter_cmd;
> +	const char *args[] = { path_filter_script, s->name, host, dir, NULL };
> +
> +	loginfo("Executing path filter script: '%s %s'", path_filter_script, dir);
> +	memset(&filter_cmd, 0, sizeof(filter_cmd));
> +	filter_cmd.argv = args;
> +	filter_cmd.out = -1;
> +	
> +	if (start_command(&filter_cmd)) {
> +		logerror("path filter: unable to fork path_filter_script");
> +		return NULL;
> +	}
> +	
> +	read(filter_cmd.out, result, sizeof(result) - 1);
No error checking?

BTW we do have strbuf_read(), which would solve your "static char *" problem nicely.

Show 11 quoted lines
> +	close(filter_cmd.out);
> +	if (finish_command(&filter_cmd)) {
> +		logerror("path filter died with strange error");
> +		return NULL;
> +	}
> +
> +	if (result) {
> +		real_path = result;
> +		return real_path;
> +	}
> +	return NULL;
What would be the difference if you wrote
	return result;
instead?
Show 5 quoted lines
> @@ -495,6 +532,7 @@ static void parse_extra_args(char *extra_args, int buflen)
>  static int execute(struct sockaddr *addr)
>  {
>  	static char line[1000];
> +	char *path;

Is it not rather "const char *"? But that point would be moot should you decide to use strbufs.

Show 8 quoted lines
> @@ -553,11 +591,20 @@ static int execute(struct sockaddr *addr)
>  		if (!prefixcmp(line, "git-") &&
>  		    !strncmp(s->name, line + 4, namelen) &&
>  		    line[namelen + 4] == ' ') {
> +			path = line + namelen + 5;
> +			if (path_filter_script) {
> +				loginfo("path_filter_script %s for path %s", path_filter_script, path);
> +				char *tdir;
Declaration after a call to a function.
> +				if ((tdir = run_path_filter_script(s, hostname, path))) {
> +					path = tdir;
> +				}
Unnecessary curly brackets.

And your code would be even easier to read if your run_path_filter_script() would never return NULL, but the unchanged path instead.

Ciao, Dscho

Johan Sørensen· Mar 12, 2009, 15:48 UTC · re: Johannes Schindelin · lore

The parameter for filter-path is an executable that will receive the service name, the client hostname and path to the repos the client requests as as arguments. It is then the responsibility of the script to return a zero terminated string on its stdout with the real path of the target repository.

Signed-off-by: Johan Sørensen <johan@johansorensen.com>
---
 Documentation/git-daemon.txt |   15 +++++++++++
 daemon.c                     |   54 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 1 deletions(-)
Show changes to 2 files +68 −1

Documentation/git-daemon.txt, daemon.c

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 36f00ae..bf8d31f 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 	     [--strict-paths] [--base-path=path] [--base-path-relaxed]
 	     [--user-path | --user-path=path]
 	     [--interpolated-path=pathtemplate]
+	     [--path-filter=executable]
 	     [--reuseaddr] [--detach] [--pid-file=file]
 	     [--enable=service] [--disable=service]
 	     [--allow-override=service] [--forbid-override=service]
@@ -71,6 +72,20 @@ OPTIONS
 	After interpolation, the path is validated against the directory
 	whitelist.
 
+--path-filter=executable::
+	To support a more flexible directory layout a path filter script
+	can be used. The executable will receive the service name (upload-pack,
+	upload-archive or receive-pack), the client hostname and the request git
+	directory as arguments. The executable must return a zero-terminated
+	string on stdout which is the real path 'git-daemon' should serve. This
+	is useful when --interpolated-path doesn't buy you enough flexibility.
+	You could for instance keep support for old clone urls if you rename your
+	repository, or fetch a custom url-mapping from a third-party repo manager
+	application, or	map deeply nested repository directories to a more
+	sensible layout for the outside world.
+	Please be aware that the executable spawned will have the same privileges
+	as the user you are running the git-daemon under.
+
 --export-all::
 	Allow pulling from all directories that look like GIT repositories
 	(have the 'objects' and 'refs' subdirectories), even if they
diff --git a/daemon.c b/daemon.c
index d93cf96..e865e78 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pkt-line.h"
 #include "exec_cmd.h"
+#include "run-command.h"
 
 #include <syslog.h>
 
@@ -22,6 +23,7 @@ static const char daemon_usage[] =
 "           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
 "           [--interpolated-path=path]\n"
+"           [--path-filter=path]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
@@ -58,6 +60,11 @@ static char *canon_hostname;
 static char *ip_address;
 static char *tcp_port;
 
+/* if defined, the script will be executed with the service name, hostname,
+ * and requested path on stdin and _must_ return with a successful exitcode
+ * and the new path on stdout */
+static char *path_filter_script;
+
 static void logreport(int priority, const char *err, va_list params)
 {
 	if (log_syslog) {
@@ -287,6 +294,42 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static char *run_path_filter_script(struct daemon_service *s, char *host,
+			    char *dir) {
+	struct strbuf result_buf = STRBUF_INIT;
+	struct child_process filter_cmd;
+	const char *args[] = { path_filter_script, s->name, host, dir, NULL };
+
+	loginfo("Executing path filter script: '%s %s %s %s'",
+					path_filter_script, s->name, host, dir);
+	memset(&filter_cmd, 0, sizeof(filter_cmd));
+	filter_cmd.argv = args;
+	filter_cmd.out = -1;
+
+	if (start_command(&filter_cmd)) {
+		logerror("path filter: unable to fork path_filter_script");
+		return dir;
+	}
+
+	if (strbuf_read(&result_buf, filter_cmd.out, PATH_MAX) < 0) {
+		strbuf_release(&result_buf);
+		close(filter_cmd.out);
+		logerror("path filter: script read returned %s", strerror(errno));
+		return dir;
+	}
+
+	close(filter_cmd.out);
+	if (finish_command(&filter_cmd)) {
+		logerror("path filter script died with strange error");
+		return dir;
+	}
+
+	if (result_buf.len > 0)
+		dir = strbuf_detach(&result_buf, NULL);
+
+	return dir;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;
@@ -557,7 +600,12 @@ static int execute(struct sockaddr *addr)
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			if (path_filter_script) {
+				return run_service(run_path_filter_script(s, hostname,
+				                   line + namelen + 5), s);
+			} else {
+				return run_service(line + namelen + 5, s);
+			}
 		}
 	}
 
@@ -1018,6 +1066,10 @@ int main(int argc, char **argv)
 			pid_file = arg + 11;
 			continue;
 		}
+		if (!prefixcmp(arg, "--path-filter=")) {
+			path_filter_script = arg + 14;
+			continue;
+		}
 		if (!strcmp(arg, "--detach")) {
 			detach = 1;
 			log_syslog = 1;
-- 
1.6.1
Johannes Schindelin· Mar 12, 2009, 16:50 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Hi,
On Thu, 12 Mar 2009, Johan Sørensen wrote:
Show 6 quoted lines
> The parameter for filter-path is an executable that will receive the service
> name, the client hostname and path to the repos the client requests as as
> arguments. It is then the responsibility of the script to return a zero
> terminated string on its stdout with the real path of the target repository.
> 
> Signed-off-by: Johan Sørensen <johan@johansorensen.com>

I see that you addressed some, but not all of my concerns. Do you think that I am wrong? Then please, by all means, increase my knowledge.

Ciao, Dscho

Johan Sørensen· Mar 12, 2009, 19:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

On Thu, Mar 12, 2009 at 12:29 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

(all my comments below refer to my latest patch)
> More importantly, you might want to point out the security concerns of
> running a script with the full permissions of git-daemon.  (AFAICT from
> your patch you are not dropping any privileges at any point.)

Do you really think this is needed? It doesn't seem like running the hook scripts does anything more than trusting the script author and permissions of the hook scripts (?). I see the path-filter script exactly the same way, with the exception of having to double-check the user supplied path the script receives.

> Which brings me to another idea: we have quite a few places in Git where
> we use regular expressions.  Would they not be enough for your use case?

Hmm, please do elaborate on your idea. If you mean being able to supply a bunch of regexp mappings when starting the daemon then it wouldn't cut it for me; I'm in need of something more dynamic.

Show 5 quoted lines
>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> index 36f00ae..efd1687 100644
>> --- a/Documentation/git-daemon.txt
>> +++ b/Documentation/git-daemon.txt
>> @@ -71,6 +72,18 @@ OPTIONS
[snip]
> Please keep the lines shorter than 81 characters.
I believe the longest line I've added in the docs is 77.
> But there is more: what about concurrent accesses?

The external path-filter script is run from the execute(), which is forked+exec'ed for each incoming connection to the daemon, so that would mean a concurrency of one in that child-process, unless I've missed something in the code path?

Show 6 quoted lines
>> +     read(filter_cmd.out, result, sizeof(result) - 1);
>
> No error checking?
>
> BTW we do have strbuf_read(), which would solve your "static char *"
> problem nicely.

I'm using strbuf_read() now, but this being my very first git patch, I may have misunderstood the api slightly?

> And your code would be even easier to read if your
> run_path_filter_script() would never return NULL, but the unchanged path
> instead.
Done.

Thanks for giving my patch the run-through. I'm still curious to hear what people think about the idea in general though!

>
> Ciao,
> Dscho
>

Cheers, JS

Junio C Hamano· Mar 14, 2009, 06:58 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Johan Sørensen <johan@johansorensen.com> writes:
Show 9 quoted lines
>> More importantly, you might want to point out the security concerns of
>> running a script with the full permissions of git-daemon.  (AFAICT from
>> your patch you are not dropping any privileges at any point.)
>
> Do you really think this is needed? It doesn't seem like running the
> hook scripts does anything more than trusting the script author and
> permissions of the hook scripts (?). I see the path-filter script
> exactly the same way, with the exception of having to double-check the
> user supplied path the script receives.

If I am not misreading the patch (I only skimmed it), the script is what is given to the git-daemon process from its command line, so it is under total control of the site owner. It is much much much less problematic than the security worry of allowing random hook scripts to be installed in the repositories hosted at a hosting site. I think Dscho is being a bit too paranoid in this particular case.

However, being paranoid is a good thing when we talk about instructions we give to the end users. The site owner who uses this facility needs to be aware that the script is run as the same user that runs git-daemon, and that more than one instances of the script can be run at the same time. The script writer needs to be careful about using the same scratchpad location for the temporary files the script uses and not letting multiple instances of scripts stomping on each other's toes. These things need to be documented.

Do you run git-daemon from inetd, or standalone, by the way? I am wondering how well it would scale if you spawn an external "filter path" script (by the way, "filter path" sounds as if it checks and conditionally denies access to, or something like that, which is not what you are using it for. It is more about rewriting paths, a la mod_rewrite, and I think the option is misnamed) every time you get a request.

Johan Sørensen· Mar 14, 2009, 14:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

On Sat, Mar 14, 2009 at 7:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> However, being paranoid is a good thing when we talk about instructions we
> give to the end users.  The site owner who uses this facility needs to be
> aware that the script is run as the same user that runs git-daemon, and
> that more than one instances of the script can be run at the same time.
> The script writer needs to be careful about using the same scratchpad
> location for the temporary files the script uses and not letting multiple
> instances of scripts stomping on each other's toes.  These things need to
> be documented.
Will expand the docs further.
> Do you run git-daemon from inetd, or standalone, by the way?
Standalone.
> I am wondering how well it would scale if you spawn an external "filter path"
> script every time you get a request.
A quick test of 250 consecutive requests with ls-remote to localhost
(all without the --verbose flag), slowest run:
- Baseline (no --filter-path agument): 3.39s
$ cat filter.c
#import "stdio.h"
int main (int argc, char const *argv[]) {
	printf("%s", "/existing.git\0");
	return 0;
}
- 3.84s
$ cat filter.rb
#!/usr/bin/ruby
print "/existing.git\0"
- 4.76s

So, obviously highly dependent on how long it takes the script to launch and how much work it does. And yes, neither of the above really does anything :) nor takes any increased cpu load into account

Another approach is to keep the external script running and feed it on stdin, but that would involve a bit more micro-management of the external process. I will revisit that idea if I find out that's needed.

> (by the way, "filter path" sounds as if it checks and conditionally
> denies access to, or something like that, which is not what you are using
> it for.  It is more about rewriting paths, a la mod_rewrite, and I think
> the option is misnamed)
Maybe --rewrite-script or --rewrite-command  instead?

Cheers, JS

Junio C Hamano· Mar 14, 2009, 18:23 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Johan Sørensen <johan@johansorensen.com> writes:
Show 32 quoted lines
>> Do you run git-daemon from inetd, or standalone, by the way?
>
> Standalone.
>
>> I am wondering how well it would scale if you spawn an external "filter path"
>> script every time you get a request.
>
> A quick test of 250 consecutive requests with ls-remote to localhost
> (all without the --verbose flag), slowest run:
> - Baseline (no --filter-path agument): 3.39s
>
> $ cat filter.c
> #import "stdio.h"
> int main (int argc, char const *argv[]) {
> 	printf("%s", "/existing.git\0");
> 	return 0;
> }
> - 3.84s
>
> $ cat filter.rb
> #!/usr/bin/ruby
> print "/existing.git\0"
> - 4.76s
>
> So, obviously highly dependent on how long it takes the script to
> launch and how much work it does. And yes, neither of the above really
> does anything :) nor takes any increased cpu load into account
>
> Another approach is to keep the external script running and feed it on
> stdin, but that would involve a bit more micro-management of the
> external process. I will revisit that idea if I find out that's
> needed.

I actually was hoping (especially we have Dscho on Cc: list) that somebody like you would start suggesting a "plug in" approach to load .so files, which would lead to a easy-to-port dso support with the help from msysgit folks we can use later in other parts of the system (e.g. customizable filters used for diff textconv, clean/smudge, etc.)

Show 6 quoted lines
>> (by the way, "filter path" sounds as if it checks and conditionally
>> denies access to, or something like that, which is not what you are using
>> it for.  It is more about rewriting paths, a la mod_rewrite, and I think
>> the option is misnamed)
>
> Maybe --rewrite-script or --rewrite-command  instead?
Perhaps.
Johannes Schindelin· Mar 19, 2009, 00:15 UTC · re: Junio C Hamano · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Hi,
On Sat, 14 Mar 2009, Junio C Hamano wrote:
Show 40 quoted lines
> Johan Sørensen <johan@johansorensen.com> writes:
> 
> >> Do you run git-daemon from inetd, or standalone, by the way?
> >
> > Standalone.
> >
> >> I am wondering how well it would scale if you spawn an external 
> >>"filter path" script every time you get a request.
> >
> > A quick test of 250 consecutive requests with ls-remote to localhost
> > (all without the --verbose flag), slowest run:
> > - Baseline (no --filter-path agument): 3.39s
> >
> > $ cat filter.c
> > #import "stdio.h"
> > int main (int argc, char const *argv[]) {
> > 	printf("%s", "/existing.git\0");
> > 	return 0;
> > }
> > - 3.84s
> >
> > $ cat filter.rb
> > #!/usr/bin/ruby
> > print "/existing.git\0"
> > - 4.76s
> >
> > So, obviously highly dependent on how long it takes the script to 
> > launch and how much work it does. And yes, neither of the above really 
> > does anything :) nor takes any increased cpu load into account
> >
> > Another approach is to keep the external script running and feed it on 
> > stdin, but that would involve a bit more micro-management of the 
> > external process. I will revisit that idea if I find out that's 
> > needed.
> 
> I actually was hoping (especially we have Dscho on Cc: list) that somebody
> like you would start suggesting a "plug in" approach to load .so files,
> which would lead to a easy-to-port dso support with the help from msysgit
> folks we can use later in other parts of the system (e.g. customizable
> filters used for diff textconv, clean/smudge, etc.)

I do not like that at all. Dynamic libraries -- especially on Windows -- are a major hassle.

However, I cannot think of anything Johan might want to do that would not be possible using a bunch of regular expressions together with substitions.

FWIW I have experimental code in my personal tree that sports strbuf_regsub(), a function to replace matches of a regular expression (possibly with groups) by a given string (which may contain \0 .. \9, being replaced with the respective group's contents).

Ciao, Dscho

Johan Sørensen· Mar 19, 2009, 13:02 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

2009/3/19 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 12 quoted lines
>> I actually was hoping (especially we have Dscho on Cc: list) that somebody
>> like you would start suggesting a "plug in" approach to load .so files,
>> which would lead to a easy-to-port dso support with the help from msysgit
>> folks we can use later in other parts of the system (e.g. customizable
>> filters used for diff textconv, clean/smudge, etc.)
>
> I do not like that at all.  Dynamic libraries -- especially on Windows --
> are a major hassle.
>
> However, I cannot think of anything Johan might want to do that would not
> be possible using a bunch of regular expressions together with
> substitions.

Let me reiterate my use-case then: I need to dynamically substitute one path with another. Perhaps "map" paints a better picture than "substitute" here. Please refer to my second mail in this thread for more details.

The only way I can see regexps work, is that if they're read, on a per-request basis (reloading git-daemon every time they change is just silly), from somewhere outside the git-daemon. Then, you might as well take the full-on approach this patch provides.

Cheers, JS

Show 9 quoted lines
>
> FWIW I have experimental code in my personal tree that sports
> strbuf_regsub(), a function to replace matches of a regular expression
> (possibly with groups) by a given string (which may contain \0 .. \9,
> being replaced with the respective group's contents).
>
> Ciao,
> Dscho
>
Johannes Schindelin· Mar 20, 2009, 22:27 UTC · re: Johan Sørensen · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

Hi,
On Thu, 19 Mar 2009, Johan Sørensen wrote:
Show 29 quoted lines
> 2009/3/19 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> >> I actually was hoping (especially we have Dscho on Cc: list) that 
> >> somebody like you would start suggesting a "plug in" approach to load 
> >> .so files, which would lead to a easy-to-port dso support with the 
> >> help from msysgit folks we can use later in other parts of the system 
> >> (e.g. customizable filters used for diff textconv, clean/smudge, 
> >> etc.)
> >
> > I do not like that at all.  Dynamic libraries -- especially on Windows 
> > -- are a major hassle.
> >
> > However, I cannot think of anything Johan might want to do that would 
> > not be possible using a bunch of regular expressions together with 
> > substitions.
> 
> Let me reiterate my use-case then: I need to dynamically substitute one 
> path with another. Perhaps "map" paints a better picture than 
> "substitute" here. Please refer to my second mail in this thread for 
> more details.
> 
> The only way I can see regexps work, is that if they're read, on a 
> per-request basis (reloading git-daemon every time they change is just 
> silly), from somewhere outside the git-daemon. Then, you might as well 
> take the full-on approach this patch provides.
>
> > FWIW I have experimental code in my personal tree that sports 
> > strbuf_regsub(), a function to replace matches of a regular expression 
> > (possibly with groups) by a given string (which may contain \0 .. \9, 
> > being replaced with the respective group's contents).
Do not get me wrong, I can see your use case.

But I have been cautioning against other possibly regrettably things, and it gave me _no_ pleasure at all to be proven correct in hindsight.

I'd rather be called grumpy old Git, be ridiculed and insulted, but at the same time have precautions in git.git that prevent having to admit mournfully that some change was not so brilliant after all.

So if some rules consisting of regular expressions with appropriate substitutions, even if they will have to be updated from time to time, solve your case, I'd rather have that than allow a server to run external programs that are not exactly well audited against all kinds of attacks.

Ciao, Dscho

Johan Sørensen· Mar 12, 2009, 10:26 UTC · re: Johannes Sixt · lore

Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing custom path transformations

On Wed, Mar 11, 2009 at 4:58 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 12 quoted lines
> Johan Sørensen schrieb:
>> This buys us a lot of flexibility when it comes to managing different
>> repositories, possibly located in many different dirs, but with a uniform
>> url-structure to the outside world.
>
> It's the first time that I see a deamon with this feature - except perhaps
> Apache's ModRewrite. Are you sure you are not working around your problem
> at the wrong place?
>
> Doesn't --interpolated-path already solve your problem? If not, then you
> at least you must describe in the documentation the use-cases when
> --path-filter should be preferred.

Maybe I am barking up the wrong tree, but here's my real-world use case: I'm currently working on some bigger changes for gitorious.org, where the repository url-structure could potentially change over time, as a consequence of various features. Using the path-filter script I can keep the old urls around and still working, and I can map any url to a on-disk uniquely hashed path, so I don't have to move the files around, maintain symlinks and so forth for information the gitorious application already has nicely structured and easy to lookup.

I know these may be highly specialized needs, but so is interpolated-path for the common user. I think this patch could be useful for anyone else wanting to set up a flexible repo hosting system. I think the url-structure is a major part of the UI for any app exposing them, even for a git-daemon, so the mod_rewrite comparison isn't too far fetched in my opinion...

> Your implementation does not pass the target hostname to the script, but
> it should; otherwise you lose flexibility (for virtual hosting).

Good point. I've added the hostname as well as the service name as arguments for the script.

>> +     switch ((pid = fork())) {
[snip]
>
> Use start_command()/finish_command() instead of rolling your own fork/exec
> combo.
Ah nice! I'm sending an updated patch.
>
> -- Hannes
>

Cheers, JS

← back to recent threads