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

From: Jeff King <peff@peff.net>
Date: Oct 14, 2011, 21:12 UTC
Message-ID: <20111014211244.GA16429@sigill.intra.peff.net>
In-Reply-To: <20111014210251.GD16371@elie.hsd1.il.comcast.net>

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

Show 12 quoted lines

> > 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.

Show 8 quoted lines

> > 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.

Show 13 quoted lines

> 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).

Show 21 quoted lines

> > --- a/daemon.c
> > +++ b/daemon.c
> > @@ -20,6 +20,7 @@
> [...]
> > @@ -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