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

On Fri, Oct 14, 2011 at 04:02:51PM -0500, Jonathan Nieder wrote:

quoted

Instead, let's print an "ERR" line, which git clients
understand since v1.6.1 (2008-12-24).

Just to be clear, "git archive --remote" does not understand ERR lines
in the 'master' branch (though 908aaceb makes it understand them in
'next').  But I consider even distinguishing

 a. fatal: git archive: protocol error
 b. fatal: git archive: expected ACK/NAK, got EOF

[(a) is how an ERR response is reported, and (b) a remote hangup] to
be progress, so it's not so important. :)

Thanks, I forgot to mention that. It's not as nice as the push/fetch
case, but I agree it's a step forward.

quoted

Because there is a risk of leaking information about
non-exported repositories, by default all errors simply say
"access denied". Open sites can pass a flag to turn on more
specific messages.

I'm not sure what an "open site" is. :)  But having this flag for
sites to declare whether they consider whether a repository exists to
be privileged information seems reasonable to me.

I meant sites which are just serving a bunch of public repos, like
kernel.org.

Note that this really would be privileged information in some
not-too-weird cases.  For example, if many users have a repository at
~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to
access

	/home/alice/.git
	/home/alice/.config/.git
	/home/alice/src/linux/.git
	/home/bob/.git
	...

in turn to find a valid username, as reconnaisance for a later
attack not involving git.

I sort of assume everybody serves a specific directory hierarchy, but
maybe that is not the case. I don't run git-daemon myself, so I am
probably guilty of generalizing how other people use it.

Anyway, I think the issue is sufficiently nuanced that we should keep
the default to the conservative "access denied" (i.e., throw away my
second patch for now).

quoted

--- a/daemon.c
+++ b/daemon.c

@@ -20,6 +20,7 @@

[...]

quoted

@@ -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;
+		}
+		else if (!prefixcmp(arg, "--no-informative-errors")) {
+			informative_errors = 0;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {

Micronit: uncuddled "else".  The style of the surrounding code is to
just not include the "else" at all and rely on "continue" to
short-circuit things.

Oops. Yes, it should just drop the else to match the surrounding code.

-Peff