Useless error message?

Useless error message?

[Aghiles]

"fatal: The remote end hung up unexpectedly"

Is that really meaningful ? Or maybe it is a configuration problem
on my side ?

  -- aghiles

Re: Useless error message?

[Kim Ebert]

I find that it usually means I didn't set up git-daemon-export-ok. Of 
course, that has usually been my experience.

Aghiles wrote:
> "fatal: The remote end hung up unexpectedly"
>
> Is that really meaningful ? Or maybe it is a configuration problem
> on my side ?
>
>   -- aghiles
> --
> 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
>
>   

Re: Useless error message?

[Jonathan Nieder]

Aghiles wrote:

> "fatal: The remote end hung up unexpectedly"
> 
> Is that really meaningful ? Or maybe it is a configuration problem
> on my side ?

Please, fix it. :)

The problem is this: as far as I can tell, the git protocols are
designed around the success case.  Sometimes if there is an error or
other interesting event, the servers are kind enough to notify the
user “on the side”.  But in the end, all too often, they do not bother
to inform the client _program_ that a fatal error occured.

We can’t just throw away this hang-up message because sometimes when
the remote host hangs up it really was unexpected.

So the trick is to make it expected more often.  See the side-band-64k
capability in Documentation/technical/protocol-capabilities.txt: the
goal is to have fatal error messages for as many failure modes as
possible.

Examples (I could be missing nuances; I am just trying to convey
the idea):

upload-archive:
 - a pipe(), write(), or poll() failure when communicating over
   the wire or between local processes will result in an unexpected
   hangup.  I have not checked, but I suspect a SIGPIPE can kill
   upload-archive, too.
 - On the bright side, all other error conditions are properly
   handled.  The code for this is very nice and worth imitating.

upload-pack:
 - a missing or shallow repo, HEAD or some other ref pointing to
   a nonexistent object, early protocol error, or failure to start
   rev-list or pack-objects will result in an unexpected hangup.
 - errors from rev-list, pack-objects, or the transition of the
   generated pack are correctly handled.

receive-pack:
 - all errors result in unexpected hup as far as I can tell.

daemon:
 - hangs up without an explanation (except to syslog) for invalid
   or disabled repositories
 - if the underlying service hangs up, hangs up.  If the underlying
   service writes a message to stderr, writes that message to
   syslog.  Surely the client is not interested...

If any other information would help, please let me know.
Jonathan

Re: Useless error message?

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> The problem is this: as far as I can tell, the git protocols are
> designed around the success case.  Sometimes if there is an error or
> other interesting event, the servers are kind enough to notify the
> user “on the side”.  But in the end, all too often, they do not bother
> to inform the client _program_ that a fatal error occured.

The true story is a bit different.

To avoid information leak to git-daemon clients, we deliberately choose
not to give detailed error messages, so that you cannot tell if an error
means a user "u" does not exist or "u" does but ~u/repo.git repository
does not exist.

> So the trick is to make it expected more often.  See the side-band-64k
> capability in Documentation/technical/protocol-capabilities.txt: the
> goal is to have fatal error messages for as many failure modes as
> possible.

For authenticated users (read: services that typically are behind auth) it
would be a good thing, but "as many as possible" you shouldn't be followed
blindly.

Re: Useless error message?

[Jonathan Nieder]

Junio C Hamano wrote:

> The true story is a bit different.
> 
> To avoid information leak to git-daemon clients, we deliberately choose
> not to give detailed error messages, so that you cannot tell if an error
> means a user "u" does not exist or "u" does but ~u/repo.git repository
> does not exist.

Thanks for the clarification.  As I see it, these are two different
classes of problem:

1. The git daemon is very quiet, usually for good reason, as you
   mentioned [1] [2].

2. The git daemon and protocol helpers do not always send the datum “a
   controlled fatal error occured” by writing some message (any
   message) to side band 3.

Fixing the daemon’s share in both might require setting up a side band
very early.  If an RFC patch appears setting up the side band (or an
explanation for why that’s not possible), I would be happy to start
work building from there.

That has been the big obstacle for me experimenting with it, more than
the information disclosure.  But this is easy to say.  The doing is
more important.

Thanks again, and sorry for the noise.
Jonathan

[1] I do suspect that in the case of failing enter_repo() or missing
git-daemon-export-ok, saying “cannot read the specified repo” would be
fine.  Most of the time, there is not much value in disclosing a more
detailed reason, anyway.

[2] Example fix for a problem in this class:
http://thread.gmane.org/gmane.comp.version-control.git/139029

Re: Useless error message?

[Andreas Ericsson]

On 04/22/2010 11:42 AM, Jonathan Nieder wrote:
> Junio C Hamano wrote:
> 
>> The true story is a bit different.
>>
>> To avoid information leak to git-daemon clients, we deliberately choose
>> not to give detailed error messages, so that you cannot tell if an error
>> means a user "u" does not exist or "u" does but ~u/repo.git repository
>> does not exist.
> 
> Thanks for the clarification.  As I see it, these are two different
> classes of problem:
> 
> 1. The git daemon is very quiet, usually for good reason, as you
>     mentioned [1] [2].
> 
> 2. The git daemon and protocol helpers do not always send the datum “a
>     controlled fatal error occured” by writing some message (any
>     message) to side band 3.
> 
> [1] I do suspect that in the case of failing enter_repo() or missing
> git-daemon-export-ok, saying “cannot read the specified repo” would be
> fine.  Most of the time, there is not much value in disclosing a more
> detailed reason, anyway.
> 

That would make it possible for random attackers to determine whether
a specific user exists on the system, which is very bad indeed.

> [2] Example fix for a problem in this class:
> http://thread.gmane.org/gmane.comp.version-control.git/139029

That's a different problem. We only end up in {send,receive}-pack if
the remote user asked for an existing repository, which means he or
she is either a very determined guesser or, more likely, already
knows that the user exists and where he or she keeps git repos. A
possible issue, to be sure, but definitely a far narrower window
than just guessing a username.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: Useless error message?

[Jonathan Nieder]

Andreas Ericsson wrote:
> On 04/22/2010 11:42 AM, Jonathan Nieder wrote:

>> [1] I do suspect that in the case of failing enter_repo() or missing
>> git-daemon-export-ok, saying “cannot read the specified repo” would be
>> fine.  Most of the time, there is not much value in disclosing a more
>> detailed reason, anyway.
>
> That would make it possible for random attackers to determine whether
> a specific user exists on the system, which is very bad indeed.

I guess I am missing something.  How would

(*) $ git clone git://git.example.com/~u/foo
    remote: Cannot read the specified repo

tell me whether that user existed on the system?  If the daemon gives
the same message for ENOENT, missing git-daemon-export-ok, EPERM, and
so on so I cannot distinguish the cases, then I just don’t see the
problem.

If the daemon failed for some other reason, like a flaky network, I
would see

    $ git clone git://git.example.com/~u/foo
    fatal: The remote end hung up unexpectedly

So the extra information could still be helpful, without unwanted
information disclosure.  In the case (*) I learn definitively that the
address I specified does not represent a repo I have access to, rather
than this being some random, transient unexplained problem.

Thanks for the comment.
Jonathan

Re: Useless error message?

[Andreas Ericsson]

On 04/22/2010 12:15 PM, Jonathan Nieder wrote:
> Andreas Ericsson wrote:
>> On 04/22/2010 11:42 AM, Jonathan Nieder wrote:
> 
>>> [1] I do suspect that in the case of failing enter_repo() or missing
>>> git-daemon-export-ok, saying “cannot read the specified repo” would be
>>> fine.  Most of the time, there is not much value in disclosing a more
>>> detailed reason, anyway.
>>
>> That would make it possible for random attackers to determine whether
>> a specific user exists on the system, which is very bad indeed.
> 
> I guess I am missing something.  How would
> 
> (*) $ git clone git://git.example.com/~u/foo
>      remote: Cannot read the specified repo
> 
> tell me whether that user existed on the system?  If the daemon gives
> the same message for ENOENT, missing git-daemon-export-ok, EPERM, and
> so on so I cannot distinguish the cases, then I just don’t see the
> problem.
> 
> If the daemon failed for some other reason, like a flaky network, I
> would see
> 
>      $ git clone git://git.example.com/~u/foo
>      fatal: The remote end hung up unexpectedly
> 
> So the extra information could still be helpful, without unwanted
> information disclosure.  In the case (*) I learn definitively that the
> address I specified does not represent a repo I have access to, rather
> than this being some random, transient unexplained problem.
> 

So that would be the new error message for everything that fails, then?

One big reason why I'm not bothered with running the git-daemon on a
public server is that it's very simple. If something goes wrong, it
dies without fiddling about.

How would it benefit you if it said "fatal: Something went wrong, but
I didn't crash" instead of just hanging up? If you have the wrong
repo address, you'd still have to check up with whoever gave it to
you to get it right. If it *does* crash, you'd still have to get
hold of the server admin to tell him that it has crashed.

A minor patch to git-fetch, updating the error message with a few
possible reasons would be far better. I don't care about it myself,
but I'm sure such a patch would be a lot easier to get into git.git
than something that adds a lot of complexity to the git daemon.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: Useless error message?

[Jonathan Nieder]

Andreas Ericsson wrote:
> On 04/22/2010 12:15 PM, Jonathan Nieder wrote:

>> (*) $ git clone git://git.example.com/~u/foo
>>      remote: Cannot read the specified repo
[...]
> So that would be the new error message for everything that fails, then?

No.  Of course, the opposite is the point.  I just mean there should
be an error message for all conditions that are lumped together with
ENOENT to avoid information disclosure.  I don’t care much how the
message is phrased.

> If you have the wrong
> repo address, you'd still have to check up with whoever gave it to
> you to get it right. If it *does* crash, you'd still have to get
> hold of the server admin to tell him that it has crashed.

Many things can go wrong other than a missing repo.  For example,
there might be objects missing, or high load, or memory corruption.

In the “wrong address” case, I know who to go to to fix it: ask the
person who gave me the address.

In the “corrupt repository” case, I should go to the repository
owner.

In the “server hung up without any good reason” case, I should try to
narrow down the problem, perhaps with the help of the server admin or
my network administrator.  Is it really so unusual to want to
distinguish this from other cases?

Without code this is all theoretical anyway.  And that’s the real
problem: to the uninitiated, it is not easy to write code to try this
out because the side band is not set up in time.

Sigh,
Jonathan

Re: Useless error message?

[Petr Baudis]

On Wed, Apr 21, 2010 at 05:19:54PM -0500, Jonathan Nieder wrote:
> Aghiles wrote:
> 
> > "fatal: The remote end hung up unexpectedly"
> > 
> > Is that really meaningful ? Or maybe it is a configuration problem
> > on my side ?
> 
> Please, fix it. :)

I have seen a lot of users who plainly had a lot of trouble even
_understanding_ the error message - it is phrased in super-dense
networking jargon. I think something like

	"fatal: Server terminated the connection for unknown reason"

might come a long way (though of course specific error messages would
still be far more helpful).

(I assume that the remote end is the server since (i) it is most often
the case (ii) it is if you look at it through the client-server optics,
which may not always be the best one, but see (i).)

-- 
				Petr "Pasky" Baudis
http://pasky.or.cz/ | "Ars longa, vita brevis." -- Hippocrates

Re: Useless error message?

[Ilari Liusvaara]

On Thu, Apr 22, 2010 at 04:42:16AM -0500, Jonathan Nieder wrote:
> 
> Thanks for the clarification.  As I see it, these are two different
> classes of problem:
> 
> 1. The git daemon is very quiet, usually for good reason, as you
>    mentioned [1] [2].
> 
> 2. The git daemon and protocol helpers do not always send the datum “a
>    controlled fatal error occured” by writing some message (any
>    message) to side band 3.
> 
> Fixing the daemon’s share in both might require setting up a side band
> very early.  If an RFC patch appears setting up the side band (or an
> explanation for why that’s not possible), I would be happy to start
> work building from there.

There are few subcases of daemon-level errors:

1A) Invalid request

No feedback is needed. These are protocol violations and well-behaved
clients don't send these.

1B) Request for invalid repository

These should have one error. That error can be sent using ERR response
(already supported).

1C) Request for disabled service

These too can be reported via ERR. One has to be careful not to create
information leak using these.

1D) Catastrophic network error

One can't do anything about these.

1E) Relay error

Really shouldn't happen. Due to service state being unknown at time
of things going wrong, one can't do much about these (what if
relay error occurs in middle of packet? pad packet with zeroes?)


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.

> That has been the big obstacle for me experimenting with it, more than
> the information disclosure.  But this is easy to say.  The doing is
> more important.

-Ilari

Re: Useless error message?

[Aghiles]

Hello,

>
> I have seen a lot of users who plainly had a lot of trouble even
> _understanding_ the error message - it is phrased in super-dense
> networking jargon. I think something like
>
>        "fatal: Server terminated the connection for unknown reason"
>
> might come a long way (though of course specific error messages would
> still be far more helpful).
>

I would say that:

  "fatal: Server terminated the connection."

Is best and actually says what it has to stay. "Unknown reason" sounds like a
godly intervention into the git protocol. Sometimes, the "hung up" message is
given as an extra information that actually causes more harm than good:

 % git pull hummus
 fatal: 'hummus' does not appear to be a git repository
 fatal: The remote end hung up unexpectedly

  -- aghiles

[PATCH] daemon: report inaccessible repositories to user

[Jonathan Nieder]

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