Re: [PATCH] daemon: return "access denied" if a service is not allowed

[Nguyen Thai Ngoc Duy <pclouds@gmail.com>]

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