[PATCH 1/3] git-daemon: single-line logs

[PATCH 1/3] git-daemon: single-line logs

[Jan Engelhardt]

parent v1.6.1

git-daemon: single-line logs

Having just a single line per connection attempt, much like Apache
httpd2 access logs, makes log parsing much easier, especially when
just glancing over it non-automated.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

---
 daemon.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Index: git-1.6.1/daemon.c
===================================================================
--- git-1.6.1.orig/daemon.c
+++ git-1.6.1/daemon.c
@@ -295,12 +295,13 @@ static int git_daemon_config(const char
 	return 0;
 }
 
-static int run_service(char *dir, struct daemon_service *service)
+static int run_service(char *dir, struct daemon_service *service,
+    const char *origin, const char *vhost)
 {
 	const char *path;
 	int enabled = service->enabled;
 
-	loginfo("Request %s for '%s'", service->name, dir);
+	loginfo("%s->%s %s \"%s\"\n", origin, vhost, service->name, dir);
 
 	if (!enabled && !service->overridable) {
 		logerror("'%s': service not enabled.", service->name);
@@ -507,10 +508,10 @@ static void parse_extra_args(char *extra
 static int execute(struct sockaddr *addr)
 {
 	static char line[1000];
+	char addrbuf[256] = "";
 	int pktlen, len, i;
 
 	if (addr) {
-		char addrbuf[256] = "";
 		int port = -1;
 
 		if (addr->sa_family == AF_INET) {
@@ -529,7 +530,6 @@ static int execute(struct sockaddr *addr
 			port = ntohs(sin6_addr->sin6_port);
 #endif
 		}
-		loginfo("Connection from %s:%d", addrbuf, port);
 		setenv("REMOTE_ADDR", addrbuf, 1);
 	}
 	else {
@@ -541,10 +541,6 @@ static int execute(struct sockaddr *addr
 	alarm(0);
 
 	len = strlen(line);
-	if (pktlen != len)
-		loginfo("Extended attributes (%d bytes) exist <%.*s>",
-			(int) pktlen - len,
-			(int) pktlen - len, line + len + 1);
 	if (len && line[len-1] == '\n') {
 		line[--len] = 0;
 		pktlen--;
@@ -569,7 +565,8 @@ static int execute(struct sockaddr *addr
 			 * Note: The directory here is probably context sensitive,
 			 * and might depend on the actual service being performed.
 			 */
-			return run_service(line + namelen + 5, s);
+			return run_service(line + namelen + 5, s,
+			       addrbuf, hostname);
 		}
 	}
 

[PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Jan Engelhardt]

parent v1.6.1

git-daemon: use getnameinfo to resolve hostname

This is much shorter than inet_ntop'ing, and also translated
unresolvable addresses into a string.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

---
 daemon.c |   26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

Index: git-1.6.1/daemon.c
===================================================================
--- git-1.6.1.orig/daemon.c
+++ git-1.6.1/daemon.c
@@ -512,25 +512,13 @@ static int execute(struct sockaddr *addr
 	int pktlen, len, i;
 
 	if (addr) {
-		int port = -1;
-
-		if (addr->sa_family == AF_INET) {
-			struct sockaddr_in *sin_addr = (void *) addr;
-			inet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf, sizeof(addrbuf));
-			port = ntohs(sin_addr->sin_port);
-#ifndef NO_IPV6
-		} else if (addr && addr->sa_family == AF_INET6) {
-			struct sockaddr_in6 *sin6_addr = (void *) addr;
-
-			char *buf = addrbuf;
-			*buf++ = '['; *buf = '\0'; /* stpcpy() is cool */
-			inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf, sizeof(addrbuf) - 1);
-			strcat(buf, "]");
-
-			port = ntohs(sin6_addr->sin6_port);
-#endif
-		}
-		setenv("REMOTE_ADDR", addrbuf, 1);
+		i = getnameinfo(addr, (addr->sa_family == AF_INET6) ?
+		    sizeof(struct sockaddr_in6) : sizeof(struct sockaddr_in),
+		    addrbuf, sizeof(addrbuf), NULL, 0, 0);
+		if (i == 0)
+			setenv("REMOTE_ADDR", addrbuf, 1);
+		else
+			unsetenv("REMOTE_ADDR");
 	}
 	else {
 		unsetenv("REMOTE_ADDR");

[PATCH 3/3] git-daemon: vhost support

[Jan Engelhardt]

parent v1.6.1

git-daemon: support vhosts

Since git clients usually send the target hostname in the request
similar to the "Host:" HTTP header, one can do virtual hosting.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

---
 daemon.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Index: git-1.6.1/daemon.c
===================================================================
--- git-1.6.1.orig/daemon.c
+++ git-1.6.1/daemon.c
@@ -2,6 +2,7 @@
 #include "pkt-line.h"
 #include "exec_cmd.h"
 
+#include <stdbool.h>
 #include <syslog.h>
 
 #ifndef HOST_NAME_MAX
@@ -21,7 +22,7 @@ static const char daemon_usage[] =
 "           [--timeout=n] [--init-timeout=n] [--max-connections=n]\n"
 "           [--strict-paths] [--base-path=path] [--base-path-relaxed]\n"
 "           [--user-path | --user-path=path]\n"
-"           [--interpolated-path=path]\n"
+"           [--interpolated-path=path] [--vhost]\n"
 "           [--reuseaddr] [--detach] [--pid-file=file]\n"
 "           [--[enable|disable|allow-override|forbid-override]=service]\n"
 "           [--inetd | [--listen=host_or_ipaddr] [--port=n]\n"
@@ -36,6 +37,7 @@ static int strict_paths;
 static int export_all_trees;
 
 /* Take all paths relative to this one if non-NULL */
+static bool enable_vhosting;
 static char *base_path;
 static char *interpolated_path;
 static int base_path_relaxed;
@@ -309,8 +311,18 @@ static int run_service(char *dir, struct
 		return -1;
 	}
 
-	if (!(path = path_ok(dir)))
-		return -1;
+	if (enable_vhosting) {
+		char vdir[256];
+
+		if (avoid_alias(dir) != 0)
+			return -1;
+		snprintf(vdir, sizeof(vdir), "/%s%s", hostname, dir);
+		if ((path = path_ok(vdir)) == NULL)
+			return -1;
+	} else {
+		if ((path = path_ok(dir)) == NULL)
+			return -1;
+	}
 
 	/*
 	 * Security on the cheap.
@@ -1046,6 +1058,10 @@ int main(int argc, char **argv)
 			make_service_overridable(arg + 18, 0);
 			continue;
 		}
+		if (strcmp(arg, "--vhost") == 0) {
+			enable_vhosting = true;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;

Re: [PATCH 1/3] git-daemon: single-line logs

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> parent v1.6.1
>
> git-daemon: single-line logs

Please drop these two needless lines when/if you are submitting patches
for inclusion..

> Having just a single line per connection attempt, much like Apache
> httpd2 access logs, makes log parsing much easier, especially when
> just glancing over it non-automated.

While I like the motivation, and I wish the log were as terse as possible
from the day one, I think changing the output format unconditionally like
this patch does is a horrible idea.  I'd expect there are many people who
already have their infrastructure set up to parse the current output; this
patch actively breaks things for them, doesn't it?

> @@ -295,12 +295,13 @@ static int git_daemon_config(const char
>  	return 0;
>  }
>  
> -static int run_service(char *dir, struct daemon_service *service)
> +static int run_service(char *dir, struct daemon_service *service,
> +    const char *origin, const char *vhost)
>  {
>  	const char *path;
>  	int enabled = service->enabled;
>  
> -	loginfo("Request %s for '%s'", service->name, dir);
> +	loginfo("%s->%s %s \"%s\"\n", origin, vhost, service->name, dir);

Mental note.  You are adding origin and vhost probably because you are
losing them from elsewhere..

> @@ -507,10 +508,10 @@ static void parse_extra_args(char *extra
>  static int execute(struct sockaddr *addr)
>  {
>  	static char line[1000];
> +	char addrbuf[256] = "";
>  	int pktlen, len, i;
>  
>  	if (addr) {
> -		char addrbuf[256] = "";
>  		int port = -1;
>  
>  		if (addr->sa_family == AF_INET) {
> @@ -529,7 +530,6 @@ static int execute(struct sockaddr *addr
>  			port = ntohs(sin6_addr->sin6_port);
>  #endif
>  		}
> -		loginfo("Connection from %s:%d", addrbuf, port);

Mental note.  Port is not logged anymore here.

> @@ -541,10 +541,6 @@ static int execute(struct sockaddr *addr
>  	alarm(0);
>  
>  	len = strlen(line);
> -	if (pktlen != len)
> -		loginfo("Extended attributes (%d bytes) exist <%.*s>",
> -			(int) pktlen - len,
> -			(int) pktlen - len, line + len + 1);

Mental note.  XA are not logged here anymore.

> @@ -569,7 +565,8 @@ static int execute(struct sockaddr *addr
>  			 * Note: The directory here is probably context sensitive,
>  			 * and might depend on the actual service being performed.
>  			 */
> -			return run_service(line + namelen + 5, s);
> +			return run_service(line + namelen + 5, s,
> +			       addrbuf, hostname);
>  		}
>  	}

So not just you are changing the format, but you are losing information as
well.

By the way, I think hostname has already been freed and NULLed at this
call site.  Aren't you getting entries like:

	192.168.0.1->(null) upload-pack "/pub/git.git"

in your log?

Re: [PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> parent v1.6.1
>
> git-daemon: use getnameinfo to resolve hostname
>
> This is much shorter than inet_ntop'ing, and also translated
> unresolvable addresses into a string.

translated?  (I think you meant "translates" but my English is bad, so I
am double checking).

This indeed is much nicer, provided if it is available at least as widely
as inet_ntop() is.

We seem to ship inet_ntop() in compat/; a few questions.

 (1) Do we need similar compat/ function for getnameinfo()?  I am guessing
     that most likely places are the ones that need NO_INET_NTOP and
     NO_INET_PTON, and googling seems to indicate old Cygwin and HP-UX
     11.00 may be among them.

 (2) Do we still use inet_ntop() elsewhere, and if not, can we remove the
     compat/ definitions?

Re: [PATCH 3/3] git-daemon: vhost support

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> parent v1.6.1
>
> git-daemon: support vhosts
>
> Since git clients usually send the target hostname in the request
> similar to the "Host:" HTTP header, one can do virtual hosting.

Isn't this what --interpolated-path option (especially H and CH
interpolations) is about?

Re: [PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Jeff King]

On Wed, Jan 14, 2009 at 11:48:38AM +0100, Jan Engelhardt wrote:

> This is much shorter than inet_ntop'ing, and also translated
> unresolvable addresses into a string.

Er, doesn't this totally change the meaning of REMOTE_ADDR from an IP
address to a hostname? That seems bad because:

  - people already have hooks that compare REMOTE_ADDR against an
    address, so we are breaking their hooks

  - we are losing IP information in favor of hostname information; since
    (I assume) this is primarily intended for IP-based access control,
    we are adding an extra layer of indirection in the middle of our
    security model (i.e., I used to have to spoof an IP to fool your
    hook, but now I can do that _or_ spoof DNS).

So at the very least, you should be adding REMOTE_HOST in _addition_ to
REMOTE_ADDR, not instead of. But that still leaves one final concern,
which is that some git-daemon admins might not want to pay the cost for
a reverse lookup for every request. It's extra network traffic, and adds
extra latency to the process (but I don't personally run git-daemon, and
I don't know whether big sites like kernel.org actually care about
this).

-Peff

Re: [PATCH 1/3] git-daemon: single-line logs

[Jan Engelhardt]

On Wednesday 2009-01-14 12:33, Junio C Hamano wrote:
>
>> parent v1.6.1
>>
>> git-daemon: single-line logs
>
>Please drop these two needless lines when/if you are submitting patches
>for inclusion..

The patches are produced by my git-export-patch script (or in this
specific case it was quilt); so they usually look like this.
Especially when I do make mental notes that do not go into the
commit log, the patch is tacked on, as in, for example,
http://marc.info/?l=netfilter-devel&m=123191159015731&w=2

>> Having just a single line per connection attempt, much like Apache
>> httpd2 access logs, makes log parsing much easier, especially when
>> just glancing over it non-automated.
>
>While I like the motivation, and I wish the log were as terse as possible
>from the day one,

Well, why did no one do it like that then? My guess is that it was not a 
"real" logging infrastructure but more of a debug aid. Especially when 
it required --syslog --verbose to pass to git-daemon this seems like 
--debug=yesPlease.

>I think changing the output format unconditionally like
>this patch does is a horrible idea.  I'd expect there are many people who
>already have their infrastructure set up to parse the current output; this
>patch actively breaks things for them, doesn't it?

Probably. Which just shows that git-daemon is in need of
some configuration .. thing so that each user can choose
his own if desired.


>> @@ -295,12 +295,13 @@ static int git_daemon_config(const char
>> -static int run_service(char *dir, struct daemon_service *service)
>> +static int run_service(char *dir, struct daemon_service *service,
>> +    const char *origin, const char *vhost)
>>  {
>>  	const char *path;
>>  	int enabled = service->enabled;
>>  
>> -	loginfo("Request %s for '%s'", service->name, dir);
>> +	loginfo("%s->%s %s \"%s\"\n", origin, vhost, service->name, dir);
>
>Mental note.  You are adding origin and vhost probably because you are
>losing them from elsewhere..

Not quite sure what you mean by losing.

But in 1.6.0.x, run_service had a variable interp of type itable or so 
and it was possible to use interp[SLOT_DIR].val without someone raising 
a hand declaring I lost them elsewhere ;-)

>> @@ -529,7 +530,6 @@ static int execute(struct sockaddr *addr
>>  			port = ntohs(sin6_addr->sin6_port);
>>  #endif
>>  		}
>> -		loginfo("Connection from %s:%d", addrbuf, port);
>
>Mental note.  Port is not logged anymore here.

Right, I did not see a need for it, and it in itself just stood
in the way of getting 1-line-output.

>> @@ -541,10 +541,6 @@ static int execute(struct sockaddr *addr
>>  	alarm(0);
>>  
>>  	len = strlen(line);
>> -	if (pktlen != len)
>> -		loginfo("Extended attributes (%d bytes) exist <%.*s>",
>> -			(int) pktlen - len,
>> -			(int) pktlen - len, line + len + 1);
>
>Mental note.  XA are not logged here anymore.

I only ever saw the hostname XA, and this is still logged.

>> @@ -569,7 +565,8 @@ static int execute(struct sockaddr *addr
>>  			 * Note: The directory here is probably context sensitive,
>>  			 * and might depend on the actual service being performed.
>>  			 */
>> -			return run_service(line + namelen + 5, s);
>> +			return run_service(line + namelen + 5, s,
>> +			       addrbuf, hostname);
>>  		}
>>  	}
>
>So not just you are changing the format, but you are losing information as
>well.
>
>By the way, I think hostname has already been freed and NULLed at this
>call site.  Aren't you getting entries like:
>
>	192.168.0.1->(null) upload-pack "/pub/git.git"
>
>in your log?

No. Which means someone succeeded at obfuscating daemon.c.
It seems that parse_extra_args() sets hostname again after it has been 
NULLified just moments ago.

Re: [PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Jan Engelhardt]

On Wednesday 2009-01-14 12:33, Junio C Hamano wrote:
>> git-daemon: use getnameinfo to resolve hostname
>>
>> This is much shorter than inet_ntop'ing, and also translated
>> unresolvable addresses into a string.
>
>translated?  (I think you meant "translates" but my English is bad, so I
>am double checking).

yes, keyboard slipped away.

>This indeed is much nicer, provided if it is available at least as widely
>as inet_ntop() is.

Both inet_ntop and getnameinfo are in POSIX.1-2001.

> (1) Do we need similar compat/ function for getnameinfo()?  I am guessing
>     that most likely places are the ones that need NO_INET_NTOP and
>     NO_INET_PTON, and googling seems to indicate old Cygwin and HP-UX
>     11.00 may be among them.
>
> (2) Do we still use inet_ntop() elsewhere, and if not, can we remove the
>     compat/ definitions?
>

Yes, it is still used elsewhere.

Re: [PATCH 3/3] git-daemon: vhost support

[Jan Engelhardt]

On Wednesday 2009-01-14 12:33, Junio C Hamano wrote:
>> git-daemon: support vhosts
>>
>> Since git clients usually send the target hostname in the request
>> similar to the "Host:" HTTP header, one can do virtual hosting.
>
>Isn't this what --interpolated-path option (especially H and CH
>interpolations) is about?

Looks like it. In this case this third patch is not needed.

Re: [PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Adeodato Simó]

* Jeff King [Wed, 14 Jan 2009 07:25:36 -0500]:

> On Wed, Jan 14, 2009 at 11:48:38AM +0100, Jan Engelhardt wrote:

> > This is much shorter than inet_ntop'ing, and also translated
> > unresolvable addresses into a string.

> Er, doesn't this totally change the meaning of REMOTE_ADDR from an IP
> address to a hostname?

Yes, I believe so.

However, AFAIK you can obtain the intended behavior if you pass
NI_NUMERICHOST as a flag to the getnameinfo() call. With that, this
patch can be still considered for inclusing if the original "don't
hardcode protocol-specific bits" is still deemed worthy.

-- 
Adeodato Simó                                     dato at net.com.org.es
Debian Developer                                  adeodato at debian.org
 
- Why are you whispering?
- Because I just think that no matter where she is, my mom can hear this
  conversation.
                -- Rory and Lane

Re: [PATCH 2/3] git-daemon: use getnameinfo to resolve hostname

[Jay Soffian]

On Wed, Jan 14, 2009 at 7:25 AM, Jeff King <peff@peff.net> wrote:
> So at the very least, you should be adding REMOTE_HOST in _addition_ to
> REMOTE_ADDR, not instead of. But that still leaves one final concern,
> which is that some git-daemon admins might not want to pay the cost for
> a reverse lookup for every request. It's extra network traffic, and adds
> extra latency to the process (but I don't personally run git-daemon, and
> I don't know whether big sites like kernel.org actually care about
> this).

Speaking for large sites everywhere, yes they do care. Enabling DNS
lookups must be configurable.

j.

Re: [2/3] git-daemon: use getnameinfo to resolve hostname

[Jan Engelhardt]

On Wednesday 2009-01-14 13:25, Jeff King wrote:
>On Wed, Jan 14, 2009 at 11:48:38AM +0100, Jan Engelhardt wrote:
>
>> This is much shorter than inet_ntop'ing, and also translated
>> unresolvable addresses into a string.
>
>Er, doesn't this totally change the meaning of REMOTE_ADDR from an IP
>address to a hostname? That seems bad because:
>[...]
>  - people already have hooks that compare REMOTE_ADDR against an
>    address, so we are breaking their hooks
>[...]
>So at the very least, you should be adding REMOTE_HOST in _addition_ to
>REMOTE_ADDR, not instead of.

Good catch. It's always good to have someone else look through it.
Changed, and below is the proposition as a non-mergable diff.

In case getnameinfo fails, the IP address from inet_ntop
should be left in addrbuf, is not it?

And yeah, it does not have a flag to disable DNS resolution, but
it's a draft for now.

---8<---
git-daemon: resolve source host's DNS

Try to resolve DNS addresses so that run_service() can print the
name of the host from which the request originated.
[addrbuf is passed to run_service as a result of patch 1/3]

---
 daemon.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: git-1.6.1/daemon.c
===================================================================
--- git-1.6.1.orig/daemon.c
+++ git-1.6.1/daemon.c
@@ -530,6 +530,10 @@ static int execute(struct sockaddr *addr
 #endif
 		}
 		setenv("REMOTE_ADDR", addrbuf, 1);
+		getnameinfo(addr, (addr->sa_family == AF_INET6) ?
+			sizeof(struct sockaddr_in6) :
+			sizeof(struct sockaddr_in),
+			addrbuf, sizeof(addrbuf), NULL, 0, 0);
 	}
 	else {
 		unsetenv("REMOTE_ADDR");