[PATCH] daemon: report inaccessible repositories to user
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 22, 2010, 22:21 UTC
- Message-ID
- <20100422222153.GA12000@progeny.tock>
- In-Reply-To
- <20100422124453.GA30328@LK-Perkele-V2.elisa-laajakaista.fi>
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:
[...]
Show 10 quoted lines
> 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