git: please honor DNS SRV records

Hi,

[...]

So I have played with it a little more. Very rough, completely untested, in desperate need of a test script, but I think it is ready for some feedback. To recap, this is really just an evil workaround for the lack of support for redirects in git protocol, but it has a nice side effect of making load balancing a little easier for server administrators.

Patches 1-3 are code movement, to give the code that is being changed a home and make it a little easier to contemplate. In particular patch 3 draws attention to some code from git daemon that makes DNS lookups to get the canonical hostname and IP a client contacted. Should it pay attention to SRV records?

Patch 5 is another cleanup patch. It unifies the ipv4 and ipv6 implementations of git_tcp_connect by abstracting away some of the differences between the underlying OS interfaces. Patch 4 is a tiny bugfix noticed in the process.

None of the above should be necessary for or even has much to do with patch 6, the nominal topic of the series. Sorry about that.

Anyway, patch 6 is an attempt to implement rfc2782, expanded from the implementation of a subset that Julien sent. It doesn't pay attention to the "additional data" section of the SRV response to avoid A or AAAA lookups, but it probably should.

At the back of my head there is this nagging feeling that getaddrinfo should accept an ai_flags bit to do this for us automatically, but it doesn't as far as I can tell. Maybe this can be a stepping stone toward that...

If nothing else, it's kind of fun. Thoughts? Does the general idea seem sane? Am I overlooking some standard function that takes care of this all automatically?

Before adding functionality to them, let's move the functions git_tcp_connect and git_proxy_connect used to resolve a hostname and connect to it to a separate tcp.c file.

---
 Makefile  |    2 +
 connect.c |  270 +------------------------------------------------------------
 tcp.c     |  269 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tcp.h     |    8 ++
 4 files changed, 280 insertions(+), 269 deletions(-)
 create mode 100644 tcp.c
 create mode 100644 tcp.h

index e40ac0c..fa3a47f 100644
@@ -666,6 +666,7 @@ LIB_OBJS += string-list.o
 LIB_OBJS += submodule.o
 LIB_OBJS += symlinks.o
 LIB_OBJS += tag.o
+LIB_OBJS += tcp.o
 LIB_OBJS += trace.o
 LIB_OBJS += transport.o
 LIB_OBJS += transport-helper.o

@@ -1958,6 +1959,7 @@ builtin/prune.o builtin/reflog.o reachable.o: reachable.h
 builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
 builtin/tar-tree.o archive-tar.o: tar.h
 connect.o transport.o http-backend.o: url.h
+connect.o tcp.o: tcp.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
 http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 

new file mode 100644
index 0000000..4de5f71
@@ -0,0 +1,8 @@
+#ifndef TCP_H
+#define TCP_H
+
+extern int git_use_proxy(const char *host);
+extern void git_tcp_connect(int fd[2], char *host, int flags);
+extern struct child_process *git_proxy_connect(int fd[2], char *host);
+
+#endif

1.7.5.3

For each incoming connection, the git daemon runs a DNS query using the client-supplied hostname parameter, to determine the canonical hostname and IP address in case it is needed by the --interpolated-path feature (to distinguish between virtual hosts). Put the code for this in a separate function so the details of this lookup can be easily tweaked.

Signed-off-by; Jonathan Nieder <[email protected]>

---
 daemon.c |  112 +++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 63 insertions(+), 49 deletions(-)

index 4c8346d..3958cb6 100644
@@ -432,6 +432,65 @@ static void parse_host_and_port(char *hostport, char **host,
 	}
 }
 
+#ifndef NO_IPV6
+
+static void locate_host(const char *hostname, char **ip_address,
+						char **canon_hostname)
+{
+	struct addrinfo hints;
+	struct addrinfo *ai;
+	int gai;
+	static char addrbuf[HOST_NAME_MAX + 1];
+	struct sockaddr_in *sin_addr;
+
+	memset(&hints, 0, sizeof(hints));
+	hints.ai_flags = AI_CANONNAME;
+
+	gai = getaddrinfo(hostname, NULL, &hints, &ai);
+	if (gai)
+		return;
+
+	sin_addr = (void *)ai->ai_addr;
+	inet_ntop(AF_INET, &sin_addr->sin_addr, addrbuf, sizeof(addrbuf));
+	free(*ip_address);
+	*ip_address = xstrdup(addrbuf);
+
+	free(*canon_hostname);
+	*canon_hostname = xstrdup(ai->ai_canonname ?
+				  ai->ai_canonname : *ip_address);
+
+	freeaddrinfo(ai);
+}
+
+#else
+
+static void locate_host(const char *hostname, char **ip_address,
+						char **canon_hostname)
+{
+	struct hostent *hent;
+	struct sockaddr_in sa;
+	char **ap;
+	static char addrbuf[HOST_NAME_MAX + 1];
+
+	hent = gethostbyname(hostname);
+
+	ap = hent->h_addr_list;
+	memset(&sa, 0, sizeof sa);
+	sa.sin_family = hent->h_addrtype;
+	sa.sin_port = htons(0);
+	memcpy(&sa.sin_addr, *ap, hent->h_length);
+
+	inet_ntop(hent->h_addrtype, &sa.sin_addr,
+		  addrbuf, sizeof(addrbuf));
+
+	free(*canon_hostname);
+	*canon_hostname = xstrdup(hent->h_name);
+	free(*ip_address);
+	*ip_address = xstrdup(addrbuf);
+}
+
+#endif
+
 /*
  * Read the host as supplied by the client connection.
  */

@@ -467,56 +526,11 @@ static void parse_host_arg(char *extra_args, int buflen)
 	}
 
 	/*
-	 * Locate canonical hostname and its IP address.
+	 * Locate canonical hostname and its IP address,
+	 * if possible.
 	 */
-	if (hostname) {
-#ifndef NO_IPV6
-		struct addrinfo hints;
-		struct addrinfo *ai;
-		int gai;
-		static char addrbuf[HOST_NAME_MAX + 1];
-
-		memset(&hints, 0, sizeof(hints));
-		hints.ai_flags = AI_CANONNAME;
-
-		gai = getaddrinfo(hostname, NULL, &hints, &ai);
-		if (!gai) {
-			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
-
-			inet_ntop(AF_INET, &sin_addr->sin_addr,
-				  addrbuf, sizeof(addrbuf));
-			free(ip_address);
-			ip_address = xstrdup(addrbuf);
-
-			free(canon_hostname);
-			canon_hostname = xstrdup(ai->ai_canonname ?
-						 ai->ai_canonname : ip_address);
-
-			freeaddrinfo(ai);
-		}
-#else
-		struct hostent *hent;
-		struct sockaddr_in sa;
-		char **ap;
-		static char addrbuf[HOST_NAME_MAX + 1];
-
-		hent = gethostbyname(hostname);
-
-		ap = hent->h_addr_list;
-		memset(&sa, 0, sizeof sa);
-		sa.sin_family = hent->h_addrtype;
-		sa.sin_port = htons(0);
-		memcpy(&sa.sin_addr, *ap, hent->h_length);
-
-		inet_ntop(hent->h_addrtype, &sa.sin_addr,
-			  addrbuf, sizeof(addrbuf));
-
-		free(canon_hostname);
-		canon_hostname = xstrdup(hent->h_name);
-		free(ip_address);
-		ip_address = xstrdup(addrbuf);
-#endif
-	}
+	if (hostname)
+		locate_host(hostname, &ip_address, &canon_hostname);
 }
 
 

1.7.5.3

Keep the different name resolution functions close together so they can learn from each other and perhaps share code in the future.

---
 Makefile |    2 +-
 daemon.c |   62 ++------------------------------------------------------------
 tcp.c    |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 tcp.h    |    3 +++
 4 files changed, 59 insertions(+), 61 deletions(-)

index fa3a47f..df84157 100644
@@ -1959,7 +1959,7 @@ builtin/prune.o builtin/reflog.o reachable.o: reachable.h
 builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
 builtin/tar-tree.o archive-tar.o: tar.h
 connect.o transport.o http-backend.o: url.h
-connect.o tcp.o: tcp.h
+connect.o daemon.o tcp.o: tcp.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
 http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 

index 4de5f71..bed3cdc 100644
@@ -1,6 +1,9 @@
 #ifndef TCP_H
 #define TCP_H
 
+extern void git_locate_host(const char *hostname,
+			char **ip_address, char **canon_hostname);
+
 extern int git_use_proxy(const char *host);
 extern void git_tcp_connect(int fd[2], char *host, int flags);
 extern struct child_process *git_proxy_connect(int fd[2], char *host);

1.7.5.3

Ever since v1.5.3-rc0~185^2~1 (Verbose connect messages to show the IP addresses used, 2007-05-23), git's ipv4 connection failure messages have looked something like

	git.example.com[2: 127.0.0.1]: errno=Network Unreachable

The increasing index ("2" above) indicates which item in the list of addresses returned by gethostbyname was being tried so the operator can tell that git was not just trying to resolve the same hostname again and again.

The ipv6 codepath prints something similar, but the counter is never incremented so the index is always "0". Fix it.

---
 tcp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 2cb90db..9916272 100644
@@ -134,7 +134,7 @@ static int git_tcp_connect_sock(char *host, int flags)
 	const char *port = STR(DEFAULT_GIT_PORT);
 	struct addrinfo hints, *ai0, *ai;
 	int gai;
-	int cnt = 0;
+	int cnt;
 
 	get_host_and_port(&host, &port);
 	if (!*port)

@@ -154,7 +154,7 @@ static int git_tcp_connect_sock(char *host, int flags)
 	if (flags & CONNECT_VERBOSE)
 		fprintf(stderr, "done.\nConnecting to %s (port %s) ... ", host, port);
 
-	for (ai0 = ai; ai; ai = ai->ai_next) {
+	for (cnt = 0, ai0 = ai; ai; ai = ai->ai_next, cnt++) {
 		sockfd = socket(ai->ai_family,
 				ai->ai_socktype, ai->ai_protocol);
 		if (sockfd < 0) {

1.7.5.3

The new DNS API abstracts away differences between the gethostbyname- and getaddrinfo-centric interfaces for looking up a host, making the code to use them in connect.c a little easier to read.

To make a lookup:

	resolver_result ai;
	dns_resolve(host, port, 0, &ai);
	...
	dns_free(ai);

To iterate over responses:

	resolved_address i;
	for_each_address(i, ai) {
		...
	}

In the !NO_IPV6 codepath, the git_locate_host function used to find the canonical IP and hostname for a git server's public address (for virtual hosting) tells getaddrinfo to restrict attention to TCP services after this patch. That should make a difference because the service parameter is NULL.

No functional change intended.

---
 Makefile   |    5 ++
 dns-ipv4.c |   33 +++++++++++
 dns-ipv4.h |   65 ++++++++++++++++++++++
 dns-ipv6.c |   45 +++++++++++++++
 dns-ipv6.h |   31 ++++++++++
 tcp.c      |  180 +++++++++++-------------------------------------------------
 6 files changed, 211 insertions(+), 148 deletions(-)
 create mode 100644 dns-ipv4.c
 create mode 100644 dns-ipv4.h
 create mode 100644 dns-ipv6.c
 create mode 100644 dns-ipv6.h

index df84157..7d3cb41 100644
@@ -1474,6 +1474,11 @@ ifdef NO_TRUSTABLE_FILEMODE
 endif
 ifdef NO_IPV6
 	BASIC_CFLAGS += -DNO_IPV6
+	LIB_OBJS += dns-ipv4.o
+	LIB_H += dns-ipv4.h
+else
+	LIB_OBJS += dns-ipv6.o
+	LIB_H += dns-ipv6.h
 endif
 ifdef NO_UINTMAX_T
 	BASIC_CFLAGS += -Duintmax_t=uint32_t

new file mode 100644
index 0000000..39849c3
@@ -0,0 +1,33 @@
+#include "cache.h"
+#include "dns-ipv4.h"
+
+int dns_resolve(const char *host, const char *port, int flags,
+		resolver_result *res)
+{
+	char *ep;
+	struct hostent *he;
+	unsigned int nport;
+
+	he = gethostbyname(host);
+	if (!he && (flags & RESOLVE_IGNORE_ERROR))
+		return -1;
+	if (!he)
+		die("Unable to look up %s (%s)", host, hstrerror(h_errno));
+
+	if (!port) {
+		nport = 0;
+		goto done;
+	}
+	nport = strtoul(port, &ep, 10);
+	if ( ep == port || *ep ) {
+		/* Not numeric */
+		struct servent *se = getservbyname(port,"tcp");
+		if ( !se )
+			die("Unknown port %s", port);
+		nport = se->s_port;
+	}
+done:
+	res->he = he;
+	res->port = nport;
+	return 0;
+}

new file mode 100644
index 0000000..339877f
@@ -0,0 +1,31 @@
+#ifndef DNS_IPV6_H
+#define DNS_IPV6_H
+
+typedef struct addrinfo *resolver_result;
+typedef const struct addrinfo *resolved_address;
+
+enum {
+	RESOLVE_CANONNAME = 1,
+	RESOLVE_IGNORE_ERROR = 2
+};
+extern int dns_resolve(const char *host, const char *port, int flags,
+			resolver_result *res);
+/* result is in static buffer */
+extern const char *dns_name(const resolved_address *i);
+/* result is in malloc'ed buffer */
+extern char *dns_ip_address(const resolved_address *i,
+				const resolver_result *ai);
+
+#define for_each_address(i, ai) \
+	for (i = ai; i; i = (i)->ai_next)
+
+#define dns_family(i, ai) ((i)->ai_family)
+#define dns_socktype(i, ai) ((i)->ai_socktype)
+#define dns_protocol(i, ai) ((i)->ai_protocol)
+#define dns_addr(i, ai) ((i)->ai_addr)
+#define dns_addrlen(i, ai) ((i)->ai_addrlen)
+#define dns_canonname(i, ai) ((i)->ai_canonname)
+
+#define dns_free(ai) freeaddrinfo(ai)
+
+#endif

1.7.5.3

Sorry, the last sentence should say "That should _not_ make a difference". You would be able to tell the difference between the before and after with "strace git daemon" because of the ai_socktype and ai_protocol settings:

[...]

As rfc2782 explains, SRV records allow administrators "to use serveral servers for a single domain, to move services from host to host with little fuss, and to designate some hosts as primary servers for a service and others as backups". Someone noticed in a recent server move that that second part would have been very handy (since the git protocol doesn't include a concept of redirects).

Sadly, getaddrinfo itself does not include a facility to resolve SRV records automatically. So this patch uses the libbind functions from libresolv to parse the response for ourselves. Hopefully after reading enough specimens of code like this, someone will be disgusted enough to extend libc to take care of this in general.

RFC 2782 requires some non-determinism in the order of hosts contacted; this patch uses drand48() for that. To avoid causing trouble for platforms without the libbind ns_* functions or drand48(), the new SRV support is only built when requested with the USE_SRV_RR compile-time option.

Based on a patch by Julien Cristau <[email protected]>.

---
Thanks for reading.

 Makefile |   10 ++
 srv.c    |  290 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 srv.h    |   15 +++
 tcp.c    |   90 +++++++++++++-------
 4 files changed, 375 insertions(+), 30 deletions(-)
 create mode 100644 srv.c
 create mode 100644 srv.h

index 7d3cb41..316b29e 100644
@@ -124,6 +124,10 @@ all::
 # Notably on Solaris hstrerror resides in libresolv and on Solaris 7
 # inet_ntop and inet_pton additionally reside there.
 #
+# Define USE_SRV_RR if you want git to pay attention to SRV resource records
+# when looking up servers to contact over git protocol.  This implies
+# NEEDS_RESOLV.
+#
 # Define NO_MMAP if you want to avoid mmap.
 #
 # Define NO_PTHREADS if you do not have or do not want to use Pthreads.

@@ -1357,6 +1361,11 @@ endif
 ifdef NEEDS_NSL
 	EXTLIBS += -lnsl
 endif
+ifdef USE_SRV_RR
+	BASIC_CFLAGS += -DUSE_SRV_RR
+	LIB_OBJS += srv.o
+	NEEDS_RESOLV = YesPlease
+endif
 ifdef NEEDS_RESOLV
 	EXTLIBS += -lresolv
 endif

@@ -1965,6 +1974,7 @@ builtin/commit.o builtin/revert.o wt-status.o: wt-status.h
 builtin/tar-tree.o archive-tar.o: tar.h
 connect.o transport.o http-backend.o: url.h
 connect.o daemon.o tcp.o: tcp.h
+tcp.o srv.o: srv.h
 http-fetch.o http-walker.o remote-curl.o transport.o walker.o: walker.h
 http.o http-walker.o http-push.o http-fetch.o remote-curl.o: http.h url.h
 

new file mode 100644
index 0000000..7cea4f4
@@ -0,0 +1,15 @@
+#ifndef SRV_H
+#define SRV_H
+
+struct host {
+	char *hostname;
+	char *port;
+};
+
+#ifndef USE_SRV_RR
+#define get_srv(host, hosts) 0
+#else
+extern int get_srv(const char *host, struct host **hosts);
+#endif
+
+#endif

1.7.5.3

Here's a diffstat for the curious.

Jonathan Nieder (6):
transport: expose git_tcp_connect and friends in new tcp.h
daemon: make host resolution into a separate function
daemon: move locate_host to tcp.c
transport: fix index in ipv6 connection failed message
tcp: unify ipv4 and ipv6 code paths
transport: learn to honor DNS SRV records

 Makefile   |   17 ++++
 connect.c  |  270 +-------------------------------------------------------
 daemon.c   |   54 +----------
 dns-ipv4.c |   33 +++++++
 dns-ipv4.h |   65 ++++++++++++++
 dns-ipv6.c |   45 +++++++++
 dns-ipv6.h |   31 +++++++
 srv.c      |  290 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 srv.h      |   15 +++
 tcp.c      |  236 ++++++++++++++++++++++++++++++++++++++++++++++++
 tcp.h      |   11 +++
 11 files changed, 749 insertions(+), 318 deletions(-)
 create mode 100644 dns-ipv4.c
 create mode 100644 dns-ipv4.h
 create mode 100644 dns-ipv6.c
 create mode 100644 dns-ipv6.h
 create mode 100644 srv.c
 create mode 100644 srv.h
 create mode 100644 tcp.c
 create mode 100644 tcp.h