README.md

@@ -64,11 +64,16 @@ DEBUG=routewatch ./routewatch
   collector, stream and handler queue statistics
 - `GET /api/v1/ip/{ip}` - Looks up each address: its most specific live route
   (`netblock`), that route's origin AS with its WHOIS details, and the address's
-  PTR names. `{ip}` is an IP address, a host name (each address it resolves to
-  is looked up), or a comma-separated list of these. `results` has one entry per
-  address with a live route and `errors` one per address or name that failed.
-  The answer is 400 when nothing in `{ip}` is an IP address or a host name that
-  resolves, and 404 when none of the addresses has a live route.
+  PTR names. `{ip}` is an IP address, a host name, or a comma-separated list of
+  at most 10 of these. Of the addresses a host name resolves to, only public
+  ones are looked up: a name with none, such as one that resolves to a private
+  address, is reported as `host name has no public address`, without its
+  addresses. `results` has one entry per address with a live route and `errors`
+  one per address or name that failed. The answer is 400 when `{ip}` lists more
+  than 10, when it comes to more than 10 addresses once its names are resolved,
+  or when nothing in it is an IP address or a host name with a public address;
+  and 404 when none of the addresses has a live route. A PTR lookup gets 2
+  seconds; an address whose lookup takes longer is answered without PTR names.
 - `GET /api/v1/as/{asn}` - The AS and the prefixes it originates, IPv4 and IPv6
   apart, each with when its newest route was updated
 - `GET /api/v1/prefix/{prefix}/{length}` - Every live route for the prefix, one

internal/server/handlers.go

@@ -31,6 +31,18 @@ const (
 	healthCheckTimeout = 2 * time.Second
 )
 
+// Limits of one IP lookup.
+const (
+	// maxLookupTargets is the most addresses and host names an IP lookup may
+	// list, and the most addresses it may have once the names are resolved:
+	// each address costs up to 113 database reads and a PTR lookup.
+	maxLookupTargets = 10
+
+	// ptrLookupTimeout is each PTR lookup's own time limit, so that ten
+	// addresses whose PTR lookups never answer fit in the 30-second limit.
+	ptrLookupTimeout = 2 * time.Second
+)
+
 // Keys of the JSON envelope the handlers and middleware write, and the status
 // value of an error answer.
 const (
@@ -41,10 +53,13 @@ const (
 	jsonStatusError = "error"
 )
 
-// Error messages an answer sends in place of the error that caused it.
+// Error messages an answer sends, in place of the error behind it where there
+// is one.
 const (
 	invalidPrefixMessage   = "Invalid prefix"
 	hostNotResolvedMessage = "could not resolve host name"
+	hostNotPublicMessage   = "host name has no public address"
+	tooManyTargetsMessage  = "Too many addresses or host names"
 )
 
 // IP versions as database.LiveRoute.IPVersion records them.
@@ -678,17 +693,23 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 		response := IPLookupResponse{Query: target, Results: make([]*database.IPInfo, 0)}
 
 		// Collect all IPs to look up
-		ipsToLookup, resolveErrors := s.resolveLookupTargets(ctx, target)
+		ipsToLookup, resolveErrors, err := s.resolveLookupTargets(ctx, target)
 		if deadlinePassed(ctx) {
 			s.writeJSONError(w, http.StatusGatewayTimeout, requestTimeoutMessage)
 
 			return
 		}
 
+		if err != nil {
+			s.writeJSONError(w, http.StatusBadRequest, tooManyTargetsMessage)
+
+			return
+		}
+
 		response.Errors = append(response.Errors, resolveErrors...)
 
 		if len(ipsToLookup) == 0 {
-			s.writeJSONError(w, http.StatusBadRequest, "No valid IPs or hostnames provided")
+			s.writeJSONError(w, http.StatusBadRequest, noTargetsMessage(resolveErrors))
 
 			return
 		}
@@ -718,7 +739,7 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 			}
 
 			// Do PTR lookup for this IP
-			ptrs := lookupPTRNames(ctx, ip)
+			ptrs := s.lookupPTRNames(ctx, ip)
 			if deadlinePassed(ctx) {
 				s.writeJSONError(w, http.StatusGatewayTimeout, requestTimeoutMessage)
 
@@ -739,17 +760,37 @@ func (s *Server) handleIPInfo() http.HandlerFunc {
 		// Queue WHOIS refresh for stale ASNs (non-blocking)
 		s.queueWHOISRefresh(refreshASNs)
 
-		// Return response (even if no results, include errors)
-		if len(response.Results) == 0 && len(response.Errors) > 0 {
-			s.writeJSONError(w, http.StatusNotFound, "No routes found: "+response.Errors[0])
+		s.writeIPLookupAnswer(w, response)
+	}
+}
 
-			return
-		}
+// noTargetsMessage returns the message of the 400 an IP lookup answers when
+// no address is left to look up, followed by the first of resolveErrors, which
+// says why, when there is one.
+func noTargetsMessage(resolveErrors []string) string {
+	const message = "No valid IPs or hostnames provided"
 
-		err := writeJSONSuccess(w, response)
-		if err != nil {
-			s.logger.Error("Failed to encode IP lookup response", "error", err)
-		}
+	if len(resolveErrors) == 0 {
+		return message
+	}
+
+	return message + ": " + resolveErrors[0]
+}
+
+// writeIPLookupAnswer sends response, or 404 with its first error when no
+// address had a route.
+func (s *Server) writeIPLookupAnswer(
+	w http.ResponseWriter, response IPLookupResponse,
+) {
+	if len(response.Results) == 0 && len(response.Errors) > 0 {
+		s.writeJSONError(w, http.StatusNotFound, "No routes found: "+response.Errors[0])
+
+		return
+	}
+
+	err := writeJSONSuccess(w, response)
+	if err != nil {
+		s.logger.Error("Failed to encode IP lookup response", "error", err)
 	}
 }
 
@@ -781,15 +822,16 @@ func (s *Server) queueWHOISRefresh(asns map[int]bool) {
 	}
 }
 
-// resolveLookupTargets splits a comma-separated list of IP addresses and
-// hostnames and returns the IP addresses to look up, resolving each hostname.
-// A hostname that does not resolve is reported in the returned errors instead,
-// with the resolver's own error only in the log, since it names the DNS server.
-// It stops once ctx's deadline has passed; the caller then answers 504.
-func (s *Server) resolveLookupTargets(
-	ctx context.Context, target string,
-) ([]string, []string) {
-	var ipsToLookup, resolveErrors []string
+// errTooManyTargets is what resolveLookupTargets returns for a list of more
+// than maxLookupTargets addresses and host names, or of names that resolve to
+// more than maxLookupTargets addresses.
+var errTooManyTargets = errors.New("too many addresses or host names")
+
+// splitLookupTargets returns the addresses and host names in a comma-separated
+// list, leaving out blank entries, or errTooManyTargets when there are more
+// than maxLookupTargets.
+func splitLookupTargets(target string) ([]string, error) {
+	var targets []string
 
 	for t := range strings.SplitSeq(target, ",") {
 		t = strings.TrimSpace(t)
@@ -797,6 +839,36 @@ func (s *Server) resolveLookupTargets(
 			continue
 		}
 
+		if len(targets) == maxLookupTargets {
+			return nil, errTooManyTargets
+		}
+
+		targets = append(targets, t)
+	}
+
+	return targets, nil
+}
+
+// resolveLookupTargets splits a comma-separated list of IP addresses and
+// hostnames and returns the IP addresses to look up, resolving each hostname.
+// A hostname that does not resolve is reported in the returned errors instead,
+// with the resolver's own error only in the log, since it names the DNS server.
+// Only the public addresses a hostname resolves to are kept, so the answer
+// does not show the deployment's private addresses; a hostname with none is
+// reported in the errors. More than maxLookupTargets entries, or addresses once
+// resolved, return errTooManyTargets. It stops once ctx's deadline has passed;
+// the caller then answers 504.
+func (s *Server) resolveLookupTargets(
+	ctx context.Context, target string,
+) ([]string, []string, error) {
+	targets, err := splitLookupTargets(target)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	var ipsToLookup, resolveErrors []string
+
+	for _, t := range targets {
 		// Check if this target is an IP address
 		if net.ParseIP(t) != nil {
 			ipsToLookup = append(ipsToLookup, t)
@@ -805,9 +877,9 @@ func (s *Server) resolveLookupTargets(
 		}
 
 		// It's a hostname - resolve it
-		resolved, err := net.DefaultResolver.LookupHost(ctx, t)
+		resolved, err := s.resolver.LookupHost(ctx, t)
 		if deadlinePassed(ctx) {
-			return nil, nil
+			return nil, nil, nil
 		}
 
 		if err != nil {
@@ -817,16 +889,45 @@ func (s *Server) resolveLookupTargets(
 			continue
 		}
 
-		ipsToLookup = append(ipsToLookup, resolved...)
+		public := publicAddresses(resolved)
+		if len(public) == 0 {
+			resolveErrors = append(resolveErrors, t+": "+hostNotPublicMessage)
+
+			continue
+		}
+
+		ipsToLookup = append(ipsToLookup, public...)
 	}
 
-	return ipsToLookup, resolveErrors
+	if len(ipsToLookup) > maxLookupTargets {
+		return nil, nil, errTooManyTargets
+	}
+
+	return ipsToLookup, resolveErrors, nil
+}
+
+// publicAddresses returns the addresses in addrs that are global unicast and
+// not private; Go counts the private ranges as global unicast.
+func publicAddresses(addrs []string) []string {
+	var public []string
+
+	for _, a := range addrs {
+		ip, err := netip.ParseAddr(a)
+		if err == nil && ip.IsGlobalUnicast() && !ip.IsPrivate() {
+			public = append(public, a)
+		}
+	}
+
+	return public
 }
 
 // lookupPTRNames returns the PTR records of ip without their trailing dots, or
-// nil when the lookup fails.
-func lookupPTRNames(ctx context.Context, ip string) []string {
-	ptrs, err := net.DefaultResolver.LookupAddr(ctx, ip)
+// nil when the lookup fails or runs past ptrLookupTimeout.
+func (s *Server) lookupPTRNames(ctx context.Context, ip string) []string {
+	ctx, cancel := context.WithTimeout(ctx, ptrLookupTimeout)
+	defer cancel()
+
+	ptrs, err := s.resolver.LookupAddr(ctx, ip)
 	if err != nil {
 		return nil
 	}

internal/server/handlers_internal_test.go

@@ -9,7 +9,9 @@ import (
 	"net/http/httptest"
 	"runtime"
 	"slices"
+	"strconv"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -917,6 +919,253 @@ func TestIPLookupAnswers500WhenDatabaseReadFails(t *testing.T) {
 	}
 }
 
+// errNoSuchHost is what fakeResolver answers a lookup it has no answer for
+// with.
+var errNoSuchHost = errors.New("no such host")
+
+// fakeResolver answers a host name lookup with the addresses hosts lists for
+// the name, and a PTR lookup with no names or, when ptrNeverAnswers is set,
+// not at all: it waits until the lookup's context ends. hostLookups counts the
+// host name lookups.
+type fakeResolver struct {
+	hosts           map[string][]string
+	ptrNeverAnswers bool
+
+	hostLookups atomic.Int64
+}
+
+func (r *fakeResolver) LookupHost(_ context.Context, host string) ([]string, error) {
+	r.hostLookups.Add(1)
+
+	addrs, ok := r.hosts[host]
+	if !ok {
+		return nil, errNoSuchHost
+	}
+
+	return addrs, nil
+}
+
+func (r *fakeResolver) LookupAddr(ctx context.Context, _ string) ([]string, error) {
+	if r.ptrNeverAnswers {
+		<-ctx.Done()
+
+		return nil, ctx.Err()
+	}
+
+	return nil, errNoSuchHost
+}
+
+// countingIPLookupDB embeds database.Store (left nil) and counts its IP
+// lookups, answering each that the address has no route. Every other Store
+// method is unused and would panic if called.
+type countingIPLookupDB struct {
+	database.Store
+
+	lookups atomic.Int64
+}
+
+func (d *countingIPLookupDB) GetIPInfoContext(
+	_ context.Context, _ string,
+) (*database.IPInfo, error) {
+	d.lookups.Add(1)
+
+	return nil, database.ErrNoRoute
+}
+
+// TestIPLookupRefusesTooManyAddresses sends IP lookups of maxLookupTargets
+// addresses and of one more, listed or as a host name that resolves to them,
+// and a lookup listing maxLookupTargets+1 host names. The longer lookups answer
+// 400 without reading the database, and the list of host names without
+// resolving any of them; the others read the database once per address. Before
+// the fix every address was read and every name resolved, however many there
+// were (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupRefusesTooManyAddresses(t *testing.T) {
+	t.Parallel()
+
+	addrs := make([]string, maxLookupTargets+1)
+	names := make([]string, maxLookupTargets+1)
+
+	for i := range addrs {
+		addrs[i] = "192.0.2." + strconv.Itoa(i+1)
+		names[i] = "host" + strconv.Itoa(i+1) + ".test"
+	}
+
+	hosts := map[string][]string{
+		"most.test":     addrs[:maxLookupTargets],
+		"too-many.test": addrs,
+	}
+
+	tests := []struct {
+		target          string
+		wantCode        int
+		wantLookups     int64
+		wantHostLookups int64
+	}{
+		{
+			strings.Join(addrs[:maxLookupTargets], ","),
+			http.StatusNotFound, maxLookupTargets, 0,
+		},
+		{strings.Join(addrs, ","), http.StatusBadRequest, 0, 0},
+		{"most.test", http.StatusNotFound, maxLookupTargets, 1},
+		{"too-many.test", http.StatusBadRequest, 0, 1},
+		{strings.Join(names, ","), http.StatusBadRequest, 0, 0},
+	}
+
+	for _, tt := range tests {
+		db := &countingIPLookupDB{}
+		s := New(db, streamer.New(logger.New(), metrics.New()), logger.New(),
+			&config.Config{})
+		resolver := &fakeResolver{hosts: hosts}
+		s.resolver = resolver
+
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, "/api/v1/ip/"+tt.target, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				tt.target, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		checkJSONErrorCode(t, tt.target, rec, tt.wantCode)
+
+		got := db.lookups.Load()
+		if got != tt.wantLookups {
+			t.Errorf("%s: %d database reads, want %d",
+				tt.target, got, tt.wantLookups)
+		}
+
+		gotHostLookups := resolver.hostLookups.Load()
+		if gotHostLookups != tt.wantHostLookups {
+			t.Errorf("%s: %d host name lookups, want %d",
+				tt.target, gotHostLookups, tt.wantHostLookups)
+		}
+	}
+}
+
+// addrInTestPrefixV4 is an address in testPrefixV4, which
+// newServerWithTestRoutes stores a route for.
+const addrInTestPrefixV4 = "192.0.2.1"
+
+// TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer looks up a routed address
+// with a resolver that never answers its PTR lookup. Once the PTR lookup's own
+// limit passes, the lookup answers 200 with the route. Before the fix the PTR
+// lookup ran until the request's time limit, and the lookup answered 504
+// without the route (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupKeepsRouteWhenPTRLookupDoesNotAnswer(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+	s.resolver = &fakeResolver{ptrNeverAnswers: true}
+
+	// The request's time limit passes after the PTR lookup's.
+	ctx, cancel := context.WithTimeout(t.Context(), 2*ptrLookupTimeout)
+	defer cancel()
+
+	rec := httptest.NewRecorder()
+	s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+		ctx, http.MethodGet, "/api/v1/ip/"+addrInTestPrefixV4, nil))
+
+	if rec.Code != http.StatusOK {
+		t.Fatalf("status %d, want %d; body %s", rec.Code, http.StatusOK, rec.Body)
+	}
+
+	var body struct {
+		Data IPLookupResponse `json:"data"`
+	}
+
+	err := json.Unmarshal(rec.Body.Bytes(), &body)
+	if err != nil {
+		t.Fatalf("decoding the answer %q: %v", rec.Body, err)
+	}
+
+	if len(body.Data.Results) != 1 || body.Data.Results[0].Netblock != testPrefixV4 {
+		t.Errorf("answer %s, want one result in %s", rec.Body, testPrefixV4)
+	}
+}
+
+// The host name TestIPLookupHidesPrivateAddresses resolves to a private
+// address, and that address.
+const (
+	internalHostName = "internal.test"
+	internalHostAddr = "10.0.0.5"
+)
+
+// TestIPLookupHidesPrivateAddresses looks up host names that resolve to a
+// private address, alone or beside a routed address. No answer carries the
+// private address, and a name with no other address is reported as having no
+// public address, in the 400 when nothing else is left to look up. Before the
+// fix the private address was looked up and named in the answer, "No routes
+// found: 10.0.0.5: no route found for IP 10.0.0.5", so the API showed what the
+// deployment's own DNS knows (https://git.eeqj.de/sneak/routewatch/issues/110).
+func TestIPLookupHidesPrivateAddresses(t *testing.T) {
+	t.Parallel()
+
+	s := newServerWithTestRoutes(t)
+	s.resolver = &fakeResolver{hosts: map[string][]string{
+		internalHostName: {internalHostAddr},
+		"mixed.test":     {internalHostAddr, addrInTestPrefixV4},
+	}}
+
+	notPublic := internalHostName + ": " + hostNotPublicMessage
+
+	tests := []struct {
+		target      string
+		wantCode    int
+		wantMessage string
+		wantErrors  []string
+	}{
+		{
+			internalHostName, http.StatusBadRequest,
+			"No valid IPs or hostnames provided: " + notPublic, nil,
+		},
+		{
+			internalHostName + "," + addrInTestPrefixV4, http.StatusOK, "",
+			[]string{notPublic},
+		},
+		{"mixed.test", http.StatusOK, "", nil},
+	}
+
+	for _, tt := range tests {
+		rec := httptest.NewRecorder()
+		s.router.ServeHTTP(rec, httptest.NewRequestWithContext(
+			t.Context(), http.MethodGet, "/api/v1/ip/"+tt.target, nil))
+
+		if rec.Code != tt.wantCode {
+			t.Errorf("%s: status %d, want %d; body %s",
+				tt.target, rec.Code, tt.wantCode, rec.Body)
+		}
+
+		if strings.Contains(rec.Body.String(), internalHostAddr) {
+			t.Errorf("%s: body %s carries %s", tt.target, rec.Body, internalHostAddr)
+		}
+
+		var body struct {
+			Data  IPLookupResponse `json:"data"`
+			Error struct {
+				Msg string `json:"msg"`
+			} `json:"error"`
+		}
+
+		err := json.Unmarshal(rec.Body.Bytes(), &body)
+		if err != nil {
+			t.Errorf("%s: decoding the answer %q: %v", tt.target, rec.Body, err)
+
+			continue
+		}
+
+		if body.Error.Msg != tt.wantMessage {
+			t.Errorf("%s: error message %q, want %q",
+				tt.target, body.Error.Msg, tt.wantMessage)
+		}
+
+		if !slices.Equal(body.Data.Errors, tt.wantErrors) {
+			t.Errorf("%s: errors %q, want %q",
+				tt.target, body.Data.Errors, tt.wantErrors)
+		}
+	}
+}
+
 // settledGoroutineCount lets transient goroutines finish, then reports the
 // current count.
 func settledGoroutineCount() int {

internal/server/server.go

@@ -32,6 +32,13 @@ type ASNFetcher interface {
 	GetStats() ASNFetcherStats
 }
 
+// dnsResolver looks up the addresses of host names and the names of addresses
+// in DNS, as *net.Resolver does.
+type dnsResolver interface {
+	LookupHost(ctx context.Context, host string) ([]string, error)
+	LookupAddr(ctx context.Context, addr string) ([]string, error)
+}
+
 // Server provides HTTP endpoints for status monitoring
 type Server struct {
 	router     *chi.Mux
@@ -45,6 +52,10 @@ type Server struct {
 	// trustedProxies are the reverse proxies whose X-Forwarded-For header
 	// RealIPMiddleware believes
 	trustedProxies []netip.Prefix
+
+	// resolver makes the IP lookup's DNS lookups: net.DefaultResolver, or a
+	// fake in tests
+	resolver dnsResolver
 }
 
 // New creates a new HTTP server
@@ -60,6 +71,7 @@ func New(
 		logger:         logger,
 		port:           cfg.Port,
 		trustedProxies: cfg.TrustedProxies,
+		resolver:       net.DefaultResolver,
 	}
 
 	s.setupRoutes()