Re: [PATCH] daemon: print "access denied" if a service does not work

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Note that if a service fails, then "access denied" is printed too.

After run_service() returns with a failure, it is simply irresponsible to
append an "ERR" packet to the output channel without knowing what the
state of that channel is (e.g. it might be that the service wrote only
half a pkt_line it wanted to write, and you may be appending to it).

I think the earlier patch from Peff makes the division of responsibility
clearer. If the daemon's dispatch code notices it does not want to run it,
it is the daemon's job to report it. Otherwise the service can and should
report what it does, and after it starts running, the channel belongs to
the service.

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 OK let's try again. I don't send ERR when faults happen in
 service->fn() (eventually run_service_command) because

  - if it's start_command(), it's likely due to service configuration
    fault (wrong --exec-path..)

  - if it's finish_command(), the service may have run and sent
    something back to users. We may break the protocol by sending ERR

 daemon.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..f0cae24 100644
--- a/daemon.c
+++ b/daemon.c

@@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		goto failed;
 
 	/*
 	 * Security on the cheap.

@@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	if (service->overridable) {

@@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		goto failed;
 	}
 
 	/*

@@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)
 	signal(SIGTERM, SIG_IGN);
 
 	return service->fn();
+
+failed:
+	packet_write(1, "ERR %s: access denied", dir);
+	return -1;
 }
 
 static void copy_to_log(int fd)

-- 
1.7.3.1.256.g2539c.dirty

On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:

---
 daemon.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index 4c8346d..ec88fd0 100644
--- a/daemon.c
+++ b/daemon.c

@@ -20,6 +20,7 @@
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
+static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"

@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+static int daemon_error(const char *dir, const char *msg)
+{
+	if (!informative_errors)
+		msg = "access denied";
+	packet_write(1, "ERR %s: %s", dir, msg);
+	return -1;
+}
+
 static int run_service(char *dir, struct daemon_service *service)
 {
 	const char *path;

@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	if (!(path = path_ok(dir)))
-		return -1;
+		return daemon_error(dir, "no such repository");
 
 	/*
 	 * Security on the cheap.

@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "repository not exported");
 	}
 
 	if (service->overridable) {

@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return -1;
+		return daemon_error(dir, "service not enabled");
 	}
 
 	/*

@@ -1167,6 +1176,10 @@ int main(int argc, char **argv)
 			make_service_overridable(arg + 18, 0);
 			continue;
 		}
+		if (!prefixcmp(arg, "--informative-errors")) {
+			informative_errors = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;

-- 
1.7.7.rc2.21.gb9948

(+cc: Andreas[*])
Jeff King wrote:

On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

quoted

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:

FWIW the more verbose version you suggest also sounds fine to me.  A
person trying to find the names of local users by checking for
repositories with names like "/home/user" would always receive the
error "no such repository", whether that user exists or not and
whether the actual error encountered was ENOENT, EACCES, lack of git
metadata, or the path running afoul of a whitelist or blacklist.

Either Duy's patch or this patch sounds very good to me.  Thanks to
both of you for working on it.

[*] context:
http://thread.gmane.org/gmane.comp.version-control.git/182529/focus=183409

On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:

On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

quoted

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:

How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..

I don't know. I'm not a real server admin so maybe I'm just too
paranoid. Any admins care to speak up?

On the other hand, grouping all messages at one place may be easier to
audit, even if we don't allow customization.

Anyway, two cents on top of your patch..

-- 8< --

diff --git a/daemon.c b/daemon.c
index ec88fd0..a846ef1 100644
--- a/daemon.c
+++ b/daemon.c

@@ -17,10 +17,25 @@
 #define initgroups(x, y) (0) /* nothing */
 #endif
 
+/* Must match messages[] order below */
+#define MSG_SERVICE_NOT_ENABLED     0
+#define MSG_NO_SUCH_REPOSITORY      1
+#define MSG_REPOSITORY_NOT_EXPORTED 2
+
+static struct daemon_message
+{
+	const char *message;
+	const char *config;
+	int enabled;
+} messages[] = {
+	{ "service not enabled", "message.serviceNotEnabled" },
+	{ "no such repository", "message.noSuchRepository" },
+	{ "repository not exported", "message.repositoryNotExported" },
+};
+
 static int log_syslog;
 static int verbose;
 static int reuseaddr;
-static int informative_errors;
 
 static const char daemon_usage[] =
 "git daemon [--verbose] [--syslog] [--export-all]\n"

@@ -238,20 +253,31 @@ static int service_enabled;
 
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
+	int i;
+
 	if (!prefixcmp(var, "daemon.") &&
 	    !strcmp(var + 7, service_looking_at->config_name)) {
 		service_enabled = git_config_bool(var, value);
 		return 0;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(messages); i++)
+		if (!strcmp(var, messages[i].config)) {
+			messages[i].enabled = git_config_bool(var, value);
+			return 0;
+		}
+
 	/* we are not interested in parsing any other configuration here */
 	return 0;
 }
 
-static int daemon_error(const char *dir, const char *msg)
+static int daemon_error(const char *dir, int msg_id)
 {
-	if (!informative_errors)
+	const char *msg;
+	if (!messages[msg_id].enabled)
 		msg = "access denied";
+	else
+		msg = messages[msg_id].message;
 	packet_write(1, "ERR %s: %s", dir, msg);
 	return -1;
 }

@@ -266,11 +292,11 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
 		errno = EACCES;
-		return daemon_error(dir, "service not enabled");
+		return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
 	}
 
 	if (!(path = path_ok(dir)))
-		return daemon_error(dir, "no such repository");
+		return daemon_error(dir, MSG_NO_SUCH_REPOSITORY);
 
 	/*
 	 * Security on the cheap.

@@ -286,7 +312,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!export_all_trees && access("git-daemon-export-ok", F_OK)) {
 		logerror("'%s': repository not exported.", path);
 		errno = EACCES;
-		return daemon_error(dir, "repository not exported");
+		return daemon_error(dir, MSG_REPOSITORY_NOT_EXPORTED);
 	}
 
 	if (service->overridable) {

@@ -300,7 +326,7 @@ static int run_service(char *dir, struct daemon_service *service)
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
 		errno = EACCES;
-		return daemon_error(dir, "service not enabled");
+		return daemon_error(dir, MSG_SERVICE_NOT_ENABLED);
 	}
 
 	/*

@@ -1177,7 +1203,9 @@ int main(int argc, char **argv)
 			continue;
 		}
 		if (!prefixcmp(arg, "--informative-errors")) {
-			informative_errors = 1;
+			int i;
+			for (i = 0; i < ARRAY_SIZE(messages); i++)
+				messages[i].enabled = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--")) {
-- 8< --

Nguyen Thai Ngoc Duy wrote:

How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..

[...]

+	{ "service not enabled", "message.serviceNotEnabled" },
+	{ "no such repository", "message.noSuchRepository" },
+	{ "repository not exported", "message.repositoryNotExported" },

I administer a private server that is only accessible as "localhost".
:)  This much customization would leave me confused about what the
right choices are and what the choices mean (even if I were to make
the server public and start having security worries).

What is the intended use --- translation?  The idealist in me thinks
that should be taken care of on the client side, if at all.  (This
way, we would not be preventing especially friendly clients from
offering pertinent detailed advice for each error condition.
Alternatively, maybe some day the protocol will want to provide a way
for clients to indicate a preferred language and message verbosity.)

On Thu, Oct 13, 2011 at 4:59 PM, Jonathan Nieder [off-list ref] wrote:

Nguyen Thai Ngoc Duy wrote:

quoted

How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..

[...]

quoted

+     { "service not enabled", "message.serviceNotEnabled" },
+     { "no such repository", "message.noSuchRepository" },
+     { "repository not exported", "message.repositoryNotExported" },

I administer a private server that is only accessible as "localhost".
:)  This much customization would leave me confused about what the
right choices are and what the choices mean (even if I were to make
the server public and start having security worries).

--informative-errors is your friend. All errors are enabled.

What is the intended use --- translation?  The idealist in me thinks
that should be taken care of on the client side, if at all.  (This
way, we would not be preventing especially friendly clients from
offering pertinent detailed advice for each error condition.
Alternatively, maybe some day the protocol will want to provide a way
for clients to indicate a preferred language and message verbosity.)

Translation could be fun to do, but it's more about how much admins
want to reveal. For example, I may only want to show "service not
enabled" and "no such repository", not the last one, which simply
becomes "access denied".

Again I'm not real admin and this may be just bogus.
-- 
Duy

On Thu, Oct 13, 2011 at 5:56 PM, Nguyen Thai Ngoc Duy [off-list ref] wrote:

Translation could be fun to do

By translation, I don't mean inter-language translation. More like
personification. Instead of "service not enabled" you may want
"service is off, you want to attack me or what?"
-- 
Duy

On Thu, Oct 13, 2011 at 03:45:44PM +1100, Nguyen Thai Ngoc Duy wrote:

On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:

quoted

On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:

quoted

The message is chosen to avoid leaking information, yet let users know
that they are deliberately not allowed to use the service, not a fault
in service configuration or the service itself.

I do think this is an improvement, but I wonder if the verbosity should
be configurable. Then open sites like kernel.org could be friendlier to
their users. Something like this instead:

How about allow users to select which messages they want to print? We
can even go further, allowing users to specify the messages themselves..

I thought about that, but it just seemed like it was making things way
more complex than it needed to be. GitHub does do this kind of
customization, but we also have a custom layer that intercepts git://
connections, anyway, so we added the relevant code there.

I don't know if medium-sized sites (i.e., ones that aren't so big they
are running custom proxies on the frontend) would care about adding
custom messages here or not.

I don't know. I'm not a real server admin so maybe I'm just too
paranoid. Any admins care to speak up?

I doubt anybody would care that much about turning individual messages
on and off. I think the real value is in being able to say "don't push
by git://. The right way to push to this site is...".

But your patch kind of falls short of what people would want to do for
two reasons:

  1. The message isn't dynamic at all. So I can't say:

        You tried to push to git://host.tld/foo.git. The right way to do
        that is:

          git push https://host.tld/foo.git

     That's what the GitHub message does if you try to push over git://;
     it gives you a new remote name that will actually work, customized
     to the repo you wanted to push to.

  2. Tweaking just the message for anything but "service not enabled"
     isn't all that useful. What do you say about "no such repository"
     in a simple message, even with placeholders?

     If you _really_ want to get fancy, a server could do a fuzzy
     search on the available repos and say "did you mean...?".
     But now we are talking about hooking arbitrary code into the
     message.

So if we want to do anything, I would think it would be a hook. Except
that we may or may not have a repo, so it would not be a hook in
$GIT_DIR/hooks, but rather some script to be run passed on the command
line, like:

  git daemon --informative-errors=/path/to/hook

-Peff