Re: [PATCHv3] daemon: give friendlier error messages to clients

From: Sitaram Chamarty <sitaramc@gmail.com>
Date: Oct 14, 2011, 23:39 UTC
Message-ID: <CAMK1S_g0aKUa=+ndAm7rqeoPAobjVb6oJ1Z4DqSeNrdauXNH3w@mail.gmail.com>
In-Reply-To: <20111014211921.GB16429@sigill.intra.peff.net>

On Sat, Oct 15, 2011 at 2:49 AM, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> When the git-daemon is asked about an inaccessible
> repository, it simply hangs up the connection without saying
> anything further. This makes it hard to distinguish between
> a repository we cannot access (e.g., due to typo), and a
> service or network outage.
>
> Instead, let's print an "ERR" line, which git clients
> understand since v1.6.1 (2008-12-24).
>
> Because there is a risk of leaking information about
> non-exported repositories, by default all errors simply say
> "access denied". Sites which don't have hidden repositories,

I suggest that even the "secure" version of the message say something like "access denied or repository not exported". You're still not leaking anything, but it reduces confusion to the user, who otherwise may not realise it *could be* the latter.

regards

sitaram

Show 118 quoted lines

> or don't care, can pass a flag to turn on more specific
> messages.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Minor tweaks to the documentation and code style to make Jonathan happy.
> :)
>
> Note: I labeled this "v3", as it is the third one posted, but the prior
> ones were not labeled with versions at all.
>
>  Documentation/git-daemon.txt |   10 ++++++++++
>  daemon.c                     |   25 +++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
> index 69a1e4a..31b28fc 100644
> --- a/Documentation/git-daemon.txt
> +++ b/Documentation/git-daemon.txt
> @@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning
>        repository configuration.  By default, all the services
>        are overridable.
>
> +--informative-errors::
> +--no-informative-errors::
> +       When informative errors are turned on, git-daemon will report
> +       more verbose errors to the client, differentiating conditions
> +       like "no such repository" from "repository not exported". This
> +       is more convenient for clients, but may leak information about
> +       the existence of unexported repositories.  When informative
> +       errors are not enabled, all errors report "access denied" to the
> +       client. The default is --no-informative-errors.
> +
>  <directory>::
>        A directory to add to the whitelist of allowed directories. Unless
>        --strict-paths is specified this will also include subdirectories
> diff --git a/daemon.c b/daemon.c
> index 4c8346d..6f111af 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", msg, dir);
> +       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,14 @@ int main(int argc, char **argv)
>                        make_service_overridable(arg + 18, 0);
>                        continue;
>                }
> +               if (!prefixcmp(arg, "--informative-errors")) {
> +                       informative_errors = 1;
> +                       continue;
> +               }
> +               if (!prefixcmp(arg, "--no-informative-errors")) {
> +                       informative_errors = 0;
> +                       continue;
> +               }
>                if (!strcmp(arg, "--")) {
>                        ok_paths = &argv[i+1];
>                        break;
> --
> 1.7.6.4.37.g43b58b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sitaram