[PATCH] daemon: report inaccessible repositories to user

[Jonathan Nieder <jrnieder@gmail.com>]

This is a follow-up to v1.6.1-rc1~101 (connect.c: add a way for
git-daemon to pass an error back to client, 2008-11-01).  Although
not all clients support that protocol extension yet, it should be safe
to start using it anyway, as explained below.

This patch teaches ‘git daemon’ to let the client know when the
request to access the repository was denied.  Instead of just hanging
up, now the server lets the client know that access was denied, with a
carefully worded error that echoes the request:

  fatal: remote error: /foo/example: unreadable or fetching not allowed.
  fatal: remote error: /foo/example: unreadable or pushing not allowed.
  fatal: remote error: /foo/example: unreadable or snapshotting not allowed.

The failure could be due to one of a few causes.  The message does not
distinguish them:

 - chdir() failure
 - the protocol was disabled
 - not a git repository
 - not marked for export

Non-admin clients have no reason to care --- all of these situations
represent the same “not a public repository” condition.  Server
admins, on the other hand, would care a great deal to know that the
daemon does not reveal information about the machine’s configuration
(such as what directories exist) aside from what requests it is
configured to honor.

The corresponding safety for protocol helpers used over ssh has been
in place since v0.99.9k^2~54 (Server-side support for user-relative
paths, 2005-11-17).

The error message used does _not_ match the output from backends
(which is sent to stderr rather than to the client, anyway) when
enter_repo() fails.  That is fine since ‘git daemon’ already checks
that the repository exists by calling enter_repo() itself.

Pre-1.6.1 git versions will treat the error request as a breach of
protocol.  The result is an acceptable if somewhat funny message.

  fatal: protocol error: expected sha/ref, got 'ERR /foo/example:
  unreadable or fetching not allowed.'

Thanks to Dscho, Ilari, and Andreas for help.  Thanks especially to
Ilari for explaining how this should work.

Cc: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Cc: Andreas Ericsson <ae@op5.se>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Ilari Liusvaara wrote:

> There are few subcases of daemon-level errors:
[...]
> So, pretty much the only daemon-level errors with feedback required
> would be one for invalid repository and disabled service. How about:
> 
> "foo/example: unreadable or anonymous fetching not allowed."
> "foo/example: unreadable or anonymous pushing not allowed."
> "foo/example: unreadable or anonymous snapshotting not allowed."
> "fooserv: requested service unknown."
> 
> And all of these can be sent over ERR. I don't see need for using
> sidebands.

I omitted the qualifier “anonymous” in case we want to reuse these
messages for authenticated situations.  I haven’t thought about it
deeply, though.

I would also like to write tests.  To begin with, it might be easiest
to test in inetd mode to avoid allocating a port for it.

Anyway, that shouldn’t hold up giving you a chance to nak the patch. :)

Thanks for the pointers.  ERR packets do seem like the right way to
do this.

Jonathan

 daemon.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/daemon.c b/daemon.c
index a90ab10..732a339 100644
--- a/daemon.c
+++ b/daemon.c
@@ -222,6 +222,7 @@ static char *path_ok(char *directory)
 typedef int (*daemon_service_fn)(void);
 struct daemon_service {
 	const char *name;
+	const char *description;
 	const char *config_name;
 	daemon_service_fn fn;
 	int enabled;
@@ -231,6 +232,12 @@ struct daemon_service {
 static struct daemon_service *service_looking_at;
 static int service_enabled;
 
+static void report_inaccessible(char *dir, struct daemon_service *service)
+{
+	packet_write(1, "ERR %s: unreadable or %s not allowed.\n",
+	             dir, service->description);
+}
+
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "daemon.") &&
@@ -252,12 +259,15 @@ static int run_service(char *dir, struct daemon_service *service)
 
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}
 
-	if (!(path = path_ok(dir)))
+	if (!(path = path_ok(dir))) {
+		report_inaccessible(dir, service);
 		return -1;
+	}
 
 	/*
 	 * Security on the cheap.
@@ -272,6 +282,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);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}
@@ -286,6 +297,7 @@ static int run_service(char *dir, struct daemon_service *service)
 	if (!enabled) {
 		logerror("'%s': service not enabled for '%s'",
 			 service->name, path);
+		report_inaccessible(dir, service);
 		errno = EACCES;
 		return -1;
 	}
@@ -362,9 +374,9 @@ static int receive_pack(void)
 }
 
 static struct daemon_service daemon_service[] = {
-	{ "upload-archive", "uploadarch", upload_archive, 0, 1 },
-	{ "upload-pack", "uploadpack", upload_pack, 1, 1 },
-	{ "receive-pack", "receivepack", receive_pack, 0, 1 },
+	{ "upload-archive", "snapshotting", "uploadarch", upload_archive, 0, 1 },
+	{ "upload-pack", "fetching", "uploadpack", upload_pack, 1, 1 },
+	{ "receive-pack", "pushing", "receivepack", receive_pack, 0, 1 },
 };
 
 static void enable_service(const char *name, int ena)
-- 
1.7.1.rc2.8.ga54f9